request_firmware: skip timeout if userspace was not notified

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.

Javier

PS Please CC: me on replies, as I am not subscribed to the list.

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?

Quoted 5 lines

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)

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.

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

lib/kobject_uevent.c +5 -1

 		}
 	}
 #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 {

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

drivers/base/firmware_class.c +0 -1

@@ -420,8 +420,12 @@
 			add_timer(&fw_priv->timeout);
 		}
 
-		kobject_uevent(&f_dev->kobj, KOBJ_ADD);

Quoted 5 lines

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.

Quoted 4 lines

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.

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

Quoted 6 lines

Quoted 4 lines

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.

Quoted 7 lines · Javier Pello

Quoted 6 lines

Quoted 4 lines

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.

Quoted 9 lines

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.

Quoted 6 lines

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.

Quoted 4 lines

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

Quoted 6 lines

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

Quoted 7 lines · Kay Sievers

Quoted 6 lines

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

Quoted 6 lines

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.

Quoted 9 lines

Quoted 6 lines

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?

Quoted 6 lines · Kay Sievers

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.

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

Quoted 9 lines · Cornelia Huck

Quoted 6 lines · Kay Sievers

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.

Quoted 3 lines

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.

Quoted 4 lines

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.

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

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.

Quoted 2 lines · Kay Sievers

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

Quoted 4 lines

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.

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.

Quoted 5 lines · Kay Sievers

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?

Quoted 5 lines · Kay Sievers

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.

Quoted 2 lines · Kay Sievers

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

Quoted 5 lines

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.

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

Quoted 2 lines · Cornelia Huck

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.

Quoted 4 lines

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

Quoted 4 lines · Javier Pello

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.

Quoted 11 lines · Kay Sievers

Quoted 4 lines · Javier Pello

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.

Quoted 10 lines

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.

Quoted 2 lines

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.

Quoted 2 lines

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.

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

lib/kobject_uevent.c +7 -2

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