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 <[email protected]>
---
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
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.
> + 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.
Nathan Lynch <[email protected]> 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 <[email protected]> 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
Hi Joachim-
Joachim Fenkes wrote:
> Nathan Lynch <[email protected]> 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. :)
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 <[email protected]>
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 <><
> 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 <[email protected]>
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
[email protected] (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
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
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 <><
> > 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