Fix Firmware class name collision

Fix Firmware class name collision

[Timur Tabi]

Markus and Greg,

I've found a problem with this patch that probably affects a number of embedded 
PowerPC systems:

http://marc.info/?l=linux-kernel&m=119222892713518&w=2

The problem is that the device name for many PowerPC SoC devices is based on the 
physical address.  So we have stuff like this:

# ls -l /sys/devices/
drwxr-xr-x    9 root     root            0 Nov  9 17:29 e0000000.soc8323
drwxr-xr-x   11 root     root            0 Nov  9 17:22 e0100000.qe
[snip]
# ls -l /sys/devices/e0000000.soc8323/
drwxr-xr-x    2 root     root            0 Nov  9 17:34 e0000200.wdt
drwxr-xr-x    2 root     root            0 Nov  9 17:34 e0000700.pic
drwxr-xr-x    2 root     root            0 Nov  9 17:34 e0001400.par_io
drwxr-xr-x    2 root     root            0 Nov  9 17:34 e0003000.i2c
drwxr-xr-x    2 root     root            0 Nov  9 17:34 e0004500.serial
drwxr-xr-x    2 root     root            0 Nov  9 17:34 e0004600.serial
drwxr-xr-x    2 root     root            0 Nov  9 17:34 e0030000.crypto
[snip]

With this patch, the device names in /sys/class/firmware look like this:

# ls -l /sys/class/firmware/
drwxr-xr-x    2 root     root            0 Nov  9 17:37 firmware-e0102400.u
-rw-r--r--    1 root     root         4096 Nov  9 17:22 timeout

In other words, the only thing you get is the first letter of the device name. 
You used to get the whole name.  The physical address obviously isn't very helpful.

The problem is the size of the string is only 20 characters:

static inline void fw_setup_device_id(struct device *f_dev, struct device *dev)
{
	snprintf(f_dev->bus_id, BUS_ID_SIZE, "firmware-%s", dev->bus_id);
}

Now, there are two solutions:

1) Change the PowerPC device names from physical_address.device_name to 
device_name.physical_address (so that e0102400.ucc becomes ucc.e0102400)

2) Change fw_setup_device_id() to something like this:

	snprintf(f_dev->bus_id, BUS_ID_SIZE, "fw-%s", dev->bus_id);

which do you think is a better idea?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Fix Firmware class name collision

[Scott Wood]

Timur Tabi wrote:
> 
> In other words, the only thing you get is the first letter of the device name. 
> You used to get the whole name.  The physical address obviously isn't very helpful.

The physical address certainly is useful when you have more than one 
device of the same name.

> Now, there are two solutions:
> 
> 1) Change the PowerPC device names from physical_address.device_name to 
> device_name.physical_address (so that e0102400.ucc becomes ucc.e0102400)

So then you'd get "firmware-ucc.e01024".  What if there's another ucc at 
  e0102480?  For devices with longer names, you'd have even less 
precision in the address.

-Scott

Re: Fix Firmware class name collision

[Timur Tabi]

Scott Wood wrote:

> The physical address certainly is useful when you have more than one 
> device of the same name.

What I meant was that the physical address isn't helpful by itself.

> So then you'd get "firmware-ucc.e01024".  What if there's another ucc at 
>  e0102480?  For devices with longer names, you'd have even less 
> precision in the address.

Maybe we need to consider a more sophisticated algorithm, one that guarantees 
that the device name in its entirety is preserved?  Either that, or replace 
the physical address with something shorter, like the offset to the root node 
only?  That way, ucc.e0102400 because just ucc.2400.

-- 
Timur Tabi
Linux Kernel Developer @ Freescale

Re: Fix Firmware class name collision

[Greg KH]

On Tue, Dec 04, 2007 at 07:28:00PM -0600, Timur Tabi wrote:
> Scott Wood wrote:
>
>> The physical address certainly is useful when you have more than one 
>> device of the same name.
>
> What I meant was that the physical address isn't helpful by itself.
>
>> So then you'd get "firmware-ucc.e01024".  What if there's another ucc at  
>> e0102480?  For devices with longer names, you'd have even less precision 
>> in the address.
>
> Maybe we need to consider a more sophisticated algorithm, one that 
> guarantees that the device name in its entirety is preserved?  Either that, 
> or replace the physical address with something shorter, like the offset to 
> the root node only?  That way, ucc.e0102400 because just ucc.2400.

You should do something :)

In the near future (2.6.26) there will not be a limit on the size of the
file name, so we should not have this problem anymore.

thanks,

greg k-h

Re: Fix Firmware class name collision

[Timur Tabi]

Greg KH wrote:

> In the near future (2.6.26) there will not be a limit on the size of the
> file name, so we should not have this problem anymore.

In that case, I won't worry about it.  I would have preferred 2.6.25, since I 
have code going into 2.6.25 that is affected by the current limit.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: Fix Firmware class name collision

[Benjamin Herrenschmidt]

On Fri, 2007-12-14 at 14:40 -0800, Greg KH wrote:
> On Tue, Dec 04, 2007 at 07:28:00PM -0600, Timur Tabi wrote:
> > Scott Wood wrote:
> >
> >> The physical address certainly is useful when you have more than one 
> >> device of the same name.
> >
> > What I meant was that the physical address isn't helpful by itself.
> >
> >> So then you'd get "firmware-ucc.e01024".  What if there's another ucc at  
> >> e0102480?  For devices with longer names, you'd have even less precision 
> >> in the address.
> >
> > Maybe we need to consider a more sophisticated algorithm, one that 
> > guarantees that the device name in its entirety is preserved?  Either that, 
> > or replace the physical address with something shorter, like the offset to 
> > the root node only?  That way, ucc.e0102400 because just ucc.2400.
> 
> You should do something :)
> 
> In the near future (2.6.26) there will not be a limit on the size of the
> file name, so we should not have this problem anymore.

Not even .25 ? damn ! Any way that fix can be fastracked ? This
limitation has been a major PITA for some time now (this is just -one-
example where it gets in the way).

Ben.

Re: Fix Firmware class name collision

[Greg KH]

On Sat, Dec 15, 2007 at 05:14:29PM +1100, Benjamin Herrenschmidt wrote:
> 
> On Fri, 2007-12-14 at 14:40 -0800, Greg KH wrote:
> > On Tue, Dec 04, 2007 at 07:28:00PM -0600, Timur Tabi wrote:
> > > Scott Wood wrote:
> > >
> > >> The physical address certainly is useful when you have more than one 
> > >> device of the same name.
> > >
> > > What I meant was that the physical address isn't helpful by itself.
> > >
> > >> So then you'd get "firmware-ucc.e01024".  What if there's another ucc at  
> > >> e0102480?  For devices with longer names, you'd have even less precision 
> > >> in the address.
> > >
> > > Maybe we need to consider a more sophisticated algorithm, one that 
> > > guarantees that the device name in its entirety is preserved?  Either that, 
> > > or replace the physical address with something shorter, like the offset to 
> > > the root node only?  That way, ucc.e0102400 because just ucc.2400.
> > 
> > You should do something :)
> > 
> > In the near future (2.6.26) there will not be a limit on the size of the
> > file name, so we should not have this problem anymore.
> 
> Not even .25 ? damn ! Any way that fix can be fastracked ? This
> limitation has been a major PITA for some time now (this is just -one-
> example where it gets in the way).

I'll let Kay answer that, last he said, it involves a _lot_ of changes
throughout the kernel :(

thanks,

greg k-h

Re: Fix Firmware class name collision

[Kay Sievers]

On Fri, 2007-12-14 at 22:39 -0800, Greg KH wrote:
> On Sat, Dec 15, 2007 at 05:14:29PM +1100, Benjamin Herrenschmidt wrote:
> > 
> > On Fri, 2007-12-14 at 14:40 -0800, Greg KH wrote:
> > > On Tue, Dec 04, 2007 at 07:28:00PM -0600, Timur Tabi wrote:
> > > > Scott Wood wrote:
> > > >
> > > >> The physical address certainly is useful when you have more than one 
> > > >> device of the same name.
> > > >
> > > > What I meant was that the physical address isn't helpful by itself.
> > > >
> > > >> So then you'd get "firmware-ucc.e01024".  What if there's another ucc at  
> > > >> e0102480?  For devices with longer names, you'd have even less precision 
> > > >> in the address.
> > > >
> > > > Maybe we need to consider a more sophisticated algorithm, one that 
> > > > guarantees that the device name in its entirety is preserved?  Either that, 
> > > > or replace the physical address with something shorter, like the offset to 
> > > > the root node only?  That way, ucc.e0102400 because just ucc.2400.
> > > 
> > > You should do something :)
> > > 
> > > In the near future (2.6.26) there will not be a limit on the size of the
> > > file name, so we should not have this problem anymore.
> > 
> > Not even .25 ? damn ! Any way that fix can be fastracked ? This
> > limitation has been a major PITA for some time now (this is just -one-
> > example where it gets in the way).
> 
> I'll let Kay answer that, last he said, it involves a _lot_ of changes
> throughout the kernel :(

The current patch gets rid of bus_id and uses the dynamic kobject_name
directly all over the place. It's huge, because the current code expects
a static array and uses strncpy/snprintf all over the place.

I'm currently waiting for the new kobject api to finish, before I update
the patch, not sure if we will make it in time for .25.

Kay