Re: [PATCH v2 1/6] staging: wfx: fix init/remove vs IRQ race

From: JÃrÃme Pouiller
Date: Tue Feb 11 2020 - 06:19:24 EST

On Tuesday 11 February 2020 11:35:01 CET MichaÅ MirosÅaw wrote:
> Current code races in init/exit with interrupt handlers. This is noticed
> by the warning below. Fix it by using devres for ordering allocations and
> IRQ de/registration.
>
> WARNING: CPU: 0 PID: 827 at drivers/staging/wfx/bus_spi.c:142 wfx_spi_irq_handler+0x5c/0x64 [wfx]
> race condition in driver init/deinit
>
> Cc: stable@xxxxxxxxxxxxxxx
> Fixes: 0096214a59a7 ("staging: wfx: add support for I/O access")
> Signed-off-by: MichaÅ MirosÅaw <mirq-linux@xxxxxxxxxxxx>
> ---
> v2: use devres to fix the races
> ---
> drivers/staging/wfx/bus_sdio.c | 15 ++++++---------
> drivers/staging/wfx/bus_spi.c | 27 ++++++++++++++-------------
> drivers/staging/wfx/main.c | 21 +++++++++++++--------
> drivers/staging/wfx/main.h | 1 -
> 4 files changed, 33 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/staging/wfx/bus_sdio.c b/drivers/staging/wfx/bus_sdio.c
> index f8901164c206..5450bd5e1b5d 100644
> --- a/drivers/staging/wfx/bus_sdio.c
> +++ b/drivers/staging/wfx/bus_sdio.c
> @@ -200,25 +200,23 @@ static int wfx_sdio_probe(struct sdio_func *func,
> if (ret)
> goto err0;
>
> - ret = wfx_sdio_irq_subscribe(bus);
> - if (ret)
> - goto err1;
> -
> bus->core = wfx_init_common(&func->dev, &wfx_sdio_pdata,
> &wfx_sdio_hwbus_ops, bus);
> if (!bus->core) {
> ret = -EIO;
> - goto err2;
> + goto err1;
> }
>
> + ret = wfx_sdio_irq_subscribe(bus);
> + if (ret)
> + goto err1;
> +
> ret = wfx_probe(bus->core);
> if (ret)
> - goto err3;
> + goto err2;
>
> return 0;
>
> -err3:
> - wfx_free_common(bus->core);
> err2:
> wfx_sdio_irq_unsubscribe(bus);
> err1:
> @@ -234,7 +232,6 @@ static void wfx_sdio_remove(struct sdio_func *func)
> struct wfx_sdio_priv *bus = sdio_get_drvdata(func);
>
> wfx_release(bus->core);
> - wfx_free_common(bus->core);
> wfx_sdio_irq_unsubscribe(bus);
> sdio_claim_host(func);
> sdio_disable_func(func);
> diff --git a/drivers/staging/wfx/bus_spi.c b/drivers/staging/wfx/bus_spi.c
> index 40bc33035de2..605ad74068b7 100644
> --- a/drivers/staging/wfx/bus_spi.c
> +++ b/drivers/staging/wfx/bus_spi.c
> @@ -154,6 +154,11 @@ static void wfx_spi_request_rx(struct work_struct *work)
> wfx_bh_request_rx(bus->core);
> }
>
> +static void wfx_flush_irq_work(void *w)
> +{
> + flush_work(w);
> +}
> +
> static size_t wfx_spi_align_size(void *priv, size_t size)
> {
> // Most of SPI controllers avoid DMA if buffer size is not 32bit aligned
> @@ -207,22 +212,23 @@ static int wfx_spi_probe(struct spi_device *func)
> udelay(2000);
> }
>
> - ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> - IRQF_TRIGGER_RISING, "wfx", bus);
> - if (ret)
> - return ret;
> -
> INIT_WORK(&bus->request_rx, wfx_spi_request_rx);
> bus->core = wfx_init_common(&func->dev, &wfx_spi_pdata,
> &wfx_spi_hwbus_ops, bus);
> if (!bus->core)
> return -EIO;
>
> - ret = wfx_probe(bus->core);
> + ret = devm_add_action_or_reset(&func->dev, wfx_flush_irq_work,
> + &bus->request_rx);
> if (ret)
> - wfx_free_common(bus->core);
> + return ret;
>
> - return ret;
> + ret = devm_request_irq(&func->dev, func->irq, wfx_spi_irq_handler,
> + IRQF_TRIGGER_RISING, "wfx", bus);
> + if (ret)
> + return ret;
> +
> + return wfx_probe(bus->core);
> }
>
> static int wfx_spi_remove(struct spi_device *func)
> @@ -230,11 +236,6 @@ static int wfx_spi_remove(struct spi_device *func)
> struct wfx_spi_priv *bus = spi_get_drvdata(func);
>
> wfx_release(bus->core);
> - wfx_free_common(bus->core);
> - // A few IRQ will be sent during device release. Hopefully, no IRQ
> - // should happen after wdev/wvif are released.
> - devm_free_irq(&func->dev, func->irq, bus);
> - flush_work(&bus->request_rx);
> return 0;
> }
>
> diff --git a/drivers/staging/wfx/main.c b/drivers/staging/wfx/main.c
> index 84adad64fc30..76b2ff7fc7fe 100644
> --- a/drivers/staging/wfx/main.c
> +++ b/drivers/staging/wfx/main.c
> @@ -262,6 +262,16 @@ static int wfx_send_pdata_pds(struct wfx_dev *wdev)
> return ret;
> }
>
> +static void wfx_free_common(void *data)
> +{
> + struct wfx_dev *wdev = data;
> +
> + mutex_destroy(&wdev->rx_stats_lock);
> + mutex_destroy(&wdev->conf_mutex);
> + wfx_tx_queues_deinit(wdev);
> + ieee80211_free_hw(wdev->hw);
> +}
> +
> struct wfx_dev *wfx_init_common(struct device *dev,
> const struct wfx_platform_data *pdata,
> const struct hwbus_ops *hwbus_ops,
> @@ -332,17 +342,12 @@ struct wfx_dev *wfx_init_common(struct device *dev,
> wfx_init_hif_cmd(&wdev->hif_cmd);
> wfx_tx_queues_init(wdev);
>
> + if (devm_add_action_or_reset(dev, wfx_free_common, wdev))
> + return NULL;
> +
> return wdev;
> }
>
> -void wfx_free_common(struct wfx_dev *wdev)
> -{
> - mutex_destroy(&wdev->rx_stats_lock);
> - mutex_destroy(&wdev->conf_mutex);
> - wfx_tx_queues_deinit(wdev);
> - ieee80211_free_hw(wdev->hw);
> -}
> -
> int wfx_probe(struct wfx_dev *wdev)
> {
> int i;
> diff --git a/drivers/staging/wfx/main.h b/drivers/staging/wfx/main.h
> index 875f8c227803..9c9410072def 100644
> --- a/drivers/staging/wfx/main.h
> +++ b/drivers/staging/wfx/main.h
> @@ -34,7 +34,6 @@ struct wfx_dev *wfx_init_common(struct device *dev,
> const struct wfx_platform_data *pdata,
> const struct hwbus_ops *hwbus_ops,
> void *hwbus_priv);
> -void wfx_free_common(struct wfx_dev *wdev);
>
> int wfx_probe(struct wfx_dev *wdev);
> void wfx_release(struct wfx_dev *wdev);
> --
> 2.20.1
>

Are you sure you can completely drop wfx_free_common()? If you want to
use devres, I think that wfx_flush_irq_work() should call
wfx_free_common(), no?

--
JÃrÃme Pouiller