On Thu, 2010-07-15 at 13:37 -0500, Nathan Fontenot wrote:
> @@ -123,13 +130,20 @@
> static ssize_t show_mem_removable(struct sys_device *dev,
> struct sysdev_attribute *attr, char *buf)
> {
> + struct memory_block *mem;
> + struct memory_block_section *mbs;
> unsigned long start_pfn;
> - int ret;
> - struct memory_block *mem =
> - container_of(dev, struct memory_block, sysdev);
> + int ret = 1;
> +
> + mem = container_of(dev, struct memory_block, sysdev);
> + mutex_lock(&mem->state_mutex);
>
> - start_pfn = section_nr_to_pfn(mem->phys_index);
> - ret = is_mem_section_removable(start_pfn, PAGES_PER_SECTION);
> + list_for_each_entry(mbs, &mem->sections, next) {
> + start_pfn = section_nr_to_pfn(mbs->phys_index);
> + ret &= is_mem_section_removable(start_pfn, PAGES_PER_SECTION);
> + }
> +
> + mutex_unlock(&mem->state_mutex);
> return sprintf(buf, "%d\n", ret);
> }
Now that the "state_mutex" is getting used for other stuff, should we
just make it "mutex"?
> @@ -182,16 +196,16 @@
> * OK to have direct references to sparsemem variables in here.
> */
> static int
> -memory_block_action(struct memory_block *mem, unsigned long action)
> +memory_block_action(struct memory_block_section *mbs, unsigned long action)
> {
> int i;
> unsigned long psection;
> unsigned long start_pfn, start_paddr;
> struct page *first_page;
> int ret;
> - int old_state = mem->state;
> + int old_state = mbs->state;
>
> - psection = mem->phys_index;
> + psection = mbs->phys_index;
> first_page = pfn_to_page(psection << PFN_SECTION_SHIFT);
>
> /*
> @@ -217,18 +231,18 @@
> ret = online_pages(start_pfn, PAGES_PER_SECTION);
> break;
> case MEM_OFFLINE:
> - mem->state = MEM_GOING_OFFLINE;
> + mbs->state = MEM_GOING_OFFLINE;
> start_paddr = page_to_pfn(first_page) << PAGE_SHIFT;
> ret = remove_memory(start_paddr,
> PAGES_PER_SECTION << PAGE_SHIFT);
> if (ret) {
> - mem->state = old_state;
> + mbs->state = old_state;
> break;
> }
> break;
> default:
> WARN(1, KERN_WARNING "%s(%p, %ld) unknown action:
> %ld\n",
> - __func__, mem, action, action);
> + __func__, mbs, action, action);
> ret = -EINVAL;
> }
>
> @@ -238,19 +252,34 @@
> static int memory_block_change_state(struct memory_block *mem,
> unsigned long to_state, unsigned long from_state_req)
> {
> + struct memory_block_section *mbs;
> int ret = 0;
> +
> mutex_lock(&mem->state_mutex);
>
> - if (mem->state != from_state_req) {
> - ret = -EINVAL;
> - goto out;
> + list_for_each_entry(mbs, &mem->sections, next) {
> + if (mbs->state != from_state_req)
> + continue;
> +
> + ret = memory_block_action(mbs, to_state);
> + if (ret)
> + break;
> + }
> +
> + if (ret) {
> + list_for_each_entry(mbs, &mem->sections, next) {
> + if (mbs->state == from_state_req)
> + continue;
> +
> + if (memory_block_action(mbs, to_state))
> + printk(KERN_ERR "Could not re-enable memory "
> + "section %lx\n", mbs->phys_index);
> + }
> }
Please just use a goto here. It's nicer looking, and much more in line
with what's there already.
...
> ===================================================================
> --- linux-2.6.orig/include/linux/memory.h 2010-07-15 08:48:41.000000000
> -0500
> +++ linux-2.6/include/linux/memory.h 2010-07-15 09:54:06.000000000 -0500
> @@ -19,9 +19,15 @@
> #include <linux/node.h>
> #include <linux/compiler.h>
> #include <linux/mutex.h>
> +#include <linux/list.h>
>
> -struct memory_block {
> +struct memory_block_section {
> + unsigned long state;
> unsigned long phys_index;
> + struct list_head next;
> +};
> +
> +struct memory_block {
> unsigned long state;
> /*
> * This serializes all state change requests. It isn't
> @@ -34,6 +40,7 @@
> void *hw; /* optional pointer to fw/hw data */
> int (*phys_callback)(struct memory_block *);
> struct sys_device sysdev;
> + struct list_head sections;
> };
It looks like we have state in both the memory_block and
memory_block_section. That seems a bit confusing to me. This also
looks like it would permit non-contiguous memory_block_sections in a
memory_block. Is that what you intended?
If the memory_block's state was inferred to be the same as each
memory_block_section, couldn't we just keep a start and end phys_index
in the memory_block, and get away from having memory_block_sections at
all?
-- Dave
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
