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

[Eddie James]

On 09/25/2017 01:16 AM, Andrew Jeffery wrote:
> 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.

Problem is that if the child devices (occ, for example) don't have a 
cancel function, then we would hang waiting for stuff to wake up in 
their remove() functions. So I think we need to set rc and wake_up 
first, then start unregistering clients.

>
>>> +     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.

rc is only ever written with error values, fortunately, so I'm not too 
concerned about it.

Thanks,
Eddie