2022-05-02 17:42:20

by Jérôme Pouiller

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

On Monday 2 May 2022 10:16:06 CEST Tetsuo Handa wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
>
> Flushing system-wide workqueues is dangerous and will be forbidden.
> Replace system_highpri_wq with per "struct wfx_dev" bh_wq.
>
> Signed-off-by: Tetsuo Handa <[email protected]>
> ---
> Note: This patch is only compile tested.
>
> Changes in v2:
> Use per "struct wfx_dev" workqueue.
>
> drivers/net/wireless/silabs/wfx/bh.c | 6 +++---
> drivers/net/wireless/silabs/wfx/hif_tx.c | 2 +-
> drivers/net/wireless/silabs/wfx/main.c | 6 ++++++
> drivers/net/wireless/silabs/wfx/wfx.h | 1 +
> 4 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/silabs/wfx/bh.c b/drivers/net/wireless/silabs/wfx/bh.c
> index bcea9d5b119c..21dfdcf9cc27 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(wdev->bh_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(wdev->bh_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(wdev->bh_wq);
> start = ktime_get();
> for (;;) {
> wfx_control_reg_read(wdev, &reg);
> diff --git a/drivers/net/wireless/silabs/wfx/hif_tx.c b/drivers/net/wireless/silabs/wfx/hif_tx.c
> index 9c653d0e9034..d35dd940d968 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(wdev->bh_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..e015bfb8d221 100644
> --- a/drivers/net/wireless/silabs/wfx/main.c
> +++ b/drivers/net/wireless/silabs/wfx/main.c
> @@ -345,6 +345,10 @@ int wfx_probe(struct wfx_dev *wdev)
> wdev->pdata.gpio_wakeup = NULL;
> wdev->poll_irq = true;
>
> + wdev->bh_wq = alloc_workqueue("wfx_bh_wq", WQ_HIGHPRI, 0);
> + if (!wdev->bh_wq)
> + return -ENOMEM;
> +
> wfx_bh_register(wdev);
>
> err = wfx_init_device(wdev);
> @@ -458,6 +462,7 @@ int wfx_probe(struct wfx_dev *wdev)
> wdev->hwbus_ops->irq_unsubscribe(wdev->hwbus_priv);
> bh_unregister:
> wfx_bh_unregister(wdev);
> + destroy_workqueue(wdev->bh_wq);
> return err;
> }
>
> @@ -467,6 +472,7 @@ void wfx_release(struct wfx_dev *wdev)
> wfx_hif_shutdown(wdev);
> wdev->hwbus_ops->irq_unsubscribe(wdev->hwbus_priv);
> wfx_bh_unregister(wdev);
> + destroy_workqueue(wdev->bh_wq);
> }
>
> static int __init wfx_core_init(void)
> diff --git a/drivers/net/wireless/silabs/wfx/wfx.h b/drivers/net/wireless/silabs/wfx/wfx.h
> index 6594cc647c2f..6f5e95dae21f 100644
> --- a/drivers/net/wireless/silabs/wfx/wfx.h
> +++ b/drivers/net/wireless/silabs/wfx/wfx.h
> @@ -57,6 +57,7 @@ struct wfx_dev {
> struct mutex rx_stats_lock;
> struct wfx_hif_tx_power_loop_info tx_power_loop_info;
> struct mutex tx_power_loop_info_lock;
> + struct workqueue_struct *bh_wq;
> };
>
> struct wfx_vif {
> --
> 2.34.1
>
>
>

It's right for me.

Signed-off-by: J?r?me Pouiller <[email protected]>

--
J?r?me Pouiller



2022-05-02 22:46:11

by Kalle Valo

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

Jérôme Pouiller <[email protected]> writes:

> On Monday 2 May 2022 10:16:06 CEST Tetsuo Handa wrote:
>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe.
>>
>>
>> Flushing system-wide workqueues is dangerous and will be forbidden.
>> Replace system_highpri_wq with per "struct wfx_dev" bh_wq.
>>
>> Signed-off-by: Tetsuo Handa <[email protected]>

[...]

> It's right for me.
>
> Signed-off-by: Jérôme Pouiller <[email protected]>

Thanks, but I'll change this to Acked-by:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by

Also please edit your quotes, otherwise patchwork is hard to use.

--
https://patchwork.kernel.org/project/linux-wireless/list/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches