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

["Michał Mirosław" <mirq-linux@rere.qmqm.pl>]

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