Re: microcode loading got really slow.

[Ming Lei]

On Thu, May 23, 2013 at 10:36 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> At Thu, 23 May 2013 22:28:51 +0800,
> Ming Lei wrote:
>>
>> On Thu, May 23, 2013 at 9:21 PM, Takashi Iwai <tiwai@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx> 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?

Yes.

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

If request_firmware_no_wait() is called on one same firmware image,
one waiter should be OK since the returned data is same for all requests.

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


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