LinuxLists.cc - [PATCH v2] firmware loader: allow disabling of udev as firmware loader

[permalink] [raw]

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

On Wed, Jun 4, 2014 at 11:48 PM, Takashi Iwai <[email protected]> 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 <[email protected]>
> Cc: Ming Lei <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>
> Cc: Abhay Salunke <[email protected]>
> Cc: Stefan Roese <[email protected]>
> Cc: Arnd Bergmann <[email protected]>
> Cc: Kay Sievers <[email protected]>
> Signed-off-by: Takashi Iwai <[email protected]>
> ---
> 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.

>
> config DEBUG_DRIVER
> bool "Driver Core verbose debug messages"
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index d276e33880be..46ea5f4c3bb5 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -100,9 +100,14 @@ static inline long firmware_loading_timeout(void)
> #define FW_OPT_UEVENT (1U << 0)
> #define FW_OPT_NOWAIT (1U << 1)
> #ifdef CONFIG_FW_LOADER_USER_HELPER
> -#define FW_OPT_FALLBACK (1U << 2)
> +#define FW_OPT_USERHELPER (1U << 2)
> #else
> -#define FW_OPT_FALLBACK 0
> +#define FW_OPT_USERHELPER 0
> +#endif
> +#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
> +#define FW_OPT_FALLBACK FW_OPT_USERHELPER
> +#else
> +#define FW_OPT_FALLBACK 0
> #endif
>
> struct firmware_cache {
> @@ -1111,7 +1116,7 @@ _request_firmware(const struct firmware **firmware_p, const char *name,
>
> ret = fw_get_filesystem_firmware(device, fw->priv);
> if (ret) {
> - if (opt_flags & FW_OPT_FALLBACK) {
> + if (opt_flags & FW_OPT_USERHELPER) {
> dev_warn(device,
> "Direct firmware load failed with error %d\n",
> ret);
> @@ -1171,7 +1176,7 @@ request_firmware(const struct firmware **firmware_p, const char *name,
> }
> EXPORT_SYMBOL(request_firmware);
>
> -#ifdef CONFIG_FW_LOADER_USER_HELPER
> +#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
> /**
> * request_firmware: - load firmware directly without usermode helper
> * @firmware_p: pointer to firmware image
> @@ -1277,7 +1282,7 @@ request_firmware_nowait(
> fw_work->context = context;
> fw_work->cont = cont;
> fw_work->opt_flags = FW_OPT_NOWAIT | FW_OPT_FALLBACK |
> - (uevent ? FW_OPT_UEVENT : 0);
> + (uevent ? FW_OPT_UEVENT : FW_OPT_USERHELPER);
>
> if (!try_module_get(module)) {
> kfree(fw_work);
> diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> index 59529330efd6..67e5b801af0c 100644
> --- a/include/linux/firmware.h
> +++ b/include/linux/firmware.h
> @@ -68,7 +68,7 @@ static inline void release_firmware(const struct firmware *fw)
>
> #endif
>
> -#ifdef CONFIG_FW_LOADER_USER_HELPER
> +#ifdef CONFIG_FW_LOADER_USER_HELPER_FALLBACK
> int request_firmware_direct(const struct firmware **fw, const char *name,
> struct device *device);
> #else
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2014-06-05 13:31:59

[permalink] [raw]

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

On Thu, Jun 5, 2014 at 8:25 PM, Tom Gundersen <[email protected]> wrote:
>
> On 5 Jun 2014 14:18, "Ming Lei" <[email protected]> wrote:
>>
>> On Wed, Jun 4, 2014 at 11:48 PM, Takashi Iwai <[email protected]> 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 <[email protected]>
>> > Cc: Ming Lei <[email protected]>
>> > Cc: Greg Kroah-Hartman <[email protected]>
>> > Cc: Abhay Salunke <[email protected]>
>> > Cc: Stefan Roese <[email protected]>
>> > Cc: Arnd Bergmann <[email protected]>
>> > Cc: Kay Sievers <[email protected]>
>> > Signed-off-by: Takashi Iwai <[email protected]>
>> > ---
>> > 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.

Thanks,
--
Ming Lei

2014-06-05 13:48:00

[permalink] [raw]

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

At Thu, 5 Jun 2014 21:31:56 +0800,
Ming Lei wrote:
>
> On Thu, Jun 5, 2014 at 8:25 PM, Tom Gundersen <[email protected]> wrote:
> >
> > On 5 Jun 2014 14:18, "Ming Lei" <[email protected]> wrote:
> >>
> >> On Wed, Jun 4, 2014 at 11:48 PM, Takashi Iwai <[email protected]> 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 <[email protected]>
> >> > Cc: Ming Lei <[email protected]>
> >> > Cc: Greg Kroah-Hartman <[email protected]>
> >> > Cc: Abhay Salunke <[email protected]>
> >> > Cc: Stefan Roese <[email protected]>
> >> > Cc: Arnd Bergmann <[email protected]>
> >> > Cc: Kay Sievers <[email protected]>
> >> > Signed-off-by: Takashi Iwai <[email protected]>
> >> > ---
> >> > 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

2014-06-05 13:59:56

[permalink] [raw]

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

On Thu, Jun 5, 2014 at 9:47 PM, Takashi Iwai <[email protected]> wrote:
> At Thu, 5 Jun 2014 21:31:56 +0800,
> Ming Lei wrote:
>>
>> On Thu, Jun 5, 2014 at 8:25 PM, Tom Gundersen <[email protected]> wrote:
>> >
>> > On 5 Jun 2014 14:18, "Ming Lei" <[email protected]> wrote:
>> >>
>> >> On Wed, Jun 4, 2014 at 11:48 PM, Takashi Iwai <[email protected]> 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 <[email protected]>
>> >> > Cc: Ming Lei <[email protected]>
>> >> > Cc: Greg Kroah-Hartman <[email protected]>
>> >> > Cc: Abhay Salunke <[email protected]>
>> >> > Cc: Stefan Roese <[email protected]>
>> >> > Cc: Arnd Bergmann <[email protected]>
>> >> > Cc: Kay Sievers <[email protected]>
>> >> > Signed-off-by: Takashi Iwai <[email protected]>
>> >> > ---
>> >> > 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

2014-06-05 14:00:25

[permalink] [raw]

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

On Thu, Jun 5, 2014 at 3:31 PM, Ming Lei <[email protected]> wrote:
> On Thu, Jun 5, 2014 at 8:25 PM, Tom Gundersen <[email protected]> wrote:
>>
>> On 5 Jun 2014 14:18, "Ming Lei" <[email protected]> wrote:
>>>
>>> On Wed, Jun 4, 2014 at 11:48 PM, Takashi Iwai <[email protected]> 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 <[email protected]>
>>> > Cc: Ming Lei <[email protected]>
>>> > Cc: Greg Kroah-Hartman <[email protected]>
>>> > Cc: Abhay Salunke <[email protected]>
>>> > Cc: Stefan Roese <[email protected]>
>>> > Cc: Arnd Bergmann <[email protected]>
>>> > Cc: Kay Sievers <[email protected]>
>>> > Signed-off-by: Takashi Iwai <[email protected]>
>>> > ---
>>> > 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.

The kernel looks in the same paths as udev does, so unless you are
doing something very special (which should mean you know what you are
doing), saying N will not hurt.

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

Things will still work, but I'm sure we would get a lot of
confused/angry users. After all, the 60 second hangs were precisely
the reason the firmware loader was moved to the kernel in the first
place.

> BTW, are you sure most distros will do that in the near future?

There is no reason not to, and upstream udev will certainly remove the
feature very soon (so anyone who wants it would have to patch it back
in).

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

I don't follow. Saying N will always be safe unless you have a very
custom setup, saying Y will most likely cause lots of confusing hangs
unless you really have made sure that your userland is set up
accordingly...

Cheers,

Tom

2014-06-05 14:06:00

[permalink] [raw]

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

At Thu, 5 Jun 2014 21:59:52 +0800,
Ming Lei wrote:
>
> On Thu, Jun 5, 2014 at 9:47 PM, Takashi Iwai <[email protected]> wrote:
> > At Thu, 5 Jun 2014 21:31:56 +0800,
> > Ming Lei wrote:
> >>
> >> On Thu, Jun 5, 2014 at 8:25 PM, Tom Gundersen <[email protected]> wrote:
> >> >
> >> > On 5 Jun 2014 14:18, "Ming Lei" <[email protected]> wrote:
> >> >>
> >> >> On Wed, Jun 4, 2014 at 11:48 PM, Takashi Iwai <[email protected]> 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 <[email protected]>
> >> >> > Cc: Ming Lei <[email protected]>
> >> >> > Cc: Greg Kroah-Hartman <[email protected]>
> >> >> > Cc: Abhay Salunke <[email protected]>
> >> >> > Cc: Stefan Roese <[email protected]>
> >> >> > Cc: Arnd Bergmann <[email protected]>
> >> >> > Cc: Kay Sievers <[email protected]>
> >> >> > Signed-off-by: Takashi Iwai <[email protected]>
> >> >> > ---
> >> >> > 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.

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

thanks,

Takashi

2014-06-05 14:07:08

[permalink] [raw]

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

On Thu, Jun 5, 2014 at 3:59 PM, Ming Lei <[email protected]> wrote:
> On Thu, Jun 5, 2014 at 9:47 PM, Takashi Iwai <[email protected]> wrote:
>> At Thu, 5 Jun 2014 21:31:56 +0800,
>> Ming Lei wrote:
>>>
>>> On Thu, Jun 5, 2014 at 8:25 PM, Tom Gundersen <[email protected]> wrote:
>>> >
>>> > On 5 Jun 2014 14:18, "Ming Lei" <[email protected]> wrote:
>>> >>
>>> >> On Wed, Jun 4, 2014 at 11:48 PM, Takashi Iwai <[email protected]> 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 <[email protected]>
>>> >> > Cc: Ming Lei <[email protected]>
>>> >> > Cc: Greg Kroah-Hartman <[email protected]>
>>> >> > Cc: Abhay Salunke <[email protected]>
>>> >> > Cc: Stefan Roese <[email protected]>
>>> >> > Cc: Arnd Bergmann <[email protected]>
>>> >> > Cc: Kay Sievers <[email protected]>
>>> >> > Signed-off-by: Takashi Iwai <[email protected]>
>>> >> > ---
>>> >> > 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.

The difference will be that userspace will stop doing the firmware
loading (as everyone agrees that it should be the kernel's job).

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

Notice that getting firmware requests for nonexistent firmware is
actually quite common, so we really do want to get a negative answer
asap, and not wait for some timeout.

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

2014-06-05 14:24:55

[permalink] [raw]

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

On Thu, Jun 5, 2014 at 10:05 PM, Takashi Iwai <[email protected]> wrote:
> At Thu, 5 Jun 2014 21:59:52 +0800,
> Ming Lei wrote:
>>
>> On Thu, Jun 5, 2014 at 9:47 PM, Takashi Iwai <[email protected]> wrote:
>> > At Thu, 5 Jun 2014 21:31:56 +0800,
>> > Ming Lei wrote:
>> >>
>> >> On Thu, Jun 5, 2014 at 8:25 PM, Tom Gundersen <[email protected]> wrote:
>> >> >
>> >> > On 5 Jun 2014 14:18, "Ming Lei" <[email protected]> wrote:
>> >> >>
>> >> >> On Wed, Jun 4, 2014 at 11:48 PM, Takashi Iwai <[email protected]> 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 <[email protected]>
>> >> >> > Cc: Ming Lei <[email protected]>
>> >> >> > Cc: Greg Kroah-Hartman <[email protected]>
>> >> >> > Cc: Abhay Salunke <[email protected]>
>> >> >> > Cc: Stefan Roese <[email protected]>
>> >> >> > Cc: Arnd Bergmann <[email protected]>
>> >> >> > Cc: Kay Sievers <[email protected]>
>> >> >> > Signed-off-by: Takashi Iwai <[email protected]>
>> >> >> > ---
>> >> >> > 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

2014-06-05 14:32:47

[permalink] [raw]

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

On Thu, Jun 5, 2014 at 4:24 PM, Ming Lei <[email protected]> wrote:
> On Thu, Jun 5, 2014 at 10:05 PM, Takashi Iwai <[email protected]> wrote:
>> At Thu, 5 Jun 2014 21:59:52 +0800,
>> Ming Lei wrote:
>>>
>>> On Thu, Jun 5, 2014 at 9:47 PM, Takashi Iwai <[email protected]> wrote:
>>> > At Thu, 5 Jun 2014 21:31:56 +0800,
>>> > Ming Lei wrote:
>>> >>
>>> >> On Thu, Jun 5, 2014 at 8:25 PM, Tom Gundersen <[email protected]> wrote:
>>> >> >
>>> >> > On 5 Jun 2014 14:18, "Ming Lei" <[email protected]> wrote:
>>> >> >>
>>> >> >> On Wed, Jun 4, 2014 at 11:48 PM, Takashi Iwai <[email protected]> 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 <[email protected]>
>>> >> >> > Cc: Ming Lei <[email protected]>
>>> >> >> > Cc: Greg Kroah-Hartman <[email protected]>
>>> >> >> > Cc: Abhay Salunke <[email protected]>
>>> >> >> > Cc: Stefan Roese <[email protected]>
>>> >> >> > Cc: Arnd Bergmann <[email protected]>
>>> >> >> > Cc: Kay Sievers <[email protected]>
>>> >> >> > Signed-off-by: Takashi Iwai <[email protected]>
>>> >> >> > ---
>>> >> >> > 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

2014-06-05 14:54:37

[permalink] [raw]

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

On Thu, Jun 5, 2014 at 10:32 PM, Tom Gundersen <[email protected]> wrote:
> On Thu, Jun 5, 2014 at 4:24 PM, Ming Lei <[email protected]> wrote:
>> On Thu, Jun 5, 2014 at 10:05 PM, Takashi Iwai <[email protected]> wrote:
>>> At Thu, 5 Jun 2014 21:59:52 +0800,
>>> Ming Lei wrote:
>>>>
>>>> On Thu, Jun 5, 2014 at 9:47 PM, Takashi Iwai <[email protected]> wrote:
>>>> > At Thu, 5 Jun 2014 21:31:56 +0800,
>>>> > Ming Lei wrote:
>>>> >>
>>>> >> On Thu, Jun 5, 2014 at 8:25 PM, Tom Gundersen <[email protected]> wrote:
>>>> >> >
>>>> >> > On 5 Jun 2014 14:18, "Ming Lei" <[email protected]> wrote:
>>>> >> >>
>>>> >> >> On Wed, Jun 4, 2014 at 11:48 PM, Takashi Iwai <[email protected]> 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 <[email protected]>
>>>> >> >> > Cc: Ming Lei <[email protected]>
>>>> >> >> > Cc: Greg Kroah-Hartman <[email protected]>
>>>> >> >> > Cc: Abhay Salunke <[email protected]>
>>>> >> >> > Cc: Stefan Roese <[email protected]>
>>>> >> >> > Cc: Arnd Bergmann <[email protected]>
>>>> >> >> > Cc: Kay Sievers <[email protected]>
>>>> >> >> > Signed-off-by: Takashi Iwai <[email protected]>
>>>> >> >> > ---
>>>> >> >> > 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

2014-06-05 15:12:37

[permalink] [raw]

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

At Thu, 5 Jun 2014 22:54:33 +0800,
Ming Lei wrote:
>
> On Thu, Jun 5, 2014 at 10:32 PM, Tom Gundersen <[email protected]> wrote:
> > On Thu, Jun 5, 2014 at 4:24 PM, Ming Lei <[email protected]> wrote:
> >> On Thu, Jun 5, 2014 at 10:05 PM, Takashi Iwai <[email protected]> wrote:
> >>> At Thu, 5 Jun 2014 21:59:52 +0800,
> >>> Ming Lei wrote:
> >>>>
> >>>> On Thu, Jun 5, 2014 at 9:47 PM, Takashi Iwai <[email protected]> wrote:
> >>>> > At Thu, 5 Jun 2014 21:31:56 +0800,
> >>>> > Ming Lei wrote:
> >>>> >>
> >>>> >> On Thu, Jun 5, 2014 at 8:25 PM, Tom Gundersen <[email protected]> wrote:
> >>>> >> >
> >>>> >> > On 5 Jun 2014 14:18, "Ming Lei" <[email protected]> wrote:
> >>>> >> >>
> >>>> >> >> On Wed, Jun 4, 2014 at 11:48 PM, Takashi Iwai <[email protected]> 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 <[email protected]>
> >>>> >> >> > Cc: Ming Lei <[email protected]>
> >>>> >> >> > Cc: Greg Kroah-Hartman <[email protected]>
> >>>> >> >> > Cc: Abhay Salunke <[email protected]>
> >>>> >> >> > Cc: Stefan Roese <[email protected]>
> >>>> >> >> > Cc: Arnd Bergmann <[email protected]>
> >>>> >> >> > Cc: Kay Sievers <[email protected]>
> >>>> >> >> > Signed-off-by: Takashi Iwai <[email protected]>
> >>>> >> >> > ---
> >>>> >> >> > 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?

Why it breaks? They can just select it.

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

And, they are sure what Kconfig options to take.

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

Ming, we're discussing about the help text for people who aren't sure
what to select. Which chance would you bet as such a target? A
newbie, or a bearded techy sticking with old distros?

For people who are using the old distros *and* with non-standard
firmware paths, they must be sure what they're doing and what they
must select.

thanks,

Takashi

2014-06-05 15:15:28

[permalink] [raw]

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

On Thu, Jun 5, 2014 at 4:54 PM, Ming Lei <[email protected]> wrote:
>> 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,

Again: disabling this option will not give problems (unless you have a
custom setup), even on old userspace, only keeping it enabled on
future userspace may.

> do
> you want to break/confuse these users?

Not really, no :)

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

That may be, but I don't think this text is primarily aimed at the
people who put together Android systems... Surely their non-standard
userspace means that a lot of the advice (and it is nothing else than
advice!) in the help text does not necessarily apply?

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

Even on old distributions, falling back to stock udev will not give
any benefits. It will look in the same paths as the kernel and fail in
the same way.

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

These devices will never have worked with stock udev, as it looks in
the same path as the kernel (the paths the kernel looks in was taken
from udev to make sure they work the sam). So this must be a
special/custom setup by someone who knows what they are doing, and
hopefully knows how to answer the Kconfig.

Cheers,

Tom

2014-06-05 23:00:27

[permalink] [raw]

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

On Thu, Jun 5, 2014 at 11:15 PM, Tom Gundersen <[email protected]> wrote:
> On Thu, Jun 5, 2014 at 4:54 PM, Ming Lei <[email protected]> wrote:
>>> 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,
>
> Again: disabling this option will not give problems (unless you have a
> custom setup), even on old userspace, only keeping it enabled on
> future userspace may.
>
>> do
>> you want to break/confuse these users?
>
> Not really, no :)
>
>>> 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.
>
> That may be, but I don't think this text is primarily aimed at the
> people who put together Android systems... Surely their non-standard
> userspace means that a lot of the advice (and it is nothing else than
> advice!) in the help text does not necessarily apply?
>
>>>>>> 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.
>
> Even on old distributions, falling back to stock udev will not give
> any benefits. It will look in the same paths as the kernel and fail in
> the same way.
>
>>> 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.
>
> These devices will never have worked with stock udev, as it looks in
> the same path as the kernel (the paths the kernel looks in was taken
> from udev to make sure they work the sam). So this must be a
> special/custom setup by someone who knows what they are doing, and
> hopefully knows how to answer the Kconfig.

I have to say all your statement about old distributions is just your
take for granted, and you can't verified on all old distributions at all.
That is said, disabling fallback may cause regression on these
distributions.

Given that the user helper(fallback) has been used for tens of
years on these distributions, I suggest you to change Kconfig
help message as something like below:

If your aren't sure, please check your udev/systemd version, if it
is below than XXX or no udev/systemd shipped in your system,
say Y here, otherwise say N.

Thanks,
--
Ming Lei

2014-06-06 14:03:13

[permalink] [raw]

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

At Fri, 6 Jun 2014 07:00:22 +0800,
Ming Lei wrote:
>
> On Thu, Jun 5, 2014 at 11:15 PM, Tom Gundersen <[email protected]> wrote:
> > On Thu, Jun 5, 2014 at 4:54 PM, Ming Lei <[email protected]> wrote:
> >>> 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,
> >
> > Again: disabling this option will not give problems (unless you have a
> > custom setup), even on old userspace, only keeping it enabled on
> > future userspace may.
> >
> >> do
> >> you want to break/confuse these users?
> >
> > Not really, no :)
> >
> >>> 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.
> >
> > That may be, but I don't think this text is primarily aimed at the
> > people who put together Android systems... Surely their non-standard
> > userspace means that a lot of the advice (and it is nothing else than
> > advice!) in the help text does not necessarily apply?
> >
> >>>>>> 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.
> >
> > Even on old distributions, falling back to stock udev will not give
> > any benefits. It will look in the same paths as the kernel and fail in
> > the same way.
> >
> >>> 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.
> >
> > These devices will never have worked with stock udev, as it looks in
> > the same path as the kernel (the paths the kernel looks in was taken
> > from udev to make sure they work the sam). So this must be a
> > special/custom setup by someone who knows what they are doing, and
> > hopefully knows how to answer the Kconfig.
>
> I have to say all your statement about old distributions is just your
> take for granted, and you can't verified on all old distributions at all.
> That is said, disabling fallback may cause regression on these
> distributions.
>
> Given that the user helper(fallback) has been used for tens of
> years on these distributions, I suggest you to change Kconfig
> help message as something like below:
>
> If your aren't sure, please check your udev/systemd version, if it
> is below than XXX or no udev/systemd shipped in your system,
> say Y here, otherwise say N.

Yeah, a detailed suggestion like the above would be more helpful.

Takashi

2014-06-06 14:15:25

[permalink] [raw]

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

On Fri, Jun 6, 2014 at 4:03 PM, Takashi Iwai <[email protected]> wrote:
> At Fri, 6 Jun 2014 07:00:22 +0800,
> Ming Lei wrote:
>>
>> On Thu, Jun 5, 2014 at 11:15 PM, Tom Gundersen <[email protected]> wrote:
>> > On Thu, Jun 5, 2014 at 4:54 PM, Ming Lei <[email protected]> wrote:
>> >>> 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,
>> >
>> > Again: disabling this option will not give problems (unless you have a
>> > custom setup), even on old userspace, only keeping it enabled on
>> > future userspace may.
>> >
>> >> do
>> >> you want to break/confuse these users?
>> >
>> > Not really, no :)
>> >
>> >>> 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.
>> >
>> > That may be, but I don't think this text is primarily aimed at the
>> > people who put together Android systems... Surely their non-standard
>> > userspace means that a lot of the advice (and it is nothing else than
>> > advice!) in the help text does not necessarily apply?
>> >
>> >>>>>> 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.
>> >
>> > Even on old distributions, falling back to stock udev will not give
>> > any benefits. It will look in the same paths as the kernel and fail in
>> > the same way.
>> >
>> >>> 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.
>> >
>> > These devices will never have worked with stock udev, as it looks in
>> > the same path as the kernel (the paths the kernel looks in was taken
>> > from udev to make sure they work the sam). So this must be a
>> > special/custom setup by someone who knows what they are doing, and
>> > hopefully knows how to answer the Kconfig.
>>
>> I have to say all your statement about old distributions is just your
>> take for granted, and you can't verified on all old distributions at all.
>> That is said, disabling fallback may cause regression on these
>> distributions.
>>
>> Given that the user helper(fallback) has been used for tens of
>> years on these distributions, I suggest you to change Kconfig
>> help message as something like below:
>>
>> If your aren't sure, please check your udev/systemd version, if it
>> is below than XXX or no udev/systemd shipped in your system,
>> say Y here, otherwise say N.
>
> Yeah, a detailed suggestion like the above would be more helpful.

How about:

"If you rely on a customized udev (or other userspace tool) to load
firmware from a non-standard path, say Y. Otherwise, say N. If your
udev version does not support firmware loading (which is currently the
upstream default), you must say N."

?

Cheers,

Tom

2014-06-06 15:19:07

[permalink] [raw]

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

On Fri, Jun 6, 2014 at 10:15 PM, Tom Gundersen <[email protected]> wrote:
> On Fri, Jun 6, 2014 at 4:03 PM, Takashi Iwai <[email protected]> wrote:
>> At Fri, 6 Jun 2014 07:00:22 +0800,
>> Ming Lei wrote:
>>>
>>> On Thu, Jun 5, 2014 at 11:15 PM, Tom Gundersen <[email protected]> wrote:
>>> > On Thu, Jun 5, 2014 at 4:54 PM, Ming Lei <[email protected]> wrote:
>>> >>> 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,
>>> >
>>> > Again: disabling this option will not give problems (unless you have a
>>> > custom setup), even on old userspace, only keeping it enabled on
>>> > future userspace may.
>>> >
>>> >> do
>>> >> you want to break/confuse these users?
>>> >
>>> > Not really, no :)
>>> >
>>> >>> 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.
>>> >
>>> > That may be, but I don't think this text is primarily aimed at the
>>> > people who put together Android systems... Surely their non-standard
>>> > userspace means that a lot of the advice (and it is nothing else than
>>> > advice!) in the help text does not necessarily apply?
>>> >
>>> >>>>>> 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.
>>> >
>>> > Even on old distributions, falling back to stock udev will not give
>>> > any benefits. It will look in the same paths as the kernel and fail in
>>> > the same way.
>>> >
>>> >>> 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.
>>> >
>>> > These devices will never have worked with stock udev, as it looks in
>>> > the same path as the kernel (the paths the kernel looks in was taken
>>> > from udev to make sure they work the sam). So this must be a
>>> > special/custom setup by someone who knows what they are doing, and
>>> > hopefully knows how to answer the Kconfig.
>>>
>>> I have to say all your statement about old distributions is just your
>>> take for granted, and you can't verified on all old distributions at all.
>>> That is said, disabling fallback may cause regression on these
>>> distributions.
>>>
>>> Given that the user helper(fallback) has been used for tens of
>>> years on these distributions, I suggest you to change Kconfig
>>> help message as something like below:
>>>
>>> If your aren't sure, please check your udev/systemd version, if it
>>> is below than XXX or no udev/systemd shipped in your system,
>>> say Y here, otherwise say N.
>>
>> Yeah, a detailed suggestion like the above would be more helpful.
>
> How about:
>
> "If you rely on a customized udev (or other userspace tool) to load
> firmware from a non-standard path, say Y. Otherwise, say N. If your
> udev version does not support firmware loading (which is currently the
> upstream default), you must say N."

Please make it explicit since we know the fisrt version of
udev which disables firmware loading at default. And 'upstream
default' isn't needed because it depends on udev upstream version, and
'currently' is confused too because old distribution ships old udev.

Something like below should be enough:

If you aren't sure, please check the udev/systemd version, if it
is below than XXX or no udev/systemd shipped in your system,
say Y here, otherwise say N.

Thanks,
--
Ming Lei

2014-06-06 15:22:49

by Greg Kroah-Hartman

[permalink] [raw]

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

On Fri, Jun 06, 2014 at 04:15:03PM +0200, Tom Gundersen wrote:
> How about:
>
> "If you rely on a customized udev (or other userspace tool) to load
> firmware from a non-standard path, say Y. Otherwise, say N. If your
> udev version does not support firmware loading (which is currently the
> upstream default), you must say N."

Like anyone ever reads help entries for config options...

"customized udev"? What does that mean?

This is all bikeshedding, right?

greg k-h

2014-06-06 15:26:08

by Ilia Mirkin

[permalink] [raw]

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

On Fri, Jun 6, 2014 at 11:19 AM, Ming Lei <[email protected]> wrote:
> On Fri, Jun 6, 2014 at 10:15 PM, Tom Gundersen <[email protected]> wrote:
>> On Fri, Jun 6, 2014 at 4:03 PM, Takashi Iwai <[email protected]> wrote:
>>> At Fri, 6 Jun 2014 07:00:22 +0800,
>>> Ming Lei wrote:
>>>>
>>>> On Thu, Jun 5, 2014 at 11:15 PM, Tom Gundersen <[email protected]> wrote:
>>>> > On Thu, Jun 5, 2014 at 4:54 PM, Ming Lei <[email protected]> wrote:
>>>> >>> 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,
>>>> >
>>>> > Again: disabling this option will not give problems (unless you have a
>>>> > custom setup), even on old userspace, only keeping it enabled on
>>>> > future userspace may.
>>>> >
>>>> >> do
>>>> >> you want to break/confuse these users?
>>>> >
>>>> > Not really, no :)
>>>> >
>>>> >>> 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.
>>>> >
>>>> > That may be, but I don't think this text is primarily aimed at the
>>>> > people who put together Android systems... Surely their non-standard
>>>> > userspace means that a lot of the advice (and it is nothing else than
>>>> > advice!) in the help text does not necessarily apply?
>>>> >
>>>> >>>>>> 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.
>>>> >
>>>> > Even on old distributions, falling back to stock udev will not give
>>>> > any benefits. It will look in the same paths as the kernel and fail in
>>>> > the same way.
>>>> >
>>>> >>> 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.
>>>> >
>>>> > These devices will never have worked with stock udev, as it looks in
>>>> > the same path as the kernel (the paths the kernel looks in was taken
>>>> > from udev to make sure they work the sam). So this must be a
>>>> > special/custom setup by someone who knows what they are doing, and
>>>> > hopefully knows how to answer the Kconfig.
>>>>
>>>> I have to say all your statement about old distributions is just your
>>>> take for granted, and you can't verified on all old distributions at all.
>>>> That is said, disabling fallback may cause regression on these
>>>> distributions.
>>>>
>>>> Given that the user helper(fallback) has been used for tens of
>>>> years on these distributions, I suggest you to change Kconfig
>>>> help message as something like below:
>>>>
>>>> If your aren't sure, please check your udev/systemd version, if it
>>>> is below than XXX or no udev/systemd shipped in your system,
>>>> say Y here, otherwise say N.
>>>
>>> Yeah, a detailed suggestion like the above would be more helpful.
>>
>> How about:
>>
>> "If you rely on a customized udev (or other userspace tool) to load
>> firmware from a non-standard path, say Y. Otherwise, say N. If your
>> udev version does not support firmware loading (which is currently the
>> upstream default), you must say N."
>
> Please make it explicit since we know the fisrt version of
> udev which disables firmware loading at default. And 'upstream
> default' isn't needed because it depends on udev upstream version, and
> 'currently' is confused too because old distribution ships old udev.
>
> Something like below should be enough:
>
> If you aren't sure, please check the udev/systemd version, if it
> is below than XXX or no udev/systemd shipped in your system,
> say Y here, otherwise say N.

There are basically no reasonable conditions when it should be
enabled, irrespective of udev version. Countless man-hours have been
spent debugging issues with weird hangs caused by this. I know I've
spent at least one of my own working out why resume from suspend hung
mysteriously. Is there an actual, _real_, situation where the usermode
helper is used for something constructive (on a kernel that can load
its own firmware)? Certainly one can be imagined, but were there any
systems that made use of that imagination?

-ilia

2014-06-06 15:28:34