| Message ID | 20220225112405.355599-10-Jerome.Pouiller@silabs.com |
|---|---|
| State | New |
| Headers | show |
| Series | staging: wfx: usual maintenance | expand |
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/49925af7-78a8-a3dd-bce6-cfc02e1a9236@I-love.SAKURA.ne.jp > > 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.
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 = {
diff --git a/drivers/staging/wfx/bh.c b/drivers/staging/wfx/bh.c index 4c6ba9c342a6..bcea9d5b119c 100644 --- a/drivers/staging/wfx/bh.c +++ b/drivers/staging/wfx/bh.c @@ -295,6 +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); start = ktime_get(); for (;;) { wfx_control_reg_read(wdev, ®);