Message-ID: <CACVXFVNKGPyv6AVUctotD=Sc97VRRGCXVzJ50pprjCNB7jhUxg@mail.gmail.com>
Date:	Thu, 5 Jun 2014 22:54:33 +0800
From:	Ming Lei <ming.lei@...onical.com>
To:	Tom Gundersen <teg@...m.no>
Cc:	Takashi Iwai <tiwai@...e.de>, 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 10:32 PM, Tom Gundersen <teg@...m.no> wrote:
> On Thu, Jun 5, 2014 at 4:24 PM, Ming Lei <ming.lei@...onical.com> wrote:
>> On Thu, Jun 5, 2014 at 10:05 PM, Takashi Iwai <tiwai@...e.de> wrote:
>>> At Thu, 5 Jun 2014 21:59:52 +0800,
>>> Ming Lei wrote:
>>>>
>>>> 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.
>>>
>>> More trouble = more bug reports.  At least, a handful distros suffer.
>>> I don't know the situation with Ubuntu, though.
>>
>> Looks recently not see related report, :-)
>
> Ubuntu currently enables the firmware loader in both the kernel and in
> udev, so would not yet have a problem here at the moment. However, I
> spoke with Martin Pitt and he told me that both Debian and Ubuntu
> would like to switch this off in udev once it is possible (i.e., once
> this patch has been merged I guess). Fedora already did, and speaking
> for Arch we definitely would like to do the same. I did not check with
> other distros, but I'm pretty sure "everyone" will disable this in
> udev by the next cycle. At which point having it enabled in the kernel
> will cause real problems and bug reports.

That won't cover the case of old distributions with new kernel, do
you want to break/confuse these users?

>
> For distro kernels that's not a problem, as they know what to do, but
> I guess for random kernel users we should give them the correct
> recommendation.

Also I remember that android users put firmware under their
special path.

>
>>>> 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.
>>
>>> Some drivers fall back to different firmware (e.g. different revision
>>> suffix) if the requested file isn't found.  So, this happens in
>>> reality.
>>
>> So do you think the fallback is better or worse? For CPU microcode
>> case, maybe it is fine, but for other devices, maybe it is better to
>> get a firmware for working at least.
>
> What usecase do you have in mind here? For people using stock udev,
> enabling the fallback will not give any benefit, but it will soon

Again, we have old distributions, also it should make sense to fall back
to userspace for non-exist firmwares under default path.

> start giving real problems...

If there isn't firmwares at default path for devices, the device may
not work, and that is the real problem.

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