[PATCH linux dev-4.10 1/3] drivers/fsi/sbefifo: refactor to upstream list state

[Andrew Jeffery]

Hi, it's me again.

On Mon, 2017-09-25 at 14:27 +0930, Andrew Jeffery wrote:
> >  static int sbefifo_remove(struct device *dev)
> >  {
> > -     struct fsi_device *fsi_dev = to_fsi_dev(dev);
> > -     struct sbefifo *sbefifo, *sbefifo_tmp;
> > +     struct sbefifo *sbefifo = dev_get_drvdata(dev);
> >       struct sbefifo_xfr *xfr;
> >  
> > -     list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {
> > -             if (sbefifo->fsi_dev != fsi_dev)
> > -                     continue;
> > -
> > -             device_for_each_child(dev, NULL, sbefifo_unregister_child);
> > +     WRITE_ONCE(sbefifo->rc, -ENODEV);
> > +     wake_up(&sbefifo->wait);
> >  
> > -             misc_deregister(&sbefifo->mdev);
> > -             list_del(&sbefifo->link);
> > -             ida_simple_remove(&sbefifo_ida, sbefifo->idx);
> > +     misc_deregister(&sbefifo->mdev);

I think we should do this before the wake_up() to ensure that we wake
all the submitted jobs. Otherwise we might miss some.

> > +     device_for_each_child(dev, NULL, sbefifo_unregister_child);
> >  
> > -             if (del_timer_sync(&sbefifo->poll_timer))
> > -                     sbefifo_put(sbefifo);
> > +     ida_simple_remove(&sbefifo_ida, sbefifo->idx);
> >  
> > -             spin_lock(&sbefifo->lock);
> > -             list_for_each_entry(xfr, &sbefifo->xfrs, xfrs)
> > -                     kfree(xfr);
> > -             spin_unlock(&sbefifo->lock);
> > +     if (del_timer_sync(&sbefifo->poll_timer))
> > +             sbefifo_put(sbefifo);

Shouldn't we be doing this before WRITE_ONCE(sbefifo->rc, -ENODEV) to
ensure we avoid a race on the value of ->rc? Otherwise the timer could
fire between the WRITE_ONCE() and here and potentially write ->rc or
cause another job to hang in the FIFO.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20170925/fb90c677/attachment.sig>