On 09/12/2016 03:24 PM, Gregor Boirie wrote:
> Hi,
>
> On 09/12/2016 02:18 PM, Lars-Peter Clausen wrote:
>> 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.
> Well, some input device only don't IFAIK (I'm no expert on this) as
> POLLHUP might be focused on output errors only.
The way I understand it is that the 'output only' description for POLLHUP
means that you can't pass POLLHUP as a event to listen for. A POLLHUP will
always wakeup the poll() and be set in the revents field. It does not meant
that is only set for output devices.
>
>>
>>>> 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 took iio_buffer_read_first_n_outer() as a reference : the check of
> indio_dev->info is performed at least twice :
> * once at function start
> * each time wait_event_interruptible() returns.
>
> I thought it would be a good starting point.
>
Strictly speaking the first check there is redundant.
>> 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.
> No risk that an "exotic" userspace process perform 2 sys_poll() in the time
> between iio_device_unregister() and iio_device_free() ?
> I mean : am I wrong in thinking that the filep is still valid as long as the
> fd has
> not been released completly ?
The filep is valid as long as the file is open, same is true for the iio_dev
and iio_buffer structures. The open file holds a reference to them.
What I meant with the race condition here is that we should make sure that
no matter how the device unregister() and the sys_poll() are timed relative
to each other it should still work as expected and not result in a
indefinitely waiting poll() operation.
When multiple CPUs are involved the two operations can happen at the same
time, so we need to cover that.
>
>>
>> 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
--
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
