Slow firmware timeouts again (Re: [3.11 regression?] iwlwifi firmware takes two minutes to load)

Slow firmware timeouts again (Re: [3.11 regression?] iwlwifi firmware takes two minutes to load)

[Andy Lutomirski]

[cc: linux-kernel, linux-hotplug, and systemd-devel.  This is 3.11-rc3+]

On Fri, Aug 2, 2013 at 12:38 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2013-08-01 at 21:38 -0700, Andy Lutomirski wrote:
>> At boot, I get:
>> [   12.537108] iwlwifi 0000:03:00.0: irq 51 for MSI/MSI-X
>> ...
>> [  132.676781] iwlwifi 0000:03:00.0: loaded firmware version 9.221.4.1
>> build 25532 op_mode iwldvm
>>
>> This sounds familiar, but wasn't it fixed awhile ago?
>
> It wasn't exactly fixed and it's really more of a userspace problem - we
> probably request firmware version 8, and then it takes 30 seconds to
> time out for each of 8,7,6,5, after which the next request for 4 is
> successful.

Why's it requesting those firmwares?  They don't seem to exist on
intellinuxwireless.org.

I have:

CONFIG_FW_LOADER=y
# CONFIG_FIRMWARE_IN_KERNEL is not set
CONFIG_EXTRA_FIRMWARE=""
CONFIG_FW_LOADER_USER_HELPER=y


>
> I don't know why your userspace isn't behaving differently though.
>
> johannes
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Slow firmware timeouts again (Re: [3.11 regression?] iwlwifi firmware takes two minutes to load)

[Andy Lutomirski]

[cc: linux-kernel, linux-hotplug, and systemd-devel.  This is 3.11-rc3+]

On Fri, Aug 2, 2013 at 12:38 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Thu, 2013-08-01 at 21:38 -0700, Andy Lutomirski wrote:
>> At boot, I get:
>> [   12.537108] iwlwifi 0000:03:00.0: irq 51 for MSI/MSI-X
>> ...
>> [  132.676781] iwlwifi 0000:03:00.0: loaded firmware version 9.221.4.1
>> build 25532 op_mode iwldvm
>>
>> This sounds familiar, but wasn't it fixed awhile ago?
>
> It wasn't exactly fixed and it's really more of a userspace problem - we
> probably request firmware version 8, and then it takes 30 seconds to
> time out for each of 8,7,6,5, after which the next request for 4 is
> successful.

Why's it requesting those firmwares?  They don't seem to exist on
intellinuxwireless.org.

I have:

CONFIG_FW_LOADER=y
# CONFIG_FIRMWARE_IN_KERNEL is not set
CONFIG_EXTRA_FIRMWARE=""
CONFIG_FW_LOADER_USER_HELPER=y


>
> I don't know why your userspace isn't behaving differently though.
>
> johannes
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: Slow firmware timeouts again (Re: [3.11 regression?] iwlwifi firmware takes two minutes to load)

[Johannes Berg]

On Fri, 2013-08-02 at 09:04 -0700, Andy Lutomirski wrote:

> > It wasn't exactly fixed and it's really more of a userspace problem - we
> > probably request firmware version 8, and then it takes 30 seconds to
> > time out for each of 8,7,6,5, after which the next request for 4 is
> > successful.
> 
> Why's it requesting those firmwares?  They don't seem to exist on
> intellinuxwireless.org.

Well for one you've never even mentioned what device you have, and then
also it's not requesting 8/7 only 6,5,4 -- I guess the timeout was
increased to 60 seconds (or I'm remembering wrong and it always was? I
thought it was 30s)

johannes

Re: Slow firmware timeouts again (Re: [3.11 regression?] iwlwifi firmware takes two minutes to load)

[Johannes Berg]

On Fri, 2013-08-02 at 09:04 -0700, Andy Lutomirski wrote:

> > It wasn't exactly fixed and it's really more of a userspace problem - we
> > probably request firmware version 8, and then it takes 30 seconds to
> > time out for each of 8,7,6,5, after which the next request for 4 is
> > successful.
> 
> Why's it requesting those firmwares?  They don't seem to exist on
> intellinuxwireless.org.

Well for one you've never even mentioned what device you have, and then
also it's not requesting 8/7 only 6,5,4 -- I guess the timeout was
increased to 60 seconds (or I'm remembering wrong and it always was? I
thought it was 30s)

johannes

Re: Slow firmware timeouts again (Re: [3.11 regression?] iwlwifi firmware takes two minutes to load)

[Andy Lutomirski]

On Fri, Aug 2, 2013 at 9:21 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2013-08-02 at 09:04 -0700, Andy Lutomirski wrote:
>
>> > It wasn't exactly fixed and it's really more of a userspace problem - we
>> > probably request firmware version 8, and then it takes 30 seconds to
>> > time out for each of 8,7,6,5, after which the next request for 4 is
>> > successful.
>>
>> Why's it requesting those firmwares?  They don't seem to exist on
>> intellinuxwireless.org.
>
> Well for one you've never even mentioned what device you have, and then
> also it's not requesting 8/7 only 6,5,4 -- I guess the timeout was
> increased to 60 seconds (or I'm remembering wrong and it always was? I
> thought it was 30s)

I have an "Ultimate-N 6300 (rev 35)".  It's requesting at least
versions 6 and 5 (I saw them in udevadm monitor).  It looks like the
g2a and g2b variants have -5 and -6 versions, but 6000-4 appears to be
the only relevant version for my hardware.

Re: Slow firmware timeouts again (Re: [3.11 regression?] iwlwifi firmware takes two minutes to load)

[Andy Lutomirski]

On Fri, Aug 2, 2013 at 9:21 AM, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2013-08-02 at 09:04 -0700, Andy Lutomirski wrote:
>
>> > It wasn't exactly fixed and it's really more of a userspace problem - we
>> > probably request firmware version 8, and then it takes 30 seconds to
>> > time out for each of 8,7,6,5, after which the next request for 4 is
>> > successful.
>>
>> Why's it requesting those firmwares?  They don't seem to exist on
>> intellinuxwireless.org.
>
> Well for one you've never even mentioned what device you have, and then
> also it's not requesting 8/7 only 6,5,4 -- I guess the timeout was
> increased to 60 seconds (or I'm remembering wrong and it always was? I
> thought it was 30s)

I have an "Ultimate-N 6300 (rev 35)".  It's requesting at least
versions 6 and 5 (I saw them in udevadm monitor).  It looks like the
g2a and g2b variants have -5 and -6 versions, but 6000-4 appears to be
the only relevant version for my hardware.

Re: Slow firmware timeouts again (Re: [3.11 regression?] iwlwifi firmware takes two minutes to load)

[Zbigniew Jędrzejewski-Szmek]

On Fri, Aug 02, 2013 at 09:04:44AM -0700, Andy Lutomirski wrote:
> CONFIG_FW_LOADER_USER_HELPER=y
Do you need this? Unsetting this should 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."""

Zbyszek
-- 
they are not broken. they are refucktored
                           -- alxchk

Re: Slow firmware timeouts again (Re: [3.11 regression?] iwlwifi firmware takes two minutes to load)

[Zbigniew Jędrzejewski-Szmek]

On Fri, Aug 02, 2013 at 09:04:44AM -0700, Andy Lutomirski wrote:
> CONFIG_FW_LOADER_USER_HELPER=y
Do you need this? Unsetting this should 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."""

Zbyszek
-- 
they are not broken. they are refucktored
                           -- alxchk

Re: [systemd-devel] Slow firmware timeouts again (Re: [3.11 regression?] iwlwifi firmware takes two

[Kay Sievers]

On Fri, Aug 2, 2013 at 6:28 PM, Zbigniew Jędrzejewski-Szmek
<zbyszek@in.waw.pl> wrote:
> On Fri, Aug 02, 2013 at 09:04:44AM -0700, Andy Lutomirski wrote:
>> CONFIG_FW_LOADER_USER_HELPER=y
> Do you need this? Unsetting this should 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."""

On recent systems, if the kernel configures
CONFIG_FW_LOADER_USER_HELPER=y and a firmware is not found by the
kernel, the kernel will issue a request which is ignored by userspace
and will block in that for 60 seconds.

Udev is no longer in the game of firmware loading, not even as a
fallback, it will just completely ignore all kernel firmware class
events.

The source code in udev to handle firmware requests is disabled by
default, currently still kept around for old kernels without the
in-kernel firmware loader, but it will be deleted in the near future.

Any issues with firmware timeouts should be addressed in the kernel by
disabling CONFIG_FW_LOADER_USER_HELPER or by removing the fallback
code from the in-kernel loader.

Kay

Re: [systemd-devel] Slow firmware timeouts again (Re: [3.11 regression?] iwlwifi firmware takes two minutes to load)

[Kay Sievers]

On Fri, Aug 2, 2013 at 6:28 PM, Zbigniew Jędrzejewski-Szmek
<zbyszek@in.waw.pl> wrote:
> On Fri, Aug 02, 2013 at 09:04:44AM -0700, Andy Lutomirski wrote:
>> CONFIG_FW_LOADER_USER_HELPER=y
> Do you need this? Unsetting this should 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."""

On recent systems, if the kernel configures
CONFIG_FW_LOADER_USER_HELPER=y and a firmware is not found by the
kernel, the kernel will issue a request which is ignored by userspace
and will block in that for 60 seconds.

Udev is no longer in the game of firmware loading, not even as a
fallback, it will just completely ignore all kernel firmware class
events.

The source code in udev to handle firmware requests is disabled by
default, currently still kept around for old kernels without the
in-kernel firmware loader, but it will be deleted in the near future.

Any issues with firmware timeouts should be addressed in the kernel by
disabling CONFIG_FW_LOADER_USER_HELPER or by removing the fallback
code from the in-kernel loader.

Kay

Re: [systemd-devel] Slow firmware timeouts again (Re: [3.11 regression?] iwlwifi firmware takes two

[Andy Lutomirski]

On Mon, Aug 5, 2013 at 4:18 AM, Kay Sievers <kay@vrfy.org> wrote:
> On Fri, Aug 2, 2013 at 6:28 PM, Zbigniew Jêdrzejewski-Szmek
> <zbyszek@in.waw.pl> wrote:
>> On Fri, Aug 02, 2013 at 09:04:44AM -0700, Andy Lutomirski wrote:
>>> CONFIG_FW_LOADER_USER_HELPER=y
>> Do you need this? Unsetting this should 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."""
>
> On recent systems, if the kernel configures
> CONFIG_FW_LOADER_USER_HELPER=y and a firmware is not found by the
> kernel, the kernel will issue a request which is ignored by userspace
> and will block in that for 60 seconds.
>
> Udev is no longer in the game of firmware loading, not even as a
> fallback, it will just completely ignore all kernel firmware class
> events.
>
> The source code in udev to handle firmware requests is disabled by
> default, currently still kept around for old kernels without the
> in-kernel firmware loader, but it will be deleted in the near future.

Any chance you'd consider a less regression-inducing path to getting
rid of this feature?  For example, have udev warn and immediate fail
firmware loading requests for a few releases, then just warn, then
drop support?

Meanwhile, CONFIG_FW_LOADER_USER_HELPER is still default y (!), so
udev has introduced massive bootup delays into the default
configuration with no warning.  It might be nice to change it to
default n, get rid of everything that selects it, and possible even
rename it to something with LEGACY or OBSOLETE in the name so that
make oldconfig will prompt.

--Andy

>
> Any issues with firmware timeouts should be addressed in the kernel by
> disabling CONFIG_FW_LOADER_USER_HELPER or by removing the fallback
> code from the in-kernel loader.
>
> Kay



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [systemd-devel] Slow firmware timeouts again (Re: [3.11 regression?] iwlwifi firmware takes two minutes to load)

[Andy Lutomirski]

On Mon, Aug 5, 2013 at 4:18 AM, Kay Sievers <kay@vrfy.org> wrote:
> On Fri, Aug 2, 2013 at 6:28 PM, Zbigniew Jędrzejewski-Szmek
> <zbyszek@in.waw.pl> wrote:
>> On Fri, Aug 02, 2013 at 09:04:44AM -0700, Andy Lutomirski wrote:
>>> CONFIG_FW_LOADER_USER_HELPER=y
>> Do you need this? Unsetting this should 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."""
>
> On recent systems, if the kernel configures
> CONFIG_FW_LOADER_USER_HELPER=y and a firmware is not found by the
> kernel, the kernel will issue a request which is ignored by userspace
> and will block in that for 60 seconds.
>
> Udev is no longer in the game of firmware loading, not even as a
> fallback, it will just completely ignore all kernel firmware class
> events.
>
> The source code in udev to handle firmware requests is disabled by
> default, currently still kept around for old kernels without the
> in-kernel firmware loader, but it will be deleted in the near future.

Any chance you'd consider a less regression-inducing path to getting
rid of this feature?  For example, have udev warn and immediate fail
firmware loading requests for a few releases, then just warn, then
drop support?

Meanwhile, CONFIG_FW_LOADER_USER_HELPER is still default y (!), so
udev has introduced massive bootup delays into the default
configuration with no warning.  It might be nice to change it to
default n, get rid of everything that selects it, and possible even
rename it to something with LEGACY or OBSOLETE in the name so that
make oldconfig will prompt.

--Andy

>
> Any issues with firmware timeouts should be addressed in the kernel by
> disabling CONFIG_FW_LOADER_USER_HELPER or by removing the fallback
> code from the in-kernel loader.
>
> Kay



-- 
Andy Lutomirski
AMA Capital Management, LLC

[PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it

[Andy Lutomirski]

The systemd commit below can delay firmware loading by multiple
minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
noticed that the systemd-udev change would break new kernels as well
as old kernels.

Since the kernel apparently can't count on reasonable userspace
support, turn this thing off by default.

commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
Author: Tom Gundersen <teg@jklm.no>
Date:   Mon Mar 18 15:12:18 2013 +0100

    udev: make firmware loading optional and disable by default

    Distros that whish to support old kernels should set
      --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
    to retain the old behaviour.
---
 drivers/base/Kconfig     | 15 +++++++++++----
 drivers/firmware/Kconfig |  1 -
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 5daa259..de3903e 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -146,13 +146,20 @@ config EXTRA_FIRMWARE_DIR
 config FW_LOADER_USER_HELPER
 	bool "Fallback user-helper invocation for firmware loading"
 	depends on FW_LOADER
-	default y
+	default n
 	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.
+	  direct file loading in kernel fails.
+
+	  Since March 2013, a default udev build does not understand
+	  firmware loading requests.  These udev versions will not
+	  even indicate failure; instead they cause long timeouts.
+	  This can dramatically slow down the boot process.
+
+	  Say Y only if you have special firmware-loading requirements
+	  and if you have a non-standard helper that will handle these
+	  requests.
 
 config DEBUG_DRIVER
 	bool "Driver Core verbose debug messages"
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 07478728..9387630 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -64,7 +64,6 @@ config DELL_RBU
 	tristate "BIOS update support for DELL systems via sysfs"
 	depends on X86
 	select FW_LOADER
-	select FW_LOADER_USER_HELPER
 	help
 	 Say m if you want to have the option of updating the BIOS for your
 	 DELL system. Note you need a Dell OpenManage or Dell Update package (DUP)
-- 
1.8.3.1

[PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it

[Andy Lutomirski]

The systemd commit below can delay firmware loading by multiple
minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
noticed that the systemd-udev change would break new kernels as well
as old kernels.

Since the kernel apparently can't count on reasonable userspace
support, turn this thing off by default.

commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
Author: Tom Gundersen <teg@jklm.no>
Date:   Mon Mar 18 15:12:18 2013 +0100

    udev: make firmware loading optional and disable by default

    Distros that whish to support old kernels should set
      --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
    to retain the old behaviour.
---
 drivers/base/Kconfig     | 15 +++++++++++----
 drivers/firmware/Kconfig |  1 -
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 5daa259..de3903e 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -146,13 +146,20 @@ config EXTRA_FIRMWARE_DIR
 config FW_LOADER_USER_HELPER
 	bool "Fallback user-helper invocation for firmware loading"
 	depends on FW_LOADER
-	default y
+	default n
 	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.
+	  direct file loading in kernel fails.
+
+	  Since March 2013, a default udev build does not understand
+	  firmware loading requests.  These udev versions will not
+	  even indicate failure; instead they cause long timeouts.
+	  This can dramatically slow down the boot process.
+
+	  Say Y only if you have special firmware-loading requirements
+	  and if you have a non-standard helper that will handle these
+	  requests.
 
 config DEBUG_DRIVER
 	bool "Driver Core verbose debug messages"
diff --git a/drivers/firmware/Kconfig b/drivers/firmware/Kconfig
index 07478728..9387630 100644
--- a/drivers/firmware/Kconfig
+++ b/drivers/firmware/Kconfig
@@ -64,7 +64,6 @@ config DELL_RBU
 	tristate "BIOS update support for DELL systems via sysfs"
 	depends on X86
 	select FW_LOADER
-	select FW_LOADER_USER_HELPER
 	help
 	 Say m if you want to have the option of updating the BIOS for your
 	 DELL system. Note you need a Dell OpenManage or Dell Update package (DUP)
-- 
1.8.3.1

Re: [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it

[Maarten Lankhorst]

Op 05-08-13 18:29, Andy Lutomirski schreef:
> The systemd commit below can delay firmware loading by multiple
> minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
> noticed that the systemd-udev change would break new kernels as well
> as old kernels.
>
> Since the kernel apparently can't count on reasonable userspace
> support, turn this thing off by default.
>
> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
> Author: Tom Gundersen <teg@jklm.no>
> Date:   Mon Mar 18 15:12:18 2013 +0100
>
>     udev: make firmware loading optional and disable by default
>
>     Distros that whish to support old kernels should set
>       --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
>     to retain the old behaviour.
>
methinks this patch should be reverted then, or a stub should be added to udev to always fail firmware loading so timeouts don't occur.

~Maarten

Re: [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it

[Maarten Lankhorst]

Op 05-08-13 18:29, Andy Lutomirski schreef:
> The systemd commit below can delay firmware loading by multiple
> minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
> noticed that the systemd-udev change would break new kernels as well
> as old kernels.
>
> Since the kernel apparently can't count on reasonable userspace
> support, turn this thing off by default.
>
> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
> Author: Tom Gundersen <teg@jklm.no>
> Date:   Mon Mar 18 15:12:18 2013 +0100
>
>     udev: make firmware loading optional and disable by default
>
>     Distros that whish to support old kernels should set
>       --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
>     to retain the old behaviour.
>
methinks this patch should be reverted then, or a stub should be added to udev to always fail firmware loading so timeouts don't occur.

~Maarten

Re: [systemd-devel] [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it

[Tom Gundersen]

On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
<m.b.lankhorst@gmail.com> wrote:
> Op 05-08-13 18:29, Andy Lutomirski schreef:
>> The systemd commit below can delay firmware loading by multiple
>> minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
>> noticed that the systemd-udev change would break new kernels as well
>> as old kernels.
>>
>> Since the kernel apparently can't count on reasonable userspace
>> support, turn this thing off by default.
>>
>> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
>> Author: Tom Gundersen <teg@jklm.no>
>> Date:   Mon Mar 18 15:12:18 2013 +0100
>>
>>     udev: make firmware loading optional and disable by default
>>
>>     Distros that whish to support old kernels should set
>>       --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
>>     to retain the old behaviour.
>>
> methinks this patch should be reverted then,

Well, all the code is still there, so it can be enabled if anyone wants it.

> or a stub should be added to udev to always fail firmware loading so timeouts don't occur.

I think the only use (if any) of a userspace firmware loader would be
for anyone who wants a custom one (i.e., not udev), so we shouldn't
just fail the loading from udev unconditionally.

How about we just improve the udev documentation a bit, similar to
Andy's kernel patch?

Cheers,

Tom

Re: [systemd-devel] [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it

[Tom Gundersen]

On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
<m.b.lankhorst@gmail.com> wrote:
> Op 05-08-13 18:29, Andy Lutomirski schreef:
>> The systemd commit below can delay firmware loading by multiple
>> minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
>> noticed that the systemd-udev change would break new kernels as well
>> as old kernels.
>>
>> Since the kernel apparently can't count on reasonable userspace
>> support, turn this thing off by default.
>>
>> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
>> Author: Tom Gundersen <teg@jklm.no>
>> Date:   Mon Mar 18 15:12:18 2013 +0100
>>
>>     udev: make firmware loading optional and disable by default
>>
>>     Distros that whish to support old kernels should set
>>       --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
>>     to retain the old behaviour.
>>
> methinks this patch should be reverted then,

Well, all the code is still there, so it can be enabled if anyone wants it.

> or a stub should be added to udev to always fail firmware loading so timeouts don't occur.

I think the only use (if any) of a userspace firmware loader would be
for anyone who wants a custom one (i.e., not udev), so we shouldn't
just fail the loading from udev unconditionally.

How about we just improve the udev documentation a bit, similar to
Andy's kernel patch?

Cheers,

Tom

Re: [systemd-devel] [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it

[Tom Gundersen]

On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <teg@jklm.no> wrote:
> On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
> <m.b.lankhorst@gmail.com> wrote:
>> Op 05-08-13 18:29, Andy Lutomirski schreef:
>>> The systemd commit below can delay firmware loading by multiple
>>> minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
>>> noticed that the systemd-udev change would break new kernels as well
>>> as old kernels.
>>>
>>> Since the kernel apparently can't count on reasonable userspace
>>> support, turn this thing off by default.
>>>
>>> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
>>> Author: Tom Gundersen <teg@jklm.no>
>>> Date:   Mon Mar 18 15:12:18 2013 +0100
>>>
>>>     udev: make firmware loading optional and disable by default
>>>
>>>     Distros that whish to support old kernels should set
>>>       --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
>>>     to retain the old behaviour.
>>>
>> methinks this patch should be reverted then,
>
> Well, all the code is still there, so it can be enabled if anyone wants it.
>
>> or a stub should be added to udev to always fail firmware loading so timeouts don't occur.
>
> I think the only use (if any) of a userspace firmware loader would be
> for anyone who wants a custom one (i.e., not udev), so we shouldn't
> just fail the loading from udev unconditionally.
>
> How about we just improve the udev documentation a bit, similar to
> Andy's kernel patch?

Sorry, I should first have checked. We already document this in the README:

>        Userspace firmware loading is deprecated, will go away, and
>        sometimes causes problems:
>          CONFIG_FW_LOADER_USER_HELPER=n

-t

Re: [systemd-devel] [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it

[Tom Gundersen]

On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <teg@jklm.no> wrote:
> On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
> <m.b.lankhorst@gmail.com> wrote:
>> Op 05-08-13 18:29, Andy Lutomirski schreef:
>>> The systemd commit below can delay firmware loading by multiple
>>> minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
>>> noticed that the systemd-udev change would break new kernels as well
>>> as old kernels.
>>>
>>> Since the kernel apparently can't count on reasonable userspace
>>> support, turn this thing off by default.
>>>
>>> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
>>> Author: Tom Gundersen <teg@jklm.no>
>>> Date:   Mon Mar 18 15:12:18 2013 +0100
>>>
>>>     udev: make firmware loading optional and disable by default
>>>
>>>     Distros that whish to support old kernels should set
>>>       --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
>>>     to retain the old behaviour.
>>>
>> methinks this patch should be reverted then,
>
> Well, all the code is still there, so it can be enabled if anyone wants it.
>
>> or a stub should be added to udev to always fail firmware loading so timeouts don't occur.
>
> I think the only use (if any) of a userspace firmware loader would be
> for anyone who wants a custom one (i.e., not udev), so we shouldn't
> just fail the loading from udev unconditionally.
>
> How about we just improve the udev documentation a bit, similar to
> Andy's kernel patch?

Sorry, I should first have checked. We already document this in the README:

>        Userspace firmware loading is deprecated, will go away, and
>        sometimes causes problems:
>          CONFIG_FW_LOADER_USER_HELPER=n

-t

Re: [systemd-devel] [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it

[Andy Lutomirski]

On Tue, Aug 6, 2013 at 2:17 AM, Tom Gundersen <teg@jklm.no> wrote:
> On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <teg@jklm.no> wrote:
>> On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
>> <m.b.lankhorst@gmail.com> wrote:
>>> Op 05-08-13 18:29, Andy Lutomirski schreef:
>>>> The systemd commit below can delay firmware loading by multiple
>>>> minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
>>>> noticed that the systemd-udev change would break new kernels as well
>>>> as old kernels.
>>>>
>>>> Since the kernel apparently can't count on reasonable userspace
>>>> support, turn this thing off by default.
>>>>
>>>> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
>>>> Author: Tom Gundersen <teg@jklm.no>
>>>> Date:   Mon Mar 18 15:12:18 2013 +0100
>>>>
>>>>     udev: make firmware loading optional and disable by default
>>>>
>>>>     Distros that whish to support old kernels should set
>>>>       --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
>>>>     to retain the old behaviour.
>>>>
>>> methinks this patch should be reverted then,
>>
>> Well, all the code is still there, so it can be enabled if anyone wants it.
>>
>>> or a stub should be added to udev to always fail firmware loading so timeouts don't occur.
>>
>> I think the only use (if any) of a userspace firmware loader would be
>> for anyone who wants a custom one (i.e., not udev), so we shouldn't
>> just fail the loading from udev unconditionally.
>>
>> How about we just improve the udev documentation a bit, similar to
>> Andy's kernel patch?
>
> Sorry, I should first have checked. We already document this in the README:
>
>>        Userspace firmware loading is deprecated, will go away, and
>>        sometimes causes problems:
>>          CONFIG_FW_LOADER_USER_HELPER=n

TBH, the udev README is the last thing I'm going to check to figure
out why I don't have wifi for a couple minutes after boot.  Also, the
message is missing the point.  It's not that it's deprecated and
sometimes causes problems -- it's that udev *changed behavior* and
breaks your system if you have CONFIG_FW_LOADER_USER_HELPER=y.

If udev logged something (for a couple of years), that would be a
different story.

--Andy

Re: [systemd-devel] [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it

[Andy Lutomirski]

On Tue, Aug 6, 2013 at 2:17 AM, Tom Gundersen <teg@jklm.no> wrote:
> On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <teg@jklm.no> wrote:
>> On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
>> <m.b.lankhorst@gmail.com> wrote:
>>> Op 05-08-13 18:29, Andy Lutomirski schreef:
>>>> The systemd commit below can delay firmware loading by multiple
>>>> minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
>>>> noticed that the systemd-udev change would break new kernels as well
>>>> as old kernels.
>>>>
>>>> Since the kernel apparently can't count on reasonable userspace
>>>> support, turn this thing off by default.
>>>>
>>>> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
>>>> Author: Tom Gundersen <teg@jklm.no>
>>>> Date:   Mon Mar 18 15:12:18 2013 +0100
>>>>
>>>>     udev: make firmware loading optional and disable by default
>>>>
>>>>     Distros that whish to support old kernels should set
>>>>       --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
>>>>     to retain the old behaviour.
>>>>
>>> methinks this patch should be reverted then,
>>
>> Well, all the code is still there, so it can be enabled if anyone wants it.
>>
>>> or a stub should be added to udev to always fail firmware loading so timeouts don't occur.
>>
>> I think the only use (if any) of a userspace firmware loader would be
>> for anyone who wants a custom one (i.e., not udev), so we shouldn't
>> just fail the loading from udev unconditionally.
>>
>> How about we just improve the udev documentation a bit, similar to
>> Andy's kernel patch?
>
> Sorry, I should first have checked. We already document this in the README:
>
>>        Userspace firmware loading is deprecated, will go away, and
>>        sometimes causes problems:
>>          CONFIG_FW_LOADER_USER_HELPER=n

TBH, the udev README is the last thing I'm going to check to figure
out why I don't have wifi for a couple minutes after boot.  Also, the
message is missing the point.  It's not that it's deprecated and
sometimes causes problems -- it's that udev *changed behavior* and
breaks your system if you have CONFIG_FW_LOADER_USER_HELPER=y.

If udev logged something (for a couple of years), that would be a
different story.

--Andy

Re: [systemd-devel] [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it

[Bryan Kadzban]

On Tue, Aug 06, 2013 at 11:17:17AM +0200, Tom Gundersen wrote:
> On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <teg@jklm.no> wrote:
> > On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
> > <m.b.lankhorst@gmail.com> wrote:
> >> Op 05-08-13 18:29, Andy Lutomirski schreef:
> >>> The systemd commit below can delay firmware loading by multiple
> >>> minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
> >>> noticed that the systemd-udev change would break new kernels as well
> >>> as old kernels.
> >>>
> >>> Since the kernel apparently can't count on reasonable userspace
> >>> support, turn this thing off by default.
> >>>
> >>> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
> >>> Author: Tom Gundersen <teg@jklm.no>
> >>> Date:   Mon Mar 18 15:12:18 2013 +0100
> >>>
> >>>     udev: make firmware loading optional and disable by default
> >>>
> >>>     Distros that whish to support old kernels should set
> >>>       --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
> >>>     to retain the old behaviour.
> >>>
> >> methinks this patch should be reverted then,
> >
> > Well, all the code is still there, so it can be enabled if anyone wants it.
> >
> >> or a stub should be added to udev to always fail firmware loading so timeouts don't occur.
> >
> > I think the only use (if any) of a userspace firmware loader would be
> > for anyone who wants a custom one (i.e., not udev), so we shouldn't
> > just fail the loading from udev unconditionally.
> >
> > How about we just improve the udev documentation a bit, similar to
> > Andy's kernel patch?
> 
> Sorry, I should first have checked. We already document this in the README:
> 
> >        Userspace firmware loading is deprecated, will go away, and
> >        sometimes causes problems:
> >          CONFIG_FW_LOADER_USER_HELPER=n

...And this patch is making the kernel default to the correct behavior,
instead of the now-broken-by-udev behavior.

I'm not sure I see the issue with it?  :-)

(Add me to the list of people that think udev is broken too, fwiw.  But
let's at least not leave *both* sides in a broken-by-default state.)

Re: [systemd-devel] [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it

[Bryan Kadzban]

On Tue, Aug 06, 2013 at 11:17:17AM +0200, Tom Gundersen wrote:
> On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <teg@jklm.no> wrote:
> > On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
> > <m.b.lankhorst@gmail.com> wrote:
> >> Op 05-08-13 18:29, Andy Lutomirski schreef:
> >>> The systemd commit below can delay firmware loading by multiple
> >>> minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
> >>> noticed that the systemd-udev change would break new kernels as well
> >>> as old kernels.
> >>>
> >>> Since the kernel apparently can't count on reasonable userspace
> >>> support, turn this thing off by default.
> >>>
> >>> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
> >>> Author: Tom Gundersen <teg@jklm.no>
> >>> Date:   Mon Mar 18 15:12:18 2013 +0100
> >>>
> >>>     udev: make firmware loading optional and disable by default
> >>>
> >>>     Distros that whish to support old kernels should set
> >>>       --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
> >>>     to retain the old behaviour.
> >>>
> >> methinks this patch should be reverted then,
> >
> > Well, all the code is still there, so it can be enabled if anyone wants it.
> >
> >> or a stub should be added to udev to always fail firmware loading so timeouts don't occur.
> >
> > I think the only use (if any) of a userspace firmware loader would be
> > for anyone who wants a custom one (i.e., not udev), so we shouldn't
> > just fail the loading from udev unconditionally.
> >
> > How about we just improve the udev documentation a bit, similar to
> > Andy's kernel patch?
> 
> Sorry, I should first have checked. We already document this in the README:
> 
> >        Userspace firmware loading is deprecated, will go away, and
> >        sometimes causes problems:
> >          CONFIG_FW_LOADER_USER_HELPER=n

...And this patch is making the kernel default to the correct behavior,
instead of the now-broken-by-udev behavior.

I'm not sure I see the issue with it?  :-)

(Add me to the list of people that think udev is broken too, fwiw.  But
let's at least not leave *both* sides in a broken-by-default state.)

Re: [systemd-devel] [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it

[Andy Lutomirski]

On Tue, Aug 6, 2013 at 5:24 PM, Tom Gundersen <teg@jklm.no> wrote:
>
> On 6 Aug 2013 18:32, "Bryan Kadzban" <bryan@kadzban.is-a-geek.net> wrote:
>>
>> On Tue, Aug 06, 2013 at 11:17:17AM +0200, Tom Gundersen wrote:
>> > On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <teg@jklm.no> wrote:
>> > > On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
>> > > <m.b.lankhorst@gmail.com> wrote:
>> > >> Op 05-08-13 18:29, Andy Lutomirski schreef:
>> > >>> The systemd commit below can delay firmware loading by multiple
>> > >>> minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
>> > >>> noticed that the systemd-udev change would break new kernels as well
>> > >>> as old kernels.
>> > >>>
>> > >>> Since the kernel apparently can't count on reasonable userspace
>> > >>> support, turn this thing off by default.
>> > >>>
>> > >>> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
>> > >>> Author: Tom Gundersen <teg@jklm.no>
>> > >>> Date:   Mon Mar 18 15:12:18 2013 +0100
>> > >>>
>> > >>>     udev: make firmware loading optional and disable by default
>> > >>>
>> > >>>     Distros that whish to support old kernels should set
>> > >>>
>> > >>> --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
>> > >>>     to retain the old behaviour.
>> > >>>
>> > >> methinks this patch should be reverted then,
>> > >
>> > > Well, all the code is still there, so it can be enabled if anyone
>> > > wants it.
>> > >
>> > >> or a stub should be added to udev to always fail firmware loading so
>> > >> timeouts don't occur.
>> > >
>> > > I think the only use (if any) of a userspace firmware loader would be
>> > > for anyone who wants a custom one (i.e., not udev), so we shouldn't
>> > > just fail the loading from udev unconditionally.
>> > >
>> > > How about we just improve the udev documentation a bit, similar to
>> > > Andy's kernel patch?
>> >
>> > Sorry, I should first have checked. We already document this in the
>> > README:
>> >
>> > >        Userspace firmware loading is deprecated, will go away, and
>> > >        sometimes causes problems:
>> > >          CONFIG_FW_LOADER_USER_HELPER=n
>>
>> ...And this patch is making the kernel default to the correct behavior,
>> instead of the now-broken-by-udev behavior.
>>
>> I'm not sure I see the issue with it?  :-)
>
> Oh yeah this patch is totally the right thing to do, I was just arguing that
> there is nothing to be done on the udev side.
>
>> (Add me to the list of people that think udev is broken too, fwiw.  But
>> let's at least not leave *both* sides in a broken-by-default state.)
>
> Well I don't think it is too much to ask that the kernel and udev should be
> configured in a consistent way. Especially as thing still work even if you
> get it wrong, albeit with a delay.

Except that the current defaults are inconsistent and there is no
explanation anywhere in the logs when this is screwed up.

--Andy

Re: [systemd-devel] [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it

[Andy Lutomirski]

On Tue, Aug 6, 2013 at 5:24 PM, Tom Gundersen <teg@jklm.no> wrote:
>
> On 6 Aug 2013 18:32, "Bryan Kadzban" <bryan@kadzban.is-a-geek.net> wrote:
>>
>> On Tue, Aug 06, 2013 at 11:17:17AM +0200, Tom Gundersen wrote:
>> > On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <teg@jklm.no> wrote:
>> > > On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
>> > > <m.b.lankhorst@gmail.com> wrote:
>> > >> Op 05-08-13 18:29, Andy Lutomirski schreef:
>> > >>> The systemd commit below can delay firmware loading by multiple
>> > >>> minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
>> > >>> noticed that the systemd-udev change would break new kernels as well
>> > >>> as old kernels.
>> > >>>
>> > >>> Since the kernel apparently can't count on reasonable userspace
>> > >>> support, turn this thing off by default.
>> > >>>
>> > >>> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
>> > >>> Author: Tom Gundersen <teg@jklm.no>
>> > >>> Date:   Mon Mar 18 15:12:18 2013 +0100
>> > >>>
>> > >>>     udev: make firmware loading optional and disable by default
>> > >>>
>> > >>>     Distros that whish to support old kernels should set
>> > >>>
>> > >>> --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
>> > >>>     to retain the old behaviour.
>> > >>>
>> > >> methinks this patch should be reverted then,
>> > >
>> > > Well, all the code is still there, so it can be enabled if anyone
>> > > wants it.
>> > >
>> > >> or a stub should be added to udev to always fail firmware loading so
>> > >> timeouts don't occur.
>> > >
>> > > I think the only use (if any) of a userspace firmware loader would be
>> > > for anyone who wants a custom one (i.e., not udev), so we shouldn't
>> > > just fail the loading from udev unconditionally.
>> > >
>> > > How about we just improve the udev documentation a bit, similar to
>> > > Andy's kernel patch?
>> >
>> > Sorry, I should first have checked. We already document this in the
>> > README:
>> >
>> > >        Userspace firmware loading is deprecated, will go away, and
>> > >        sometimes causes problems:
>> > >          CONFIG_FW_LOADER_USER_HELPER=n
>>
>> ...And this patch is making the kernel default to the correct behavior,
>> instead of the now-broken-by-udev behavior.
>>
>> I'm not sure I see the issue with it?  :-)
>
> Oh yeah this patch is totally the right thing to do, I was just arguing that
> there is nothing to be done on the udev side.
>
>> (Add me to the list of people that think udev is broken too, fwiw.  But
>> let's at least not leave *both* sides in a broken-by-default state.)
>
> Well I don't think it is too much to ask that the kernel and udev should be
> configured in a consistent way. Especially as thing still work even if you
> get it wrong, albeit with a delay.

Except that the current defaults are inconsistent and there is no
explanation anywhere in the logs when this is screwed up.

--Andy

[PATCH] udev: fail firmware loading immediately if no search path is defined

[Maarten Lankhorst]

Op 07-08-13 02:26, Andy Lutomirski schreef:
> On Tue, Aug 6, 2013 at 5:24 PM, Tom Gundersen <teg@jklm.no> wrote:
>> On 6 Aug 2013 18:32, "Bryan Kadzban" <bryan@kadzban.is-a-geek.net> wrote:
>>> On Tue, Aug 06, 2013 at 11:17:17AM +0200, Tom Gundersen wrote:
>>>> On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <teg@jklm.no> wrote:
>>>>> On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
>>>>> <m.b.lankhorst@gmail.com> wrote:
>>>>>> Op 05-08-13 18:29, Andy Lutomirski schreef:
>>>>>>> The systemd commit below can delay firmware loading by multiple
>>>>>>> minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
>>>>>>> noticed that the systemd-udev change would break new kernels as well
>>>>>>> as old kernels.
>>>>>>>
>>>>>>> Since the kernel apparently can't count on reasonable userspace
>>>>>>> support, turn this thing off by default.
>>>>>>>
>>>>>>> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
>>>>>>> Author: Tom Gundersen <teg@jklm.no>
>>>>>>> Date:   Mon Mar 18 15:12:18 2013 +0100
>>>>>>>
>>>>>>>     udev: make firmware loading optional and disable by default
>>>>>>>
>>>>>>>     Distros that whish to support old kernels should set
>>>>>>>
>>>>>>> --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
>>>>>>>     to retain the old behaviour.
>>>>>>>
>>>>>> methinks this patch should be reverted then,
>>>>> Well, all the code is still there, so it can be enabled if anyone
>>>>> wants it.
>>>>>
>>>>>> or a stub should be added to udev to always fail firmware loading so
>>>>>> timeouts don't occur.
>>>>> I think the only use (if any) of a userspace firmware loader would be
>>>>> for anyone who wants a custom one (i.e., not udev), so we shouldn't
>>>>> just fail the loading from udev unconditionally.
>>>>>
>>>>> How about we just improve the udev documentation a bit, similar to
>>>>> Andy's kernel patch?
>>>> Sorry, I should first have checked. We already document this in the
>>>> README:
>>>>
>>>>>        Userspace firmware loading is deprecated, will go away, and
>>>>>        sometimes causes problems:
>>>>>          CONFIG_FW_LOADER_USER_HELPER=n
>>> ...And this patch is making the kernel default to the correct behavior,
>>> instead of the now-broken-by-udev behavior.
>>>
>>> I'm not sure I see the issue with it?  :-)
>> Oh yeah this patch is totally the right thing to do, I was just arguing that
>> there is nothing to be done on the udev side.
>>
>>> (Add me to the list of people that think udev is broken too, fwiw.  But
>>> let's at least not leave *both* sides in a broken-by-default state.)
>> Well I don't think it is too much to ask that the kernel and udev should be
>> configured in a consistent way. Especially as thing still work even if you
>> get it wrong, albeit with a delay.
> Except that the current defaults are inconsistent and there is no
> explanation anywhere in the logs when this is screwed up.
>
So what is wrong with my 'fail in udev immediately if not configured' idea? In that case it
doesn't matter whether CONFIG_FW_LOADER_USER_HELPER is set or not.

You could even print a useful message for the user in udev to the log, so they have an idea of what
happened. Breaking udev on older still supported kernels by default without printing any debug info
is silly, and the only cost is a small increase in disk space when unused. I did so in below patch.

~Maarten

I converted < ELEMENTSOF to != ELEMENTSOF in the loop to work correctly when the array is empty.
Most code in udev-builtin-firmware is eliminated at -O2 optimization level when FIRMWARE_PATH
is not explicitly set.

8<----
diff --git a/Makefile.am b/Makefile.am
index b8b8d06..2097629 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2235,6 +2235,7 @@ libudev_core_la_SOURCES = \
 	src/udev/udev-ctrl.c \
 	src/udev/udev-builtin.c \
 	src/udev/udev-builtin-btrfs.c \
+	src/udev/udev-builtin-firmware.c \
 	src/udev/udev-builtin-hwdb.c \
 	src/udev/udev-builtin-input_id.c \
 	src/udev/udev-builtin-keyboard.c \
@@ -2271,14 +2272,6 @@ libudev_core_la_CPPFLAGS = \
 	$(AM_CPPFLAGS) \
 	-DFIRMWARE_PATH="$(FIRMWARE_PATH)"
 
-if ENABLE_FIRMWARE
-libudev_core_la_SOURCES += \
-	src/udev/udev-builtin-firmware.c
-
-dist_udevrules_DATA += \
-	rules/50-firmware.rules
-endif
-
 if HAVE_KMOD
 libudev_core_la_SOURCES += \
 	src/udev/udev-builtin-kmod.c
diff --git a/configure.ac b/configure.ac
index 0ecc716..dc7a3e3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -823,8 +823,6 @@ for i in $with_firmware_path; do
 done
 IFS=$OLD_IFS
 AC_SUBST(FIRMWARE_PATH)
-AS_IF([test "x${FIRMWARE_PATH}" != "x"], [ AC_DEFINE(HAVE_FIRMWARE, 1, [Define if FIRMWARE is available]) ])
-AM_CONDITIONAL(ENABLE_FIRMWARE, [test "x${FIRMWARE_PATH}" != "x"])
 
 # ------------------------------------------------------------------------------
 AC_ARG_ENABLE([gudev],
diff --git a/rules/50-firmware.rules b/rules/50-firmware.rules
deleted file mode 100644
index f0ae684..0000000
--- a/rules/50-firmware.rules
+++ /dev/null
@@ -1,3 +0,0 @@
-# do not edit this file, it will be overwritten on update
-
-SUBSYSTEM="firmware", ACTION="add", RUN{builtin}="firmware"
diff --git a/rules/50-udev-default.rules b/rules/50-udev-default.rules
index f764789..645830e 100644
--- a/rules/50-udev-default.rules
+++ b/rules/50-udev-default.rules
@@ -8,6 +8,7 @@ SUBSYSTEM="rtc", KERNEL="rtc0", SYMLINK+="rtc", OPTIONS+="link_priority=-100"
 
 SUBSYSTEM="usb", ENV{DEVTYPE}="usb_device", IMPORT{builtin}="usb_id", IMPORT{builtin}="hwdb --subsystem=usb"
 SUBSYSTEM="input", ENV{ID_INPUT}="", IMPORT{builtin}="input_id"
+SUBSYSTEM="firmware", ACTION="add", IMPORT{builtin}="firmware"
 ENV{MODALIAS}!="", IMPORT{builtin}="hwdb --subsystem=$env{SUBSYSTEM}"
 
 ACTION!="add", GOTO="default_permissions_end"
diff --git a/shell-completion/bash/udevadm b/shell-completion/bash/udevadm
index 8ad8550..c22a3e2 100644
--- a/shell-completion/bash/udevadm
+++ b/shell-completion/bash/udevadm
@@ -83,7 +83,7 @@ _udevadm() {
                         fi
                         ;;
                 'test-builtin')
-                        comps='blkid btrfs hwdb input_id keyboard kmod net_id path_id usb_id uaccess'
+                        comps='blkid btrfs firmware hwdb input_id keyboard kmod net_id path_id usb_id uaccess'
                         ;;
                 *)
                         comps=${VERBS[*]}
diff --git a/src/udev/udev-builtin-firmware.c b/src/udev/udev-builtin-firmware.c
index b80940b..e678545 100644
--- a/src/udev/udev-builtin-firmware.c
+++ b/src/udev/udev-builtin-firmware.c
@@ -97,7 +97,7 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo
 
         /* lookup firmware file */
         uname(&kernel);
-        for (i = 0; i < ELEMENTSOF(searchpath); i++) {
+        for (i = 0; i != ELEMENTSOF(searchpath); i++) {
                 strscpyl(fwpath, sizeof(fwpath), searchpath[i], kernel.release, "/", firmware, NULL);
                 fwfile = fopen(fwpath, "re");
                 if (fwfile != NULL)
@@ -112,7 +112,10 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo
         strscpyl(loadpath, sizeof(loadpath), udev_device_get_syspath(dev), "/loading", NULL);
 
         if (fwfile = NULL) {
-                log_debug("did not find firmware file '%s'\n", firmware);
+                if (ELEMENTSOF(searchpath))
+                    log_debug("did not find firmware file '%s'\n", firmware);
+                else
+                    log_error("could not load firmware file '%s', firmware loading not enabled\n", firmware);
                 rc = EXIT_FAILURE;
                 /*
                  * Do not cancel the request in the initrd, the real root might have
diff --git a/src/udev/udev-builtin.c b/src/udev/udev-builtin.c
index 6b3a518..5a5cb30 100644
--- a/src/udev/udev-builtin.c
+++ b/src/udev/udev-builtin.c
@@ -34,9 +34,7 @@ static const struct udev_builtin *builtins[] = {
         [UDEV_BUILTIN_BLKID] = &udev_builtin_blkid,
 #endif
         [UDEV_BUILTIN_BTRFS] = &udev_builtin_btrfs,
-#ifdef HAVE_FIRMWARE
         [UDEV_BUILTIN_FIRMWARE] = &udev_builtin_firmware,
-#endif
         [UDEV_BUILTIN_HWDB] = &udev_builtin_hwdb,
         [UDEV_BUILTIN_INPUT_ID] = &udev_builtin_input_id,
         [UDEV_BUILTIN_KEYBOARD] = &udev_builtin_keyboard,
diff --git a/src/udev/udev.h b/src/udev/udev.h
index 8395926..31fa606 100644
--- a/src/udev/udev.h
+++ b/src/udev/udev.h
@@ -140,9 +140,7 @@ enum udev_builtin_cmd {
         UDEV_BUILTIN_BLKID,
 #endif
         UDEV_BUILTIN_BTRFS,
-#ifdef HAVE_FIRMWARE
         UDEV_BUILTIN_FIRMWARE,
-#endif
         UDEV_BUILTIN_HWDB,
         UDEV_BUILTIN_INPUT_ID,
         UDEV_BUILTIN_KEYBOARD,
@@ -170,9 +168,7 @@ struct udev_builtin {
 extern const struct udev_builtin udev_builtin_blkid;
 #endif
 extern const struct udev_builtin udev_builtin_btrfs;
-#ifdef HAVE_FIRMWARE
 extern const struct udev_builtin udev_builtin_firmware;
-#endif
 extern const struct udev_builtin udev_builtin_hwdb;
 extern const struct udev_builtin udev_builtin_input_id;
 extern const struct udev_builtin udev_builtin_keyboard;
diff --git a/src/udev/udevd.c b/src/udev/udevd.c
index 45ec3d6..e27a33a 100644
--- a/src/udev/udevd.c
+++ b/src/udev/udevd.c
@@ -98,9 +98,7 @@ struct event {
         dev_t devnum;
         int ifindex;
         bool is_block;
-#ifdef HAVE_FIRMWARE
         bool nodelay;
-#endif
 };
 
 static inline struct event *node_to_event(struct udev_list_node *node)
@@ -444,10 +442,8 @@ static int event_queue_insert(struct udev_device *dev)
         event->devnum = udev_device_get_devnum(dev);
         event->is_block = streq("block", udev_device_get_subsystem(dev));
         event->ifindex = udev_device_get_ifindex(dev);
-#ifdef HAVE_FIRMWARE
         if (streq(udev_device_get_subsystem(dev), "firmware"))
                 event->nodelay = true;
-#endif
 
         udev_queue_export_device_queued(udev_queue_export, dev);
         log_debug("seq %llu queued, '%s' '%s'\n", udev_device_get_seqnum(dev),
@@ -527,11 +523,9 @@ static bool is_devpath_busy(struct event *event)
                         return true;
                 }
 
-#ifdef HAVE_FIRMWARE
                 /* allow to bypass the dependency tracking */
                 if (event->nodelay)
                         continue;
-#endif
 
                 /* parent device event found */
                 if (event->devpath[common] = '/') {

[PATCH] udev: fail firmware loading immediately if no search path is defined

[Maarten Lankhorst]

Op 07-08-13 02:26, Andy Lutomirski schreef:
> On Tue, Aug 6, 2013 at 5:24 PM, Tom Gundersen <teg@jklm.no> wrote:
>> On 6 Aug 2013 18:32, "Bryan Kadzban" <bryan@kadzban.is-a-geek.net> wrote:
>>> On Tue, Aug 06, 2013 at 11:17:17AM +0200, Tom Gundersen wrote:
>>>> On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <teg@jklm.no> wrote:
>>>>> On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
>>>>> <m.b.lankhorst@gmail.com> wrote:
>>>>>> Op 05-08-13 18:29, Andy Lutomirski schreef:
>>>>>>> The systemd commit below can delay firmware loading by multiple
>>>>>>> minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
>>>>>>> noticed that the systemd-udev change would break new kernels as well
>>>>>>> as old kernels.
>>>>>>>
>>>>>>> Since the kernel apparently can't count on reasonable userspace
>>>>>>> support, turn this thing off by default.
>>>>>>>
>>>>>>> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
>>>>>>> Author: Tom Gundersen <teg@jklm.no>
>>>>>>> Date:   Mon Mar 18 15:12:18 2013 +0100
>>>>>>>
>>>>>>>     udev: make firmware loading optional and disable by default
>>>>>>>
>>>>>>>     Distros that whish to support old kernels should set
>>>>>>>
>>>>>>> --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
>>>>>>>     to retain the old behaviour.
>>>>>>>
>>>>>> methinks this patch should be reverted then,
>>>>> Well, all the code is still there, so it can be enabled if anyone
>>>>> wants it.
>>>>>
>>>>>> or a stub should be added to udev to always fail firmware loading so
>>>>>> timeouts don't occur.
>>>>> I think the only use (if any) of a userspace firmware loader would be
>>>>> for anyone who wants a custom one (i.e., not udev), so we shouldn't
>>>>> just fail the loading from udev unconditionally.
>>>>>
>>>>> How about we just improve the udev documentation a bit, similar to
>>>>> Andy's kernel patch?
>>>> Sorry, I should first have checked. We already document this in the
>>>> README:
>>>>
>>>>>        Userspace firmware loading is deprecated, will go away, and
>>>>>        sometimes causes problems:
>>>>>          CONFIG_FW_LOADER_USER_HELPER=n
>>> ...And this patch is making the kernel default to the correct behavior,
>>> instead of the now-broken-by-udev behavior.
>>>
>>> I'm not sure I see the issue with it?  :-)
>> Oh yeah this patch is totally the right thing to do, I was just arguing that
>> there is nothing to be done on the udev side.
>>
>>> (Add me to the list of people that think udev is broken too, fwiw.  But
>>> let's at least not leave *both* sides in a broken-by-default state.)
>> Well I don't think it is too much to ask that the kernel and udev should be
>> configured in a consistent way. Especially as thing still work even if you
>> get it wrong, albeit with a delay.
> Except that the current defaults are inconsistent and there is no
> explanation anywhere in the logs when this is screwed up.
>
So what is wrong with my 'fail in udev immediately if not configured' idea? In that case it
doesn't matter whether CONFIG_FW_LOADER_USER_HELPER is set or not.

You could even print a useful message for the user in udev to the log, so they have an idea of what
happened. Breaking udev on older still supported kernels by default without printing any debug info
is silly, and the only cost is a small increase in disk space when unused. I did so in below patch.

~Maarten

I converted < ELEMENTSOF to != ELEMENTSOF in the loop to work correctly when the array is empty.
Most code in udev-builtin-firmware is eliminated at -O2 optimization level when FIRMWARE_PATH
is not explicitly set.

8<----
diff --git a/Makefile.am b/Makefile.am
index b8b8d06..2097629 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -2235,6 +2235,7 @@ libudev_core_la_SOURCES = \
 	src/udev/udev-ctrl.c \
 	src/udev/udev-builtin.c \
 	src/udev/udev-builtin-btrfs.c \
+	src/udev/udev-builtin-firmware.c \
 	src/udev/udev-builtin-hwdb.c \
 	src/udev/udev-builtin-input_id.c \
 	src/udev/udev-builtin-keyboard.c \
@@ -2271,14 +2272,6 @@ libudev_core_la_CPPFLAGS = \
 	$(AM_CPPFLAGS) \
 	-DFIRMWARE_PATH="$(FIRMWARE_PATH)"
 
-if ENABLE_FIRMWARE
-libudev_core_la_SOURCES += \
-	src/udev/udev-builtin-firmware.c
-
-dist_udevrules_DATA += \
-	rules/50-firmware.rules
-endif
-
 if HAVE_KMOD
 libudev_core_la_SOURCES += \
 	src/udev/udev-builtin-kmod.c
diff --git a/configure.ac b/configure.ac
index 0ecc716..dc7a3e3 100644
--- a/configure.ac
+++ b/configure.ac
@@ -823,8 +823,6 @@ for i in $with_firmware_path; do
 done
 IFS=$OLD_IFS
 AC_SUBST(FIRMWARE_PATH)
-AS_IF([test "x${FIRMWARE_PATH}" != "x"], [ AC_DEFINE(HAVE_FIRMWARE, 1, [Define if FIRMWARE is available]) ])
-AM_CONDITIONAL(ENABLE_FIRMWARE, [test "x${FIRMWARE_PATH}" != "x"])
 
 # ------------------------------------------------------------------------------
 AC_ARG_ENABLE([gudev],
diff --git a/rules/50-firmware.rules b/rules/50-firmware.rules
deleted file mode 100644
index f0ae684..0000000
--- a/rules/50-firmware.rules
+++ /dev/null
@@ -1,3 +0,0 @@
-# do not edit this file, it will be overwritten on update
-
-SUBSYSTEM=="firmware", ACTION=="add", RUN{builtin}="firmware"
diff --git a/rules/50-udev-default.rules b/rules/50-udev-default.rules
index f764789..645830e 100644
--- a/rules/50-udev-default.rules
+++ b/rules/50-udev-default.rules
@@ -8,6 +8,7 @@ SUBSYSTEM=="rtc", KERNEL=="rtc0", SYMLINK+="rtc", OPTIONS+="link_priority=-100"
 
 SUBSYSTEM=="usb", ENV{DEVTYPE}=="usb_device", IMPORT{builtin}="usb_id", IMPORT{builtin}="hwdb --subsystem=usb"
 SUBSYSTEM=="input", ENV{ID_INPUT}=="", IMPORT{builtin}="input_id"
+SUBSYSTEM=="firmware", ACTION=="add", IMPORT{builtin}="firmware"
 ENV{MODALIAS}!="", IMPORT{builtin}="hwdb --subsystem=$env{SUBSYSTEM}"
 
 ACTION!="add", GOTO="default_permissions_end"
diff --git a/shell-completion/bash/udevadm b/shell-completion/bash/udevadm
index 8ad8550..c22a3e2 100644
--- a/shell-completion/bash/udevadm
+++ b/shell-completion/bash/udevadm
@@ -83,7 +83,7 @@ _udevadm() {
                         fi
                         ;;
                 'test-builtin')
-                        comps='blkid btrfs hwdb input_id keyboard kmod net_id path_id usb_id uaccess'
+                        comps='blkid btrfs firmware hwdb input_id keyboard kmod net_id path_id usb_id uaccess'
                         ;;
                 *)
                         comps=${VERBS[*]}
diff --git a/src/udev/udev-builtin-firmware.c b/src/udev/udev-builtin-firmware.c
index b80940b..e678545 100644
--- a/src/udev/udev-builtin-firmware.c
+++ b/src/udev/udev-builtin-firmware.c
@@ -97,7 +97,7 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo
 
         /* lookup firmware file */
         uname(&kernel);
-        for (i = 0; i < ELEMENTSOF(searchpath); i++) {
+        for (i = 0; i != ELEMENTSOF(searchpath); i++) {
                 strscpyl(fwpath, sizeof(fwpath), searchpath[i], kernel.release, "/", firmware, NULL);
                 fwfile = fopen(fwpath, "re");
                 if (fwfile != NULL)
@@ -112,7 +112,10 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo
         strscpyl(loadpath, sizeof(loadpath), udev_device_get_syspath(dev), "/loading", NULL);
 
         if (fwfile == NULL) {
-                log_debug("did not find firmware file '%s'\n", firmware);
+                if (ELEMENTSOF(searchpath))
+                    log_debug("did not find firmware file '%s'\n", firmware);
+                else
+                    log_error("could not load firmware file '%s', firmware loading not enabled\n", firmware);
                 rc = EXIT_FAILURE;
                 /*
                  * Do not cancel the request in the initrd, the real root might have
diff --git a/src/udev/udev-builtin.c b/src/udev/udev-builtin.c
index 6b3a518..5a5cb30 100644
--- a/src/udev/udev-builtin.c
+++ b/src/udev/udev-builtin.c
@@ -34,9 +34,7 @@ static const struct udev_builtin *builtins[] = {
         [UDEV_BUILTIN_BLKID] = &udev_builtin_blkid,
 #endif
         [UDEV_BUILTIN_BTRFS] = &udev_builtin_btrfs,
-#ifdef HAVE_FIRMWARE
         [UDEV_BUILTIN_FIRMWARE] = &udev_builtin_firmware,
-#endif
         [UDEV_BUILTIN_HWDB] = &udev_builtin_hwdb,
         [UDEV_BUILTIN_INPUT_ID] = &udev_builtin_input_id,
         [UDEV_BUILTIN_KEYBOARD] = &udev_builtin_keyboard,
diff --git a/src/udev/udev.h b/src/udev/udev.h
index 8395926..31fa606 100644
--- a/src/udev/udev.h
+++ b/src/udev/udev.h
@@ -140,9 +140,7 @@ enum udev_builtin_cmd {
         UDEV_BUILTIN_BLKID,
 #endif
         UDEV_BUILTIN_BTRFS,
-#ifdef HAVE_FIRMWARE
         UDEV_BUILTIN_FIRMWARE,
-#endif
         UDEV_BUILTIN_HWDB,
         UDEV_BUILTIN_INPUT_ID,
         UDEV_BUILTIN_KEYBOARD,
@@ -170,9 +168,7 @@ struct udev_builtin {
 extern const struct udev_builtin udev_builtin_blkid;
 #endif
 extern const struct udev_builtin udev_builtin_btrfs;
-#ifdef HAVE_FIRMWARE
 extern const struct udev_builtin udev_builtin_firmware;
-#endif
 extern const struct udev_builtin udev_builtin_hwdb;
 extern const struct udev_builtin udev_builtin_input_id;
 extern const struct udev_builtin udev_builtin_keyboard;
diff --git a/src/udev/udevd.c b/src/udev/udevd.c
index 45ec3d6..e27a33a 100644
--- a/src/udev/udevd.c
+++ b/src/udev/udevd.c
@@ -98,9 +98,7 @@ struct event {
         dev_t devnum;
         int ifindex;
         bool is_block;
-#ifdef HAVE_FIRMWARE
         bool nodelay;
-#endif
 };
 
 static inline struct event *node_to_event(struct udev_list_node *node)
@@ -444,10 +442,8 @@ static int event_queue_insert(struct udev_device *dev)
         event->devnum = udev_device_get_devnum(dev);
         event->is_block = streq("block", udev_device_get_subsystem(dev));
         event->ifindex = udev_device_get_ifindex(dev);
-#ifdef HAVE_FIRMWARE
         if (streq(udev_device_get_subsystem(dev), "firmware"))
                 event->nodelay = true;
-#endif
 
         udev_queue_export_device_queued(udev_queue_export, dev);
         log_debug("seq %llu queued, '%s' '%s'\n", udev_device_get_seqnum(dev),
@@ -527,11 +523,9 @@ static bool is_devpath_busy(struct event *event)
                         return true;
                 }
 
-#ifdef HAVE_FIRMWARE
                 /* allow to bypass the dependency tracking */
                 if (event->nodelay)
                         continue;
-#endif
 
                 /* parent device event found */
                 if (event->devpath[common] == '/') {

Re: [PATCH] udev: fail firmware loading immediately if no search path is defined

[Andy Lutomirski]

On Wed, Aug 7, 2013 at 12:52 AM, Maarten Lankhorst
<m.b.lankhorst@gmail.com> wrote:
> Op 07-08-13 02:26, Andy Lutomirski schreef:
>> On Tue, Aug 6, 2013 at 5:24 PM, Tom Gundersen <teg@jklm.no> wrote:
>>> On 6 Aug 2013 18:32, "Bryan Kadzban" <bryan@kadzban.is-a-geek.net> wrote:
>>>> On Tue, Aug 06, 2013 at 11:17:17AM +0200, Tom Gundersen wrote:
>>>>> On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <teg@jklm.no> wrote:
>>>>>> On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
>>>>>> <m.b.lankhorst@gmail.com> wrote:
>>>>>>> Op 05-08-13 18:29, Andy Lutomirski schreef:
>>>>>>>> The systemd commit below can delay firmware loading by multiple
>>>>>>>> minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
>>>>>>>> noticed that the systemd-udev change would break new kernels as well
>>>>>>>> as old kernels.
>>>>>>>>
>>>>>>>> Since the kernel apparently can't count on reasonable userspace
>>>>>>>> support, turn this thing off by default.
>>>>>>>>
>>>>>>>> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
>>>>>>>> Author: Tom Gundersen <teg@jklm.no>
>>>>>>>> Date:   Mon Mar 18 15:12:18 2013 +0100
>>>>>>>>
>>>>>>>>     udev: make firmware loading optional and disable by default
>>>>>>>>
>>>>>>>>     Distros that whish to support old kernels should set
>>>>>>>>
>>>>>>>> --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
>>>>>>>>     to retain the old behaviour.
>>>>>>>>
>>>>>>> methinks this patch should be reverted then,
>>>>>> Well, all the code is still there, so it can be enabled if anyone
>>>>>> wants it.
>>>>>>
>>>>>>> or a stub should be added to udev to always fail firmware loading so
>>>>>>> timeouts don't occur.
>>>>>> I think the only use (if any) of a userspace firmware loader would be
>>>>>> for anyone who wants a custom one (i.e., not udev), so we shouldn't
>>>>>> just fail the loading from udev unconditionally.
>>>>>>
>>>>>> How about we just improve the udev documentation a bit, similar to
>>>>>> Andy's kernel patch?
>>>>> Sorry, I should first have checked. We already document this in the
>>>>> README:
>>>>>
>>>>>>        Userspace firmware loading is deprecated, will go away, and
>>>>>>        sometimes causes problems:
>>>>>>          CONFIG_FW_LOADER_USER_HELPER=n
>>>> ...And this patch is making the kernel default to the correct behavior,
>>>> instead of the now-broken-by-udev behavior.
>>>>
>>>> I'm not sure I see the issue with it?  :-)
>>> Oh yeah this patch is totally the right thing to do, I was just arguing that
>>> there is nothing to be done on the udev side.
>>>
>>>> (Add me to the list of people that think udev is broken too, fwiw.  But
>>>> let's at least not leave *both* sides in a broken-by-default state.)
>>> Well I don't think it is too much to ask that the kernel and udev should be
>>> configured in a consistent way. Especially as thing still work even if you
>>> get it wrong, albeit with a delay.
>> Except that the current defaults are inconsistent and there is no
>> explanation anywhere in the logs when this is screwed up.
>>
> So what is wrong with my 'fail in udev immediately if not configured' idea? In that case it
> doesn't matter whether CONFIG_FW_LOADER_USER_HELPER is set or not.
>
> You could even print a useful message for the user in udev to the log, so they have an idea of what
> happened. Breaking udev on older still supported kernels by default without printing any debug info
> is silly, and the only cost is a small increase in disk space when unused. I did so in below patch.

Seems reasonable to me.  It might be worth adding something to the
message to suggest turning off CONFIG_FW_LOADER_USER_HELPER.

>
> ~Maarten
>
> I converted < ELEMENTSOF to != ELEMENTSOF in the loop to work correctly when the array is empty.
> Most code in udev-builtin-firmware is eliminated at -O2 optimization level when FIRMWARE_PATH
> is not explicitly set.
>
> 8<----
> diff --git a/Makefile.am b/Makefile.am
> index b8b8d06..2097629 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -2235,6 +2235,7 @@ libudev_core_la_SOURCES = \
>         src/udev/udev-ctrl.c \
>         src/udev/udev-builtin.c \
>         src/udev/udev-builtin-btrfs.c \
> +       src/udev/udev-builtin-firmware.c \
>         src/udev/udev-builtin-hwdb.c \
>         src/udev/udev-builtin-input_id.c \
>         src/udev/udev-builtin-keyboard.c \
> @@ -2271,14 +2272,6 @@ libudev_core_la_CPPFLAGS = \
>         $(AM_CPPFLAGS) \
>         -DFIRMWARE_PATH="$(FIRMWARE_PATH)"
>
> -if ENABLE_FIRMWARE
> -libudev_core_la_SOURCES += \
> -       src/udev/udev-builtin-firmware.c
> -
> -dist_udevrules_DATA += \
> -       rules/50-firmware.rules
> -endif
> -
>  if HAVE_KMOD
>  libudev_core_la_SOURCES += \
>         src/udev/udev-builtin-kmod.c
> diff --git a/configure.ac b/configure.ac
> index 0ecc716..dc7a3e3 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -823,8 +823,6 @@ for i in $with_firmware_path; do
>  done
>  IFS=$OLD_IFS
>  AC_SUBST(FIRMWARE_PATH)
> -AS_IF([test "x${FIRMWARE_PATH}" != "x"], [ AC_DEFINE(HAVE_FIRMWARE, 1, [Define if FIRMWARE is available]) ])
> -AM_CONDITIONAL(ENABLE_FIRMWARE, [test "x${FIRMWARE_PATH}" != "x"])
>
>  # ------------------------------------------------------------------------------
>  AC_ARG_ENABLE([gudev],
> diff --git a/rules/50-firmware.rules b/rules/50-firmware.rules
> deleted file mode 100644
> index f0ae684..0000000
> --- a/rules/50-firmware.rules
> +++ /dev/null
> @@ -1,3 +0,0 @@
> -# do not edit this file, it will be overwritten on update
> -
> -SUBSYSTEM="firmware", ACTION="add", RUN{builtin}="firmware"
> diff --git a/rules/50-udev-default.rules b/rules/50-udev-default.rules
> index f764789..645830e 100644
> --- a/rules/50-udev-default.rules
> +++ b/rules/50-udev-default.rules
> @@ -8,6 +8,7 @@ SUBSYSTEM="rtc", KERNEL="rtc0", SYMLINK+="rtc", OPTIONS+="link_priority=-100"
>
>  SUBSYSTEM="usb", ENV{DEVTYPE}="usb_device", IMPORT{builtin}="usb_id", IMPORT{builtin}="hwdb --subsystem=usb"
>  SUBSYSTEM="input", ENV{ID_INPUT}="", IMPORT{builtin}="input_id"
> +SUBSYSTEM="firmware", ACTION="add", IMPORT{builtin}="firmware"
>  ENV{MODALIAS}!="", IMPORT{builtin}="hwdb --subsystem=$env{SUBSYSTEM}"
>
>  ACTION!="add", GOTO="default_permissions_end"
> diff --git a/shell-completion/bash/udevadm b/shell-completion/bash/udevadm
> index 8ad8550..c22a3e2 100644
> --- a/shell-completion/bash/udevadm
> +++ b/shell-completion/bash/udevadm
> @@ -83,7 +83,7 @@ _udevadm() {
>                          fi
>                          ;;
>                  'test-builtin')
> -                        comps='blkid btrfs hwdb input_id keyboard kmod net_id path_id usb_id uaccess'
> +                        comps='blkid btrfs firmware hwdb input_id keyboard kmod net_id path_id usb_id uaccess'
>                          ;;
>                  *)
>                          comps=${VERBS[*]}
> diff --git a/src/udev/udev-builtin-firmware.c b/src/udev/udev-builtin-firmware.c
> index b80940b..e678545 100644
> --- a/src/udev/udev-builtin-firmware.c
> +++ b/src/udev/udev-builtin-firmware.c
> @@ -97,7 +97,7 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo
>
>          /* lookup firmware file */
>          uname(&kernel);
> -        for (i = 0; i < ELEMENTSOF(searchpath); i++) {
> +        for (i = 0; i != ELEMENTSOF(searchpath); i++) {
>                  strscpyl(fwpath, sizeof(fwpath), searchpath[i], kernel.release, "/", firmware, NULL);
>                  fwfile = fopen(fwpath, "re");
>                  if (fwfile != NULL)
> @@ -112,7 +112,10 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo
>          strscpyl(loadpath, sizeof(loadpath), udev_device_get_syspath(dev), "/loading", NULL);
>
>          if (fwfile = NULL) {
> -                log_debug("did not find firmware file '%s'\n", firmware);
> +                if (ELEMENTSOF(searchpath))
> +                    log_debug("did not find firmware file '%s'\n", firmware);
> +                else
> +                    log_error("could not load firmware file '%s', firmware loading not enabled\n", firmware);
>                  rc = EXIT_FAILURE;
>                  /*
>                   * Do not cancel the request in the initrd, the real root might have
> diff --git a/src/udev/udev-builtin.c b/src/udev/udev-builtin.c
> index 6b3a518..5a5cb30 100644
> --- a/src/udev/udev-builtin.c
> +++ b/src/udev/udev-builtin.c
> @@ -34,9 +34,7 @@ static const struct udev_builtin *builtins[] = {
>          [UDEV_BUILTIN_BLKID] = &udev_builtin_blkid,
>  #endif
>          [UDEV_BUILTIN_BTRFS] = &udev_builtin_btrfs,
> -#ifdef HAVE_FIRMWARE
>          [UDEV_BUILTIN_FIRMWARE] = &udev_builtin_firmware,
> -#endif
>          [UDEV_BUILTIN_HWDB] = &udev_builtin_hwdb,
>          [UDEV_BUILTIN_INPUT_ID] = &udev_builtin_input_id,
>          [UDEV_BUILTIN_KEYBOARD] = &udev_builtin_keyboard,
> diff --git a/src/udev/udev.h b/src/udev/udev.h
> index 8395926..31fa606 100644
> --- a/src/udev/udev.h
> +++ b/src/udev/udev.h
> @@ -140,9 +140,7 @@ enum udev_builtin_cmd {
>          UDEV_BUILTIN_BLKID,
>  #endif
>          UDEV_BUILTIN_BTRFS,
> -#ifdef HAVE_FIRMWARE
>          UDEV_BUILTIN_FIRMWARE,
> -#endif
>          UDEV_BUILTIN_HWDB,
>          UDEV_BUILTIN_INPUT_ID,
>          UDEV_BUILTIN_KEYBOARD,
> @@ -170,9 +168,7 @@ struct udev_builtin {
>  extern const struct udev_builtin udev_builtin_blkid;
>  #endif
>  extern const struct udev_builtin udev_builtin_btrfs;
> -#ifdef HAVE_FIRMWARE
>  extern const struct udev_builtin udev_builtin_firmware;
> -#endif
>  extern const struct udev_builtin udev_builtin_hwdb;
>  extern const struct udev_builtin udev_builtin_input_id;
>  extern const struct udev_builtin udev_builtin_keyboard;
> diff --git a/src/udev/udevd.c b/src/udev/udevd.c
> index 45ec3d6..e27a33a 100644
> --- a/src/udev/udevd.c
> +++ b/src/udev/udevd.c
> @@ -98,9 +98,7 @@ struct event {
>          dev_t devnum;
>          int ifindex;
>          bool is_block;
> -#ifdef HAVE_FIRMWARE
>          bool nodelay;
> -#endif
>  };
>
>  static inline struct event *node_to_event(struct udev_list_node *node)
> @@ -444,10 +442,8 @@ static int event_queue_insert(struct udev_device *dev)
>          event->devnum = udev_device_get_devnum(dev);
>          event->is_block = streq("block", udev_device_get_subsystem(dev));
>          event->ifindex = udev_device_get_ifindex(dev);
> -#ifdef HAVE_FIRMWARE
>          if (streq(udev_device_get_subsystem(dev), "firmware"))
>                  event->nodelay = true;
> -#endif
>
>          udev_queue_export_device_queued(udev_queue_export, dev);
>          log_debug("seq %llu queued, '%s' '%s'\n", udev_device_get_seqnum(dev),
> @@ -527,11 +523,9 @@ static bool is_devpath_busy(struct event *event)
>                          return true;
>                  }
>
> -#ifdef HAVE_FIRMWARE
>                  /* allow to bypass the dependency tracking */
>                  if (event->nodelay)
>                          continue;
> -#endif
>
>                  /* parent device event found */
>                  if (event->devpath[common] = '/') {
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH] udev: fail firmware loading immediately if no search path is defined

[Andy Lutomirski]

On Wed, Aug 7, 2013 at 12:52 AM, Maarten Lankhorst
<m.b.lankhorst@gmail.com> wrote:
> Op 07-08-13 02:26, Andy Lutomirski schreef:
>> On Tue, Aug 6, 2013 at 5:24 PM, Tom Gundersen <teg@jklm.no> wrote:
>>> On 6 Aug 2013 18:32, "Bryan Kadzban" <bryan@kadzban.is-a-geek.net> wrote:
>>>> On Tue, Aug 06, 2013 at 11:17:17AM +0200, Tom Gundersen wrote:
>>>>> On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <teg@jklm.no> wrote:
>>>>>> On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
>>>>>> <m.b.lankhorst@gmail.com> wrote:
>>>>>>> Op 05-08-13 18:29, Andy Lutomirski schreef:
>>>>>>>> The systemd commit below can delay firmware loading by multiple
>>>>>>>> minutes if CONFIG_FW_LOADER_USER_HELPER=y.  Unfortunately no one
>>>>>>>> noticed that the systemd-udev change would break new kernels as well
>>>>>>>> as old kernels.
>>>>>>>>
>>>>>>>> Since the kernel apparently can't count on reasonable userspace
>>>>>>>> support, turn this thing off by default.
>>>>>>>>
>>>>>>>> commit a3bd8447be4ea2ce230eb8ae0e815c04d85fa15a
>>>>>>>> Author: Tom Gundersen <teg@jklm.no>
>>>>>>>> Date:   Mon Mar 18 15:12:18 2013 +0100
>>>>>>>>
>>>>>>>>     udev: make firmware loading optional and disable by default
>>>>>>>>
>>>>>>>>     Distros that whish to support old kernels should set
>>>>>>>>
>>>>>>>> --with-firmware-dirs="/usr/lib/firmware/updates:/usr/lib/firmware"
>>>>>>>>     to retain the old behaviour.
>>>>>>>>
>>>>>>> methinks this patch should be reverted then,
>>>>>> Well, all the code is still there, so it can be enabled if anyone
>>>>>> wants it.
>>>>>>
>>>>>>> or a stub should be added to udev to always fail firmware loading so
>>>>>>> timeouts don't occur.
>>>>>> I think the only use (if any) of a userspace firmware loader would be
>>>>>> for anyone who wants a custom one (i.e., not udev), so we shouldn't
>>>>>> just fail the loading from udev unconditionally.
>>>>>>
>>>>>> How about we just improve the udev documentation a bit, similar to
>>>>>> Andy's kernel patch?
>>>>> Sorry, I should first have checked. We already document this in the
>>>>> README:
>>>>>
>>>>>>        Userspace firmware loading is deprecated, will go away, and
>>>>>>        sometimes causes problems:
>>>>>>          CONFIG_FW_LOADER_USER_HELPER=n
>>>> ...And this patch is making the kernel default to the correct behavior,
>>>> instead of the now-broken-by-udev behavior.
>>>>
>>>> I'm not sure I see the issue with it?  :-)
>>> Oh yeah this patch is totally the right thing to do, I was just arguing that
>>> there is nothing to be done on the udev side.
>>>
>>>> (Add me to the list of people that think udev is broken too, fwiw.  But
>>>> let's at least not leave *both* sides in a broken-by-default state.)
>>> Well I don't think it is too much to ask that the kernel and udev should be
>>> configured in a consistent way. Especially as thing still work even if you
>>> get it wrong, albeit with a delay.
>> Except that the current defaults are inconsistent and there is no
>> explanation anywhere in the logs when this is screwed up.
>>
> So what is wrong with my 'fail in udev immediately if not configured' idea? In that case it
> doesn't matter whether CONFIG_FW_LOADER_USER_HELPER is set or not.
>
> You could even print a useful message for the user in udev to the log, so they have an idea of what
> happened. Breaking udev on older still supported kernels by default without printing any debug info
> is silly, and the only cost is a small increase in disk space when unused. I did so in below patch.

Seems reasonable to me.  It might be worth adding something to the
message to suggest turning off CONFIG_FW_LOADER_USER_HELPER.

>
> ~Maarten
>
> I converted < ELEMENTSOF to != ELEMENTSOF in the loop to work correctly when the array is empty.
> Most code in udev-builtin-firmware is eliminated at -O2 optimization level when FIRMWARE_PATH
> is not explicitly set.
>
> 8<----
> diff --git a/Makefile.am b/Makefile.am
> index b8b8d06..2097629 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -2235,6 +2235,7 @@ libudev_core_la_SOURCES = \
>         src/udev/udev-ctrl.c \
>         src/udev/udev-builtin.c \
>         src/udev/udev-builtin-btrfs.c \
> +       src/udev/udev-builtin-firmware.c \
>         src/udev/udev-builtin-hwdb.c \
>         src/udev/udev-builtin-input_id.c \
>         src/udev/udev-builtin-keyboard.c \
> @@ -2271,14 +2272,6 @@ libudev_core_la_CPPFLAGS = \
>         $(AM_CPPFLAGS) \
>         -DFIRMWARE_PATH="$(FIRMWARE_PATH)"
>
> -if ENABLE_FIRMWARE
> -libudev_core_la_SOURCES += \
> -       src/udev/udev-builtin-firmware.c
> -
> -dist_udevrules_DATA += \
> -       rules/50-firmware.rules
> -endif
> -
>  if HAVE_KMOD
>  libudev_core_la_SOURCES += \
>         src/udev/udev-builtin-kmod.c
> diff --git a/configure.ac b/configure.ac
> index 0ecc716..dc7a3e3 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -823,8 +823,6 @@ for i in $with_firmware_path; do
>  done
>  IFS=$OLD_IFS
>  AC_SUBST(FIRMWARE_PATH)
> -AS_IF([test "x${FIRMWARE_PATH}" != "x"], [ AC_DEFINE(HAVE_FIRMWARE, 1, [Define if FIRMWARE is available]) ])
> -AM_CONDITIONAL(ENABLE_FIRMWARE, [test "x${FIRMWARE_PATH}" != "x"])
>
>  # ------------------------------------------------------------------------------
>  AC_ARG_ENABLE([gudev],
> diff --git a/rules/50-firmware.rules b/rules/50-firmware.rules
> deleted file mode 100644
> index f0ae684..0000000
> --- a/rules/50-firmware.rules
> +++ /dev/null
> @@ -1,3 +0,0 @@
> -# do not edit this file, it will be overwritten on update
> -
> -SUBSYSTEM=="firmware", ACTION=="add", RUN{builtin}="firmware"
> diff --git a/rules/50-udev-default.rules b/rules/50-udev-default.rules
> index f764789..645830e 100644
> --- a/rules/50-udev-default.rules
> +++ b/rules/50-udev-default.rules
> @@ -8,6 +8,7 @@ SUBSYSTEM=="rtc", KERNEL=="rtc0", SYMLINK+="rtc", OPTIONS+="link_priority=-100"
>
>  SUBSYSTEM=="usb", ENV{DEVTYPE}=="usb_device", IMPORT{builtin}="usb_id", IMPORT{builtin}="hwdb --subsystem=usb"
>  SUBSYSTEM=="input", ENV{ID_INPUT}=="", IMPORT{builtin}="input_id"
> +SUBSYSTEM=="firmware", ACTION=="add", IMPORT{builtin}="firmware"
>  ENV{MODALIAS}!="", IMPORT{builtin}="hwdb --subsystem=$env{SUBSYSTEM}"
>
>  ACTION!="add", GOTO="default_permissions_end"
> diff --git a/shell-completion/bash/udevadm b/shell-completion/bash/udevadm
> index 8ad8550..c22a3e2 100644
> --- a/shell-completion/bash/udevadm
> +++ b/shell-completion/bash/udevadm
> @@ -83,7 +83,7 @@ _udevadm() {
>                          fi
>                          ;;
>                  'test-builtin')
> -                        comps='blkid btrfs hwdb input_id keyboard kmod net_id path_id usb_id uaccess'
> +                        comps='blkid btrfs firmware hwdb input_id keyboard kmod net_id path_id usb_id uaccess'
>                          ;;
>                  *)
>                          comps=${VERBS[*]}
> diff --git a/src/udev/udev-builtin-firmware.c b/src/udev/udev-builtin-firmware.c
> index b80940b..e678545 100644
> --- a/src/udev/udev-builtin-firmware.c
> +++ b/src/udev/udev-builtin-firmware.c
> @@ -97,7 +97,7 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo
>
>          /* lookup firmware file */
>          uname(&kernel);
> -        for (i = 0; i < ELEMENTSOF(searchpath); i++) {
> +        for (i = 0; i != ELEMENTSOF(searchpath); i++) {
>                  strscpyl(fwpath, sizeof(fwpath), searchpath[i], kernel.release, "/", firmware, NULL);
>                  fwfile = fopen(fwpath, "re");
>                  if (fwfile != NULL)
> @@ -112,7 +112,10 @@ static int builtin_firmware(struct udev_device *dev, int argc, char *argv[], boo
>          strscpyl(loadpath, sizeof(loadpath), udev_device_get_syspath(dev), "/loading", NULL);
>
>          if (fwfile == NULL) {
> -                log_debug("did not find firmware file '%s'\n", firmware);
> +                if (ELEMENTSOF(searchpath))
> +                    log_debug("did not find firmware file '%s'\n", firmware);
> +                else
> +                    log_error("could not load firmware file '%s', firmware loading not enabled\n", firmware);
>                  rc = EXIT_FAILURE;
>                  /*
>                   * Do not cancel the request in the initrd, the real root might have
> diff --git a/src/udev/udev-builtin.c b/src/udev/udev-builtin.c
> index 6b3a518..5a5cb30 100644
> --- a/src/udev/udev-builtin.c
> +++ b/src/udev/udev-builtin.c
> @@ -34,9 +34,7 @@ static const struct udev_builtin *builtins[] = {
>          [UDEV_BUILTIN_BLKID] = &udev_builtin_blkid,
>  #endif
>          [UDEV_BUILTIN_BTRFS] = &udev_builtin_btrfs,
> -#ifdef HAVE_FIRMWARE
>          [UDEV_BUILTIN_FIRMWARE] = &udev_builtin_firmware,
> -#endif
>          [UDEV_BUILTIN_HWDB] = &udev_builtin_hwdb,
>          [UDEV_BUILTIN_INPUT_ID] = &udev_builtin_input_id,
>          [UDEV_BUILTIN_KEYBOARD] = &udev_builtin_keyboard,
> diff --git a/src/udev/udev.h b/src/udev/udev.h
> index 8395926..31fa606 100644
> --- a/src/udev/udev.h
> +++ b/src/udev/udev.h
> @@ -140,9 +140,7 @@ enum udev_builtin_cmd {
>          UDEV_BUILTIN_BLKID,
>  #endif
>          UDEV_BUILTIN_BTRFS,
> -#ifdef HAVE_FIRMWARE
>          UDEV_BUILTIN_FIRMWARE,
> -#endif
>          UDEV_BUILTIN_HWDB,
>          UDEV_BUILTIN_INPUT_ID,
>          UDEV_BUILTIN_KEYBOARD,
> @@ -170,9 +168,7 @@ struct udev_builtin {
>  extern const struct udev_builtin udev_builtin_blkid;
>  #endif
>  extern const struct udev_builtin udev_builtin_btrfs;
> -#ifdef HAVE_FIRMWARE
>  extern const struct udev_builtin udev_builtin_firmware;
> -#endif
>  extern const struct udev_builtin udev_builtin_hwdb;
>  extern const struct udev_builtin udev_builtin_input_id;
>  extern const struct udev_builtin udev_builtin_keyboard;
> diff --git a/src/udev/udevd.c b/src/udev/udevd.c
> index 45ec3d6..e27a33a 100644
> --- a/src/udev/udevd.c
> +++ b/src/udev/udevd.c
> @@ -98,9 +98,7 @@ struct event {
>          dev_t devnum;
>          int ifindex;
>          bool is_block;
> -#ifdef HAVE_FIRMWARE
>          bool nodelay;
> -#endif
>  };
>
>  static inline struct event *node_to_event(struct udev_list_node *node)
> @@ -444,10 +442,8 @@ static int event_queue_insert(struct udev_device *dev)
>          event->devnum = udev_device_get_devnum(dev);
>          event->is_block = streq("block", udev_device_get_subsystem(dev));
>          event->ifindex = udev_device_get_ifindex(dev);
> -#ifdef HAVE_FIRMWARE
>          if (streq(udev_device_get_subsystem(dev), "firmware"))
>                  event->nodelay = true;
> -#endif
>
>          udev_queue_export_device_queued(udev_queue_export, dev);
>          log_debug("seq %llu queued, '%s' '%s'\n", udev_device_get_seqnum(dev),
> @@ -527,11 +523,9 @@ static bool is_devpath_busy(struct event *event)
>                          return true;
>                  }
>
> -#ifdef HAVE_FIRMWARE
>                  /* allow to bypass the dependency tracking */
>                  if (event->nodelay)
>                          continue;
> -#endif
>
>                  /* parent device event found */
>                  if (event->devpath[common] == '/') {
>



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH] udev: fail firmware loading immediately if no search path is defined

[Kay Sievers]

On Sat, Aug 10, 2013 at 11:00 PM, Tom Gundersen <teg@jklm.no> wrote:
> It would be simple enough to add an udev rule to just print 'ignoring
> firmware event' to the logs.

This and I guess:
  SUBSYSTEM="firmware", ACTION="add", ATTR{loading}="-1"
would also just cancel the request at the same time without any other
code needed.

The udev firmware support just a configure option, just like the
kernel has them. So distributions should enable it in udev and the
kernel if they need it.

We simply cannot coordinate the defaults of systemd and the kernel
because the rules of the kernel are different. The kernel does "keep
defaults like stuff has been in the past" and udev does "make default
what makes the most sense on current systems".

> We should really ignore the event though, and
> not cancel it. Not sure if this is something we want upstream (after all,
> there are plenty of situations where we don't warn if the recommendations in
> the README file are not followed), or if distros and whoever wants it should
> ship that themselves. I'll leave that for Kay to decide.

The proper fix is that userspace firmware should be disabled in the
kernel for new systems, and kept enabled only for old systems. Old
systems need to enable a new udev version to support firmware loading.

There are currently broken in-kernel mis-users of the firmware
interface that use the firmware interface but disable uevents, they
still pull-in the user interface of the firmware loader. If nobody
wants to fix them, the code for the common users of the firmware
loader should at least get rid of the userspace fallback to call out
to userspace. At that point the udev configure option would not matter
any more.

> Lastly, note that the plan is to drop all the firmware code from udev in the
> not too distant future, so it doesn't really maker much sense to add new
> functionality to that at this point.

Right, I think all is fine. It's something that people can control
with the kernel and udev configuration options. It's just that the
defaults of the kernel and udev don't match at the moment, because
they have different policies of setting default values.

Kay

Re: [PATCH] udev: fail firmware loading immediately if no search path is defined

[Kay Sievers]

On Sat, Aug 10, 2013 at 11:00 PM, Tom Gundersen <teg@jklm.no> wrote:
> It would be simple enough to add an udev rule to just print 'ignoring
> firmware event' to the logs.

This and I guess:
  SUBSYSTEM=="firmware", ACTION=="add", ATTR{loading}="-1"
would also just cancel the request at the same time without any other
code needed.

The udev firmware support just a configure option, just like the
kernel has them. So distributions should enable it in udev and the
kernel if they need it.

We simply cannot coordinate the defaults of systemd and the kernel
because the rules of the kernel are different. The kernel does "keep
defaults like stuff has been in the past" and udev does "make default
what makes the most sense on current systems".

> We should really ignore the event though, and
> not cancel it. Not sure if this is something we want upstream (after all,
> there are plenty of situations where we don't warn if the recommendations in
> the README file are not followed), or if distros and whoever wants it should
> ship that themselves. I'll leave that for Kay to decide.

The proper fix is that userspace firmware should be disabled in the
kernel for new systems, and kept enabled only for old systems. Old
systems need to enable a new udev version to support firmware loading.

There are currently broken in-kernel mis-users of the firmware
interface that use the firmware interface but disable uevents, they
still pull-in the user interface of the firmware loader. If nobody
wants to fix them, the code for the common users of the firmware
loader should at least get rid of the userspace fallback to call out
to userspace. At that point the udev configure option would not matter
any more.

> Lastly, note that the plan is to drop all the firmware code from udev in the
> not too distant future, so it doesn't really maker much sense to add new
> functionality to that at this point.

Right, I think all is fine. It's something that people can control
with the kernel and udev configuration options. It's just that the
defaults of the kernel and udev don't match at the moment, because
they have different policies of setting default values.

Kay