firmware loading vs. initrd

firmware loading vs. initrd

[Johannes Berg]

Hi,

We recently converted a few wireless drivers to use
request_firmware_nowait() in order to be able to load firmware from
probe. We would like to continue with that so we can load the firmware
before registering with any other subsystems, as we had discussed at the
wireless summit last year.

However, in actually trying this today, I noticed that there's a problem
with this. I made my drivers all built in, and then I ended up with it
not working because the firmware agent that was in my initrd was telling
[1], and the kernel that the firmware could not be found.

I thought of modifying the firmware agent in the initrd to not tell the
kernel it doesn't have it, but that is problematic when using
request_firmware(), the kernel boot will be delayed until the timeout
(one minute).

This can be solved by adding an environment variable to the uevent that
tells userspace whether or not this is coming from request_firmware() or
request_firmware_nowait(). I will follow up with a patch doing that.

Additionally, however, we need to make a change like below to the
firmware agent in order to not reply to asynchronous firmware loads
during the initrd stage. I'm not sure how to actually check that we're
running in an initrd (is that possible?) nor did I actually verify that
the NOWAIT environment variable is set properly.

Thoughts? It seems like this would solve our problems nicely if we can
determine whether we're in the initrd or not.

johannes

--- /lib/udev/firmware.agent	2010-02-18 16:40:39.000000000 -0800
+++ firmware.agent	2010-03-11 12:00:40.000000000 -0800
@@ -21,6 +21,16 @@
     exit 0
 done
 
+if [ "$NOWAIT" = "1" ] && [ INITRD? ] ; then
+	# Leave the request open in case we are
+	# running from initrd -- we'll deal with
+	# it when we process things again after
+	# the root filesystem is mounted.
+
+	# exit 1 instead?
+	exit 0
+fi
+
 # the firmware was not found
 echo -1 > /sys/$DEVPATH/loading
 

[PATCH] firmware class: export nowait to userspace

[Johannes Berg]

When we use request_firmware_nowait(), userspace may
not want to answer negatively right away when for
example it is answering from an initrd only, but
with request_firmware() it has to in order to not
delay the kernel boot until the request times out.

This allows userspace to differentiate between the
two in order to be able to reply negatively to async
requests only when all filesystems have been mounted
and have been checked for the requested firmware file.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 drivers/base/firmware_class.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

--- wireless-testing.orig/drivers/base/firmware_class.c	2010-03-11 11:50:39.000000000 -0800
+++ wireless-testing/drivers/base/firmware_class.c	2010-03-11 11:55:15.000000000 -0800
@@ -50,6 +50,7 @@ struct firmware_priv {
 	int page_array_size;
 	const char *vdata;
 	struct timer_list timeout;
+	bool nowait;
 };
 
 #ifdef CONFIG_FW_LOADER
@@ -107,6 +108,8 @@ static int firmware_uevent(struct device
 		return -ENOMEM;
 	if (add_uevent_var(env, "TIMEOUT=%i", loading_timeout))
 		return -ENOMEM;
+	if (add_uevent_var(env, "NOWAIT=%d", fw_priv->nowait))
+		return -ENOMEM;
 
 	return 0;
 }
@@ -422,7 +425,7 @@ error_kfree:
 
 static int fw_setup_device(struct firmware *fw, struct device **dev_p,
 			   const char *fw_name, struct device *device,
-			   int uevent)
+			   int uevent, bool nowait)
 {
 	struct device *f_dev;
 	struct firmware_priv *fw_priv;
@@ -438,6 +441,8 @@ static int fw_setup_device(struct firmwa
 
 	fw_priv = dev_get_drvdata(f_dev);
 
+	fw_priv->nowait = nowait;
+
 	fw_priv->fw = fw;
 	retval = sysfs_create_bin_file(&f_dev->kobj, &fw_priv->attr_data);
 	if (retval) {
@@ -464,7 +469,7 @@ out:
 
 static int
 _request_firmware(const struct firmware **firmware_p, const char *name,
-		 struct device *device, int uevent)
+		 struct device *device, int uevent, bool nowait)
 {
 	struct device *f_dev;
 	struct firmware_priv *fw_priv;
@@ -497,7 +502,8 @@ _request_firmware(const struct firmware
 	if (uevent)
 		dev_info(device, "firmware: requesting %s\n", name);
 
-	retval = fw_setup_device(firmware, &f_dev, name, device, uevent);
+	retval = fw_setup_device(firmware, &f_dev, name, device,
+				 uevent, nowait);
 	if (retval)
 		goto error_kfree_fw;
 
@@ -554,7 +560,7 @@ request_firmware(const struct firmware *
                  struct device *device)
 {
         int uevent = 1;
-        return _request_firmware(firmware_p, name, device, uevent);
+        return _request_firmware(firmware_p, name, device, uevent, false);
 }
 
 /**
@@ -600,7 +606,7 @@ request_firmware_work_func(void *arg)
 		return 0;
 	}
 	ret = _request_firmware(&fw, fw_work->name, fw_work->device,
-		fw_work->uevent);
+		fw_work->uevent, true);
 
 	fw_work->cont(fw, fw_work->context);
 

Re: [PATCH] firmware class: export nowait to userspace

[Kay Sievers]

On Thu, Mar 11, 2010 at 23:56, Johannes Berg <johannes@sipsolutions.net> wrote:
> When we use request_firmware_nowait(), userspace may
> not want to answer negatively right away when for
> example it is answering from an initrd only, but
> with request_firmware() it has to in order to not
> delay the kernel boot until the request times out.
>
> This allows userspace to differentiate between the
> two in order to be able to reply negatively to async
> requests only when all filesystems have been mounted
> and have been checked for the requested firmware file.

The firmware_class already always exports a TIMEOUT= value, right? If
this is the case, it should be set to 0, I guess.

Sounds fine to have a flag like this, while "NOWAIT" is, I guess, a
pretty bad name from the event perspective. This name might make sense
for the called kernel function, but not so much for a firmware loader
instruction.

Thanks,
Kay

Re: [PATCH] firmware class: export nowait to userspace

[Johannes Berg]

On Fri, 2010-03-12 at 05:21 +0100, Kay Sievers wrote:
> On Thu, Mar 11, 2010 at 23:56, Johannes Berg <johannes@sipsolutions.net> wrote:
> > When we use request_firmware_nowait(), userspace may
> > not want to answer negatively right away when for
> > example it is answering from an initrd only, but
> > with request_firmware() it has to in order to not
> > delay the kernel boot until the request times out.
> >
> > This allows userspace to differentiate between the
> > two in order to be able to reply negatively to async
> > requests only when all filesystems have been mounted
> > and have been checked for the requested firmware file.
> 
> The firmware_class already always exports a TIMEOUT= value, right? If
> this is the case, it should be set to 0, I guess.

Yes and no. It is exported, but typically it will be 60 seconds. And
even in this case it makes sense to eventually time out when userspace
is not responding.

> Sounds fine to have a flag like this, while "NOWAIT" is, I guess, a
> pretty bad name from the event perspective. This name might make sense
> for the called kernel function, but not so much for a firmware loader
> instruction.

Yeah, I can agree with that. How about "ASYNC" or so? It needs to
distinguish between "I'm going to wait for this please respond" and "I'm
not really waiting, please let me now when you can".

johannes

Re: [PATCH] firmware class: export nowait to userspace

[Kay Sievers]

On Fri, Mar 12, 2010 at 05:46, Johannes Berg <johannes@sipsolutions.net> wrote:
> On Fri, 2010-03-12 at 05:21 +0100, Kay Sievers wrote:
>> The firmware_class already always exports a TIMEOUT= value, right? If
>> this is the case, it should be set to 0, I guess.
>
> Yes and no. It is exported, but typically it will be 60 seconds. And
> even in this case it makes sense to eventually time out when userspace
> is not responding.

Ok, I didn't expect the async version to time out. Sounds fine then.

>> Sounds fine to have a flag like this, while "NOWAIT" is, I guess, a
>> pretty bad name from the event perspective. This name might make sense
>> for the called kernel function, but not so much for a firmware loader
>> instruction.
>
> Yeah, I can agree with that. How about "ASYNC" or so? It needs to
> distinguish between "I'm going to wait for this please respond" and "I'm
> not really waiting, please let me now when you can".

Yeah, sounds fine to me.

Thanks,
Kay

[PATCH] firmware class: export nowait to userspace

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

When we use request_firmware_nowait(), userspace may
not want to answer negatively right away when for
example it is answering from an initrd only, but
with request_firmware() it has to in order to not
delay the kernel boot until the request times out.

This allows userspace to differentiate between the
two in order to be able to reply negatively to async
requests only when all filesystems have been mounted
and have been checked for the requested firmware file.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
Oops, changed the patch but forgot to resend.

 drivers/base/firmware_class.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

--- wireless-testing.orig/drivers/base/firmware_class.c	2010-03-23 06:14:21.000000000 +0100
+++ wireless-testing/drivers/base/firmware_class.c	2010-03-29 17:56:14.000000000 +0200
@@ -49,6 +49,7 @@ struct firmware_priv {
 	int page_array_size;
 	const char *vdata;
 	struct timer_list timeout;
+	bool nowait;
 };
 
 #ifdef CONFIG_FW_LOADER
@@ -111,6 +112,8 @@ static int firmware_uevent(struct device
 		return -ENOMEM;
 	if (add_uevent_var(env, "TIMEOUT=%i", loading_timeout))
 		return -ENOMEM;
+	if (add_uevent_var(env, "ASYNC=%d", fw_priv->nowait))
+		return -ENOMEM;
 
 	return 0;
 }
@@ -426,7 +429,7 @@ error_kfree:
 
 static int fw_setup_device(struct firmware *fw, struct device **dev_p,
 			   const char *fw_name, struct device *device,
-			   int uevent)
+			   int uevent, bool nowait)
 {
 	struct device *f_dev;
 	struct firmware_priv *fw_priv;
@@ -442,6 +445,8 @@ static int fw_setup_device(struct firmwa
 
 	fw_priv = dev_get_drvdata(f_dev);
 
+	fw_priv->nowait = nowait;
+
 	fw_priv->fw = fw;
 	sysfs_bin_attr_init(&fw_priv->attr_data);
 	retval = sysfs_create_bin_file(&f_dev->kobj, &fw_priv->attr_data);
@@ -469,7 +474,7 @@ out:
 
 static int
 _request_firmware(const struct firmware **firmware_p, const char *name,
-		 struct device *device, int uevent)
+		 struct device *device, int uevent, bool nowait)
 {
 	struct device *f_dev;
 	struct firmware_priv *fw_priv;
@@ -502,7 +507,8 @@ _request_firmware(const struct firmware
 	if (uevent)
 		dev_info(device, "firmware: requesting %s\n", name);
 
-	retval = fw_setup_device(firmware, &f_dev, name, device, uevent);
+	retval = fw_setup_device(firmware, &f_dev, name, device,
+				 uevent, nowait);
 	if (retval)
 		goto error_kfree_fw;
 
@@ -559,7 +565,7 @@ request_firmware(const struct firmware *
                  struct device *device)
 {
         int uevent = 1;
-        return _request_firmware(firmware_p, name, device, uevent);
+        return _request_firmware(firmware_p, name, device, uevent, false);
 }
 
 /**
@@ -605,7 +611,7 @@ request_firmware_work_func(void *arg)
 		return 0;
 	}
 	ret = _request_firmware(&fw, fw_work->name, fw_work->device,
-		fw_work->uevent);
+		fw_work->uevent, true);
 
 	fw_work->cont(fw, fw_work->context);
 

Re: firmware loading vs. initrd

[Kay Sievers]

On Thu, Mar 11, 2010 at 23:39, Johannes Berg <johannes@sipsolutions.net> wrote:
> We recently converted a few wireless drivers to use
> request_firmware_nowait() in order to be able to load firmware from
> probe. We would like to continue with that so we can load the firmware
> before registering with any other subsystems, as we had discussed at the
> wireless summit last year.
>
> However, in actually trying this today, I noticed that there's a problem
> with this. I made my drivers all built in, and then I ended up with it
> not working because the firmware agent that was in my initrd was telling
> [1], and the kernel that the firmware could not be found.
>
> I thought of modifying the firmware agent in the initrd to not tell the
> kernel it doesn't have it, but that is problematic when using
> request_firmware(), the kernel boot will be delayed until the timeout
> (one minute).
>
> This can be solved by adding an environment variable to the uevent that
> tells userspace whether or not this is coming from request_firmware() or
> request_firmware_nowait(). I will follow up with a patch doing that.
>
> Additionally, however, we need to make a change like below to the
> firmware agent in order to not reply to asynchronous firmware loads
> during the initrd stage. I'm not sure how to actually check that we're
> running in an initrd (is that possible?) nor did I actually verify that
> the NOWAIT environment variable is set properly.
>
> Thoughts? It seems like this would solve our problems nicely if we can
> determine whether we're in the initrd or not.

I don't think we can reliably make the decision that the firmware is
not available at a later point.

Depending on the system, we can have several re-trigger of the same
events during bootup, and the firmware loader does not have any idea
in which state the system is.

What's wrong to leave the request staying around for forever, until it
is possibly canceled from userspace? So this event could be
re-triggered from userspace any time later. Whatever will cancel the
firmware requests in the end, it needs to be something that knows more
about the state of "booting" than the firmware loader knows today, I
guess.

Thanks,
Kay

Re: firmware loading vs. initrd

[Johannes Berg]

On Fri, 2010-03-12 at 05:32 +0100, Kay Sievers wrote:
> On Thu, Mar 11, 2010 at 23:39, Johannes Berg <johannes@sipsolutions.net> wrote:
> > We recently converted a few wireless drivers to use
> > request_firmware_nowait() in order to be able to load firmware from
> > probe. We would like to continue with that so we can load the firmware
> > before registering with any other subsystems, as we had discussed at the
> > wireless summit last year.
> >
> > However, in actually trying this today, I noticed that there's a problem
> > with this. I made my drivers all built in, and then I ended up with it
> > not working because the firmware agent that was in my initrd was telling
> > [1], and the kernel that the firmware could not be found.
> >
> > I thought of modifying the firmware agent in the initrd to not tell the
> > kernel it doesn't have it, but that is problematic when using
> > request_firmware(), the kernel boot will be delayed until the timeout
> > (one minute).
> >
> > This can be solved by adding an environment variable to the uevent that
> > tells userspace whether or not this is coming from request_firmware() or
> > request_firmware_nowait(). I will follow up with a patch doing that.
> >
> > Additionally, however, we need to make a change like below to the
> > firmware agent in order to not reply to asynchronous firmware loads
> > during the initrd stage. I'm not sure how to actually check that we're
> > running in an initrd (is that possible?) nor did I actually verify that
> > the NOWAIT environment variable is set properly.
> >
> > Thoughts? It seems like this would solve our problems nicely if we can
> > determine whether we're in the initrd or not.
> 
> I don't think we can reliably make the decision that the firmware is
> not available at a later point.
> 
> Depending on the system, we can have several re-trigger of the same
> events during bootup, and the firmware loader does not have any idea
> in which state the system is.

Oh, interesting, I had no idea.

> What's wrong to leave the request staying around for forever, until it
> is possibly canceled from userspace? So this event could be
> re-triggered from userspace any time later. Whatever will cancel the
> firmware requests in the end, it needs to be something that knows more
> about the state of "booting" than the firmware loader knows today, I
> guess.

Nothing in particular, although when you've set the timeout to 0 (no
timeout) this would have the request and a kernel thread stick around
forever. But I didn't know about multiple stages etc. here, so yes it
would probably make sense to just leave the request around until we know
we can reliably say we don't have it.

johannes