| Message ID | e25078f4-96f4-482c-b5da-a4a22d88b072@I-love.SAKURA.ne.jp (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | wfx: avoid flush_workqueue(system_highpri_wq) usage | expand |
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> 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 <penguin-kernel@I-love.SAKURA.ne.jp> [...] > @@ -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);
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? 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. > 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. Whatever is fine. wfx is the last user of flush_workqueue(system_*_wq) and I want to know whether I can include system_highpri_wq into below patch. Subject: [PATCH] workqueue: wrap flush_workqueue() using a macro While a conversion to stop flushing of kernel-global workqueues is in progress, we can warn users who are not aware of this conversion, by wrapping flush_workqueue() as a macro and injecting BUILD_BUG_ON() tests. --- include/linux/workqueue.h | 27 +++++++++++++++++++++++++++ kernel/workqueue.c | 1 + 2 files changed, 28 insertions(+) diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h index 7b13fae377e2..3eaf19262d19 100644 --- a/include/linux/workqueue.h +++ b/include/linux/workqueue.h @@ -654,4 +654,31 @@ int workqueue_offline_cpu(unsigned int cpu); void __init workqueue_init_early(void); void __init workqueue_init(void); +/* + * Detect attempt to flush system-wide workqueues at compile time when possible. + * See https://lkml.kernel.org/r/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp for + * reasons and steps for converting system-wide workqueues into local workqueues. + * Checks for system_wq and system_highpri_wq will be added after + * all in-tree users stopped flushing these workqueues. + */ +#define flush_workqueue(wq) \ +({ \ + BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_long_wq) && \ + &(wq) == &system_long_wq, \ + "Please avoid flushing system_long_wq."); \ + BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_unbound_wq) && \ + &(wq) == &system_unbound_wq, \ + "Please avoid flushing system_unbound_wq."); \ + BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_freezable_wq) && \ + &(wq) == &system_freezable_wq, \ + "Please avoid flushing system_freezable_wq."); \ + BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_power_efficient_wq) && \ + &(wq) == &system_power_efficient_wq, \ + "Please avoid flushing system_power_efficient_wq."); \ + BUILD_BUG_ON_MSG(__builtin_constant_p(&(wq) == &system_freezable_power_efficient_wq) && \ + &(wq) == &system_freezable_power_efficient_wq, \ + "Please avoid flushing system_freezable_power_efficient_wq."); \ + flush_workqueue(wq); \ +}) + #endif diff --git a/kernel/workqueue.c b/kernel/workqueue.c index b447012df177..de53813a92d2 100644 --- a/kernel/workqueue.c +++ b/kernel/workqueue.c @@ -2813,6 +2813,7 @@ static void warn_flush_attempt(struct workqueue_struct *wq) * This function sleeps until all work items which were queued on entry * have finished execution, but it is not livelocked by new incoming ones. */ +#undef flush_workqueue void flush_workqueue(struct workqueue_struct *wq) { struct wq_flusher this_flusher = {
Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> 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.
On Sunday 1 May 2022 10:53:57 CEST Kalle Valo wrote: > Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> 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 <penguin-kernel@I-love.SAKURA.ne.jp> > > [...] > > > @@ -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); > The last time I have checked if I could use this workqueue, I remember it was not well suited for wfx (I don't remember why exactly). So I believe we have to allocate a new workqueue.
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 <jerome.pouiller@silabs.com>"); 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
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 <penguin-kernel@I-love.SAKURA.ne.jp> --- 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(-)