lkml.org 
[lkml]   [2013]   [May]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: microcode loading got really slow.
At Thu, 23 May 2013 22:28:51 +0800,
Ming Lei wrote:
>
> On Thu, May 23, 2013 at 9:21 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > 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.
>
> If you plan to do it, it'd better to add default firmware path of some
> distributions into firmware_class.c first, otherwise it may cause
> unnecessary noise for this distribution.
>
> But if more default search paths are added, it might cause mistaken
> firmwares found under incorrect path, for example, android's
> default path is "/etc/firmware" and "/vendor/firmware"(maybe different
> for different versions).
>
> Also, putting default search paths into kernel isn't good, which was
> introduced unwillingly for well-known reason.

Maybe we can create a new Kconfig to specify non-standard firmware
path?


> >> >> 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 :)
>
> It is different, the current issue is caused by udev, not by kernel, :-)

Yeah :)


> >> > 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?
>
> The request_firmware_no_wait() supports concurrent accesses on
> either same firmware or not.

Yes, but I meant about the synchronization part. Then you'll need
multiple waiters.

> > 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.
>
> No, there isn't the golden rule, and it is reasonable or inevitable
> sometimes to load firmware in module init, for example, I remember some
> wireless dongles in which people can't read its mac address without
> downloading firmware, that means some devices may not be initialized
> successfully without firmware.

Right.


Takashi


\
 
 \ /
  Last update: 2013-05-23 17:21    [W:0.056 / U:0.612 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site