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

[Ming Lei]

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

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



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/