[PATCH 2.6.23] ibmebus: Prevent bus_id collisions

[PATCH 2.6.23] ibmebus: Prevent bus_id collisions

[Joachim Fenkes]

Previously, ibmebus derived a device's bus_id from its location code. The
location code is not guaranteed to be unique, so we might get bus_id
collisions if two devices share the same location code. The OFDT full_name,
however, is unique, so we use that instead.

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

The patch has been tested and works fine. If you think it's too much change
for 2.6.23-rc5, please schedule for 2.6.24 instead.

 arch/powerpc/kernel/ibmebus.c |   30 +++++++++---------------------
 1 files changed, 9 insertions(+), 21 deletions(-)

diff --git a/arch/powerpc/kernel/ibmebus.c b/arch/powerpc/kernel/ibmebus.c
index 9a8c9af..d6a38cd 100644
--- a/arch/powerpc/kernel/ibmebus.c
+++ b/arch/powerpc/kernel/ibmebus.c
@@ -188,33 +188,21 @@ static struct ibmebus_dev* __devinit ibmebus_register_device_node(
 	struct device_node *dn)
 {
 	struct ibmebus_dev *dev;
-	const char *loc_code;
-	int length;
-
-	loc_code = of_get_property(dn, "ibm,loc-code", NULL);
-	if (!loc_code) {
-                printk(KERN_WARNING "%s: node %s missing 'ibm,loc-code'\n",
-		       __FUNCTION__, dn->name ? dn->name : "<unknown>");
-		return ERR_PTR(-EINVAL);
-        }
-
-	if (strlen(loc_code) == 0) {
-	        printk(KERN_WARNING "%s: 'ibm,loc-code' is invalid\n",
-		       __FUNCTION__);
-		return ERR_PTR(-EINVAL);
-	}
+	int i, len, bus_len;
 
 	dev = kzalloc(sizeof(struct ibmebus_dev), GFP_KERNEL);
-	if (!dev) {
+	if (!dev)
 		return ERR_PTR(-ENOMEM);
-	}
 
 	dev->ofdev.node = of_node_get(dn);
 
-	length = strlen(loc_code);
-	memcpy(dev->ofdev.dev.bus_id, loc_code
-		+ (length - min(length, BUS_ID_SIZE - 1)),
-		min(length, BUS_ID_SIZE - 1));
+	len = strlen(dn->full_name + 1);
+	bus_len = min(len, BUS_ID_SIZE - 1);
+	memcpy(dev->ofdev.dev.bus_id, dn->full_name + 1
+	       + (len - bus_len), bus_len);
+	for (i = 0; i < bus_len; i++)
+		if (dev->ofdev.dev.bus_id[i] == '/')
+			dev->ofdev.dev.bus_id[i] = '_';
 
 	/* Register with generic device framework. */
 	if (ibmebus_register_device_common(dev, dn->name) != 0) {
-- 
1.5.2

Re: [PATCH 2.6.23] ibmebus: Prevent bus_id collisions

[Nathan Lynch]

Hi-

Joachim Fenkes wrote:
> Previously, ibmebus derived a device's bus_id from its location code. The
> location code is not guaranteed to be unique, so we might get bus_id
> collisions if two devices share the same location code. The OFDT full_name,
> however, is unique, so we use that instead.

This is a userspace-visible change, but I guess it's unavoidable.
Will anything break?

Also, I dislike this approach of duplicating the firmware device tree
path in sysfs.  Are GX/ibmebus devices guaranteed to be children of
the same node in the OF device tree?  If so, their unit addresses will
be unique, and therefore suitable values for bus_id.  I believe this
is what the powerpc vio bus code does.

Re: [PATCH 2.6.23] ibmebus: Prevent bus_id collisions

[jschopp]

> +	len = strlen(dn->full_name + 1);
> +	bus_len = min(len, BUS_ID_SIZE - 1);
> +	memcpy(dev->ofdev.dev.bus_id, dn->full_name + 1
> +	       + (len - bus_len), bus_len);
> +	for (i = 0; i < bus_len; i++)
> +		if (dev->ofdev.dev.bus_id[i] == '/')
> +			dev->ofdev.dev.bus_id[i] = '_';
>  
>  	/* Register with generic device framework. */
>  	if (ibmebus_register_device_common(dev, dn->name) != 0) {

What happens when the full name is > 31 characters?  It looks to me that it 
will be truncated, which takes away the uniqueness guarantee.

There must be an individual property that is guaranteed to be unique and 
less than 32 characters.  How about "ibm,my-drc-index"?  That looks like a 
good candidate.

Re: [PATCH 2.6.23] ibmebus: Prevent bus_id collisions

[Joachim Fenkes]

Nathan Lynch <ntl@pobox.com> wrote on 29.08.2007 20:12:32:

> > Previously, ibmebus derived a device's bus_id from its location code. 
The
> > location code is not guaranteed to be unique, so we might get bus_id
> > collisions if two devices share the same location code. The OFDT 
full_name,
> > however, is unique, so we use that instead.
> 
> This is a userspace-visible change, but I guess it's unavoidable.
> Will anything break?

Nope. Userspace programs should not depend on ibmebus' way of naming the 
devices; especially since some overly long loc_codes tended to be 
truncated and thus rendered useless. I have tested IBM's DLPAR tools 
against the changed kernel, and they didn't break.
 
> Also, I dislike this approach of duplicating the firmware device tree
> path in sysfs.

Why? Any specific reasons for your dislike?

> Are GX/ibmebus devices guaranteed to be children of
> the same node in the OF device tree?  If so, their unit addresses will
> be unique, and therefore suitable values for bus_id.  I believe this
> is what the powerpc vio bus code does.

While there's no such guarantee (as in "officially signed document"), yes, 
I expect future GX devices to also appear beneath the OFDT root node. For 
the existing devices, the unit addresses are already part of the device 
name, so I save the need to use sprintf() again. Plus, I rather like using 
the full_name since it also contains a descriptive name as opposed to 
being just nondescript numbers, helping the layman (ie user) to make sense 
out of a dev_id.

jschopp <jschopp@austin.ibm.com> wrote on 29.08.2007 20:33:30:

> > +   len = strlen(dn->full_name + 1);
> > +   bus_len = min(len, BUS_ID_SIZE - 1);
> > +   memcpy(dev->ofdev.dev.bus_id, dn->full_name + 1
> > +          + (len - bus_len), bus_len);
> > +   for (i = 0; i < bus_len; i++)
> > +      if (dev->ofdev.dev.bus_id[i] == '/')
> > +         dev->ofdev.dev.bus_id[i] = '_';
> > 
> >     /* Register with generic device framework. */
> >     if (ibmebus_register_device_common(dev, dn->name) != 0) {
> 
> What happens when the full name is > 31 characters?  It looks to me that 
it 
> will be truncated, which takes away the uniqueness guarantee.

There are currently two GX devices, eHCA and eHEA, which both reside 
beneath the root node - this is required by architecture for those 
devices. Unless they invent a device called 
"supercalifragilisticexpialidocious", devices in the root note will have a 
full_name of less than 31 chars. Even in that case, the truncation occurs 
at the beginning, so the @xxx part that makes the nodes unique will stay 
in place.

If any more GX devices appear on the scene, I expect them to appear in the 
root node as well. The substitution of "/" by "_" is a safeguard so 
possible weird OFDT setups don't break the kernel.
 
> There must be an individual property that is guaranteed to be unique and 

> less than 32 characters.  How about "ibm,my-drc-index"?  That looks like 
a 
> good candidate.

On first glance, it does, however the attribute might not be present in 
all cases. Architecture states it only needs to be present on systems with 
dynamic reconfiguration enabled.

All things considered, I still like the idea of using the full_name most. 
No offense.

Regards,
  Joachim

Re: [PATCH 2.6.23] ibmebus: Prevent bus_id collisions

[Nathan Lynch]

Hi Joachim-

Joachim Fenkes wrote:
> Nathan Lynch <ntl@pobox.com> wrote on 29.08.2007 20:12:32:
> > Will anything break?
> 
> Nope. Userspace programs should not depend on ibmebus' way of naming the 
> devices; especially since some overly long loc_codes tended to be 
> truncated and thus rendered useless. I have tested IBM's DLPAR tools 
> against the changed kernel, and they didn't break.

Okay.


> > Also, I dislike this approach of duplicating the firmware device tree
> > path in sysfs.
> 
> Why? Any specific reasons for your dislike?

struct device's bus_id field is but 20 bytes in size.  Too close for
comfort?


> > Are GX/ibmebus devices guaranteed to be children of
> > the same node in the OF device tree?  If so, their unit addresses will
> > be unique, and therefore suitable values for bus_id.  I believe this
> > is what the powerpc vio bus code does.
> 
> While there's no such guarantee (as in "officially signed document"), yes, 
> I expect future GX devices to also appear beneath the OFDT root node. For 
> the existing devices, the unit addresses are already part of the device 
> name, so I save the need to use sprintf() again. Plus, I rather like using 
> the full_name since it also contains a descriptive name as opposed to 
> being just nondescript numbers, helping the layman (ie user) to make sense 
> out of a dev_id.

Okay, but your layman isn't supposed to be relying on any
user-friendly properties of the name :) Hope he doesn't work on a
distro installer.

Anyway, if you're still confident in this approach, I relent.  :)

Re: [PATCH 2.6.23] ibmebus: Prevent bus_id collisions

[Arnd Bergmann]

On Wednesday 29 August 2007, Joachim Fenkes wrote:
> Previously, ibmebus derived a device's bus_id from its location code. The
> location code is not guaranteed to be unique, so we might get bus_id
> collisions if two devices share the same location code. The OFDT full_name,
> however, is unique, so we use that instead.
> 
> Signed-off-by: Joachim Fenkes <fenkes@de.ibm.com>

Actually, I think it would be much better to convert the code to be more
like of_platform_device, or to even replace all of ibmebus with that.

The whole logic of dynamically adding and removing device is rather bogus,
and it prevents autoloading of device drivers. of_platform_make_bus_id
is the function that is responsible for creating unique names over there.

	Arnd <><

Re: [PATCH 2.6.23] ibmebus: Prevent bus_id collisions

[Joel Schopp]

> There are currently two GX devices, eHCA and eHEA, which both reside 
> beneath the root node - this is required by architecture for those 
> devices. Unless they invent a device called 
> "supercalifragilisticexpialidocious", devices in the root note will have a 
> full_name of less than 31 chars. Even in that case, the truncation occurs 
> at the beginning, so the @xxx part that makes the nodes unique will stay 
> in place.
> 

OK, didn't realize it had to be beneath the root node, and that the 
truncation truncated the front and not the back.  I would have done it 
differently, but this should work.

Acked-by: Joel Schopp <jschopp@austin.ibm.com>

Re: [PATCH 2.6.23] ibmebus: Prevent bus_id collisions

[Linas Vepstas]

On Thu, Aug 30, 2007 at 04:00:56PM +0200, Joachim Fenkes wrote:
> 
> Plus, I rather like using 
> the full_name since it also contains a descriptive name as opposed to 
> being just nondescript numbers, helping the layman (ie user) to make sense 
> out of a dev_id.

Yes, well, but no. The location code is useful as a geographical
location: slots and devices are physically labelled with stickers 
so you can tell which is which.  Handy when you have to unplug stuff. 
By contrast, the device-tree full_name is mostly just gobldy-gook, 
with some crazy phb numbering in there that, after four years of 
staring at them, I still can't reliably do anything useful with.  
Location codes are nice. 

--linas

Re: [PATCH 2.6.23] ibmebus: Prevent bus_id collisions

[Joachim Fenkes]

linas@austin.ibm.com (Linas Vepstas) wrote on 30.08.2007 23:28:16:

> On Thu, Aug 30, 2007 at 04:00:56PM +0200, Joachim Fenkes wrote:
> > 
> > Plus, I rather like using 
> > the full_name since it also contains a descriptive name as opposed to 
> > being just nondescript numbers, helping the layman (ie user) to make 
sense 
> > out of a dev_id.
>
> [...]
> Location codes are nice. 

I sure agree with you. Still, they're not unique, so we can't use them as 
bus_id. That's why we're having this discussion at all.

What I meant was "I like using the full_name over using the device address 
/ DRC index / you-name-it only". Location code is right out.

Cheers,
  Joachim

Re: [PATCH 2.6.23] ibmebus: Prevent bus_id collisions

[Joachim Fenkes]

Hi, Arnd,

> The whole logic of dynamically adding and removing device is rather 
bogus,
> and it prevents autoloading of device drivers. of_platform_make_bus_id
> is the function that is responsible for creating unique names over 
there.

The plaintiff makes a valid point. How about a staging approach: We put 
the patch as it is now into 2.6.23 so the problem is fixed, and I'll post 
a "nice" version with autoloading support and a generic of_make_bus_id 
function for 2.6.24. Agree?

Regards,
  Joachim

Re: [PATCH 2.6.23] ibmebus: Prevent bus_id collisions

[Arnd Bergmann]

On Friday 31 August 2007, Joachim Fenkes wrote:
> > The whole logic of dynamically adding and removing device is rather 
> bogus,
> > and it prevents autoloading of device drivers. of_platform_make_bus_id
> > is the function that is responsible for creating unique names over 
> there.
> 
> The plaintiff makes a valid point. How about a staging approach: We put 
> the patch as it is now into 2.6.23 so the problem is fixed, and I'll post 
> a "nice" version with autoloading support and a generic of_make_bus_id 
> function for 2.6.24. Agree?

Ok, sounds fair. Can you make sure that the resulting bus_id is the same
for the final version then?

	Arnd <><

Re: [PATCH 2.6.23] ibmebus: Prevent bus_id collisions

[Joachim Fenkes]

> > The plaintiff makes a valid point. How about a staging approach: We 
put 
> > the patch as it is now into 2.6.23 so the problem is fixed, and I'll 
post 
> > a "nice" version with autoloading support and a generic of_make_bus_id 

> > function for 2.6.24. Agree?
> 
> Ok, sounds fair. Can you make sure that the resulting bus_id is the same
> for the final version then?

No, it would change once more -- the current, minimal-invasive, variant of 
the fix uses the full_name -> "ehca@12345678", while of_make_bus_id uses 
another format: "12345678.ehca". I don't think this makes much of a 
difference, though, and users shouldn't rely on the bus_id having a 
certain format anyway, so IMHO we can live with this.

Regards,
  Joachim