Re: dell_rbu: Select CONFIG_FW_LOADER_USER_HELPER explicitly

[Takashi Iwai]

At Tue, 9 Jul 2013 16:43:30 +0800,
Ming Lei wrote:
> 
> 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.

Well, but it means that there is no way not to compile user-helper
mode code in firmware_class.c.  In other words, in which condition,
request_firmware_user() will be compiled?


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

Yes, but it sucks :)


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

Maybe just because people update kernel-firmware together with the
kernel itself.  Or, it happens with a certain udev like Fedora.
I haven't seen 60 seconds delay on openSUSE, for example.


Takashi


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