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 <[email protected]>
---
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
On Mon, 13 Feb 2012 13:37:06 -0600
Larry Finger <[email protected]> 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 <[email protected]>
> ---
> 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.
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<[email protected]>
> ---
> 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 */
>> 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<[email protected]>
>> ---
>> 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
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<[email protected]>
>>> ---
>>> 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