request_firmware() hotplug interface:
------------------------------------
Why:
---
Today, the most extended way to use firmware in the Linux kernel is linking
it statically in a header file. Which has political and technical issues:
1) Some firmware is not legal to redistribute.
2) The firmware occupies memory permanently, even though it often is just
used once.
3) Some people, like the Debian crowd, don't consider some firmware free
enough and remove entire drivers (e.g.: keyspan).
Notes:
-----
- Why OPTIONALLY caching the firmware in-kernel may be a good idea sometimes:
- If the device that needs the firmware is needed to access the
filesystem. When upon some error the device has to be reset and the
firmware reloaded, it won't be possible to get it from userspace.
e.g.:
- A diskless client with a network card that needs firmware.
- The filesystem is stored in a disk behind an scsi device
that needs firmware.
- On embedded systems (like install floppies) where there is no
userspace hotplug support, 'cp firmware_file /firmware/' can be
handy.
And the same device can be needed to access the filesystem or not depending
on the setup, so I think that the choice on what firmware to cache should
be left to userspace.
- Why register_firmware()+__init can be useful:
- For boot devices needing firmware.
- To make the transition easier:
The firmware can be declared __init and register_firmware()
called on module_init. Then the firmware is warranted to be
there even if "firmware hotplug userspace" is not there jet or
it doesn't jet provide the needed firmware.
Once the firmware is widely available in userspace, it can be
removed from the kernel. Or made optional (CONFIG_.*_FIRMWARE).
In either case, if firmware hotplug support is there, it can move the
firmware out of kernel memory into the real filesystem for later
usage (like the provided hotplug scripts do).
> How it works:
> - Driver calls request_firmware()
> - 'hotplug firmware' gets called with ACCTION=add
> - /sysfs/class/firmware/dev_name/{data,loading} show up.
>
> - echo 1 > /sysfs/class/firmware/dev_name/loading
> - cat whatever_fw > /sysfs/class/firmware/dev_name/data
> - echo 0 > /sysfs/class/firmware/dev_name/loading
>
> - The call to request_firmware() returns with the firmware in a
> memory buffer and the driver can finish loading.
> - Driver loads the firmware.
> - Driver calls release_firmware().
So, if I understand you correctly, RAM is only saved if a device
is hotpluggable and needs firmware only upon intial connection.
Which, if you do suspend to disk correctly, is no device.
And do I understand you correctly, you propose that request_firmware()
wait for the hotplug script to write the firmware to sysfs?
That means that request_firmware() is unusuable from the usual
probe() methods. You cannot kill a part of the kernel if a script
fails to perform correctly for some reason. Even worse, you
cannot detect the script terminating abnormally in that design.
You'd have to introduce some arbitrary timeout.
It seems to me that you introduce three new problems to get rid of
one old problem.
Regards
Oliver
On Thu, May 15, 2003 at 04:38:58PM -0400, Pavel Roskin wrote:
> On Thu, 15 May 2003, Manuel Estrada Sainz wrote:
>
> > This time, as Greg suggested, it is implemented on top of 'struct
> > class' and 'struct class_device' but the driver interface is the same
> > as last time.
>
> Nitpicking:
>
> - Please use max_t, not your own MAX.
I was really wondering why MAX/MIN had not being generalized :)
> - /s/jet/yet/g
> - firmware_loading_show() should be static
All three fixed.
> > - register_firmware can not be implemented without some form of
> > in-kernel firmware caching. And that is not implemented.
>
> I wrote this private already, but it needs to be said now. It's not
> _caching_ that is needed. What is needed is a filesystem that can be
> populated in the kernel binary.
I don't know that much about initramfs, but how about, firmware images
get included in the initramfs and copied into fwfs during early
userspace?
I'll call it 'persistence' instead of 'caching' from now on.
And I guess that 'blobfs' would be a much better name than 'fwfs'.
Meaning that it can be used to store any blob.
> Can I use this code to replace broken ACPI table (DSDT) I have in some of
> my systems? Can I use this code to load firmware into my SCSI adapter if
> I need it to access the only disk in the system? Can I use this code to
> program a network interface I'm going to use for root over NFS?
The interface would allow it, but some kind of persistence has to be
there to make it possible.
> > - fwfs could be used for firmware caching behind the scene
> > allowing register_firmware to be implemented and the other
> > uses. I could call it blobfs and make a subdirectory within
> > for firmware purposes.
>
> I don't understand that, but I admit that it may be the answer to my
> question. Again, "caching" is a wrong word, I believe.
- Rename 'fwfs' into 'blobfs'
- Use it as the persistence mechanism behind request_firmware()
- Use one level of hierarchy within the filesystem:
$BLOBFS_MNT/firmware/...
$BLOBFS_MNT/crypto_keys/...
...
I don't know if it would really be useful for crypto keys, but it
looked like a good sample :)
request_firmware() would then just search with in
$BLOBFS_MNT/firmware/
Regards
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
On Fri, May 16, 2003 at 10:07:31AM +0200, Oliver Neukum wrote:
>
> > How it works:
> > - Driver calls request_firmware()
> > - 'hotplug firmware' gets called with ACCTION=add
> > - /sysfs/class/firmware/dev_name/{data,loading} show up.
> >
> > - echo 1 > /sysfs/class/firmware/dev_name/loading
> > - cat whatever_fw > /sysfs/class/firmware/dev_name/data
> > - echo 0 > /sysfs/class/firmware/dev_name/loading
> >
> > - The call to request_firmware() returns with the firmware in a
> > memory buffer and the driver can finish loading.
> > - Driver loads the firmware.
> > - Driver calls release_firmware().
>
> So, if I understand you correctly, RAM is only saved if a device
> is hotpluggable and needs firmware only upon intial connection.
> Which, if you do suspend to disk correctly, is no device.
Hotpluggability is not required, it is the same for any module, which
gets loaded while the system is running. Drivers don't even need to be
aware of hotplug.
And adding some kind of persistence in the mixture so firmware can be
included in the kernel image and later discarded/reconsidered even
in-kernel drivers (meaning non modules) can benefit. Coordinating with
initramfs as Pavel suggested should bring best results in this case.
Also, the hotplug event happens every time you call request_firmware(),
not just on device load or upon initial connection. It is not the
regular "device plug event" it is an special 'firmware' event. For
example, on usb devices you would get two invocations of hotplug, one
'hotplug usb' and one 'hotplug firmware'.
In the case of suspending to disk, you would have to make sure that the
firmware for the device that holds the rest of the firmware is already
in fwfs or whatever persistence method gets finally implemented.
> And do I understand you correctly, you propose that request_firmware()
> wait for the hotplug script to write the firmware to sysfs?
Yes.
> That means that request_firmware() is unusuable from the usual
> probe() methods.
At least usb's probe() can sleep, but that is a good point. How about:
int request_firmware_nowait (
const char *name, const char *device, void *context,
void (*cont)(const struct firmware *fw, void context)
);
Then you can call request_firmware_nowait providing an appropriate
'cont' callback and 'context' pointer. Then when your callback gets
called with the firmware you finish device setup.
> You cannot kill a part of the kernel if a script fails to perform
> correctly for some reason.
Good point. Since it is easily solvable by hand:
echo 1 > /sysfs/class/firmware/dev_name/loading
echo 0 > /sysfs/class/firmware/dev_name/loading
I thought that it was OK. (I'll do the timeout)
> Even worse, you cannot detect the script terminating abnormally in
> that design.
Well, the device model doesn't provide that information :(
It would be great if it did.
Would a patch to wait for hotplug termination and provide termination
status be accepted?
Adding an 'struct completion' and 'int status' to the right place
should be just about it.
> You'd have to introduce some arbitrary timeout.
OK, I'll do that for now.
> It seems to me that you introduce three new problems to get rid of
> one old problem.
This is the kind of feedback I wanted, thanks a lot.
Let's see if I can remove all four problems now :)
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
Hi all,
On Thu, May 15, 2003 at 10:03:24PM +0200, Manuel Estrada Sainz wrote:
>
> - echo 1 > /sysfs/class/firmware/dev_name/loading
> - cat whatever_fw > /sysfs/class/firmware/dev_name/data
> - echo 0 > /sysfs/class/firmware/dev_name/loading
Why not doing that in open and require firmware data to contain
size information? Good firmware formats contain already size,
checksum and version information. Bad firmware can be wrapped to
get these. It should be made a requirement to contain at least a
size and a checksum.
To handle the big varieties of firmware formats, I would suggest
to either wrap all in user space or define 3 functions per
firmware format like the seq_file support. The thing is very
similiar, except that we read from user space instead of writing.
fw_begin_firmware_store()
fw_next_firmware_bytes()
fw_end_firmware_store()
fw_begin_firmware_store() gets at most a page of data and should
evaluate from this data, how much bytes it still needs from
user space. It will also setup a context and store it.
fw_next_firmware_bytes() will get passed more firmware bytes and
tells us again how much it still need. It will get passed the
context setup by fw_begin_firmware_store().
This function can also abort a download by returning "no bytes
needed anymore" and marking the firmware "invalid" in the
context, which fw_end_firmware_store() will use to return
"discard this firmware" to the firmware fs.
Also this function is not really necessary, if we set the
filesize of the firmware (truncate()) in firmware fs after
fw_begin_firmware_store() and let the VFS do its magic.
fw_end_firmware_store() will be called, after user space closed
the file descriptor (Note: This will handle SIGKILL also). It
must decide, whether the downloaded firmware is valid and will
be stored and can be used or will be discarded. It gets passed
the context setup by fw_begin_firmware_store() and will free
it's resources, if not needed anymore.
After fw_end_firmware_store(), the firmware can be downloaded to
the device (not before!).
This is much simpler, then it sounds. The only problems are:
1. getting the size of the firmware to be downloaded
a) firmware has always the same size, so this is a constant
b) firmware has size encoded -> use this
c) firmware size is file size -> need to wrap this to be like 1.b)
2. decide, whether the firmware is valid
a) checksum
b) versions
c) none -> trust or wrap to match 2.a) and/or 2.b)
What do you think?
Defining the prototypes and finding the places to hook into is
left as an exercise to the reader ;-)
The current idea (special file sytem) is great, but the interface
to the driver is not really perfect.
Regards
Ingo Oeser
Am Freitag, 16. Mai 2003 11:56 schrieb Manuel Estrada Sainz:
> On Fri, May 16, 2003 at 10:07:31AM +0200, Oliver Neukum wrote:
> > > How it works:
> > > - Driver calls request_firmware()
> > > - 'hotplug firmware' gets called with ACCTION=add
> > > - /sysfs/class/firmware/dev_name/{data,loading} show up.
> > >
> > > - echo 1 > /sysfs/class/firmware/dev_name/loading
> > > - cat whatever_fw > /sysfs/class/firmware/dev_name/data
> > > - echo 0 > /sysfs/class/firmware/dev_name/loading
> > >
> > > - The call to request_firmware() returns with the firmware in a
> > > memory buffer and the driver can finish loading.
> > > - Driver loads the firmware.
> > > - Driver calls release_firmware().
> >
> > So, if I understand you correctly, RAM is only saved if a device
> > is hotpluggable and needs firmware only upon intial connection.
> > Which, if you do suspend to disk correctly, is no device.
>
> Hotpluggability is not required, it is the same for any module, which
> gets loaded while the system is running. Drivers don't even need to be
> aware of hotplug.
In that case they can contain the firmware and mark it __init.
They need no RAM. They can even mark the code needed to put
firmware into the device as __init.
> And adding some kind of persistence in the mixture so firmware can be
> included in the kernel image and later discarded/reconsidered even
> in-kernel drivers (meaning non modules) can benefit. Coordinating with
> initramfs as Pavel suggested should bring best results in this case.
>
> Also, the hotplug event happens every time you call request_firmware(),
> not just on device load or upon initial connection. It is not the
> regular "device plug event" it is an special 'firmware' event. For
> example, on usb devices you would get two invocations of hotplug, one
> 'hotplug usb' and one 'hotplug firmware'.
>
> In the case of suspending to disk, you would have to make sure that the
> firmware for the device that holds the rest of the firmware is already
> in fwfs or whatever persistence method gets finally implemented.
How and what is the benefit? If you go low on battery you have to suspend,
there's no choice. This means that you have to have it in RAM always.
> > And do I understand you correctly, you propose that request_firmware()
> > wait for the hotplug script to write the firmware to sysfs?
>
> Yes.
>
> > That means that request_firmware() is unusuable from the usual
> > probe() methods.
>
> At least usb's probe() can sleep, but that is a good point. How about:
>
> int request_firmware_nowait (
> const char *name, const char *device, void *context,
> void (*cont)(const struct firmware *fw, void context)
> );
>
> Then you can call request_firmware_nowait providing an appropriate
> 'cont' callback and 'context' pointer. Then when your callback gets
> called with the firmware you finish device setup.
In this form unworkable. Removing a module could kill the machine.
That scheme requires that drivers formally register and unregister
with fwfs and provide module pointers.
An awful lot of overhead.
> > You cannot kill a part of the kernel if a script fails to perform
> > correctly for some reason.
>
> Good point. Since it is easily solvable by hand:
>
> echo 1 > /sysfs/class/firmware/dev_name/loading
> echo 0 > /sysfs/class/firmware/dev_name/loading
>
> I thought that it was OK. (I'll do the timeout)
No, it isn't. These writes must require CAP_HARDWARE, thus
is no good.
> > Even worse, you cannot detect the script terminating abnormally in
> > that design.
>
> Well, the device model doesn't provide that information :(
>
> It would be great if it did.
>
> Would a patch to wait for hotplug termination and provide termination
> status be accepted?
No, you must not wait for user space.
> Adding an 'struct completion' and 'int status' to the right place
> should be just about it.
>
> > You'd have to introduce some arbitrary timeout.
>
> OK, I'll do that for now.
>
> > It seems to me that you introduce three new problems to get rid of
> > one old problem.
>
> This is the kind of feedback I wanted, thanks a lot.
Sorry.
Regards
Oliver
On Fri, May 16, 2003 at 03:13:52PM +0200, Ingo Oeser wrote:
> Hi all,
>
> On Thu, May 15, 2003 at 10:03:24PM +0200, Manuel Estrada Sainz wrote:
> >
> > - echo 1 > /sysfs/class/firmware/dev_name/loading
> > - cat whatever_fw > /sysfs/class/firmware/dev_name/data
> > - echo 0 > /sysfs/class/firmware/dev_name/loading
>
> Why not doing that in open and require firmware data to contain
> size information? Good firmware formats contain already size,
> checksum and version information.
The reason for such an interface is to be able to do it on top of sysfs
and so that drivers can implement the same interface on themselfs.
They just need to export the device's firmware memory as 'data' via a
class_device of class firmware_class.
They also need to know when the load starts to set it up and when the
load finishes to get the device going again with the new firmware, for
that is 'loading' when they get a '1' they have to get ready for the
load and when they get a '0' they have to get the device back working.
Doing that, they would get the hotplug event and compatible hotplug
support for free. Coping the firmware image in the appropriate
directory would be the only userspace setup needed.
Feedback on the interface (data/loading) would also be welcomed, I am
probably biased by the hardware I have at hand.
It is currently not possible, but the interface means to allow it.
> Bad firmware can be wrapped to get these. It should be made a
> requirement to contain at least a size and a checksum.
That could be done, but I am not sure the complexity is worth it. If a
privileged user wants to shoot on his foot, he can do so may other
ways already.
> To handle the big varieties of firmware formats, I would suggest
> to either wrap all in user space or define 3 functions per
> firmware format like the seq_file support. The thing is very
> similiar, except that we read from user space instead of writing.
>
> fw_begin_firmware_store()
> fw_next_firmware_bytes()
> fw_end_firmware_store()
>
> fw_begin_firmware_store() gets at most a page of data and should
> evaluate from this data, how much bytes it still needs from
> user space. It will also setup a context and store it.
>
> fw_next_firmware_bytes() will get passed more firmware bytes and
> tells us again how much it still need. It will get passed the
> context setup by fw_begin_firmware_store().
>
> This function can also abort a download by returning "no bytes
> needed anymore" and marking the firmware "invalid" in the
> context, which fw_end_firmware_store() will use to return
> "discard this firmware" to the firmware fs.
>
> Also this function is not really necessary, if we set the
> filesize of the firmware (truncate()) in firmware fs after
> fw_begin_firmware_store() and let the VFS do its magic.
>
> fw_end_firmware_store() will be called, after user space closed
> the file descriptor (Note: This will handle SIGKILL also). It
> must decide, whether the downloaded firmware is valid and will
> be stored and can be used or will be discarded. It gets passed
> the context setup by fw_begin_firmware_store() and will free
> it's resources, if not needed anymore.
>
> After fw_end_firmware_store(), the firmware can be downloaded to
> the device (not before!).
>
> This is much simpler, then it sounds. The only problems are:
> 1. getting the size of the firmware to be downloaded
> a) firmware has always the same size, so this is a constant
> b) firmware has size encoded -> use this
> c) firmware size is file size -> need to wrap this to be like 1.b)
>
> 2. decide, whether the firmware is valid
> a) checksum
> b) versions
> c) none -> trust or wrap to match 2.a) and/or 2.b)
>
> What do you think?
I think that this is an overkill, the most I can see reasonable is
forcing an standard header on all firmware images with size/checksum.
And still I am not sure it is worth it.
> The current idea (special file sytem) is great, but the interface
> to the driver is not really perfect.
In which way is it not perfect?
Detailed criticism gives me the chance to try to get things better :)
Have a nice day
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
On Gwe, 2003-05-16 at 09:07, Oliver Neukum wrote:
> So, if I understand you correctly, RAM is only saved if a device
> is hotpluggable and needs firmware only upon intial connection.
> Which, if you do suspend to disk correctly, is no device.
Thats just because the interface is a little warped not the theory.
On a resume you need to reload firmware and you already handle
rediscovery on USB bus for example because the devices can change
On Fri, May 16, 2003 at 05:53:17PM +0200, Oliver Neukum wrote:
> Am Freitag, 16. Mai 2003 11:56 schrieb Manuel Estrada Sainz:
> > On Fri, May 16, 2003 at 10:07:31AM +0200, Oliver Neukum wrote:
> > > > How it works:
> > > > - Driver calls request_firmware()
> > > > - 'hotplug firmware' gets called with ACCTION=add
> > > > - /sysfs/class/firmware/dev_name/{data,loading} show up.
> > > >
> > > > - echo 1 > /sysfs/class/firmware/dev_name/loading
> > > > - cat whatever_fw > /sysfs/class/firmware/dev_name/data
> > > > - echo 0 > /sysfs/class/firmware/dev_name/loading
> > > >
> > > > - The call to request_firmware() returns with the firmware in a
> > > > memory buffer and the driver can finish loading.
> > > > - Driver loads the firmware.
> > > > - Driver calls release_firmware().
> > >
> > > So, if I understand you correctly, RAM is only saved if a device
> > > is hotpluggable and needs firmware only upon intial connection.
> > > Which, if you do suspend to disk correctly, is no device.
> >
> > Hotpluggability is not required, it is the same for any module, which
> > gets loaded while the system is running. Drivers don't even need to be
> > aware of hotplug.
>
> In that case they can contain the firmware and mark it __init.
> They need no RAM. They can even mark the code needed to put
> firmware into the device as __init.
There are issues with that approach:
- Code with firmware will not be accepted in the kernel, there are
already two drivers that are being hold back for that reason.
- Licensing issues.
- Memory consumption.
- "new" kernel policy.
- A device may need to reload firmware upon reset. And if you mark it
__init, it will not be there.
- If you include the firmware in the code you have to recompile the
kernel to update the firmware. Pavel, back me up on this one with the
ACPI tables issue.
> > And adding some kind of persistence in the mixture so firmware can be
> > included in the kernel image and later discarded/reconsidered even
> > in-kernel drivers (meaning non modules) can benefit. Coordinating with
> > initramfs as Pavel suggested should bring best results in this case.
> >
> > Also, the hotplug event happens every time you call request_firmware(),
> > not just on device load or upon initial connection. It is not the
> > regular "device plug event" it is an special 'firmware' event. For
> > example, on usb devices you would get two invocations of hotplug, one
> > 'hotplug usb' and one 'hotplug firmware'.
> >
> > In the case of suspending to disk, you would have to make sure that the
> > firmware for the device that holds the rest of the firmware is already
> > in fwfs or whatever persistence method gets finally implemented.
>
> How and what is the benefit? If you go low on battery you have to suspend,
> there's no choice. This means that you have to have it in RAM always.
If the device losses the firmware upon suspend, the driver will have to
reinitialize it as if it just got plugged, which somehow makes all
devices hotplugable.
If the driver uses request_firmware(), it doesn't need to handle any
special case, just initialize as usual and it will get the firmware
when it needs it.
If the device is needed to access the filesystem, some kind of
persistence will be needed, so the required firmware is already in
kernel space. But the driver doesn't need to care, it will just ask for
the firmware as usual.
Which brings me to another issue, the same device can be required to
access the filesystem or not:
- In a diskless client, it is the network card
- In a live-cd it is the cdrom drive
- In a multi disk system just one of them will be holding
required firmware.
So you can not decide at coding time, the latest at compile time, and
ideally at runtime (which is what I am trying to do).
> > At least usb's probe() can sleep, but that is a good point. How about:
> >
> > int request_firmware_nowait (
> > const char *name, const char *device, void *context,
> > void (*cont)(const struct firmware *fw, void context)
> > );
> >
> > Then you can call request_firmware_nowait providing an appropriate
> > 'cont' callback and 'context' pointer. Then when your callback gets
> > called with the firmware you finish device setup.
>
> In this form unworkable. Removing a module could kill the machine.
> That scheme requires that drivers formally register and unregister
> with fwfs and provide module pointers.
> An awful lot of overhead.
OK, how about:
int request_firmware_nowait (
struct module *module,
const char *name, const char *device, void *context,
void (*cont)(const struct firmware *fw, void context)
);
request_firmware code will try_module_get/module_put as needed.
Would that fix the issue?
> > > You cannot kill a part of the kernel if a script fails to perform
> > > correctly for some reason.
> >
> > Good point. Since it is easily solvable by hand:
> >
> > echo 1 > /sysfs/class/firmware/dev_name/loading
> > echo 0 > /sysfs/class/firmware/dev_name/loading
> >
> > I thought that it was OK. (I'll do the timeout)
>
> No, it isn't. These writes must require CAP_HARDWARE, thus
> is no good.
I'll do the timeout anyway, and make it configurable just in case.
> > > Even worse, you cannot detect the script terminating abnormally in
> > > that design.
> >
> > Well, the device model doesn't provide that information :(
> >
> > It would be great if it did.
> >
> > Would a patch to wait for hotplug termination and provide termination
> > status be accepted?
>
> No, you must not wait for user space.
And to get notified when userspace is done?
> > Adding an 'struct completion' and 'int status' to the right place
> > should be just about it.
> >
> > > You'd have to introduce some arbitrary timeout.
> >
> > OK, I'll do that for now.
> >
> > > It seems to me that you introduce three new problems to get rid of
> > > one old problem.
> >
> > This is the kind of feedback I wanted, thanks a lot.
>
> Sorry.
Don't be, this is still the kind of feedback I want. Weather I manage
or not, I get the chance to fix the issues.
Thanks
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
On Fri, May 16, 2003 at 05:53:17PM +0200, Oliver Neukum wrote:
> >
> > Hotpluggability is not required, it is the same for any module, which
> > gets loaded while the system is running. Drivers don't even need to be
> > aware of hotplug.
>
> In that case they can contain the firmware and mark it __init.
> They need no RAM. They can even mark the code needed to put
> firmware into the device as __init.
I think you missed a lot of the previous discussion. Many high
level kernel developpers (Jeff, Alan, Greg) have said 'niet' to binary
firmware linked with the kernel (unless there is source code
available). Please assume that all drivers currently including
firmware blobs in the kernel will need fixing and therefore should not
be taken as example.
The threads :
http://marc.theaimsgroup.com/?t=105222131600002&r=1&w=2
http://marc.theaimsgroup.com/?t=105045487300002&r=1&w=2
http://marc.theaimsgroup.com/?t=105181861200001&r=1&w=2
http://marc.theaimsgroup.com/?t=105295022700002&r=1&w=2
http://marc.theaimsgroup.com/?t=105302951900003&r=1&w=2
Yes, that's a lot to read, but this will explain the full
complexity of the situation.
> An awful lot of overhead.
It's not like we have a choice on this issue. The embedded
people are already grumbling.
> Regards
> Oliver
Jean
Am Freitag, 16. Mai 2003 18:09 schrieb Alan Cox:
> On Gwe, 2003-05-16 at 09:07, Oliver Neukum wrote:
> > So, if I understand you correctly, RAM is only saved if a device
> > is hotpluggable and needs firmware only upon intial connection.
> > Which, if you do suspend to disk correctly, is no device.
>
> Thats just because the interface is a little warped not the theory.
> On a resume you need to reload firmware and you already handle
> rediscovery on USB bus for example because the devices can change
Right. But the order of resumption is fixed by hardware needs.
So during resumption you cannot use block devices and therefore
not start a hotplug script. Or did I miss something?
Regards
Oliver
Am Freitag, 16. Mai 2003 20:49 schrieb Jean Tourrilhes:
> On Fri, May 16, 2003 at 05:53:17PM +0200, Oliver Neukum wrote:
> > > Hotpluggability is not required, it is the same for any module, which
> > > gets loaded while the system is running. Drivers don't even need to be
> > > aware of hotplug.
> >
> > In that case they can contain the firmware and mark it __init.
> > They need no RAM. They can even mark the code needed to put
> > firmware into the device as __init.
>
> I think you missed a lot of the previous discussion. Many high
> level kernel developpers (Jeff, Alan, Greg) have said 'niet' to binary
> firmware linked with the kernel (unless there is source code
> available). Please assume that all drivers currently including
> firmware blobs in the kernel will need fixing and therefore should not
> be taken as example.
> The threads :
> http://marc.theaimsgroup.com/?t=105222131600002&r=1&w=2
> http://marc.theaimsgroup.com/?t=105045487300002&r=1&w=2
> http://marc.theaimsgroup.com/?t=105181861200001&r=1&w=2
> http://marc.theaimsgroup.com/?t=105295022700002&r=1&w=2
> http://marc.theaimsgroup.com/?t=105302951900003&r=1&w=2
> Yes, that's a lot to read, but this will explain the full
> complexity of the situation.
It seems to be a whole lot of laywering not technical discussion IMHO.
> > An awful lot of overhead.
>
> It's not like we have a choice on this issue. The embedded
> people are already grumbling.
Please explain. Either firmware is in RAM or it is not.
Regards
Oliver
> > How and what is the benefit? If you go low on battery you have to
> > suspend, there's no choice. This means that you have to have it in RAM
> > always.
>
> If the device losses the firmware upon suspend, the driver will have to
> reinitialize it as if it just got plugged, which somehow makes all
> devices hotplugable.
So all firmware has to be permanently in RAM anyway?
> If the driver uses request_firmware(), it doesn't need to handle any
> special case, just initialize as usual and it will get the firmware
> when it needs it.
How or precisely, how do you know that it gets it when it needs
it? Are you planning to have a gray area where the kernel generates
a special user space before everything else gets woken?
> If the device is needed to access the filesystem, some kind of
> persistence will be needed, so the required firmware is already in
> kernel space. But the driver doesn't need to care, it will just ask for
> the firmware as usual.
>
> Which brings me to another issue, the same device can be required to
> access the filesystem or not:
> - In a diskless client, it is the network card
> - In a live-cd it is the cdrom drive
> - In a multi disk system just one of them will be holding
> required firmware.
> So you can not decide at coding time, the latest at compile time, and
> ideally at runtime (which is what I am trying to do).
How? You cannot page out memory during resumption.
You must not cause any access to disk during resumption.
> > > At least usb's probe() can sleep, but that is a good point. How about:
> > >
> > > int request_firmware_nowait (
> > > const char *name, const char *device, void *context,
> > > void (*cont)(const struct firmware *fw, void context)
> > > );
> > >
> > > Then you can call request_firmware_nowait providing an appropriate
> > > 'cont' callback and 'context' pointer. Then when your callback gets
> > > called with the firmware you finish device setup.
> >
> > In this form unworkable. Removing a module could kill the machine.
> > That scheme requires that drivers formally register and unregister
> > with fwfs and provide module pointers.
> > An awful lot of overhead.
>
> OK, how about:
>
> int request_firmware_nowait (
> struct module *module,
> const char *name, const char *device, void *context,
> void (*cont)(const struct firmware *fw, void context)
> );
>
> request_firmware code will try_module_get/module_put as needed.
>
> Would that fix the issue?
No, still no good. It means that you get a memory leak if you unload
a driver before firmware is provided. You need the ability to explicitely
cancel a request for firmware.
> > > > You cannot kill a part of the kernel if a script fails to perform
> > > > correctly for some reason.
> > >
> > > Good point. Since it is easily solvable by hand:
> > >
> > > echo 1 > /sysfs/class/firmware/dev_name/loading
> > > echo 0 > /sysfs/class/firmware/dev_name/loading
> > >
> > > I thought that it was OK. (I'll do the timeout)
> >
> > No, it isn't. These writes must require CAP_HARDWARE, thus
> > is no good.
>
> I'll do the timeout anyway, and make it configurable just in case.
>
> > > > Even worse, you cannot detect the script terminating abnormally in
> > > > that design.
> > >
> > > Well, the device model doesn't provide that information :(
> > >
> > > It would be great if it did.
> > >
> > > Would a patch to wait for hotplug termination and provide termination
> > > status be accepted?
> >
> > No, you must not wait for user space.
>
> And to get notified when userspace is done?
Not with that interface.
You'd need drivers to register both with their subsystem and
a firmware subsystem. But you cannot make device discovery
wait for user space.
You'd have to rewrite the probe method of all drivers that need
firmware.
Regards
Oliver
On Thu, May 15, 2003 at 10:03:24PM +0200, Manuel Estrada Sainz wrote:
> Hi all,
>
> This time, as Greg suggested, it is implemented on top of 'struct
> class' and 'struct class_device' but the driver interface is the same
> as last time.
First off, nice, this looks a lot better, good job.
> Attached:
> firmware.h
> firmware_class.c:
> The firmware support itself.
Can you just send this as a patch to the current kernel next time? It's
much easier to read and test with that way :)
> firmware_sample_driver.c:
> Sample code on how to use from drivers.
I didn't see this in the files you attached.
> hotplug:
> A simple hotplug replacement for testing.
> Makefile:
> The obvious.
> README:
> Still pertinent pieces from the previous round.
>
> How it works:
> - Driver calls request_firmware()
Yeah, I agree with your comment in the code, I think a struct device *
should be passed here. Or at least somewhere...
> - 'hotplug firmware' gets called with ACCTION=add
I don't see why you need to add a new environment variable in your
firmware_class_hotplug() call. What is the FIRMWARE variable for, if we
already have a device symlink back to the device that is asking for the
firmware? Oh, you don't have that :)
> - /sysfs/class/firmware/dev_name/{data,loading} show up.
If you pass a struct device to request_firmware(), then you get a
symlink to the device for free. You can also set the class_id to the
device bus_id, watching out for name collisions (bus_ids are only unique
per bus type, so different bus types can use the same bus id, but in
reality they rarely do.)
> - echo 1 > /sysfs/class/firmware/dev_name/loading
> - cat whatever_fw > /sysfs/class/firmware/dev_name/data
> - echo 0 > /sysfs/class/firmware/dev_name/loading
Nice, but can't you get rid of the loading file by just relying on
open() and close()? Oh wait, sysfs doesn't pass that down to you, hm,
looks like you need that info. But does the new binary interface in
sysfs that just got merged into the tree provide that info for you?
> - The call to request_firmware() returns with the firmware in a
> memory buffer and the driver can finish loading.
request_firmware() can't use a static struct class_device, like you have
it, in order to work properly for multiple calls to request_firmware()
at the same time by different drivers. Just create a new struct
class_device, and put it on a list, like I had to do for the tty class
code (and i2c_dev class code, but that isn't in the kernel to look at
yet...)
Other than those very minor tweaks, I like this interface, it's looking
very good. I wouldn't worry about any "checksum" calcuation crud, it's
up to the userspace tool dumping the firmware to the kernel to make sure
it's writing correct data, not the kernel.
thanks,
greg k-h
On Thu, May 15, 2003 at 04:38:58PM -0400, Pavel Roskin wrote:
> I wrote this private already, but it needs to be said now. It's not
> _caching_ that is needed. What is needed is a filesystem that can be
> populated in the kernel binary.
initramfs can do this today. It isn't the issue for this firmware
interface to solve. Same thing goes for the "resume the sleeping
device" arguement. That's not this code's issue.
> Can I use this code to replace broken ACPI table (DSDT) I have in some of
> my systems?
Hm, don't know the ACPI startup point in reference to when initramfs
gets uncompressed. But I think you might be safe.
> Can I use this code to load firmware into my SCSI adapter if
> I need it to access the only disk in the system?
With initramfs, yes.
> Can I use this code to program a network interface I'm going to use
> for root over NFS?
Again, with initramfs, yes.
thanks,
greg k-h
On Fri, May 16, 2003 at 11:49:20AM -0700, Jean Tourrilhes wrote:
>
> Many high level kernel developpers (Jeff, Alan, Greg) have said 'niet'
> to binary firmware linked with the kernel (unless there is source code
> available).
Excuse me, but I never said such a thing. I don't mind putting firmware
blobs into kernel drivers, been doing it for years :)
I just stated that _if_ we want to move firmware to userspace, why not
have a consistant interface that all kinds of drivers can use to
accomplish this. That is what has been done with this code.
thanks,
greg k-h
On Fri, May 16, 2003 at 03:36:24PM -0700, Greg KH wrote:
> On Thu, May 15, 2003 at 10:03:24PM +0200, Manuel Estrada Sainz wrote:
> > Hi all,
> >
> > This time, as Greg suggested, it is implemented on top of 'struct
> > class' and 'struct class_device' but the driver interface is the same
> > as last time.
>
> First off, nice, this looks a lot better, good job.
Thanks for the positive feedback.
> > Attached:
> > firmware.h
> > firmware_class.c:
> > The firmware support itself.
>
> Can you just send this as a patch to the current kernel next time? It's
> much easier to read and test with that way :)
The next one will come in that form.
> > firmware_sample_driver.c:
> > Sample code on how to use from drivers.
>
> I didn't see this in the files you attached.
Sorry, I thought I did, attached now.
> > hotplug:
> > A simple hotplug replacement for testing.
> > Makefile:
> > The obvious.
> > README:
> > Still pertinent pieces from the previous round.
> >
> > How it works:
> > - Driver calls request_firmware()
>
> Yeah, I agree with your comment in the code, I think a struct device *
> should be passed here. Or at least somewhere...
To make compatibility with 2.4 kernel easier, I think that I'll add a
new 'struct device *' parameter to request_firmware(). On 2.4 kernels
it can be an unused 'void *'. Does that sound too ugly?
> > - 'hotplug firmware' gets called with ACCTION=add
>
> I don't see why you need to add a new environment variable in your
> firmware_class_hotplug() call. What is the FIRMWARE variable for, if we
> already have a device symlink back to the device that is asking for the
> firmware? Oh, you don't have that :)
The same device can ask for different firmware images.
Orinoco USB devices have one image for the USB<->PCMCIA bridge, one
secondary firmware for the hermes chipset, and if we managed to get it
into master mode tertiary firmware for the hermes chipset. So just
knowing which device we have doesn't tell which firmware we need.
I guess that all three images could be packed into a single file.
I find the FIRMWARE variable cleaner, but I really wouldn't mind.
> > - /sysfs/class/firmware/dev_name/{data,loading} show up.
>
> If you pass a struct device to request_firmware(), then you get a
> symlink to the device for free. You can also set the class_id to the
> device bus_id, watching out for name collisions (bus_ids are only unique
> per bus type, so different bus types can use the same bus id, but in
> reality they rarely do.)
OK, sounds good. I'll try that.
> > - echo 1 > /sysfs/class/firmware/dev_name/loading
> > - cat whatever_fw > /sysfs/class/firmware/dev_name/data
> > - echo 0 > /sysfs/class/firmware/dev_name/loading
>
> Nice, but can't you get rid of the loading file by just relying on
> open() and close()? Oh wait, sysfs doesn't pass that down to you, hm,
> looks like you need that info. But does the new binary interface in
> sysfs that just got merged into the tree provide that info for you?
Nop. Although since I seam to be the first user of that interface,
adding support for that shouldn't break anything.
> > - The call to request_firmware() returns with the firmware in a
> > memory buffer and the driver can finish loading.
>
> request_firmware() can't use a static struct class_device, like you have
> it, in order to work properly for multiple calls to request_firmware()
> at the same time by different drivers. Just create a new struct
> class_device, and put it on a list, like I had to do for the tty class
> code (and i2c_dev class code, but that isn't in the kernel to look at
> yet...)
Sorry, I don't know how that 'static' got there, I just wanted to
allocate it on the stack. But I guess that it should be dynamically
allocated anyway. Do I really need to put it on a list?
> Other than those very minor tweaks, I like this interface, it's looking
> very good. I wouldn't worry about any "checksum" calcuation crud, it's
> up to the userspace tool dumping the firmware to the kernel to make sure
> it's writing correct data, not the kernel.
That is what I thought.
> thanks,
Thank you
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
> > - echo 1 > /sysfs/class/firmware/dev_name/loading
> > - cat whatever_fw > /sysfs/class/firmware/dev_name/data
> > - echo 0 > /sysfs/class/firmware/dev_name/loading
>
> Nice, but can't you get rid of the loading file by just relying on
> open() and close()? Oh wait, sysfs doesn't pass that down to you, hm,
> looks like you need that info. But does the new binary interface in
> sysfs that just got merged into the tree provide that info for you?
But what if the close() is due to irregular termination?
If the script is killed, do you download half a firmware?
Regards
Oliver
On Sat, May 17, 2003 at 01:37:52AM +0200, Manuel Estrada Sainz wrote:
> > > - Driver calls request_firmware()
> >
> > Yeah, I agree with your comment in the code, I think a struct device *
> > should be passed here. Or at least somewhere...
>
> To make compatibility with 2.4 kernel easier, I think that I'll add a
> new 'struct device *' parameter to request_firmware(). On 2.4 kernels
> it can be an unused 'void *'. Does that sound too ugly?
Yeah, don't use void * if you can ever help it. As there will be two
different versions for two different kernels, just don't have that
paramater, or make it a char * like you have now for 2.4. That seems to
make sense for 2.4 where you don't have a struct device.
> > > - 'hotplug firmware' gets called with ACCTION=add
> >
> > I don't see why you need to add a new environment variable in your
> > firmware_class_hotplug() call. What is the FIRMWARE variable for, if we
> > already have a device symlink back to the device that is asking for the
> > firmware? Oh, you don't have that :)
>
> The same device can ask for different firmware images.
Ah, that makes more sense now. Ok, I have no problem with it.
> > > - The call to request_firmware() returns with the firmware in a
> > > memory buffer and the driver can finish loading.
> >
> > request_firmware() can't use a static struct class_device, like you have
> > it, in order to work properly for multiple calls to request_firmware()
> > at the same time by different drivers. Just create a new struct
> > class_device, and put it on a list, like I had to do for the tty class
> > code (and i2c_dev class code, but that isn't in the kernel to look at
> > yet...)
>
> Sorry, I don't know how that 'static' got there, I just wanted to
> allocate it on the stack. But I guess that it should be dynamically
> allocated anyway. Do I really need to put it on a list?
If you want to delete it later, you have to have some way to find it
again. If you are just adding it and then removing it in the same
function, just allocate it dynamically, register it, sleep, and then
free it. So then you would not need a list.
thanks,
greg k-h
On Sat, May 17, 2003 at 01:55:15AM +0200, Oliver Neukum wrote:
>
> > > - echo 1 > /sysfs/class/firmware/dev_name/loading
> > > - cat whatever_fw > /sysfs/class/firmware/dev_name/data
> > > - echo 0 > /sysfs/class/firmware/dev_name/loading
> >
> > Nice, but can't you get rid of the loading file by just relying on
> > open() and close()? Oh wait, sysfs doesn't pass that down to you, hm,
> > looks like you need that info. But does the new binary interface in
> > sysfs that just got merged into the tree provide that info for you?
>
> But what if the close() is due to irregular termination?
> If the script is killed, do you download half a firmware?
Good point. Actually I don't think that the binary interface for sysfs
passes open and close down to the lower levels, so it's a moot point.
echo... works for me.
thanks,
greg k-h
On Sat, May 17, 2003 at 12:22:29AM +0200, Oliver Neukum wrote:
>
> > > How and what is the benefit? If you go low on battery you have to
> > > suspend, there's no choice. This means that you have to have it in RAM
> > > always.
> >
> > If the device losses the firmware upon suspend, the driver will have to
> > reinitialize it as if it just got plugged, which somehow makes all
> > devices hotplugable.
>
> So all firmware has to be permanently in RAM anyway?
If you really can't access the filesystem on wakeup, you could pull the
required firmware into ram upon suspend and remove it after wakeup.
Then while the system is running it will not use kernel memory.
> > If the driver uses request_firmware(), it doesn't need to handle any
> > special case, just initialize as usual and it will get the firmware
> > when it needs it.
>
> How or precisely, how do you know that it gets it when it needs
> it? Are you planning to have a gray area where the kernel generates
> a special user space before everything else gets woken?
For this I proposed fwfs (a kernel friendly filesystem on top of
ramfs), but it didn't get a good acceptance. And after reading Greg's
recent post initramfs should be able to fill that gap.
> > If the device is needed to access the filesystem, some kind of
> > persistence will be needed, so the required firmware is already in
> > kernel space. But the driver doesn't need to care, it will just ask for
> > the firmware as usual.
> >
> > Which brings me to another issue, the same device can be required to
> > access the filesystem or not:
> > - In a diskless client, it is the network card
> > - In a live-cd it is the cdrom drive
> > - In a multi disk system just one of them will be holding
> > required firmware.
> > So you can not decide at coding time, the latest at compile time, and
> > ideally at runtime (which is what I am trying to do).
>
> How? You cannot page out memory during resumption.
> You must not cause any access to disk during resumption.
You will have to make sure that the firmware is copied into RAM before
suspension, either via fwfs, initramfs or whatever persistence method
gets implemented.
> > OK, how about:
> >
> > int request_firmware_nowait (
> > struct module *module,
> > const char *name, const char *device, void *context,
> > void (*cont)(const struct firmware *fw, void context)
> > );
> >
> > request_firmware code will try_module_get/module_put as needed.
> >
> > Would that fix the issue?
>
> No, still no good. It means that you get a memory leak if you unload
> a driver before firmware is provided. You need the ability to explicitely
> cancel a request for firmware.
The driver will not unload if request_firmware_nowait() has called
try_module_get() on it.
But the device could get disconnected and in that case the firmware
load should be canceled. I'll add request_firmware_cancel().
> > > > Would a patch to wait for hotplug termination and provide termination
> > > > status be accepted?
> > >
> > > No, you must not wait for user space.
> >
> > And to get notified when userspace is done?
>
> Not with that interface.
> You'd need drivers to register both with their subsystem and
> a firmware subsystem.
Why?, the current interface already provides termination notification.
> But you cannot make device discovery wait for user space.
Why not?
And anyway, request_firmware_nowait() could be used if that is an
issue.
> You'd have to rewrite the probe method of all drivers that need
> firmware.
Keep in mind that the "firmware in a header" method is not allowed
any more. Drivers needing firmware will have to get their probe method
modified to get the firmware from userspace anyway.
Using request_firmware() if they can sleep (USB probe callbacks can
sleep) is trivial, and using request_firmware_nowait() if they can't
sleep is just a matter of splitting probe in two pieces.
So, what alternative are you proposing to get the firmware?
If your proposal is just keeping the firmware in a header so you just
have it there all the time, you should discuss it with Alan, Greg,
Jeff, et all, not me.
Thanks
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
Howdy,
Not to throw in two-cents you didn't ask for but...
[Quick Disclaimer: The following is extemporaneous thought based on god
knows what all, but it *sounds* reasonable to me just now... 8-)]
I have a custom device with a set of FPGAs (Field Programmable Gate Arrays)
on it, that have to have their firmware written into them after each power
on (etc.)
My solution was rather more direct than those listed below.
Firmware loading, exclusivity, and correctness is best (and quite usefully)
thought of as a one-writer-many-readers lock, which can be implemented
completely and deterministically in the (various) open routines.
Consider the following two example nodes: (I will not use sysfs specific
speak because I am not familiar enough with sysfs to avoid putting noise in
my signal)
/devfsnodething/device/user-access
/devfsnodething/device/firmware-programming
if you want to "cat my-firmware-image
>/devfsnodething/device/firmware-programming" it only makes sense for the
open-for-write to succeed if two assertions are met:
1) nobody else is writing the firmware
2) nobody else is already using the user-access point.
There is also a list of "things firmware does to a device" and similar
realities that can be deterministically known about any device. This list
isn't exhaustive, but it is nearly so. Consider these as candidate flags
and entry points suitable for framing in a driver_info like structure of
pointers to functions (etc):
FIRMWARE_FLASHABLE == set in any module to indicate that the system should
produce/use/honor a firmware-programming device node.
FIRMWARE_FLASH_DEVICE == set on any device_info structure for the access
point for loading a firmware image. (User-access points don't have this
flag set.)
atomic(FIRMWARE_NEEDS_FLASH) == set in a device that is FLASHABLE and
doesn't have a currently valid flash image in place. Acts as the guardians
against opens of the user-access device(s).
FIRMWARE_FLASH_WAIT == open of firmware-programming device should block,
waiting for a chance to flash the device until other flash writers or
user-access users are done. Non-Blocking opens will not block if this is
set. Blocking opens will wait if this is set. If this is not set, any
other node in the device being open will make the open of the
firmware-programming device return EBUSY.
dev_Can_Program() == Called during open of firmware-programming device file,
Tells you if the device can be flashed (now|again). Implements checks for
re-programmability (do you have to power-off or remove and reinsert device
in order to change the firmware, has the firmware already been flashed to
the device, etc.) Returns READY (0), EFAIL, EAGAIN (maybe others?). Non
ready returns will cause the open to fail. Blocking of the open happens in
this routine if it is going to happen at all.
dev_Invalidate_Device() == called during open, or first write operation, to
dismantle user-access devices and list the firmware image as incomplete or
unstable (etc). If the firmware is being "rewritten" this routine is
responsible for clearing the old firmware. It is also responsible for
removing user-access devices from the devfs file system and
dev_Check_Program() == Called during close (?) of the firmware-programming
device file to validate that the firmware was accepted by the device.
Returns OK or not.
dev_Enumerate_Device() == Called during close (?) for the firmware
programming device file (or maybe a kernel tasklet etc) after
dev_Check_Program() to give the driver a chance to create the user-access
nodes. (This is useful, for instance, when the firmware will "create"
within the device, a variable set of user access points.
So, the above covers almost all variations of the cases where a device
needs/may want a firmware file via different combinations of the flags and
routines. (I am sure I may have missed some cases, but you get the point.)
It does get rid of the whole "echo 1 >" nonsense as a separate action.
(which is good, because the non-script versions of such activities gets
annoying, and you don't want to activate a device with detectably bad
firmware image in it under any circumstances.)
So What Rob Would Do(tm) (all rights [to the phrase] reserved) 8-):
1) Define a firmware_ops structure with the necessary entry points and data
items in it.
2) Add a (normally null) pointer to same to the sysfs_thingy, file_ops,
device_info, and module_info structures. (I really haven't thought about
where this pointer will go.)
3) Consider any sysfs/device/module/file_info with a pointer to the same
instance of this structure as under the exclusive-open (readers/writer)
protections (giving you a nice, safe, and universally detectable place to
deal with status inquiries, locks, and whatnot for the devices, of the "oh
look, that pointer is non-null, we need to be careful" variety.] (So only
one FIRMWARE_FLASH_DEVICE with a particular firmware_ops structure can be
opened at one time. A non FIRMWARE_FLASH_DEVICE may not be opened if a non
FIRMWARE_FLASH_DEVICE is opened which has the same firmware_info pointer.
As many non FIRMWARE_FLASH_DEVICES as you choose may be opened at one time.
(hence the readers/writer like interface)
4) Take a step back, realize that this is all really a "lets make a
meta-device coordinator" pointer/dependency-system and see if the work has
already been (mostly?) done elsewhere, or if the consideration should be
broadened just a hair beyond actual firmware.
=====
What I have done to "fake all this" on my little embedded box using only
pre-hotplug-support facilities:
/etc/modules.conf:
post-install cpld /opt/casabyte/bin/do_flash
above tequal0 cpld
/opt/casabyte/bin/do_flash:
modprobe fpgaflash
cat firmware >/dev/FPGAFlash/ALL
rmmod fpgaflash
The "cpld" (Complex Programable Logic Device) is the thing that lets me
"see" the pins on the FPGA(s) and this driver creates and owns the region of
memory and IO necessary to both flash the device and operate whatever image
is placed into the flash. "Attaching" and "Detaching" from the cpld driver
acts as an exclusion primitive (lock) and only one lower-level driver can be
attached.
The fpgaflash is a lower level driver that can only flash the FPGAs and has
all the checks and such to make sure that any given FPGA is flashed once.
It handles all the clearing and whatnot. (Since the cpld can flash all of
the FPGAs at once if you want, the driver dynamically creates
/dev/FPGAFLASH/1 through 4, and "ALL"). When it is removed, those devices
obviously disappear.
The tequal0 driver is the real point, it will fail to load if at least one
FPGA hasn't been successfully flashed. It dynamically allocates the devices
based on the information it gets from cpld about what is flashed with what.
Now all that is done, with "well designed" drivers, with clear dependencies,
and six lines of script/configuration.
This would be directly analogous to just having the flashing of firmware
(start and finish) re-invoke the hotplug system as the "character of the
flashable device" changes from "dumb thing that needs an image" as one
device, and "smart thing ready to serve" exists in sequence.
=====
In short (I know, too late), I think about moving firmware into my FPGAs as
a first cousin to formatting a ram-disk-buffer. It is a
nearly-deterministic strict-dependency activity that uses one (almost
always) character device as a latch that creates/activates or
removes/deactivates one-or-more dependent devices.
This is true simply because of the before and after nature of a programmable
device.
If you feel you must codify the idea of these dependencies into the kernel
(as opposed to the dirver(s) and the(ir) configurations) then don't do it by
half measures. Third-level manipulations (the echo(s) below) are *NOT* your
friend. Transform the device presentation (in the devfs or the sysfs) as
the device changes. These file systems are dynamic for a reason.
The case of, say a USB root hub with a built in serial port who's controller
needs to be flashed. If the controller hasn't been flashed (correctly) or
is being re-flashed the serial port just doesn't really exist until that
operation completes successfully. If the controller can not be re-flashed,
then the device representing the opportunity to flash the controller becomes
meaningless (and should probably disappear) when a successful flash has been
completed. If the serial port is opened, the flash device should be
blocked.
It's simple logical exclusion, and we have primitives for all that if you
want to leave the invalid devices visible while they are invalid. Agree on
a place in the abstract, or hard-wire the exclusion into the mechanism (via
the shared info structure?), but oh-god-please don't put the in-kernel
exclusion into the user-space runtime domain.
All other considerations aside, (e.g. the new binary interface just merged
in etc.) it isn't the open-ness and close-ness that matters, it's the
programmable-device-ready-or-not-ness. Irregular termination close == bad
firmware. Programmable device not ready == bad firmware. (and so on). The
writer of any particular driver *should* already be looking for the un-ready
and the illegal-simultaneous-access cases (both in the firmware-flasher and
the user-access parts) or the driver is going to oops anyway.
Pardon my rant... 8-)
Rob.
P.S. I "improperly" use "flash" to talk about the "load" of program code
and data into devices which wont retain that information permanently. It's
morally wrong, but I work with a bunch of people who don't know the
difference and aggressively use the improper term. I live with hearing that
day-in and day-out from the management level here. I took no effort to
correct that idiomatic mistake in the above as I have aggressively fostered
a blind-spot in my head on this topic to keep my sanity 8-). Sorry if
anybody was confused. I will not entertain "we are not talking about
flashing, we are talking about loading runtime firmware" responses. The non
persistent "load" case is covered by default, but the "reload" of non
persistent images, and the "(re)flash" of persistent images are both covered
by the proper intersection of the set of cases discussed above.
So ptptpth.... 8-)
-----Original Message-----
From: [email protected]
[mailto:[email protected]]On Behalf Of Greg KH
Sent: Friday, May 16, 2003 5:04 PM
To: Oliver Neukum
Cc: Manuel Estrada Sainz; LKML; Simon Kelley; Alan Cox; Downing, Thomas;
[email protected]; Pavel Roskin
Subject: Re: request_firmware() hotplug interface, third round.
On Sat, May 17, 2003 at 01:55:15AM +0200, Oliver Neukum wrote:
>
> > > - echo 1 > /sysfs/class/firmware/dev_name/loading
> > > - cat whatever_fw > /sysfs/class/firmware/dev_name/data
> > > - echo 0 > /sysfs/class/firmware/dev_name/loading
> >
> > Nice, but can't you get rid of the loading file by just relying on
> > open() and close()? Oh wait, sysfs doesn't pass that down to you, hm,
> > looks like you need that info. But does the new binary interface in
> > sysfs that just got merged into the tree provide that info for you?
>
> But what if the close() is due to irregular termination?
> If the script is killed, do you download half a firmware?
Good point. Actually I don't think that the binary interface for sysfs
passes open and close down to the lower levels, so it's a moot point.
echo... works for me.
thanks,
greg k-h
A good bit of this is moot.
All updateable devices fall into one of three categories:
1) "Flashable", they will remember the image data until that image data is
displaced by another image. (we are generally not talking about this kind
of device here, but they deserve a nod for completeness.)
2) "Loadable && Suspendable", such a device will retain its firmware during
a suspend, either because it will not actually be powered off, or because of
some "magical" means (like a battery backup on-board activated by a
please-suspend yourself call.) or, if it doesn't retain its whole firmware,
it will at least retain enough internal state that it can pick up where it
left off when the firmware is reloaded. (...which actually puts it mostly
in the next case if the firmware comes from the kernel side as opposed to
the device side...)
3) "Reload Only", such a device will lose its firmware and internal state
completely when the device is suspended.
Given this simple reality, programs in session with the (almost exclusively
existent) type three devices are simply going to lose their state across a
suspend/resume. This is just like suspending your laptop with a telnet
session open. When the resume happens, the telnet session will still be
there, but the first time you use it, or it uses itself, it will see the
socket is gone and eat itself.
Moving into the twilight zone of theoretical analysis.... (it's ok to stop
reading here, the below is NOT cannon by any means, and is actually a little
vague 8-)
Type one devices will do whatever they do.
In the case of type two devices, the magic will magically keep you covered.
If no programs are using the device, regardless of type, then there is no
hurry regardless of intent.
Ok, that sounded a little fatuous, but frankly, any firmware-loadable device
is going to come up out-of-state. Period. Definition of the box. Loading
the firmware will bring it into state, sure. But the driver is going to
have to be able to recognize the out-of-stateness of the device until after
the load happens no matter what. Anything else is an oops waiting to
happen.
So the person who writes the driver for a reloadable device will already
have to have a way to park the driver until the firmware is loaded. That
being said you now have an "infinite" amount of time to load the driver, and
you, by definition, *can* now wait for enough user-space initialization to
take place to handle the issues for you because all access will be blocked
within the driver and the programs dependent on that driver will be hung
waiting.
Remember that all your type-3 firmware-load-targets are small computers
that, by definition, can't be suspended, if they suspended themselves they'd
be type two, so "getting all in a hurry" to reboot them during system resume
is valueless.
The only time this *couldn't* be the case is if you actually "suspend to"
something behind a type-three device. *That* could only be resolved by
keeping the firmware image in preserved ram. That would also be a very dumb
thing to do for these self-same reasons.
Let's face it, the Type-3 devices are hideously peripheral anyway. Mouse
touch-pads, serial dongles, second tier hubs, Ethernet cards that have
already dropped all their connections and will have to be re-bound into the
stack anyway. etc.
Firmware-able devices are slow external devices, with wait-states and
protocols and firmware-loading interfaces. As the system comes to life the
things created by the logic in the firmware won't exist to be interacted
with until the firmware is (reloaded. (by definition there, proof by
induction omitted for brevity... 8-). That says outright that you have a
lot of dependency-order tasking to do during resume, which in turn means
that a driver that doesn't park itself in stages, and which isn't operating
in a context where waits and yields are ok (say user context or
kernel-thread context), will either be NP complete with respect to its
dependencies on other devices in the system, or it will crash and burn
"sometimes" "for no apparent reason". So any such device, to be both
firmware-loadable and resumable, will need to be designed to resume only
after user context is available.
Once the resumeable firmware-load-safe driver has been constructed for the
device, you can easily wait for user context. Any attempt to have a driver
without that park-and-wait feature will likely oops anyway when it wanders
naively onto the CPU and access features that don't exist yet.
Sadly, each such loadable device will suffer under the burden of the unique
demands of its design, so it will be up to the driver writer for each device
do meet those demands. Building unknown protections for as-yet non-existent
devices into the kernel is not practical.
If you disagree, look again. Even something as simple as a serial port UART
will need to have its baud-rate latch restored. If that serial port UART is
an abstraction that exists solely because of a firmware image (Which is how
FPGAs work for example) then if the driver for the serial port "wakes up"
and tries to reload that divisor before the driver for the firmware load,
you will oops, or at least hang or not ever really restore the state of the
logical UART.
The furthest the kernel can go "safely and reasonably" to deal with these
issues is (in vague abstract-o-vision theoretical terms):
1) Create a mid-transaction layer where the kernel is "just about" to call
the file_ops structure members.
2) Define a well defined signal/means to wake a driver and have it go above
that layer a la return -ERESTARTSYS and guarantee that this signal will be
sent to every thread-of-control before a resume
3) Strongly urge device driver programmers to use and obey Number 2.
4) Provide a latch in number 1 that "parks" the request before being
restarted, said latch to be set to park before the signal in number 2 is
sent.
5) Add an optional driver entry point and callback interface that will let a
driver control the un-park of its own operations, and check the parkedness
of other drivers (for those cases when dependencies exist and can be
determined before sleeping or as waking)
5a) Add a well defined all-bets-are-off interface that will error out
some/all of calls on some/all of the parked threads when/if things become
recognizably hopeless. (Say the USB serial port was unplugged, and not
plugged back in, during the sleep.)
6) Provide a kernel thread (or threads) to give an independent user-context
to the call-ins created in number 5.
(and so forth)
Some programmers will keep/move their firmware in/into ram. Some will
arrange to allow for some user-space utility to reload the firmware (which
is really for the best). Some will let the resume oops. Expecting a
generic kernel mechanism to do the reload instead is paying the price of
bloat for one throw at a constantly moving target.
===
So now, many drivers resume themselves because they are just data based
abstractions. (see the entire VFS 8-)
Lots of drivers wake up, reinitialize their devices, and then carry on
because their state was internally persistent. (see any block device beneath
the VFS 8-)
Once the block devices are alive again, you can restart the virtual memory
system. Once the VM system is up you can use paged memory for your resume
data (firmware) anyway.
One quick non-blocking pass ought to be made through the Number-5's of the
system to let the first order dependencies disappear.
It is now safe to wake the user contexts.
The theorized special kernel thread(s), which are user contexts, just keep
calling the resume tasklets until the list is empty. From that vantage,
just about any user-space program (a-la hotplug) can be called at leisure.
Any driver that expects to resume itself before the block devices (and
virtual memory system) are ready bares the onus on itself to make sure it
will be able to before things go to sleep. The only case where this
actually has any point, is the case where the firmwareable device is a block
device with a swap file/partition. And such a block device is evil anyway.
Wow, two rants in one night... 8-)
Rob.
-----Original Message-----
From: [email protected]
[mailto:[email protected]]On Behalf Of Manuel Estrada
Sainz
Sent: Friday, May 16, 2003 5:59 PM
To: Oliver Neukum
Cc: LKML; Simon Kelley; Alan Cox; Downing, Thomas; Greg KH;
[email protected]; Pavel Roskin
Subject: Re: request_firmware() hotplug interface, third round.
On Sat, May 17, 2003 at 12:22:29AM +0200, Oliver Neukum wrote:
>
> > > How and what is the benefit? If you go low on battery you have to
> > > suspend, there's no choice. This means that you have to have it in RAM
> > > always.
> >
> > If the device losses the firmware upon suspend, the driver will have to
> > reinitialize it as if it just got plugged, which somehow makes all
> > devices hotplugable.
>
> So all firmware has to be permanently in RAM anyway?
If you really can't access the filesystem on wakeup, you could pull the
required firmware into ram upon suspend and remove it after wakeup.
Then while the system is running it will not use kernel memory.
> > If the driver uses request_firmware(), it doesn't need to handle any
> > special case, just initialize as usual and it will get the firmware
> > when it needs it.
>
> How or precisely, how do you know that it gets it when it needs
> it? Are you planning to have a gray area where the kernel generates
> a special user space before everything else gets woken?
For this I proposed fwfs (a kernel friendly filesystem on top of
ramfs), but it didn't get a good acceptance. And after reading Greg's
recent post initramfs should be able to fill that gap.
> > If the device is needed to access the filesystem, some kind of
> > persistence will be needed, so the required firmware is already in
> > kernel space. But the driver doesn't need to care, it will just ask for
> > the firmware as usual.
> >
> > Which brings me to another issue, the same device can be required to
> > access the filesystem or not:
> > - In a diskless client, it is the network card
> > - In a live-cd it is the cdrom drive
> > - In a multi disk system just one of them will be holding
> > required firmware.
> > So you can not decide at coding time, the latest at compile time, and
> > ideally at runtime (which is what I am trying to do).
>
> How? You cannot page out memory during resumption.
> You must not cause any access to disk during resumption.
You will have to make sure that the firmware is copied into RAM before
suspension, either via fwfs, initramfs or whatever persistence method
gets implemented.
> > OK, how about:
> >
> > int request_firmware_nowait (
> > struct module *module,
> > const char *name, const char *device, void *context,
> > void (*cont)(const struct firmware *fw, void context)
> > );
> >
> > request_firmware code will try_module_get/module_put as needed.
> >
> > Would that fix the issue?
>
> No, still no good. It means that you get a memory leak if you unload
> a driver before firmware is provided. You need the ability to explicitely
> cancel a request for firmware.
The driver will not unload if request_firmware_nowait() has called
try_module_get() on it.
But the device could get disconnected and in that case the firmware
load should be canceled. I'll add request_firmware_cancel().
> > > > Would a patch to wait for hotplug termination and provide
termination
> > > > status be accepted?
> > >
> > > No, you must not wait for user space.
> >
> > And to get notified when userspace is done?
>
> Not with that interface.
> You'd need drivers to register both with their subsystem and
> a firmware subsystem.
Why?, the current interface already provides termination notification.
> But you cannot make device discovery wait for user space.
Why not?
And anyway, request_firmware_nowait() could be used if that is an
issue.
> You'd have to rewrite the probe method of all drivers that need
> firmware.
Keep in mind that the "firmware in a header" method is not allowed
any more. Drivers needing firmware will have to get their probe method
modified to get the firmware from userspace anyway.
Using request_firmware() if they can sleep (USB probe callbacks can
sleep) is trivial, and using request_firmware_nowait() if they can't
sleep is just a matter of splitting probe in two pieces.
So, what alternative are you proposing to get the firmware?
If your proposal is just keeping the firmware in a header so you just
have it there all the time, you should discuss it with Alan, Greg,
Jeff, et all, not me.
Thanks
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
On Fri, May 16, 2003 at 05:03:38PM -0700, Greg Kroah-Hartman wrote:
> On Sat, May 17, 2003 at 01:55:15AM +0200, Oliver Neukum wrote:
> >
> > > > - echo 1 > /sysfs/class/firmware/dev_name/loading
> > > > - cat whatever_fw > /sysfs/class/firmware/dev_name/data
> > > > - echo 0 > /sysfs/class/firmware/dev_name/loading
> > >
> > > Nice, but can't you get rid of the loading file by just relying on
> > > open() and close()? Oh wait, sysfs doesn't pass that down to you, hm,
> > > looks like you need that info. But does the new binary interface in
> > > sysfs that just got merged into the tree provide that info for you?
> >
> > But what if the close() is due to irregular termination?
> > If the script is killed, do you download half a firmware?
>
> Good point. Actually I don't think that the binary interface for sysfs
> passes open and close down to the lower levels, so it's a moot point.
>
> echo... works for me.
I think we'd be better off checking for this case by requiring the
firmware to include a length field and adapting the binary interface
so that we can see the open/close boundary. The "loading" thing is
pretty damn ugly.
Plus with the "loading" file, a interrupted load will just appear to
be unterminated - and if the scripts run again they'll have to check
that the loading file was 0 to start with - and if it isn't there's
nothing they can do except wait for the dead load to time out.
Better to catch the close, check the length, then return the firmware
or throw the junk image away as appropriate.
--
David Gibson | For every complex problem there is a
[email protected] | solution which is simple, neat and
| wrong.
http://www.ozlabs.org/people/dgibson
On Sat, May 17, 2003 at 12:13:49AM +0200, Oliver Neukum wrote:
> Am Freitag, 16. Mai 2003 18:09 schrieb Alan Cox:
> > On Gwe, 2003-05-16 at 09:07, Oliver Neukum wrote:
> > > So, if I understand you correctly, RAM is only saved if a device
> > > is hotpluggable and needs firmware only upon intial connection.
> > > Which, if you do suspend to disk correctly, is no device.
> >
> > Thats just because the interface is a little warped not the theory.
> > On a resume you need to reload firmware and you already handle
> > rediscovery on USB bus for example because the devices can change
>
> Right. But the order of resumption is fixed by hardware needs.
> So during resumption you cannot use block devices and therefore
> not start a hotplug script. Or did I miss something?
For devices that aren't essentialy to get userspace running
(e.g. network on a laptop running from local disk), the firmware
request doesn't have to happen during the hairy part of resumption.
The device could just mark itself as unusable at suspend time, then at
resume it schedule_work()s something to reload the firmware and
complete reinitialization. Shortly after userspace is back in action,
the device will come back to life.
--
David Gibson | For every complex problem there is a
[email protected] | solution which is simple, neat and
| wrong.
http://www.ozlabs.org/people/dgibson
On Fri, May 16, 2003 at 04:59:58PM -0700, Greg Kroah-Hartman wrote:
> On Sat, May 17, 2003 at 01:37:52AM +0200, Manuel Estrada Sainz wrote:
> > > > - Driver calls request_firmware()
> > >
> > > Yeah, I agree with your comment in the code, I think a struct device *
> > > should be passed here. Or at least somewhere...
> >
> > To make compatibility with 2.4 kernel easier, I think that I'll add a
> > new 'struct device *' parameter to request_firmware(). On 2.4 kernels
> > it can be an unused 'void *'. Does that sound too ugly?
>
> Yeah, don't use void * if you can ever help it. As there will be two
> different versions for two different kernels, just don't have that
> paramater, or make it a char * like you have now for 2.4. That seems to
> make sense for 2.4 where you don't have a struct device.
>
> > > > - 'hotplug firmware' gets called with ACCTION=add
> > >
> > > I don't see why you need to add a new environment variable in your
> > > firmware_class_hotplug() call. What is the FIRMWARE variable for, if we
> > > already have a device symlink back to the device that is asking for the
> > > firmware? Oh, you don't have that :)
> >
> > The same device can ask for different firmware images.
>
> Ah, that makes more sense now. Ok, I have no problem with it.
Given this, would it be better to make the sysfs node name depend on
which firmware we're loading - rather than "data" always. I realise
we could just require firmware requests for a particular device
instance to be serialised, however my instinct says using different
nodes would be more robust: it will be easier to figure out what's
gone wrong if a script error or a kernel bug has resulted in
attempting to load two images at once.
--
David Gibson | For every complex problem there is a
[email protected] | solution which is simple, neat and
| wrong.
http://www.ozlabs.org/people/dgibson
Am Samstag, 17. Mai 2003 06:50 schrieb David Gibson:
> On Sat, May 17, 2003 at 12:13:49AM +0200, Oliver Neukum wrote:
> > Am Freitag, 16. Mai 2003 18:09 schrieb Alan Cox:
> > > On Gwe, 2003-05-16 at 09:07, Oliver Neukum wrote:
> > > > So, if I understand you correctly, RAM is only saved if a device
> > > > is hotpluggable and needs firmware only upon intial connection.
> > > > Which, if you do suspend to disk correctly, is no device.
> > >
> > > Thats just because the interface is a little warped not the theory.
> > > On a resume you need to reload firmware and you already handle
> > > rediscovery on USB bus for example because the devices can change
> >
> > Right. But the order of resumption is fixed by hardware needs.
> > So during resumption you cannot use block devices and therefore
> > not start a hotplug script. Or did I miss something?
>
> For devices that aren't essentialy to get userspace running
> (e.g. network on a laptop running from local disk), the firmware
> request doesn't have to happen during the hairy part of resumption.
Not true for network cards. Somebody might be running NFS
over it. The problem is that you cannot tell (or rather shouldn't - it's
a layering violation).
Anything that is used for paging needs to be back to life before
you can think about resurrecting user space.
Also you need to bring keyboards back to life early to make
sysreq work.
Secondly, you need a way to get essential devices to work in
all cases. If you implement it, why not use it?
> The device could just mark itself as unusable at suspend time, then at
> resume it schedule_work()s something to reload the firmware and
> complete reinitialization. Shortly after userspace is back in action,
> the device will come back to life.
Is supposed to. You cannot put blind trust into that. You need to use
a pretty arbitrary timeout to deal with this.
I fail to see technical improvements here.
Regards
Oliver
On Sat, May 17, 2003 at 09:02:58AM +0200, Oliver Neukum wrote:
> Am Samstag, 17. Mai 2003 06:50 schrieb David Gibson:
> > On Sat, May 17, 2003 at 12:13:49AM +0200, Oliver Neukum wrote:
> > > Am Freitag, 16. Mai 2003 18:09 schrieb Alan Cox:
> > > > On Gwe, 2003-05-16 at 09:07, Oliver Neukum wrote:
> > > > > So, if I understand you correctly, RAM is only saved if a device
> > > > > is hotpluggable and needs firmware only upon intial connection.
> > > > > Which, if you do suspend to disk correctly, is no device.
> > > >
> > > > Thats just because the interface is a little warped not the theory.
> > > > On a resume you need to reload firmware and you already handle
> > > > rediscovery on USB bus for example because the devices can change
> > >
> > > Right. But the order of resumption is fixed by hardware needs.
> > > So during resumption you cannot use block devices and therefore
> > > not start a hotplug script. Or did I miss something?
> >
> > For devices that aren't essentialy to get userspace running
> > (e.g. network on a laptop running from local disk), the firmware
> > request doesn't have to happen during the hairy part of resumption.
>
> Not true for network cards. Somebody might be running NFS
> over it. The problem is that you cannot tell (or rather shouldn't - it's
> a layering violation).
Potentially, yes, usually no (hence "running from local disk" above).
This is why Manuel wants to make persistence runtime selectable.
It may well be a bit messy in the scripts to make sure all the right
drivers have persistent firmwares, and I'm sure there will be some
funny corner cases, but I don't think it's an insoluble problem.
If you have to be able to reinitialize any device from suspend without
userland running, then clearly there is no way around having all
necessary firmware images RAM resident. But there are many situations
where that's not necessary - so why should they have to pay for the
worst case.
> Anything that is used for paging needs to be back to life before
> you can think about resurrecting user space.
> Also you need to bring keyboards back to life early to make
> sysreq work.
>
> Secondly, you need a way to get essential devices to work in
> all cases. If you implement it, why not use it?
>
> > The device could just mark itself as unusable at suspend time, then at
> > resume it schedule_work()s something to reload the firmware and
> > complete reinitialization. Shortly after userspace is back in action,
> > the device will come back to life.
>
> Is supposed to. You cannot put blind trust into that. You need to use
> a pretty arbitrary timeout to deal with this.
So? Something could go wrong, but things can go wrong in lots of
places, I don't see what's so especially dire about this situation?
> I fail to see technical improvements here.
Improvements over *what*. There is no existing method for pulling
firmware images into the kernel.
--
David Gibson | For every complex problem there is a
[email protected] | solution which is simple, neat and
| wrong.
http://www.ozlabs.org/people/dgibson
On Sat, May 17, 2003 at 02:47:44PM +1000, David Gibson wrote:
> On Fri, May 16, 2003 at 04:59:58PM -0700, Greg Kroah-Hartman wrote:
> > On Sat, May 17, 2003 at 01:37:52AM +0200, Manuel Estrada Sainz wrote:
> > > > > - Driver calls request_firmware()
> > > >
> > > > Yeah, I agree with your comment in the code, I think a struct device *
> > > > should be passed here. Or at least somewhere...
> > >
> > > To make compatibility with 2.4 kernel easier, I think that I'll add a
> > > new 'struct device *' parameter to request_firmware(). On 2.4 kernels
> > > it can be an unused 'void *'. Does that sound too ugly?
> >
> > Yeah, don't use void * if you can ever help it. As there will be two
> > different versions for two different kernels, just don't have that
> > paramater, or make it a char * like you have now for 2.4. That seems to
> > make sense for 2.4 where you don't have a struct device.
> >
> > > > > - 'hotplug firmware' gets called with ACCTION=add
> > > >
> > > > I don't see why you need to add a new environment variable in your
> > > > firmware_class_hotplug() call. What is the FIRMWARE variable for, if we
> > > > already have a device symlink back to the device that is asking for the
> > > > firmware? Oh, you don't have that :)
> > >
> > > The same device can ask for different firmware images.
> >
> > Ah, that makes more sense now. Ok, I have no problem with it.
>
> Given this, would it be better to make the sysfs node name depend on
> which firmware we're loading - rather than "data" always.
> I realise we could just require firmware requests for a particular
> device instance to be serialised,
I think that is a pretty good assumption.
It won't be me how loads two different firmwares concurrently to the
same device :)
> however my instinct says using different nodes would be more robust:
It would also complicate both kernel and userspace code.
> it will be easier to figure out what's gone wrong if a script error
For this matter, I could add a readonly 'name' which gives you the same
string as $FIRMWARE. That way if something goes wrong you can easily
find out which firmware image the kernel is expecting.
> or a kernel bug has resulted in attempting to load two images at once.
This will get caught, because sysfs won't allow two entries with the
same name.
Thanks
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
On Sat, May 17, 2003 at 02:44:59PM +1000, David Gibson wrote:
> On Fri, May 16, 2003 at 05:03:38PM -0700, Greg Kroah-Hartman wrote:
> > On Sat, May 17, 2003 at 01:55:15AM +0200, Oliver Neukum wrote:
> > >
> > > > > - echo 1 > /sysfs/class/firmware/dev_name/loading
> > > > > - cat whatever_fw > /sysfs/class/firmware/dev_name/data
> > > > > - echo 0 > /sysfs/class/firmware/dev_name/loading
> > > >
> > > > Nice, but can't you get rid of the loading file by just relying on
> > > > open() and close()? Oh wait, sysfs doesn't pass that down to you, hm,
> > > > looks like you need that info. But does the new binary interface in
> > > > sysfs that just got merged into the tree provide that info for you?
> > >
> > > But what if the close() is due to irregular termination?
> > > If the script is killed, do you download half a firmware?
> >
> > Good point. Actually I don't think that the binary interface for sysfs
> > passes open and close down to the lower levels, so it's a moot point.
> >
> > echo... works for me.
>
> I think we'd be better off checking for this case by requiring the
> firmware to include a length field and adapting the binary interface
> so that we can see the open/close boundary. The "loading" thing is
> pretty damn ugly.
I did look for open/close first and finally added the 'loading' because
I didn't have them.
I don't thing it is so ugly, but if I could see open/close and the
consensus is that 'loading' should go, I'll do it.
> Plus with the "loading" file, a interrupted load will just appear to
> be unterminated - and if the scripts run again they'll have to check
> that the loading file was 0 to start with - and if it isn't there's
> nothing they can do except wait for the dead load to time out.
The script will not automatically run at least until the timeout is
expired.
But in case you are doing things by hand, how about:
$ echo cancel > .../loading
or if you want to keep the content numeric:
$ echo -1 > .../loading
This will also allow the regular script to just cancel the load in case
of error, like if the firmware image is not available or a read error
happened while reading it.
I'll implement that and the other stuff that came out of Oliver's
comments later today and post the new code.
> Better to catch the close, check the length, then return the firmware
> or throw the junk image away as appropriate.
If 'loading' stays the above should fix your timeout issue, and if it
goes, yes, that is probably the way to go.
Thanks
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
On Sat, May 17, 2003 at 10:46:12AM +0200, Manuel Estrada Sainz wrote:
> On Sat, May 17, 2003 at 02:44:59PM +1000, David Gibson wrote:
> > On Fri, May 16, 2003 at 05:03:38PM -0700, Greg Kroah-Hartman wrote:
> > > On Sat, May 17, 2003 at 01:55:15AM +0200, Oliver Neukum wrote:
> > > >
> > > > > > - echo 1 > /sysfs/class/firmware/dev_name/loading
> > > > > > - cat whatever_fw > /sysfs/class/firmware/dev_name/data
> > > > > > - echo 0 > /sysfs/class/firmware/dev_name/loading
> > > > >
> > > > > Nice, but can't you get rid of the loading file by just relying on
> > > > > open() and close()? Oh wait, sysfs doesn't pass that down to you, hm,
> > > > > looks like you need that info. But does the new binary interface in
> > > > > sysfs that just got merged into the tree provide that info for you?
> > > >
> > > > But what if the close() is due to irregular termination?
> > > > If the script is killed, do you download half a firmware?
> > >
> > > Good point. Actually I don't think that the binary interface for sysfs
> > > passes open and close down to the lower levels, so it's a moot point.
> > >
> > > echo... works for me.
> >
> > I think we'd be better off checking for this case by requiring the
> > firmware to include a length field and adapting the binary interface
> > so that we can see the open/close boundary. The "loading" thing is
> > pretty damn ugly.
>
> I did look for open/close first and finally added the 'loading' because
> I didn't have them.
>
> I don't thing it is so ugly, but if I could see open/close and the
> consensus is that 'loading' should go, I'll do it.
Hmm... on consideration I don't think it's as ugly as I first
thought. But I think I have a better idea, see below...
> > Plus with the "loading" file, a interrupted load will just appear to
> > be unterminated - and if the scripts run again they'll have to check
> > that the loading file was 0 to start with - and if it isn't there's
> > nothing they can do except wait for the dead load to time out.
>
> The script will not automatically run at least until the timeout is
> expired.
>
> But in case you are doing things by hand, how about:
>
> $ echo cancel > .../loading
>
> or if you want to keep the content numeric:
>
> $ echo -1 > .../loading
>
> This will also allow the regular script to just cancel the load in case
> of error, like if the firmware image is not available or a read error
> happened while reading it.
>
> I'll implement that and the other stuff that came out of Oliver's
> comments later today and post the new code.
>
> > Better to catch the close, check the length, then return the firmware
> > or throw the junk image away as appropriate.
>
> If 'loading' stays the above should fix your timeout issue, and if it
> goes, yes, that is probably the way to go.
How about combining these two ideas: instead of "loading" and "data"
we have "size" and "data". First you write the size, then the data -
the driver accepts it once it gets the expected number of bytes.
Writing a new size throws away any partial image that's there, and
restarts the upload. Writing 0 cancels the upload entirely, and the
driver will presumably fail to initialize (or maybe use a default
image if it has one).
There's another question - what happens if userspace tries to write an
image when the driver hasn't requested one through hotplug?
--
David Gibson | For every complex problem there is a
[email protected] | solution which is simple, neat and
| wrong.
http://www.ozlabs.org/people/dgibson
On Sat, May 17, 2003 at 07:07:05PM +1000, David Gibson wrote:
> On Sat, May 17, 2003 at 10:46:12AM +0200, Manuel Estrada Sainz wrote:
> > On Sat, May 17, 2003 at 02:44:59PM +1000, David Gibson wrote:
> > > On Fri, May 16, 2003 at 05:03:38PM -0700, Greg Kroah-Hartman wrote:
> > > > On Sat, May 17, 2003 at 01:55:15AM +0200, Oliver Neukum wrote:
> > > > >
[snip]
> How about combining these two ideas: instead of "loading" and "data"
> we have "size" and "data". First you write the size, then the data -
> the driver accepts it once it gets the expected number of bytes.
> Writing a new size throws away any partial image that's there, and
> restarts the upload. Writing 0 cancels the upload entirely, and the
> driver will presumably fail to initialize (or maybe use a default
> image if it has one).
Sounds good. I'll try that.
> There's another question - what happens if userspace tries to write an
> image when the driver hasn't requested one through hotplug?
If you use request_firmware() directly, userspace will not find any
file to write to :-)
If you really want to be able to do that, instead of using
request_firmware() you could register a class_device of class firmware
and implement 'size'_show/store and 'data'_read/write. Thus exposing
the firmware memory to userspace. The sequence would be like this:
- On size_store you setup the download.
- You can write directly to the device's memory on data_write.
- Once you get all data you get the device back working.
With this approach, you get the hotplug event for free and can take
advantage of compatible userspace scripts.
This is not currently possible, but should be easy to do if found
interesting. I'll provide a (non-functional) sample next time.
Thanks
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
On Sat, May 17, 2003 at 07:07:05PM +1000, David Gibson wrote:
> On Sat, May 17, 2003 at 10:46:12AM +0200, Manuel Estrada Sainz wrote:
> > On Sat, May 17, 2003 at 02:44:59PM +1000, David Gibson wrote:
> > > On Fri, May 16, 2003 at 05:03:38PM -0700, Greg Kroah-Hartman wrote:
> > > > On Sat, May 17, 2003 at 01:55:15AM +0200, Oliver Neukum wrote:
> > > > >
[snip]
> > But in case you are doing things by hand, how about:
> >
> > $ echo cancel > .../loading
> >
> > or if you want to keep the content numeric:
> >
> > $ echo -1 > .../loading
> >
> > This will also allow the regular script to just cancel the load in case
> > of error, like if the firmware image is not available or a read error
> > happened while reading it.
> >
> > I'll implement that and the other stuff that came out of Oliver's
> > comments later today and post the new code.
> >
> > > Better to catch the close, check the length, then return the firmware
> > > or throw the junk image away as appropriate.
> >
> > If 'loading' stays the above should fix your timeout issue, and if it
> > goes, yes, that is probably the way to go.
>
> How about combining these two ideas: instead of "loading" and "data"
> we have "size" and "data". First you write the size, then the data -
> the driver accepts it once it gets the expected number of bytes.
> Writing a new size throws away any partial image that's there, and
> restarts the upload. Writing 0 cancels the upload entirely, and the
> driver will presumably fail to initialize (or maybe use a default
> image if it has one).
I just thought this over. This makes more requirements for the userspace
scripts, they will need some way to get the size of the image: stat, or
ls and some crude regex.
And we can have the same effect with loading/data:
echo 1 > .../loading:
Will start a load, discarding any previous partial load.
echo 0 > .../loading:
Will conclude the load and handle the data to the driver code.
echo -1 > .../loading:
Will conclude the load with an error and the driver won't get
any firmware, failing or using firmware in some flash if
available.
This way, the script also won't have to check the value of 'loading'.
How does that sound?
Thanks
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
On Fri, May 16, 2003 at 03:36:24PM -0700, Greg KH wrote:
> On Thu, May 15, 2003 at 10:03:24PM +0200, Manuel Estrada Sainz wrote:
[snip]
> > Attached:
> > firmware.h
> > firmware_class.c:
> > The firmware support itself.
>
> Can you just send this as a patch to the current kernel next time? It's
> much easier to read and test with that way :)
When I updated my tree (via bk cvs gateway) to make the patch I noticed
some changes in sysfs's binary support.
In general, they look good, but the size of files is set in
sysfs_create_bin_file and not changeable later. This breaks
firmware_class.c :(
With current request_firmware(), the drivers don't tell the size of the
firmware, and in some cases they don't even know, so changing the
interface is no good.
I also don't understand why sysfs needs to keep a copy of the data in
it's own buffer. It has to ask the driver for any read/write anyway,
the previous approach of one page at a time looked better to me and
saves some kernel memory :-).
And the size checks could be skipped in case of zero size.
I'll include a change proposal to sysfs/bin.c next time.
Have a nice day
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
> > If the device losses the firmware upon suspend, the driver will have to
> > reinitialize it as if it just got plugged, which somehow makes all
> > devices hotplugable.
>
> So all firmware has to be permanently in RAM anyway?
Of course not you can just go back out to user space and ask for it
> > - In a diskless client, it is the network card
Already insoluble because of routing daemons.
> No, still no good. It means that you get a memory leak if you unload
> a driver before firmware is provided. You need the ability to explicitely
> cancel a request for firmware.
Only if you program it wrongly. Its not exactly hard.
As to an interface. The simplest is probably
request_firmware()
and
request_firmware_nowait(......, workqueuehandler)
The issues brought up about it failing appear bogus too, if the hotplug
run returns a non zero exit code you know about this already.
Alan
On Fri, May 16, 2003 at 03:36:24PM -0700, Greg KH wrote:
> Other than those very minor tweaks, I like this interface, it's looking
> very good. I wouldn't worry about any "checksum" calcuation crud, it's
> up to the userspace tool dumping the firmware to the kernel to make sure
Ok, if that is true, then we could also have this tool enforce a
size. Otherwise we are reading and reading without ever ending
and allocating a lot of kernel resources while we are at it.
If we know the size, then we also now start and end. So the
"loading" attribute can certainly go.
Regards
Ingo Oeser
On Sat, May 17, 2003 at 02:23:32PM +0100, Alan Cox wrote:
[snip]
> > No, still no good. It means that you get a memory leak if you unload
> > a driver before firmware is provided. You need the ability to explicitely
> > cancel a request for firmware.
>
> Only if you program it wrongly. Its not exactly hard.
>
> As to an interface. The simplest is probably
>
> request_firmware()
> and
> request_firmware_nowait(......, workqueuehandler)
This is what I plan to use:
int request_firmware_nowait (
struct module *module,
const char *name, const char *device, void *context,
void (*cont)(const struct firmware *fw, void *context));
Working code should be available later today.
> The issues brought up about it failing appear bogus too, if the hotplug
> run returns a non zero exit code you know about this already.
In this case, hotplug gets called automatically by the device model, and
there is no way to get the return value of the hotplug run.
If I get any good feedback on it, I'll try to make that return value
available, in the mean while, I already implemented a timeout.
Have a nice day
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
On Sat, May 17, 2003 at 03:21:29PM +0200, Ingo Oeser wrote:
> On Fri, May 16, 2003 at 03:36:24PM -0700, Greg KH wrote:
> > Other than those very minor tweaks, I like this interface, it's looking
> > very good. I wouldn't worry about any "checksum" calcuation crud, it's
> > up to the userspace tool dumping the firmware to the kernel to make sure
>
> Ok, if that is true, then we could also have this tool enforce a
> size. Otherwise we are reading and reading without ever ending
> and allocating a lot of kernel resources while we are at it.
A privileged script will be able to kill the system anyway it wants, I
don't think that this is needed.
However, forcing an standard header on firmware images including the
size would allow size checks without the need for an specialized tool.
Or to prevent memory reallocation every PAGE_SIZE bytes, a tentative
size could be written to 'loading' so its meaning would become:
loading > 1:
Start/restart a load of the specified size.
loading = 1:
Start/restart a load of unknown size.
loading = 0:
Finish load.
loading = -1:
Cancel load.
> If we know the size, then we also now start and end. So the
> "loading" attribute can certainly go.
I personally like to have explicit start/end, which also allows
cancellation and restart of the firmware load.
Regards
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
On Sat, May 17, 2003 at 12:30:37PM +0200, Manuel Estrada Sainz wrote:
> On Sat, May 17, 2003 at 07:07:05PM +1000, David Gibson wrote:
> > On Sat, May 17, 2003 at 10:46:12AM +0200, Manuel Estrada Sainz wrote:
> > > On Sat, May 17, 2003 at 02:44:59PM +1000, David Gibson wrote:
> > > > On Fri, May 16, 2003 at 05:03:38PM -0700, Greg Kroah-Hartman wrote:
> > > > > On Sat, May 17, 2003 at 01:55:15AM +0200, Oliver Neukum wrote:
> > > > > >
> [snip]
> > > But in case you are doing things by hand, how about:
> > >
> > > $ echo cancel > .../loading
> > >
> > > or if you want to keep the content numeric:
> > >
> > > $ echo -1 > .../loading
> > >
> > > This will also allow the regular script to just cancel the load in case
> > > of error, like if the firmware image is not available or a read error
> > > happened while reading it.
> > >
> > > I'll implement that and the other stuff that came out of Oliver's
> > > comments later today and post the new code.
> > >
> > > > Better to catch the close, check the length, then return the firmware
> > > > or throw the junk image away as appropriate.
> > >
> > > If 'loading' stays the above should fix your timeout issue, and if it
> > > goes, yes, that is probably the way to go.
> >
> > How about combining these two ideas: instead of "loading" and "data"
> > we have "size" and "data". First you write the size, then the data -
> > the driver accepts it once it gets the expected number of bytes.
> > Writing a new size throws away any partial image that's there, and
> > restarts the upload. Writing 0 cancels the upload entirely, and the
> > driver will presumably fail to initialize (or maybe use a default
> > image if it has one).
>
> I just thought this over. This makes more requirements for the userspace
> scripts, they will need some way to get the size of the image: stat, or
> ls and some crude regex.
>
> And we can have the same effect with loading/data:
>
> echo 1 > .../loading:
> Will start a load, discarding any previous partial load.
> echo 0 > .../loading:
> Will conclude the load and handle the data to the driver code.
> echo -1 > .../loading:
> Will conclude the load with an error and the driver won't get
> any firmware, failing or using firmware in some flash if
> available.
>
> This way, the script also won't have to check the value of 'loading'.
>
> How does that sound?
Hrm... it still seems a bit icky to me, but I'm not really sure why.
I think it would be a bit better if you called it "control" or
something instead of "loading". "loading" seems to imply a boolean,
which this isn't anymore.
--
David Gibson | For every complex problem there is a
[email protected] | solution which is simple, neat and
| wrong.
http://www.ozlabs.org/people/dgibson
On Tue, May 20, 2003 at 03:21:58PM +1000, David Gibson wrote:
> On Sat, May 17, 2003 at 12:30:37PM +0200, Manuel Estrada Sainz wrote:
> > On Sat, May 17, 2003 at 07:07:05PM +1000, David Gibson wrote:
> > > On Sat, May 17, 2003 at 10:46:12AM +0200, Manuel Estrada Sainz wrote:
> > > > On Sat, May 17, 2003 at 02:44:59PM +1000, David Gibson wrote:
> > > > > On Fri, May 16, 2003 at 05:03:38PM -0700, Greg Kroah-Hartman wrote:
> > > > > > On Sat, May 17, 2003 at 01:55:15AM +0200, Oliver Neukum wrote:
> > > > > > >
> > [snip]
> > > > But in case you are doing things by hand, how about:
> > > >
> > > > $ echo cancel > .../loading
> > > >
> > > > or if you want to keep the content numeric:
> > > >
> > > > $ echo -1 > .../loading
> > > >
> > > > This will also allow the regular script to just cancel the load in case
> > > > of error, like if the firmware image is not available or a read error
> > > > happened while reading it.
> > > >
> > > > I'll implement that and the other stuff that came out of Oliver's
> > > > comments later today and post the new code.
> > > >
> > > > > Better to catch the close, check the length, then return the firmware
> > > > > or throw the junk image away as appropriate.
> > > >
> > > > If 'loading' stays the above should fix your timeout issue, and if it
> > > > goes, yes, that is probably the way to go.
> > >
> > > How about combining these two ideas: instead of "loading" and "data"
> > > we have "size" and "data". First you write the size, then the data -
> > > the driver accepts it once it gets the expected number of bytes.
> > > Writing a new size throws away any partial image that's there, and
> > > restarts the upload. Writing 0 cancels the upload entirely, and the
> > > driver will presumably fail to initialize (or maybe use a default
> > > image if it has one).
> >
> > I just thought this over. This makes more requirements for the userspace
> > scripts, they will need some way to get the size of the image: stat, or
> > ls and some crude regex.
> >
> > And we can have the same effect with loading/data:
> >
> > echo 1 > .../loading:
> > Will start a load, discarding any previous partial load.
> > echo 0 > .../loading:
> > Will conclude the load and handle the data to the driver code.
> > echo -1 > .../loading:
> > Will conclude the load with an error and the driver won't get
> > any firmware, failing or using firmware in some flash if
> > available.
> >
> > This way, the script also won't have to check the value of 'loading'.
> >
> > How does that sound?
>
> Hrm... it still seems a bit icky to me, but I'm not really sure why.
> I think it would be a bit better if you called it "control" or
> something instead of "loading". "loading" seems to imply a boolean,
> which this isn't anymore.
It is still some kind of boolean, but I don't mind to follow the crowd
on this one.
What do you guys thing is best?
a) loading
b) control
c) other:_____
Thanks
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
On Tue, May 20, 2003 at 09:21:53PM -0700, Greg KH wrote:
> On Tue, May 20, 2003 at 10:07:40AM +0200, Manuel Estrada Sainz wrote:
> > What do you guys thing is best?
> >
> > a) loading
> > b) control
> > c) other:_____
>
> d) Don't care.
>
> "loading" or "control" is fine with me, as long as it's documented :)
Once we agree on the code, I'll write some documentation for it.
Did you guys see my last post on Saturday? It is supposed to fix all
issues, including request_firmware_nowait() and a couple of sysfs
patches.
Maybe it went by unnoticed, or am I just too impatient?
Have a nice day
Manuel
--
--- Manuel Estrada Sainz <[email protected]>
<[email protected]>
<[email protected]>
------------------------ <[email protected]> -------------------
Let us have the serenity to accept the things we cannot change, courage to
change the things we can, and wisdom to know the difference.
On Tue, May 20, 2003 at 10:07:40AM +0200, Manuel Estrada Sainz wrote:
> What do you guys thing is best?
>
> a) loading
> b) control
> c) other:_____
d) Don't care.
"loading" or "control" is fine with me, as long as it's documented :)
thanks,
greg k-h