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

[Tom Gundersen]

On Thu, Jun 5, 2014 at 4:24 PM, Ming Lei <ming.lei@xxxxxxxxxxxxx> wrote:
> On Thu, Jun 5, 2014 at 10:05 PM, Takashi Iwai <tiwai@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx> wrote:
>>> >> >
>>> >> > On 5 Jun 2014 14:18, "Ming Lei" <ming.lei@xxxxxxxxxxxxx> wrote:
>>> >> >>
>>> >> >> On Wed, Jun 4, 2014 at 11:48 PM, Takashi Iwai <tiwai@xxxxxxx> 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@xxxxxxx>
>>> >> >> > Cc: Ming Lei <ming.lei@xxxxxxxxxxxxx>
>>> >> >> > Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
>>> >> >> > Cc: Abhay Salunke <Abhay_Salunke@xxxxxxxx>
>>> >> >> > Cc: Stefan Roese <sr@xxxxxxx>
>>> >> >> > Cc: Arnd Bergmann <arnd@xxxxxxxx>
>>> >> >> > Cc: Kay Sievers <kay@xxxxxxxx>
>>> >> >> > Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
>>> >> >> > ---
>>> >> >> >  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.

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.

>>> 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
start giving real problems...

Cheers,

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