SmPL for automatic request_firmware_nowait() conversion

SmPL for automatic request_firmware_nowait() conversion

[Luis R. Rodriguez]

I was just porting over an ethernet driver [0] to use request_firmware_nowait()
since firmware loading seems can take over a minute on one device, while
at it I noticed no other ethernet drivers yet use this API so figure
this may be a trend coming if devices are getting as complex as cxgb4.
The cxgb4 driver happens to even use the firmware API 3 times!

Obviously I considered writing SmPL for this, but one thing which seemed
hard was that for after the request_firmware_nowait() we tend to tuck
away into another new call the rest of the code that was in place in the
original function after the old request_firmware() call. Is there a way
to dump all that code into the new routine? I think the hardest thing
would be to also move the right set of variables over. In the third
patch in this series for example [1] there was a state variable that
I moved from beign static over to the ethernet private data structure.
Its hard for me to think of how I can hint to Coccinelle enough information
about what stuff it needs to move around. I think one hint would be:

  "Hey all that code that is static and is used *before* and *after* request_firmware()
   stuff it into the private data structure"

We'd have to infer the private data structure but that's easy and I already know
that's possible. Is this possible? The only other challenge I thought
might be tough would be to come up with are rasonable call for the
completion call, but I guess we can use the original routine name
where request_firmware() was being used and postfix _completion or something.

netdev: how worthy is this effort?

[0] https://lkml.org/lkml/2014/6/20/688
[1] https://lkml.org/lkml/2014/6/20/691
 
  Luis

Re: [Cocci] SmPL for automatic request_firmware_nowait() conversion

[Julia Lawall]

On Sat, 21 Jun 2014, Luis R. Rodriguez wrote:

> I was just porting over an ethernet driver [0] to use request_firmware_nowait()
> since firmware loading seems can take over a minute on one device, while
> at it I noticed no other ethernet drivers yet use this API so figure
> this may be a trend coming if devices are getting as complex as cxgb4.
> The cxgb4 driver happens to even use the firmware API 3 times!
> 
> Obviously I considered writing SmPL for this, but one thing which seemed
> hard was that for after the request_firmware_nowait() we tend to tuck
> away into another new call the rest of the code that was in place in the
> original function after the old request_firmware() call. Is there a way
> to dump all that code into the new routine? I think the hardest thing
> would be to also move the right set of variables over. In the third
> patch in this series for example [1] there was a state variable that
> I moved from beign static over to the ethernet private data structure.
> Its hard for me to think of how I can hint to Coccinelle enough information
> about what stuff it needs to move around. I think one hint would be:
> 
>   "Hey all that code that is static and is used *before* and *after* request_firmware()
>    stuff it into the private data structure"
> 
> We'd have to infer the private data structure but that's easy and I already know
> that's possible. Is this possible? The only other challenge I thought
> might be tough would be to come up with are rasonable call for the
> completion call, but I guess we can use the original routine name
> where request_firmware() was being used and postfix _completion or something.

This kind of thing is possible, but complicated.  Basically you have to 
put { } around what you want to move, then match a statement metavariable 
against the code that is now surrounded by braces, then find all of the 
variables that are read without being initialized in the { } region, or 
that are written by the { } region and used afterwards and arrange to copy 
them back and forth.  I did this in the context of a project where the 
goal was to move critical sections into separate functions, but it was 
around 1000 lines of SmPL code.

If one were doing this in several places onesself, then it would probably 
be better to do the fixed parts using SmPL and do the more variable parts 
by hand.  This is not really something that one would want to publoish for 
others, though.

julia

Re: [Cocci] SmPL for automatic request_firmware_nowait() conversion

[SF Markus Elfring]

> Obviously I considered writing SmPL for this, but one thing which seemed
> hard was that for after the request_firmware_nowait() we tend to tuck
> away into another new call the rest of the code that was in place in the
> original function after the old request_firmware() call. Is there a way
> to dump all that code into the new routine?

Does the refactoring "Extraction of an interface" fit also to your use case?
http://refactoring.com/catalog/extractInterface.html
http://c2.com/cgi/wiki?ExtractInterface


> I think the hardest thing would be to also move the right set of variables over.

The current syntax for semantic patches has got some limitations for its
expression power. I guess that it is still a software development challenge to
support also variations in involved statements.


> Its hard for me to think of how I can hint to Coccinelle enough information
> about what stuff it needs to move around. I think one hint would be:
> 
>   "Hey all that code that is static and is used *before* and *after* request_firmware()
>    stuff it into the private data structure"

Do you need dynamic source code introspection here?

Regards,
Markus

Re: SmPL for automatic request_firmware_nowait() conversion

[Francois Romieu]

Luis R. Rodriguez <mcgrof@suse.com> :
> I was just porting over an ethernet driver [0] to use request_firmware_nowait()
> since firmware loading seems can take over a minute on one device, while
> at it I noticed no other ethernet drivers yet use this API so figure
> this may be a trend coming if devices are getting as complex as cxgb4.
> The cxgb4 driver happens to even use the firmware API 3 times!

There should be no problem for the firmware requests issued through
ethtool in cxgb4.

[...]
> netdev: how worthy is this effort?

Biased comment below :o)

I'm wondering the benefit of automatic API changes when it could be argued
that the symptoms call for driver dependant code rework.

The 60 seconds delay is kind of a pavlovian signal: one can bet that the
driver includes a request_firmware in its probe method. So does cxgb4.

I still believe in the old school "request firmware from net_device_ops.open".
I've been happy with it since f1e02ed109df5f99abf942b8ccc99960cb09dd38.
This kind of rework may not be trivial for cxgb4. Please get code tested on
real hardware (modular/monolithic build, with/without firmware, etc.) as I
won't argue against your crusade for cxgb4.

Asynchronous firmware loading provides a rather nice high level API but
it's imho not the essence of network devices firmware loading problems.

-- 
Ueimor

Re: SmPL for automatic request_firmware_nowait() conversion

[Luis R. Rodriguez]

On Sat, Jun 21, 2014 at 12:52:05PM +0200, Francois Romieu wrote:
> Luis R. Rodriguez <mcgrof@suse.com> :
> > I was just porting over an ethernet driver [0] to use request_firmware_nowait()
> > since firmware loading seems can take over a minute on one device, while
> > at it I noticed no other ethernet drivers yet use this API so figure
> > this may be a trend coming if devices are getting as complex as cxgb4.
> > The cxgb4 driver happens to even use the firmware API 3 times!
> 
> There should be no problem for the firmware requests issued through
> ethtool in cxgb4.

OK.

> 
> [...]
> > netdev: how worthy is this effort?
> 
> Biased comment below :o)
> 
> I'm wondering the benefit of automatic API changes when it could be argued
> that the symptoms call for driver dependant code rework.

I should have elaborated a bit more on motivation. Tons of drivers don't use
request_firmware_nowait() which means boot time is increased given that probe
won't complete until firmware loading completes. The cxgb4 driver is a good
example of one that takes quite a bit of time so learning that firmware loading
can take over a minute seemed to beg a rework to use request_firmware_nowait().
Note that in the worst case if udev is present in the worst case if the firmware
is not present loading can take up to timeout * num CPUs [0], but work is
underway to try to remove udev firmware loading support [1]. Regardless
then the current approach seemed to beg a rework.

[0] https://lkml.org/lkml/2013/10/23/264
[1] http://comments.gmane.org/gmane.comp.sysutils.systemd.devel/19440

> The 60 seconds delay is kind of a pavlovian signal: one can bet that the
> driver includes a request_firmware in its probe method. So does cxgb4.

The kernel's timeout is 60 seconds, and this is static, but lets be clear
that there is a difference between the timeout for reading the firmware
from the filesystem and actually dumping the firmware onto the device
by the driver and ensuring that the driver is done poking at it. The
kernel timeout is for the kernel reading it and tossing it back to the
driver.

> I still believe in the old school "request firmware from net_device_ops.open".
> I've been happy with it since f1e02ed109df5f99abf942b8ccc99960cb09dd38.

I was actually in the hopes a suitable transormation can be designed
to put a wait_for_completion() in say ndo_init(). I was looking to
see if a tranformation can be generalized of course if the above
observations on probe() is something desirable to be addressed.

> This kind of rework may not be trivial for cxgb4. Please get code tested on
> real hardware (modular/monolithic build, with/without firmware, etc.) as I
> won't argue against your crusade for cxgb4.

OK I'll look for hardware but if some folks already have it and are
interesed in some parts of this patch set my hope was that it can be
slightly modified to move the wait_for_completion() to an ndo_init() then
as well. That would really put better use of the API.

> Asynchronous firmware loading provides a rather nice high level API but
> it's imho not the essence of network devices firmware loading problems.

Agreed.

  Luis

Re: [Cocci] SmPL for automatic request_firmware_nowait() conversion

[Luis R. Rodriguez]

On Tue, Jun 24, 2014 at 01:21:07AM +0200, Luis R. Rodriguez wrote:
> On Sat, Jun 21, 2014 at 12:52:05PM +0200, Francois Romieu wrote:
> > Luis R. Rodriguez <mcgrof@suse.com> :
> I was actually in the hopes a suitable transormation can be designed
> to put a wait_for_completion() in say ndo_init(). I was looking to
> see if a tranformation can be generalized of course if the above
> observations on probe() is something desirable to be addressed.

And a really cool check would be to transform all those non fatal
request_firmware() calls to request_firmware_direct().

  Luis

Re: SmPL for automatic request_firmware_nowait() conversion

[Francois Romieu]

Luis R. Rodriguez <mcgrof@suse.com> :
> On Sat, Jun 21, 2014 at 12:52:05PM +0200, Francois Romieu wrote:
> > Luis R. Rodriguez <mcgrof@suse.com> :
[...]
> Note that in the worst case if udev is present in the worst case if the
> firmware is not present loading can take up to timeout * num CPUs [0],
> but work is underway to try to remove udev firmware loading support [1].
> Regardless then the current approach seemed to beg a rework.
> 
> [0] https://lkml.org/lkml/2013/10/23/264
> [1] http://comments.gmane.org/gmane.comp.sysutils.systemd.devel/19440

Thanks for the pointers.

I can't help thinking that there's a bit of sensationalism in the
timeout * num CPUs argument though. :o)

[...]
> The kernel's timeout is 60 seconds, and this is static, but lets be clear
> that there is a difference between the timeout for reading the firmware
> from the filesystem and actually dumping the firmware onto the device
> by the driver and ensuring that the driver is done poking at it. The
> kernel timeout is for the kernel reading it and tossing it back to the
> driver.

You are right.

[...]
> I was actually in the hopes a suitable transormation can be designed
> to put a wait_for_completion() in say ndo_init(). I was looking to
> see if a tranformation can be generalized of course if the above
> observations on probe() is something desirable to be addressed.

register_netdev is commonly called at the end of the probe method. probe()
would thus schedule an async firmware loading, fly, then wait_for_completion()
through register_netdev->ndo_init(). If so there won't be much gain.

There could be more gain with a wait_for_completion() in ndo_open()
but probe() + ndo_open() would still hit any fw read/poke/complete wall.

It can be done but it will imvho require a lot of driver dependant work.

-- 
Ueimor

Re: SmPL for automatic request_firmware_nowait() conversion

[Luis R. Rodriguez]

On Tue, Jun 24, 2014 at 2:58 PM, Francois Romieu <romieu@fr.zoreil.com> wrote:
> Thanks for the pointers.
>
> I can't help thinking that there's a bit of sensationalism in the
> timeout * num CPUs argument though. :o)

Indeed, this actually only applies to microcode which will end up
using the API for each CPU it seems.

  Luis