Message-ID: <CACVXFVOy95SzeL2_wHMShZwG7eE7evrZhaEvJAsd1LLDcSD1Vw@mail.gmail.com>
Date:	Thu, 23 May 2013 22:28:51 +0800
From:	Ming Lei <tom.leiming@...il.com>
To:	Takashi Iwai <tiwai@...e.de>
Cc:	Dave Jones <davej@...hat.com>, "H. Peter Anvin" <hpa@...or.com>,
	Linux Kernel <linux-kernel@...r.kernel.org>, x86@...nel.org,
	fenghua.yu@...el.com
Subject: Re: microcode loading got really slow.

On Thu, May 23, 2013 at 9:21 PM, Takashi Iwai <tiwai@...e.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@...e.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@...e.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.

>
>> >> 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, :-)

>
>> > 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.

>
> 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.


Thanks,
-- 
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists