[PATCH 2.6.21-rc1] ibmebus: Support dynamic addition and removal of adapters

[PATCH 2.6.21-rc1] ibmebus: Support dynamic addition and removal of adapters

[Hoang-Nam Nguyen]

This patch will add two sysfs attributes to /sys/bus/ibmebus which can be used
to notify the ebus driver of added / removed ebus devices in the OF device
tree.

Echoing the device's location code (as found in the OFDT "ibm,loc-code"
property) into the "probe" attribute will notify ebus of addition of the device
and cause the appropriate device driver's probe function to be called on the
device.

Likewise, echoing the location code into the "remove" attribute will cause
the device to be removed from the system.

Additionally, the uevent interface is now implemented in the driver.


Signed-off-by: Joachim Fenkes <fenkes@de.ibm.com>
---


 arch/powerpc/kernel/ibmebus.c |  121 +++++++++++++++++++++++++++++++++++++++---
 include/asm-powerpc/ibmebus.h |    2 
 2 files changed, 114 insertions(+), 9 deletions(-)


diff -wurp linux-2.6.20/arch/powerpc/kernel/ibmebus.c linux-2.6.20-ebus/arch/powerpc/kernel/ibmebus.c
--- linux-2.6.20/arch/powerpc/kernel/ibmebus.c	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20-ebus/arch/powerpc/kernel/ibmebus.c	2007-02-14 17:58:00.000000000 +0100
@@ -2,6 +2,7 @@
  * IBM PowerPC IBM eBus Infrastructure Support.
  *
  * Copyright (c) 2005 IBM Corporation
+ *  Joachim Fenkes <fenkes@de.ibm.com>
  *  Heiko J Schick <schickhj@de.ibm.com>
  *    
  * All rights reserved.
@@ -44,7 +45,6 @@
 #include <asm/abs_addr.h>
 
 static struct ibmebus_dev ibmebus_bus_device = { /* fake "parent" device */
-	.name = ibmebus_bus_device.ofdev.dev.bus_id,
 	.ofdev.dev.bus_id = "ibmebus",
 	.ofdev.dev.bus    = &ibmebus_bus_type,
 };
@@ -161,7 +161,9 @@ static void __devinit ibmebus_dev_releas
 static ssize_t ibmebusdev_show_name(struct device *dev, 
 				    struct device_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%s\n", to_ibmebus_dev(dev)->name);
+	struct ibmebus_dev *ebus_dev = to_ibmebus_dev(dev);
+	char *name = (char*)get_property(ebus_dev->ofdev.node, "name", NULL);
+	return sprintf(buf, "%s\n", name);
 }
 static DEVICE_ATTR(name, S_IRUSR | S_IRGRP | S_IROTH, ibmebusdev_show_name, 
 		   NULL);
@@ -171,7 +173,6 @@ static struct ibmebus_dev* __devinit ibm
 {
 	int err = 0;
 
-	dev->name = name;
 	dev->ofdev.dev.parent  = &ibmebus_bus_device.ofdev.dev;
 	dev->ofdev.dev.bus     = &ibmebus_bus_type;
 	dev->ofdev.dev.release = ibmebus_dev_release;
@@ -262,11 +263,19 @@ static void ibmebus_add_devices_by_id(st
 	return;
 }
 
-static int ibmebus_match_helper(struct device *dev, void *data)
+static int ibmebus_match_helper_name(struct device *dev, void *data)
 {
-	if (strcmp((char*)data, to_ibmebus_dev(dev)->name) == 0)
-		return 1;
+	const struct ibmebus_dev *ebus_dev = to_ibmebus_dev(dev);
+	char *name;
 	
+	/* parent device has no of_device node, so skip it */
+	if (ebus_dev != &ibmebus_bus_device) {
+		name = (char*)get_property(
+			ebus_dev->ofdev.node, "name", NULL);
+	
+		if (name && (strcmp((char*)data, name) == 0))
+			return 1;
+	}
 	return 0;
 }
 
@@ -285,11 +294,10 @@ static void ibmebus_remove_devices_by_id
 	while (strlen(idt->name) > 0) {
 		while ((dev = bus_find_device(&ibmebus_bus_type, NULL, 
 					      (void*)idt->name,
-					      ibmebus_match_helper))) {
+					      ibmebus_match_helper_name))) {
 			ibmebus_unregister_device(dev);
 		}
 		idt++;
-		
 	}
 	
 	return;
@@ -307,6 +315,9 @@ int ibmebus_register_driver(struct ibmeb
 	if ((err = driver_register(&drv->driver) != 0))
 		return err;
 
+	/* remove all supported devices first, in case someone
+	 * probed them manually before registering the driver */
+	ibmebus_remove_devices_by_id(drv->id_table);
 	ibmebus_add_devices_by_id(drv->id_table);
 	
 	return 0;
@@ -361,12 +372,101 @@ static int ibmebus_bus_match(struct devi
 	return 0;
 }
 
+static int ibmebus_uevent(struct device *dev, char **envp, int num_envp,
+			char *buffer, int buffer_size)
+{
+	const struct ibmebus_dev *ebus_dev = to_ibmebus_dev(dev);
+	char *name, *cp, *loc_code;
+	int length;
+
+	if (!num_envp)
+		return -ENOMEM;
+
+	if (!ebus_dev->ofdev.node)
+		return -ENODEV;
+
+	name = (char *)get_property(ebus_dev->ofdev.node, "name", NULL);
+	cp = (char *)get_property(ebus_dev->ofdev.node, "compatible", NULL);
+	loc_code = (char *)get_property(ebus_dev->ofdev.node,
+					"ibm,loc-code", NULL);
+	if (!(name && cp && loc_code))
+		return -ENODEV;
+
+	envp[0] = buffer;
+	length = scnprintf(buffer, buffer_size,
+			   "MODALIAS=ibmebus:T%s:S%s:L%s",
+			   name, cp, loc_code);
+	if (buffer_size - length <= 0)
+		return -ENOMEM;
+	envp[1] = NULL;
+
+	return 0;
+}
+
 struct bus_type ibmebus_bus_type = {
 	.name = "ibmebus",
+	.uevent = ibmebus_uevent,
 	.match = ibmebus_bus_match,
 };
 EXPORT_SYMBOL(ibmebus_bus_type);
 
+static int ibmebus_match_helper_loc_code(struct device *dev, void *data)
+{
+	const struct ibmebus_dev *ebus_dev = to_ibmebus_dev(dev);
+	char *loc_code;
+
+	/* parent device has no of_device node, so skip it */
+	if (ebus_dev != &ibmebus_bus_device) {
+		loc_code = (char*)get_property(
+			ebus_dev->ofdev.node, "ibm,loc-code", NULL);
+
+		if (loc_code && (strcmp((char*)data, loc_code) == 0))
+			return 1;
+	}
+	return 0;
+}
+
+static ssize_t ibmebus_store_probe(struct bus_type *dev,
+				   const char *buf, size_t count)
+{
+	struct device_node *dn = NULL;
+	char *loc_code;
+
+	if (bus_find_device(&ibmebus_bus_type, NULL, (char*)buf,
+			     ibmebus_match_helper_loc_code)) {
+		printk(KERN_WARNING "%s: loc_code %s has already been probed\n",
+		       __FUNCTION__, buf);
+		return count;
+	}
+
+	while ((dn = of_find_all_nodes(dn))) {
+		loc_code = (char *)get_property(dn, "ibm,loc-code", NULL);
+		if (loc_code && (strncmp(loc_code, buf, count) == 0)) {
+			if (ibmebus_register_device_node(dn) == NULL) {
+				of_node_put(dn);
+				break;
+			}
+		}
+	}
+
+	return count;
+}
+static BUS_ATTR(probe, S_IWUSR, NULL, ibmebus_store_probe);
+
+static ssize_t ibmebus_store_remove(struct bus_type *bus,
+				    const char *buf, size_t count)
+{
+	struct device *dev;
+
+	while ((dev = bus_find_device(&ibmebus_bus_type, NULL, (char*)buf,
+				      ibmebus_match_helper_loc_code))) {
+		ibmebus_unregister_device(dev);
+	}
+
+	return count;
+}
+static BUS_ATTR(remove, S_IWUSR, NULL, ibmebus_store_remove);
+
 static int __init ibmebus_bus_init(void)
 {
 	int err;
@@ -389,6 +489,9 @@ static int __init ibmebus_bus_init(void)
 		return err;
 	}
 	
+	bus_create_file(&ibmebus_bus_type, &bus_attr_probe);
+	bus_create_file(&ibmebus_bus_type, &bus_attr_remove);
+
 	return 0;
 }
 __initcall(ibmebus_bus_init);
diff -wurp linux-2.6.20/include/asm-powerpc/ibmebus.h linux-2.6.20-ebus/include/asm-powerpc/ibmebus.h
--- linux-2.6.20/include/asm-powerpc/ibmebus.h	2007-02-04 19:44:54.000000000 +0100
+++ linux-2.6.20-ebus/include/asm-powerpc/ibmebus.h	2007-02-14 17:58:55.000000000 +0100
@@ -2,6 +2,7 @@
  * IBM PowerPC eBus Infrastructure Support.
  *
  * Copyright (c) 2005 IBM Corporation
+ *  Joachim Fenkes <fenkes@de.ibm.com>
  *  Heiko J Schick <schickhj@de.ibm.com>
  *    
  * All rights reserved.
@@ -47,7 +48,6 @@
 extern struct bus_type ibmebus_bus_type;
 
 struct ibmebus_dev {	
-	const char *name;
 	struct of_device ofdev;
 };
 

Re: [PATCH 2.6.21-rc1] ibmebus: Support dynamic addition and removal of adapters

[John Rose]

Hi-

Looks good.  Questions: how can the user space tools verify the success
of an add or remove?  

Also, will /sys/bus/ibmebus exist even if the system booted with no LHEA
nodes?

One more comment below.

@@ -161,7 +161,9 @@ static void __devinit ibmebus_dev_releas
 static ssize_t ibmebusdev_show_name(struct device *dev, 
 				    struct device_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%s\n", to_ibmebus_dev(dev)->name);
+	struct ibmebus_dev *ebus_dev = to_ibmebus_dev(dev);
+	char *name = (char*)get_property(ebus_dev->ofdev.node, "name", NULL);
+	return sprintf(buf, "%s\n", name);
 }
 static DEVICE_ATTR(name, S_IRUSR | S_IRGRP | S_IROTH, ibmebusdev_show_name, 
 		   NULL);


Can we also have an attribute "devspec" that communicates the open
firmware path through sysfs?  User space DLPAR tools need a way to make
this correlation.  This is consistent with other dynamic-capable sysfs
objects (cpus, etc).  I assume you could get this string with something
like ebus_dev->ofdev.node->full_name.  For example:

# cat /sys/bus/ibmebus/$ebus_device/devspec
/lhea@xxxxxxxxx/

Thanks-
John

Re: [PATCH 2.6.21-rc1] ibmebus: Support dynamic addition and removal of adapters

[Sylvain Munaut]

Hoang-Nam Nguyen wrote:
> Additionally, the uevent interface is now implemented in the driver.
>   
Mmmh, I posted a patch that added a common uevent interface for all
of_device based
bus. And that kinda clash with this one.

I think it's a much cleaner approach to make it as common as possible.
But as
"ibm,loc_code" is local to this bus it's not handled by my code. Is it
necessary
to match the correct driver ?

By the way you generate your uevent string is wrong imho:
 - it doesn't account for the fact that there can be multiple strings in
"compatible".
 - it can be a lot nicer if you use the helper add_uevent_var to fill
the event.
 - you generate a string "ibmebus:T%s:S%s:L%s" but did you just invent that
string ? I see no support for the "ibmebus" marker in mod_devicetable.h, nor
in file2alias.c nor any where else for that matter.


    Sylvain


>  
> +static int ibmebus_uevent(struct device *dev, char **envp, int num_envp,
> +			char *buffer, int buffer_size)
> +{
> +	const struct ibmebus_dev *ebus_dev = to_ibmebus_dev(dev);
> +	char *name, *cp, *loc_code;
> +	int length;
> +
> +	if (!num_envp)
> +		return -ENOMEM;
> +
> +	if (!ebus_dev->ofdev.node)
> +		return -ENODEV;
> +
> +	name = (char *)get_property(ebus_dev->ofdev.node, "name", NULL);
> +	cp = (char *)get_property(ebus_dev->ofdev.node, "compatible", NULL);
> +	loc_code = (char *)get_property(ebus_dev->ofdev.node,
> +					"ibm,loc-code", NULL);
> +	if (!(name && cp && loc_code))
> +		return -ENODEV;
> +
> +	envp[0] = buffer;
> +	length = scnprintf(buffer, buffer_size,
> +			   "MODALIAS=ibmebus:T%s:S%s:L%s",
> +			   name, cp, loc_code);
> +	if (buffer_size - length <= 0)
> +		return -ENOMEM;
> +	envp[1] = NULL;
> +
> +	return 0;
> +}
> +
>   

Re: [PATCH 2.6.21-rc1] ibmebus: Support dynamic addition and removal of adapters

[Joachim Fenkes]

John Rose <johnrose@austin.ibm.com> wrote on 15.02.2007 14:57:37:

> Looks good.  Questions: how can the user space tools verify the success
> of an add or remove? 

If the probe operation succeeds, the respective device will show up 
beneath
/sys/bus/ibmebus/devices.
 
> Also, will /sys/bus/ibmebus exist even if the system booted with no LHEA
> nodes?

Sure.
 
> Can we also have an attribute "devspec" that communicates the open
> firmware path through sysfs?

This is already taken care of by of_device_register().

Cheers,
  Joachim

---
Joachim Fenkes  --  eHCA Linux Driver Developer and Hardware Tamer 
extraordinaire
IBM Deutschland Entwicklung GmbH  --  Dept. 3627 (I/O Firmware Development 
2)
Schoenaicher Strasse 220  --  71032 Boeblingen  --  Germany
eMail: fenkes@de.ibm.com  --  Phone: +49 7031 16 1239 

Vorsitzender des Aufsichtsrats: Johann Weihen  --  Geschäftsführung: 
Herbert Kircher
Sitz der Gesellschaft: Böblingen  --  Registergericht: Amtsgericht 
Stuttgart, HRB 243294

Re: [PATCH 2.6.21-rc1] ibmebus: Support dynamic addition and removal of adapters

[Joachim Fenkes]

Sylvain Munaut <tnt@246tNt.com> wrote on 15.02.2007 15:11:42:

> Mmmh, I posted a patch that added a common uevent interface for all
> of_device based bus. And that kinda clash with this one.

Oops, I was not aware of this. I had a look at your patch and yes, it's a 
nice
and generic interface. I concur that using your generic function is the 
better
approach.

> But as "ibm,loc_code" is local to this bus it's not handled by my code.
> Is it necessary to match the correct driver ?

The intention of adding the location code was to include a device property
into the environment that's unique to each device. Actually, the DEVPATH 
variable
provides exactly this: For ebus devices, it points to the ebus device 
itself,
which natually is unique.

If a userspace tool really needs the location code, the "devspec" 
attribute of
the ebus device will point it to the device tree node, which in turn 
contains the
location code property.

So I think there's nothing preventing me from using your function. Thanks 
for
pointing this out.

Regards,
  Joachim

---
Joachim Fenkes  --  eHCA Linux Driver Developer and Hardware Tamer 
extraordinaire
IBM Deutschland Entwicklung GmbH  --  Dept. 3627 (I/O Firmware Development 
2)
Schoenaicher Strasse 220  --  71032 Boeblingen  --  Germany
eMail: fenkes@de.ibm.com  --  Phone: +49 7031 16 1239 

Vorsitzender des Aufsichtsrats: Johann Weihen  --  Geschäftsführung: 
Herbert Kircher
Sitz der Gesellschaft: Böblingen  --  Registergericht: Amtsgericht 
Stuttgart, HRB 243294

Re: [PATCH 2.6.21-rc1] ibmebus: Support dynamic addition and removal of adapters

[John Rose]

> If the probe operation succeeds, the respective device will show up 
> beneath
> /sys/bus/ibmebus/devices.

This approach is not particularly synchronous.  Take the case of an add
failure: how long would an application wait before deciding that the new
device is not going to appear?

It might be preferable to have the write to the probe function fail. 
For example, if we can't find the device node, return -EINVAL or
something.

> This is already taken care of by of_device_register().

If we are scrapping the use of of_device for ibmebus devices, can we
still create a devspec file for OF correlation purposes?

Thanks-
John

Re: [PATCH 2.6.21-rc1] ibmebus: Support dynamic addition and removal of adapters

[Joachim Fenkes]

John Rose <johnrose@austin.ibm.com> wrote on 20.02.2007 12:25:06:

> > If the probe operation succeeds, the respective device will show up 
> > beneath
> > /sys/bus/ibmebus/devices.
> 
> This approach is not particularly synchronous.  Take the case of an add
> failure: how long would an application wait before deciding that the new
> device is not going to appear?
>
> It might be preferable to have the write to the probe function fail. 
> For example, if we can't find the device node, return -EINVAL or
> something.

Can be done, sure. Just a matter of putting some more sophisticated error
handling into my code. Patch follows in a bit.
 
> > This is already taken care of by of_device_register().
> 
> If we are scrapping the use of of_device for ibmebus devices, can we
> still create a devspec file for OF correlation purposes?

We don't. My recent patch only changes the fake root device.
Ibmebus-based devices will still be of_devices, of course.

Regards,
  Joachim

---
Joachim Fenkes  --  eHCA Linux Driver Developer and Hardware Tamer
IBM Deutschland Entwicklung GmbH  --  Dept. 3627 (I/O Firmware Dev. 2)
Schoenaicher Strasse 220  --  71032 Boeblingen  --  Germany
eMail: fenkes@de.ibm.com  --  Phone: +49 7031 16 1239 

Sitz der Gesellschaft: Böblingen  --  Geschftsfhrung: Herbert Kircher
Vorsitzender des Aufsichtsrats: Johann Weihen
Registergericht: Amtsgericht Stuttgart, HRB 243294