Re: microcode loading got really slow.

[Takashi Iwai <tiwai@suse.de>]

At Thu, 23 May 2013 21:04:53 +0800,
Ming Lei wrote:
> 
> On Thu, May 23, 2013 at 8:05 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Thu, 23 May 2013 18:45:29 +0800,
> > Ming Lei wrote:
> >>
> >> On Thu, May 23, 2013 at 6:36 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> >
> >> > No, f/w loader always fall back to user mode helper, as long as its
> >> > support is built in.  And doing that for microcode driver in that code
> >> > path isn't only superfluous but also broken due to request_firmware
> >> > call in module init.
> >>
> >> Firstly, it is not good to do this since some distributions doesn't support
> >> direct loading and doesn't have udevd(such as, android).
> >>
> >> Secondly, returning failure from request_firmware_direct() doesn't mean
> >> the firmware doesn't exist since distribution may put the firmware other where.
> >
> > Right, the non-standard path is the problem, and basically the only
> > problem.  The distribution that doesn't support the direct loading
> > means nothing but that.
> 
> Suppose it is, it is the fact, and it isn't OK to break this distribution.
> 
> Also udev supports user-defined rules to load firmware, which
> means some drivers may not put their firmware in the default
> path of distribution's firmware.

It's why I suggested to put a warning in that path as the first step.
So we can see whether there is any actual user.

> >> Anyway, this example is very specific(no firmware can be accepted), and
> >> request_firmware_nowait() should be OK for the situation.
> >
> > Oh no, rewriting with request_firmware_nowait() should be really the
> > last choice.  It would change the code flow awfully bad in most
> > cases.
> >
> > The new kernel driver has a better firmware mechanism.  If it's only
> > the question of paths, we should move on toward that direction and
> > drop the too complex old way.  I'd vote for a warning shown when a
> 
> Simply dropping the old way may cause user space regression.

It's already broken :)

> > firmware file is loaded via user mode helper (except for explicit
> > cases like FW_ACTION_NOHOTPLUG), for example.
> 
> As it is a very driver specific problem, it is better to solve it inside driver.

Yes, this proposal is basically not meant as a fix for this particular
issue but rather for future movement in general.

> >> >> wrt. this problem, I think we
> >> >> need to know why the direct loading is failed.
> >> >
> >> > The reason is obvious: the requested f/w file doesn't exist.
> >> > And it's fine, because the microcode update is an optional operation.
> >> > If no f/w file is found, it's not handled as an error.  It just means
> >> > that no need to update, continuing to work.
> >>
> >> OK, as said above, the example is very specific, and might be
> >> workarounded by request_firmware_nowait().
> >
> > It's not that easy in this case.  The microcode loader driver core
> > module doesn't invoke request_firmware() directly but it's via cpu
> > driver.  And the same callback is called in different code paths, not
> > only at init but also via sysfs write.  Thus the request_firmware()
> > call must be synchronous there.
> 
> I don't think the way is too difficult to implement. In the path which
> requires synchronization, it can be waited on one completion after
> calling request_firmware_nowait().

This sounds already like unnecessary complexity.  Also, what if
concurrent accesses?

Also, I wonder why the kernel needs to be "fixed" for this, if the
problem is really the stuck in udev.  In this regard, we didn't change
anything from the beginning.  There was an implicit "wish", that the
f/w loading shouldn't be done in the module init, but this has been
never treated as a golden rule.


Takashi