[PATCH 00/04] Load keyspan firmware with hotplug

[PATCH 00/04] Load keyspan firmware with hotplug

[Jan Harkes]

On Mon, Apr 04, 2005 at 12:17:46PM -0700, Greg KH wrote:
> On Mon, Apr 04, 2005 at 08:27:53PM +0200, Sven Luther wrote:
> > Mmm, probably that 2001 discussion about the keyspan firmware, right ?
> > 
> >   http://lists.debian.org/debian-legal/2001/04/msg00145.html
> > 
> > Can you summarize the conclusion of the thread, or what you did get from it,
> > please ? 
> 
> That people didn't like the inclusion of firmware, I posted how you can
> fix it by moving it outside of the kernel, and asked for patches.
> 
> None have come.

Didn't know you were waiting for it. How about something like the
following series of patches?

[01/04] - add simple Intel IHEX format parser to the firmware loader.
[02/04] - make the keyspan driver use request_firmware.
[03/04] - converter program used to dump the keyspan headers as IHex files.
[04/04] - result of running the previous program.

This ofcourse doesn't actually solve Debian's distribution issues since
the keyspan firmware can only be distributed as part of 'Linux or other
Open Source operating system kernel'.

> So I refuse to listen to talk about this, as obviously, no one cares
> enough about this to actually fix the issue.

I got tired of always building my own kernels on Debian just to get my
serial dongle to work since their included keyspan.ko driver is so
useless that it isn't even worth having. The only way to use it with a
Debian kernel is to have the dongle in a powered hub and first boot into
Windows or a normal kernel.org kernel to get the thing initialized.
Didn't send the patch earlier since I wanted to split off the
pre-numeration part of the driver so that after intialization we can
unload the unused parts of the driver as well as the the firmware class
module.

Jan

Re: [PATCH 00/04] Load keyspan firmware with hotplug

[Dmitry Torokhov]

On Monday 04 April 2005 23:23, Jan Harkes wrote:
> On Mon, Apr 04, 2005 at 12:17:46PM -0700, Greg KH wrote:
> > On Mon, Apr 04, 2005 at 08:27:53PM +0200, Sven Luther wrote:
> > > Mmm, probably that 2001 discussion about the keyspan firmware, right ?
> > > 
> > >   http://lists.debian.org/debian-legal/2001/04/msg00145.html
> > > 
> > > Can you summarize the conclusion of the thread, or what you did get from it,
> > > please ? 
> > 
> > That people didn't like the inclusion of firmware, I posted how you can
> > fix it by moving it outside of the kernel, and asked for patches.
> > 
> > None have come.
> 
> Didn't know you were waiting for it. How about something like the
> following series of patches?
> 
> [01/04] - add simple Intel IHEX format parser to the firmware loader.

Firmware loader is format-agnostic, I think having IHEX parser in a separate
file would be better...

-- 
Dmitry

Re: [PATCH 00/04] Load keyspan firmware with hotplug

[Sven Luther]

On Tue, Apr 05, 2005 at 12:23:29AM -0400, Jan Harkes wrote:
> On Mon, Apr 04, 2005 at 12:17:46PM -0700, Greg KH wrote:
> > On Mon, Apr 04, 2005 at 08:27:53PM +0200, Sven Luther wrote:
> > > Mmm, probably that 2001 discussion about the keyspan firmware, right ?
> > > 
> > >   http://lists.debian.org/debian-legal/2001/04/msg00145.html
> > > 
> > > Can you summarize the conclusion of the thread, or what you did get from it,
> > > please ? 
> > 
> > That people didn't like the inclusion of firmware, I posted how you can
> > fix it by moving it outside of the kernel, and asked for patches.
> > 
> > None have come.
> 
> Didn't know you were waiting for it. How about something like the
> following series of patches?
> 
> [01/04] - add simple Intel IHEX format parser to the firmware loader.
> [02/04] - make the keyspan driver use request_firmware.
> [03/04] - converter program used to dump the keyspan headers as IHex files.
> [04/04] - result of running the previous program.
> 
> This ofcourse doesn't actually solve Debian's distribution issues since
> the keyspan firmware can only be distributed as part of 'Linux or other
> Open Source operating system kernel'.

Well, if this is the case, it can be distributed on the non-free archive.

> > So I refuse to listen to talk about this, as obviously, no one cares
> > enough about this to actually fix the issue.
> 
> I got tired of always building my own kernels on Debian just to get my
> serial dongle to work since their included keyspan.ko driver is so
> useless that it isn't even worth having. The only way to use it with a
> Debian kernel is to have the dongle in a powered hub and first boot into
> Windows or a normal kernel.org kernel to get the thing initialized.

The non-free driver package should solve that for you.

> Didn't send the patch earlier since I wanted to split off the
> pre-numeration part of the driver so that after intialization we can
> unload the unused parts of the driver as well as the the firmware class
> module.

Friendly,

Sven Luther

Re: [PATCH 00/04] Load keyspan firmware with hotplug

[Kay Sievers]

On Mon, 2005-04-04 at 23:51 -0500, Dmitry Torokhov wrote:
> On Monday 04 April 2005 23:23, Jan Harkes wrote:
> > On Mon, Apr 04, 2005 at 12:17:46PM -0700, Greg KH wrote:
> > > On Mon, Apr 04, 2005 at 08:27:53PM +0200, Sven Luther wrote:
> > > > Mmm, probably that 2001 discussion about the keyspan firmware, right ?
> > > > 
> > > >   http://lists.debian.org/debian-legal/2001/04/msg00145.html
> > > > 
> > > > Can you summarize the conclusion of the thread, or what you did get from it,
> > > > please ? 
> > > 
> > > That people didn't like the inclusion of firmware, I posted how you can
> > > fix it by moving it outside of the kernel, and asked for patches.
> > > 
> > > None have come.
> > 
> > Didn't know you were waiting for it. How about something like the
> > following series of patches?
> > 
> > [01/04] - add simple Intel IHEX format parser to the firmware loader.
> 
> Firmware loader is format-agnostic, I think having IHEX parser in a separate
> file would be better...

Why should this be in-kernel at all? Convert the firmware into a binary
blob or do it in the userspace request.

Kay

Re: [PATCH 00/04] Load keyspan firmware with hotplug

[Marcel Holtmann]

Hi Jan,

> > > > Mmm, probably that 2001 discussion about the keyspan firmware, right ?
> > > > 
> > > >   http://lists.debian.org/debian-legal/2001/04/msg00145.html
> > > > 
> > > > Can you summarize the conclusion of the thread, or what you did get from it,
> > > > please ? 
> > > 
> > > That people didn't like the inclusion of firmware, I posted how you can
> > > fix it by moving it outside of the kernel, and asked for patches.
> > > 
> > > None have come.
> > 
> > Didn't know you were waiting for it. How about something like the
> > following series of patches?
> > 
> > [01/04] - add simple Intel IHEX format parser to the firmware loader.
> 
> Firmware loader is format-agnostic, I think having IHEX parser in a separate
> file would be better...

I agree with Dmitry on this point. The IHEX parser should not be inside
firmware_class.c. What about using keyspan_ihex.[ch] for it?

Regards

Marcel

Re: [PATCH 00/04] Load keyspan firmware with hotplug

[Jan Harkes]

On Tue, Apr 05, 2005 at 10:32:38AM +0200, Kay Sievers wrote:
> On Mon, 2005-04-04 at 23:51 -0500, Dmitry Torokhov wrote:
> > Firmware loader is format-agnostic, I think having IHEX parser in a separate
> > file would be better...
> 
> Why should this be in-kernel at all? Convert the firmware into a binary
> blob or do it in the userspace request.

Because the keyspan headers seem to have a specific sequence of
operations, something like 'load these 5 bytes at offset 10, load this
byte at offset 0, etc.' I don't know if there is some magic in that
sequence. Without knowing what is in the firmware, it looks to me like a
little bit of bootstrap code is loaded first.

Jan

Re: [PATCH 00/04] Load keyspan firmware with hotplug

[Jan Harkes]

On Tue, Apr 05, 2005 at 11:22:06AM +0200, Marcel Holtmann wrote:
> I agree with Dmitry on this point. The IHEX parser should not be inside
> firmware_class.c. What about using keyspan_ihex.[ch] for it?

That's what I had originally, actually called firmware_ihex.ko, since
the IHEX format parser is not in any way keyspan specific and there are
several usb-serial converters that seem to use the same IHEX->.h trick
which could trivially be modified to use this loader.

But the compiled parser fairly small (< 2KB) and adding it to the
existing module didn't effectively add any size to the firmware_class
module since things are rounded to a page boundary anyways.

Jan

Re: [PATCH 00/04] Load keyspan firmware with hotplug

[Marcel Holtmann]

Hi Jan,

> > I agree with Dmitry on this point. The IHEX parser should not be inside
> > firmware_class.c. What about using keyspan_ihex.[ch] for it?
> 
> That's what I had originally, actually called firmware_ihex.ko, since
> the IHEX format parser is not in any way keyspan specific and there are
> several usb-serial converters that seem to use the same IHEX->.h trick
> which could trivially be modified to use this loader.
> 
> But the compiled parser fairly small (< 2KB) and adding it to the
> existing module didn't effectively add any size to the firmware_class
> module since things are rounded to a page boundary anyways.

so it seems that this is usb-serial specific at the moment. Then I would
propose to add it to the core of the usb-serial driver. Unless no other
driver in the kernel needs a IHEX parser, I think it is bad idea to mess
it up at the moment. People are also working on a replacement for the
current request_firmware(), because the needs are changing. Try to keep
it close with the usb-serial for now.

Regards

Marcel

Re: [PATCH 00/04] Load keyspan firmware with hotplug

[Humberto Massa]

Sven Luther wrote:

>  On Tue, Apr 05, 2005 at 12:23:29AM -0400, Jan Harkes wrote:
> > This ofcourse doesn't actually solve Debian's distribution issues since
> > the keyspan firmware can only be distributed as part of 'Linux or other
> > Open Source operating system kernel'.
>
>
>  Well, if this is the case, it can be distributed on the non-free
>  archive.

Nope, looks awfully non-distributable for me, unless you put it into a 
kernel-non-free with the entire kernel -- but it says clearly that it's 
undistributable by itself.


HTH,
Massa

Re: [PATCH 00/04] Load keyspan firmware with hotplug

[Dmitry Torokhov]

On Apr 5, 2005 9:36 AM, Marcel Holtmann <marcel@holtmann.org> wrote:

> People are also working on a replacement for the
> current request_firmware(), because the needs are changing. Try to keep
> it close with the usb-serial for now.
> 

Could you elaborate on what do you think is needed? I have some of
patches to firmware loader and wondering if we should coordinate out
efforts. I have

1. convert from using a timer to wait_for_comletion_timeout which
simplifies logic
2. tightening of state transition rules and removing possible memory
leak (very unlikely)
3. converting firmware_priv to fw_class_dev to simplify memory management.
4. removing request_firmware_nowait as noone seems to be using it -
and I doubt one would ever want to request firmware from an interrupt.
5. Creating firmware class in a separate thread to work around selinux
(with prism54 firmware is loaded when interface is configured and thus
firware loader runs in context of /sbin/ip which may not have access
to sysfs. Having separate thread will ensure that firmware loader runs
in kernel context).

And I was thinking about caching firmware (siomething like if you do
"echo 2 > /sys/class/firmware/blah/loading" instead of 0 it will keep
a copy of the firmware in memory. One could see all firmwares in
/sys/class/firmware/loaded/<xxx> and by erase cached firmware by
echoing 0 into it).

What do you think?

-- 
Dmitry

Re: [PATCH 00/04] Load keyspan firmware with hotplug

[Jan Harkes]

On Tue, Apr 05, 2005 at 04:36:31PM +0200, Marcel Holtmann wrote:
> > > I agree with Dmitry on this point. The IHEX parser should not be inside
> > > firmware_class.c. What about using keyspan_ihex.[ch] for it?
> > 
> > That's what I had originally, actually called firmware_ihex.ko, since
> > the IHEX format parser is not in any way keyspan specific and there are
> > several usb-serial converters that seem to use the same IHEX->.h trick
> > which could trivially be modified to use this loader.
> > 
> > But the compiled parser fairly small (< 2KB) and adding it to the
> > existing module didn't effectively add any size to the firmware_class
> > module since things are rounded to a page boundary anyways.
> 
> so it seems that this is usb-serial specific at the moment. Then I would

I really don't see the point you are trying to argue. I said, sure it is
possible to make it a separate module, that is what my initial
implementation was. Why do you want to pigeon-hole it with anything but
the existing firmware loading code?

It is _not_ usb-serial specific, in fact once the device is initialized
this isn't even needed. And the initialization as far as I can see uses
little or no usb-serial code.

It happens that many usb-serial devices are built around the ezusb
chipset, which in turn seems to be a 8051-based microcontroller. The
compilers for such microcontrollers seem to generate IHEX formatted
output possibly because eprom burners generally support the format.

> it up at the moment. People are also working on a replacement for the
> current request_firmware(), because the needs are changing. Try to keep
> it close with the usb-serial for now.

What? I find the existing request_firmware fairly simple and
straightforward, it has a very KISS-like quality to it, it is nice and
small and even the userspace support is trivial. I only saw a mention
about 'replacing' it in the current thread which mostly involved
complaints but didn't actually see anyone volunteering to start working
on such a replacement.

If a driver wants to load 5 different chunks, just call request_firmware
5 times (i.e. drivers/bluetooth/bcm203x.c). If the data is a single
binary blob, just ask for the single binary blob. In this case there
seems to be some structure to the blob that I wanted to preserve, and
that would either be some custom binary format that contains
[<address><length><data>]... tuples, which ofcourse causes problems for
our big-endian brothers, or a similar ascii format, where the IHEX is
not only pretty much standardized, it is trivial to parse and even adds
checksum information.

The only thing that I see missing right now is a change to the makefiles
to have in-tree firmware files get installed in /lib/modules/`uname
-r`/firmware or some similar place. Ideally people would add a line
like,

    fw-$(CONFIG_FOO) = foo-firmware-blob.fw

And make install could drop it a place where hotplug can find it.

Jan

Re: [PATCH 00/04] Load keyspan firmware with hotplug

[Marcel Holtmann]

Hi Dmitry,

> > People are also working on a replacement for the
> > current request_firmware(), because the needs are changing. Try to keep
> > it close with the usb-serial for now.
> > 
> 
> Could you elaborate on what do you think is needed? I have some of
> patches to firmware loader and wondering if we should coordinate out
> efforts. I have
> 
> 1. convert from using a timer to wait_for_comletion_timeout which
> simplifies logic
> 2. tightening of state transition rules and removing possible memory
> leak (very unlikely)
> 3. converting firmware_priv to fw_class_dev to simplify memory management.
> 4. removing request_firmware_nowait as noone seems to be using it -
> and I doubt one would ever want to request firmware from an interrupt.
> 5. Creating firmware class in a separate thread to work around selinux
> (with prism54 firmware is loaded when interface is configured and thus
> firware loader runs in context of /sbin/ip which may not have access
> to sysfs. Having separate thread will ensure that firmware loader runs
> in kernel context).
> 
> And I was thinking about caching firmware (siomething like if you do
> "echo 2 > /sys/class/firmware/blah/loading" instead of 0 it will keep
> a copy of the firmware in memory. One could see all firmwares in
> /sys/class/firmware/loaded/<xxx> and by erase cached firmware by
> echoing 0 into it).
> 
> What do you think?

actually there is a thread about firmware loading rewrite and POST
calling of programs. It must be on LKML or the hotplug mailing list.
However my backlog for the interesting topics of these lists increased
while I was traveling the last 5-6 weeks. I think you should simply
start a new one on LKML.

Regards

Marcel

Re: [PATCH 00/04] Load keyspan firmware with hotplug

[Dmitry Torokhov]

On Apr 5, 2005 6:45 AM, Jan Harkes <jaharkes@cs.cmu.edu> wrote:
> On Tue, Apr 05, 2005 at 11:22:06AM +0200, Marcel Holtmann wrote:
> > I agree with Dmitry on this point. The IHEX parser should not be inside
> > firmware_class.c. What about using keyspan_ihex.[ch] for it?
> 
> That's what I had originally, actually called firmware_ihex.ko, since
> the IHEX format parser is not in any way keyspan specific and there are
> several usb-serial converters that seem to use the same IHEX->.h trick
> which could trivially be modified to use this loader.
> 
> But the compiled parser fairly small (< 2KB) and adding it to the
> existing module didn't effectively add any size to the firmware_class
> module since things are rounded to a page boundary anyways.
> 

It is not about the size, it is about source separation. Since it has
nothing to do with actualy loading of a BLOB from userspace to kernel
space I'd put it into lib/, like crc functions, etc. It does not
really have to work with "struct firmware *", "void *data, size_t len"
should be just as useable.

-- 
Dmitry

Re: [PATCH 00/04] Load keyspan firmware with hotplug

[Marcel Holtmann]

Hi Jan,

> > > > I agree with Dmitry on this point. The IHEX parser should not be inside
> > > > firmware_class.c. What about using keyspan_ihex.[ch] for it?
> > > 
> > > That's what I had originally, actually called firmware_ihex.ko, since
> > > the IHEX format parser is not in any way keyspan specific and there are
> > > several usb-serial converters that seem to use the same IHEX->.h trick
> > > which could trivially be modified to use this loader.
> > > 
> > > But the compiled parser fairly small (< 2KB) and adding it to the
> > > existing module didn't effectively add any size to the firmware_class
> > > module since things are rounded to a page boundary anyways.
> > 
> > so it seems that this is usb-serial specific at the moment. Then I would
> 
> I really don't see the point you are trying to argue. I said, sure it is
> possible to make it a separate module, that is what my initial
> implementation was. Why do you want to pigeon-hole it with anything but
> the existing firmware loading code?

the existing request_firmware() has nothing in common with IHEX parser
and such a parser should not belong there. So either make it a separate
module or add it to the module that is using it. In this case this is
the keyspan module or the usb-serial core.

> It is _not_ usb-serial specific, in fact once the device is initialized
> this isn't even needed. And the initialization as far as I can see uses
> little or no usb-serial code.
> 
> It happens that many usb-serial devices are built around the ezusb
> chipset, which in turn seems to be a 8051-based microcontroller. The
> compilers for such microcontrollers seem to generate IHEX formatted
> output possibly because eprom burners generally support the format.

Then make it at separate module.

> > it up at the moment. People are also working on a replacement for the
> > current request_firmware(), because the needs are changing. Try to keep
> > it close with the usb-serial for now.
> 
> What? I find the existing request_firmware fairly simple and
> straightforward, it has a very KISS-like quality to it, it is nice and
> small and even the userspace support is trivial. I only saw a mention
> about 'replacing' it in the current thread which mostly involved
> complaints but didn't actually see anyone volunteering to start working
> on such a replacement.
> 
> If a driver wants to load 5 different chunks, just call request_firmware
> 5 times (i.e. drivers/bluetooth/bcm203x.c). If the data is a single
> binary blob, just ask for the single binary blob. In this case there
> seems to be some structure to the blob that I wanted to preserve, and
> that would either be some custom binary format that contains
> [<address><length><data>]... tuples, which ofcourse causes problems for
> our big-endian brothers, or a similar ascii format, where the IHEX is
> not only pretty much standardized, it is trivial to parse and even adds
> checksum information.

I am not going to repeat the current arguments and it is not about
loading multiple firmware files (btw I wrote the bcm203x). Check the
mailing list archives for the details. I still need to catch up with the
discussion, but there is some ongoing work.

> The only thing that I see missing right now is a change to the makefiles
> to have in-tree firmware files get installed in /lib/modules/`uname
> -r`/firmware or some similar place. Ideally people would add a line
> like,
> 
>     fw-$(CONFIG_FOO) = foo-firmware-blob.fw
> 
> And make install could drop it a place where hotplug can find it.

This is another approach and if you want something like that, then send
a patch for it and let Sam and others comment on it.

Regards

Marcel