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