On 12/09/16 14:36, Lars-Peter Clausen wrote: > 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. Feel free to post a patch killing it off :) J > >>> 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 > -- 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