LinuxLists.cc - [PATCH] request_firmware: skip timeout if userspace was not notified

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

> I've been told that it's possible to have the kernel pull the firmware
> off of an initrd (or was it initramfs, I keep confusing the two) without
> having any userspace, just put the right file in the right place
> (unfortunantly I've never gotten around to testing this) will this patch
> break this feature?

Correct me if I'm wrong, but I understand that the way this works is having
the initramfs become an "early userspace", a filesystem that is mounted,
well, earlier during boot, so that it is available before drivers begin
to initialise (earlier: as a rootfs_initcall, as opposed to at the end of
the boot process), but that in all respects behaves as a fully-fledged root
filesystem. In particular, firmware should be requested from userspace in
the standard way, and the patch should not break anything. I would test
this to be sure, but I have absolutely no experience preparing an initramfs.

Javier

2007-08-04 17:11:18

by David Lang

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Sat, 4 Aug 2007, Javier Pello wrote:

>> I've been told that it's possible to have the kernel pull the firmware
>> off of an initrd (or was it initramfs, I keep confusing the two) without
>> having any userspace, just put the right file in the right place
>> (unfortunantly I've never gotten around to testing this) will this patch
>> break this feature?
>
> Correct me if I'm wrong, but I understand that the way this works is having
> the initramfs become an "early userspace", a filesystem that is mounted,
> well, earlier during boot, so that it is available before drivers begin
> to initialise (earlier: as a rootfs_initcall, as opposed to at the end of
> the boot process), but that in all respects behaves as a fully-fledged root
> filesystem. In particular, firmware should be requested from userspace in
> the standard way, and the patch should not break anything. I would test
> this to be sure, but I have absolutely no experience preparing an initramfs.

what I've been told is that with the drive built-in instead of modular you
can create a filesystem that has only the firmware on it, nothing else,
and have the kernel find and load it (no userspace software involved)

David Lang

2007-08-04 21:20:44

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Sat, 4 Aug 2007, David Lang wrote:
> what I've been told is that with the drive built-in instead of
> modular you can create a filesystem that has only the firmware
> on it, nothing else, and have the kernel find and load it (no
> userspace software involved)

I'm afraid my understanding of the kernel is not very deep, so
I don't know of this method you're describing.

Anyway, if there's no userspace involved then nothing should
change. The patch I sent only affects the behaviour of the kernel
when it actually tries to send a firmware request to userspace
(and it determines that the request was not received). If the
kernel loads a firmware image without userspace support, it must
do so in a codepath different from request_firmware, whose point
is precisely to send a firmware request and wait for it, and
request_firmware is the only function that effectively changes
with my patch.

Javier

2007-08-06 12:24:30

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Fri, 03 Aug 2007 21:07:35 +0200,
Javier Pello <[email protected]> wrote:

> 1. The first part changes kobject_uevent_env in lib/kobject_uevent.c
> to report a failure if both netlink_broadcast (if applicable) and
> call_usermodehelper fail to send the event to userspace. Nothing in
> the kernel seems to care about the return value of kobject_uevent_env,
> so this should not break anything.

Famous last words ;) I recall that things broke when the driver core
tried to care about the return code of kobject_uevent(), so this may
work if you check just in your special case.

>
> 2. The second part changes _request_firmware in
> drivers/base/firmware_class.c to actually check the return value of
> kobject_uevent and skip the loading_timeout delay if the loading event
> was not delivered to userspace at all.

Note that kobject_uevent() returns 0 if the event has been filtered.

>
> The patches apply cleanly against 2.6.23-rc1. Any suggestions or feedback
> will be welcome.

It would be better if you sent the patches seperately.

>
> Javier
>
> PS Please CC: me on replies, as I am not subscribed to the list.
>
>
> ==========
> From: Javier Pello <[email protected]>
>
> kobject_uevent: return an error if event was not delivered to userspace
>
> Make kobject_uevent_env return an error to the caller if the event was
> not delivered to userspace either via netlink or via uevent_helper.
>
> Signed-off-by: Javier Pello <[email protected]>
>
> ---
> diff -u linux-2.6.22/lib/kobject_uevent.c linux-2.6.22-p/lib/kobject_uevent.c
> --- linux-2.6.22/lib/kobject_uevent.c 2007-07-09 01:32:17.000000000 +0200
> +++ linux-2.6.22-p/lib/kobject_uevent.c 2007-07-18 20:17:10.000000000 +0200
> @@ -186,7 +186,8 @@
> }
>
> NETLINK_CB(skb).dst_group = 1;
> - netlink_broadcast(uevent_sock, skb, 0, 1, GFP_KERNEL);
> + retval = netlink_broadcast(uevent_sock, skb, 0, 1,
> + GFP_KERNEL);

Maybe add a pr_debug() here for debugging?
> }
> }
> #endif
> @@ -198,7 +199,18 @@
> argv [0] = uevent_helper;
> argv [1] = (char *)subsystem;
> argv [2] = NULL;
> - call_usermodehelper (argv[0], argv, envp, UMH_WAIT_EXEC);
> +#if defined(CONFIG_NET)
> + if (retval) {
> + retval = call_usermodehelper (argv[0], argv, envp,
> + UMH_WAIT_EXEC);
> + } else {
> + call_usermodehelper (argv[0], argv, envp,
> + UMH_WAIT_EXEC);
> + }
> +#else
> + retval = call_usermodehelper (argv[0], argv, envp,
> + UMH_WAIT_EXEC);
> +#endif
> }
>
> exit:

Here a pr_debug() may also be nice.

>
> ==========
> From: Javier Pello <[email protected]>
>
> request_firmware: skip timeout if userspace was not notified
>
> Stop _request_firmware from setting up a timer and waiting for
> the firmware image to appear if kobject_uevent returns an error
> (meaning userspace was not notified at all).
>
> This prevents useless delays if there is no userspace tool to
> provide the firmware image (eg during boot).
>
> Signed-off-by: Javier Pello <[email protected]>
>
> ---
> diff -u linux-2.6.22/drivers/base/firmware_class.c linux-2.6.22-p/drivers/base/firmware_class.c
> --- linux-2.6.22/drivers/base/firmware_class.c 2007-07-09 01:32:17.000000000 +0200
> +++ linux-2.6.22-p/drivers/base/firmware_class.c 2007-07-18 20:06:53.000000000 +0200
> @@ -420,8 +420,12 @@
> add_timer(&fw_priv->timeout);
> }
>
> - kobject_uevent(&f_dev->kobj, KOBJ_ADD);
> - wait_for_completion(&fw_priv->completion);
> + retval = kobject_uevent(&f_dev->kobj, KOBJ_ADD);
> + if (retval) {
> + fw_load_abort(fw_priv);
> + } else {
> + wait_for_completion(&fw_priv->completion);
> + }
> set_bit(FW_STATUS_DONE, &fw_priv->status);
> del_timer_sync(&fw_priv->timeout);
> } else

Maybe add a small comment here?

2007-08-06 20:23:19

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Mon, 06 Aug 2007, Cornelia Huck wrote:
> > 1. The first part changes kobject_uevent_env in lib/kobject_uevent.c
> > to report a failure if both netlink_broadcast (if applicable) and
> > call_usermodehelper fail to send the event to userspace. Nothing
> > in the kernel seems to care about the return value of
> > kobject_uevent_env, so this should not break anything.
>
> Famous last words ;) I recall that things broke when the driver core
> tried to care about the return code of kobject_uevent(), so this may
> work if you check just in your special case.

I have hunted down all calls to kobject_uevent/kobject_uevent_env and
they are all statements (return value discarded). My intention is to
only add a check in request_firmware.

> > 2. The second part changes _request_firmware in
> > drivers/base/firmware_class.c to actually check the return value
> > of kobject_uevent and skip the loading_timeout delay if the
> > loading event was not delivered to userspace at all.
>
> Note that kobject_uevent() returns 0 if the event has been filtered.

Oops, yes, I had not noticed. This brings another point: When should
kobject_uevent return a "failure"? The code, as it is, only returns a
failure when things are really wrong (out of memory, etc.), and
returns success when the event was simply dropped. This is reasonable
behaviour, but it prevents callers from knowing whether the event was
actually delivered (which is what request_firmware needs). On the
other hand, my patch tries to make nondelivery an error, but on
second thoughts that could prevent the caller from telling hard
errors from simple nondelivery. I can think of two possibilities
to sort this out:

- kobject_uevent returns an error code both on a hard error and
on nondelivery; the error codes for both situations are different,
so the caller can tell them apart.

- kobject_uevent returns an error code (<0) only on a hard error,
returns 0 on nondelivery and 1 on delivery; this makes things
even clearer.

I am biased towards the latter. Of course, we can do anything as
the return value is actually never used, but I would still like
to know other opinions about what the right thing is.

> It would be better if you sent the patches seperately.
> [...]
> Maybe add a small comment here?

Thank you for your suggestions. I will prepare a new version of
the patch and send it as a followup to this message as soon as
I decide about the return value of kobject_uevent.

Javier

2007-08-07 10:57:49

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Mon, 06 Aug 2007 22:23:07 +0200,
Javier Pello <[email protected]> wrote:

> > > 2. The second part changes _request_firmware in
> > > drivers/base/firmware_class.c to actually check the return value
> > > of kobject_uevent and skip the loading_timeout delay if the
> > > loading event was not delivered to userspace at all.
> >
> > Note that kobject_uevent() returns 0 if the event has been filtered.
>
> Oops, yes, I had not noticed. This brings another point: When should
> kobject_uevent return a "failure"? The code, as it is, only returns a
> failure when things are really wrong (out of memory, etc.), and
> returns success when the event was simply dropped. This is reasonable
> behaviour, but it prevents callers from knowing whether the event was
> actually delivered (which is what request_firmware needs). On the
> other hand, my patch tries to make nondelivery an error, but on
> second thoughts that could prevent the caller from telling hard
> errors from simple nondelivery.

You could say that not delivering is caused by a hard error when trying
to deliver, though. OTOH, filtering an event is certainly not an error.

> I can think of two possibilities
> to sort this out:
>
> - kobject_uevent returns an error code both on a hard error and
> on nondelivery; the error codes for both situations are different,
> so the caller can tell them apart.

Tough. For example, both today's kobject_uevent() and
call_usermodehelper() may return -ENOMEM.

>
> - kobject_uevent returns an error code (<0) only on a hard error,
> returns 0 on nondelivery and 1 on delivery; this makes things
> even clearer.

Hm, I have an aversion against tri-state return values :( OTOH, a)
callers generally don't care and b) it is in line with how
kobject_uevent() is defined if !CONFIG_HOTPLUG.

- Use an extra parameter in which successful delivery can be indicated.
Make this
int kobject_uevent_env_check(struct kobject *kobject,
enum kobject_action action,
char *envp[], int delivered);
so existing callers that don't care don't have to be changed.

>
> I am biased towards the latter. Of course, we can do anything as
> the return value is actually never used, but I would still like
> to know other opinions about what the right thing is.

2007-08-07 11:47:06

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On 8/7/07, Cornelia Huck <[email protected]> wrote:
> On Mon, 06 Aug 2007 22:23:07 +0200,
> Javier Pello <[email protected]> wrote:
>
> > > > 2. The second part changes _request_firmware in
> > > > drivers/base/firmware_class.c to actually check the return value
> > > > of kobject_uevent and skip the loading_timeout delay if the
> > > > loading event was not delivered to userspace at all.
> > >
> > > Note that kobject_uevent() returns 0 if the event has been filtered.
> >
> > Oops, yes, I had not noticed. This brings another point: When should
> > kobject_uevent return a "failure"? The code, as it is, only returns a
> > failure when things are really wrong (out of memory, etc.), and
> > returns success when the event was simply dropped. This is reasonable
> > behaviour, but it prevents callers from knowing whether the event was
> > actually delivered (which is what request_firmware needs). On the
> > other hand, my patch tries to make nondelivery an error, but on
> > second thoughts that could prevent the caller from telling hard
> > errors from simple nondelivery.
>
> You could say that not delivering is caused by a hard error when trying
> to deliver, though. OTOH, filtering an event is certainly not an error.
>
> > I can think of two possibilities
> > to sort this out:
> >
> > - kobject_uevent returns an error code both on a hard error and
> > on nondelivery; the error codes for both situations are different,
> > so the caller can tell them apart.
>
> Tough. For example, both today's kobject_uevent() and
> call_usermodehelper() may return -ENOMEM.
>
> >
> > - kobject_uevent returns an error code (<0) only on a hard error,
> > returns 0 on nondelivery and 1 on delivery; this makes things
> > even clearer.
>
> Hm, I have an aversion against tri-state return values :( OTOH, a)
> callers generally don't care and b) it is in line with how
> kobject_uevent() is defined if !CONFIG_HOTPLUG.
>
> - Use an extra parameter in which successful delivery can be indicated.
> Make this
> int kobject_uevent_env_check(struct kobject *kobject,
> enum kobject_action action,
> char *envp[], int delivered);
> so existing callers that don't care don't have to be changed.

How do you check if events have been "handled"? None of the recent
distros uses /sbin/hotplug anymore. Netlink events are broadcasted,
but no failure in delivery doesn't mean anything like "handled", or
delivered to the right instance. Even if you check that the netlink
socket has listeners, that wouldn't be sufficient to tell that is is
handled.

Thanks,
Kay

2007-08-07 12:09:37

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Tue, 7 Aug 2007 13:46:55 +0200,
"Kay Sievers" <[email protected]> wrote:

> > - Use an extra parameter in which successful delivery can be indicated.
> > Make this
> > int kobject_uevent_env_check(struct kobject *kobject,
> > enum kobject_action action,
> > char *envp[], int delivered);
> > so existing callers that don't care don't have to be changed.
>
> How do you check if events have been "handled"? None of the recent
> distros uses /sbin/hotplug anymore. Netlink events are broadcasted,
> but no failure in delivery doesn't mean anything like "handled", or
> delivered to the right instance. Even if you check that the netlink
> socket has listeners, that wouldn't be sufficient to tell that is is
> handled.

You can't check if it's been handled, yes; but you can certainly check
if you delivered it. I guess Javier wants to exclude the cases where
userspace didn't have any chance to handle it.

Javier: Do you have an idea how common the case "we broadcasted, but
nobody listened so we got a timeout" is? If the usual reason for the
timeout is that firmware was requested before a listener showed up, I'm
not sure whether that check is worth it...

2007-08-07 12:31:46

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On 8/7/07, Cornelia Huck <[email protected]> wrote:
> On Tue, 7 Aug 2007 13:46:55 +0200,
> "Kay Sievers" <[email protected]> wrote:
>
> > > - Use an extra parameter in which successful delivery can be indicated.
> > > Make this
> > > int kobject_uevent_env_check(struct kobject *kobject,
> > > enum kobject_action action,
> > > char *envp[], int delivered);
> > > so existing callers that don't care don't have to be changed.
> >
> > How do you check if events have been "handled"? None of the recent
> > distros uses /sbin/hotplug anymore. Netlink events are broadcasted,
> > but no failure in delivery doesn't mean anything like "handled", or
> > delivered to the right instance. Even if you check that the netlink
> > socket has listeners, that wouldn't be sufficient to tell that is is
> > handled.
>
> You can't check if it's been handled, yes; but you can certainly check
> if you delivered it.

The broadcast returns a failure when nobody received the message?

Kay

2007-08-07 12:46:54

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Tue, 07 Aug 2007, Cornelia Huck wrote:

> On Tue, 7 Aug 2007 13:46:55 +0200,
> "Kay Sievers" <[email protected]> wrote:
> > How do you check if events have been "handled"? None of the recent
> > distros uses /sbin/hotplug anymore. Netlink events are broadcasted,
> > but no failure in delivery doesn't mean anything like "handled", or
> > delivered to the right instance. Even if you check that the netlink
> > socket has listeners, that wouldn't be sufficient to tell that is
> > handled.
>
> You can't check if it's been handled, yes; but you can certainly check
> if you delivered it. I guess Javier wants to exclude the cases where
> userspace didn't have any chance to handle it.

That's right. There's no way to know if userspace does handle the event
after reception, and that's why we have a timeout (so we don't wait
forever), but we can be sure that userspace won't handle an event that
wasn't received. We want to be conservative here: if there is any
chance that the event was received, we set the timeout and wait.

> Javier: Do you have an idea how common the case "we broadcasted, but
> nobody listened so we got a timeout" is? If the usual reason for the
> timeout is that firmware was requested before a listener showed up,
> I'm not sure whether that check is worth it...

I don't know how common this is in general, but in my box it happens
(or rather happened, until I patched it) once per boot. I've got a
device that requires some firmware during initialisation. I always
build the driver into the kernel (I don't like modules for things
that are permanently attached to the computer), and that means that
it tries to initialise before the rootfs is mounted (there's no
initramfs). Before the patch, the kernel would hang for a minute
in request_firmware during each boot, which was rather annoying,
and that is the reason why I decided to fix it.

Javier

2007-08-07 12:47:58

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Tue, 7 Aug 2007 14:31:34 +0200,
"Kay Sievers" <[email protected]> wrote:

> > > How do you check if events have been "handled"? None of the recent
> > > distros uses /sbin/hotplug anymore. Netlink events are broadcasted,
> > > but no failure in delivery doesn't mean anything like "handled", or
> > > delivered to the right instance. Even if you check that the netlink
> > > socket has listeners, that wouldn't be sufficient to tell that is is
> > > handled.
> >
> > You can't check if it's been handled, yes; but you can certainly check
> > if you delivered it.
>
> The broadcast returns a failure when nobody received the message?

To my understanding, netlink_broadcast() returns 0 if the message was
delivered to someone, -ENOBUFS in case of failure and -ESRCH if nobody
listened.

2007-08-07 12:55:33

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Tue, 2007-08-07 at 14:47 +0200, Javier Pello wrote:
> On Tue, 07 Aug 2007, Cornelia Huck wrote:
>
> > On Tue, 7 Aug 2007 13:46:55 +0200,
> > "Kay Sievers" <[email protected]> wrote:
> > > How do you check if events have been "handled"? None of the recent
> > > distros uses /sbin/hotplug anymore. Netlink events are broadcasted,
> > > but no failure in delivery doesn't mean anything like "handled", or
> > > delivered to the right instance. Even if you check that the netlink
> > > socket has listeners, that wouldn't be sufficient to tell that is
> > > handled.
> >
> > You can't check if it's been handled, yes; but you can certainly check
> > if you delivered it. I guess Javier wants to exclude the cases where
> > userspace didn't have any chance to handle it.
>
> That's right. There's no way to know if userspace does handle the event
> after reception, and that's why we have a timeout (so we don't wait
> forever), but we can be sure that userspace won't handle an event that
> wasn't received. We want to be conservative here: if there is any
> chance that the event was received, we set the timeout and wait.
>
> > Javier: Do you have an idea how common the case "we broadcasted, but
> > nobody listened so we got a timeout" is? If the usual reason for the
> > timeout is that firmware was requested before a listener showed up,
> > I'm not sure whether that check is worth it...
>
> I don't know how common this is in general, but in my box it happens
> (or rather happened, until I patched it) once per boot. I've got a
> device that requires some firmware during initialisation. I always
> build the driver into the kernel (I don't like modules for things
> that are permanently attached to the computer), and that means that
> it tries to initialise before the rootfs is mounted (there's no
> initramfs). Before the patch, the kernel would hang for a minute
> in request_firmware during each boot, which was rather annoying,
> and that is the reason why I decided to fix it.

If you don't have modules and the initial request fails, how do you load
the firmware later?

The real fix would be to change the driver not to block in the firmware
request and use async version of firmware loading. The whole firmware
class with its silly timeout is just a piece of crap, that needs to be
replaced.

Kay

2007-08-07 13:14:42

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Tue, 07 Aug 2007 14:57:52 +0200,
Kay Sievers <[email protected]> wrote:

> The real fix would be to change the driver not to block in the firmware
> request and use async version of firmware loading.

I guess some drivers want to fail the probe if they can't get the
firmware. This sounds like the "some parts of ->probe may take ages and
block everything else" problem again...

2007-08-07 13:59:38

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Tue, 07 Aug 2007, Kay Sievers wrote:

> If you don't have modules and the initial request fails, how do you
> load the firmware later?

I trigger a rebinding of the device to the driver in an init file:
# echo -n [device] >/sys/.../bind

> The real fix would be to change the driver not to block in the
> firmware request and use async version of firmware loading. The
> whole firmware class with its silly timeout is just a piece of
> crap, that needs to be replaced.

I don't think that would be a real fix. You've done away with the
kernel blocking, but the timeout is still there, only that in the
background, and my point is still true: it is useless to wait at
all if userspace didn't receive the event. Waiting asynchronously
is less annoying than waiting synchronously, but equally useless.

Javier

2007-08-07 14:05:31

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Tue, 2007-08-07 at 15:59 +0200, Javier Pello wrote:
> On Tue, 07 Aug 2007, Kay Sievers wrote:
>
> > If you don't have modules and the initial request fails, how do you
> > load the firmware later?
>
> I trigger a rebinding of the device to the driver in an init file:
> # echo -n [device] >/sys/.../bind
>
> > The real fix would be to change the driver not to block in the
> > firmware request and use async version of firmware loading. The
> > whole firmware class with its silly timeout is just a piece of
> > crap, that needs to be replaced.
>
> I don't think that would be a real fix. You've done away with the
> kernel blocking, but the timeout is still there, only that in the
> background, and my point is still true: it is useless to wait at
> all if userspace didn't receive the event. Waiting asynchronously
> is less annoying than waiting synchronously, but equally useless.

Nope, you would just fulfill in a completely generic way all outstanding
requests when you are ready. All requests are all nicely grouped and
visible in sysfs. There would be no need of coding your own device
specific rebind. No timeout is needed or wanted, all requests would stay
until userspace has handled them successfully or canceled them.

Kay

2007-08-07 14:25:44

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Tue, 07 Aug 2007 15:59:38 +0200,
Javier Pello <[email protected]> wrote:

> On Tue, 07 Aug 2007, Kay Sievers wrote:
>
> > If you don't have modules and the initial request fails, how do you
> > load the firmware later?
>
> I trigger a rebinding of the device to the driver in an init file:
> # echo -n [device] >/sys/.../bind

So it is indeed that this driver wants to fail its probe if it cannot
get the firmware. A possibilty to achieve a similar effect would be to
use request_firmware_nowait() and to call device_release_driver() from
the callback if no firmware is loaded. (This would imply a split of
that driver's probe function into two stages.)

>
> > The real fix would be to change the driver not to block in the
> > firmware request and use async version of firmware loading. The
> > whole firmware class with its silly timeout is just a piece of
> > crap, that needs to be replaced.
>
> I don't think that would be a real fix. You've done away with the
> kernel blocking, but the timeout is still there, only that in the
> background, and my point is still true: it is useless to wait at
> all if userspace didn't receive the event. Waiting asynchronously
> is less annoying than waiting synchronously, but equally useless.

Well, at least it doesn't block the whole probing process... although
this waiting is indeed useless.

2007-08-07 14:38:25

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Tue, 07 Aug 2007 16:08:25 +0200,
Kay Sievers <[email protected]> wrote:

> Nope, you would just fulfill in a completely generic way all outstanding
> requests when you are ready. All requests are all nicely grouped and
> visible in sysfs. There would be no need of coding your own device
> specific rebind. No timeout is needed or wanted, all requests would stay
> until userspace has handled them successfully or canceled them.

Hm, that would mean that no build-in driver may call
request_firmware(), only request_firmware_nowait() with no timeout.
Likely drivers that want to fail probe for no firmware will need to
split their probe functions.

2007-08-07 20:05:46

by Andrew Morton

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Fri, 03 Aug 2007 21:07:35 +0200
Javier Pello <[email protected]> wrote:

> Hi,
>
> I have prepared a patch that makes request_firmware skip the usual
> grace period that it gives firmware images to show up, if it determines
> that userspace was not notified at all.
>
> When request_firmware is called, it sends an event to userspace to
> ask for the firmware image that is needed, and then it installs a
> timeout so that it will not wait forever for the image. However,
> if userspace did not receive the event at all (no /sbin/hotplug,
> for instance), then having the kernel wait is pointless. This is
> particularly true during boot, when the wait is done synchronously
> and the whole kernel freezes for a minute.
>
> The attached patch fixes this by making _request_firmware check whether
> the firmware loading event was succesfully sent to userspace, and skip
> the timeout if it has not. The patch comes in two parts:
>
> 1. The first part changes kobject_uevent_env in lib/kobject_uevent.c
> to report a failure if both netlink_broadcast (if applicable) and
> call_usermodehelper fail to send the event to userspace. Nothing in
> the kernel seems to care about the return value of kobject_uevent_env,
> so this should not break anything.
>
> 2. The second part changes _request_firmware in
> drivers/base/firmware_class.c to actually check the return value of
> kobject_uevent and skip the loading_timeout delay if the loading event
> was not delivered to userspace at all.
>
> The patches apply cleanly against 2.6.23-rc1. Any suggestions or feedback
> will be welcome.
>
> ---
> diff -u linux-2.6.22/lib/kobject_uevent.c linux-2.6.22-p/lib/kobject_uevent.c
> --- linux-2.6.22/lib/kobject_uevent.c 2007-07-09 01:32:17.000000000 +0200
> +++ linux-2.6.22-p/lib/kobject_uevent.c 2007-07-18 20:17:10.000000000 +0200
> @@ -186,7 +186,8 @@
> }
>
> NETLINK_CB(skb).dst_group = 1;
> - netlink_broadcast(uevent_sock, skb, 0, 1, GFP_KERNEL);
> + retval = netlink_broadcast(uevent_sock, skb, 0, 1,
> + GFP_KERNEL);
> }
> }
> #endif
> @@ -198,7 +199,18 @@
> argv [0] = uevent_helper;
> argv [1] = (char *)subsystem;
> argv [2] = NULL;
> - call_usermodehelper (argv[0], argv, envp, UMH_WAIT_EXEC);
> +#if defined(CONFIG_NET)
> + if (retval) {
> + retval = call_usermodehelper (argv[0], argv, envp,
> + UMH_WAIT_EXEC);
> + } else {
> + call_usermodehelper (argv[0], argv, envp,
> + UMH_WAIT_EXEC);
> + }
> +#else
> + retval = call_usermodehelper (argv[0], argv, envp,
> + UMH_WAIT_EXEC);
> +#endif
> }
>
> exit:

If uevent_helper[0] is \0 then this function can return success, even
though it didn't deliver anything to userspace.

I suppose that's correct behaviour: the operator configured the system to
disable the hotplug callout. However in this scenario, the firmware laoder
will still needlessly implement the grace period.

It's probably not worth worrying about.

2007-08-09 09:13:21

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Tue, 07 Aug 2007, Kay Sievers wrote:
> Nope, you would just fulfill in a completely generic way all outstanding
> requests when you are ready. All requests are all nicely grouped and
> visible in sysfs. There would be no need of coding your own device
> specific rebind. No timeout is needed or wanted, all requests would stay
> until userspace has handled them successfully or canceled them.

If I'm not mistaken, as it is now, requests are not grouped in any way.
The only hint that a firmware loading request is in progress are a couple
of files in the device directory in sysfs. Should I run, at boot, through
all the device directories to check which firmware requests are pending?

Also, this could pose problems if two drivers can handle a device but
the first that gets to bind to it needs firmware and userspace doesn't
provide it or cancel the loading. The device will remain bound without
giving the other driver a chance to handle it.

Anyway, I think something must be done, either removing the timeout and
making all firmware requests asynchronous, or at least skipping the
delay when we're sure that it's useless.

Javier

2007-08-09 09:18:29

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Thu, 2007-08-09 at 11:13 +0200, Javier Pello wrote:
> On Tue, 07 Aug 2007, Kay Sievers wrote:
> > Nope, you would just fulfill in a completely generic way all outstanding
> > requests when you are ready. All requests are all nicely grouped and
> > visible in sysfs. There would be no need of coding your own device
> > specific rebind. No timeout is needed or wanted, all requests would stay
> > until userspace has handled them successfully or canceled them.
>
> If I'm not mistaken, as it is now, requests are not grouped in any way.
> The only hint that a firmware loading request is in progress are a couple
> of files in the device directory in sysfs. Should I run, at boot, through
> all the device directories to check which firmware requests are pending?

/sys/class/firmware/* contains all pending requests, no need to search
anywhere else.

Kay

2007-08-09 09:27:46

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Thu, 09 Aug 2007 11:13:12 +0200,
Javier Pello <[email protected]> wrote:

> On Tue, 07 Aug 2007, Kay Sievers wrote:
> > Nope, you would just fulfill in a completely generic way all outstanding
> > requests when you are ready. All requests are all nicely grouped and
> > visible in sysfs. There would be no need of coding your own device
> > specific rebind. No timeout is needed or wanted, all requests would stay
> > until userspace has handled them successfully or canceled them.
>
> If I'm not mistaken, as it is now, requests are not grouped in any way.
> The only hint that a firmware loading request is in progress are a couple
> of files in the device directory in sysfs. Should I run, at boot, through
> all the device directories to check which firmware requests are pending?

I think that is what Kay meant.

> Also, this could pose problems if two drivers can handle a device but
> the first that gets to bind to it needs firmware and userspace doesn't
> provide it or cancel the loading. The device will remain bound without
> giving the other driver a chance to handle it.

Yes, this relies on someone canceling the load. If the driver has a
two-stage probe for which the first stage kicks the async firmware
loading and the second stage triggers an unbind if the loading fails,
this will work. Otherwise you'll have to manually unbind on load
cancellation. You'll also have to bind to the other driver manually.

2007-08-09 09:36:00

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Tue, 07 Aug 2007, Cornelia Huck wrote:

> So it is indeed that this driver wants to fail its probe if it
> cannot get the firmware.

That's right. The driver unbinds itself from the device if it doesn't
get the firmware.

> A possibilty to achieve a similar effect would be to use
> request_firmware_nowait() and to call device_release_driver() from
> the callback if no firmware is loaded. (This would imply a split of
> that driver's probe function into two stages.)

The comments in the source code say that request_firmware_nowait()
is an "asynchronous version of request_firmware() for contexts where
it is not possible to sleep". So a driver's decision to call one of
them is not based on whether it wants to wait or not, but whether it
_can_ wait.

Of course, it can be decided that we never want to wait, but then
the best course of action would be to make request_firmware itself
behave as request_firmware_nowait (no need to change drivers).

Anyway, my point is that it is useless to have the kernel block for
a minute at boot waiting for something that cannot happen, and that
it should be avoided (even if my proposed solution is not the way
to go).

Javier

2007-08-09 11:55:34

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Thu, 2007-08-09 at 11:36 +0200, Javier Pello wrote:
> On Tue, 07 Aug 2007, Cornelia Huck wrote:
>
> > So it is indeed that this driver wants to fail its probe if it
> > cannot get the firmware.
>
> That's right. The driver unbinds itself from the device if it doesn't
> get the firmware.
>
> > A possibilty to achieve a similar effect would be to use
> > request_firmware_nowait() and to call device_release_driver() from
> > the callback if no firmware is loaded. (This would imply a split of
> > that driver's probe function into two stages.)
>
> The comments in the source code say that request_firmware_nowait()
> is an "asynchronous version of request_firmware() for contexts where
> it is not possible to sleep". So a driver's decision to call one of
> them is not based on whether it wants to wait or not, but whether it
> _can_ wait.
>
> Of course, it can be decided that we never want to wait, but then
> the best course of action would be to make request_firmware itself
> behave as request_firmware_nowait (no need to change drivers).
>
> Anyway, my point is that it is useless to have the kernel block for
> a minute at boot waiting for something that cannot happen, and that
> it should be avoided (even if my proposed solution is not the way
> to go).

That's true. And it sounds all reasonable from your point of view, and
the firmware loader needs fixing, and the silly blocking request needs
to be removed from the kernel, that's known for a very long time now,
but nobody did the work so far.

But in this specific case, it is more the combination of your options,
what causes this problem to appear. You don't have an initramfs, you
don't use modules, but you are linking a driver into the kernel image
which depends on a conceptually broken blocking userspace transaction to
initialize.
This combination of options just doesn't make sense. Either use
initramfs, or use a kernel module for the driver that needs userspace to
initialize, or patch the driver not to block in the request, or patch
the driver to optionally include the firmware in the driver.

You just picked a set of options that doesn't work nicely together. No
distro setup has this problem, that's probably why nobody really cared
and it didn't get fixed so far.

Kay

2007-08-10 21:25:27

[permalink] [raw]

Subject: Re: [PATCH] request_firmware: skip timeout if userspace was not notified

On Thu 09 Aug 2007, Kay Sievers wrote:

> On Thu, 2007-08-09 at 11:36 +0200, Javier Pello wrote:
>
> > Anyway, my point is that it is useless to have the kernel block for
> > a minute at boot waiting for something that cannot happen, and that
> > it should be avoided (even if my proposed solution is not the way
> > to go).
>
> That's true. And it sounds all reasonable from your point of view,
> and the firmware loader needs fixing, and the silly blocking request
> needs to be removed from the kernel, that's known for a very long
> time now, but nobody did the work so far.

If I see it correctly, your point is that the firmware loader is
totally broken and needs replacing. That's fine, and I won't say
otherwise. But it doesn't seem that such replacement is under way
and, in the meanwhile, we are stuck with what we have. I'm not
defending the current loader but, while we have it, we might as
well not have it freeze the whole kernel for a minute waiting
for something that won't happen.

> But in this specific case, it is more the combination of your
> options, what causes this problem to appear. You don't have an
> initramfs, you don't use modules, but you are linking a driver
> into the kernel image which depends on a conceptually broken
> blocking userspace transaction to initialize.
> This combination of options just doesn't make sense. Either
> use initramfs, or use a kernel module for the driver that needs
> userspace to initialize, or patch the driver not to block in
> the request, or patch the driver to optionally include the
> firmware in the driver.

Note that the problem is not getting the driver to work---I can
do that pretty easily. The problem is that there's a number of
drivers that, just because they require firmware, will hang the
kernel on boot if built in unless an initramfs is carefully
prepared. An allyesconfig kernel could freeze for 10 minutes
during boot just because it came across 10 devices requiring
firmware, even if you don't intend to use them.

> You just picked a set of options that doesn't work nicely
> together.

I agree. That's why I sent the patch, to make it work better.

> No distro setup has this problem, that's probably why nobody
> really cared and it didn't get fixed so far.

I agree again. But the fact that it didn't get fixed so far
doesn't mean that it can never get fixed, does it?

Also, note that I'm not proposing massive changes, or changes
that will break things for other people (not intentionally,
anyway), or that will add complexity and unmaintainability
to the kernel. They try to do a reasonable thing and are
small and to the point.

Javier

2007-08-11 13:26:19