[RFC/RFT] p54spi: Convert driver to use asynchronous firmware loading

[RFC/RFT] p54spi: Convert driver to use asynchronous firmware loading

[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 using request_firmware_nowait()
and delaying the start of mac80211 until the firmware is loaded.

To prevent the possibility of the driver being unloaded while the firmware
loading callback is still active, a completion queue entry is used.

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

This conversion of p54spi to use asynchronous firmware loading is based
on the method used in p54usb. As I do not have the hardware, it is only
compile tested. I would appreciate any feedback from people that have the
hardware.

Larry
---

Index: wireless-testing-new/drivers/net/wireless/p54/p54spi.c
===================================================================
--- wireless-testing-new.orig/drivers/net/wireless/p54/p54spi.c
+++ wireless-testing-new/drivers/net/wireless/p54/p54spi.c
@@ -163,25 +163,41 @@ static int p54spi_spi_write_dma(struct p
 	return 0;
 }
 
-static int p54spi_request_firmware(struct ieee80211_hw *dev)
+
+static void p54s_load_firmware_cb(const struct firmware *firmware,
+				  void *context)
 {
+	struct ieee80211_hw *dev = context;
 	struct p54s_priv *priv = dev->priv;
 	int ret;
 
-	/* FIXME: should driver use it's own struct device? */
-	ret = request_firmware(&priv->firmware, "3826.arm", &priv->spi->dev);
-
-	if (ret < 0) {
-		dev_err(&priv->spi->dev, "request_firmware() failed: %d", ret);
-		return ret;
+	if (!firmware) {
+		/* alternate firmware not found */
+		dev_err(&priv->spi->dev, "Firmware loading failed\n");
+		return;
 	}
+	complete(&priv->fw_loaded);
+	priv->firmware = firmware;
 
 	ret = p54_parse_firmware(dev, priv->firmware);
-	if (ret) {
+	if (ret)
 		release_firmware(priv->firmware);
+}
+
+static int p54spi_request_firmware(struct ieee80211_hw *dev)
+{
+	struct p54s_priv *priv = dev->priv;
+	int ret;
+
+	init_completion(&priv->fw_loaded);
+	/* FIXME: should driver use it's own struct device? */
+	ret = request_firmware_nowait(THIS_MODULE, 1, "3826.arm",
+				      &priv->spi->dev, GFP_KERNEL, dev,
+				      p54s_load_firmware_cb);
+	if (ret < 0) {
+		dev_err(&priv->spi->dev, "request_firmware() failed: %d", ret);
 		return ret;
 	}
-
 	return 0;
 }
 
@@ -681,6 +697,7 @@ static int __devexit p54spi_remove(struc
 {
 	struct p54s_priv *priv = dev_get_drvdata(&spi->dev);
 
+	wait_for_completion(&priv->fw_loaded);
 	p54_unregister_common(priv->hw);
 
 	free_irq(gpio_to_irq(p54spi_gpio_irq), spi);
Index: wireless-testing-new/drivers/net/wireless/p54/p54spi.h
===================================================================
--- wireless-testing-new.orig/drivers/net/wireless/p54/p54spi.h
+++ wireless-testing-new/drivers/net/wireless/p54/p54spi.h
@@ -120,6 +120,7 @@ struct p54s_priv {
 
 	enum fw_state fw_state;
 	const struct firmware *firmware;
+	struct completion fw_loaded;
 };
 
 #endif /* P54SPI_H */

Re: [RFC/RFT] p54spi: Convert driver to use asynchronous firmware loading

[Michael Büsch]

On Thu, 09 Feb 2012 19:05:37 -0600
Larry Finger <Larry.Finger@lwfinger.net> wrote:

> Index: wireless-testing-new/drivers/net/wireless/p54/p54spi.c
> ===================================================================
> --- wireless-testing-new.orig/drivers/net/wireless/p54/p54spi.c
> +++ wireless-testing-new/drivers/net/wireless/p54/p54spi.c
> @@ -163,25 +163,41 @@ static int p54spi_spi_write_dma(struct p
>  	return 0;
>  }
>  
> -static int p54spi_request_firmware(struct ieee80211_hw *dev)
> +
> +static void p54s_load_firmware_cb(const struct firmware *firmware,
> +				  void *context)
>  {
> +	struct ieee80211_hw *dev = context;
>  	struct p54s_priv *priv = dev->priv;
>  	int ret;
>  
> -	/* FIXME: should driver use it's own struct device? */
> -	ret = request_firmware(&priv->firmware, "3826.arm", &priv->spi->dev);
> -
> -	if (ret < 0) {
> -		dev_err(&priv->spi->dev, "request_firmware() failed: %d", ret);
> -		return ret;
> +	if (!firmware) {
> +		/* alternate firmware not found */
> +		dev_err(&priv->spi->dev, "Firmware loading failed\n");
> +		return;
>  	}
> +	complete(&priv->fw_loaded);
> +	priv->firmware = firmware;
>  
>  	ret = p54_parse_firmware(dev, priv->firmware);
> -	if (ret) {
> +	if (ret)
>  		release_firmware(priv->firmware);
> +}
> +
> +static int p54spi_request_firmware(struct ieee80211_hw *dev)
> +{
> +	struct p54s_priv *priv = dev->priv;
> +	int ret;
> +
> +	init_completion(&priv->fw_loaded);
> +	/* FIXME: should driver use it's own struct device? */
> +	ret = request_firmware_nowait(THIS_MODULE, 1, "3826.arm",
> +				      &priv->spi->dev, GFP_KERNEL, dev,
> +				      p54s_load_firmware_cb);
> +	if (ret < 0) {
> +		dev_err(&priv->spi->dev, "request_firmware() failed: %d", ret);
>  		return ret;
>  	}
> -
>  	return 0;
>  }

I think we need a wait_for_completion in op_start. We need to make sure firmware load is finished.
It might currently work, though, because we have the other fw_comp completion.
Please also rename your new completion to fw_requested or something like this to avoid
confusion with the real fw-loaded completion.

Also, what about eeprom request?

> @@ -681,6 +697,7 @@ static int __devexit p54spi_remove(struc
>  {
>  	struct p54s_priv *priv = dev_get_drvdata(&spi->dev);
>  
> +	wait_for_completion(&priv->fw_loaded);
>  	p54_unregister_common(priv->hw);
>  
>  	free_irq(gpio_to_irq(p54spi_gpio_irq), spi);
> Index: wireless-testing-new/drivers/net/wireless/p54/p54spi.h
> ===================================================================
> --- wireless-testing-new.orig/drivers/net/wireless/p54/p54spi.h
> +++ wireless-testing-new/drivers/net/wireless/p54/p54spi.h
> @@ -120,6 +120,7 @@ struct p54s_priv {
>  
>  	enum fw_state fw_state;
>  	const struct firmware *firmware;
> +	struct completion fw_loaded;
>  };
>  
>  #endif /* P54SPI_H */
> 



-- 
Greetings, Michael.

Re: [RFC/RFT] p54spi: Convert driver to use asynchronous firmware loading

[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 using request_firmware_nowait()
> and delaying the start of mac80211 until the firmware is loaded.
>
> To prevent the possibility of the driver being unloaded while the firmware
> loading callback is still active, a completion queue entry is used.
>
> Signed-off-by: Larry Finger <Larry.Finger@lwfinger.net>
> ---
>
> This conversion of p54spi to use asynchronous firmware loading is based
> on the method used in p54usb. As I do not have the hardware, it is only
> compile tested. I would appreciate any feedback from people that have the
> hardware.

Hi, Larry.

Are there any prerequisites for this patch?
For now I'm applying it to the linux-omap ToT and having the following:

<7>[    2.968048] calling  p54spi_init+0x0/0x30 [p54spi] @ 456
<4>[    2.973937] ------------[ cut here ]------------
<4>[    2.974090] WARNING: at
/home/dumb/ws/osll/n8x0/linux-3.3/linux-omap-2.6/fs/sysfs/dir.c:481
sysfs_add_one+0x74/0x9c()
<4>[    2.974212] sysfs: cannot create duplicate filename
'/devices/platform/omap/omap2_mcspi.2/spi_master/spi2/spi2.0/firmware/spi2.0'
<4>[    2.974334] Modules linked in: p54spi(+)
<4>[    2.974487] [<c00182bc>] (unwind_backtrace+0x0/0xe4) from
[<c002ef0c>] (warn_slowpath_common+0x4c/0x64)
<4>[    2.974670] [<c002ef0c>] (warn_slowpath_common+0x4c/0x64) from
[<c002efa4>] (warn_slowpath_fmt+0x2c/0x3c)
<4>[    2.974792] [<c002efa4>] (warn_slowpath_fmt+0x2c/0x3c) from
[<c00e5888>] (sysfs_add_one+0x74/0x9c)
<4>[    2.974945] [<c00e5888>] (sysfs_add_one+0x74/0x9c) from
[<c00e6290>] (create_dir+0x5c/0xac)
<4>[    2.975067] [<c00e6290>] (create_dir+0x5c/0xac) from
[<c00e6394>] (sysfs_create_dir+0xb4/0xcc)
<4>[    2.975219] [<c00e6394>] (sysfs_create_dir+0xb4/0xcc) from
[<c0142ce8>] (kobject_add_internal+0xe4/0x1c0)
<4>[    2.975372] [<c0142ce8>] (kobject_add_internal+0xe4/0x1c0) from
[<c0142f30>] (kobject_add+0x44/0x54)
<4>[    2.975494] [<c0142f30>] (kobject_add+0x44/0x54) from
[<c0193c98>] (device_add+0xe0/0x530)
<4>[    2.975646] [<c0193c98>] (device_add+0xe0/0x530) from
[<c019f184>] (_request_firmware+0x190/0x394)
<4>[    2.975799] [<c019f184>] (_request_firmware+0x190/0x394) from
[<c019f3c8>] (request_firmware_work_func+0x40/0x78)
<4>[    2.975952] [<c019f3c8>] (request_firmware_work_func+0x40/0x78)
from [<c00473cc>] (kthread+0x88/0x94)
<4>[    2.976135] [<c00473cc>] (kthread+0x88/0x94) from [<c0013844>]
(kernel_thread_exit+0x0/0x8)
<4>[    2.976226] ---[ end trace 181b959cc0c448db ]---
<3>[    2.976318] kobject_add_internal failed for spi2.0 with -EEXIST,
don't try to register things with the same name in the same directory.
<4>[    2.976501] [<c00182bc>] (unwind_backtrace+0x0/0xe4) from
[<c0142d84>] (kobject_add_internal+0x180/0x1c0)
<4>[    2.976654] [<c0142d84>] (kobject_add_internal+0x180/0x1c0) from
[<c0142f30>] (kobject_add+0x44/0x54)
<4>[    2.976776] [<c0142f30>] (kobject_add+0x44/0x54) from
[<c0193c98>] (device_add+0xe0/0x530)
<4>[    2.976928] [<c0193c98>] (device_add+0xe0/0x530) from
[<c019f184>] (_request_firmware+0x190/0x394)
<4>[    2.977081] [<c019f184>] (_request_firmware+0x190/0x394) from
[<c019f3c8>] (request_firmware_work_func+0x40/0x78)
<4>[    2.977233] [<c019f3c8>] (request_firmware_work_func+0x40/0x78)
from [<c00473cc>] (kthread+0x88/0x94)
<4>[    2.977386] [<c00473cc>] (kthread+0x88/0x94) from [<c0013844>]
(kernel_thread_exit+0x0/0x8)
<3>[    2.977508] p54spi spi2.0: fw_create_instance: device_register failed
<3>[    2.977600] p54spi spi2.0: Firmware loading failed
<3>[    3.104492] firmware spi2.0: firmware_loading_store: vmap() failed
<6>[    3.108337] p54spi spi2.0: loading default eeprom...
<6>[    3.108764] ieee80211 phy0: hwaddr 00:02:ee:c0:ff:ee,
MAC:isl3820 RF:Longbow

-- 
Thanks.
-- Max

Re: [RFC/RFT] p54spi: Convert driver to use asynchronous firmware loading

[Larry Finger]

On 02/12/2012 06:20 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 using request_firmware_nowait()
>> and delaying the start of mac80211 until the firmware is loaded.
>>
>> To prevent the possibility of the driver being unloaded while the firmware
>> loading callback is still active, a completion queue entry is used.
>>
>> Signed-off-by: Larry Finger<Larry.Finger@lwfinger.net>
>> ---
>>
>> This conversion of p54spi to use asynchronous firmware loading is based
>> on the method used in p54usb. As I do not have the hardware, it is only
>> compile tested. I would appreciate any feedback from people that have the
>> hardware.
>
> Hi, Larry.
>
> Are there any prerequisites for this patch?
> For now I'm applying it to the linux-omap ToT and having the following:

No, there are no prerequisites; however, as Michael Buesch noted, I missed the 
eeprom loading. This patch will not work.

I am currently testing an approach that is much simpler. Rather than using 
request_firmware_nowait(), I am having the probe routine start a work queue, and 
let that routine do a normal request_firmware(). This form of the patch is a lot 
less intrusive.

I hope to post a revised patch within a day.

Thanks for testing.

Larry