Re: dell_rbu: Select CONFIG_FW_LOADER_USER_HELPER explicitly

[Ming Lei]

On Tue, Jul 9, 2013 at 1:33 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
> At Tue, 9 Jul 2013 11:15:20 +0800,
> Ming Lei wrote:
>>
>> On Mon, Jul 8, 2013 at 5:05 PM, Takashi Iwai <tiwai@xxxxxxx> wrote:
>> > At Sat, 6 Jul 2013 15:30:03 -0700,
>> > Greg KH wrote:
>> >>
>> >> On Sat, Jul 06, 2013 at 06:14:01PM -0400, Dave Jones wrote:
>> >> > On Tue, Jul 02, 2013 at 07:27:49PM +0000, Linux Kernel wrote:
>> >> >  > Gitweb:     http://git.kernel.org/linus/;a=commit;h=d05c39ea67f5786a549ac9d672d2951992b658c6
>> >> >  > Commit:     d05c39ea67f5786a549ac9d672d2951992b658c6
>> >> >  > Parent:     a3b2c8c7aa1ca860edcf0b0afa371d9eb2269c3c
>> >> >  > Author:     Takashi Iwai <tiwai@xxxxxxx>
>> >> >  > AuthorDate: Wed May 22 18:28:37 2013 +0200
>> >> >  > Committer:  Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> >> >  > CommitDate: Mon Jun 3 13:57:29 2013 -0700
>> >> >  >
>> >> >  >     dell_rbu: Select CONFIG_FW_LOADER_USER_HELPER explicitly
>> >> >  >
>> >> >  >     The usermode helper is mandatory for this driver.
>> >> >  >
>> >> >  >     Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
>> >> >  >     Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>> >> >  > ---
>> >> >  >  drivers/firmware/Kconfig |    1 +
>> >> >  >  1 files changed, 1 insertions(+), 0 deletions(-)
>> >> >  >
>> >> >  > diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
>> >> >  > index 9387630..0747872 100644
>> >> >  > --- a/drivers/firmware/Kconfig
>> >> >  > +++ b/drivers/firmware/Kconfig
>> >> >  > @@ -64,6 +64,7 @@ config DELL_RBU
>> >> >  >          tristate "BIOS update support for DELL systems via sysfs"
>> >> >  >          depends on X86
>> >> >  >          select FW_LOADER
>> >> >  > +        select FW_LOADER_USER_HELPER
>> >> >  >          help
>> >> >  >           Say m if you want to have the option of updating the BIOS for your
>> >> >  >           DELL system. Note you need a Dell OpenManage or Dell Update package (DUP)
>> >> >
>> >> >
>> >> > This is pretty crappy. Now every distro kernel has to have that enabled,
>> >> > which screws up for eg, the x86 microcode driver. (It takes 1 minute per cpu
>> >> > when this is enabled).
>> >>
>> >> I'll let you and Takashi battle this one out, for some reason I thought
>> >> he added it _because_ a distro kernel needed it...
>> >
>> > The reason is that dell_rbu driver requires it.  Without the kconfig
>> > option, this driver won't work at all.  So, it's a right fix for
>> > dell_rbu.
>> >
>> > AFAIK, the consensus in the kernel side is that this too long fw
>> > loading time is basically a regression of user-space (udev or
>> > whatever).  There is no change in the kernel behavior.  The problem
>> > must exist even with the older kernels.
>> >
>> > But, looking at the development, we can't expect that udev will be
>> > fixed soon, and this breakage persists already way too long.  Maybe a
>> > better solution is to kill the fallback to udev for normal f/w loading
>> > (i.e. for distro kernels).
>> >
>> > The patch below is an untested quick hack.  It adds a new Kconfig and
>> > a new function request_firmware_via_user_helper().  Distro kernels may
>> > set CONFIG_FW_LOADER_USER_HELPER_FALLBACK=n for avoiding 60 seconds
>>
>> I understand your proposed approach is basically equal to unset DELL_RBU,
>> don't I?
>>
>> Actually if DELL_RBU is set, FW_LOADER_USER_HELPER is still set, and
>> it won't avoid the fallback to loading from userspace.
>
> No, it changes the behavior.  The fallback is now checked explicitly
> via #ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK (this is the only
> place where this kconfig is referred to).  The patch isn't great and
> can be rewritten better, but the idea is to split the fallback
> behavior (for normal drivers) and the firmware loading that mandates
> the user-mode helper (for dell_rbu).

Right, sorry for missing that.

>
>
>> > stall for non-existing firmware file access -- as distributions know
>> > that the firmware files should be placed in the right path.
>> >
>> > Thoughts?
>>
>> I suggest not to introduce new config options in firmware loader any more,
>> and the current options are too many already and it is very easy to trigger
>> build problem, and introduce extra complexity to users at the same time.
>
> Yeah, I hate adding a new kconfig, too.  But, in this case, I couldn't
> have other idea for solving in a reasonable amount of change.

One approach I thought of is to introduce request_firmware_user()
which should be used only in FW_ACTION_NOHOTPLUG situation,
but allows CONFIG_FW_LOADER_USER_HELPER disabled.

Actually from view of distribution, dell rbu has to be compiled in, then
no code size can be saved any more, which means looks the introduced
option in this patch isn't necessary.

>
>> About Dave's problem, I think distribution may not trigger it since
>> cpu microcode
>> should exist,
>
> It doesn't have to exist -- imagine a brand new CPU that is shipped
> without errata (I guess it's the case Dave hit).

Yes, I mean other drivers(or most of drivers) may have not the situation
to be afraid of.

>
>> so I am wondering if Dave can disable DELL_RBU to work around
>> the problem in his system? Or fake a one byte microcode file to fool the driver?
>
> Well, the kernel cannot know whether the microcode f/w exists or not
> beforehand.  It needs to probe.  And the probing itself stalls for 60
> seconds...

The driver can use request_firmware_no_wait() to work around it too...

>
>> Also looks the problem should be handled inside x86 microcode driver because
>> the usage is unique in x86 microcode driver. Other drivers either need one
>> firmware or stop to work without a firmware, but this driver can work
>> well no matter
>> the firmware loading is satisfied or not.
>
> Not really specific to microcode driver.  Other drivers work without
> the inquired firmware file.  For example, iwlwifi has a fallback
> mechanism to fetch the old version of firmware.  If each probe of
> non-existing firmware file stalls for 60 seconds, it may take really
> long.

I am wondering why people didn't complain the loading in iwlwifi, :-)

>
>> Or could we force dell rbu to use uevent based loading now?
>
> .... only if we break dell_rbu user-space behavior.
>
> So, our options are:
>
> A. Ignore, with a hope that udev is/will be fixed.
>
> B. Break dell_rbu, rewrite dell_rbu driver code and user-space at the
>    same time
>
> C. Split the user-space fallback and the mandatory user-space loading
>    (as my patch does)



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/