[PATCH] ar9170: load firmware asynchronously

[PATCH] ar9170: load firmware asynchronously

[Johannes Berg]

This converts ar9170 to load firmware asynchronously
out of ->probe() and only register with mac80211 when
all firmware has been loaded successfully. If, on the
other hand, any firmware fails to load, it will now
unbind from the device.

Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
---
This can go in now, the prereq patch went in via gregkh.

This ought to make it possible to build the driver
into the kernel and as long as your system boots
within 60 seconds it'll all just work.

This is how we need to load firmware for drivers that
require the firmware image to detect which features
to register to mac80211/cfg80211, but it's also useful
when no features depend on the firmware.

I've done ar9170 because I'm somewhat familiar with its
implementation. b43 should probably be converted to this
scheme since it actually has features that depend on the
firmware, and libertas_tf has the MAC addresss depending
on the firmware (well it needs the firmware to read it).

The only complication with this is that now the user is
mostly unaware they have a wireless device, they can only
find it via lsusb/lspci etc. if firmware loading fails,
or from kernel messages. And also manual binding in sysfs,
device re-plugging or module re-loading is required to
get the module to find the device after putting firmware
in place...

Still I think this is better than what we have now with
many drivers -- if firmware is missing you get network
interfaces etc. but cannot ever use them, and it's not
always entirely clear that is due to missing firmware.

 drivers/net/wireless/ath/ar9170/ar9170.h |    1 
 drivers/net/wireless/ath/ar9170/main.c   |   10 +
 drivers/net/wireless/ath/ar9170/usb.c    |  170 ++++++++++++++++++-------------
 3 files changed, 111 insertions(+), 70 deletions(-)

--- wireless-testing.orig/drivers/net/wireless/ath/ar9170/usb.c	2009-11-18 11:37:43.000000000 +0100
+++ wireless-testing/drivers/net/wireless/ath/ar9170/usb.c	2009-11-18 11:38:07.000000000 +0100
@@ -580,43 +580,6 @@ static int ar9170_usb_upload(struct ar91
 	return 0;
 }
 
-static int ar9170_usb_request_firmware(struct ar9170_usb *aru)
-{
-	int err = 0;
-
-	err = request_firmware(&aru->firmware, "ar9170.fw",
-			       &aru->udev->dev);
-	if (!err) {
-		aru->init_values = NULL;
-		return 0;
-	}
-
-	if (aru->req_one_stage_fw) {
-		dev_err(&aru->udev->dev, "ar9170.fw firmware file "
-			"not found and is required for this device\n");
-		return -EINVAL;
-	}
-
-	dev_err(&aru->udev->dev, "ar9170.fw firmware file "
-		"not found, trying old firmware...\n");
-
-	err = request_firmware(&aru->init_values, "ar9170-1.fw",
-			       &aru->udev->dev);
-	if (err) {
-		dev_err(&aru->udev->dev, "file with init values not found.\n");
-		return err;
-	}
-
-	err = request_firmware(&aru->firmware, "ar9170-2.fw", &aru->udev->dev);
-	if (err) {
-		release_firmware(aru->init_values);
-		dev_err(&aru->udev->dev, "firmware file not found.\n");
-		return err;
-	}
-
-	return err;
-}
-
 static int ar9170_usb_reset(struct ar9170_usb *aru)
 {
 	int ret, lock = (aru->intf->condition != USB_INTERFACE_BINDING);
@@ -755,6 +718,103 @@ err_out:
 	return err;
 }
 
+static void ar9170_usb_firmware_failed(struct ar9170_usb *aru)
+{
+	struct device *parent = aru->udev->dev.parent;
+
+	/* unbind anything failed */
+	if (parent)
+		down(&parent->sem);
+	device_release_driver(&aru->udev->dev);
+	if (parent)
+		up(&parent->sem);
+}
+
+static void ar9170_usb_firmware_finish(const struct firmware *fw, void *context)
+{
+	struct ar9170_usb *aru = context;
+	int err;
+
+	aru->firmware = fw;
+
+	if (!fw) {
+		dev_err(&aru->udev->dev, "firmware file not found.\n");
+		goto err_freefw;
+	}
+
+	err = ar9170_usb_init_device(aru);
+	if (err)
+		goto err_freefw;
+
+	err = ar9170_usb_open(&aru->common);
+	if (err)
+		goto err_unrx;
+
+	err = ar9170_register(&aru->common, &aru->udev->dev);
+
+	ar9170_usb_stop(&aru->common);
+	if (err)
+		goto err_unrx;
+
+	return;
+
+ err_unrx:
+	ar9170_usb_cancel_urbs(aru);
+
+ err_freefw:
+	ar9170_usb_firmware_failed(aru);
+}
+
+static void ar9170_usb_firmware_inits(const struct firmware *fw,
+				      void *context)
+{
+	struct ar9170_usb *aru = context;
+	int err;
+
+	if (!fw) {
+		dev_err(&aru->udev->dev, "file with init values not found.\n");
+		ar9170_usb_firmware_failed(aru);
+		return;
+	}
+
+	aru->init_values = fw;
+
+	/* ok so we have the init values -- get code for two-stage */
+
+	err = request_firmware_nowait(THIS_MODULE, 1, "ar9170-2.fw",
+				      &aru->udev->dev, GFP_KERNEL, aru,
+				      ar9170_usb_firmware_finish);
+	if (err)
+		ar9170_usb_firmware_failed(aru);
+}
+
+static void ar9170_usb_firmware_step2(const struct firmware *fw, void *context)
+{
+	struct ar9170_usb *aru = context;
+	int err;
+
+	if (fw) {
+		ar9170_usb_firmware_finish(fw, context);
+		return;
+	}
+
+	if (aru->req_one_stage_fw) {
+		dev_err(&aru->udev->dev, "ar9170.fw firmware file "
+			"not found and is required for this device\n");
+		ar9170_usb_firmware_failed(aru);
+		return;
+	}
+
+	dev_err(&aru->udev->dev, "ar9170.fw firmware file "
+		"not found, trying old firmware...\n");
+
+	err = request_firmware_nowait(THIS_MODULE, 1, "ar9170-1.fw",
+				      &aru->udev->dev, GFP_KERNEL, aru,
+				      ar9170_usb_firmware_inits);
+	if (err)
+		ar9170_usb_firmware_failed(aru);
+}
+
 static bool ar9170_requires_one_stage(const struct usb_device_id *id)
 {
 	if (!id->driver_info)
@@ -812,33 +872,9 @@ static int ar9170_usb_probe(struct usb_i
 	if (err)
 		goto err_freehw;
 
-	err = ar9170_usb_request_firmware(aru);
-	if (err)
-		goto err_freehw;
-
-	err = ar9170_usb_init_device(aru);
-	if (err)
-		goto err_freefw;
-
-	err = ar9170_usb_open(ar);
-	if (err)
-		goto err_unrx;
-
-	err = ar9170_register(ar, &udev->dev);
-
-	ar9170_usb_stop(ar);
-	if (err)
-		goto err_unrx;
-
-	return 0;
-
-err_unrx:
-	ar9170_usb_cancel_urbs(aru);
-
-err_freefw:
-	release_firmware(aru->init_values);
-	release_firmware(aru->firmware);
-
+	return request_firmware_nowait(THIS_MODULE, 1, "ar9170.fw",
+				       &aru->udev->dev, GFP_KERNEL, aru,
+				       ar9170_usb_firmware_step2);
 err_freehw:
 	usb_set_intfdata(intf, NULL);
 	usb_put_dev(udev);
@@ -858,12 +894,12 @@ static void ar9170_usb_disconnect(struct
 	ar9170_unregister(&aru->common);
 	ar9170_usb_cancel_urbs(aru);
 
-	release_firmware(aru->init_values);
-	release_firmware(aru->firmware);
-
 	usb_put_dev(aru->udev);
 	usb_set_intfdata(intf, NULL);
 	ieee80211_free_hw(aru->common.hw);
+
+	release_firmware(aru->init_values);
+	release_firmware(aru->firmware);
 }
 
 #ifdef CONFIG_PM
--- wireless-testing.orig/drivers/net/wireless/ath/ar9170/ar9170.h	2009-11-18 11:31:48.000000000 +0100
+++ wireless-testing/drivers/net/wireless/ath/ar9170/ar9170.h	2009-11-18 11:38:07.000000000 +0100
@@ -160,6 +160,7 @@ struct ar9170 {
 	struct ath_common common;
 	struct mutex mutex;
 	enum ar9170_device_state state;
+	bool registered;
 	unsigned long bad_hw_nagger;
 
 	int (*open)(struct ar9170 *);
--- wireless-testing.orig/drivers/net/wireless/ath/ar9170/main.c	2009-11-18 11:37:43.000000000 +0100
+++ wireless-testing/drivers/net/wireless/ath/ar9170/main.c	2009-11-18 11:38:07.000000000 +0100
@@ -2724,7 +2724,8 @@ int ar9170_register(struct ar9170 *ar, s
 	dev_info(pdev, "Atheros AR9170 is registered as '%s'\n",
 		 wiphy_name(ar->hw->wiphy));
 
-	return err;
+	ar->registered = true;
+	return 0;
 
 err_unreg:
 	ieee80211_unregister_hw(ar->hw);
@@ -2735,11 +2736,14 @@ err_out:
 
 void ar9170_unregister(struct ar9170 *ar)
 {
+	if (ar->registered) {
 #ifdef CONFIG_AR9170_LEDS
-	ar9170_unregister_leds(ar);
+		ar9170_unregister_leds(ar);
 #endif /* CONFIG_AR9170_LEDS */
 
-	kfree_skb(ar->rx_failover);
 	ieee80211_unregister_hw(ar->hw);
+	}
+
+	kfree_skb(ar->rx_failover);
 	mutex_destroy(&ar->mutex);
 }

Re: [PATCH] ar9170: load firmware asynchronously

[Luis R. Rodriguez]

On Fri, Dec 18, 2009 at 2:47 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> This converts ar9170 to load firmware asynchronously
> out of ->probe() and only register with mac80211 when
> all firmware has been loaded successfully. If, on the
> other hand, any firmware fails to load, it will now
> unbind from the device.
>
> Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> ---
> This can go in now, the prereq patch went in via gregkh.
>
> This ought to make it possible to build the driver
> into the kernel and as long as your system boots
> within 60 seconds it'll all just work.
>
> This is how we need to load firmware for drivers that
> require the firmware image to detect which features
> to register to mac80211/cfg80211, but it's also useful
> when no features depend on the firmware.
>
> I've done ar9170 because I'm somewhat familiar with its
> implementation. b43 should probably be converted to this
> scheme since it actually has features that depend on the
> firmware, and libertas_tf has the MAC addresss depending
> on the firmware (well it needs the firmware to read it).
>
> The only complication with this is that now the user is
> mostly unaware they have a wireless device, they can only
> find it via lsusb/lspci etc. if firmware loading fails,
> or from kernel messages. And also manual binding in sysfs,
> device re-plugging or module re-loading is required to
> get the module to find the device after putting firmware
> in place...
>
> Still I think this is better than what we have now with
> many drivers -- if firmware is missing you get network
> interfaces etc. but cannot ever use them, and it's not
> always entirely clear that is due to missing firmware.

Sweet -- we could also expose firmware load events if not done so
already and let userspace propagate such nasty warnings, I think we'd
need to pass the firmware expected and the driver the requested it so
that once the firmware is in place (I guess through inotify maybe) the
driver modprobe can be retried.

  Luis

Re: [PATCH] ar9170: load firmware asynchronously

[Marcel Holtmann]

Hi Luis,

> > This converts ar9170 to load firmware asynchronously
> > out of ->probe() and only register with mac80211 when
> > all firmware has been loaded successfully. If, on the
> > other hand, any firmware fails to load, it will now
> > unbind from the device.
> >
> > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
> > ---
> > This can go in now, the prereq patch went in via gregkh.
> >
> > This ought to make it possible to build the driver
> > into the kernel and as long as your system boots
> > within 60 seconds it'll all just work.
> >
> > This is how we need to load firmware for drivers that
> > require the firmware image to detect which features
> > to register to mac80211/cfg80211, but it's also useful
> > when no features depend on the firmware.
> >
> > I've done ar9170 because I'm somewhat familiar with its
> > implementation. b43 should probably be converted to this
> > scheme since it actually has features that depend on the
> > firmware, and libertas_tf has the MAC addresss depending
> > on the firmware (well it needs the firmware to read it).
> >
> > The only complication with this is that now the user is
> > mostly unaware they have a wireless device, they can only
> > find it via lsusb/lspci etc. if firmware loading fails,
> > or from kernel messages. And also manual binding in sysfs,
> > device re-plugging or module re-loading is required to
> > get the module to find the device after putting firmware
> > in place...
> >
> > Still I think this is better than what we have now with
> > many drivers -- if firmware is missing you get network
> > interfaces etc. but cannot ever use them, and it's not
> > always entirely clear that is due to missing firmware.
> 
> Sweet -- we could also expose firmware load events if not done so
> already and let userspace propagate such nasty warnings, I think we'd
> need to pass the firmware expected and the driver the requested it so
> that once the firmware is in place (I guess through inotify maybe) the
> driver modprobe can be retried.

actually PackageKit should be doing something like this already. It
might need to be adapted a bit, but the basics should be there.

Regards

Marcel

Re: [PATCH] ar9170: load firmware asynchronously

[Luis R. Rodriguez]

On Fri, Dec 18, 2009 at 3:08 PM, Marcel Holtmann <marcel@holtmann.org> wrote:
> Hi Luis,
>
>> > This converts ar9170 to load firmware asynchronously
>> > out of ->probe() and only register with mac80211 when
>> > all firmware has been loaded successfully. If, on the
>> > other hand, any firmware fails to load, it will now
>> > unbind from the device.
>> >
>> > Signed-off-by: Johannes Berg <johannes@sipsolutions.net>
>> > ---
>> > This can go in now, the prereq patch went in via gregkh.
>> >
>> > This ought to make it possible to build the driver
>> > into the kernel and as long as your system boots
>> > within 60 seconds it'll all just work.
>> >
>> > This is how we need to load firmware for drivers that
>> > require the firmware image to detect which features
>> > to register to mac80211/cfg80211, but it's also useful
>> > when no features depend on the firmware.
>> >
>> > I've done ar9170 because I'm somewhat familiar with its
>> > implementation. b43 should probably be converted to this
>> > scheme since it actually has features that depend on the
>> > firmware, and libertas_tf has the MAC addresss depending
>> > on the firmware (well it needs the firmware to read it).
>> >
>> > The only complication with this is that now the user is
>> > mostly unaware they have a wireless device, they can only
>> > find it via lsusb/lspci etc. if firmware loading fails,
>> > or from kernel messages. And also manual binding in sysfs,
>> > device re-plugging or module re-loading is required to
>> > get the module to find the device after putting firmware
>> > in place...
>> >
>> > Still I think this is better than what we have now with
>> > many drivers -- if firmware is missing you get network
>> > interfaces etc. but cannot ever use them, and it's not
>> > always entirely clear that is due to missing firmware.
>>
>> Sweet -- we could also expose firmware load events if not done so
>> already and let userspace propagate such nasty warnings, I think we'd
>> need to pass the firmware expected and the driver the requested it so
>> that once the firmware is in place (I guess through inotify maybe) the
>> driver modprobe can be retried.
>
> actually PackageKit should be doing something like this already. It
> might need to be adapted a bit, but the basics should be there.

Wow, I'm impressed. Thanks for the heads up.

  Luis

Re: [PATCH] ar9170: load firmware asynchronously

[Johannes Berg]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

On Fri, 2009-12-18 at 15:02 -0800, Luis R. Rodriguez wrote:

> Sweet -- we could also expose firmware load events if not done so
> already and let userspace propagate such nasty warnings, I think we'd
> need to pass the firmware expected and the driver the requested it so
> that once the firmware is in place (I guess through inotify maybe) the
> driver modprobe can be retried.

Actually, since the driver doesn't fail loading, the _bind_ has to be
retried. I'm not sure there's a way to trigger a re-probe though, which
would be easiest since userspace doesn't necessarily know what to bind.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]