* Re: [RFC/RFT 5/5] p54spi: Load firmware from work queue and not from probe routine
2012-02-13 19:37 ` [RFC/RFT 5/5] p54spi: Load firmware from work queue and not from probe routine Larry Finger
@ 2012-02-13 21:57 ` Michael Büsch
2012-02-29 16:00 ` Larry Finger
1 sibling, 0 replies; 5+ messages in thread
From: Michael Büsch @ 2012-02-13 21:57 UTC (permalink / raw)
To: Larry Finger
Cc: linville, linux-wireless, chunkeey, Max Filippov, linux-kernel,
devel
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC/RFT 5/5] p54spi: Load firmware from work queue and not from probe routine
2012-02-13 19:37 ` [RFC/RFT 5/5] p54spi: Load firmware from work queue and not from probe routine Larry Finger
2012-02-13 21:57 ` Michael Büsch
@ 2012-02-29 16:00 ` Larry Finger
2012-02-29 19:54 ` Max Filippov
1 sibling, 1 reply; 5+ messages in thread
From: Larry Finger @ 2012-02-29 16:00 UTC (permalink / raw)
To: linux-wireless
Cc: Larry Finger, linville, chunkeey, Max Filippov, m, linux-kernel,
devel
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 */
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC/RFT 5/5] p54spi: Load firmware from work queue and not from probe routine
2012-02-29 16:00 ` Larry Finger
@ 2012-02-29 19:54 ` Max Filippov
2012-02-29 20:01 ` Larry Finger
0 siblings, 1 reply; 5+ messages in thread
From: Max Filippov @ 2012-02-29 19:54 UTC (permalink / raw)
To: Larry Finger; +Cc: linux-wireless, linville, chunkeey, m, linux-kernel, devel
>> 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
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [RFC/RFT 5/5] p54spi: Load firmware from work queue and not from probe routine
2012-02-29 19:54 ` Max Filippov
@ 2012-02-29 20:01 ` Larry Finger
0 siblings, 0 replies; 5+ messages in thread
From: Larry Finger @ 2012-02-29 20:01 UTC (permalink / raw)
To: Max Filippov; +Cc: linux-wireless, linville, chunkeey, m, linux-kernel, devel
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
^ permalink raw reply [flat|nested] 5+ messages in thread