v4l: return POLLERR on V4L2 sub-devices if no events are subscribed

From: Teemu Tuominen <[email protected]>

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 <[email protected]>

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 <[email protected]>

Tested-by Teemu Tuominen <[email protected]>

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

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(+)

(quoted diff) +0 -0

b/drivers/media/v4l2-core/v4l2-event.c index 86dcb54..b53897e 100644

drivers/media/v4l2-core/v4l2-event.c +15 -0

@@ -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);

(quoted diff) +0 -0

b/drivers/media/v4l2-core/v4l2-subdev.c index 996c248..f2aa00f 100644

drivers/media/v4l2-core/v4l2-subdev.c +3 -0

@@ -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;
+
 	return 0;
 }
 

include/media/v4l2-event.h +1 -0

index be05d01..a9ca2b5 100644
@@ -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);

From: Teemu Tuominen <[email protected]>

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 <[email protected]>

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 <[email protected]>

Tested-by Teemu Tuominen <[email protected]>

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

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(+)

drivers/media/v4l2-core/v4l2-event.c +15 -0

index 86dcb54..b53897e 100644
@@ -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);

drivers/media/v4l2-core/v4l2-subdev.c +0 -0

index 996c248..f2aa00f 100644

drivers/media/v4l2-core/v4l2-subdev.c +0 -0

 	return 0;
 }
 

include/media/v4l2-event.h +0 -0

index be05d01..a9ca2b5 100644

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.

Quoted 45 lines · Sakari Ailus

From: Teemu Tuominen <[email protected]>

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 <[email protected]>

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 <[email protected]>

Tested-by Teemu Tuominen <[email protected]>

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

Quoted 2 lines

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

Quoted 50 lines

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(+)

drivers/media/v4l2-core/v4l2-event.c +15 -0

index 86dcb54..b53897e 100644
@@ -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);

drivers/media/v4l2-core/v4l2-subdev.c +0 -0

index 996c248..f2aa00f 100644
@@ -382,6 +382,9 @@ static unsigned int subdev_poll(struct file *file, poll_table *wait)
  	if (v4l2_event_pending(fh))
  		return POLLPRI;
 

drivers/media/v4l2-core/v4l2-subdev.c static unsigned int subdev_poll(struct file *file, poll_table *wait) · line 387

Quoted 3 lines

drivers/media/v4l2-core/v4l2-subdev.c +3 -0

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