Hi Teemu, Sakari,
I've been thinking about this change and I think it is a reasonable change.
I had some doubts earlier, but after thinking it through I agree with this.
But it needs a bit more work, see below.
On 09/17/2013 08:07 PM, Sakari Ailus wrote:
> From: Teemu Tuominen <teemux.tuominen@xxxxxxxxx>
>
> Add check and return POLLERR from subdev_poll() in case of no events
> subscribed and wakeup once the last event subscription is removed.
>
> This change is essentially done to add possibility to wakeup polling
> with concurrent unsubscribe.
>
> Signed-off-by: Teemu Tuominen <teemux.tuominen@xxxxxxxxx>
>
> Move the check after calling poll_wait(). Otherwise it's possible that we go
> to sleep without getting notified if the subscription went away between the
> two.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Tested-by: Teemu Tuominen <teemux.tuominen@xxxxxxxxx>
> ---
> Hi all,
>
> Poll for events will sleep forever if there are no events subscribed.
> Calling poll from an application that has not subscribed any event indeed
> sounds silly, but un multi-threaded applications this is not quite as
> straightforward.
>
> Assume the following: an application has two threads where one handles
> event subscription and the other handles the events. The first thread
> unsubscribes the events and the latter will sleep forever. And do this while
> the program intends to quit without an intention to subscribe for any
> further events, and there's a deadlock.
>
> Alternative solutions to handle this are signals (rather a nuisance if the
> application happens to be a library instead) or a pipe (2) between the
> threads. Pipe is workable, but instead of being a proper solution to the
> problem still looks like a workaround instead.
>
> This patch fixes the issue on kernel side by waking up the processes
> sleeping in poll and returning POLLERR when the last subscribed event is
> gone. The behaviour mirrors that of videobuf2 which will return POLLERR if
> either streaming is disabled or no buffers are queued.
>
> Just "waking up the sleeping threads once" is not an option: threads are not
> visible to the kernel at this level and just "waking them up" isn't either
> since poll will go back to sleep before returning the control back to user
> space as long as it would return zero.
>
> (Thinking about it --- similar change should probably be made to videobuf2
> event poll handling as well.)
Yes, this should be added here as well. Can you implement that as well?
>
> Kind regards,
> Sakari
>
> drivers/media/v4l2-core/v4l2-event.c | 15 +++++++++++++++
> drivers/media/v4l2-core/v4l2-subdev.c | 3 +++
> include/media/v4l2-event.h | 1 +
> 3 files changed, 19 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-event.c b/drivers/media/v4l2-core/v4l2-event.c
> index 86dcb54..b53897e 100644
> --- a/drivers/media/v4l2-core/v4l2-event.c
> +++ b/drivers/media/v4l2-core/v4l2-event.c
> @@ -107,6 +107,19 @@ static struct v4l2_subscribed_event *v4l2_event_subscribed(
> return NULL;
> }
>
> +bool v4l2_event_has_subscribed(struct v4l2_fh *fh)
> +{
> + unsigned long flags;
> + bool rval;
> +
> + spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> + rval = !list_empty(&fh->subscribed);
> + spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> + return rval;
> +}
> +EXPORT_SYMBOL(v4l2_event_has_subscribed);
> +
> static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev,
> const struct timespec *ts)
> {
> @@ -299,6 +312,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> fh->navailable--;
> }
> list_del(&sev->list);
> + if (list_empty(&fh->subscribed))
> + wake_up_all(&fh->wait);
> }
>
> spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index 996c248..f2aa00f 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -382,6 +382,9 @@ static unsigned int subdev_poll(struct file *file, poll_table *wait)
> if (v4l2_event_pending(fh))
> return POLLPRI;
>
> + if (!v4l2_event_has_subscribed(fh))
> + return POLLERR;
> +
This needs a bit more work: you should also check that poll() actually is waiting
for exceptions. If not, then also return POLLERR. This has never been checked before,
but it is needed.
> return 0;
> }
>
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> index be05d01..a9ca2b5 100644
> --- a/include/media/v4l2-event.h
> +++ b/include/media/v4l2-event.h
> @@ -121,6 +121,7 @@ struct v4l2_subscribed_event {
>
> int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event,
> int nonblocking);
> +bool v4l2_event_has_subscribed(struct v4l2_fh *fh);
> void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev);
> void v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *ev);
> int v4l2_event_pending(struct v4l2_fh *fh);
>
Regards,
Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
