[RFC/RFT 0/5] Firmware load change for various wireless drivers

[RFC/RFT 0/5] Firmware load change for various wireless drivers

[Larry Finger]

These patches change 5 drivers that are now loading firmware from the probe
routine. As each of them has the possibility of loading more than one
file, the method of using request_firmware_nowait() has some difficulty,
as it gets duplicate names in sysfs when the requests are launched in
parallel. When the callback routine is used to launch a second request,
the structure gets messy, particularly with b43legacy, which loads 4
firmware files for some hardware. My solution is to create a delayed work
queue that is started in the probe routine with a delay of one second. The
routine that is triggered does the normal request_firmware() calls
and starts mac80211 when the firmware is available.

The patches for b43legacy, b43, and p54usb have been tested. Those for
p54pci and p54spi are only compile tested.

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---

Larry Finger (5):
  b43legacy: Load firmware from work queue instead of from probe
    routine
  b43: Load firmware from a work queue and not from the probe routine
  p54usb: Load firmware from work queue and not from probe routine
  p54pci: Load firmware from work queue and not from probe routine
  p54spi: Load firmware from work queue and not from probe routine

 drivers/net/wireless/b43/b43.h             |    3 +
 drivers/net/wireless/b43/main.c            |   38 +++++----
 drivers/net/wireless/b43legacy/b43legacy.h |    3 +
 drivers/net/wireless/b43legacy/main.c      |   32 +++++---
 drivers/net/wireless/p54/p54pci.c          |   66 +++++++++------
 drivers/net/wireless/p54/p54pci.h          |    1 +
 drivers/net/wireless/p54/p54spi.c          |   38 ++++++---
 drivers/net/wireless/p54/p54spi.h          |    1 +
 drivers/net/wireless/p54/p54usb.c          |  120 +++++++++++++---------------
 drivers/net/wireless/p54/p54usb.h          |    1 +
 10 files changed, 174 insertions(+), 129 deletions(-)

-- 
1.7.7

[RFC/RFT 1/5] b43legacy: Load firmware from work queue instead of from probe routine

[Larry Finger]

Recent changes in udev are causing problems for drivers that load firmware
from the probe routine. As b43legacy has such a structure, it must be changed.
As this driver loads 3 or 4 firmware files, changing to the asynchronous routine
request_firmware_nowait() would be complicated. In this implementation, the probe
routine starts a delayed_work queue that calls the firmware loading routines when
the delay (1 sec) expires..

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
 drivers/net/wireless/b43legacy/b43legacy.h |    3 ++
 drivers/net/wireless/b43legacy/main.c      |   32 ++++++++++++++++-----------
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/b43legacy/b43legacy.h b/drivers/net/wireless/b43legacy/b43legacy.h
index 98e3d44..e9337e5 100644
--- a/drivers/net/wireless/b43legacy/b43legacy.h
+++ b/drivers/net/wireless/b43legacy/b43legacy.h
@@ -581,6 +581,9 @@ struct b43legacy_wl {
 	struct mutex mutex;		/* locks wireless core state */
 	spinlock_t leds_lock;		/* lock for leds */
 
+	/* delayed firmware loading */
+	struct delayed_work firmware_load;
+
 	/* We can only have one operating interface (802.11 core)
 	 * at a time. General information about this interface follows.
 	 */
diff --git a/drivers/net/wireless/b43legacy/main.c b/drivers/net/wireless/b43legacy/main.c
index 75e70bc..99d1237 100644
--- a/drivers/net/wireless/b43legacy/main.c
+++ b/drivers/net/wireless/b43legacy/main.c
@@ -1557,8 +1557,15 @@ err_format:
 	return -EPROTO;
 }
 
-static int b43legacy_request_firmware(struct b43legacy_wldev *dev)
+static int b43legacy_one_core_attach(struct ssb_device *dev,
+				     struct b43legacy_wl *wl);
+static void b43legacy_one_core_detach(struct ssb_device *dev);
+
+static void b43legacy_request_firmware(struct work_struct *work)
 {
+	struct b43legacy_wl *wl = container_of(to_delayed_work(work),
+				  struct b43legacy_wl, firmware_load);
+	struct b43legacy_wldev *dev = wl->current_dev;
 	struct b43legacy_firmware *fw = &dev->fw;
 	const u8 rev = dev->dev->id.revision;
 	const char *filename;
@@ -1624,8 +1631,14 @@ static int b43legacy_request_firmware(struct b43legacy_wldev *dev)
 		if (err)
 			goto err_load;
 	}
+	err = ieee80211_register_hw(wl->hw);
+	if (err)
+		goto err_one_core_detach;
+	return;
 
-	return 0;
+err_one_core_detach:
+	b43legacy_one_core_detach(dev->dev);
+	goto error;
 
 err_load:
 	b43legacy_print_fw_helptext(dev->wl);
@@ -1639,7 +1652,7 @@ err_no_initvals:
 
 error:
 	b43legacy_release_firmware(dev);
-	return err;
+	return;
 }
 
 static int b43legacy_upload_microcode(struct b43legacy_wldev *dev)
@@ -2153,9 +2166,6 @@ static int b43legacy_chip_init(struct b43legacy_wldev *dev)
 	macctl |= B43legacy_MACCTL_INFRA;
 	b43legacy_write32(dev, B43legacy_MMIO_MACCTL, macctl);
 
-	err = b43legacy_request_firmware(dev);
-	if (err)
-		goto out;
 	err = b43legacy_upload_microcode(dev);
 	if (err)
 		goto out; /* firmware is released later */
@@ -3860,17 +3870,13 @@ static int b43legacy_probe(struct ssb_device *dev,
 	if (err)
 		goto err_wireless_exit;
 
-	if (first) {
-		err = ieee80211_register_hw(wl->hw);
-		if (err)
-			goto err_one_core_detach;
-	}
+	/* setup and start delayed work to load firmware */
+	INIT_DELAYED_WORK(&wl->firmware_load, b43legacy_request_firmware);
+	schedule_delayed_work(&wl->firmware_load, HZ);
 
 out:
 	return err;
 
-err_one_core_detach:
-	b43legacy_one_core_detach(dev);
 err_wireless_exit:
 	if (first)
 		b43legacy_wireless_exit(dev, wl);
-- 
1.7.7

[RFC/RFT 2/5] b43: Load firmware from a work queue and not from the probe routine

[Larry Finger]

Recent changes in udev are causing problems for drivers that load firmware
from the probe routine. As b43 has such a structure, it must be changed.
As this driver loads more than 1 firmware file, changing to the asynchronous routine
request_firmware_nowait() would be complicated. In this implementation, the probe
routine starts a delayed_work queue that calls the firmware loading routines when
the delay (1 sec) expires..

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
 drivers/net/wireless/b43/b43.h  |    3 +++
 drivers/net/wireless/b43/main.c |   38 ++++++++++++++++++++++----------------
 2 files changed, 25 insertions(+), 16 deletions(-)

diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
index 835462d..532ba79 100644
--- a/drivers/net/wireless/b43/b43.h
+++ b/drivers/net/wireless/b43/b43.h
@@ -932,6 +932,9 @@ struct b43_wl {
 	/* Flag that implement the queues stopping. */
 	bool tx_queue_stopped[B43_QOS_QUEUE_NUM];
 
+	/* delayed firmware loading */
+	struct delayed_work firmware_load;
+
 	/* The device LEDs. */
 	struct b43_leds leds;
 
diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
index 1b540d2..903e1ea 100644
--- a/drivers/net/wireless/b43/main.c
+++ b/drivers/net/wireless/b43/main.c
@@ -2390,8 +2390,14 @@ error:
 	return err;
 }
 
-static int b43_request_firmware(struct b43_wldev *dev)
+static int b43_one_core_attach(struct b43_bus_dev *dev, struct b43_wl *wl);
+static void b43_one_core_detach(struct b43_bus_dev *dev);
+
+static void b43_request_firmware(struct work_struct *work)
 {
+	struct b43_wl *wl = container_of(to_delayed_work(work),
+			    struct b43_wl, firmware_load);
+	struct b43_wldev *dev = wl->current_dev;
 	struct b43_request_fw_context *ctx;
 	unsigned int i;
 	int err;
@@ -2399,13 +2405,13 @@ static int b43_request_firmware(struct b43_wldev *dev)
 
 	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
 	if (!ctx)
-		return -ENOMEM;
+		return;
 	ctx->dev = dev;
 
 	ctx->req_type = B43_FWTYPE_PROPRIETARY;
 	err = b43_try_request_fw(ctx);
 	if (!err)
-		goto out; /* Successfully loaded it. */
+		goto start_ieee80211; /* Successfully loaded it. */
 	err = ctx->fatal_failure;
 	if (err)
 		goto out;
@@ -2413,7 +2419,7 @@ static int b43_request_firmware(struct b43_wldev *dev)
 	ctx->req_type = B43_FWTYPE_OPENSOURCE;
 	err = b43_try_request_fw(ctx);
 	if (!err)
-		goto out; /* Successfully loaded it. */
+		goto start_ieee80211; /* Successfully loaded it. */
 	err = ctx->fatal_failure;
 	if (err)
 		goto out;
@@ -2427,9 +2433,17 @@ static int b43_request_firmware(struct b43_wldev *dev)
 	b43_print_fw_helptext(dev->wl, 1);
 	err = -ENOENT;
 
+start_ieee80211:
+	err = ieee80211_register_hw(wl->hw);
+	if (err)
+		goto err_one_core_detach;
+	goto out;
+
+err_one_core_detach:
+	b43_one_core_detach(dev->dev);
+
 out:
 	kfree(ctx);
-	return err;
 }
 
 static int b43_upload_microcode(struct b43_wldev *dev)
@@ -3021,9 +3035,6 @@ static int b43_chip_init(struct b43_wldev *dev)
 	macctl |= B43_MACCTL_INFRA;
 	b43_write32(dev, B43_MMIO_MACCTL, macctl);
 
-	err = b43_request_firmware(dev);
-	if (err)
-		goto out;
 	err = b43_upload_microcode(dev);
 	if (err)
 		goto out;	/* firmware is released later */
@@ -5391,18 +5402,13 @@ int b43_ssb_probe(struct ssb_device *sdev, const struct ssb_device_id *id)
 	if (err)
 		goto err_wireless_exit;
 
-	if (first) {
-		err = ieee80211_register_hw(wl->hw);
-		if (err)
-			goto err_one_core_detach;
-		b43_leds_register(wl->current_dev);
-	}
+	/* setup and start delayed work to load firmware */
+	INIT_DELAYED_WORK(&wl->firmware_load, b43_request_firmware);
+	schedule_delayed_work(&wl->firmware_load, HZ);
 
       out:
 	return err;
 
-      err_one_core_detach:
-	b43_one_core_detach(dev);
       err_wireless_exit:
 	if (first)
 		b43_wireless_exit(dev, wl);
-- 
1.7.7

Re: [RFC/RFT 2/5] b43: Load firmware from a work queue and not from the probe routine

[Julian Calaby]

Hi Larry,

On Tue, Feb 14, 2012 at 06:37, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> Recent changes in udev are causing problems for drivers that load firmware
> from the probe routine. As b43 has such a structure, it must be changed.
> As this driver loads more than 1 firmware file, changing to the asynchronous routine
> request_firmware_nowait() would be complicated. In this implementation, the probe
> routine starts a delayed_work queue that calls the firmware loading routines when
> the delay (1 sec) expires..
>
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
> diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
> index 835462d..532ba79 100644
> --- a/drivers/net/wireless/b43/b43.h
> +++ b/drivers/net/wireless/b43/b43.h
> @@ -932,6 +932,9 @@ struct b43_wl {
>        /* Flag that implement the queues stopping. */
>        bool tx_queue_stopped[B43_QOS_QUEUE_NUM];
>
> +       /* delayed firmware loading */
> +       struct delayed_work firmware_load;
> +
>        /* The device LEDs. */
>        struct b43_leds leds;
>
> diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
> index 1b540d2..903e1ea 100644
> --- a/drivers/net/wireless/b43/main.c
> +++ b/drivers/net/wireless/b43/main.c
> @@ -2427,9 +2433,17 @@ static int b43_request_firmware(struct b43_wldev *dev)
>        b43_print_fw_helptext(dev->wl, 1);
>        err = -ENOENT;

Should something be done here rather than immediately going to
register the card with ieee80211?

> +start_ieee80211:
> +       err = ieee80211_register_hw(wl->hw);
> +       if (err)
> +               goto err_one_core_detach;
> +       goto out;
> +
> +err_one_core_detach:
> +       b43_one_core_detach(dev->dev);
> +
>  out:
>        kfree(ctx);
> -       return err;
>  }
>
>  static int b43_upload_microcode(struct b43_wldev *dev)

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

Re: [RFC/RFT 2/5] b43: Load firmware from a work queue and not from the probe routine

[Larry Finger]

On 02/13/2012 05:06 PM, Julian Calaby wrote:
> Hi Larry,
>
> On Tue, Feb 14, 2012 at 06:37, Larry Finger<Larry.Finger@lwfinger.net>  wrote:
>> Recent changes in udev are causing problems for drivers that load firmware
>> from the probe routine. As b43 has such a structure, it must be changed.
>> As this driver loads more than 1 firmware file, changing to the asynchronous routine
>> request_firmware_nowait() would be complicated. In this implementation, the probe
>> routine starts a delayed_work queue that calls the firmware loading routines when
>> the delay (1 sec) expires..
>>
>> Signed-off-by: Larry Finger<Larry.Finger@lwfinger.net>
>> ---
>> diff --git a/drivers/net/wireless/b43/b43.h b/drivers/net/wireless/b43/b43.h
>> index 835462d..532ba79 100644
>> --- a/drivers/net/wireless/b43/b43.h
>> +++ b/drivers/net/wireless/b43/b43.h
>> @@ -932,6 +932,9 @@ struct b43_wl {
>>         /* Flag that implement the queues stopping. */
>>         bool tx_queue_stopped[B43_QOS_QUEUE_NUM];
>>
>> +       /* delayed firmware loading */
>> +       struct delayed_work firmware_load;
>> +
>>         /* The device LEDs. */
>>         struct b43_leds leds;
>>
>> diff --git a/drivers/net/wireless/b43/main.c b/drivers/net/wireless/b43/main.c
>> index 1b540d2..903e1ea 100644
>> --- a/drivers/net/wireless/b43/main.c
>> +++ b/drivers/net/wireless/b43/main.c
>> @@ -2427,9 +2433,17 @@ static int b43_request_firmware(struct b43_wldev *dev)
>>         b43_print_fw_helptext(dev->wl, 1);
>>         err = -ENOENT;
>
> Should something be done here rather than immediately going to
> register the card with ieee80211?

I don't think so. When firmware is loaded from the probe routine, it registers 
the card as the next step. I did miss the step that registers the leds - that 
has now been added.

Larry

>
>> +start_ieee80211:
>> +       err = ieee80211_register_hw(wl->hw);
>> +       if (err)
>> +               goto err_one_core_detach;
>> +       goto out;
>> +
>> +err_one_core_detach:
>> +       b43_one_core_detach(dev->dev);
>> +
>>   out:
>>         kfree(ctx);
>> -       return err;
>>   }
>>
>>   static int b43_upload_microcode(struct b43_wldev *dev)
>
> Thanks,
>

Re: [RFC/RFT 2/5] b43: Load firmware from a work queue and not from the probe routine

[Julian Calaby]

Hi Larry,

On Tue, Feb 14, 2012 at 10:28, Larry Finger <Larry.Finger@lwfinger.net> wrote:
> On 02/13/2012 05:06 PM, Julian Calaby wrote:
>>
>> Hi Larry,
>>
>> On Tue, Feb 14, 2012 at 06:37, Larry Finger<Larry.Finger@lwfinger.net>
>>  wrote:
>>>
>>> Recent changes in udev are causing problems for drivers that load
>>> firmware
>>> from the probe routine. As b43 has such a structure, it must be changed.
>>> As this driver loads more than 1 firmware file, changing to the
>>> asynchronous routine
>>> request_firmware_nowait() would be complicated. In this implementation,
>>> the probe
>>> routine starts a delayed_work queue that calls the firmware loading
>>> routines when
>>> the delay (1 sec) expires..
>>>
>>> Signed-off-by: Larry Finger<Larry.Finger@lwfinger.net>
>>> ---
>>> diff --git a/drivers/net/wireless/b43/b43.h
>>> b/drivers/net/wireless/b43/b43.h
>>> index 835462d..532ba79 100644
>>> --- a/drivers/net/wireless/b43/b43.h
>>> +++ b/drivers/net/wireless/b43/b43.h
>>> @@ -932,6 +932,9 @@ struct b43_wl {
>>>        /* Flag that implement the queues stopping. */
>>>        bool tx_queue_stopped[B43_QOS_QUEUE_NUM];
>>>
>>> +       /* delayed firmware loading */
>>> +       struct delayed_work firmware_load;
>>> +
>>>        /* The device LEDs. */
>>>        struct b43_leds leds;
>>>
>>> diff --git a/drivers/net/wireless/b43/main.c
>>> b/drivers/net/wireless/b43/main.c
>>> index 1b540d2..903e1ea 100644
>>> --- a/drivers/net/wireless/b43/main.c
>>> +++ b/drivers/net/wireless/b43/main.c
>>> @@ -2427,9 +2433,17 @@ static int b43_request_firmware(struct b43_wldev
>>> *dev)
>>>        b43_print_fw_helptext(dev->wl, 1);
>>>        err = -ENOENT;
>>
>>
>> Should something be done here rather than immediately going to
>> register the card with ieee80211?
>
>
> I don't think so. When firmware is loaded from the probe routine, it
> registers the card as the next step. I did miss the step that registers the
> leds - that has now been added.

My point here is that in the original flow, the -ENOENT would have
been returned to b43_chip_init() and the hardware would never have
been registered with mac80211. After this patch, it appears that if
the firmware cannot be found, it still registers the hardware with
mac80211.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
.Plan: http://sites.google.com/site/juliancalaby/

Re: [RFC/RFT 2/5] b43: Load firmware from a work queue and not from the probe routine

[Larry Finger]

On 02/13/2012 05:41 PM, Julian Calaby wrote:
> Hi Larry,
>
> On Tue, Feb 14, 2012 at 10:28, Larry Finger<Larry.Finger@lwfinger.net>  wrote:
>> On 02/13/2012 05:06 PM, Julian Calaby wrote:
>>>
>>> Hi Larry,
>>>
>>> On Tue, Feb 14, 2012 at 06:37, Larry Finger<Larry.Finger@lwfinger.net>
>>>   wrote:
>>>>
>>>> Recent changes in udev are causing problems for drivers that load
>>>> firmware
>>>> from the probe routine. As b43 has such a structure, it must be changed.
>>>> As this driver loads more than 1 firmware file, changing to the
>>>> asynchronous routine
>>>> request_firmware_nowait() would be complicated. In this implementation,
>>>> the probe
>>>> routine starts a delayed_work queue that calls the firmware loading
>>>> routines when
>>>> the delay (1 sec) expires..
>>>>
>>>> Signed-off-by: Larry Finger<Larry.Finger@lwfinger.net>
>>>> ---
>>>> diff --git a/drivers/net/wireless/b43/b43.h
>>>> b/drivers/net/wireless/b43/b43.h
>>>> index 835462d..532ba79 100644
>>>> --- a/drivers/net/wireless/b43/b43.h
>>>> +++ b/drivers/net/wireless/b43/b43.h
>>>> @@ -932,6 +932,9 @@ struct b43_wl {
>>>>         /* Flag that implement the queues stopping. */
>>>>         bool tx_queue_stopped[B43_QOS_QUEUE_NUM];
>>>>
>>>> +       /* delayed firmware loading */
>>>> +       struct delayed_work firmware_load;
>>>> +
>>>>         /* The device LEDs. */
>>>>         struct b43_leds leds;
>>>>
>>>> diff --git a/drivers/net/wireless/b43/main.c
>>>> b/drivers/net/wireless/b43/main.c
>>>> index 1b540d2..903e1ea 100644
>>>> --- a/drivers/net/wireless/b43/main.c
>>>> +++ b/drivers/net/wireless/b43/main.c
>>>> @@ -2427,9 +2433,17 @@ static int b43_request_firmware(struct b43_wldev
>>>> *dev)
>>>>         b43_print_fw_helptext(dev->wl, 1);
>>>>         err = -ENOENT;
>>>
>>>
>>> Should something be done here rather than immediately going to
>>> register the card with ieee80211?
>>
>>
>> I don't think so. When firmware is loaded from the probe routine, it
>> registers the card as the next step. I did miss the step that registers the
>> leds - that has now been added.
>
> My point here is that in the original flow, the -ENOENT would have
> been returned to b43_chip_init() and the hardware would never have
> been registered with mac80211. After this patch, it appears that if
> the firmware cannot be found, it still registers the hardware with
> mac80211.

I see what you mean. The statement before the start_ieee80211 label should be 
'goto out' rather than falling through.

Larry

[RFC/RFT 3/5] p54usb: Load firmware from work queue and not from probe routine

[Larry Finger]

Drivers that load firmware from their probe routine have problems with the
latest versions of udev as they get timeouts while waiting for user
space to start. The problem is fixed by loading the firmware and starting
mac80211 from a delayed_work queue. By using this method, most of the
original code is preserved.

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
 drivers/net/wireless/p54/p54usb.c |  120 +++++++++++++++++-------------------
 drivers/net/wireless/p54/p54usb.h |    1 +
 2 files changed, 58 insertions(+), 63 deletions(-)

diff --git a/drivers/net/wireless/p54/p54usb.c b/drivers/net/wireless/p54/p54usb.c
index f4d28c3..b8eff19 100644
--- a/drivers/net/wireless/p54/p54usb.c
+++ b/drivers/net/wireless/p54/p54usb.c
@@ -836,9 +836,33 @@ fail:
 	return err;
 }
 
-static int p54u_load_firmware(struct ieee80211_hw *dev)
+static int p54u_open(struct ieee80211_hw *dev)
 {
 	struct p54u_priv *priv = dev->priv;
+	int err;
+
+	err = p54u_init_urbs(dev);
+	if (err)
+		return err;
+
+	priv->common.open = p54u_init_urbs;
+
+	return 0;
+}
+
+static void p54u_stop(struct ieee80211_hw *dev)
+{
+	/* TODO: figure out how to reliably stop the 3887 and net2280 so
+	   the hardware is still usable next time we want to start it.
+	   until then, we just stop listening to the hardware.. */
+	p54u_free_urbs(dev);
+}
+
+static void p54u_load_firmware(struct work_struct *work)
+{
+	struct p54u_priv *priv = container_of(to_delayed_work(work),
+				 struct p54u_priv, firmware_load);
+	struct ieee80211_hw *dev = usb_get_intfdata(priv->intf);
 	int err, i;
 
 	BUILD_BUG_ON(ARRAY_SIZE(p54u_fwlist) != __NUM_P54U_HWTYPES);
@@ -847,59 +871,53 @@ static int p54u_load_firmware(struct ieee80211_hw *dev)
 		if (p54u_fwlist[i].type == priv->hw_type)
 			break;
 
-	if (i == __NUM_P54U_HWTYPES)
-		return -EOPNOTSUPP;
+	if (i == __NUM_P54U_HWTYPES) {
+		dev_err(&priv->udev->dev, "Device not supported\n");
+		return;
+	}
 
 	err = request_firmware(&priv->fw, p54u_fwlist[i].fw, &priv->udev->dev);
 	if (err) {
-		dev_err(&priv->udev->dev, "(p54usb) cannot load firmware %s "
-					  "(%d)!\n", p54u_fwlist[i].fw, err);
-
 		err = request_firmware(&priv->fw, p54u_fwlist[i].fw_legacy,
 				       &priv->udev->dev);
-		if (err)
-			return err;
+
+		if (err) {
+			dev_err(&priv->udev->dev,
+				"(p54usb) cannot load firmware %s or %s(%d)!\n",
+				p54u_fwlist[i].fw, p54u_fwlist[i].fw_legacy,
+				err);
+			return;
+		}
 	}
 
 	err = p54_parse_firmware(dev, priv->fw);
-	if (err)
-		goto out;
+	if (err) {
+		dev_err(&priv->udev->dev, "Error parsing firmware\n");
+		return;
+	}
 
 	if (priv->common.fw_interface != p54u_fwlist[i].intf) {
 		dev_err(&priv->udev->dev, "wrong firmware, please get "
 			"a firmware for \"%s\" and try again.\n",
 			p54u_fwlist[i].hw);
-		err = -EINVAL;
+		return;
 	}
-
-out:
-	if (err)
-		release_firmware(priv->fw);
-
-	return err;
-}
-
-static int p54u_open(struct ieee80211_hw *dev)
-{
-	struct p54u_priv *priv = dev->priv;
-	int err;
-
-	err = p54u_init_urbs(dev);
+	err = priv->upload_fw(dev);
 	if (err) {
-		return err;
+		dev_err(&priv->udev->dev, "Error uploading firmware\n");
+		return;
 	}
 
-	priv->common.open = p54u_init_urbs;
-
-	return 0;
-}
+	p54u_open(dev);
+	err = p54_read_eeprom(dev);
+	if (err)
+		dev_err(&priv->udev->dev, "Error reading eeprom\n");
+	p54u_stop(dev);
+	if (err)
+		return;
 
-static void p54u_stop(struct ieee80211_hw *dev)
-{
-	/* TODO: figure out how to reliably stop the 3887 and net2280 so
-	   the hardware is still usable next time we want to start it.
-	   until then, we just stop listening to the hardware.. */
-	p54u_free_urbs(dev);
+	/* firmware available - start operations */
+	err = p54_register_common(dev, &priv->udev->dev);
 }
 
 static int __devinit p54u_probe(struct usb_interface *intf,
@@ -969,34 +987,10 @@ static int __devinit p54u_probe(struct usb_interface *intf,
 		priv->common.tx = p54u_tx_net2280;
 		priv->upload_fw = p54u_upload_firmware_net2280;
 	}
-	err = p54u_load_firmware(dev);
-	if (err)
-		goto err_free_dev;
-
-	err = priv->upload_fw(dev);
-	if (err)
-		goto err_free_fw;
-
-	p54u_open(dev);
-	err = p54_read_eeprom(dev);
-	p54u_stop(dev);
-	if (err)
-		goto err_free_fw;
-
-	err = p54_register_common(dev, &udev->dev);
-	if (err)
-		goto err_free_fw;
-
+	/* setup and start delayed work to load firmware */
+	INIT_DELAYED_WORK(&priv->firmware_load, p54u_load_firmware);
+	schedule_delayed_work(&priv->firmware_load, HZ);
 	return 0;
-
-err_free_fw:
-	release_firmware(priv->fw);
-
-err_free_dev:
-	p54_free_common(dev);
-	usb_set_intfdata(intf, NULL);
-	usb_put_dev(udev);
-	return err;
 }
 
 static void __devexit p54u_disconnect(struct usb_interface *intf)
diff --git a/drivers/net/wireless/p54/p54usb.h b/drivers/net/wireless/p54/p54usb.h
index ed4034a..6070a21 100644
--- a/drivers/net/wireless/p54/p54usb.h
+++ b/drivers/net/wireless/p54/p54usb.h
@@ -143,6 +143,7 @@ struct p54u_priv {
 	struct sk_buff_head rx_queue;
 	struct usb_anchor submitted;
 	const struct firmware *fw;
+	struct delayed_work firmware_load;
 };
 
 #endif /* P54USB_H */
-- 
1.7.7

[PATCH 4/5] p54pci: Load firmware from work queue and not from probe routine

[Larry Finger]

Drivers that load firmware from their probe routine have problems with the
latest versions of udev as they get timeouts while waiting for user
space to start. The problem is fixed by loading the firmware and starting
mac80211 from a delayed_work queue. By using this method, most of the
original code is preserved.

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---

This patch is compile tested only.

Larry
---
 drivers/net/wireless/p54/p54pci.c |   66 ++++++++++++++++++++++--------------
 drivers/net/wireless/p54/p54pci.h |    1 +
 2 files changed, 41 insertions(+), 26 deletions(-)

diff --git a/drivers/net/wireless/p54/p54pci.c b/drivers/net/wireless/p54/p54pci.c
index b1f51a2..7e4188a 100644
--- a/drivers/net/wireless/p54/p54pci.c
+++ b/drivers/net/wireless/p54/p54pci.c
@@ -488,6 +488,43 @@ static int p54p_open(struct ieee80211_hw *dev)
 	return 0;
 }
 
+static void p54p_load_firmware(struct work_struct *work)
+{
+	struct p54p_priv *priv = container_of(to_delayed_work(work),
+				 struct p54p_priv, firmware_load);
+	struct ieee80211_hw *hw = pci_get_drvdata(priv->pdev);
+	struct device *dev = &priv->pdev->dev;
+	int err;
+
+	err = request_firmware(&priv->firmware, "isl3886pci",
+			       &priv->pdev->dev);
+	if (err) {
+		dev_err(dev, "Cannot find firmware (isl3886pci)\n");
+		err = request_firmware(&priv->firmware, "isl3886",
+				       &priv->pdev->dev);
+		if (err) {
+			dev_err(dev, "Cannot find firmware (isl3886pci)\n");
+			goto err_free_firmware;
+		}
+	}
+
+	err = p54p_open(hw);
+	if (err)
+		goto err_free_firmware;
+	err = p54_read_eeprom(hw);
+	p54p_stop(hw);
+	if (err)
+		goto err_free_firmware;
+
+	err = p54_register_common(hw, dev);
+	if (err)
+		goto err_free_firmware;
+	return;
+
+err_free_firmware:
+	release_firmware(priv->firmware);
+}
+
 static int __devinit p54p_probe(struct pci_dev *pdev,
 				const struct pci_device_id *id)
 {
@@ -561,35 +598,12 @@ static int __devinit p54p_probe(struct pci_dev *pdev,
 	spin_lock_init(&priv->lock);
 	tasklet_init(&priv->tasklet, p54p_tasklet, (unsigned long)dev);
 
-	err = request_firmware(&priv->firmware, "isl3886pci",
-			       &priv->pdev->dev);
-	if (err) {
-		dev_err(&pdev->dev, "Cannot find firmware (isl3886pci)\n");
-		err = request_firmware(&priv->firmware, "isl3886",
-				       &priv->pdev->dev);
-		if (err)
-			goto err_free_common;
-	}
-
-	err = p54p_open(dev);
-	if (err)
-		goto err_free_common;
-	err = p54_read_eeprom(dev);
-	p54p_stop(dev);
-	if (err)
-		goto err_free_common;
-
-	err = p54_register_common(dev, &pdev->dev);
-	if (err)
-		goto err_free_common;
+	/* setup and start delayed work to load firmware */
+	INIT_DELAYED_WORK(&priv->firmware_load, p54p_load_firmware);
+	schedule_delayed_work(&priv->firmware_load, HZ);
 
 	return 0;
 
- err_free_common:
-	release_firmware(priv->firmware);
-	pci_free_consistent(pdev, sizeof(*priv->ring_control),
-			    priv->ring_control, priv->ring_control_dma);
-
  err_iounmap:
 	iounmap(priv->map);
 
diff --git a/drivers/net/wireless/p54/p54pci.h b/drivers/net/wireless/p54/p54pci.h
index 7aa509f..de8edff 100644
--- a/drivers/net/wireless/p54/p54pci.h
+++ b/drivers/net/wireless/p54/p54pci.h
@@ -105,6 +105,7 @@ struct p54p_priv {
 	struct sk_buff *tx_buf_data[32];
 	struct sk_buff *tx_buf_mgmt[4];
 	struct completion boot_comp;
+	struct delayed_work firmware_load;
 };
 
 #endif /* P54USB_H */
-- 
1.7.7

Re: [PATCH 4/5] p54pci: Load firmware from work queue and not from probe routine

[Larry Finger]

On 02/13/2012 01:37 PM, Larry Finger wrote:
> Drivers that load firmware from their probe routine have problems with the
> latest versions of udev as they get timeouts while waiting for user
> space to start. The problem is fixed by loading the firmware and starting
> mac80211 from a delayed_work queue. By using this method, most of the
> original code is preserved.
>
> Signed-off-by: Larry Finger<Larry.Finger@lwfinger.net>
> ---
>
> This patch is compile tested only.
>
> Larry

Has anyone tested this patch? I would like to submit this series.

Thanks,

Larry

Re: [PATCH 4/5] p54pci: Load firmware from work queue and not from probe routine

[Christian Lamparter]

On Wednesday, February 29, 2012 04:59:14 PM Larry Finger wrote:
> On 02/13/2012 01:37 PM, Larry Finger wrote:
> > Drivers that load firmware from their probe routine have problems with the
> > latest versions of udev as they get timeouts while waiting for user
> > space to start. The problem is fixed by loading the firmware and starting
> > mac80211 from a delayed_work queue. By using this method, most of the
> > original code is preserved.
> >
> > Signed-off-by: Larry Finger<Larry.Finger@lwfinger.net>
> > ---
> >
> > This patch is compile tested only.
> >
> > Larry
> 
> Has anyone tested this patch? I would like to submit this series.
only briefly, but I'm not sure if the workqueue approach is adequate. On
resume, I get WARNINGs and the card fails to resume because the firmware
file can't be loaded :(.

WARNING: at drivers/base/firmware_class.c:538 _request_firmware+0x3ae/0x3e0()
Pid: 1083, comm: kworker/0:3 Tainted:G		O 3.3.0-rc5-wl+
Call Trace:
	[<c102c658>] ? wanr_slowpath_common+0x78/0xb0
	[<c12c728e>] ? _request_firmware+0x3ae/0x3e0
	[<c12c728e>] ? _requrst_firmware+0x3ae/0x3e0
	[<c102c6a9>] ? warn_slowpath_common+0x19/0x20
	[<c12c728e>] ? _request_firmware+0x3ae/0x3e0
	[<c12c7347>] ? request_firmware+0x17/0x20
	[<f86v1dc7>] ? p54p_load_firmware+0x47/0x120 [p54pci]
	[...]

Regards,
	Chr

BTW: I will take a closer look at it over the weekend.

Re: [PATCH 4/5] p54pci: Load firmware from work queue and not from probe routine

[Larry Finger]

On 02/29/2012 11:43 AM, Christian Lamparter wrote:
> On Wednesday, February 29, 2012 04:59:14 PM Larry Finger wrote:
>> On 02/13/2012 01:37 PM, Larry Finger wrote:
>>> Drivers that load firmware from their probe routine have problems with the
>>> latest versions of udev as they get timeouts while waiting for user
>>> space to start. The problem is fixed by loading the firmware and starting
>>> mac80211 from a delayed_work queue. By using this method, most of the
>>> original code is preserved.
>>>
>>> Signed-off-by: Larry Finger<Larry.Finger@lwfinger.net>
>>> ---
>>>
>>> This patch is compile tested only.
>>>
>>> Larry
>>
>> Has anyone tested this patch? I would like to submit this series.
> only briefly, but I'm not sure if the workqueue approach is adequate. On
> resume, I get WARNINGs and the card fails to resume because the firmware
> file can't be loaded :(.
>
> WARNING: at drivers/base/firmware_class.c:538 _request_firmware+0x3ae/0x3e0()
> Pid: 1083, comm: kworker/0:3 Tainted:G		O 3.3.0-rc5-wl+
> Call Trace:
> 	[<c102c658>] ? wanr_slowpath_common+0x78/0xb0
> 	[<c12c728e>] ? _request_firmware+0x3ae/0x3e0
> 	[<c12c728e>] ? _requrst_firmware+0x3ae/0x3e0
> 	[<c102c6a9>] ? warn_slowpath_common+0x19/0x20
> 	[<c12c728e>] ? _request_firmware+0x3ae/0x3e0
> 	[<c12c7347>] ? request_firmware+0x17/0x20
> 	[<f86v1dc7>] ? p54p_load_firmware+0x47/0x120 [p54pci]
> 	[...]
>
> Regards,
> 	Chr
>
> BTW: I will take a closer look at it over the weekend.

Does the driver come back through the probe routine on resume? If not, the 
firmware should have been cached and not needed reloading. I'll have to look at 
the driver structure some more. I likely screwed that up.

Larry

Re: [PATCH 4/5] p54pci: Load firmware from work queue and not from probe routine

[Christian Lamparter]

On Wednesday, February 29, 2012 07:15:31 PM Larry Finger wrote:
> On 02/29/2012 11:43 AM, Christian Lamparter wrote:
> > On Wednesday, February 29, 2012 04:59:14 PM Larry Finger wrote:
> >> On 02/13/2012 01:37 PM, Larry Finger wrote:
> >> Has anyone tested this patch? I would like to submit this series.
> > only briefly, but I'm not sure if the workqueue approach is adequate. On
> > resume, I get WARNINGs and the card fails to resume because the firmware
> > file can't be loaded :(.
> >
> > WARNING: at drivers/base/firmware_class.c:538 _request_firmware+0x3ae/0x3e0()
> > Pid: 1083, comm: kworker/0:3 Tainted:G		O 3.3.0-rc5-wl+
> > Call Trace:
> > 	[<c102c658>] ? wanr_slowpath_common+0x78/0xb0
> > 	[<c12c728e>] ? _request_firmware+0x3ae/0x3e0
> > 	[<c12c728e>] ? _requrst_firmware+0x3ae/0x3e0
> > 	[<c102c6a9>] ? warn_slowpath_common+0x19/0x20
> > 	[<c12c728e>] ? _request_firmware+0x3ae/0x3e0
> > 	[<c12c7347>] ? request_firmware+0x17/0x20
> > 	[<f86v1dc7>] ? p54p_load_firmware+0x47/0x120 [p54pci]
> > 	[...]
> >
> > Regards,
> > 	Chr
> >
> > BTW: I will take a closer look at it over the weekend.
> 
> Does the driver come back through the probe routine on resume?
Yes. I've got a pcmcia card [in fact I have two :D]. The pcmcia
subsystem goes to a rebind on resume.

Regards,
	Christian

[RFC/RFT 5/5] p54spi: Load firmware from work queue and not from probe routine

[Larry Finger]

Drivers that load firmware from their probe routine have problems with the
latest versions of udev as they get timeouts while waiting for user
space to start. The problem is fixed by loading the firmware and starting
mac80211 from a delayed_work queue. By using this method, most of the
original code is preserved.

Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
---
This one is compile-tested only.

Larry
---
 drivers/net/wireless/p54/p54spi.c |   38 ++++++++++++++++++++++++++----------
 drivers/net/wireless/p54/p54spi.h |    1 +
 2 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/drivers/net/wireless/p54/p54spi.c b/drivers/net/wireless/p54/p54spi.c
index 7faed62..87c2634 100644
--- a/drivers/net/wireless/p54/p54spi.c
+++ b/drivers/net/wireless/p54/p54spi.c
@@ -595,6 +595,30 @@ static void p54spi_op_stop(struct ieee80211_hw *dev)
 	cancel_work_sync(&priv->work);
 }
 
+static void p54s_load_firmware(struct work_struct *work)
+{
+	struct p54s_priv *priv = container_of(to_delayed_work(work),
+				 struct p54s_priv, firmware_load);
+	struct ieee80211_hw *hw = priv->hw;
+	int ret;
+
+	ret = p54spi_request_firmware(hw);
+	if (ret < 0)
+		goto err_free_common;
+
+	ret = p54spi_request_eeprom(hw);
+	if (ret)
+		goto err_free_common;
+
+	ret = p54_register_common(hw, &priv->spi->dev);
+	if (ret)
+		goto err_free_common;
+	return;
+
+err_free_common:
+	dev_err(&priv->spi->dev, "Failed to read firmware\n");
+}
+
 static int __devinit p54spi_probe(struct spi_device *spi)
 {
 	struct p54s_priv *priv = NULL;
@@ -658,17 +682,9 @@ static int __devinit p54spi_probe(struct spi_device *spi)
 	priv->common.stop = p54spi_op_stop;
 	priv->common.tx = p54spi_op_tx;
 
-	ret = p54spi_request_firmware(hw);
-	if (ret < 0)
-		goto err_free_common;
-
-	ret = p54spi_request_eeprom(hw);
-	if (ret)
-		goto err_free_common;
-
-	ret = p54_register_common(hw, &priv->spi->dev);
-	if (ret)
-		goto err_free_common;
+	/* setup and start delayed work to load firmware */
+	INIT_DELAYED_WORK(&priv->firmware_load, p54s_load_firmware);
+	schedule_delayed_work(&priv->firmware_load, HZ);
 
 	return 0;
 
diff --git a/drivers/net/wireless/p54/p54spi.h b/drivers/net/wireless/p54/p54spi.h
index dfaa62a..5f7714b 100644
--- a/drivers/net/wireless/p54/p54spi.h
+++ b/drivers/net/wireless/p54/p54spi.h
@@ -120,6 +120,7 @@ struct p54s_priv {
 
 	enum fw_state fw_state;
 	const struct firmware *firmware;
+	struct delayed_work firmware_load;
 };
 
 #endif /* P54SPI_H */
-- 
1.7.7

Re: [RFC/RFT 5/5] p54spi: Load firmware from work queue and not from probe routine

[Michael Büsch]

On Mon, 13 Feb 2012 13:37:06 -0600
Larry Finger <Larry.Finger@lwfinger.net> wrote:

> Drivers that load firmware from their probe routine have problems with the
> latest versions of udev as they get timeouts while waiting for user
> space to start. The problem is fixed by loading the firmware and starting
> mac80211 from a delayed_work queue. By using this method, most of the
> original code is preserved.
> 
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
> This one is compile-tested only.
> 
> Larry
> ---
>  drivers/net/wireless/p54/p54spi.c |   38 ++++++++++++++++++++++++++----------
>  drivers/net/wireless/p54/p54spi.h |    1 +
>  2 files changed, 28 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/net/wireless/p54/p54spi.c b/drivers/net/wireless/p54/p54spi.c
> index 7faed62..87c2634 100644
> --- a/drivers/net/wireless/p54/p54spi.c
> +++ b/drivers/net/wireless/p54/p54spi.c
> @@ -595,6 +595,30 @@ static void p54spi_op_stop(struct ieee80211_hw *dev)
>  	cancel_work_sync(&priv->work);
>  }
>  
> +static void p54s_load_firmware(struct work_struct *work)
> +{
> +	struct p54s_priv *priv = container_of(to_delayed_work(work),
> +				 struct p54s_priv, firmware_load);
> +	struct ieee80211_hw *hw = priv->hw;
> +	int ret;
> +
> +	ret = p54spi_request_firmware(hw);
> +	if (ret < 0)
> +		goto err_free_common;
> +
> +	ret = p54spi_request_eeprom(hw);
> +	if (ret)
> +		goto err_free_common;
> +
> +	ret = p54_register_common(hw, &priv->spi->dev);
> +	if (ret)
> +		goto err_free_common;
> +	return;
> +
> +err_free_common:
> +	dev_err(&priv->spi->dev, "Failed to read firmware\n");
> +}
> +
>  static int __devinit p54spi_probe(struct spi_device *spi)
>  {
>  	struct p54s_priv *priv = NULL;
> @@ -658,17 +682,9 @@ static int __devinit p54spi_probe(struct spi_device *spi)
>  	priv->common.stop = p54spi_op_stop;
>  	priv->common.tx = p54spi_op_tx;
>  
> -	ret = p54spi_request_firmware(hw);
> -	if (ret < 0)
> -		goto err_free_common;
> -
> -	ret = p54spi_request_eeprom(hw);
> -	if (ret)
> -		goto err_free_common;
> -
> -	ret = p54_register_common(hw, &priv->spi->dev);
> -	if (ret)
> -		goto err_free_common;
> +	/* setup and start delayed work to load firmware */
> +	INIT_DELAYED_WORK(&priv->firmware_load, p54s_load_firmware);
> +	schedule_delayed_work(&priv->firmware_load, HZ);

Why delay it one second?
That seems arbitrary. Just use a non-delayed workqueue. That should be good enough.

>  
>  	return 0;
>  
> diff --git a/drivers/net/wireless/p54/p54spi.h b/drivers/net/wireless/p54/p54spi.h
> index dfaa62a..5f7714b 100644
> --- a/drivers/net/wireless/p54/p54spi.h
> +++ b/drivers/net/wireless/p54/p54spi.h
> @@ -120,6 +120,7 @@ struct p54s_priv {
>  
>  	enum fw_state fw_state;
>  	const struct firmware *firmware;
> +	struct delayed_work firmware_load;
>  };
>  
>  #endif /* P54SPI_H */

I think we have to cancel the work in the remove handler. Just in case the device was removed
before fw load wq could run.

-- 
Greetings, Michael.

Re: [RFC/RFT 5/5] p54spi: Load firmware from work queue and not from probe routine

[Larry Finger]

On 02/13/2012 01:37 PM, Larry Finger wrote:
> Drivers that load firmware from their probe routine have problems with the
> latest versions of udev as they get timeouts while waiting for user
> space to start. The problem is fixed by loading the firmware and starting
> mac80211 from a delayed_work queue. By using this method, most of the
> original code is preserved.
>
> Signed-off-by: Larry Finger<Larry.Finger@lwfinger.net>
> ---
> This one is compile-tested only.
>
> Larry
> ---

Any testing done here?

Larry

>   drivers/net/wireless/p54/p54spi.c |   38 ++++++++++++++++++++++++++----------
>   drivers/net/wireless/p54/p54spi.h |    1 +
>   2 files changed, 28 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/wireless/p54/p54spi.c b/drivers/net/wireless/p54/p54spi.c
> index 7faed62..87c2634 100644
> --- a/drivers/net/wireless/p54/p54spi.c
> +++ b/drivers/net/wireless/p54/p54spi.c
> @@ -595,6 +595,30 @@ static void p54spi_op_stop(struct ieee80211_hw *dev)
>   	cancel_work_sync(&priv->work);
>   }
>
> +static void p54s_load_firmware(struct work_struct *work)
> +{
> +	struct p54s_priv *priv = container_of(to_delayed_work(work),
> +				 struct p54s_priv, firmware_load);
> +	struct ieee80211_hw *hw = priv->hw;
> +	int ret;
> +
> +	ret = p54spi_request_firmware(hw);
> +	if (ret<  0)
> +		goto err_free_common;
> +
> +	ret = p54spi_request_eeprom(hw);
> +	if (ret)
> +		goto err_free_common;
> +
> +	ret = p54_register_common(hw,&priv->spi->dev);
> +	if (ret)
> +		goto err_free_common;
> +	return;
> +
> +err_free_common:
> +	dev_err(&priv->spi->dev, "Failed to read firmware\n");
> +}
> +
>   static int __devinit p54spi_probe(struct spi_device *spi)
>   {
>   	struct p54s_priv *priv = NULL;
> @@ -658,17 +682,9 @@ static int __devinit p54spi_probe(struct spi_device *spi)
>   	priv->common.stop = p54spi_op_stop;
>   	priv->common.tx = p54spi_op_tx;
>
> -	ret = p54spi_request_firmware(hw);
> -	if (ret<  0)
> -		goto err_free_common;
> -
> -	ret = p54spi_request_eeprom(hw);
> -	if (ret)
> -		goto err_free_common;
> -
> -	ret = p54_register_common(hw,&priv->spi->dev);
> -	if (ret)
> -		goto err_free_common;
> +	/* setup and start delayed work to load firmware */
> +	INIT_DELAYED_WORK(&priv->firmware_load, p54s_load_firmware);
> +	schedule_delayed_work(&priv->firmware_load, HZ);
>
>   	return 0;
>
> diff --git a/drivers/net/wireless/p54/p54spi.h b/drivers/net/wireless/p54/p54spi.h
> index dfaa62a..5f7714b 100644
> --- a/drivers/net/wireless/p54/p54spi.h
> +++ b/drivers/net/wireless/p54/p54spi.h
> @@ -120,6 +120,7 @@ struct p54s_priv {
>
>   	enum fw_state fw_state;
>   	const struct firmware *firmware;
> +	struct delayed_work firmware_load;
>   };
>
>   #endif /* P54SPI_H */

Re: [RFC/RFT 5/5] p54spi: Load firmware from work queue and not from probe routine

[Max Filippov]

>> Drivers that load firmware from their probe routine have problems with the
>> latest versions of udev as they get timeouts while waiting for user
>> space to start. The problem is fixed by loading the firmware and starting
>> mac80211 from a delayed_work queue. By using this method, most of the
>> original code is preserved.
>>
>> Signed-off-by: Larry Finger<Larry.Finger@lwfinger.net>
>> ---
>> This one is compile-tested only.
>>
>> Larry
>> ---
>
>
> Any testing done here?

insmod immediately followed by rmmod causes NULL pointer dereference.
The same always happens on rmmode when the firmware image is absent:

[   52.482696] calling  p54spi_init+0x0/0x30 [p54spi] @ 941
[   52.486053] initcall p54spi_init+0x0/0x30 [p54spi] returned 0 after
3071 usecs
[   53.522430] p54spi spi2.0: request_firmware() failed: -2
[   53.522521] p54spi spi2.0: Failed to read firmware
[   56.337036] Unable to handle kernel NULL pointer dereference at
virtual address 00000044
[   56.337188] pgd = c6c2c000
[   56.337219] [00000044] *pgd=87b07831, *pte=00000000, *ppte=00000000
[   56.337341] Internal error: Oops: 17 [#1] PREEMPT
[   56.337402] Modules linked in: p54spi(-)
[   56.337493] CPU: 0    Not tainted  (3.3.0-rc3-11564-g1465640 #12)
[   56.337585] PC is at drain_workqueue+0x10/0x1b4
[   56.337677] LR is at drain_workqueue+0x10/0x1b4
[   56.337738] pc : [<c0043340>]    lr : [<c0043340>]    psr: 80000013
[   56.337768] sp : c79e3eb8  ip : 00000000  fp : bed7cc1c
[   56.337890] r10: 00000000  r9 : c79e2000  r8 : c0012a48
[   56.337951] r7 : c7863634  r6 : c7863600  r5 : bf00115c  r4 : 00000000
[   56.338043] r3 : 00000000  r2 : 00000000  r1 : c040c844  r0 : 00000001
[   56.338134] Flags: Nzcv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
[   56.338226] Control: 00c5387d  Table: 86c2c000  DAC: 00000015
[   56.338317] Process rmmod (pid: 950, stack limit = 0xc79e2268)
[   56.338378] Stack: (0xc79e3eb8 to 0xc79e4000)
[   56.338470] 3ea0:
    00000000 bf00115c
[   56.338592] 3ec0: c7863600 c7863634 c0012a48 c79e2000 00000000
c00434f0 c7816300 bf00115c
[   56.338714] 3ee0: c7863600 c0278c38 c7816fa0 bf000ba0 c7863600
bf001ff8 bf001ff8 c01aa53c
[   56.338836] 3f00: c01aa524 c0195808 c7863600 c79e2000 bf001ff8
c01958fc bf001ff8 00000000
[   56.338989] 3f20: c03e3d44 bed7cbf8 c0012a48 c0194c88 bf002030
00000000 00000013 c0061dc8
[   56.339111] 3f40: c79cdf20 73343570 b6006970 c008577c ffffffff
c6ca0180 c79cdf20 c0086774
[   56.339233] 3f60: b6f23000 b6f22000 bed7cc1c c01466f0 c6ca01b4
00000000 b6f22000 00ca0180
[   56.339355] 3f80: bf002030 00000880 c79e3f8c 00000000 b6f233ff
00000880 bed7cbf8 bed7ce97
[   56.339508] 3fa0: 00000081 c00128a0 00000880 bed7cbf8 bed7cbf8
00000880 bed7cbec 00000000
[   56.339630] 3fc0: 00000880 bed7cbf8 bed7ce97 00000081 00000001
00000001 00011fe4 bed7cc1c
[   56.339752] 3fe0: 00000880 bed7cbf8 00009004 b6e908bc 60000010
bed7cbf8 00000000 00000000
[   56.339904] [<c0043340>] (drain_workqueue+0x10/0x1b4) from
[<c00434f0>] (destroy_workqueue+0xc/0x150)
[   56.340057] [<c00434f0>] (destroy_workqueue+0xc/0x150) from
[<c0278c38>] (ieee80211_unregister_hw+0xbc/0xe4)
[   56.340209] [<c0278c38>] (ieee80211_unregister_hw+0xbc/0xe4) from
[<bf000ba0>] (p54spi_remove+0x1c/0x58 [p54spi])
[   56.340393] [<bf000ba0>] (p54spi_remove+0x1c/0x58 [p54spi]) from
[<c01aa53c>] (spi_drv_remove+0x18/0x1c)
[   56.340576] [<c01aa53c>] (spi_drv_remove+0x18/0x1c) from
[<c0195808>] (__device_release_driver+0x7c/0xbc)
[   56.340728] [<c0195808>] (__device_release_driver+0x7c/0xbc) from
[<c01958fc>] (driver_detach+0xb4/0xdc)
[   56.340850] [<c01958fc>] (driver_detach+0xb4/0xdc) from
[<c0194c88>] (bus_remove_driver+0x8c/0xb4)
[   56.341003] [<c0194c88>] (bus_remove_driver+0x8c/0xb4) from
[<c0061dc8>] (sys_delete_module+0x1c8/0x234)
[   56.341186] [<c0061dc8>] (sys_delete_module+0x1c8/0x234) from
[<c00128a0>] (ret_fast_syscall+0x0/0x30)
[   56.341308] Code: e92d47f0 e1a04000 e3a00001 eb002dbb (e5943044)
[   56.341552] ---[ end trace 79a2a071aae86862 ]---
[   56.341644] note: rmmod[950] exited with preempt_count 1

-- 
Thanks.
-- Max

Re: [RFC/RFT 5/5] p54spi: Load firmware from work queue and not from probe routine

[Larry Finger]

On 02/29/2012 01:54 PM, Max Filippov wrote:
>>> Drivers that load firmware from their probe routine have problems with the
>>> latest versions of udev as they get timeouts while waiting for user
>>> space to start. The problem is fixed by loading the firmware and starting
>>> mac80211 from a delayed_work queue. By using this method, most of the
>>> original code is preserved.
>>>
>>> Signed-off-by: Larry Finger<Larry.Finger@lwfinger.net>
>>> ---
>>> This one is compile-tested only.
>>>
>>> Larry
>>> ---
>>
>>
>> Any testing done here?
>
> insmod immediately followed by rmmod causes NULL pointer dereference.
> The same always happens on rmmode when the firmware image is absent:

Thanks for testing.

Larry

Re: [RFC/RFT 0/5] Firmware load change for various wireless drivers

[Johannes Berg]

On Mon, 2012-02-13 at 13:37 -0600, Larry Finger wrote:
> These patches change 5 drivers that are now loading firmware from the probe
> routine. As each of them has the possibility of loading more than one
> file, the method of using request_firmware_nowait() has some difficulty,
> as it gets duplicate names in sysfs when the requests are launched in
> parallel.

Why not simply start off with one, and then when that returns
successfully request the other ones?

> When the callback routine is used to launch a second request,
> the structure gets messy, particularly with b43legacy, which loads 4
> firmware files for some hardware. My solution is to create a delayed work
> queue that is started in the probe routine with a delay of one second. The
> routine that is triggered does the normal request_firmware() calls
> and starts mac80211 when the firmware is available.

I suppose this works, but I'd be a little worried that when the driver
is built into the kernel it doesn't help much. I'm asking the udev
people to not answer async loads while in initramfs, but they'd still
give you a negative answer here, and they won't be able to tell the
difference between a synchronous request from a work item (which doesn't
block boot) and a synchronous request from probe (which does block
booting).

johannes

Re: [RFC/RFT 0/5] Firmware load change for various wireless drivers

[Larry Finger]

On 02/14/2012 04:56 AM, Johannes Berg wrote:
> On Mon, 2012-02-13 at 13:37 -0600, Larry Finger wrote:
>> These patches change 5 drivers that are now loading firmware from the probe
>> routine. As each of them has the possibility of loading more than one
>> file, the method of using request_firmware_nowait() has some difficulty,
>> as it gets duplicate names in sysfs when the requests are launched in
>> parallel.
>
> Why not simply start off with one, and then when that returns
> successfully request the other ones?

I have code that does that, and it is a reasonable solution when the driver 
loads only two files, or one and an alternate.

>> When the callback routine is used to launch a second request,
>> the structure gets messy, particularly with b43legacy, which loads 4
>> firmware files for some hardware. My solution is to create a delayed work
>> queue that is started in the probe routine with a delay of one second. The
>> routine that is triggered does the normal request_firmware() calls
>> and starts mac80211 when the firmware is available.
>
> I suppose this works, but I'd be a little worried that when the driver
> is built into the kernel it doesn't help much. I'm asking the udev
> people to not answer async loads while in initramfs, but they'd still
> give you a negative answer here, and they won't be able to tell the
> difference between a synchronous request from a work item (which doesn't
> block boot) and a synchronous request from probe (which does block
> booting).

I asked a variant of this question on LKML and the driverdevel ML, but got no 
answer.

Unless I get a definitive answer, I'll have to go back to the chain loading, no 
matter how messy.

Larry

Re: [RFC/RFT 0/5] Firmware load change for various wireless drivers

[Johannes Berg]

On Tue, 2012-02-14 at 10:34 -0600, Larry Finger wrote:

> >> When the callback routine is used to launch a second request,
> >> the structure gets messy, particularly with b43legacy, which loads 4
> >> firmware files for some hardware. My solution is to create a delayed work
> >> queue that is started in the probe routine with a delay of one second. The
> >> routine that is triggered does the normal request_firmware() calls
> >> and starts mac80211 when the firmware is available.
> >
> > I suppose this works, but I'd be a little worried that when the driver
> > is built into the kernel it doesn't help much. I'm asking the udev
> > people to not answer async loads while in initramfs, but they'd still
> > give you a negative answer here, and they won't be able to tell the
> > difference between a synchronous request from a work item (which doesn't
> > block boot) and a synchronous request from probe (which does block
> > booting).
> 
> I asked a variant of this question on LKML and the driverdevel ML, but got no 
> answer.
> 
> Unless I get a definitive answer, I'll have to go back to the chain loading, no 
> matter how messy.

I'm not even sure they've done *anything* until now, so I'm not sure it
matters. I believe the behavioural change that prompted these changes
will actually make it easier to implement the delay in a matter that
covers both this and async loading, so you may well be safe, I haven't
really understood the full details of the change that prompted all this.

johannes

Re: [RFC/RFT 0/5] Firmware load change for various wireless drivers

[Larry Finger]

On 02/14/2012 10:37 AM, Johannes Berg wrote:
>
> I'm not even sure they've done *anything* until now, so I'm not sure it
> matters. I believe the behavioural change that prompted these changes
> will actually make it easier to implement the delay in a matter that
> covers both this and async loading, so you may well be safe, I haven't
> really understood the full details of the change that prompted all this.

I do not think the changes have been merged into any distro yet, but I have 
gotten feedback converning udev timeouts from the rtlwifi drivers when booting. 
That is the reason I'm trying to get ahead of the curve now.

Larry