On Tue, Feb 11, 2020 at 11:19:18AM +0000, Jérôme Pouiller wrote:
> 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
[...]
> > --- 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?
wfx_free_common() will be called by devres in the right order. That's ensured
by devm_add_action_or_reset() at the end of wfx_init_common().
Best Regards,
Michał Mirosław
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
