Hi Greg,
I am a little confused by the directories created when one registers a
class device. When a class device is registered as the children of a
real device, a subdirectory by the class name is created, and the class
device is created there, effectively granting each class a separate
namespace. Example:
/sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0
where 0000:00:1f.3 is the physical device, i2c-adapter the class name
and i2c-0 the class device.
OTOH, if I create a class device as the children of another class
device, the class device is created directly, without a directory
between the parent and the child. Example:
/sys/class/i2c-adapter/i2c-0/i2c-0
where the first i2c-0 is an i2c-adapter class device, and the second
i2c-0 is an i2c-dev class device. I would have expected:
/sys/class/i2c-adapter/i2c-0/i2c-dev/i2c-0
The current behavior seems inconsistent to me. Is it done so on purpose,
or is this accidental? If on purpose, what's the reason?
I am asking because this is causing trouble in practice. We have both
i2c-dev and firmware_class which try to create class devices by the
same name and this of course collides. While I would blame
firmware_class for coming up with an horrible naming scheme (or
actually, for not coming up with any naming scheme) it might still be a
good idea to prevent such collisions at the driver core level.
Thanks,
--
Jean Delvare
On Sun, Mar 29, 2009 at 17:48, Jean Delvare <[email protected]> wrote:
> I am a little confused by the directories created when one registers a
> class device. When a class device is registered as the children of a
> real device, a subdirectory by the class name is created, and the class
> device is created there, effectively granting each class a separate
> namespace. Example:
>
> /sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0
>
> where 0000:00:1f.3 is the physical device, i2c-adapter the class name
> and i2c-0 the class device.
>
> OTOH, if I create a class device as the children of another class
> device, the class device is created directly, without a directory
> between the parent and the child. Example:
>
> /sys/class/i2c-adapter/i2c-0/i2c-0
>
> where the first i2c-0 is an i2c-adapter class device, and the second
> i2c-0 is an i2c-dev class device. I would have expected:
>
> /sys/class/i2c-adapter/i2c-0/i2c-dev/i2c-0
>
> The current behavior seems inconsistent to me. Is it done so on purpose,
> or is this accidental? If on purpose, what's the reason?
It's on purpose. The "glue" directory exists only between class
devices and bus devices. There is no need for class devices to have
such a "glue". When we moved the class devs as childs of the bus devs,
people complained, that they could no longer rename their netif to
"irq", because the name was already taken by a pci dev atrribute. :)
> I am asking because this is causing trouble in practice. We have both
> i2c-dev and firmware_class which try to create class devices by the
> same name and this of course collides. While I would blame
> firmware_class for coming up with an horrible naming scheme (or
> actually, for not coming up with any naming scheme) it might still be a
> good idea to prevent such collisions at the driver core level.
You have multiple instances, which request a firmware file for the
same parent device at the same time? Can't they request the firmware
for the actual child instead of using the same parent?
If the same parent needs to work, the firmware class needs to be fixed
to allow that. Maybe it should use the requested firmware filename
with the '/' translated to '!' as the name in sysfs, instead of the
easy-to-have-a-clash device name of the parent?
Thanks,
Kay
Hi Kay,
On Sun, 29 Mar 2009 18:36:46 +0200, Kay Sievers wrote:
> On Sun, Mar 29, 2009 at 17:48, Jean Delvare <[email protected]> wrote:
> > I am a little confused by the directories created when one registers a
> > class device. When a class device is registered as the children of a
> > real device, a subdirectory by the class name is created, and the class
> > device is created there, effectively granting each class a separate
> > namespace. Example:
> >
> > /sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0
> >
> > where 0000:00:1f.3 is the physical device, i2c-adapter the class name
> > and i2c-0 the class device.
> >
> > OTOH, if I create a class device as the children of another class
> > device, the class device is created directly, without a directory
> > between the parent and the child. Example:
> >
> > /sys/class/i2c-adapter/i2c-0/i2c-0
> >
> > where the first i2c-0 is an i2c-adapter class device, and the second
> > i2c-0 is an i2c-dev class device. I would have expected:
> >
> > /sys/class/i2c-adapter/i2c-0/i2c-dev/i2c-0
> >
> > The current behavior seems inconsistent to me. Is it done so on purpose,
> > or is this accidental? If on purpose, what's the reason?
>
> It's on purpose. The "glue" directory exists only between class
> devices and bus devices. There is no need for class devices to have
> such a "glue".
The example I have is one where the need exists.
> When we moved the class devs as childs of the bus devs,
> people complained, that they could no longer rename their netif to
> "irq", because the name was already taken by a pci dev atrribute. :)
That's what I had guessed, although this only moves the problem: if a
class has the same name as an attribute, a collision will happen.
Probably that's just less likely to happen than collisions between
class device names and device attributes.
> > I am asking because this is causing trouble in practice. We have both
> > i2c-dev and firmware_class which try to create class devices by the
> > same name and this of course collides. While I would blame
> > firmware_class for coming up with an horrible naming scheme (or
> > actually, for not coming up with any naming scheme) it might still be a
> > good idea to prevent such collisions at the driver core level.
>
> You have multiple instances, which request a firmware file for the
> same parent device at the same time? Can't they request the firmware
> for the actual child instead of using the same parent?
No, there's a single firmware request which collides with an i2c-dev
class device name. The firmware request happens to use the parent
device's name as its class device name, and it happens that both the
i2c-adapter class and the i2c-dev class name their class devices the
same way (which makes sense as they refer to the same thing,
i2c-adapter for the kernel-space access and i2c-dev for the user-space
access), and the latter is a child of the former. So when an
i2c-adapter class device is used as the parent for a firmware request,
and the i2c-dev driver is loaded, you get a collision.
It could be argued that i2c-adapter and i2c-dev should be the same
class, but changing this now would break several user-space tools so I
doubt it's going to happen. The fact that i2c-dev is optional can also
be seen as a desirable feature.
> If the same parent needs to work, the firmware class needs to be fixed
> to allow that. Maybe it should use the requested firmware filename
> with the '/' translated to '!' as the name in sysfs, instead of the
> easy-to-have-a-clash device name of the parent?
Changing the name of the firmware class devices was already attempted
once, but was quickly reverted because it broke some user-space tools:
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=109f0e93b6b728f03c1eb4af02bc25d71b646c59
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commit;h=7d640c4a5b36c4733460065db1554da924044511
Unless the user-space tool in question (Dell BIOS updater) has been
modified to no longer depend on this since then, changing the firmware
class device names isn't possible.
Now that we have a use case which would justify that a class device
child of another class device gets its own namespace, is there a chance
to see it happen?
Thanks,
--
Jean Delvare
On Mon, Mar 30, 2009 at 10:49, Jean Delvare <[email protected]> wrote:
> On Sun, 29 Mar 2009 18:36:46 +0200, Kay Sievers wrote:
>> On Sun, Mar 29, 2009 at 17:48, Jean Delvare <[email protected]> wrote:
>> You have multiple instances, which request a firmware file for the
>> same parent device at the same time? Can't they request the firmware
>> for the actual child instead of using the same parent?
>
> No, there's a single firmware request which collides with an i2c-dev
> class device name. The firmware request happens to use the parent
> device's name as its class device name, and it happens that both the
> i2c-adapter class and the i2c-dev class name their class devices the
> same way (which makes sense as they refer to the same thing,
> i2c-adapter for the kernel-space access and i2c-dev for the user-space
> access), and the latter is a child of the former. So when an
> i2c-adapter class device is used as the parent for a firmware request,
> and the i2c-dev driver is loaded, you get a collision.
Ah, I see. If you have a single firmware request, how about using the
bus parent device of your i2c device to request the file, instead of
the class parent device? Then you should get the glue directory, and
all should work.
If that does not work for some reason, we can make the firmware class
to add the "firmware-%s" prefix, like your patch did, only if the
parent is a class device, not a bus device. That should prevent the
breakage of tools making these wrong assumptions.
> Now that we have a use case which would justify that a class device
> child of another class device gets its own namespace, is there a chance
> to see it happen?
Maybe we could do that, and do it only in the case of a class device
uses a class device of a different class as a parent. We need to make
sure, that we don't insert "block" between the disk and the partition,
and the like. It would be kind of inconsistent though, because we
don't do the same thing for any bus devices.
It's very uncommon that class devices want to parent with a class
device of a different class. In most cases, the parent device class
should be converted to a bus, and also use proper device binding
logic. If device binding is not needed, then one of the classes may be
just not needed. Having inter-dependencies between different classes
of the same subsystem, and use each other as a parent sounds rather
strange to me, and I would need to be convinced, that the driver core
would need to work around such issues, and not the subsystem itself.
:)
Thanks,
Kay
Hi Kay,
Sorry, completely overlooked your reply... Ah, now I remember, I was
waiting for Michael to show up and comment on this. Michael, ping? As
long as we don't know the exact requirements and constraints of the
dell firmware tool, we're somewhat stuck.
Matt, maybe you can help? Could you please point us to the source code
of the dell firmware tool which relies on the dell_rbu driver?
On Mon, 30 Mar 2009 13:24:59 +0200, Kay Sievers wrote:
> On Mon, Mar 30, 2009 at 10:49, Jean Delvare <[email protected]> wrote:
> > No, there's a single firmware request which collides with an i2c-dev
> > class device name. The firmware request happens to use the parent
> > device's name as its class device name, and it happens that both the
> > i2c-adapter class and the i2c-dev class name their class devices the
> > same way (which makes sense as they refer to the same thing,
> > i2c-adapter for the kernel-space access and i2c-dev for the user-space
> > access), and the latter is a child of the former. So when an
> > i2c-adapter class device is used as the parent for a firmware request,
> > and the i2c-dev driver is loaded, you get a collision.
>
> Ah, I see. If you have a single firmware request, how about using the
> bus parent device of your i2c device to request the file, instead of
> the class parent device? Then you should get the glue directory, and
> all should work.
I did propose this already. I received the objection that some devices
were USB and some were PCI and some were neither. I don't think this is
really a problem, the i2c adapter should have a valid parent either way.
So I do think this is a valid workaround, although not trivial: all
affected drivers must be investigated and fixed individually.
> If that does not work for some reason, we can make the firmware class
> to add the "firmware-%s" prefix, like your patch did, only if the
> parent is a class device, not a bus device. That should prevent the
> breakage of tools making these wrong assumptions.
Given that the dell firmware device is a platform device, yes, that
would work around the breakage. This approach is quite hackish though,
adding an inconsistency.
> > Now that we have a use case which would justify that a class device
> > child of another class device gets its own namespace, is there a chance
> > to see it happen?
>
> Maybe we could do that, and do it only in the case of a class device
> uses a class device of a different class as a parent. We need to make
> sure, that we don't insert "block" between the disk and the partition,
> and the like. It would be kind of inconsistent though, because we
Ah, yeah, I didn't have that case in mind at first. Glad I asked you
for your opinion :)
> don't do the same thing for any bus devices.
Well, we already have an exception for the case where the parent is a
bus device and the child is a class device. Extending it to the case
where the parent and child are class devices of different classes
doesn't sound too illogical to me. I think this approach has my
preference. Could you please propose a patch doing this? You are way
more familiar with the driver core code than I am, so I'm sure you'll
be much quicker than me to implement this.
> It's very uncommon that class devices want to parent with a class
> device of a different class. In most cases, the parent device class
> should be converted to a bus, and also use proper device binding
> logic. If device binding is not needed, then one of the classes may be
> just not needed. Having inter-dependencies between different classes
> of the same subsystem, and use each other as a parent sounds rather
> strange to me, and I would need to be convinced, that the driver core
> would need to work around such issues, and not the subsystem itself.
> :)
For i2c-dev it seems to be the right thing to do, as the i2c-dev class
device is an extension of the underlying i2c-adapter class device. You
can't have an i2c-dev without its i2c-adapter, so it seems OK that the
former is a child of the latter.
The i2c-adapter class device implements kernel-space access to an I2C
bus. The i2c-dev class device adds user-space access to the same I2C
bus. I have been thinking of a way to clean this up, but as far as I
can see we would have to merge i2c-dev into i2c-adapter, that is, make
user-space access support mandatory while it is optional today. While
i2c-dev isn't too big, I'm still not sure if everyone will enjoy this.
If anyone has an idea how to improve the situation, I'm all ears. But
anyway, this isn't going to happen quickly, and I still believe we
should solve the namespace problem I reported.
For firmware, it doesn't seem as legitimate, I can't really think of a
good reason to use the i2c-adapter as the parent for the firmware
device.
--
Jean Delvare
On Sun, Apr 26, 2009 at 08:54:01AM +0200, Jean Delvare wrote:
> Hi Kay,
>
> Sorry, completely overlooked your reply... Ah, now I remember, I was
> waiting for Michael to show up and comment on this. Michael, ping? As
> long as we don't know the exact requirements and constraints of the
> dell firmware tool, we're somewhat stuck.
>
> Matt, maybe you can help? Could you please point us to the source code
> of the dell firmware tool which relies on the dell_rbu driver?
There's a git tree at:
http://linux.dell.com/git/?p=libsmbios.git
which you can get via rsync at:
rsync -va rsync://linux.dell.com/git/libsmbios.git/
the application is dellBiosUpdate, and the source is at this point in
the tree:
http://linux.dell.com/git/?p=libsmbios.git;a=blob;f=src/bin/dellBiosUpdate.cpp;h=b32e157b7ceb0be0d53eed4a4d171d24fe6456a7;hb=HEAD
--
Matt Domsch
Linux Technology Strategist, Dell Office of the CTO
linux.dell.com & http://www.dell.com/linux
On Sun, Apr 26, 2009 at 07:44:43AM -0500, Matt Domsch wrote:
> On Sun, Apr 26, 2009 at 08:54:01AM +0200, Jean Delvare wrote:
> > Hi Kay,
> >
> > Sorry, completely overlooked your reply... Ah, now I remember, I was
> > waiting for Michael to show up and comment on this. Michael, ping? As
> > long as we don't know the exact requirements and constraints of the
> > dell firmware tool, we're somewhat stuck.
> >
> > Matt, maybe you can help? Could you please point us to the source code
> > of the dell firmware tool which relies on the dell_rbu driver?
>
>
> There's a git tree at:
> http://linux.dell.com/git/?p=libsmbios.git
>
> which you can get via rsync at:
> rsync -va rsync://linux.dell.com/git/libsmbios.git/
>
> the application is dellBiosUpdate, and the source is at this point in
> the tree:
>
> http://linux.dell.com/git/?p=libsmbios.git;a=blob;f=src/bin/dellBiosUpdate.cpp;h=b32e157b7ceb0be0d53eed4a4d171d24fe6456a7;hb=HEAD
I'll note there is a second implementation that uses dell_rbu, and
that is included in each and every "Dell Update Package" (aka DUP)
BIOS update posted on support.dell.com for every Dell PowerEdge server
for the past several years. Because the updater and they BIOS payload
are packaged in the same package, a change which would require
changing that updater will result in requiring Dell to spin and
re-release hundreds of DUPs, which would not go over well... Thank
you for being careful not to change the interface capriciously - it
could have a huge impact on us.
Thanks,
Matt
--
Matt Domsch
Linux Technology Strategist, Dell Office of the CTO
linux.dell.com & http://www.dell.com/linux
On Sun, Apr 26, 2009 at 08:54, Jean Delvare <[email protected]> wrote:
> But anyway, this isn't going to happen quickly, and I still believe we
> should solve the namespace problem I reported.
I think the real namespace problem lives well inside the i2c devices,
not in the core. :) Sure, the firmware class is pretty dumb with its
naming, but the root case is the way i2c devices are created.
First, i2c stacks class devices, which it should not do, it should use
a bus not a class, if it has device <-> children relations between
different types.
Second, it names all the devices from different classes with the
_same_ name, then puts them in a hierarchy, and the tries to mix-in a
third class, the firmware class, which also uses the _same_ name. That
just asks for trouble. :)
> For firmware, it doesn't seem as legitimate, I can't really think of a
> good reason to use the i2c-adapter as the parent for the firmware
> device.
Oh, I may miss something, I thought that is what you already do and
causes the problem? I though you use the "adapter" to request the
firmware? Otherwise I don't see how this can conflict.
You have the "adapter" and the "device" stacked, right?
/sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0/i2c-0/
Then you use the adapter:
/sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0
to request the firmware, which will try to create:
/sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0/i2c-0/
I thought you could use the "device" and not the "adapter" to request
the firmware? Then you would get a "nice" chain of devices with all
the same names, like :)
/sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0/i2c-0/i2c-0/
I see the following options:
- the i2c "device", not the "adapter" would request the firmware
- the i2c-adapter converts to a bus, and is no longer a class
- i2c "adapters" and "devices" don't get the same duplicated name
- if "adapters" can only ever have a single "device", the
both could just be merged to a single instance, and maybe compat
symlinks created
- the firmware class adds a firmware-% prefix in the case the
requesting device is a class device, and not the common case
which is a bus device
- change the "glue dir" rules, which I would try to avoid, because
we don't know who else might stack class devices of different
classes, and might get surprised by this
What do you think?
Thanks,
Kay
On Sun, Apr 26, 2009 at 08:54:01AM +0200, Jean Delvare wrote:
> Hi Kay,
>
> Sorry, completely overlooked your reply... Ah, now I remember, I was
> waiting for Michael to show up and comment on this. Michael, ping? As
> long as we don't know the exact requirements and constraints of the
> dell firmware tool, we're somewhat stuck.
>
> Matt, maybe you can help? Could you please point us to the source code
> of the dell firmware tool which relies on the dell_rbu driver?
Sorry about that. Skimmed the thread and looks like I missed out...
The problem is that there are two users: 1) libsmbios as Matt has
already mentioned, and 2) Dell OpenMange Server Administrator.
Specifically srvadmin-hapi has the code to use the dell_rbu code
(proprietary, no source available). The srvadmin-hapi code is embedded
into all Dell Update Packages (DUPs).
The problem being that any change to the paths breaks all released DUPs.
--
Michael
Hi Michael,
On Mon, 27 Apr 2009 08:20:43 -0500, Michael E Brown wrote:
> On Sun, Apr 26, 2009 at 08:54:01AM +0200, Jean Delvare wrote:
> > Hi Kay,
> >
> > Sorry, completely overlooked your reply... Ah, now I remember, I was
> > waiting for Michael to show up and comment on this. Michael, ping? As
> > long as we don't know the exact requirements and constraints of the
> > dell firmware tool, we're somewhat stuck.
> >
> > Matt, maybe you can help? Could you please point us to the source code
> > of the dell firmware tool which relies on the dell_rbu driver?
>
> Sorry about that. Skimmed the thread and looks like I missed out...
>
> The problem is that there are two users: 1) libsmbios as Matt has
> already mentioned, and 2) Dell OpenMange Server Administrator.
> Specifically srvadmin-hapi has the code to use the dell_rbu code
> (proprietary, no source available). The srvadmin-hapi code is embedded
> into all Dell Update Packages (DUPs).
>
> The problem being that any change to the paths breaks all released DUPs.
The question was: any change to _which_ paths exactly? Sysfs often
offers several ways to reach the same object, and it matters to know
which need to be preserved and which can change.
Looking at src/libsmbios_c++/rbu/Rbu_Linux.cpp, I see the following
paths are used:
const char *rbu_v1_mono_data_file = "/sys/firmware/rbu/rbudata";
const char *rbu_v1_mono_size_file = "/sys/firmware/rbu/rbudatasize";
const char *rbu_v1_pkt_data_file = "/sys/firmware/rbu/packetdata";
const char *rbu_v1_pkt_size_file = "/sys/firmware/rbu/packetdatasize";
const char *rbu_v2_fw_data_file = "/sys/class/firmware/dell_rbu/data";
const char *rbu_v2_fw_load_file = "/sys/class/firmware/dell_rbu/loading";
const char *rbu_v2_img_type_file = "/sys/devices/platform/dell_rbu/image_type";
const char *rbu_v2_pkt_size_file = "/sys/devices/platform/dell_rbu/packet_size";
I guess that noawadays v2 applies, so the only path which needs to be
preserved is "/sys/class/firmware/dell_rbu". Is that correct? Is it the
same for srvadmin-hap?
Thanks,
--
Jean Delvare
On Sun, 26 Apr 2009 08:01:22 -0500, Matt Domsch wrote:
> On Sun, Apr 26, 2009 at 07:44:43AM -0500, Matt Domsch wrote:
> > On Sun, Apr 26, 2009 at 08:54:01AM +0200, Jean Delvare wrote:
> > > Matt, maybe you can help? Could you please point us to the source code
> > > of the dell firmware tool which relies on the dell_rbu driver?
> >
> > There's a git tree at:
> > http://linux.dell.com/git/?p=libsmbios.git
> >
> > which you can get via rsync at:
> > rsync -va rsync://linux.dell.com/git/libsmbios.git/
> >
> > the application is dellBiosUpdate, and the source is at this point in
> > the tree:
> >
> > http://linux.dell.com/git/?p=libsmbios.git;a=blob;f=src/bin/dellBiosUpdate.cpp;h=b32e157b7ceb0be0d53eed4a4d171d24fe6456a7;hb=HEAD
OK, thanks for the pointers.
> I'll note there is a second implementation that uses dell_rbu, and
> that is included in each and every "Dell Update Package" (aka DUP)
> BIOS update posted on support.dell.com for every Dell PowerEdge server
> for the past several years. Because the updater and they BIOS payload
> are packaged in the same package, a change which would require
> changing that updater will result in requiring Dell to spin and
> re-release hundreds of DUPs, which would not go over well... Thank
> you for being careful not to change the interface capriciously - it
> could have a huge impact on us.
I'll be careful, but you have to admit that duplicating utility code
like this is poor engineering from Dell :( I don't want to think of the
mess it will be if you ever have to fix a critical bug in that code.
--
Jean Delvare
Hi Kay,
On Sun, 26 Apr 2009 15:42:25 +0200, Kay Sievers wrote:
> On Sun, Apr 26, 2009 at 08:54, Jean Delvare <[email protected]> wrote:
> > But anyway, this isn't going to happen quickly, and I still believe we
> > should solve the namespace problem I reported.
>
> I think the real namespace problem lives well inside the i2c devices,
> not in the core. :) Sure, the firmware class is pretty dumb with its
> naming, but the root case is the way i2c devices are created.
>
> First, i2c stacks class devices, which it should not do, it should use
> a bus not a class, if it has device <-> children relations between
> different types.
Could as well be. I didn't invent it, it was that way when I took over
maintenance of the code. If i2c-adapters shouldn't be class devices,
let's change that... with all due carefulness for user-space tools that
may be relying on sysfs paths.
> Second, it names all the devices from different classes with the
> _same_ name, then puts them in a hierarchy,
Yes... but that shouldn't be (and isn't, really) a problem. We control
the hierarchy, and the fact that different class devices have the same
name doesn't matter at this point.
> and the tries to mix-in a
> third class, the firmware class, which also uses the _same_ name. That
> just asks for trouble. :)
Yes it is. But you will note that the naming of firmware class devices
is the root of the problem. It doesn't even allow for two different
firmware class devices off the same parent. Even the author of the code
put in a warning in the code saying that this would inevitably lead to
collisions. This really should have never be accepted in the kernel to
start with.
> > For firmware, it doesn't seem as legitimate, I can't really think of a
> > good reason to use the i2c-adapter as the parent for the firmware
> > device.
>
> Oh, I may miss something, I thought that is what you already do and
> causes the problem? I though you use the "adapter" to request the
> firmware? Otherwise I don't see how this can conflict.
Err, yes, this is exactly what we do, and also what I said just above.
Was I not clear maybe?
> You have the "adapter" and the "device" stacked, right?
> /sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0/i2c-0/
>
> Then you use the adapter:
> /sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0
> to request the firmware, which will try to create:
> /sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0/i2c-0/
>
> I thought you could use the "device" and not the "adapter" to request
> the firmware? Then you would get a "nice" chain of devices with all
> the same names, like :)
> /sys/devices/pci0000:00/0000:00:1f.3/i2c-adapter/i2c-0/i2c-0/i2c-0/
Ah, that's what you mean by "use the device". I read it the other way
around, using the _parent_ (bus) device:
/sys/devices/pci0000:00/0000:00:1f.3/firmware/0000:00:1f.3
Actually I have already sent a patch doing this change to dvb drivers:
http://www.spinics.net/lists/linux-media/msg04926.html
This still needs testing though.
What you propose wouldn't work, loading i2c-dev is optional so the
device may or may not be there. We definitely don't want to make all
the v4l/dvb stack depend on i2c-dev.
> I see the following options:
> - the i2c "device", not the "adapter" would request the firmware
See above.
> - the i2c-adapter converts to a bus, and is no longer a class
You apparently think this should be done either way?
> - i2c "adapters" and "devices" don't get the same duplicated name
I consider this a feature. Really, they are the same devices, the
device nodes are there for user-space access to the adapters.
> - if "adapters" can only ever have a single "device", the
> both could just be merged to a single instance, and maybe compat
> symlinks created
As said before, I have been considering this already. This would
certainly make things a little easier, but also less flexible (i2c-dev
could only be disabled at build time; if enabled it would always be
loaded, together with i2c-core). I'll think about it some more, in the
light of other i2c-core cleanups.
>
> - the firmware class adds a firmware-% prefix in the case the
> requesting device is a class device, and not the common case
> which is a bus device
That works but adds inconsistency which may in turn cause trouble in
the future (e.g. converting a bus device to class device or back would
break user-space compatibility.)
>
> - change the "glue dir" rules, which I would try to avoid, because
> we don't know who else might stack class devices of different
> classes, and might get surprised by this
I understand your reluctance, but still believe this is something we
need to fix. Sure there are alternative fixes or workarounds for the
problem at hand, but there may be other cases in the future.
Paradox of the day, either there are no other stacks of class devices,
and the change is safe and needless. Or there are other stacks of class
devices, and the change is needed but potentially risky. Oh well.
> What do you think?
I'd like to give a try to converting i2c-adapter devices to bus devices
rather than class devices, if you think that's how things should be.
Can you please explain to me how this would be done?
Thanks,
--
Jean Delvare
On Mon, Apr 27, 2009 at 10:27 AM, Jean Delvare <[email protected]> wrote:
> The question was: any change to _which_ paths exactly? Sysfs often
> offers several ways to reach the same object, and it matters to know
> which need to be preserved and which can change.
>
> const char *rbu_v2_fw_data_file = "/sys/class/firmware/dell_rbu/data";
> const char *rbu_v2_fw_load_file = "/sys/class/firmware/dell_rbu/loading";
> const char *rbu_v2_img_type_file = "/sys/devices/platform/dell_rbu/image_type";
> const char *rbu_v2_pkt_size_file = "/sys/devices/platform/dell_rbu/packet_size";
>
> I guess that noawadays v2 applies, so the only path which needs to be
> preserved is "/sys/class/firmware/dell_rbu". Is that correct? Is it the
> same for srvadmin-hap?
You are correct WRT libsmbios. For hapi, I've cc'd Doug who can tell
you about their code. I've done a quick grep of their code, and the
paths above look like the same ones he is using, but I could be wrong.
hapi/apilibrary/dchbas/linux/umhbaslx.c:#define
HBAS_RBU_SYSFS_NEW_DATAREAD_PFNAME "/sys/devices/platform/dell_rbu/data"
hapi/apilibrary/dchbas/linux/umhbaslx.c:#define
HBAS_RBU_SYSFS_NEW_IMAGETYPE_PFNAME "/sys/devices/platform/dell_rbu/image_type"
hapi/apilibrary/dchbas/linux/umhbaslx.c:#define
HBAS_RBU_SYSFS_NEW_PKTSIZE_PFNAME "/sys/devices/platform/dell_rbu/packet_size"
--
Michael
On Mon, Apr 27, 2009 at 05:30:05PM +0200, Jean Delvare wrote:
> On Sun, 26 Apr 2009 08:01:22 -0500, Matt Domsch wrote:
> > I'll note there is a second implementation that uses dell_rbu, and
> > that is included in each and every "Dell Update Package" (aka DUP)
> > BIOS update posted on support.dell.com for every Dell PowerEdge server
> > for the past several years. Because the updater and they BIOS payload
> > are packaged in the same package, a change which would require
> > changing that updater will result in requiring Dell to spin and
> > re-release hundreds of DUPs, which would not go over well... Thank
> > you for being careful not to change the interface capriciously - it
> > could have a huge impact on us.
>
> I'll be careful, but you have to admit that duplicating utility code
> like this is poor engineering from Dell :( I don't want to think of the
> mess it will be if you ever have to fix a critical bug in that code.
Yes, this is well-understood, and steps are in process to address this
model. http://linux.dell.com/firmware-tools is the preferred model,
with split inventory/execution components from firmware payloads.
Thanks,
Matt
--
Matt Domsch
Linux Technology Strategist, Dell Office of the CTO
linux.dell.com & http://www.dell.com/linux
On Mon, Apr 27, 2009 at 18:00, Jean Delvare <[email protected]> wrote:
> I'd like to give a try to converting i2c-adapter devices to bus devices
> rather than class devices, if you think that's how things should be.
> Can you please explain to me how this would be done?
Register a "i2c" bus_type with the core, and instead of assigning
dev.class = class, you assign dev.bus = bus to the devices you
register, that should work, if there is nothing more complicated going
on in the background.
Thanks,
Kay
Hi Kay,
On Mon, 27 Apr 2009 23:57:40 +0200, Kay Sievers wrote:
> On Mon, Apr 27, 2009 at 18:00, Jean Delvare <[email protected]> wrote:
>
> > I'd like to give a try to converting i2c-adapter devices to bus devices
> > rather than class devices, if you think that's how things should be.
> > Can you please explain to me how this would be done?
>
> Register a "i2c" bus_type with the core, and instead of assigning
> dev.class = class, you assign dev.bus = bus to the devices you
> register, that should work, if there is nothing more complicated going
> on in the background.
Err, I'm confused. We _already_ have an "i2c" bus type, and we already
assign dev.bus = &i2c_bus_type, but for i2c devices (or slaves if you
prefer), not adapters (masters). Doing the same for adapters (the
parents) and devices (the children) looks totally wrong to me.
Are you really certain that i2c-adapters should be bus devices rather
than class devices?
--
Jean Delvare
On Mon, 27 Apr 2009 12:23:30 -0500, Michael Brown wrote:
> On Mon, Apr 27, 2009 at 10:27 AM, Jean Delvare <[email protected]> wrote:
> > The question was: any change to _which_ paths exactly? Sysfs often
> > offers several ways to reach the same object, and it matters to know
> > which need to be preserved and which can change.
> >
> > const char *rbu_v2_fw_data_file = "/sys/class/firmware/dell_rbu/data";
> > const char *rbu_v2_fw_load_file = "/sys/class/firmware/dell_rbu/loading";
> > const char *rbu_v2_img_type_file = "/sys/devices/platform/dell_rbu/image_type";
> > const char *rbu_v2_pkt_size_file = "/sys/devices/platform/dell_rbu/packet_size";
> >
> > I guess that noawadays v2 applies, so the only path which needs to be
> > preserved is "/sys/class/firmware/dell_rbu". Is that correct? Is it the
> > same for srvadmin-hap?
>
> You are correct WRT libsmbios. For hapi, I've cc'd Doug who can tell
> you about their code. I've done a quick grep of their code, and the
> paths above look like the same ones he is using, but I could be wrong.
>
> hapi/apilibrary/dchbas/linux/umhbaslx.c:#define
> HBAS_RBU_SYSFS_NEW_DATAREAD_PFNAME "/sys/devices/platform/dell_rbu/data"
> hapi/apilibrary/dchbas/linux/umhbaslx.c:#define
> HBAS_RBU_SYSFS_NEW_IMAGETYPE_PFNAME "/sys/devices/platform/dell_rbu/image_type"
> hapi/apilibrary/dchbas/linux/umhbaslx.c:#define
> HBAS_RBU_SYSFS_NEW_PKTSIZE_PFNAME "/sys/devices/platform/dell_rbu/packet_size"
This doesn't make use of the firmware class at all, so I guess it won't
be affected by any change to the firmware class?
--
Jean Delvare
On Tue, Apr 28, 2009 at 10:08, Jean Delvare <[email protected]> wrote:
> On Mon, 27 Apr 2009 23:57:40 +0200, Kay Sievers wrote:
>> Register a "i2c" bus_type with the core, and instead of assigning
>> dev.class = class, you assign dev.bus = bus to the devices you
>> register, that should work, if there is nothing more complicated going
>> on in the background.
>
> Err, I'm confused. We _already_ have an "i2c" bus type, and we already
> assign dev.bus = &i2c_bus_type, but for i2c devices (or slaves if you
> prefer), not adapters (masters). Doing the same for adapters (the
> parents) and devices (the children) looks totally wrong to me.
>
> Are you really certain that i2c-adapters should be bus devices rather
> than class devices?
i2c seem to do things nothing else is doing, so I can not really be
certain. :) But I still think it would be nice to do that, if the
adapters have child devices.
It is unusual to stack class devices of different classes. The common
model would be to put the adapters, and all other possible device
types to the already existing "i2c" bus, and distinguish them by name,
and internally by "struct device_type" if needed.
Like USB puts usb-interface, usb-device, root-hub under the "usb"
bus_type, or SCSI puts host, target, lun under the "scsi" bus. All
these different devices build the tree of the core devices of a
specific subsystem.
Any possible class devices would only be leaves in these trees, which
do not have any interconnection between the individual class devices.
Thanks,
Kay
On Tue, Apr 28, 2009 at 19:39, Doug Warzecha <[email protected]> wrote:
> /sys/class/firmware/dell_rbu/loading
> /sys/class/firmware/dell_rbu/data
These pathes only ever exist for a fraction of a second, during a
kernel initiated firmware request. This request is usually only
handled by the udev firmware loader script, and not interesting for
anything else. Are you replacing the udev firmware script here? Are
you sure you do something else here?
Thanks,
Kay
On Tue, Apr 28, 2009 at 10:10:20AM +0200, Jean Delvare wrote:
> On Mon, 27 Apr 2009 12:23:30 -0500, Michael Brown wrote:
> > On Mon, Apr 27, 2009 at 10:27 AM, Jean Delvare <[email protected]> wrote:
> > > The question was: any change to _which_ paths exactly? Sysfs often
> > > offers several ways to reach the same object, and it matters to know
> > > which need to be preserved and which can change.
> > >
> > > const char *rbu_v2_fw_data_file = "/sys/class/firmware/dell_rbu/data";
> > > const char *rbu_v2_fw_load_file = "/sys/class/firmware/dell_rbu/loading";
> > > const char *rbu_v2_img_type_file = "/sys/devices/platform/dell_rbu/image_type";
> > > const char *rbu_v2_pkt_size_file = "/sys/devices/platform/dell_rbu/packet_size";
> > >
> > > I guess that noawadays v2 applies, so the only path which needs to be
> > > preserved is "/sys/class/firmware/dell_rbu". Is that correct? Is it the
> > > same for srvadmin-hap?
> >
> > You are correct WRT libsmbios. For hapi, I've cc'd Doug who can tell
> > you about their code. I've done a quick grep of their code, and the
> > paths above look like the same ones he is using, but I could be wrong.
> >
> > hapi/apilibrary/dchbas/linux/umhbaslx.c:#define
> > HBAS_RBU_SYSFS_NEW_DATAREAD_PFNAME "/sys/devices/platform/dell_rbu/data"
> > hapi/apilibrary/dchbas/linux/umhbaslx.c:#define
> > HBAS_RBU_SYSFS_NEW_IMAGETYPE_PFNAME "/sys/devices/platform/dell_rbu/image_type"
> > hapi/apilibrary/dchbas/linux/umhbaslx.c:#define
> > HBAS_RBU_SYSFS_NEW_PKTSIZE_PFNAME "/sys/devices/platform/dell_rbu/packet_size"
>
> This doesn't make use of the firmware class at all, so I guess it won't
> be affected by any change to the firmware class?
>
Dell makes use of the following dell_rbu paths:
/sys/class/firmware/dell_rbu/loading
/sys/class/firmware/dell_rbu/data
/sys/devices/platform/dell_rbu/data
/sys/devices/platform/dell_rbu/image_type
/sys/devices/platform/dell_rbu/packet_size
Doug
Hi Doug,
On Tue, 28 Apr 2009 12:39:29 -0500, Doug Warzecha wrote:
> On Tue, Apr 28, 2009 at 10:10:20AM +0200, Jean Delvare wrote:
> > On Mon, 27 Apr 2009 12:23:30 -0500, Michael Brown wrote:
> > > On Mon, Apr 27, 2009 at 10:27 AM, Jean Delvare <[email protected]> wrote:
> > > > The question was: any change to _which_ paths exactly? Sysfs often
> > > > offers several ways to reach the same object, and it matters to know
> > > > which need to be preserved and which can change.
> > > >
> > > > const char *rbu_v2_fw_data_file = "/sys/class/firmware/dell_rbu/data";
> > > > const char *rbu_v2_fw_load_file = "/sys/class/firmware/dell_rbu/loading";
> > > > const char *rbu_v2_img_type_file = "/sys/devices/platform/dell_rbu/image_type";
> > > > const char *rbu_v2_pkt_size_file = "/sys/devices/platform/dell_rbu/packet_size";
> > > >
> > > > I guess that noawadays v2 applies, so the only path which needs to be
> > > > preserved is "/sys/class/firmware/dell_rbu". Is that correct? Is it the
> > > > same for srvadmin-hap?
> > >
> > > You are correct WRT libsmbios. For hapi, I've cc'd Doug who can tell
> > > you about their code. I've done a quick grep of their code, and the
> > > paths above look like the same ones he is using, but I could be wrong.
> > >
> > > hapi/apilibrary/dchbas/linux/umhbaslx.c:#define
> > > HBAS_RBU_SYSFS_NEW_DATAREAD_PFNAME "/sys/devices/platform/dell_rbu/data"
> > > hapi/apilibrary/dchbas/linux/umhbaslx.c:#define
> > > HBAS_RBU_SYSFS_NEW_IMAGETYPE_PFNAME "/sys/devices/platform/dell_rbu/image_type"
> > > hapi/apilibrary/dchbas/linux/umhbaslx.c:#define
> > > HBAS_RBU_SYSFS_NEW_PKTSIZE_PFNAME "/sys/devices/platform/dell_rbu/packet_size"
> >
> > This doesn't make use of the firmware class at all, so I guess it won't
> > be affected by any change to the firmware class?
> >
>
> Dell makes use of the following dell_rbu paths:
>
> /sys/class/firmware/dell_rbu/loading
> /sys/class/firmware/dell_rbu/data
> /sys/devices/platform/dell_rbu/data
> /sys/devices/platform/dell_rbu/image_type
> /sys/devices/platform/dell_rbu/packet_size
Thanks for the clarification, which confirms my earlier findings.
--
Jean Delvare
On Tue, Apr 28, 2009 at 07:46:57PM +0200, Kay Sievers wrote:
> On Tue, Apr 28, 2009 at 19:39, Doug Warzecha <[email protected]> wrote:
>
> > /sys/class/firmware/dell_rbu/loading
> > /sys/class/firmware/dell_rbu/data
>
> These pathes only ever exist for a fraction of a second, during a
> kernel initiated firmware request. This request is usually only
> handled by the udev firmware loader script, and not interesting for
> anything else. Are you replacing the udev firmware script here? Are
> you sure you do something else here?
A Dell library writes data to those paths when a Dell BIOS Update Package is run and requests the BIOS image to be loaded into memory.
Doug
On Tue, Apr 28, 2009 at 23:58, Doug Warzecha <[email protected]> wrote:
> On Tue, Apr 28, 2009 at 07:46:57PM +0200, Kay Sievers wrote:
>> On Tue, Apr 28, 2009 at 19:39, Doug Warzecha <[email protected]> wrote:
>>
>> > /sys/class/firmware/dell_rbu/loading
>> > /sys/class/firmware/dell_rbu/data
>>
>> These pathes only ever exist for a fraction of a second, during a
>> kernel initiated firmware request. This request is usually only
>> handled by the udev firmware loader script, and not interesting for
>> anything else. Are you replacing the udev firmware script here? Are
>> you sure you do something else here?
>
> A Dell library writes data to those paths when a Dell BIOS Update Package is run and requests the BIOS image to be loaded into memory.
How do you do this? These paths exist for a short time only. How do
you get udev out of your way handling the firmware request for you?
How do you plug into the event processing with a custom tool?
Thanks,
Kay
On Tue, Apr 28, 2009 at 8:42 PM, Kay Sievers <[email protected]> wrote:
> On Tue, Apr 28, 2009 at 23:58, Doug Warzecha <[email protected]> wrote:
>> On Tue, Apr 28, 2009 at 07:46:57PM +0200, Kay Sievers wrote:
>>> On Tue, Apr 28, 2009 at 19:39, Doug Warzecha <[email protected]> wrote:
>>>
>>> > /sys/class/firmware/dell_rbu/loading
>>> > /sys/class/firmware/dell_rbu/data
>>>
>>> These pathes only ever exist for a fraction of a second, during a
>>> kernel initiated firmware request. This request is usually only
>>> handled by the udev firmware loader script, and not interesting for
>>> anything else. Are you replacing the udev firmware script here? Are
>>> you sure you do something else here?
>>
>> A Dell library writes data to those paths when a Dell BIOS Update Package is run and requests the BIOS image to be loaded into memory.
>
> How do you do this? These paths exist for a short time only. How do
> you get udev out of your way handling the firmware request for you?
> How do you plug into the event processing with a custom tool?
Overall, the interface is pretty pathetic because it doesnt fit our
use case *at all*, but way back in the day that is what we had to do
to get it reviewed and approved for merge upstream. At least it is
better than the earlier versions where we had to do a separate
request_firmware() for every 4k packet (and spin waiting for files to
disappear/reappear)
All this code is in libsmbios(*) if you want to see the ugly details,
but the short version is this:
1) we poke the image_type file and packet_size files with appropriate
data, which causes the driver to do a request firmware
2) spin until request_firmware creates the files we need
2) loop over our file packet_size bytes at a time writing data into
the data file
3) set loading to 0 when done
*) src/python/libsmbios_c/rbu_update.py
The code basically "packetizes" the bios update into page-sized chunks
with a header so BIOS can find them and reassemble them for the final
update.
For older systems (monolithic mode), the kernel code has to allocate
physically contiguous region to hold the entire firmware image.
--
Michael
On Wed, Apr 29, 2009 at 05:19, Michael Brown <[email protected]> wrote:
> On Tue, Apr 28, 2009 at 8:42 PM, Kay Sievers <[email protected]> wrote:
>> On Tue, Apr 28, 2009 at 23:58, Doug Warzecha <[email protected]> wrote:
>>> On Tue, Apr 28, 2009 at 07:46:57PM +0200, Kay Sievers wrote:
>>>> On Tue, Apr 28, 2009 at 19:39, Doug Warzecha <[email protected]> wrote:
>>>>
>>>> > /sys/class/firmware/dell_rbu/loading
>>>> > /sys/class/firmware/dell_rbu/data
>>>>
>>>> These pathes only ever exist for a fraction of a second, during a
>>>> kernel initiated firmware request. This request is usually only
>>>> handled by the udev firmware loader script, and not interesting for
>>>> anything else. Are you replacing the udev firmware script here? Are
>>>> you sure you do something else here?
>>>
>>> A Dell library writes data to those paths when a Dell BIOS Update Package is run and requests the BIOS image to be loaded into memory.
>>
>> How do you do this? These paths exist for a short time only. How do
>> you get udev out of your way handling the firmware request for you?
>> How do you plug into the event processing with a custom tool?
>
> Overall, the interface is pretty pathetic because it doesnt fit our
> use case *at all*, but way back in the day that is what we had to do
> to get it reviewed and approved for merge upstream. At least it is
> better than the earlier versions where we had to do a separate
> request_firmware() for every 4k packet (and spin waiting for files to
> disappear/reappear)
It handles huge files just fine, since the beginning. The space is
realloced in the kernel. I have a modem which loads 200kb files since
years.
> All this code is in libsmbios(*) if you want to see the ugly details,
> but the short version is this:
>
> 1) we poke the image_type file and packet_size files with appropriate
> data, which causes the driver to do a request firmware
> 2) spin until request_firmware creates the files we need
This is totally insane!
> 2) loop over our file packet_size bytes at a time writing data into
> the data file
> 3) set loading to 0 when done
>
> *) src/python/libsmbios_c/rbu_update.py
>
> The code basically "packetizes" the bios update into page-sized chunks
> with a header so BIOS can find them and reassemble them for the final
> update.
>
> For older systems (monolithic mode), the kernel code has to allocate
> physically contiguous region to hold the entire firmware image.
Yeah, maybe you can not use the in-kernel firmware-request filled
memory. But you can copy it just fine, once it is in the kernel. There
is no reason to fiddle around with chunks here in userspace.
How do you make sure udev does not handle the request at the same
time? This interface is not public at all, on a general system. As
long as udev is active you can not use it at all. Udev will run at the
same time and write to the same files, and even cancel the request if
the requested file is not found. How does your software cope with
that?
Thanks,
Kay
Hi Kay,
On Tue, 28 Apr 2009 14:36:27 +0200, Kay Sievers wrote:
> On Tue, Apr 28, 2009 at 10:08, Jean Delvare <[email protected]> wrote:
> > On Mon, 27 Apr 2009 23:57:40 +0200, Kay Sievers wrote:
>
> >> Register a "i2c" bus_type with the core, and instead of assigning
> >> dev.class = class, you assign dev.bus = bus to the devices you
> >> register, that should work, if there is nothing more complicated going
> >> on in the background.
> >
> > Err, I'm confused. We _already_ have an "i2c" bus type, and we already
> > assign dev.bus = &i2c_bus_type, but for i2c devices (or slaves if you
> > prefer), not adapters (masters). Doing the same for adapters (the
> > parents) and devices (the children) looks totally wrong to me.
> >
> > Are you really certain that i2c-adapters should be bus devices rather
> > than class devices?
>
> i2c seem to do things nothing else is doing, so I can not really be
> certain. :) But I still think it would be nice to do that, if the
> adapters have child devices.
>
> It is unusual to stack class devices of different classes. The common
> model would be to put the adapters, and all other possible device
> types to the already existing "i2c" bus, and distinguish them by name,
> and internally by "struct device_type" if needed.
>
> Like USB puts usb-interface, usb-device, root-hub under the "usb"
> bus_type, or SCSI puts host, target, lun under the "scsi" bus. All
> these different devices build the tree of the core devices of a
> specific subsystem.
>
> Any possible class devices would only be leaves in these trees, which
> do not have any interconnection between the individual class devices.
OK, I see the idea. I may give it a try, but I'd rather get rid of the
legacy i2c binding model first. One subsystem-wide cleanup at a time ;)
Thanks for the clarification!
--
Jean Delvare
On Wed, Apr 29, 2009 at 5:30 AM, Kay Sievers <[email protected]> wrote:
>> Overall, the interface is pretty pathetic because it doesnt fit our
>> use case *at all*, but way back in the day that is what we had to do
>> to get it reviewed and approved for merge upstream. At least it is
>> better than the earlier versions where we had to do a separate
>> request_firmware() for every 4k packet (and spin waiting for files to
>> disappear/reappear)
>
> It handles huge files just fine, since the beginning. The space is
> realloced in the kernel. I have a modem which loads 200kb files since
> years.
Sorry, to be brief in my email I probably didnt give quite enough
info. There are two types of bios update: monolithic and packetized.
The monolithic mode requires *physically* contiguous memory below the
4GB boundary, usually 1MB worth. This is unbelievably hard to get
normally, and is almost definitely different from your modem upload
which almost certainly just needs a virtually contiguous 200kB.
Packetized mode lifts the physically-contiguous restriction and just
has to have page-aligned packets.
>
>> All this code is in libsmbios(*) if you want to see the ugly details,
>> but the short version is this:
>>
>> 1) we poke the image_type file and packet_size files with appropriate
>> data, which causes the driver to do a request firmware
>> 2) spin until request_firmware creates the files we need
>
> This is totally insane!
Yes, that was the point. :)
>
>> 2) loop over our file packet_size bytes at a time writing data into
>> the data file
>> 3) set loading to 0 when done
>>
>> *) src/python/libsmbios_c/rbu_update.py
>>
>> The code basically "packetizes" the bios update into page-sized chunks
>> with a header so BIOS can find them and reassemble them for the final
>> update.
>>
>> For older systems (monolithic mode), the kernel code has to allocate
>> physically contiguous region to hold the entire firmware image.
>
> Yeah, maybe you can not use the in-kernel firmware-request filled
> memory. But you can copy it just fine, once it is in the kernel. There
> is no reason to fiddle around with chunks here in userspace.
Pretty much describes exactly what dell_rbu does, except that the
in-kernel packetizing code was rejected on our first submission, so we
moved it into userspace.
> How do you make sure udev does not handle the request at the same
> time? This interface is not public at all, on a general system. As
> long as udev is active you can not use it at all. Udev will run at the
> same time and write to the same files, and even cancel the request if
> the requested file is not found. How does your software cope with
> that?
That is a very interesting question, as I've never seen udev do
anything to interfere with our process. We dont do anything special to
disable udev. Like I said above, we scribble some values to one file,
wait for the request_firmware() files to appear and write stuff to
them when they do. Udev hasnt ever tried to handle this that I have
seen, especially since we dont have any appropriate files sitting
around that udev would ever have access to in order to feed
request_firmware().
--
Michael
On Wed, Apr 29, 2009 at 19:00, Michael Brown <[email protected]> wrote:
> On Wed, Apr 29, 2009 at 5:30 AM, Kay Sievers <[email protected]> wrote:
>> Yeah, maybe you can not use the in-kernel firmware-request filled
>> memory. But you can copy it just fine, once it is in the kernel. There
>> is no reason to fiddle around with chunks here in userspace.
>
> Pretty much describes exactly what dell_rbu does, except that the
> in-kernel packetizing code was rejected on our first submission, so we
> moved it into userspace.
That's the wrong reason to reject anything like that. I mean, you
should not use that interface at all for stuff like that, but
rejecting it for that reason seem wrong.
>> How do you make sure udev does not handle the request at the same
>> time? This interface is not public at all, on a general system. As
>> long as udev is active you can not use it at all. Udev will run at the
>> same time and write to the same files, and even cancel the request if
>> the requested file is not found. How does your software cope with
>> that?
>
> That is a very interesting question, as I've never seen udev do
> anything to interfere with our process. We dont do anything special to
> disable udev. Like I said above, we scribble some values to one file,
> wait for the request_firmware() files to appear and write stuff to
> them when they do. Udev hasnt ever tried to handle this that I have
> seen, especially since we dont have any appropriate files sitting
> around that udev would ever have access to in order to feed
> request_firmware().
Udev runs exactly at the time you create the device, and is not
unlikely faster than your polling, at least not predictable slower.
And it will cancel all requests which can not be fulfilled by udev
itself.
I strongly recommend switching to a different solution, you can not
use the firmware_class interface on any udev system, the interface is
already taken, and it's a single-user interface the way it is
implemented today. That it seems to work for you, is pure luck, I
guess. :)
Thanks,
Kay
On Wed, Apr 29, 2009 at 2:10 PM, Kay Sievers <[email protected]> wrote:
> Udev runs exactly at the time you create the device, and is not
> unlikely faster than your polling, at least not predictable slower.
> And it will cancel all requests which can not be fulfilled by udev
> itself.
>
> I strongly recommend switching to a different solution, you can not
> use the firmware_class interface on any udev system, the interface is
> already taken, and it's a single-user interface the way it is
> implemented today. That it seems to work for you, is pure luck, I
> guess. :)
Is there a safe/easy way to tell udev that we will handle a particular
request? I cant say that I've ever seen any problems due to udev
cancelling a firmware request. In fact, if I manually trigger a
request using "echo" from the cmdline, I dont see udev take any action
with the dell_rbu device. eg (Fedora 10, udev-127-5.fc10):
# find /sys -name dell_rbu
/sys/devices/platform/dell_rbu
/sys/bus/platform/devices/dell_rbu
/sys/module/dell_rbu
[root@duo dell_rbu]# echo "init" > /sys/devices/platform/dell_rbu/image_type
[root@duo dell_rbu]# find /sys -name dell_rbu
/sys/devices/platform/dell_rbu
/sys/devices/platform/dell_rbu/firmware/dell_rbu
/sys/bus/platform/devices/dell_rbu
/sys/class/firmware/dell_rbu
/sys/module/dell_rbu
[root@duo dell_rbu]# ll /sys/class/firmware/dell_rbu/
total 0
-rw-r--r-- 1 root root 0 2009-04-29 16:25 data
lrwxrwxrwx 1 root root 0 2009-04-29 16:25 device -> ../../../dell_rbu
-rw-r--r-- 1 root root 4096 2009-04-29 16:25 loading
drwxr-xr-x 2 root root 0 2009-04-29 16:25 power
lrwxrwxrwx 1 root root 0 2009-04-29 16:25 subsystem ->
../../../../../class/firmware
-rw-r--r-- 1 root root 4096 2009-04-29 16:25 uevent
[root@duo dell_rbu]#
[root@duo dell_rbu]# echo 0 > /sys/class/firmware/dell_rbu/loading
[root@duo dell_rbu]#
[root@duo dell_rbu]# find /sys -name dell_rbu
/sys/devices/platform/dell_rbu
/sys/bus/platform/devices/dell_rbu
/sys/module/dell_rbu
I dont see any of the behaviour that you have talked about. If I let
it sit there for hours, it will stay at that state. It only closes up
the request_firmware() request when I echo 0 > loading.
At this point, we have had this interface in the upstream kernel.org
kernel since 2.6.14 and have a pretty huge legacy codebase that relies
on this behaviour. We need to make sure that the current behaviour
remains.
--
Michael
On Wed, Apr 29, 2009 at 23:28, Michael Brown <[email protected]> wrote:
> On Wed, Apr 29, 2009 at 2:10 PM, Kay Sievers <[email protected]> wrote:
>> Udev runs exactly at the time you create the device, and is not
>> unlikely faster than your polling, at least not predictable slower.
>> And it will cancel all requests which can not be fulfilled by udev
>> itself.
>>
>> I strongly recommend switching to a different solution, you can not
>> use the firmware_class interface on any udev system, the interface is
>> already taken, and it's a single-user interface the way it is
>> implemented today. That it seems to work for you, is pure luck, I
>> guess. :)
>
> Is there a safe/easy way to tell udev that we will handle a particular
> request?
No, not with the current interface.
> I cant say that I've ever seen any problems due to udev
> cancelling a firmware request. In fact, if I manually trigger a
> request using "echo" from the cmdline, I dont see udev take any action
> with the dell_rbu device. eg (Fedora 10, udev-127-5.fc10):
If you run:
udevmonitor --udev --env
at the same time, what does it say?
> I dont see any of the behaviour that you have talked about. If I let
> it sit there for hours, it will stay at that state. It only closes up
> the request_firmware() request when I echo 0 > loading.
Udev will run in the moment this sysfs device is created, and it
should trigger the removal of the device, if it does not find the
requested firmware file.
> At this point, we have had this interface in the upstream kernel.org
> kernel since 2.6.14 and have a pretty huge legacy codebase that relies
> on this behaviour. We need to make sure that the current behaviour
> remains.
If we are talking about the same thing, which I'm not really sure
anymore, then you can not be sure that this will work in the future.
You can not reliably take over requests for the firmware class with
custom tools while udev is running.
Kay
On Wed, 2009-04-29 at 23:53 +0200, Kay Sievers wrote:
> On Wed, Apr 29, 2009 at 23:28, Michael Brown <[email protected]> wrote:
> > I cant say that I've ever seen any problems due to udev
> > cancelling a firmware request. In fact, if I manually trigger a
> > request using "echo" from the cmdline, I dont see udev take any action
> > with the dell_rbu device. eg (Fedora 10, udev-127-5.fc10):
>
> If you run:
> udevmonitor --udev --env
> at the same time, what does it say?
>
> > I dont see any of the behaviour that you have talked about. If I let
> > it sit there for hours, it will stay at that state. It only closes up
> > the request_firmware() request when I echo 0 > loading.
>
> Udev will run in the moment this sysfs device is created, and it
> should trigger the removal of the device, if it does not find the
> requested firmware file.
drivers/firmware/dell_rbu.c does this:
req_firm_rc = request_firmware_nowait(THIS_MODULE,
FW_ACTION_NOHOTPLUG, "dell_rbu",
&rbu_device->dev, &context,
callbackfn_rbu);
I've not gone looking to verify, but FW_ACTION_NOHOTPLUG implies to me
that udev never sees a uevent for it.
On Thu, Apr 30, 2009 at 17:18, David Dillow <[email protected]> wrote:
> On Wed, 2009-04-29 at 23:53 +0200, Kay Sievers wrote:
>> On Wed, Apr 29, 2009 at 23:28, Michael Brown <[email protected]> wrote:
>> > I cant say that I've ever seen any problems due to udev
>> > cancelling a firmware request. In fact, if I manually trigger a
>> > request using "echo" from the cmdline, I dont see udev take any action
>> > with the dell_rbu device. eg (Fedora 10, udev-127-5.fc10):
>>
>> If you run:
>> udevmonitor --udev --env
>> at the same time, what does it say?
>>
>> > I dont see any of the behaviour that you have talked about. If I let
>> > it sit there for hours, it will stay at that state. It only closes up
>> > the request_firmware() request when I echo 0 > loading.
>>
>> Udev will run in the moment this sysfs device is created, and it
>> should trigger the removal of the device, if it does not find the
>> requested firmware file.
>
> drivers/firmware/dell_rbu.c does this:
> req_firm_rc = request_firmware_nowait(THIS_MODULE,
> FW_ACTION_NOHOTPLUG, "dell_rbu",
> &rbu_device->dev, &context,
> callbackfn_rbu);
>
> I've not gone looking to verify, but FW_ACTION_NOHOTPLUG implies to me
> that udev never sees a uevent for it.
Ah, I see. Pretty weird idea to do that with polling, should be better
some key that tells udev to ignore the event, and you could listen to
the events yourself, instead of checking for them in sysfs at a
specific path and fixed name. But looks fine:
if (uevent)
dev_set_uevent_suppress(f_dev, 0);
Thanks,
Kay