2013-08-02 16:05:08

by Andy Lutomirski

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

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

On Fri, Aug 2, 2013 at 12:38 AM, Johannes Berg
<[email protected]> 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


2013-08-02 16:21:36

by Johannes Berg

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

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

2013-08-02 16:25:02

by Andy Lutomirski

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

On Fri, Aug 2, 2013 at 9:21 AM, Johannes Berg <[email protected]> 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.

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

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

2013-08-05 11:18:39

by Kay Sievers

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

On Fri, Aug 2, 2013 at 6:28 PM, Zbigniew Jędrzejewski-Szmek
<[email protected]> 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

2013-08-05 16:10:16

by Andy Lutomirski

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

On Mon, Aug 5, 2013 at 4:18 AM, Kay Sievers <[email protected]> wrote:
> On Fri, Aug 2, 2013 at 6:28 PM, Zbigniew J?drzejewski-Szmek
> <[email protected]> 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

2013-08-05 16:29:24

by Andy Lutomirski

[permalink] [raw]
Subject: [PATCH] Change CONFIG_FW_LOADER_USER_HELPER to default n and don't select it

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 <[email protected]>
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

2013-08-06 08:20:20

by Maarten Lankhorst

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

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 <[email protected]>
> 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

2013-08-06 09:12:15

by Tom Gundersen

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

On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
<[email protected]> 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 <[email protected]>
>> 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

2013-08-06 09:17:41

by Tom Gundersen

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

On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <[email protected]> wrote:
> On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
> <[email protected]> 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 <[email protected]>
>>> 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

2013-08-06 16:08:52

by Andy Lutomirski

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

On Tue, Aug 6, 2013 at 2:17 AM, Tom Gundersen <[email protected]> wrote:
> On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <[email protected]> wrote:
>> On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
>> <[email protected]> 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 <[email protected]>
>>>> 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

2013-08-06 16:40:45

by Bryan Kadzban

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

On Tue, Aug 06, 2013 at 11:17:17AM +0200, Tom Gundersen wrote:
> On Tue, Aug 6, 2013 at 11:11 AM, Tom Gundersen <[email protected]> wrote:
> > On Tue, Aug 6, 2013 at 10:20 AM, Maarten Lankhorst
> > <[email protected]> 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 <[email protected]>
> >>> 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.)