Various ctrl and event frame work patches (version 3)

Various ctrl and event frame work patches (version 3)

[Hans de Goede]

Hi All,

This patch set obsoletes my previous "add v4l2_subscribed_event_ops" set,
while working on adding support for ctrl-events to the uvc driver I found
a few bugs in the event code, which this patchset fixes.

Changes since version 1:
------------------------

4/5 v4l2-event: Add v4l2_subscribed_event_ops:
-Added a documentation update (update v4l2-framework.txt) to:

Changes since version 2:
------------------------

2/5 v4l2-event: Remove pending events from fh event queue:
-Simplify loop

3/5 v4l2-event: Don't set sev->fh to NULL on unsubscribe
-Remove sev->fh != NULL check from send_event
-Extend commit message with info how the old set to NULL + check code was
 actually race and why simply removing this is safe

4/5 v4l2-event: Add v4l2_subscribed_event_ops:
-Ignore queuing events for subscribed_event-s which add op has not completed
 yet


Regards,

Hans

[PATCH 1/5] v4l2-event: Deny subscribing with a type of V4L2_EVENT_ALL

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/v4l2-event.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 53b190c..9f56f18 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -215,6 +215,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 	unsigned long flags;
 	unsigned i;
 
+	if (sub->type == V4L2_EVENT_ALL)
+		return -EINVAL;
+
 	if (elems < 1)
 		elems = 1;
 	if (sub->type == V4L2_EVENT_CTRL) {
-- 
1.7.7

[PATCH 2/5] v4l2-event: Remove pending events from fh event queue when unsubscribing

[Hans de Goede]

The kev pointers inside the pending events queue (the available queue) of the
fh point to data inside the sev, unsubscribing frees the sev, thus making these
pointers point to freed memory!

This patch fixes these dangling pointers in the available queue by removing
all matching pending events on unsubscription.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/v4l2-event.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 9f56f18..4d01f17 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -285,6 +285,7 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 {
 	struct v4l2_subscribed_event *sev;
 	unsigned long flags;
+	int i;
 
 	if (sub->type == V4L2_EVENT_ALL) {
 		v4l2_event_unsubscribe_all(fh);
@@ -295,6 +296,11 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 
 	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
 	if (sev != NULL) {
+		/* Remove any pending events for this subscription */
+		for (i = 0; i < sev->in_use; i++) {
+			list_del(&sev->events[sev_pos(sev, i)].list);
+			fh->navailable--;
+		}
 		list_del(&sev->list);
 		sev->fh = NULL;
 	}
-- 
1.7.7

[PATCH 3/5] v4l2-event: Don't set sev->fh to NULL on unsubscribe

[Hans de Goede]

Setting sev->fh to NULL causes problems for the del op added in the next
patch of this series, since this op needs a way to get to its own data
structures, and typically this will be done by using container_of on an
embedded v4l2_fh struct.

The reason the original code is setting sev->fh to NULL is to signal
to users of the event framework that the unsubscription has happened,
but since their is no shared lock between the event framework and users
of it, this is inherently racy, and it also turns out to be unnecessary
as long as both the event framework and the user of the framework do their
own locking properly and the user guarantees that it holds no references
to the subcribed_event structure after its del operation has been called.

This is best explained by looking at the only code currently checking for
sev->fh being set to NULL on unsubscribe, which is the v4l2-ctrls.c send_event
function. Here is the relevant code from v4l2-ctrls: send_event():

	if (sev->fh && (sev->fh != fh ||
			(sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK)))
		v4l2_event_queue_fh(sev->fh, &ev);

Now lets say that v4l2_event_unsubscribe and v4l2-ctrls: send_event() race
on the same sev, then the following could happens:

1) send_event checks sev->fh, finds it is not NULL
<thread switch>
2) v4l2_event_unsubscribe sets sev->fh NULL
3) v4l2_event_unsubscribe calls v4l2_ctrls del_event function, this blocks
   as the thread calling send_event holds the ctrl_lock
<thread switch>
4) send_event calls v4l2_event_queue_fh(sev->fh, &ev) which not is equivalent
   to calling: v4l2_event_queue_fh(NULL, &ev)
5) oops, NULL pointer deref.

Now again without setting sev->fh to NULL in v4l2_event_unsubscribe and
without the (now senseless since always true) sev->fh != NULL check in
send_event:

1) send_event is about to call v4l2_event_queue_fh(sev->fh, &ev)
<thread switch>
2) v4l2_event_unsubscribe removes sev->list from the fh->subscribed list
<thread switch>
3) send_event calls v4l2_event_queue_fh(sev->fh, &ev)
4) v4l2_event_queue_fh blocks on the fh_lock spinlock
<thread switch>
5) v4l2_event_unsubscribe unlocks the fh_lock spinlock
6) v4l2_event_unsubscribe calls v4l2_ctrls del_event function, this blocks
   as the thread calling send_event holds the ctrl_lock
<thread switch>
8) v4l2_event_queue_fh takes the fh_lock
7) v4l2_event_queue_fh calls v4l2_event_subscribed, does not find it since
   sev->list has been removed from fh->subscribed already -> does nothing
9) v4l2_event_queue_fh releases the fh_lock
10) the caller of send_event releases the ctrl lock (mutex)
<thread switch>
11) v4l2_ctrls del_event takes the ctrl lock
12) v4l2_ctrls del_event removes sev->node from the ev_subs list
13) v4l2_ctrls del_event releases the ctrl lock
14) v4l2_event_unsubscribe frees the sev, to which no references are being
    held anymore

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/media/video/v4l2-ctrls.c |    4 ++--
 drivers/media/video/v4l2-event.c |    1 -
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 69e24f4..1832a87 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -819,8 +819,8 @@ static void send_event(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 changes)
 	fill_event(&ev, ctrl, changes);
 
 	list_for_each_entry(sev, &ctrl->ev_subs, node)
-		if (sev->fh && (sev->fh != fh ||
-				(sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK)))
+		if (sev->fh != fh ||
+		    (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK))
 			v4l2_event_queue_fh(sev->fh, &ev);
 }
 
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 4d01f17..3d93251 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -302,7 +302,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 			fh->navailable--;
 		}
 		list_del(&sev->list);
-		sev->fh = NULL;
 	}
 
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
-- 
1.7.7

[PATCH 4/5] v4l2-event: Add v4l2_subscribed_event_ops

[Hans de Goede]

Just like with ctrl events, drivers may want to get called back on
listener add / remove for other event types too. Rather then special
casing all of this in subscribe / unsubscribe event it is better to
use ops for this.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 Documentation/video4linux/v4l2-framework.txt |   28 ++++++++++---
 drivers/media/video/ivtv/ivtv-ioctl.c        |    2 +-
 drivers/media/video/omap3isp/ispccdc.c       |    2 +-
 drivers/media/video/omap3isp/ispstat.c       |    2 +-
 drivers/media/video/pwc/pwc-v4l.c            |    2 +-
 drivers/media/video/v4l2-event.c             |   54 ++++++++++++++++++++-----
 drivers/media/video/vivi.c                   |    2 +-
 include/media/v4l2-event.h                   |   24 ++++++++---
 8 files changed, 86 insertions(+), 30 deletions(-)

diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
index f8dcabf..16eb8af 100644
--- a/Documentation/video4linux/v4l2-framework.txt
+++ b/Documentation/video4linux/v4l2-framework.txt
@@ -930,21 +930,35 @@ fast.
 
 Useful functions:
 
-- v4l2_event_queue()
+void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev)
 
   Queue events to video device. The driver's only responsibility is to fill
   in the type and the data fields. The other fields will be filled in by
   V4L2.
 
-- v4l2_event_subscribe()
+int v4l2_event_subscribe(struct v4l2_fh *fh,
+			 struct v4l2_event_subscription *sub, unsigned elems,
+			 const struct v4l2_subscribed_event_ops *ops)
 
   The video_device->ioctl_ops->vidioc_subscribe_event must check the driver
   is able to produce events with specified event id. Then it calls
-  v4l2_event_subscribe() to subscribe the event. The last argument is the
-  size of the event queue for this event. If it is 0, then the framework
-  will fill in a default value (this depends on the event type).
+  v4l2_event_subscribe() to subscribe the event.
 
-- v4l2_event_unsubscribe()
+  The elems argument is the size of the event queue for this event. If it is 0,
+  then the framework will fill in a default value (this depends on the event
+  type).
+
+  The ops argument allows the driver to specify a number of callbacks:
+  * add:     called when a new listener gets added (subscribing to the same
+             event twice will only cause this callback to get called once)
+  * del:     called when a listener stops listening
+  * replace: replace event 'old' with event 'new'.
+  * merge:   merge event 'old' into event 'new'.
+  All 4 callbacks are optional, if you don't want to specify any callbacks
+  the ops argument itself maybe NULL.
+
+int v4l2_event_unsubscribe(struct v4l2_fh *fh,
+			   struct v4l2_event_subscription *sub)
 
   vidioc_unsubscribe_event in struct v4l2_ioctl_ops. A driver may use
   v4l2_event_unsubscribe() directly unless it wants to be involved in
@@ -953,7 +967,7 @@ Useful functions:
   The special type V4L2_EVENT_ALL may be used to unsubscribe all events. The
   drivers may want to handle this in a special way.
 
-- v4l2_event_pending()
+int v4l2_event_pending(struct v4l2_fh *fh)
 
   Returns the number of pending events. Useful when implementing poll.
 
diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c b/drivers/media/video/ivtv/ivtv-ioctl.c
index ecafa69..9aec8a0 100644
--- a/drivers/media/video/ivtv/ivtv-ioctl.c
+++ b/drivers/media/video/ivtv/ivtv-ioctl.c
@@ -1456,7 +1456,7 @@ static int ivtv_subscribe_event(struct v4l2_fh *fh, struct v4l2_event_subscripti
 	case V4L2_EVENT_VSYNC:
 	case V4L2_EVENT_EOS:
 	case V4L2_EVENT_CTRL:
-		return v4l2_event_subscribe(fh, sub, 0);
+		return v4l2_event_subscribe(fh, sub, 0, NULL);
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/media/video/omap3isp/ispccdc.c b/drivers/media/video/omap3isp/ispccdc.c
index 40b141c..b6da736 100644
--- a/drivers/media/video/omap3isp/ispccdc.c
+++ b/drivers/media/video/omap3isp/ispccdc.c
@@ -1700,7 +1700,7 @@ static int ccdc_subscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
 	if (sub->id != 0)
 		return -EINVAL;
 
-	return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS);
+	return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS, NULL);
 }
 
 static int ccdc_unsubscribe_event(struct v4l2_subdev *sd, struct v4l2_fh *fh,
diff --git a/drivers/media/video/omap3isp/ispstat.c b/drivers/media/video/omap3isp/ispstat.c
index 8080659..4f337a2 100644
--- a/drivers/media/video/omap3isp/ispstat.c
+++ b/drivers/media/video/omap3isp/ispstat.c
@@ -1049,7 +1049,7 @@ int omap3isp_stat_subscribe_event(struct v4l2_subdev *subdev,
 	if (sub->type != stat->event_type)
 		return -EINVAL;
 
-	return v4l2_event_subscribe(fh, sub, STAT_NEVENTS);
+	return v4l2_event_subscribe(fh, sub, STAT_NEVENTS, NULL);
 }
 
 int omap3isp_stat_unsubscribe_event(struct v4l2_subdev *subdev,
diff --git a/drivers/media/video/pwc/pwc-v4l.c b/drivers/media/video/pwc/pwc-v4l.c
index 68e1323..7f159bf 100644
--- a/drivers/media/video/pwc/pwc-v4l.c
+++ b/drivers/media/video/pwc/pwc-v4l.c
@@ -1138,7 +1138,7 @@ static int pwc_subscribe_event(struct v4l2_fh *fh,
 {
 	switch (sub->type) {
 	case V4L2_EVENT_CTRL:
-		return v4l2_event_subscribe(fh, sub, 0);
+		return v4l2_event_subscribe(fh, sub, 0, NULL);
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 3d93251..aad2a79 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -119,6 +119,14 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
 	if (sev == NULL)
 		return;
 
+	/*
+	 * If the event has been added to the fh->subscribed list, but its
+	 * add op has not completed yet elems will be 0, treat this as
+	 * not being subscribed.
+	 */
+	if (!sev->elems)
+		return;
+
 	/* Increase event sequence number on fh. */
 	fh->sequence++;
 
@@ -131,14 +139,14 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh, const struct v4l2_event *e
 		sev->first = sev_pos(sev, 1);
 		fh->navailable--;
 		if (sev->elems == 1) {
-			if (sev->replace) {
-				sev->replace(&kev->event, ev);
+			if (sev->ops && sev->ops->replace) {
+				sev->ops->replace(&kev->event, ev);
 				copy_payload = false;
 			}
-		} else if (sev->merge) {
+		} else if (sev->ops && sev->ops->merge) {
 			struct v4l2_kevent *second_oldest =
 				sev->events + sev_pos(sev, 0);
-			sev->merge(&kev->event, &second_oldest->event);
+			sev->ops->merge(&kev->event, &second_oldest->event);
 		}
 	}
 
@@ -207,8 +215,14 @@ static void ctrls_merge(const struct v4l2_event *old, struct v4l2_event *new)
 	new->u.ctrl.changes |= old->u.ctrl.changes;
 }
 
+static const struct v4l2_subscribed_event_ops ctrl_ops = {
+	.replace = ctrls_replace,
+	.merge = ctrls_merge,
+};
+
 int v4l2_event_subscribe(struct v4l2_fh *fh,
-			 struct v4l2_event_subscription *sub, unsigned elems)
+			 struct v4l2_event_subscription *sub, unsigned elems,
+			 const struct v4l2_subscribed_event_ops *ops)
 {
 	struct v4l2_subscribed_event *sev, *found_ev;
 	struct v4l2_ctrl *ctrl = NULL;
@@ -235,10 +249,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 	sev->id = sub->id;
 	sev->flags = sub->flags;
 	sev->fh = fh;
-	sev->elems = elems;
+	sev->ops = ops;
 	if (ctrl) {
-		sev->replace = ctrls_replace;
-		sev->merge = ctrls_merge;
+		sev->ops = &ctrl_ops;
 	}
 
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
@@ -247,12 +260,27 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 		list_add(&sev->list, &fh->subscribed);
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
 
-	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
-	if (found_ev)
+	if (found_ev) {
 		kfree(sev);
-	else if (ctrl)
+		return 0; /* Already listening */
+	}
+
+	if (sev->ops && sev->ops->add) {
+		int ret = sev->ops->add(sev);
+		if (ret) {
+			sev->ops = NULL;
+			v4l2_event_unsubscribe(fh, sub);
+			return ret;
+		}
+	}
+
+	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
+	if (ctrl)
 		v4l2_ctrl_add_event(ctrl, sev);
 
+	/* Mark as ready for use */
+	sev->elems = elems;
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
@@ -305,6 +333,10 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 	}
 
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+
+	if (sev && sev->ops && sev->ops->del)
+		sev->ops->del(sev);
+
 	if (sev && sev->type == V4L2_EVENT_CTRL) {
 		struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
 
diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
index c25787d..74ebbad 100644
--- a/drivers/media/video/vivi.c
+++ b/drivers/media/video/vivi.c
@@ -1013,7 +1013,7 @@ static int vidioc_subscribe_event(struct v4l2_fh *fh,
 {
 	switch (sub->type) {
 	case V4L2_EVENT_CTRL:
-		return v4l2_event_subscribe(fh, sub, 0);
+		return v4l2_event_subscribe(fh, sub, 0, NULL);
 	default:
 		return -EINVAL;
 	}
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index 5f14e88..88fa9a1 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -78,6 +78,19 @@ struct v4l2_kevent {
 	struct v4l2_event	event;
 };
 
+/** struct v4l2_subscribed_event_ops - Subscribed event operations.
+  * @add:	Optional callback, called when a new listener is added
+  * @del:	Optional callback, called when a listener stops listening
+  * @replace:	Optional callback that can replace event 'old' with event 'new'.
+  * @merge:	Optional callback that can merge event 'old' into event 'new'.
+  */
+struct v4l2_subscribed_event_ops {
+	int  (*add)(struct v4l2_subscribed_event *sev);
+	void (*del)(struct v4l2_subscribed_event *sev);
+	void (*replace)(struct v4l2_event *old, const struct v4l2_event *new);
+	void (*merge)(const struct v4l2_event *old, struct v4l2_event *new);
+};
+
 /** struct v4l2_subscribed_event - Internal struct representing a subscribed event.
   * @list:	List node for the v4l2_fh->subscribed list.
   * @type:	Event type.
@@ -85,8 +98,7 @@ struct v4l2_kevent {
   * @flags:	Copy of v4l2_event_subscription->flags.
   * @fh:	Filehandle that subscribed to this event.
   * @node:	List node that hooks into the object's event list (if there is one).
-  * @replace:	Optional callback that can replace event 'old' with event 'new'.
-  * @merge:	Optional callback that can merge event 'old' into event 'new'.
+  * @ops:	v4l2_subscribed_event_ops
   * @elems:	The number of elements in the events array.
   * @first:	The index of the events containing the oldest available event.
   * @in_use:	The number of queued events.
@@ -99,10 +111,7 @@ struct v4l2_subscribed_event {
 	u32			flags;
 	struct v4l2_fh		*fh;
 	struct list_head	node;
-	void			(*replace)(struct v4l2_event *old,
-					   const struct v4l2_event *new);
-	void			(*merge)(const struct v4l2_event *old,
-					 struct v4l2_event *new);
+	const struct v4l2_subscribed_event_ops *ops;
 	unsigned		elems;
 	unsigned		first;
 	unsigned		in_use;
@@ -115,7 +124,8 @@ 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);
 int v4l2_event_subscribe(struct v4l2_fh *fh,
-			 struct v4l2_event_subscription *sub, unsigned elems);
+			 struct v4l2_event_subscription *sub, unsigned elems,
+			 const struct v4l2_subscribed_event_ops *ops);
 int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 			   struct v4l2_event_subscription *sub);
 void v4l2_event_unsubscribe_all(struct v4l2_fh *fh);
-- 
1.7.7

[PATCH 5/5] v4l2-ctrls: Use v4l2_subscribed_event_ops

[Hans de Goede]

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/video/ivtv/ivtv-ioctl.c |    3 +-
 drivers/media/video/pwc/pwc-v4l.c     |    2 +-
 drivers/media/video/v4l2-ctrls.c      |   56 +++++++++++++++++++++++++++------
 drivers/media/video/v4l2-event.c      |   39 -----------------------
 drivers/media/video/vivi.c            |    2 +-
 include/media/v4l2-ctrls.h            |    7 ++--
 6 files changed, 53 insertions(+), 56 deletions(-)

diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c b/drivers/media/video/ivtv/ivtv-ioctl.c
index 9aec8a0..72fd74f 100644
--- a/drivers/media/video/ivtv/ivtv-ioctl.c
+++ b/drivers/media/video/ivtv/ivtv-ioctl.c
@@ -1455,8 +1455,9 @@ static int ivtv_subscribe_event(struct v4l2_fh *fh, struct v4l2_event_subscripti
 	switch (sub->type) {
 	case V4L2_EVENT_VSYNC:
 	case V4L2_EVENT_EOS:
-	case V4L2_EVENT_CTRL:
 		return v4l2_event_subscribe(fh, sub, 0, NULL);
+	case V4L2_EVENT_CTRL:
+		return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/media/video/pwc/pwc-v4l.c b/drivers/media/video/pwc/pwc-v4l.c
index 7f159bf..afc5b15 100644
--- a/drivers/media/video/pwc/pwc-v4l.c
+++ b/drivers/media/video/pwc/pwc-v4l.c
@@ -1138,7 +1138,7 @@ static int pwc_subscribe_event(struct v4l2_fh *fh,
 {
 	switch (sub->type) {
 	case V4L2_EVENT_CTRL:
-		return v4l2_event_subscribe(fh, sub, 0, NULL);
+		return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
 	default:
 		return -EINVAL;
 	}
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 1832a87..d69b3c4 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -2329,10 +2329,22 @@ int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
 }
 EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
 
-void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
-				struct v4l2_subscribed_event *sev)
+static int v4l2_ctrl_add_event(struct v4l2_subscribed_event *sev)
 {
-	v4l2_ctrl_lock(ctrl);
+	struct v4l2_ctrl_handler *hdl = sev->fh->ctrl_handler;
+	struct v4l2_ctrl_ref *ref;
+	struct v4l2_ctrl *ctrl;
+	int ret = 0;
+
+	mutex_lock(&hdl->lock);
+
+	ref = find_ref(hdl, sev->id);
+	if (!ref) {
+		ret = -EINVAL;
+		goto leave;
+	}
+	ctrl = ref->ctrl;
+
 	list_add_tail(&sev->node, &ctrl->ev_subs);
 	if (ctrl->type != V4L2_CTRL_TYPE_CTRL_CLASS &&
 	    (sev->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL)) {
@@ -2344,15 +2356,39 @@ void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
 		fill_event(&ev, ctrl, changes);
 		v4l2_event_queue_fh(sev->fh, &ev);
 	}
-	v4l2_ctrl_unlock(ctrl);
+leave:
+	mutex_unlock(&hdl->lock);
+	return ret;
 }
-EXPORT_SYMBOL(v4l2_ctrl_add_event);
 
-void v4l2_ctrl_del_event(struct v4l2_ctrl *ctrl,
-				struct v4l2_subscribed_event *sev)
+static void v4l2_ctrl_del_event(struct v4l2_subscribed_event *sev)
 {
-	v4l2_ctrl_lock(ctrl);
+	struct v4l2_ctrl_handler *hdl = sev->fh->ctrl_handler;
+
+	mutex_lock(&hdl->lock);
 	list_del(&sev->node);
-	v4l2_ctrl_unlock(ctrl);
+	mutex_unlock(&hdl->lock);
 }
-EXPORT_SYMBOL(v4l2_ctrl_del_event);
+
+void v4l2_ctrl_replace(struct v4l2_event *old, const struct v4l2_event *new)
+{
+	u32 old_changes = old->u.ctrl.changes;
+
+	old->u.ctrl = new->u.ctrl;
+	old->u.ctrl.changes |= old_changes;
+}
+EXPORT_SYMBOL(v4l2_ctrl_replace);
+
+void v4l2_ctrl_merge(const struct v4l2_event *old, struct v4l2_event *new)
+{
+	new->u.ctrl.changes |= old->u.ctrl.changes;
+}
+EXPORT_SYMBOL(v4l2_ctrl_merge);
+
+const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops = {
+	.add = v4l2_ctrl_add_event,
+	.del = v4l2_ctrl_del_event,
+	.replace = v4l2_ctrl_replace,
+	.merge = v4l2_ctrl_merge,
+};
+EXPORT_SYMBOL(v4l2_ctrl_sub_ev_ops);
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index aad2a79..31a303e 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -25,7 +25,6 @@
 #include <media/v4l2-dev.h>
 #include <media/v4l2-fh.h>
 #include <media/v4l2-event.h>
-#include <media/v4l2-ctrls.h>
 
 #include <linux/sched.h>
 #include <linux/slab.h>
@@ -202,30 +201,11 @@ int v4l2_event_pending(struct v4l2_fh *fh)
 }
 EXPORT_SYMBOL_GPL(v4l2_event_pending);
 
-static void ctrls_replace(struct v4l2_event *old, const struct v4l2_event *new)
-{
-	u32 old_changes = old->u.ctrl.changes;
-
-	old->u.ctrl = new->u.ctrl;
-	old->u.ctrl.changes |= old_changes;
-}
-
-static void ctrls_merge(const struct v4l2_event *old, struct v4l2_event *new)
-{
-	new->u.ctrl.changes |= old->u.ctrl.changes;
-}
-
-static const struct v4l2_subscribed_event_ops ctrl_ops = {
-	.replace = ctrls_replace,
-	.merge = ctrls_merge,
-};
-
 int v4l2_event_subscribe(struct v4l2_fh *fh,
 			 struct v4l2_event_subscription *sub, unsigned elems,
 			 const struct v4l2_subscribed_event_ops *ops)
 {
 	struct v4l2_subscribed_event *sev, *found_ev;
-	struct v4l2_ctrl *ctrl = NULL;
 	unsigned long flags;
 	unsigned i;
 
@@ -234,11 +214,6 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 
 	if (elems < 1)
 		elems = 1;
-	if (sub->type == V4L2_EVENT_CTRL) {
-		ctrl = v4l2_ctrl_find(fh->ctrl_handler, sub->id);
-		if (ctrl == NULL)
-			return -EINVAL;
-	}
 
 	sev = kzalloc(sizeof(*sev) + sizeof(struct v4l2_kevent) * elems, GFP_KERNEL);
 	if (!sev)
@@ -250,9 +225,6 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 	sev->flags = sub->flags;
 	sev->fh = fh;
 	sev->ops = ops;
-	if (ctrl) {
-		sev->ops = &ctrl_ops;
-	}
 
 	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
 	found_ev = v4l2_event_subscribed(fh, sub->type, sub->id);
@@ -274,10 +246,6 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 		}
 	}
 
-	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock */
-	if (ctrl)
-		v4l2_ctrl_add_event(ctrl, sev);
-
 	/* Mark as ready for use */
 	sev->elems = elems;
 
@@ -337,13 +305,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 	if (sev && sev->ops && sev->ops->del)
 		sev->ops->del(sev);
 
-	if (sev && sev->type == V4L2_EVENT_CTRL) {
-		struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev->id);
-
-		if (ctrl)
-			v4l2_ctrl_del_event(ctrl, sev);
-	}
-
 	kfree(sev);
 
 	return 0;
diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
index 74ebbad..ce1783d 100644
--- a/drivers/media/video/vivi.c
+++ b/drivers/media/video/vivi.c
@@ -1013,7 +1013,7 @@ static int vidioc_subscribe_event(struct v4l2_fh *fh,
 {
 	switch (sub->type) {
 	case V4L2_EVENT_CTRL:
-		return v4l2_event_subscribe(fh, sub, 0, NULL);
+		return v4l2_event_subscribe(fh, sub, 0, &v4l2_ctrl_sub_ev_ops);
 	default:
 		return -EINVAL;
 	}
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index eeb3df6..fde17ff 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -487,10 +487,9 @@ s32 v4l2_ctrl_g_ctrl(struct v4l2_ctrl *ctrl);
 int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val);
 
 /* Internal helper functions that deal with control events. */
-void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
-		struct v4l2_subscribed_event *sev);
-void v4l2_ctrl_del_event(struct v4l2_ctrl *ctrl,
-		struct v4l2_subscribed_event *sev);
+extern const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops;
+void v4l2_ctrl_replace(struct v4l2_event *old, const struct v4l2_event *new);
+void v4l2_ctrl_merge(const struct v4l2_event *old, struct v4l2_event *new);
 
 /* Helpers for ioctl_ops. If hdl == NULL then they will all return -EINVAL. */
 int v4l2_queryctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_queryctrl *qc);
-- 
1.7.7

Re: [PATCH 2/5] v4l2-event: Remove pending events from fh event queue when unsubscribing

[Hans Verkuil]

On Wednesday 02 November 2011 11:13:22 Hans de Goede wrote:
> The kev pointers inside the pending events queue (the available queue) of
> the fh point to data inside the sev, unsubscribing frees the sev, thus
> making these pointers point to freed memory!
> 
> This patch fixes these dangling pointers in the available queue by removing
> all matching pending events on unsubscription.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

> ---
>  drivers/media/video/v4l2-event.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-event.c
> b/drivers/media/video/v4l2-event.c index 9f56f18..4d01f17 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -285,6 +285,7 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  {
>  	struct v4l2_subscribed_event *sev;
>  	unsigned long flags;
> +	int i;
> 
>  	if (sub->type == V4L2_EVENT_ALL) {
>  		v4l2_event_unsubscribe_all(fh);
> @@ -295,6 +296,11 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> 
>  	sev = v4l2_event_subscribed(fh, sub->type, sub->id);
>  	if (sev != NULL) {
> +		/* Remove any pending events for this subscription */
> +		for (i = 0; i < sev->in_use; i++) {
> +			list_del(&sev->events[sev_pos(sev, i)].list);
> +			fh->navailable--;
> +		}
>  		list_del(&sev->list);
>  		sev->fh = NULL;
>  	}

Re: [PATCH 3/5] v4l2-event: Don't set sev->fh to NULL on unsubscribe

[Hans Verkuil]

On Wednesday 02 November 2011 11:13:23 Hans de Goede wrote:
> Setting sev->fh to NULL causes problems for the del op added in the next
> patch of this series, since this op needs a way to get to its own data
> structures, and typically this will be done by using container_of on an
> embedded v4l2_fh struct.
> 
> The reason the original code is setting sev->fh to NULL is to signal
> to users of the event framework that the unsubscription has happened,
> but since their is no shared lock between the event framework and users
> of it, this is inherently racy, and it also turns out to be unnecessary
> as long as both the event framework and the user of the framework do their
> own locking properly and the user guarantees that it holds no references
> to the subcribed_event structure after its del operation has been called.
> 
> This is best explained by looking at the only code currently checking for
> sev->fh being set to NULL on unsubscribe, which is the v4l2-ctrls.c
> send_event function. Here is the relevant code from v4l2-ctrls:
> send_event():
> 
> 	if (sev->fh && (sev->fh != fh ||
> 			(sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK)))
> 		v4l2_event_queue_fh(sev->fh, &ev);
> 
> Now lets say that v4l2_event_unsubscribe and v4l2-ctrls: send_event() race
> on the same sev, then the following could happens:
> 
> 1) send_event checks sev->fh, finds it is not NULL
> <thread switch>
> 2) v4l2_event_unsubscribe sets sev->fh NULL
> 3) v4l2_event_unsubscribe calls v4l2_ctrls del_event function, this blocks
>    as the thread calling send_event holds the ctrl_lock
> <thread switch>
> 4) send_event calls v4l2_event_queue_fh(sev->fh, &ev) which not is
> equivalent to calling: v4l2_event_queue_fh(NULL, &ev)
> 5) oops, NULL pointer deref.
> 
> Now again without setting sev->fh to NULL in v4l2_event_unsubscribe and
> without the (now senseless since always true) sev->fh != NULL check in
> send_event:
> 
> 1) send_event is about to call v4l2_event_queue_fh(sev->fh, &ev)
> <thread switch>
> 2) v4l2_event_unsubscribe removes sev->list from the fh->subscribed list
> <thread switch>
> 3) send_event calls v4l2_event_queue_fh(sev->fh, &ev)
> 4) v4l2_event_queue_fh blocks on the fh_lock spinlock
> <thread switch>
> 5) v4l2_event_unsubscribe unlocks the fh_lock spinlock
> 6) v4l2_event_unsubscribe calls v4l2_ctrls del_event function, this blocks
>    as the thread calling send_event holds the ctrl_lock
> <thread switch>
> 8) v4l2_event_queue_fh takes the fh_lock
> 7) v4l2_event_queue_fh calls v4l2_event_subscribed, does not find it since
>    sev->list has been removed from fh->subscribed already -> does nothing
> 9) v4l2_event_queue_fh releases the fh_lock
> 10) the caller of send_event releases the ctrl lock (mutex)
> <thread switch>
> 11) v4l2_ctrls del_event takes the ctrl lock
> 12) v4l2_ctrls del_event removes sev->node from the ev_subs list
> 13) v4l2_ctrls del_event releases the ctrl lock
> 14) v4l2_event_unsubscribe frees the sev, to which no references are being
>     held anymore
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

> ---
>  drivers/media/video/v4l2-ctrls.c |    4 ++--
>  drivers/media/video/v4l2-event.c |    1 -
>  2 files changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-ctrls.c
> b/drivers/media/video/v4l2-ctrls.c index 69e24f4..1832a87 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -819,8 +819,8 @@ static void send_event(struct v4l2_fh *fh, struct
> v4l2_ctrl *ctrl, u32 changes) fill_event(&ev, ctrl, changes);
> 
>  	list_for_each_entry(sev, &ctrl->ev_subs, node)
> -		if (sev->fh && (sev->fh != fh ||
> -				(sev->flags & 
V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK)))
> +		if (sev->fh != fh ||
> +		    (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK))
>  			v4l2_event_queue_fh(sev->fh, &ev);
>  }
> 
> diff --git a/drivers/media/video/v4l2-event.c
> b/drivers/media/video/v4l2-event.c index 4d01f17..3d93251 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -302,7 +302,6 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  			fh->navailable--;
>  		}
>  		list_del(&sev->list);
> -		sev->fh = NULL;
>  	}
> 
>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);

Re: [PATCH 4/5] v4l2-event: Add v4l2_subscribed_event_ops

[Hans Verkuil]

On Wednesday 02 November 2011 11:13:24 Hans de Goede wrote:
> Just like with ctrl events, drivers may want to get called back on
> listener add / remove for other event types too. Rather then special
> casing all of this in subscribe / unsubscribe event it is better to
> use ops for this.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

> ---
>  Documentation/video4linux/v4l2-framework.txt |   28 ++++++++++---
>  drivers/media/video/ivtv/ivtv-ioctl.c        |    2 +-
>  drivers/media/video/omap3isp/ispccdc.c       |    2 +-
>  drivers/media/video/omap3isp/ispstat.c       |    2 +-
>  drivers/media/video/pwc/pwc-v4l.c            |    2 +-
>  drivers/media/video/v4l2-event.c             |   54
> ++++++++++++++++++++----- drivers/media/video/vivi.c                   |  
>  2 +-
>  include/media/v4l2-event.h                   |   24 ++++++++---
>  8 files changed, 86 insertions(+), 30 deletions(-)
> 
> diff --git a/Documentation/video4linux/v4l2-framework.txt
> b/Documentation/video4linux/v4l2-framework.txt index f8dcabf..16eb8af
> 100644
> --- a/Documentation/video4linux/v4l2-framework.txt
> +++ b/Documentation/video4linux/v4l2-framework.txt
> @@ -930,21 +930,35 @@ fast.
> 
>  Useful functions:
> 
> -- v4l2_event_queue()
> +void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event
> *ev)
> 
>    Queue events to video device. The driver's only responsibility is to
> fill in the type and the data fields. The other fields will be filled in
> by V4L2.
> 
> -- v4l2_event_subscribe()
> +int v4l2_event_subscribe(struct v4l2_fh *fh,
> +			 struct v4l2_event_subscription *sub, unsigned elems,
> +			 const struct v4l2_subscribed_event_ops *ops)
> 
>    The video_device->ioctl_ops->vidioc_subscribe_event must check the
> driver is able to produce events with specified event id. Then it calls - 
> v4l2_event_subscribe() to subscribe the event. The last argument is the - 
> size of the event queue for this event. If it is 0, then the framework - 
> will fill in a default value (this depends on the event type).
> +  v4l2_event_subscribe() to subscribe the event.
> 
> -- v4l2_event_unsubscribe()
> +  The elems argument is the size of the event queue for this event. If it
> is 0, +  then the framework will fill in a default value (this depends on
> the event +  type).
> +
> +  The ops argument allows the driver to specify a number of callbacks:
> +  * add:     called when a new listener gets added (subscribing to the
> same +             event twice will only cause this callback to get called
> once) +  * del:     called when a listener stops listening
> +  * replace: replace event 'old' with event 'new'.
> +  * merge:   merge event 'old' into event 'new'.
> +  All 4 callbacks are optional, if you don't want to specify any callbacks
> +  the ops argument itself maybe NULL.
> +
> +int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> +			   struct v4l2_event_subscription *sub)
> 
>    vidioc_unsubscribe_event in struct v4l2_ioctl_ops. A driver may use
>    v4l2_event_unsubscribe() directly unless it wants to be involved in
> @@ -953,7 +967,7 @@ Useful functions:
>    The special type V4L2_EVENT_ALL may be used to unsubscribe all events.
> The drivers may want to handle this in a special way.
> 
> -- v4l2_event_pending()
> +int v4l2_event_pending(struct v4l2_fh *fh)
> 
>    Returns the number of pending events. Useful when implementing poll.
> 
> diff --git a/drivers/media/video/ivtv/ivtv-ioctl.c
> b/drivers/media/video/ivtv/ivtv-ioctl.c index ecafa69..9aec8a0 100644
> --- a/drivers/media/video/ivtv/ivtv-ioctl.c
> +++ b/drivers/media/video/ivtv/ivtv-ioctl.c
> @@ -1456,7 +1456,7 @@ static int ivtv_subscribe_event(struct v4l2_fh *fh,
> struct v4l2_event_subscripti case V4L2_EVENT_VSYNC:
>  	case V4L2_EVENT_EOS:
>  	case V4L2_EVENT_CTRL:
> -		return v4l2_event_subscribe(fh, sub, 0);
> +		return v4l2_event_subscribe(fh, sub, 0, NULL);
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/drivers/media/video/omap3isp/ispccdc.c
> b/drivers/media/video/omap3isp/ispccdc.c index 40b141c..b6da736 100644
> --- a/drivers/media/video/omap3isp/ispccdc.c
> +++ b/drivers/media/video/omap3isp/ispccdc.c
> @@ -1700,7 +1700,7 @@ static int ccdc_subscribe_event(struct v4l2_subdev
> *sd, struct v4l2_fh *fh, if (sub->id != 0)
>  		return -EINVAL;
> 
> -	return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS);
> +	return v4l2_event_subscribe(fh, sub, OMAP3ISP_CCDC_NEVENTS, NULL);
>  }
> 
>  static int ccdc_unsubscribe_event(struct v4l2_subdev *sd, struct v4l2_fh
> *fh, diff --git a/drivers/media/video/omap3isp/ispstat.c
> b/drivers/media/video/omap3isp/ispstat.c index 8080659..4f337a2 100644
> --- a/drivers/media/video/omap3isp/ispstat.c
> +++ b/drivers/media/video/omap3isp/ispstat.c
> @@ -1049,7 +1049,7 @@ int omap3isp_stat_subscribe_event(struct v4l2_subdev
> *subdev, if (sub->type != stat->event_type)
>  		return -EINVAL;
> 
> -	return v4l2_event_subscribe(fh, sub, STAT_NEVENTS);
> +	return v4l2_event_subscribe(fh, sub, STAT_NEVENTS, NULL);
>  }
> 
>  int omap3isp_stat_unsubscribe_event(struct v4l2_subdev *subdev,
> diff --git a/drivers/media/video/pwc/pwc-v4l.c
> b/drivers/media/video/pwc/pwc-v4l.c index 68e1323..7f159bf 100644
> --- a/drivers/media/video/pwc/pwc-v4l.c
> +++ b/drivers/media/video/pwc/pwc-v4l.c
> @@ -1138,7 +1138,7 @@ static int pwc_subscribe_event(struct v4l2_fh *fh,
>  {
>  	switch (sub->type) {
>  	case V4L2_EVENT_CTRL:
> -		return v4l2_event_subscribe(fh, sub, 0);
> +		return v4l2_event_subscribe(fh, sub, 0, NULL);
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/drivers/media/video/v4l2-event.c
> b/drivers/media/video/v4l2-event.c index 3d93251..aad2a79 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -119,6 +119,14 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
> const struct v4l2_event *e if (sev == NULL)
>  		return;
> 
> +	/*
> +	 * If the event has been added to the fh->subscribed list, but its
> +	 * add op has not completed yet elems will be 0, treat this as
> +	 * not being subscribed.
> +	 */
> +	if (!sev->elems)
> +		return;
> +
>  	/* Increase event sequence number on fh. */
>  	fh->sequence++;
> 
> @@ -131,14 +139,14 @@ static void __v4l2_event_queue_fh(struct v4l2_fh *fh,
> const struct v4l2_event *e sev->first = sev_pos(sev, 1);
>  		fh->navailable--;
>  		if (sev->elems == 1) {
> -			if (sev->replace) {
> -				sev->replace(&kev->event, ev);
> +			if (sev->ops && sev->ops->replace) {
> +				sev->ops->replace(&kev->event, ev);
>  				copy_payload = false;
>  			}
> -		} else if (sev->merge) {
> +		} else if (sev->ops && sev->ops->merge) {
>  			struct v4l2_kevent *second_oldest =
>  				sev->events + sev_pos(sev, 0);
> -			sev->merge(&kev->event, &second_oldest->event);
> +			sev->ops->merge(&kev->event, &second_oldest->event);
>  		}
>  	}
> 
> @@ -207,8 +215,14 @@ static void ctrls_merge(const struct v4l2_event *old,
> struct v4l2_event *new) new->u.ctrl.changes |= old->u.ctrl.changes;
>  }
> 
> +static const struct v4l2_subscribed_event_ops ctrl_ops = {
> +	.replace = ctrls_replace,
> +	.merge = ctrls_merge,
> +};
> +
>  int v4l2_event_subscribe(struct v4l2_fh *fh,
> -			 struct v4l2_event_subscription *sub, unsigned elems)
> +			 struct v4l2_event_subscription *sub, unsigned elems,
> +			 const struct v4l2_subscribed_event_ops *ops)
>  {
>  	struct v4l2_subscribed_event *sev, *found_ev;
>  	struct v4l2_ctrl *ctrl = NULL;
> @@ -235,10 +249,9 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>  	sev->id = sub->id;
>  	sev->flags = sub->flags;
>  	sev->fh = fh;
> -	sev->elems = elems;
> +	sev->ops = ops;
>  	if (ctrl) {
> -		sev->replace = ctrls_replace;
> -		sev->merge = ctrls_merge;
> +		sev->ops = &ctrl_ops;
>  	}
> 
>  	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> @@ -247,12 +260,27 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>  		list_add(&sev->list, &fh->subscribed);
>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> 
> -	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock 
*/
> -	if (found_ev)
> +	if (found_ev) {
>  		kfree(sev);
> -	else if (ctrl)
> +		return 0; /* Already listening */
> +	}
> +
> +	if (sev->ops && sev->ops->add) {
> +		int ret = sev->ops->add(sev);
> +		if (ret) {
> +			sev->ops = NULL;
> +			v4l2_event_unsubscribe(fh, sub);
> +			return ret;
> +		}
> +	}
> +
> +	/* v4l2_ctrl_add_event uses a mutex, so do this outside the spin lock 
*/
> +	if (ctrl)
>  		v4l2_ctrl_add_event(ctrl, sev);
> 
> +	/* Mark as ready for use */
> +	sev->elems = elems;
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
> @@ -305,6 +333,10 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  	}
> 
>  	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> +	if (sev && sev->ops && sev->ops->del)
> +		sev->ops->del(sev);
> +
>  	if (sev && sev->type == V4L2_EVENT_CTRL) {
>  		struct v4l2_ctrl *ctrl = v4l2_ctrl_find(fh->ctrl_handler, sev-
>id);
> 
> diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
> index c25787d..74ebbad 100644
> --- a/drivers/media/video/vivi.c
> +++ b/drivers/media/video/vivi.c
> @@ -1013,7 +1013,7 @@ static int vidioc_subscribe_event(struct v4l2_fh *fh,
>  {
>  	switch (sub->type) {
>  	case V4L2_EVENT_CTRL:
> -		return v4l2_event_subscribe(fh, sub, 0);
> +		return v4l2_event_subscribe(fh, sub, 0, NULL);
>  	default:
>  		return -EINVAL;
>  	}
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> index 5f14e88..88fa9a1 100644
> --- a/include/media/v4l2-event.h
> +++ b/include/media/v4l2-event.h
> @@ -78,6 +78,19 @@ struct v4l2_kevent {
>  	struct v4l2_event	event;
>  };
> 
> +/** struct v4l2_subscribed_event_ops - Subscribed event operations.
> +  * @add:	Optional callback, called when a new listener is added
> +  * @del:	Optional callback, called when a listener stops listening
> +  * @replace:	Optional callback that can replace event 'old' with 
event
> 'new'. +  * @merge:	Optional callback that can merge event 'old' into
> event 'new'. +  */
> +struct v4l2_subscribed_event_ops {
> +	int  (*add)(struct v4l2_subscribed_event *sev);
> +	void (*del)(struct v4l2_subscribed_event *sev);
> +	void (*replace)(struct v4l2_event *old, const struct v4l2_event *new);
> +	void (*merge)(const struct v4l2_event *old, struct v4l2_event *new);
> +};
> +
>  /** struct v4l2_subscribed_event - Internal struct representing a
> subscribed event. * @list:	List node for the v4l2_fh->subscribed list.
>    * @type:	Event type.
> @@ -85,8 +98,7 @@ struct v4l2_kevent {
>    * @flags:	Copy of v4l2_event_subscription->flags.
>    * @fh:	Filehandle that subscribed to this event.
>    * @node:	List node that hooks into the object's event list (if there is
> one). -  * @replace:	Optional callback that can replace event 'old' with
> event 'new'. -  * @merge:	Optional callback that can merge event 'old'
> into event 'new'. +  * @ops:	v4l2_subscribed_event_ops
>    * @elems:	The number of elements in the events array.
>    * @first:	The index of the events containing the oldest available event.
>    * @in_use:	The number of queued events.
> @@ -99,10 +111,7 @@ struct v4l2_subscribed_event {
>  	u32			flags;
>  	struct v4l2_fh		*fh;
>  	struct list_head	node;
> -	void			(*replace)(struct v4l2_event *old,
> -					   const struct v4l2_event *new);
> -	void			(*merge)(const struct v4l2_event *old,
> -					 struct v4l2_event *new);
> +	const struct v4l2_subscribed_event_ops *ops;
>  	unsigned		elems;
>  	unsigned		first;
>  	unsigned		in_use;
> @@ -115,7 +124,8 @@ 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);
>  int v4l2_event_subscribe(struct v4l2_fh *fh,
> -			 struct v4l2_event_subscription *sub, unsigned elems);
> +			 struct v4l2_event_subscription *sub, unsigned elems,
> +			 const struct v4l2_subscribed_event_ops *ops);
>  int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  			   struct v4l2_event_subscription *sub);
>  void v4l2_event_unsubscribe_all(struct v4l2_fh *fh);

Re: Various ctrl and event frame work patches (version 3)

[Hans de Goede]

Hi Hans V.,

Thanks for the review and the acks. So do you want to get these
patches in through my tree, or through yours?

I believe we should get patches 1-3 into 3.2, 4 & 5 could also
go to 3.2, or we can delay them to 3.3 .

Regards,

Hansg

Re: Various ctrl and event frame work patches (version 3)

[Hans Verkuil]

On Wednesday 02 November 2011 11:52:03 Hans de Goede wrote:
> Hi Hans V.,
> 
> Thanks for the review and the acks. So do you want to get these
> patches in through my tree, or through yours?

Your tree is fine.

> I believe we should get patches 1-3 into 3.2, 4 & 5 could also
> go to 3.2, or we can delay them to 3.3 .

I would delay patches 4 & 5 to 3.3.

	Hans

> 
> Regards,
> 
> Hansg