2022-05-02 10:43:05

by Jérôme Pouiller

[permalink] [raw]
Subject: Re: [PATCH] wfx: avoid flush_workqueue(system_highpri_wq) usage

On Sunday 1 May 2022 10:53:57 CEST Kalle Valo wrote:
> 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);
>

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.


--
J?r?me Pouiller