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

["Jérôme Pouiller" <Jerome.Pouiller@silabs.com>]

On Tuesday 11 February 2020 13:49:22 CET Michał Mirosław 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.
> 
> 
> 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().
Indeed.

Reviewed-by: Jérôme Pouiller <jerome.pouiller@silabs.com>

-- 
Jérôme Pouiller