Re: [PATCH v2] firmware loader: allow disabling of udev as firmware loader

[Takashi Iwai <tiwai@suse.de>]

At Thu, 5 Jun 2014 21:31:56 +0800,
Ming Lei wrote:
> 
> On Thu, Jun 5, 2014 at 8:25 PM, Tom Gundersen <teg@jklm.no> wrote:
> >
> > On 5 Jun 2014 14:18, "Ming Lei" <ming.lei@canonical.com> wrote:
> >>
> >> On Wed, Jun 4, 2014 at 11:48 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> > [The patch was originally proposed by Tom Gundersen, and rewritten
> >> >  afterwards by me; most of changelogs below borrowed from Tom's
> >> >  original patch -- tiwai]
> >> >
> >> > Currently (at least) the dell-rbu driver selects FW_LOADER_USER_HELPER,
> >> > which means that distros can't really stop loading firmware through
> >> > udev without breaking other users (though some have).
> >> >
> >> > Ideally we would remove/disable the udev firmware helper in both the
> >> > kernel and in udev, but if we were to disable it in udev and not the
> >> > kernel, the result would be (seemingly) hung kernels as no one would
> >> > be around to cancel firmware requests.
> >> >
> >> > This patch allows udev firmware loading to be disabled while still
> >> > allowing non-udev firmware loading, as done by the dell-rbu driver, to
> >> > continue working. This is achieved by only using the fallback
> >> > mechanism when the uevent is suppressed.
> >> >
> >> > The patch renames the user-selectable Kconfig from FW_LOADER_USER_HELPER
> >> > to FW_LOADER_USER_HELPER_FALLBACK, and the former is reverse-selected
> >> > by the latter or the drivers that need userhelper like dell-rbu.
> >> >
> >> > Also, the "default y" is removed together with this change, since it's
> >> > been deprecated in udev upstream, thus rather better to disable it
> >> > nowadays.
> >> >
> >> > Tested with
> >> >     FW_LOADER_USER_HELPER=n
> >> >     LATTICE_ECP3_CONFIG=y
> >> >     DELL_RBU=y
> >> > and udev without the firmware loading support, but I don't have the
> >> > hardware to test the lattice/dell drivers, so additional testing would
> >> > be appreciated.
> >> >
> >> > Reviewed-by: Tom Gundersen <teg@jklm.no>
> >> > Cc: Ming Lei <ming.lei@canonical.com>
> >> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> >> > Cc: Abhay Salunke <Abhay_Salunke@dell.com>
> >> > Cc: Stefan Roese <sr@denx.de>
> >> > Cc: Arnd Bergmann <arnd@arndb.de>
> >> > Cc: Kay Sievers <kay@vrfy.org>
> >> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> >> > ---
> >> >  drivers/base/Kconfig          | 10 ++++++++--
> >> >  drivers/base/firmware_class.c | 15 ++++++++++-----
> >> >  include/linux/firmware.h      |  2 +-
> >> >  3 files changed, 19 insertions(+), 8 deletions(-)
> >> >
> >> > diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
> >> > index 8fa8deab6449..d0bb32e4c416 100644
> >> > --- a/drivers/base/Kconfig
> >> > +++ b/drivers/base/Kconfig
> >> > @@ -144,15 +144,21 @@ config EXTRA_FIRMWARE_DIR
> >> >           some other directory containing the firmware files.
> >> >
> >> >  config FW_LOADER_USER_HELPER
> >> > +       bool
> >> > +
> >> > +config FW_LOADER_USER_HELPER_FALLBACK
> >> >         bool "Fallback user-helper invocation for firmware loading"
> >> >         depends on FW_LOADER
> >> > -       default y
> >> > +       select FW_LOADER_USER_HELPER
> >> >         help
> >> >           This option enables / disables the invocation of user-helper
> >> >           (e.g. udev) for loading firmware files as a fallback after the
> >> >           direct file loading in kernel fails.  The user-mode helper is
> >> >           no longer required unless you have a special firmware file
> >> > that
> >> > -         resides in a non-standard path.
> >> > +         resides in a non-standard path. Moreover, the udev support has
> >> > +         been deprecated upstream.
> >> > +
> >> > +         If you are unsure about this, say N here.
> >>
> >> It may be safer to say Y here for fallback if not sure.
> >
> > Saying Y here will be actively harmful if the firmware loader is disabled in
> > udev (which I guess most distros will do as soon as they can), so I think we
> 
> If fallback is triggered, it means that the firmware can't be found
> in default direct rootfs path, so it is better to continue to look for it
> from userspace.
> 
> Also it won't a big problem for hanging the request user context
> for some while(60sec at default) if udev is disabled, will it?
> 
> BTW, are you sure most distros will do that in the near future?
> 
> > should advice people to say N unless they really know what they are doing
> > and that their userspace can cope with it.
> 
> That is why I suggest to say Y if someone isn't sure.

For the time being, having default this Y causes more troubles.
In such a case, we should decide from utilitarianism POV, IMO.
Anyone who isn't sure would likely have a recent system that cannot
cope with user-space f/w loader.  If anyone install to a non-standard 
path, they do something special, after all.

(And, due to Kconfig rename, user has to review the Kconfig again.
 So they must see the change.)


Takashi