[PATCH 4/5 v3] kernel handling of memory DLPAR

[Michael Ellerman]

On Tue, 2009-10-13 at 13:13 -0500, Nathan Fontenot wrote:
> This adds the capability to DLPAR add and remove memory from the kernel.  The

Hi Nathan,

Sorry to only get around to reviewing version 3, time is a commodity in
short supply :)

> Index: powerpc/arch/powerpc/platforms/pseries/dlpar.c
> ===================================================================
> --- powerpc.orig/arch/powerpc/platforms/pseries/dlpar.c	2009-10-08 11:08:42.000000000 -0500
> +++ powerpc/arch/powerpc/platforms/pseries/dlpar.c	2009-10-13 13:08:22.000000000 -0500
> @@ -16,6 +16,10 @@
>  #include <linux/notifier.h>
>  #include <linux/proc_fs.h>
>  #include <linux/spinlock.h>
> +#include <linux/memory_hotplug.h>
> +#include <linux/sysdev.h>
> +#include <linux/sysfs.h>
> +
>  
>  #include <asm/prom.h>
>  #include <asm/machdep.h>
> @@ -404,11 +408,165 @@
>  	return 0;
>  }
>  
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +
> +static struct property *clone_property(struct property *old_prop)
> +{
> +	struct property *new_prop;
> +
> +	new_prop = kzalloc((sizeof *new_prop), GFP_KERNEL);
> +	if (!new_prop)
> +		return NULL;
> +
> +	new_prop->name = kzalloc(strlen(old_prop->name) + 1, GFP_KERNEL);

kstrdup()?

> +	new_prop->value = kzalloc(old_prop->length + 1, GFP_KERNEL);
> +	if (!new_prop->name || !new_prop->value) {
> +		free_property(new_prop);
> +		return NULL;
> +	}
> +
> +	strcpy(new_prop->name, old_prop->name);
> +	memcpy(new_prop->value, old_prop->value, old_prop->length);
> +	new_prop->length = old_prop->length;
> +
> +	return new_prop;
> +}
> +
> +int platform_probe_memory(u64 phys_addr)
> +{
> +	struct device_node *dn;
> +	struct property *new_prop, *old_prop;
> +	struct property *lmb_sz_prop;
> +	struct of_drconf_cell *drmem;
> +	u64 lmb_size;
> +	int num_entries, i, rc;
> +
> +	if (!phys_addr)
> +		return -EINVAL;
> +
> +	dn = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
> +	if (!dn)
> +		return -EINVAL;
> +
> +	lmb_sz_prop = of_find_property(dn, "ibm,lmb-size", NULL);
> +	lmb_size = *(u64 *)lmb_sz_prop->value;

of_get_property() ?
> +
> +	old_prop = of_find_property(dn, "ibm,dynamic-memory", NULL);

I know we should never fail to find these properties, but it would be
nice to check just in case.

> +
> +	num_entries = *(u32 *)old_prop->value;
> +	drmem = (struct of_drconf_cell *)
> +				((char *)old_prop->value + sizeof(u32));

You do this dance twice (see below), a struct might make it cleaner.

> +	for (i = 0; i < num_entries; i++) {
> +		u64 lmb_end_addr = drmem[i].base_addr + lmb_size;
> +		if (phys_addr >= drmem[i].base_addr
> +		    && phys_addr < lmb_end_addr)
> +			break;
> +	}
> +
> +	if (i >= num_entries) {
> +		of_node_put(dn);
> +		return -EINVAL;
> +	}
> +
> +	if (drmem[i].flags & DRCONF_MEM_ASSIGNED) {
> +		of_node_put(dn);
> +		return 0;

This is the already added case?

> +	}
> +
> +	rc = acquire_drc(drmem[i].drc_index);
> +	if (rc) {
> +		of_node_put(dn);
> +		return -1;

-1 ?

> +	}
> +
> +	new_prop = clone_property(old_prop);
> +	drmem = (struct of_drconf_cell *)
> +				((char *)new_prop->value + sizeof(u32));
> +
> +	drmem[i].flags |= DRCONF_MEM_ASSIGNED;
> +	prom_update_property(dn, new_prop, old_prop);
> +
> +	rc = blocking_notifier_call_chain(&pSeries_reconfig_chain,
> +					  PSERIES_DRCONF_MEM_ADD,
> +					  &drmem[i].base_addr);
> +	if (rc == NOTIFY_BAD) {
> +		prom_update_property(dn, old_prop, new_prop);
> +		release_drc(drmem[i].drc_index);
> +	}
> +
> +	of_node_put(dn);
> +	return rc == NOTIFY_BAD ? -1 : 0;

-1 ?

> +}
> +
> +static ssize_t memory_release_store(struct class *class, const char *buf,
> +				    size_t count)
> +{
> +	unsigned long drc_index;
> +	struct device_node *dn;
> +	struct property *new_prop, *old_prop;
> +	struct of_drconf_cell *drmem;
> +	int num_entries;
> +	int i, rc;
> +
> +	rc = strict_strtoul(buf, 0, &drc_index);
> +	if (rc)
> +		return -EINVAL;
> +
> +	dn = of_find_node_by_path("/ibm,dynamic-reconfiguration-memory");
> +	if (!dn)
> +		return 0;

0 really?

> +
> +	old_prop = of_find_property(dn, "ibm,dynamic-memory", NULL);
> +	new_prop = clone_property(old_prop);
> +
> +	num_entries = *(u32 *)new_prop->value;
> +	drmem = (struct of_drconf_cell *)
> +				((char *)new_prop->value + sizeof(u32));
> +
> +	for (i = 0; i < num_entries; i++) {
> +		if (drmem[i].drc_index == drc_index)
> +			break;
> +	}
> +
> +	if (i >= num_entries) {
> +		free_property(new_prop);
> +		of_node_put(dn);
> +		return -EINVAL;
> +	}

Couldn't use old_prop up until here? They're identical aren't they, so
you can do the clone here and you can avoid the free in the above error
case.

> +	drmem[i].flags &= ~DRCONF_MEM_ASSIGNED;
> +	prom_update_property(dn, new_prop, old_prop);
> +
> +	rc = blocking_notifier_call_chain(&pSeries_reconfig_chain,
> +					  PSERIES_DRCONF_MEM_REMOVE,
> +					  &drmem[i].base_addr);
> +	if (rc != NOTIFY_BAD)
> +		rc = release_drc(drc_index);
> +
> +	if (rc)
> +		prom_update_property(dn, old_prop, new_prop);
> +
> +	of_node_put(dn);
> +	return rc ? -1 : count;

-1, EPERM?

> +}
> +
> +static struct class_attribute class_attr_mem_release =
> +			__ATTR(release, S_IWUSR, NULL, memory_release_store);
> +#endif
> +
>  static int pseries_dlpar_init(void)
>  {
>  	if (!machine_is(pseries))
>  		return 0;
>  
> +#ifdef CONFIG_MEMORY_HOTPLUG
> +	if (sysfs_create_file(&memory_sysdev_class.kset.kobj,
> +			      &class_attr_mem_release.attr))
> +		printk(KERN_INFO "DLPAR: Could not create sysfs memory "
> +		       "release file\n");
> +#endif
> +
>  	return 0;
>  }
>  device_initcall(pseries_dlpar_init);
> Index: powerpc/arch/powerpc/mm/mem.c
> ===================================================================
> --- powerpc.orig/arch/powerpc/mm/mem.c	2009-10-08 11:07:45.000000000 -0500
> +++ powerpc/arch/powerpc/mm/mem.c	2009-10-08 11:08:54.000000000 -0500
> @@ -111,8 +111,19 @@
>  #ifdef CONFIG_MEMORY_HOTPLUG
>  
>  #ifdef CONFIG_NUMA
> +int __attribute ((weak)) platform_probe_memory(u64 start)

__weak

Though be careful, I think this is vulnerable to a bug in some
toolchains where the compiler will inline this version. See the comment
around early_irq_init() in kernel/softirq.c for example.

This will need to be a ppc_md hook as soon as another platform supports
memory hotplug, though that may be never :)

> +{
> +	return 0;
> +}
> +
>  int memory_add_physaddr_to_nid(u64 start)
>  {
> +	int rc;
> +
> +	rc = platform_probe_memory(start);
> +	if (rc)
> +		return rc;
> +
>  	return hot_add_scn_to_nid(start);
>  }
>  #endif

cheers

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/linuxppc-dev/attachments/20091014/93649afc/attachment.pgp>