Message-ID: <CACVXFVP5CSNoyThAcsmihYg_bkq2wn1JtiFJ_7ehpxwjBSaZxQ@mail.gmail.com>
Date:	Thu, 5 Jun 2014 21:59:52 +0800
From:	Ming Lei <ming.lei@...onical.com>
To:	Takashi Iwai <tiwai@...e.de>
Cc:	Tom Gundersen <teg@...m.no>, LKML <linux-kernel@...r.kernel.org>,
	Greg KH <gregkh@...uxfoundation.org>,
	Stefan Roese <sr@...x.de>, Arnd Bergmann <arnd@...db.de>,
	Abhay Salunke <Abhay_Salunke@...l.com>,
	Kay Sievers <kay@...y.org>
Subject: Re: [PATCH v2] firmware loader: allow disabling of udev as firmware loader

On Thu, Jun 5, 2014 at 9:47 PM, Takashi Iwai <tiwai@...e.de> wrote:
> At Thu, 5 Jun 2014 21:31:56 +0800,
> Ming Lei wrote:
>>
>> On Thu, Jun 5, 2014 at 8:25 PM, Tom Gundersen <teg@...m.no> wrote:
>> >
>> > On 5 Jun 2014 14:18, "Ming Lei" <ming.lei@...onical.com> wrote:
>> >>
>> >> On Wed, Jun 4, 2014 at 11:48 PM, Takashi Iwai <tiwai@...e.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@...m.no>
>> >> > Cc: Ming Lei <ming.lei@...onical.com>
>> >> > Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>> >> > Cc: Abhay Salunke <Abhay_Salunke@...l.com>
>> >> > Cc: Stefan Roese <sr@...x.de>
>> >> > Cc: Arnd Bergmann <arnd@...db.de>
>> >> > Cc: Kay Sievers <kay@...y.org>
>> >> > Signed-off-by: Takashi Iwai <tiwai@...e.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.

I am wondering why default Y may cause more troubles, as we
know, it has been default Y for long long time.

It only falls back if the request fw is _not_ found from direct loading,
so it is reasonable to try to continue to look for it from user space.

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

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