Re: [PATCH v2 1/1] iio:buffer: let poll report an error for unregistered devices

[Gregor Boirie <gregor.boirie@parrot.com>]

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.

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

> 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 ?

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