Hi,
I had some more comments inline in v1, please see below.
On 09/10/2016 05:44 PM, Jonathan Cameron wrote:
>> @@ -162,13 +163,20 @@ unsigned int iio_buffer_poll(struct file *filp,
>> {
>> struct iio_dev *indio_dev = filp->private_data;
>> struct iio_buffer *rb = indio_dev->buffer;
>> + bool rdy;
>>
>> if (!indio_dev->info)
>> - return 0;
>> + return POLLERR;
I've had a look at what other subsystems do and input returns POLLERR |
POLLHUP. Maybe we should mirror this.
>>
>> poll_wait(filp, &rb->pollq, wait);
>> - if (iio_buffer_ready(indio_dev, rb, rb->watermark, 0))
>> + rdy = iio_buffer_ready(indio_dev, rb, rb->watermark, 0);
>> +
>> + if (!indio_dev->info)
>> + return POLLERR;
Why check indio_dev->info twice though, since there is no locking involved
the second check does gain us anything in terms of race condition avoidance.
I think we should have one check after the poll_wait() call. If the device
is unregistered we wake the pollq. So adding it there should make sure that
the poll() callback is called again if the device is unregistered after the
check.
I'm not sure though if we'd need a explicit memory barrier or whether
poll_wait() can act as a implicit one. But probably we are ok, given that
poll_wait() should take a lock.
To be sure that the code is race free we need a guarantee that there is
strict ordering between the indio_dev->info = NULL and wake_up() in
unregister as well as between poll_wait() and the if (indio_dev->info) check
in poll().
The scenario is basically
CPU0 CPU1
---------------------------------------------------------
poll_wait() indio_dev->info = NULL
if (!indio_dev->info) wake_up()
If strict ordering is not observed for either side we could end up missing
the unregister and poll() will block forever just as before.
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
