Hello.
I found that commit c86176d51340d5ee ("staging: wfx: ensure HIF request has
been sent before polling") started using flush_workqueue(system_highpri_wq).
But please avoid flushing system-wide workqueues, for
flushing system-wide workqueues is dangerous and will be forbidden.
Link: https://lkml.kernel.org/r/[email protected]
On Tuesday 12 April 2022 15:11:53 CEST Tetsuo Handa wrote:
> Hello.
>
> I found that commit c86176d51340d5ee ("staging: wfx: ensure HIF request has
> been sent before polling") started using flush_workqueue(system_highpri_wq).
>
> But please avoid flushing system-wide workqueues, for
> flushing system-wide workqueues is dangerous and will be forbidden.
>
> Link: https://lkml.kernel.org/r/[email protected]
>
>
There is a workqueue provided by mac80211. But I am not sure it can
replace the system-wide workqueue.
I guess I need to declare a new workqueue. Give me a couple of weeks to
prepare a patch.
--
J?r?me Pouiller
Flushing system-wide workqueues is dangerous and will be forbidden.
Replace system_highpri_wq with local wfx_wq.
While we are at it, add missing spi_unregister_driver() call when
sdio_register_driver() failed.
Signed-off-by: Tetsuo Handa <[email protected]>
---
Note: This patch is only compile tested.
drivers/net/wireless/silabs/wfx/bh.c | 6 +++---
drivers/net/wireless/silabs/wfx/hif_tx.c | 2 +-
drivers/net/wireless/silabs/wfx/main.c | 11 +++++++++++
drivers/net/wireless/silabs/wfx/wfx.h | 2 ++
4 files changed, 17 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/silabs/wfx/bh.c b/drivers/net/wireless/silabs/wfx/bh.c
index bcea9d5b119c..377dd8b010e2 100644
--- a/drivers/net/wireless/silabs/wfx/bh.c
+++ b/drivers/net/wireless/silabs/wfx/bh.c
@@ -267,7 +267,7 @@ void wfx_bh_request_rx(struct wfx_dev *wdev)
wfx_control_reg_read(wdev, &cur);
prev = atomic_xchg(&wdev->hif.ctrl_reg, cur);
complete(&wdev->hif.ctrl_ready);
- queue_work(system_highpri_wq, &wdev->hif.bh);
+ queue_work(wfx_wq, &wdev->hif.bh);
if (!(cur & CTRL_NEXT_LEN_MASK))
dev_err(wdev->dev, "unexpected control register value: length field is 0: %04x\n",
@@ -280,7 +280,7 @@ void wfx_bh_request_rx(struct wfx_dev *wdev)
/* Driver want to send data */
void wfx_bh_request_tx(struct wfx_dev *wdev)
{
- queue_work(system_highpri_wq, &wdev->hif.bh);
+ queue_work(wfx_wq, &wdev->hif.bh);
}
/* If IRQ is not available, this function allow to manually poll the control register and simulate
@@ -295,7 +295,7 @@ void wfx_bh_poll_irq(struct wfx_dev *wdev)
u32 reg;
WARN(!wdev->poll_irq, "unexpected IRQ polling can mask IRQ");
- flush_workqueue(system_highpri_wq);
+ flush_workqueue(wfx_wq);
start = ktime_get();
for (;;) {
wfx_control_reg_read(wdev, ®);
diff --git a/drivers/net/wireless/silabs/wfx/hif_tx.c b/drivers/net/wireless/silabs/wfx/hif_tx.c
index 9c653d0e9034..85c68b1b11c4 100644
--- a/drivers/net/wireless/silabs/wfx/hif_tx.c
+++ b/drivers/net/wireless/silabs/wfx/hif_tx.c
@@ -73,7 +73,7 @@ int wfx_cmd_send(struct wfx_dev *wdev, struct wfx_hif_msg *request,
if (no_reply) {
/* Chip won't reply. Ensure the wq has send the buffer before to continue. */
- flush_workqueue(system_highpri_wq);
+ flush_workqueue(wfx_wq);
ret = 0;
goto end;
}
diff --git a/drivers/net/wireless/silabs/wfx/main.c b/drivers/net/wireless/silabs/wfx/main.c
index e575a81ca2ca..d3d18c498dd0 100644
--- a/drivers/net/wireless/silabs/wfx/main.c
+++ b/drivers/net/wireless/silabs/wfx/main.c
@@ -40,6 +40,8 @@ MODULE_DESCRIPTION("Silicon Labs 802.11 Wireless LAN driver for WF200");
MODULE_AUTHOR("J辿r担me Pouiller <[email protected]>");
MODULE_LICENSE("GPL");
+struct workqueue_struct *wfx_wq;
+
#define RATETAB_ENT(_rate, _rateid, _flags) { \
.bitrate = (_rate), \
.hw_value = (_rateid), \
@@ -473,10 +475,18 @@ static int __init wfx_core_init(void)
{
int ret = 0;
+ wfx_wq = alloc_workqueue("wfx_wq", WQ_HIGHPRI, 0);
+ if (!wfx_wq)
+ return -ENOMEM;
if (IS_ENABLED(CONFIG_SPI))
ret = spi_register_driver(&wfx_spi_driver);
if (IS_ENABLED(CONFIG_MMC) && !ret)
ret = sdio_register_driver(&wfx_sdio_driver);
+ if (ret) {
+ if (IS_ENABLED(CONFIG_SPI))
+ spi_unregister_driver(&wfx_spi_driver);
+ destroy_workqueue(wfx_wq);
+ }
return ret;
}
module_init(wfx_core_init);
@@ -487,5 +497,6 @@ static void __exit wfx_core_exit(void)
sdio_unregister_driver(&wfx_sdio_driver);
if (IS_ENABLED(CONFIG_SPI))
spi_unregister_driver(&wfx_spi_driver);
+ destroy_workqueue(wfx_wq);
}
module_exit(wfx_core_exit);
diff --git a/drivers/net/wireless/silabs/wfx/wfx.h b/drivers/net/wireless/silabs/wfx/wfx.h
index 6594cc647c2f..ea0ba002eb1f 100644
--- a/drivers/net/wireless/silabs/wfx/wfx.h
+++ b/drivers/net/wireless/silabs/wfx/wfx.h
@@ -159,4 +159,6 @@ static inline int memzcmp(void *src, unsigned int size)
return memcmp(buf, buf + 1, size - 1);
}
+extern struct workqueue_struct *wfx_wq;
+
#endif
--
2.34.1
Tetsuo Handa <[email protected]> writes:
> Flushing system-wide workqueues is dangerous and will be forbidden.
> Replace system_highpri_wq with local wfx_wq.
>
> While we are at it, add missing spi_unregister_driver() call when
> sdio_register_driver() failed.
>
> Signed-off-by: Tetsuo Handa <[email protected]>
[...]
> @@ -473,10 +475,18 @@ static int __init wfx_core_init(void)
> {
> int ret = 0;
>
> + wfx_wq = alloc_workqueue("wfx_wq", WQ_HIGHPRI, 0);
> + if (!wfx_wq)
> + return -ENOMEM;
> if (IS_ENABLED(CONFIG_SPI))
> ret = spi_register_driver(&wfx_spi_driver);
> if (IS_ENABLED(CONFIG_MMC) && !ret)
> ret = sdio_register_driver(&wfx_sdio_driver);
> + if (ret) {
> + if (IS_ENABLED(CONFIG_SPI))
> + spi_unregister_driver(&wfx_spi_driver);
> + destroy_workqueue(wfx_wq);
> + }
> return ret;
> }
> module_init(wfx_core_init);
So now the thread is created every time the module loaded, even if
there's no device available. Also I'm not really a fan of global
variables (wfx_wq). I would rather create a workqueue per device in
wfx_probe() or use the workqueue provided by mac80211.
/**
* ieee80211_queue_work - add work onto the mac80211 workqueue
*
* Drivers and mac80211 use this to add work onto the mac80211 workqueue.
* This helper ensures drivers are not queueing work when they should not be.
*
* @hw: the hardware struct for the interface we are adding work for
* @work: the work we want to add onto the mac80211 workqueue
*/
void ieee80211_queue_work(struct ieee80211_hw *hw, struct work_struct *work);
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
Tetsuo Handa <[email protected]> writes:
> On 2022/05/01 17:53, Kalle Valo wrote:
>> So now the thread is created every time the module loaded, even if
>> there's no device available.
>
> Excuse me, but what thread?
Sorry, s/thread/workqueue/.
> alloc_workqueue() without WQ_MEM_RECLAIM flag does not create a
> thread, and therefore consumes little resource where there's no device
> available does not matter.
It still allocating memory which is not needed. To me allocating
resources during module_init is wrong.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches