[PATCH v8 0/3] uvcvideo: asynchronous controls

[PATCH v8 0/3] uvcvideo: asynchronous controls

[Guennadi Liakhovetski]

Added a patch to remove a redundant check, addressed Laurent's
comments.

Guennadi Liakhovetski (3):
  uvcvideo: remove a redundant check
  uvcvideo: send a control event when a Control Change interrupt arrives
  uvcvideo: handle control pipe protocol STALLs

 drivers/media/usb/uvc/uvc_ctrl.c   | 168 ++++++++++++++++++++++++++++++-------
 drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++---
 drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
 drivers/media/usb/uvc/uvc_video.c  |  52 ++++++++++--
 drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
 include/uapi/linux/uvcvideo.h      |   2 +
 6 files changed, 302 insertions(+), 51 deletions(-)

-- 
1.9.3

Thanks
Guennadi

[PATCH v8 1/3] uvcvideo: remove a redundant check

[Guennadi Liakhovetski]

Event subscribers cannot have a NULL file handle. They are only added
at a single location in the code, and the .fh pointer is used without
checking there.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index a36b4fb..2a213c8 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1229,9 +1229,9 @@ static void uvc_ctrl_send_event(struct uvc_fh *handle,
 	uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, value, changes);
 
 	list_for_each_entry(sev, &mapping->ev_subs, node) {
-		if (sev->fh && (sev->fh != &handle->vfh ||
+		if (sev->fh != &handle->vfh ||
 		    (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK) ||
-		    (changes & V4L2_EVENT_CTRL_CH_FLAGS)))
+		    (changes & V4L2_EVENT_CTRL_CH_FLAGS))
 			v4l2_event_queue_fh(sev->fh, &ev);
 	}
 }
-- 
1.9.3

Re: [PATCH v8 1/3] uvcvideo: remove a redundant check

[Laurent Pinchart]

Hi Guennadi,

Thank you for the patch.

On Tuesday, 8 May 2018 18:07:42 EEST Guennadi Liakhovetski wrote:
> Event subscribers cannot have a NULL file handle. They are only added
> at a single location in the code, and the .fh pointer is used without
> checking there.
> 
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

and applied to my tree. I'll proceed to patches 2/3 and 3/3 tomorrow 
(Wednesday).

> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> b/drivers/media/usb/uvc/uvc_ctrl.c index a36b4fb..2a213c8 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1229,9 +1229,9 @@ static void uvc_ctrl_send_event(struct uvc_fh *handle,
> uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, value, changes);
> 
>  	list_for_each_entry(sev, &mapping->ev_subs, node) {
> -		if (sev->fh && (sev->fh != &handle->vfh ||
> +		if (sev->fh != &handle->vfh ||
>  		    (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK) ||
> -		    (changes & V4L2_EVENT_CTRL_CH_FLAGS)))
> +		    (changes & V4L2_EVENT_CTRL_CH_FLAGS))
>  			v4l2_event_queue_fh(sev->fh, &ev);
>  	}
>  }

-- 
Regards,

Laurent Pinchart

[PATCH v8 2/3] uvcvideo: send a control event when a Control Change interrupt arrives

[Guennadi Liakhovetski]

UVC defines a method of handling asynchronous controls, which sends a
USB packet over the interrupt pipe. This patch implements support for
such packets by sending a control event to the user. Since this can
involve USB traffic and, therefore, scheduling, this has to be done
in a work queue.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
---

v8:

* avoid losing events by delaying the status URB resubmission until
  after completion of the current event
* extract control value calculation into __uvc_ctrl_get_value()
* do not proactively return EBUSY if the previous control hasn't
  completed yet, let the camera handle such cases
* multiple cosmetic changes

 drivers/media/usb/uvc/uvc_ctrl.c   | 166 ++++++++++++++++++++++++++++++-------
 drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++---
 drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
 drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
 include/uapi/linux/uvcvideo.h      |   2 +
 5 files changed, 255 insertions(+), 44 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 2a213c8..796f86a 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -20,6 +20,7 @@
 #include <linux/videodev2.h>
 #include <linux/vmalloc.h>
 #include <linux/wait.h>
+#include <linux/workqueue.h>
 #include <linux/atomic.h>
 #include <media/v4l2-ctrls.h>
 
@@ -971,12 +972,30 @@ static int uvc_ctrl_populate_cache(struct uvc_video_chain *chain,
 	return 0;
 }
 
+static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
+				const u8 *data)
+{
+	s32 value = mapping->get(mapping, UVC_GET_CUR, data);
+
+	if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
+		struct uvc_menu_info *menu = mapping->menu_info;
+		unsigned int i;
+
+		for (i = 0; i < mapping->menu_count; ++i, ++menu) {
+			if (menu->value == value) {
+				value = i;
+				break;
+			}
+		}
+	}
+
+	return value;
+}
+
 static int __uvc_ctrl_get(struct uvc_video_chain *chain,
 	struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
 	s32 *value)
 {
-	struct uvc_menu_info *menu;
-	unsigned int i;
 	int ret;
 
 	if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
@@ -993,18 +1012,8 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain,
 		ctrl->loaded = 1;
 	}
 
-	*value = mapping->get(mapping, UVC_GET_CUR,
-		uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
-
-	if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
-		menu = mapping->menu_info;
-		for (i = 0; i < mapping->menu_count; ++i, ++menu) {
-			if (menu->value == *value) {
-				*value = i;
-				break;
-			}
-		}
-	}
+	*value = __uvc_ctrl_get_value(mapping,
+				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
 
 	return 0;
 }
@@ -1222,30 +1231,121 @@ static void uvc_ctrl_send_event(struct uvc_fh *handle,
 {
 	struct v4l2_subscribed_event *sev;
 	struct v4l2_event ev;
+	bool autoupdate;
 
 	if (list_empty(&mapping->ev_subs))
 		return;
 
+	if (!handle) {
+		autoupdate = true;
+		sev = list_first_entry(&mapping->ev_subs,
+				       struct v4l2_subscribed_event, node);
+		handle = container_of(sev->fh, struct uvc_fh, vfh);
+	} else {
+		autoupdate = false;
+	}
+
 	uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, value, changes);
 
 	list_for_each_entry(sev, &mapping->ev_subs, node) {
 		if (sev->fh != &handle->vfh ||
 		    (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK) ||
-		    (changes & V4L2_EVENT_CTRL_CH_FLAGS))
+		    (changes & V4L2_EVENT_CTRL_CH_FLAGS) || autoupdate)
 			v4l2_event_queue_fh(sev->fh, &ev);
 	}
 }
 
-static void uvc_ctrl_send_slave_event(struct uvc_fh *handle,
-	struct uvc_control *master, u32 slave_id,
-	const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
+static void __uvc_ctrl_send_slave_event(struct uvc_fh *handle,
+	struct uvc_video_chain *chain, struct uvc_control *master, u32 slave_id)
 {
 	struct uvc_control_mapping *mapping = NULL;
 	struct uvc_control *ctrl = NULL;
 	u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
-	unsigned int i;
 	s32 val = 0;
 
+	__uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0);
+	if (ctrl == NULL)
+		return;
+
+	if (__uvc_ctrl_get(handle ? handle->chain : chain, ctrl, mapping,
+			   &val) == 0)
+		changes |= V4L2_EVENT_CTRL_CH_VALUE;
+
+	uvc_ctrl_send_event(handle, ctrl, mapping, val, changes);
+}
+
+static void uvc_ctrl_status_event_work(struct work_struct *work)
+{
+	struct uvc_device *dev = container_of(work, struct uvc_device,
+					      async_ctrl.work);
+	struct uvc_ctrl_work *w = &dev->async_ctrl;
+	struct uvc_control_mapping *mapping;
+	struct uvc_control *ctrl = w->ctrl;
+	unsigned int i;
+	int ret;
+
+	mutex_lock(&w->chain->ctrl_mutex);
+
+	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
+		s32 value = __uvc_ctrl_get_value(mapping, w->data);
+
+		/*
+		 * So far none of the auto-update controls in the uvc_ctrls[]
+		 * table is mapped to a V4L control with slaves in the
+		 * uvc_ctrl_mappings[] list, so slave controls so far never have
+		 * handle == NULL, but this can change in the future
+		 */
+		for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
+			if (!mapping->slave_ids[i])
+				break;
+
+			__uvc_ctrl_send_slave_event(ctrl->handle, w->chain,
+						ctrl, mapping->slave_ids[i]);
+		}
+
+		uvc_ctrl_send_event(ctrl->handle, ctrl, mapping, value,
+				    V4L2_EVENT_CTRL_CH_VALUE);
+	}
+
+	mutex_unlock(&w->chain->ctrl_mutex);
+
+	ctrl->handle = NULL;
+
+	/* Resubmit the URB. */
+	w->urb->interval = dev->int_ep->desc.bInterval;
+	ret = usb_submit_urb(w->urb, GFP_KERNEL);
+	if (ret < 0)
+		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
+			   ret);
+}
+
+bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,
+			   struct uvc_control *ctrl, const u8 *data)
+{
+	struct uvc_device *dev = chain->dev;
+	struct uvc_ctrl_work *w = &dev->async_ctrl;
+
+	if (list_empty(&ctrl->info.mappings)) {
+		ctrl->handle = NULL;
+		return false;
+	}
+
+	w->data = data;
+	w->urb = urb;
+	w->chain = chain;
+	w->ctrl = ctrl;
+
+	schedule_work(&w->work);
+
+	return true;
+}
+
+static void uvc_ctrl_send_slave_event(struct uvc_fh *handle,
+	struct uvc_control *master, u32 slave_id,
+	const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
+{
+	unsigned int i;
+
 	/*
 	 * We can skip sending an event for the slave if the slave
 	 * is being modified in the same transaction.
@@ -1255,14 +1355,8 @@ static void uvc_ctrl_send_slave_event(struct uvc_fh *handle,
 			return;
 	}
 
-	__uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0);
-	if (ctrl == NULL)
-		return;
-
-	if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0)
-		changes |= V4L2_EVENT_CTRL_CH_VALUE;
-
-	uvc_ctrl_send_event(handle, ctrl, mapping, val, changes);
+	/* handle != NULL */
+	__uvc_ctrl_send_slave_event(handle, NULL, master, slave_id);
 }
 
 static void uvc_ctrl_send_events(struct uvc_fh *handle,
@@ -1277,6 +1371,10 @@ static void uvc_ctrl_send_events(struct uvc_fh *handle,
 	for (i = 0; i < xctrls_count; ++i) {
 		ctrl = uvc_find_control(handle->chain, xctrls[i].id, &mapping);
 
+		if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
+			/* Notification will be sent from an Interrupt event. */
+			continue;
+
 		for (j = 0; j < ARRAY_SIZE(mapping->slave_ids); ++j) {
 			if (!mapping->slave_ids[j])
 				break;
@@ -1472,9 +1570,10 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
 	return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value);
 }
 
-int uvc_ctrl_set(struct uvc_video_chain *chain,
+int uvc_ctrl_set(struct uvc_fh *handle,
 	struct v4l2_ext_control *xctrl)
 {
+	struct uvc_video_chain *chain = handle->chain;
 	struct uvc_control *ctrl;
 	struct uvc_control_mapping *mapping;
 	s32 value;
@@ -1581,6 +1680,9 @@ int uvc_ctrl_set(struct uvc_video_chain *chain,
 	mapping->set(mapping, value,
 		uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
 
+	if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
+		ctrl->handle = handle;
+
 	ctrl->dirty = 1;
 	ctrl->modified = 1;
 	return 0;
@@ -1612,7 +1714,9 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
 			    |  (data[0] & UVC_CONTROL_CAP_SET ?
 				UVC_CTRL_FLAG_SET_CUR : 0)
 			    |  (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
-				UVC_CTRL_FLAG_AUTO_UPDATE : 0);
+				UVC_CTRL_FLAG_AUTO_UPDATE : 0)
+			    |  (data[0] & UVC_CONTROL_CAP_ASYNCHRONOUS ?
+				UVC_CTRL_FLAG_ASYNCHRONOUS : 0);
 
 	kfree(data);
 	return ret;
@@ -2173,6 +2277,8 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
 	struct uvc_entity *entity;
 	unsigned int i;
 
+	INIT_WORK(&dev->async_ctrl.work, uvc_ctrl_status_event_work);
+
 	/* Walk the entities list and instantiate controls */
 	list_for_each_entry(entity, &dev->entities, list) {
 		struct uvc_control *ctrl;
@@ -2241,6 +2347,8 @@ void uvc_ctrl_cleanup_device(struct uvc_device *dev)
 	struct uvc_entity *entity;
 	unsigned int i;
 
+	cancel_work_sync(&dev->async_ctrl.work);
+
 	/* Free controls and control mappings for all entities. */
 	list_for_each_entry(entity, &dev->entities, list) {
 		for (i = 0; i < entity->ncontrols; ++i) {
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index 7b71041..a0f2fea 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -78,7 +78,24 @@ static void uvc_input_report_key(struct uvc_device *dev, unsigned int code,
 /* --------------------------------------------------------------------------
  * Status interrupt endpoint
  */
-static void uvc_event_streaming(struct uvc_device *dev, u8 *data, int len)
+struct uvc_streaming_status {
+	u8	bStatusType;
+	u8	bOriginator;
+	u8	bEvent;
+	u8	bValue[];
+} __packed;
+
+struct uvc_control_status {
+	u8	bStatusType;
+	u8	bOriginator;
+	u8	bEvent;
+	u8	bSelector;
+	u8	bAttribute;
+	u8	bValue[];
+} __packed;
+
+static void uvc_event_streaming(struct uvc_device *dev,
+				struct uvc_streaming_status *status, int len)
 {
 	if (len < 3) {
 		uvc_trace(UVC_TRACE_STATUS, "Invalid streaming status event "
@@ -86,31 +103,98 @@ static void uvc_event_streaming(struct uvc_device *dev, u8 *data, int len)
 		return;
 	}
 
-	if (data[2] == 0) {
+	if (status->bEvent == 0) {
 		if (len < 4)
 			return;
 		uvc_trace(UVC_TRACE_STATUS, "Button (intf %u) %s len %d\n",
-			data[1], data[3] ? "pressed" : "released", len);
-		uvc_input_report_key(dev, KEY_CAMERA, data[3]);
+			  status->bOriginator,
+			  status->bValue[0] ? "pressed" : "released", len);
+		uvc_input_report_key(dev, KEY_CAMERA, status->bValue[0]);
 	} else {
 		uvc_trace(UVC_TRACE_STATUS,
 			  "Stream %u error event %02x len %d.\n",
-			  data[1], data[2], len);
+			  status->bOriginator, status->bEvent, len);
 	}
 }
 
-static void uvc_event_control(struct uvc_device *dev, u8 *data, int len)
+#define UVC_CTRL_VALUE_CHANGE	0
+#define UVC_CTRL_INFO_CHANGE	1
+#define UVC_CTRL_FAILURE_CHANGE	2
+#define UVC_CTRL_MIN_CHANGE	3
+#define UVC_CTRL_MAX_CHANGE	4
+
+static struct uvc_control *uvc_event_entity_find_ctrl(struct uvc_entity *entity,
+						      u8 selector)
 {
-	char *attrs[3] = { "value", "info", "failure" };
+	struct uvc_control *ctrl;
+	unsigned int i;
+
+	for (i = 0, ctrl = entity->controls; i < entity->ncontrols; i++, ctrl++)
+		if (ctrl->info.selector == selector)
+			return ctrl;
 
-	if (len < 6 || data[2] != 0 || data[4] > 2) {
+	return NULL;
+}
+
+static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
+					const struct uvc_control_status *status,
+					struct uvc_video_chain **chain)
+{
+	list_for_each_entry((*chain), &dev->chains, list) {
+		struct uvc_entity *entity;
+		struct uvc_control *ctrl;
+
+		list_for_each_entry(entity, &(*chain)->entities, chain) {
+			if (entity->id != status->bOriginator)
+				continue;
+
+			ctrl = uvc_event_entity_find_ctrl(entity,
+							  status->bSelector);
+			if (ctrl && (!ctrl->handle ||
+				     ctrl->handle->chain == *chain))
+				return ctrl;
+		}
+	}
+
+	return NULL;
+}
+
+static bool uvc_event_control(struct urb *urb,
+			      const struct uvc_control_status *status, int len)
+{
+	static const char *attrs[] = { "value", "info", "failure", "min", "max" };
+	struct uvc_device *dev = urb->context;
+	struct uvc_video_chain *chain;
+	struct uvc_control *ctrl;
+
+	if (len < 6 || status->bEvent != 0 ||
+	    status->bAttribute >= ARRAY_SIZE(attrs)) {
 		uvc_trace(UVC_TRACE_STATUS, "Invalid control status event "
 				"received.\n");
-		return;
+		return false;
 	}
 
 	uvc_trace(UVC_TRACE_STATUS, "Control %u/%u %s change len %d.\n",
-		data[1], data[3], attrs[data[4]], len);
+		  status->bOriginator, status->bSelector,
+		  attrs[status->bAttribute], len);
+
+	/* Find the control. */
+	ctrl = uvc_event_find_ctrl(dev, status, &chain);
+	if (!ctrl)
+		return false;
+
+	switch (status->bAttribute) {
+	case UVC_CTRL_VALUE_CHANGE:
+		return uvc_ctrl_status_event(urb, chain, ctrl, status->bValue);
+
+	case UVC_CTRL_INFO_CHANGE:
+	case UVC_CTRL_FAILURE_CHANGE:
+	case UVC_CTRL_MIN_CHANGE:
+	case UVC_CTRL_MAX_CHANGE:
+		break;
+	}
+
+	return false;
 }
 
 static void uvc_status_complete(struct urb *urb)
@@ -139,11 +223,15 @@ static void uvc_status_complete(struct urb *urb)
 	if (len > 0) {
 		switch (dev->status[0] & 0x0f) {
 		case UVC_STATUS_TYPE_CONTROL:
-			uvc_event_control(dev, dev->status, len);
+			if (uvc_event_control(urb,
+				(struct uvc_control_status *)dev->status, len))
+				/* The URB will be resubmitted in work context */
+				return;
 			break;
 
 		case UVC_STATUS_TYPE_STREAMING:
-			uvc_event_streaming(dev, dev->status, len);
+			uvc_event_streaming(dev,
+				(struct uvc_streaming_status *)dev->status, len);
 			break;
 
 		default:
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index bd32914..18a7384 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -994,7 +994,7 @@ static int uvc_ioctl_s_ctrl(struct file *file, void *fh,
 	if (ret < 0)
 		return ret;
 
-	ret = uvc_ctrl_set(chain, &xctrl);
+	ret = uvc_ctrl_set(handle, &xctrl);
 	if (ret < 0) {
 		uvc_ctrl_rollback(handle);
 		return ret;
@@ -1069,7 +1069,7 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
 		return ret;
 
 	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
-		ret = uvc_ctrl_set(chain, ctrl);
+		ret = uvc_ctrl_set(handle, ctrl);
 		if (ret < 0) {
 			uvc_ctrl_rollback(handle);
 			ctrls->error_idx = commit ? ctrls->count : i;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index be5cf17..0e5e920 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -12,6 +12,7 @@
 #include <linux/usb/video.h>
 #include <linux/uvcvideo.h>
 #include <linux/videodev2.h>
+#include <linux/workqueue.h>
 #include <media/media-device.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
@@ -256,6 +257,8 @@ struct uvc_control {
 	   initialized:1;
 
 	u8 *uvc_data;
+
+	struct uvc_fh *handle;	/* Used for asynchronous event delivery */
 };
 
 struct uvc_format_desc {
@@ -600,6 +603,14 @@ struct uvc_device {
 	u8 *status;
 	struct input_dev *input;
 	char input_phys[64];
+
+	struct uvc_ctrl_work {
+		struct work_struct work;
+		struct urb *urb;
+		struct uvc_video_chain *chain;
+		struct uvc_control *ctrl;
+		const void *data;
+	} async_ctrl;
 };
 
 enum uvc_handle_state {
@@ -753,6 +764,8 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 int uvc_ctrl_init_device(struct uvc_device *dev);
 void uvc_ctrl_cleanup_device(struct uvc_device *dev);
 int uvc_ctrl_restore_values(struct uvc_device *dev);
+bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,
+			   struct uvc_control *ctrl, const u8 *data);
 
 int uvc_ctrl_begin(struct uvc_video_chain *chain);
 int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
@@ -770,7 +783,7 @@ static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
 }
 
 int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);
-int uvc_ctrl_set(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);
+int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl);
 
 int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
 		      struct uvc_xu_control_query *xqry);
diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
index 020714d..f80f05b 100644
--- a/include/uapi/linux/uvcvideo.h
+++ b/include/uapi/linux/uvcvideo.h
@@ -28,6 +28,8 @@
 #define UVC_CTRL_FLAG_RESTORE		(1 << 6)
 /* Control can be updated by the camera. */
 #define UVC_CTRL_FLAG_AUTO_UPDATE	(1 << 7)
+/* Control supports asynchronous reporting */
+#define UVC_CTRL_FLAG_ASYNCHRONOUS	(1 << 8)
 
 #define UVC_CTRL_FLAG_GET_RANGE \
 	(UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \
-- 
1.9.3

Re: [PATCH v8 2/3] uvcvideo: send a control event when a Control Change interrupt arrives

[Laurent Pinchart]

Hi Guennadi,

Thank you for the patch.

On Tuesday, 8 May 2018 18:07:43 EEST Guennadi Liakhovetski wrote:
> UVC defines a method of handling asynchronous controls, which sends a
> USB packet over the interrupt pipe. This patch implements support for
> such packets by sending a control event to the user. Since this can
> involve USB traffic and, therefore, scheduling, this has to be done
> in a work queue.
> 
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> ---
> 
> v8:
> 
> * avoid losing events by delaying the status URB resubmission until
>   after completion of the current event
> * extract control value calculation into __uvc_ctrl_get_value()
> * do not proactively return EBUSY if the previous control hasn't
>   completed yet, let the camera handle such cases
> * multiple cosmetic changes
> 
>  drivers/media/usb/uvc/uvc_ctrl.c   | 166 +++++++++++++++++++++++++++-------
>  drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++---
>  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
>  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
>  include/uapi/linux/uvcvideo.h      |   2 +
>  5 files changed, 255 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> b/drivers/media/usb/uvc/uvc_ctrl.c index 2a213c8..796f86a 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -20,6 +20,7 @@
>  #include <linux/videodev2.h>
>  #include <linux/vmalloc.h>
>  #include <linux/wait.h>
> +#include <linux/workqueue.h>
>  #include <linux/atomic.h>
>  #include <media/v4l2-ctrls.h>
> 
> @@ -971,12 +972,30 @@ static int uvc_ctrl_populate_cache(struct
> uvc_video_chain *chain, return 0;
>  }
> 
> +static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
> +				const u8 *data)
> +{
> +	s32 value = mapping->get(mapping, UVC_GET_CUR, data);
> +
> +	if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
> +		struct uvc_menu_info *menu = mapping->menu_info;
> +		unsigned int i;
> +
> +		for (i = 0; i < mapping->menu_count; ++i, ++menu) {
> +			if (menu->value == value) {
> +				value = i;
> +				break;
> +			}
> +		}
> +	}
> +
> +	return value;
> +}
> +
>  static int __uvc_ctrl_get(struct uvc_video_chain *chain,
>  	struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
>  	s32 *value)
>  {
> -	struct uvc_menu_info *menu;
> -	unsigned int i;
>  	int ret;
> 
>  	if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> @@ -993,18 +1012,8 @@ static int __uvc_ctrl_get(struct uvc_video_chain
> *chain, ctrl->loaded = 1;
>  	}
> 
> -	*value = mapping->get(mapping, UVC_GET_CUR,
> -		uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> -
> -	if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
> -		menu = mapping->menu_info;
> -		for (i = 0; i < mapping->menu_count; ++i, ++menu) {
> -			if (menu->value == *value) {
> -				*value = i;
> -				break;
> -			}
> -		}
> -	}
> +	*value = __uvc_ctrl_get_value(mapping,
> +				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> 
>  	return 0;
>  }
> @@ -1222,30 +1231,121 @@ static void uvc_ctrl_send_event(struct uvc_fh
> *handle, {
>  	struct v4l2_subscribed_event *sev;
>  	struct v4l2_event ev;
> +	bool autoupdate;
> 
>  	if (list_empty(&mapping->ev_subs))
>  		return;
> 
> +	if (!handle) {
> +		autoupdate = true;
> +		sev = list_first_entry(&mapping->ev_subs,
> +				       struct v4l2_subscribed_event, node);
> +		handle = container_of(sev->fh, struct uvc_fh, vfh);
> +	} else {
> +		autoupdate = false;
> +	}
> +
>  	uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, value, changes);
> 
>  	list_for_each_entry(sev, &mapping->ev_subs, node) {
>  		if (sev->fh != &handle->vfh ||
>  		    (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK) ||
> -		    (changes & V4L2_EVENT_CTRL_CH_FLAGS))
> +		    (changes & V4L2_EVENT_CTRL_CH_FLAGS) || autoupdate)
>  			v4l2_event_queue_fh(sev->fh, &ev);
>  	}
>  }
> 
> -static void uvc_ctrl_send_slave_event(struct uvc_fh *handle,
> -	struct uvc_control *master, u32 slave_id,
> -	const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
> +static void __uvc_ctrl_send_slave_event(struct uvc_fh *handle,
> +	struct uvc_video_chain *chain, struct uvc_control *master, u32 slave_id)
>  {
>  	struct uvc_control_mapping *mapping = NULL;
>  	struct uvc_control *ctrl = NULL;
>  	u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
> -	unsigned int i;
>  	s32 val = 0;
> 
> +	__uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0);
> +	if (ctrl == NULL)
> +		return;
> +
> +	if (__uvc_ctrl_get(handle ? handle->chain : chain, ctrl, mapping,
> +			   &val) == 0)
> +		changes |= V4L2_EVENT_CTRL_CH_VALUE;
> +
> +	uvc_ctrl_send_event(handle, ctrl, mapping, val, changes);
> +}
> +
> +static void uvc_ctrl_status_event_work(struct work_struct *work)
> +{
> +	struct uvc_device *dev = container_of(work, struct uvc_device,
> +					      async_ctrl.work);
> +	struct uvc_ctrl_work *w = &dev->async_ctrl;
> +	struct uvc_control_mapping *mapping;
> +	struct uvc_control *ctrl = w->ctrl;
> +	unsigned int i;
> +	int ret;
> +
> +	mutex_lock(&w->chain->ctrl_mutex);
> +
> +	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> +		s32 value = __uvc_ctrl_get_value(mapping, w->data);
> +
> +		/*
> +		 * So far none of the auto-update controls in the uvc_ctrls[]
> +		 * table is mapped to a V4L control with slaves in the
> +		 * uvc_ctrl_mappings[] list, so slave controls so far never have
> +		 * handle == NULL, but this can change in the future
> +		 */
> +		for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
> +			if (!mapping->slave_ids[i])
> +				break;
> +
> +			__uvc_ctrl_send_slave_event(ctrl->handle, w->chain,
> +						ctrl, mapping->slave_ids[i]);
> +		}
> +
> +		uvc_ctrl_send_event(ctrl->handle, ctrl, mapping, value,
> +				    V4L2_EVENT_CTRL_CH_VALUE);
> +	}
> +
> +	mutex_unlock(&w->chain->ctrl_mutex);
> +
> +	ctrl->handle = NULL;

Can't this race with a uvc_ctrl_set() call, resulting in ctrl->handle being 
NULL after the control gets set ?

> +	/* Resubmit the URB. */
> +	w->urb->interval = dev->int_ep->desc.bInterval;
> +	ret = usb_submit_urb(w->urb, GFP_KERNEL);
> +	if (ret < 0)
> +		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
> +			   ret);
> +}
> +
> +bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,
> +			   struct uvc_control *ctrl, const u8 *data)
> +{
> +	struct uvc_device *dev = chain->dev;
> +	struct uvc_ctrl_work *w = &dev->async_ctrl;
> +
> +	if (list_empty(&ctrl->info.mappings)) {
> +		ctrl->handle = NULL;
> +		return false;
> +	}
> +
> +	w->data = data;
> +	w->urb = urb;
> +	w->chain = chain;
> +	w->ctrl = ctrl;
> +
> +	schedule_work(&w->work);
> +
> +	return true;
> +}
> +
> +static void uvc_ctrl_send_slave_event(struct uvc_fh *handle,
> +	struct uvc_control *master, u32 slave_id,
> +	const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
> +{
> +	unsigned int i;
> +
>  	/*
>  	 * We can skip sending an event for the slave if the slave
>  	 * is being modified in the same transaction.
> @@ -1255,14 +1355,8 @@ static void uvc_ctrl_send_slave_event(struct uvc_fh
> *handle, return;
>  	}
> 
> -	__uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0);
> -	if (ctrl == NULL)
> -		return;
> -
> -	if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0)
> -		changes |= V4L2_EVENT_CTRL_CH_VALUE;
> -
> -	uvc_ctrl_send_event(handle, ctrl, mapping, val, changes);
> +	/* handle != NULL */
> +	__uvc_ctrl_send_slave_event(handle, NULL, master, slave_id);
>  }
> 
>  static void uvc_ctrl_send_events(struct uvc_fh *handle,
> @@ -1277,6 +1371,10 @@ static void uvc_ctrl_send_events(struct uvc_fh
> *handle, for (i = 0; i < xctrls_count; ++i) {
>  		ctrl = uvc_find_control(handle->chain, xctrls[i].id, &mapping);
> 
> +		if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> +			/* Notification will be sent from an Interrupt event. */
> +			continue;
> +
>  		for (j = 0; j < ARRAY_SIZE(mapping->slave_ids); ++j) {
>  			if (!mapping->slave_ids[j])
>  				break;
> @@ -1472,9 +1570,10 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
>  	return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value);
>  }
> 
> -int uvc_ctrl_set(struct uvc_video_chain *chain,
> +int uvc_ctrl_set(struct uvc_fh *handle,
>  	struct v4l2_ext_control *xctrl)
>  {
> +	struct uvc_video_chain *chain = handle->chain;
>  	struct uvc_control *ctrl;
>  	struct uvc_control_mapping *mapping;
>  	s32 value;
> @@ -1581,6 +1680,9 @@ int uvc_ctrl_set(struct uvc_video_chain *chain,
>  	mapping->set(mapping, value,
>  		uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> 
> +	if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> +		ctrl->handle = handle;
> +
>  	ctrl->dirty = 1;
>  	ctrl->modified = 1;
>  	return 0;
> @@ -1612,7 +1714,9 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
> 
>  			    |  (data[0] & UVC_CONTROL_CAP_SET ?
> 
>  				UVC_CTRL_FLAG_SET_CUR : 0)
> 
>  			    |  (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> 
> -				UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> +				UVC_CTRL_FLAG_AUTO_UPDATE : 0)
> +			    |  (data[0] & UVC_CONTROL_CAP_ASYNCHRONOUS ?
> +				UVC_CTRL_FLAG_ASYNCHRONOUS : 0);
> 
>  	kfree(data);
>  	return ret;
> @@ -2173,6 +2277,8 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
>  	struct uvc_entity *entity;
>  	unsigned int i;
> 
> +	INIT_WORK(&dev->async_ctrl.work, uvc_ctrl_status_event_work);
> +
>  	/* Walk the entities list and instantiate controls */
>  	list_for_each_entry(entity, &dev->entities, list) {
>  		struct uvc_control *ctrl;
> @@ -2241,6 +2347,8 @@ void uvc_ctrl_cleanup_device(struct uvc_device *dev)
>  	struct uvc_entity *entity;
>  	unsigned int i;
> 
> +	cancel_work_sync(&dev->async_ctrl.work);
> +
>  	/* Free controls and control mappings for all entities. */
>  	list_for_each_entry(entity, &dev->entities, list) {
>  		for (i = 0; i < entity->ncontrols; ++i) {
> diff --git a/drivers/media/usb/uvc/uvc_status.c
> b/drivers/media/usb/uvc/uvc_status.c index 7b71041..a0f2fea 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -78,7 +78,24 @@ static void uvc_input_report_key(struct uvc_device *dev,
> unsigned int code, /*
> --------------------------------------------------------------------------
> * Status interrupt endpoint
>   */
> -static void uvc_event_streaming(struct uvc_device *dev, u8 *data, int len)
> +struct uvc_streaming_status {
> +	u8	bStatusType;
> +	u8	bOriginator;
> +	u8	bEvent;
> +	u8	bValue[];
> +} __packed;
> +
> +struct uvc_control_status {
> +	u8	bStatusType;
> +	u8	bOriginator;
> +	u8	bEvent;
> +	u8	bSelector;
> +	u8	bAttribute;
> +	u8	bValue[];
> +} __packed;
> +
> +static void uvc_event_streaming(struct uvc_device *dev,
> +				struct uvc_streaming_status *status, int len)
>  {
>  	if (len < 3) {
>  		uvc_trace(UVC_TRACE_STATUS, "Invalid streaming status event "
> @@ -86,31 +103,98 @@ static void uvc_event_streaming(struct uvc_device *dev,
> u8 *data, int len) return;
>  	}
> 
> -	if (data[2] == 0) {
> +	if (status->bEvent == 0) {
>  		if (len < 4)
>  			return;
>  		uvc_trace(UVC_TRACE_STATUS, "Button (intf %u) %s len %d\n",
> -			data[1], data[3] ? "pressed" : "released", len);
> -		uvc_input_report_key(dev, KEY_CAMERA, data[3]);
> +			  status->bOriginator,
> +			  status->bValue[0] ? "pressed" : "released", len);
> +		uvc_input_report_key(dev, KEY_CAMERA, status->bValue[0]);
>  	} else {
>  		uvc_trace(UVC_TRACE_STATUS,
>  			  "Stream %u error event %02x len %d.\n",
> -			  data[1], data[2], len);
> +			  status->bOriginator, status->bEvent, len);
>  	}
>  }
> 
> -static void uvc_event_control(struct uvc_device *dev, u8 *data, int len)
> +#define UVC_CTRL_VALUE_CHANGE	0
> +#define UVC_CTRL_INFO_CHANGE	1
> +#define UVC_CTRL_FAILURE_CHANGE	2
> +#define UVC_CTRL_MIN_CHANGE	3
> +#define UVC_CTRL_MAX_CHANGE	4
> +
> +static struct uvc_control *uvc_event_entity_find_ctrl(struct uvc_entity
> *entity, +						      u8 selector)
>  {
> -	char *attrs[3] = { "value", "info", "failure" };
> +	struct uvc_control *ctrl;
> +	unsigned int i;
> +
> +	for (i = 0, ctrl = entity->controls; i < entity->ncontrols; i++, ctrl++)
> +		if (ctrl->info.selector == selector)
> +			return ctrl;
> 
> -	if (len < 6 || data[2] != 0 || data[4] > 2) {
> +	return NULL;
> +}
> +
> +static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
> +					const struct uvc_control_status *status,
> +					struct uvc_video_chain **chain)
> +{
> +	list_for_each_entry((*chain), &dev->chains, list) {
> +		struct uvc_entity *entity;
> +		struct uvc_control *ctrl;
> +
> +		list_for_each_entry(entity, &(*chain)->entities, chain) {
> +			if (entity->id != status->bOriginator)
> +				continue;
> +
> +			ctrl = uvc_event_entity_find_ctrl(entity,
> +							  status->bSelector);
> +			if (ctrl && (!ctrl->handle ||
> +				     ctrl->handle->chain == *chain))

I'm afraid I still don't understand why you need the chain check :-( Unless 
I'm mistaken, ctrl->handle is set in uvc_ctrl_set(), where the control is 
looked up from the chain corresponding to handle->chain. How can the check be 
false here ?

Those are my two major concerns. Apart from that I have other small concerns 
that I propose addressing myself to avoid further delays. I've been slow 
enough when it comes to reviewing this series, if we can clear the two issues 
above, I'll handle the rest.

> +				return ctrl;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +static bool uvc_event_control(struct urb *urb,
> +			      const struct uvc_control_status *status, int len)
> +{
> +	static const char *attrs[] = { "value", "info", "failure", "min", "max" };
> +	struct uvc_device *dev = urb->context;
> +	struct uvc_video_chain *chain;
> +	struct uvc_control *ctrl;
> +
> +	if (len < 6 || status->bEvent != 0 ||
> +	    status->bAttribute >= ARRAY_SIZE(attrs)) {
>  		uvc_trace(UVC_TRACE_STATUS, "Invalid control status event "
>  				"received.\n");
> -		return;
> +		return false;
>  	}
> 
>  	uvc_trace(UVC_TRACE_STATUS, "Control %u/%u %s change len %d.\n",
> -		data[1], data[3], attrs[data[4]], len);
> +		  status->bOriginator, status->bSelector,
> +		  attrs[status->bAttribute], len);
> +
> +	/* Find the control. */
> +	ctrl = uvc_event_find_ctrl(dev, status, &chain);
> +	if (!ctrl)
> +		return false;
> +
> +	switch (status->bAttribute) {
> +	case UVC_CTRL_VALUE_CHANGE:
> +		return uvc_ctrl_status_event(urb, chain, ctrl, status->bValue);
> +
> +	case UVC_CTRL_INFO_CHANGE:
> +	case UVC_CTRL_FAILURE_CHANGE:
> +	case UVC_CTRL_MIN_CHANGE:
> +	case UVC_CTRL_MAX_CHANGE:
> +		break;
> +	}
> +
> +	return false;
>  }
> 
>  static void uvc_status_complete(struct urb *urb)
> @@ -139,11 +223,15 @@ static void uvc_status_complete(struct urb *urb)
>  	if (len > 0) {
>  		switch (dev->status[0] & 0x0f) {
>  		case UVC_STATUS_TYPE_CONTROL:
> -			uvc_event_control(dev, dev->status, len);
> +			if (uvc_event_control(urb,
> +				(struct uvc_control_status *)dev->status, len))
> +				/* The URB will be resubmitted in work context */
> +				return;
>  			break;
> 
>  		case UVC_STATUS_TYPE_STREAMING:
> -			uvc_event_streaming(dev, dev->status, len);
> +			uvc_event_streaming(dev,
> +				(struct uvc_streaming_status *)dev->status, len);
>  			break;
> 
>  		default:
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c index bd32914..18a7384 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -994,7 +994,7 @@ static int uvc_ioctl_s_ctrl(struct file *file, void *fh,
> if (ret < 0)
>  		return ret;
> 
> -	ret = uvc_ctrl_set(chain, &xctrl);
> +	ret = uvc_ctrl_set(handle, &xctrl);
>  	if (ret < 0) {
>  		uvc_ctrl_rollback(handle);
>  		return ret;
> @@ -1069,7 +1069,7 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh
> *handle, return ret;
> 
>  	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> -		ret = uvc_ctrl_set(chain, ctrl);
> +		ret = uvc_ctrl_set(handle, ctrl);
>  		if (ret < 0) {
>  			uvc_ctrl_rollback(handle);
>  			ctrls->error_idx = commit ? ctrls->count : i;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index be5cf17..0e5e920 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -12,6 +12,7 @@
>  #include <linux/usb/video.h>
>  #include <linux/uvcvideo.h>
>  #include <linux/videodev2.h>
> +#include <linux/workqueue.h>
>  #include <media/media-device.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-event.h>
> @@ -256,6 +257,8 @@ struct uvc_control {
>  	   initialized:1;
> 
>  	u8 *uvc_data;
> +
> +	struct uvc_fh *handle;	/* Used for asynchronous event delivery */
>  };
> 
>  struct uvc_format_desc {
> @@ -600,6 +603,14 @@ struct uvc_device {
>  	u8 *status;
>  	struct input_dev *input;
>  	char input_phys[64];
> +
> +	struct uvc_ctrl_work {
> +		struct work_struct work;
> +		struct urb *urb;
> +		struct uvc_video_chain *chain;
> +		struct uvc_control *ctrl;
> +		const void *data;
> +	} async_ctrl;
>  };
> 
>  enum uvc_handle_state {
> @@ -753,6 +764,8 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>  int uvc_ctrl_init_device(struct uvc_device *dev);
>  void uvc_ctrl_cleanup_device(struct uvc_device *dev);
>  int uvc_ctrl_restore_values(struct uvc_device *dev);
> +bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,
> +			   struct uvc_control *ctrl, const u8 *data);
> 
>  int uvc_ctrl_begin(struct uvc_video_chain *chain);
>  int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> @@ -770,7 +783,7 @@ static inline int uvc_ctrl_rollback(struct uvc_fh
> *handle) }
> 
>  int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control
> *xctrl); -int uvc_ctrl_set(struct uvc_video_chain *chain, struct
> v4l2_ext_control *xctrl); +int uvc_ctrl_set(struct uvc_fh *handle, struct
> v4l2_ext_control *xctrl);
> 
>  int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
>  		      struct uvc_xu_control_query *xqry);
> diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
> index 020714d..f80f05b 100644
> --- a/include/uapi/linux/uvcvideo.h
> +++ b/include/uapi/linux/uvcvideo.h
> @@ -28,6 +28,8 @@
>  #define UVC_CTRL_FLAG_RESTORE		(1 << 6)
>  /* Control can be updated by the camera. */
>  #define UVC_CTRL_FLAG_AUTO_UPDATE	(1 << 7)
> +/* Control supports asynchronous reporting */
> +#define UVC_CTRL_FLAG_ASYNCHRONOUS	(1 << 8)
> 
>  #define UVC_CTRL_FLAG_GET_RANGE \
>  	(UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \


-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 2/3] uvcvideo: send a control event when a Control Change interrupt arrives

[Guennadi Liakhovetski]

Hi Laurent,

Thanks for the review.

On Thu, 12 Jul 2018, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thank you for the patch.
> 
> On Tuesday, 8 May 2018 18:07:43 EEST Guennadi Liakhovetski wrote:
> > UVC defines a method of handling asynchronous controls, which sends a
> > USB packet over the interrupt pipe. This patch implements support for
> > such packets by sending a control event to the user. Since this can
> > involve USB traffic and, therefore, scheduling, this has to be done
> > in a work queue.
> > 
> > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> > ---
> > 
> > v8:
> > 
> > * avoid losing events by delaying the status URB resubmission until
> >   after completion of the current event
> > * extract control value calculation into __uvc_ctrl_get_value()
> > * do not proactively return EBUSY if the previous control hasn't
> >   completed yet, let the camera handle such cases
> > * multiple cosmetic changes
> > 
> >  drivers/media/usb/uvc/uvc_ctrl.c   | 166 +++++++++++++++++++++++++++-------
> >  drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++---
> >  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
> >  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
> >  include/uapi/linux/uvcvideo.h      |   2 +
> >  5 files changed, 255 insertions(+), 44 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > b/drivers/media/usb/uvc/uvc_ctrl.c index 2a213c8..796f86a 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c

[snip]

> > +static void uvc_ctrl_status_event_work(struct work_struct *work)
> > +{
> > +	struct uvc_device *dev = container_of(work, struct uvc_device,
> > +					      async_ctrl.work);
> > +	struct uvc_ctrl_work *w = &dev->async_ctrl;
> > +	struct uvc_control_mapping *mapping;
> > +	struct uvc_control *ctrl = w->ctrl;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	mutex_lock(&w->chain->ctrl_mutex);
> > +
> > +	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> > +		s32 value = __uvc_ctrl_get_value(mapping, w->data);
> > +
> > +		/*
> > +		 * So far none of the auto-update controls in the uvc_ctrls[]
> > +		 * table is mapped to a V4L control with slaves in the
> > +		 * uvc_ctrl_mappings[] list, so slave controls so far never have
> > +		 * handle == NULL, but this can change in the future
> > +		 */
> > +		for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
> > +			if (!mapping->slave_ids[i])
> > +				break;
> > +
> > +			__uvc_ctrl_send_slave_event(ctrl->handle, w->chain,
> > +						ctrl, mapping->slave_ids[i]);
> > +		}
> > +
> > +		uvc_ctrl_send_event(ctrl->handle, ctrl, mapping, value,
> > +				    V4L2_EVENT_CTRL_CH_VALUE);
> > +	}
> > +
> > +	mutex_unlock(&w->chain->ctrl_mutex);
> > +
> > +	ctrl->handle = NULL;
> 
> Can't this race with a uvc_ctrl_set() call, resulting in ctrl->handle being 
> NULL after the control gets set ?

Right, it's better to set .handle to NULL before sending events. Something 
like

mutex_lock();

handle = ctrl->handle;
ctrl->handle = NULL;

list_for_each_entry() {
	...
	uvc_ctrl_send_event(handle,...);
}

mutex_unlock();

?

> 
> > +	/* Resubmit the URB. */
> > +	w->urb->interval = dev->int_ep->desc.bInterval;
> > +	ret = usb_submit_urb(w->urb, GFP_KERNEL);
> > +	if (ret < 0)
> > +		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
> > +			   ret);
> > +}
> > +
> > +bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,
> > +			   struct uvc_control *ctrl, const u8 *data)
> > +{
> > +	struct uvc_device *dev = chain->dev;
> > +	struct uvc_ctrl_work *w = &dev->async_ctrl;
> > +
> > +	if (list_empty(&ctrl->info.mappings)) {
> > +		ctrl->handle = NULL;
> > +		return false;
> > +	}
> > +
> > +	w->data = data;
> > +	w->urb = urb;
> > +	w->chain = chain;
> > +	w->ctrl = ctrl;
> > +
> > +	schedule_work(&w->work);
> > +
> > +	return true;
> > +}
> > +
> > +static void uvc_ctrl_send_slave_event(struct uvc_fh *handle,
> > +	struct uvc_control *master, u32 slave_id,
> > +	const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
> > +{
> > +	unsigned int i;
> > +
> >  	/*
> >  	 * We can skip sending an event for the slave if the slave
> >  	 * is being modified in the same transaction.
> > @@ -1255,14 +1355,8 @@ static void uvc_ctrl_send_slave_event(struct uvc_fh
> > *handle, return;
> >  	}
> > 
> > -	__uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0);
> > -	if (ctrl == NULL)
> > -		return;
> > -
> > -	if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0)
> > -		changes |= V4L2_EVENT_CTRL_CH_VALUE;
> > -
> > -	uvc_ctrl_send_event(handle, ctrl, mapping, val, changes);
> > +	/* handle != NULL */
> > +	__uvc_ctrl_send_slave_event(handle, NULL, master, slave_id);
> >  }
> > 
> >  static void uvc_ctrl_send_events(struct uvc_fh *handle,
> > @@ -1277,6 +1371,10 @@ static void uvc_ctrl_send_events(struct uvc_fh
> > *handle, for (i = 0; i < xctrls_count; ++i) {
> >  		ctrl = uvc_find_control(handle->chain, xctrls[i].id, &mapping);
> > 
> > +		if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> > +			/* Notification will be sent from an Interrupt event. */
> > +			continue;
> > +
> >  		for (j = 0; j < ARRAY_SIZE(mapping->slave_ids); ++j) {
> >  			if (!mapping->slave_ids[j])
> >  				break;
> > @@ -1472,9 +1570,10 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
> >  	return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value);
> >  }
> > 
> > -int uvc_ctrl_set(struct uvc_video_chain *chain,
> > +int uvc_ctrl_set(struct uvc_fh *handle,
> >  	struct v4l2_ext_control *xctrl)
> >  {
> > +	struct uvc_video_chain *chain = handle->chain;
> >  	struct uvc_control *ctrl;
> >  	struct uvc_control_mapping *mapping;
> >  	s32 value;
> > @@ -1581,6 +1680,9 @@ int uvc_ctrl_set(struct uvc_video_chain *chain,
> >  	mapping->set(mapping, value,
> >  		uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> > 
> > +	if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> > +		ctrl->handle = handle;
> > +
> >  	ctrl->dirty = 1;
> >  	ctrl->modified = 1;
> >  	return 0;
> > @@ -1612,7 +1714,9 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
> > 
> >  			    |  (data[0] & UVC_CONTROL_CAP_SET ?
> > 
> >  				UVC_CTRL_FLAG_SET_CUR : 0)
> > 
> >  			    |  (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> > 
> > -				UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> > +				UVC_CTRL_FLAG_AUTO_UPDATE : 0)
> > +			    |  (data[0] & UVC_CONTROL_CAP_ASYNCHRONOUS ?
> > +				UVC_CTRL_FLAG_ASYNCHRONOUS : 0);
> > 
> >  	kfree(data);
> >  	return ret;
> > @@ -2173,6 +2277,8 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
> >  	struct uvc_entity *entity;
> >  	unsigned int i;
> > 
> > +	INIT_WORK(&dev->async_ctrl.work, uvc_ctrl_status_event_work);
> > +
> >  	/* Walk the entities list and instantiate controls */
> >  	list_for_each_entry(entity, &dev->entities, list) {
> >  		struct uvc_control *ctrl;
> > @@ -2241,6 +2347,8 @@ void uvc_ctrl_cleanup_device(struct uvc_device *dev)
> >  	struct uvc_entity *entity;
> >  	unsigned int i;
> > 
> > +	cancel_work_sync(&dev->async_ctrl.work);
> > +
> >  	/* Free controls and control mappings for all entities. */
> >  	list_for_each_entry(entity, &dev->entities, list) {
> >  		for (i = 0; i < entity->ncontrols; ++i) {
> > diff --git a/drivers/media/usb/uvc/uvc_status.c
> > b/drivers/media/usb/uvc/uvc_status.c index 7b71041..a0f2fea 100644
> > --- a/drivers/media/usb/uvc/uvc_status.c
> > +++ b/drivers/media/usb/uvc/uvc_status.c
> > @@ -78,7 +78,24 @@ static void uvc_input_report_key(struct uvc_device *dev,
> > unsigned int code, /*
> > --------------------------------------------------------------------------
> > * Status interrupt endpoint
> >   */
> > -static void uvc_event_streaming(struct uvc_device *dev, u8 *data, int len)
> > +struct uvc_streaming_status {
> > +	u8	bStatusType;
> > +	u8	bOriginator;
> > +	u8	bEvent;
> > +	u8	bValue[];
> > +} __packed;
> > +
> > +struct uvc_control_status {
> > +	u8	bStatusType;
> > +	u8	bOriginator;
> > +	u8	bEvent;
> > +	u8	bSelector;
> > +	u8	bAttribute;
> > +	u8	bValue[];
> > +} __packed;
> > +
> > +static void uvc_event_streaming(struct uvc_device *dev,
> > +				struct uvc_streaming_status *status, int len)
> >  {
> >  	if (len < 3) {
> >  		uvc_trace(UVC_TRACE_STATUS, "Invalid streaming status event "
> > @@ -86,31 +103,98 @@ static void uvc_event_streaming(struct uvc_device *dev,
> > u8 *data, int len) return;
> >  	}
> > 
> > -	if (data[2] == 0) {
> > +	if (status->bEvent == 0) {
> >  		if (len < 4)
> >  			return;
> >  		uvc_trace(UVC_TRACE_STATUS, "Button (intf %u) %s len %d\n",
> > -			data[1], data[3] ? "pressed" : "released", len);
> > -		uvc_input_report_key(dev, KEY_CAMERA, data[3]);
> > +			  status->bOriginator,
> > +			  status->bValue[0] ? "pressed" : "released", len);
> > +		uvc_input_report_key(dev, KEY_CAMERA, status->bValue[0]);
> >  	} else {
> >  		uvc_trace(UVC_TRACE_STATUS,
> >  			  "Stream %u error event %02x len %d.\n",
> > -			  data[1], data[2], len);
> > +			  status->bOriginator, status->bEvent, len);
> >  	}
> >  }
> > 
> > -static void uvc_event_control(struct uvc_device *dev, u8 *data, int len)
> > +#define UVC_CTRL_VALUE_CHANGE	0
> > +#define UVC_CTRL_INFO_CHANGE	1
> > +#define UVC_CTRL_FAILURE_CHANGE	2
> > +#define UVC_CTRL_MIN_CHANGE	3
> > +#define UVC_CTRL_MAX_CHANGE	4
> > +
> > +static struct uvc_control *uvc_event_entity_find_ctrl(struct uvc_entity
> > *entity, +						      u8 selector)
> >  {
> > -	char *attrs[3] = { "value", "info", "failure" };
> > +	struct uvc_control *ctrl;
> > +	unsigned int i;
> > +
> > +	for (i = 0, ctrl = entity->controls; i < entity->ncontrols; i++, ctrl++)
> > +		if (ctrl->info.selector == selector)
> > +			return ctrl;
> > 
> > -	if (len < 6 || data[2] != 0 || data[4] > 2) {
> > +	return NULL;
> > +}
> > +
> > +static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
> > +					const struct uvc_control_status *status,
> > +					struct uvc_video_chain **chain)
> > +{
> > +	list_for_each_entry((*chain), &dev->chains, list) {
> > +		struct uvc_entity *entity;
> > +		struct uvc_control *ctrl;
> > +
> > +		list_for_each_entry(entity, &(*chain)->entities, chain) {
> > +			if (entity->id != status->bOriginator)
> > +				continue;
> > +
> > +			ctrl = uvc_event_entity_find_ctrl(entity,
> > +							  status->bSelector);
> > +			if (ctrl && (!ctrl->handle ||
> > +				     ctrl->handle->chain == *chain))
> 
> I'm afraid I still don't understand why you need the chain check :-( Unless 
> I'm mistaken, ctrl->handle is set in uvc_ctrl_set(), where the control is 
> looked up from the chain corresponding to handle->chain. How can the check be 
> false here ?

I think you're right, the bOriginator check should be enough.

> 
> Those are my two major concerns. Apart from that I have other small concerns 
> that I propose addressing myself to avoid further delays. I've been slow 
> enough when it comes to reviewing this series, if we can clear the two issues 
> above, I'll handle the rest.

Once I get your review of patch #3, I'll fix these two issues and 
resubmit, so you can also tell me your "minor concerns," since I'll be 
resubmitting anyway.

Thanks
Guennadi

> 
> > +				return ctrl;
> > +		}
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +static bool uvc_event_control(struct urb *urb,
> > +			      const struct uvc_control_status *status, int len)
> > +{
> > +	static const char *attrs[] = { "value", "info", "failure", "min", "max" };
> > +	struct uvc_device *dev = urb->context;
> > +	struct uvc_video_chain *chain;
> > +	struct uvc_control *ctrl;
> > +
> > +	if (len < 6 || status->bEvent != 0 ||
> > +	    status->bAttribute >= ARRAY_SIZE(attrs)) {
> >  		uvc_trace(UVC_TRACE_STATUS, "Invalid control status event "
> >  				"received.\n");
> > -		return;
> > +		return false;
> >  	}
> > 
> >  	uvc_trace(UVC_TRACE_STATUS, "Control %u/%u %s change len %d.\n",
> > -		data[1], data[3], attrs[data[4]], len);
> > +		  status->bOriginator, status->bSelector,
> > +		  attrs[status->bAttribute], len);
> > +
> > +	/* Find the control. */
> > +	ctrl = uvc_event_find_ctrl(dev, status, &chain);
> > +	if (!ctrl)
> > +		return false;
> > +
> > +	switch (status->bAttribute) {
> > +	case UVC_CTRL_VALUE_CHANGE:
> > +		return uvc_ctrl_status_event(urb, chain, ctrl, status->bValue);
> > +
> > +	case UVC_CTRL_INFO_CHANGE:
> > +	case UVC_CTRL_FAILURE_CHANGE:
> > +	case UVC_CTRL_MIN_CHANGE:
> > +	case UVC_CTRL_MAX_CHANGE:
> > +		break;
> > +	}
> > +
> > +	return false;
> >  }
> > 
> >  static void uvc_status_complete(struct urb *urb)
> > @@ -139,11 +223,15 @@ static void uvc_status_complete(struct urb *urb)
> >  	if (len > 0) {
> >  		switch (dev->status[0] & 0x0f) {
> >  		case UVC_STATUS_TYPE_CONTROL:
> > -			uvc_event_control(dev, dev->status, len);
> > +			if (uvc_event_control(urb,
> > +				(struct uvc_control_status *)dev->status, len))
> > +				/* The URB will be resubmitted in work context */
> > +				return;
> >  			break;
> > 
> >  		case UVC_STATUS_TYPE_STREAMING:
> > -			uvc_event_streaming(dev, dev->status, len);
> > +			uvc_event_streaming(dev,
> > +				(struct uvc_streaming_status *)dev->status, len);
> >  			break;
> > 
> >  		default:
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > b/drivers/media/usb/uvc/uvc_v4l2.c index bd32914..18a7384 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -994,7 +994,7 @@ static int uvc_ioctl_s_ctrl(struct file *file, void *fh,
> > if (ret < 0)
> >  		return ret;
> > 
> > -	ret = uvc_ctrl_set(chain, &xctrl);
> > +	ret = uvc_ctrl_set(handle, &xctrl);
> >  	if (ret < 0) {
> >  		uvc_ctrl_rollback(handle);
> >  		return ret;
> > @@ -1069,7 +1069,7 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh
> > *handle, return ret;
> > 
> >  	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> > -		ret = uvc_ctrl_set(chain, ctrl);
> > +		ret = uvc_ctrl_set(handle, ctrl);
> >  		if (ret < 0) {
> >  			uvc_ctrl_rollback(handle);
> >  			ctrls->error_idx = commit ? ctrls->count : i;
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h
> > b/drivers/media/usb/uvc/uvcvideo.h index be5cf17..0e5e920 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -12,6 +12,7 @@
> >  #include <linux/usb/video.h>
> >  #include <linux/uvcvideo.h>
> >  #include <linux/videodev2.h>
> > +#include <linux/workqueue.h>
> >  #include <media/media-device.h>
> >  #include <media/v4l2-device.h>
> >  #include <media/v4l2-event.h>
> > @@ -256,6 +257,8 @@ struct uvc_control {
> >  	   initialized:1;
> > 
> >  	u8 *uvc_data;
> > +
> > +	struct uvc_fh *handle;	/* Used for asynchronous event delivery */
> >  };
> > 
> >  struct uvc_format_desc {
> > @@ -600,6 +603,14 @@ struct uvc_device {
> >  	u8 *status;
> >  	struct input_dev *input;
> >  	char input_phys[64];
> > +
> > +	struct uvc_ctrl_work {
> > +		struct work_struct work;
> > +		struct urb *urb;
> > +		struct uvc_video_chain *chain;
> > +		struct uvc_control *ctrl;
> > +		const void *data;
> > +	} async_ctrl;
> >  };
> > 
> >  enum uvc_handle_state {
> > @@ -753,6 +764,8 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
> >  int uvc_ctrl_init_device(struct uvc_device *dev);
> >  void uvc_ctrl_cleanup_device(struct uvc_device *dev);
> >  int uvc_ctrl_restore_values(struct uvc_device *dev);
> > +bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,
> > +			   struct uvc_control *ctrl, const u8 *data);
> > 
> >  int uvc_ctrl_begin(struct uvc_video_chain *chain);
> >  int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> > @@ -770,7 +783,7 @@ static inline int uvc_ctrl_rollback(struct uvc_fh
> > *handle) }
> > 
> >  int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control
> > *xctrl); -int uvc_ctrl_set(struct uvc_video_chain *chain, struct
> > v4l2_ext_control *xctrl); +int uvc_ctrl_set(struct uvc_fh *handle, struct
> > v4l2_ext_control *xctrl);
> > 
> >  int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
> >  		      struct uvc_xu_control_query *xqry);
> > diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
> > index 020714d..f80f05b 100644
> > --- a/include/uapi/linux/uvcvideo.h
> > +++ b/include/uapi/linux/uvcvideo.h
> > @@ -28,6 +28,8 @@
> >  #define UVC_CTRL_FLAG_RESTORE		(1 << 6)
> >  /* Control can be updated by the camera. */
> >  #define UVC_CTRL_FLAG_AUTO_UPDATE	(1 << 7)
> > +/* Control supports asynchronous reporting */
> > +#define UVC_CTRL_FLAG_ASYNCHRONOUS	(1 << 8)
> > 
> >  #define UVC_CTRL_FLAG_GET_RANGE \
> >  	(UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \
> 
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

Re: [PATCH v8 2/3] uvcvideo: send a control event when a Control Change interrupt arrives

[Guennadi Liakhovetski]

Hi Laurent,

On Thu, 12 Jul 2018, Guennadi Liakhovetski wrote:

> Hi Laurent,
> 
> Thanks for the review.

[snip]

> Once I get your review of patch #3, I'll fix these two issues and 
> resubmit, so you can also tell me your "minor concerns," since I'll be 
> resubmitting anyway.

Any progress on this?

Thanks
Guennadi

Re: [PATCH v8 2/3] uvcvideo: send a control event when a Control Change interrupt arrives

[Laurent Pinchart]

Hi Guennadi,

On Thursday, 12 July 2018 10:30:46 EEST Guennadi Liakhovetski wrote:
> On Thu, 12 Jul 2018, Laurent Pinchart wrote:
> > On Tuesday, 8 May 2018 18:07:43 EEST Guennadi Liakhovetski wrote:
> >> UVC defines a method of handling asynchronous controls, which sends a
> >> USB packet over the interrupt pipe. This patch implements support for
> >> such packets by sending a control event to the user. Since this can
> >> involve USB traffic and, therefore, scheduling, this has to be done
> >> in a work queue.
> >> 
> >> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> >> ---
> >> 
> >> v8:
> >> 
> >> * avoid losing events by delaying the status URB resubmission until
> >>   after completion of the current event
> >> * extract control value calculation into __uvc_ctrl_get_value()
> >> * do not proactively return EBUSY if the previous control hasn't
> >>   completed yet, let the camera handle such cases
> >> * multiple cosmetic changes
> >> 
> >>  drivers/media/usb/uvc/uvc_ctrl.c   | 166 ++++++++++++++++++++++++-------
> >>  drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++---
> >>  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
> >>  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
> >>  include/uapi/linux/uvcvideo.h      |   2 +
> >>  5 files changed, 255 insertions(+), 44 deletions(-)
> >> 
> >> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> >> b/drivers/media/usb/uvc/uvc_ctrl.c index 2a213c8..796f86a 100644
> >> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> >> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> 
> [snip]
> 
> >> +static void uvc_ctrl_status_event_work(struct work_struct *work)
> >> +{
> >> +	struct uvc_device *dev = container_of(work, struct uvc_device,
> >> +					      async_ctrl.work);
> >> +	struct uvc_ctrl_work *w = &dev->async_ctrl;
> >> +	struct uvc_control_mapping *mapping;
> >> +	struct uvc_control *ctrl = w->ctrl;
> >> +	unsigned int i;
> >> +	int ret;
> >> +
> >> +	mutex_lock(&w->chain->ctrl_mutex);
> >> +
> >> +	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> >> +		s32 value = __uvc_ctrl_get_value(mapping, w->data);
> >> +
> >> +		/*
> >> +		 * So far none of the auto-update controls in the uvc_ctrls[]
> >> +		 * table is mapped to a V4L control with slaves in the
> >> +		 * uvc_ctrl_mappings[] list, so slave controls so far never have
> >> +		 * handle == NULL, but this can change in the future
> >> +		 */
> >> +		for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
> >> +			if (!mapping->slave_ids[i])
> >> +				break;
> >> +
> >> +			__uvc_ctrl_send_slave_event(ctrl->handle, w->chain,
> >> +						ctrl, mapping->slave_ids[i]);
> >> +		}
> >> +
> >> +		uvc_ctrl_send_event(ctrl->handle, ctrl, mapping, value,
> >> +				    V4L2_EVENT_CTRL_CH_VALUE);
> >> +	}
> >> +
> >> +	mutex_unlock(&w->chain->ctrl_mutex);
> >> +
> >> +	ctrl->handle = NULL;
> > 
> > Can't this race with a uvc_ctrl_set() call, resulting in ctrl->handle
> > being NULL after the control gets set ?
> 
> Right, it's better to set .handle to NULL before sending events. Something
> like
> 
> mutex_lock();
> 
> handle = ctrl->handle;
> ctrl->handle = NULL;
> 
> list_for_each_entry() {
> 	...
> 	uvc_ctrl_send_event(handle,...);
> }
> 
> mutex_unlock();
> 
> ?

I think you also have to take the same lock in the uvc_ctrl_set() function to 
fix the problem, otherwise the ctrl->handle = NULL line could still be 
executed after the ctrl->handle assignment in uvc_ctrl_set(), resulting in 
ctrl->handle being NULL while the control is being set.

> >> +	/* Resubmit the URB. */
> >> +	w->urb->interval = dev->int_ep->desc.bInterval;
> >> +	ret = usb_submit_urb(w->urb, GFP_KERNEL);
> >> +	if (ret < 0)
> >> +		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
> >> +			   ret);
> >> +}

[snip]

> >> diff --git a/drivers/media/usb/uvc/uvc_status.c
> >> b/drivers/media/usb/uvc/uvc_status.c index 7b71041..a0f2fea 100644
> >> --- a/drivers/media/usb/uvc/uvc_status.c
> >> +++ b/drivers/media/usb/uvc/uvc_status.c

[snip]

> >> +static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
> >> +					const struct uvc_control_status *status,
> >> +					struct uvc_video_chain **chain)
> >> +{
> >> +	list_for_each_entry((*chain), &dev->chains, list) {
> >> +		struct uvc_entity *entity;
> >> +		struct uvc_control *ctrl;
> >> +
> >> +		list_for_each_entry(entity, &(*chain)->entities, chain) {
> >> +			if (entity->id != status->bOriginator)
> >> +				continue;
> >> +
> >> +			ctrl = uvc_event_entity_find_ctrl(entity,
> >> +							  status->bSelector);
> >> +			if (ctrl && (!ctrl->handle ||
> >> +				     ctrl->handle->chain == *chain))
> > 
> > I'm afraid I still don't understand why you need the chain check :-(
> > Unless I'm mistaken, ctrl->handle is set in uvc_ctrl_set(), where the
> > control is looked up from the chain corresponding to handle->chain. How
> > can the check be false here ?
> 
> I think you're right, the bOriginator check should be enough.
> 
> > Those are my two major concerns. Apart from that I have other small
> > concerns that I propose addressing myself to avoid further delays. I've
> > been slow enough when it comes to reviewing this series, if we can clear
> > the two issues above, I'll handle the rest.
> 
> Once I get your review of patch #3, I'll fix these two issues and
> resubmit, so you can also tell me your "minor concerns," since I'll be
> resubmitting anyway.

Do you mind if I send them as a diff on top of your patch ? I'll of course add 
explanations where they are needed.

> >> +				return ctrl;
> >> +		}
> >> +	}
> >> +
> >> +	return NULL;
> >> +}

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 2/3] uvcvideo: send a control event when a Control Change interrupt arrives

[Guennadi Liakhovetski]

On Tue, 17 Jul 2018, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Thursday, 12 July 2018 10:30:46 EEST Guennadi Liakhovetski wrote:
> > On Thu, 12 Jul 2018, Laurent Pinchart wrote:
> > > On Tuesday, 8 May 2018 18:07:43 EEST Guennadi Liakhovetski wrote:
> > >> UVC defines a method of handling asynchronous controls, which sends a
> > >> USB packet over the interrupt pipe. This patch implements support for
> > >> such packets by sending a control event to the user. Since this can
> > >> involve USB traffic and, therefore, scheduling, this has to be done
> > >> in a work queue.
> > >> 
> > >> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> > >> ---
> > >> 
> > >> v8:
> > >> 
> > >> * avoid losing events by delaying the status URB resubmission until
> > >>   after completion of the current event
> > >> * extract control value calculation into __uvc_ctrl_get_value()
> > >> * do not proactively return EBUSY if the previous control hasn't
> > >>   completed yet, let the camera handle such cases
> > >> * multiple cosmetic changes
> > >> 
> > >>  drivers/media/usb/uvc/uvc_ctrl.c   | 166 ++++++++++++++++++++++++-------
> > >>  drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++---
> > >>  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
> > >>  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
> > >>  include/uapi/linux/uvcvideo.h      |   2 +
> > >>  5 files changed, 255 insertions(+), 44 deletions(-)
> > >> 
> > >> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > >> b/drivers/media/usb/uvc/uvc_ctrl.c index 2a213c8..796f86a 100644
> > >> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > >> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > 
> > [snip]
> > 
> > >> +static void uvc_ctrl_status_event_work(struct work_struct *work)
> > >> +{
> > >> +	struct uvc_device *dev = container_of(work, struct uvc_device,
> > >> +					      async_ctrl.work);
> > >> +	struct uvc_ctrl_work *w = &dev->async_ctrl;
> > >> +	struct uvc_control_mapping *mapping;
> > >> +	struct uvc_control *ctrl = w->ctrl;
> > >> +	unsigned int i;
> > >> +	int ret;
> > >> +
> > >> +	mutex_lock(&w->chain->ctrl_mutex);
> > >> +
> > >> +	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> > >> +		s32 value = __uvc_ctrl_get_value(mapping, w->data);
> > >> +
> > >> +		/*
> > >> +		 * So far none of the auto-update controls in the uvc_ctrls[]
> > >> +		 * table is mapped to a V4L control with slaves in the
> > >> +		 * uvc_ctrl_mappings[] list, so slave controls so far never have
> > >> +		 * handle == NULL, but this can change in the future
> > >> +		 */
> > >> +		for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
> > >> +			if (!mapping->slave_ids[i])
> > >> +				break;
> > >> +
> > >> +			__uvc_ctrl_send_slave_event(ctrl->handle, w->chain,
> > >> +						ctrl, mapping->slave_ids[i]);
> > >> +		}
> > >> +
> > >> +		uvc_ctrl_send_event(ctrl->handle, ctrl, mapping, value,
> > >> +				    V4L2_EVENT_CTRL_CH_VALUE);
> > >> +	}
> > >> +
> > >> +	mutex_unlock(&w->chain->ctrl_mutex);
> > >> +
> > >> +	ctrl->handle = NULL;
> > > 
> > > Can't this race with a uvc_ctrl_set() call, resulting in ctrl->handle
> > > being NULL after the control gets set ?
> > 
> > Right, it's better to set .handle to NULL before sending events. Something
> > like
> > 
> > mutex_lock();
> > 
> > handle = ctrl->handle;
> > ctrl->handle = NULL;
> > 
> > list_for_each_entry() {
> > 	...
> > 	uvc_ctrl_send_event(handle,...);
> > }
> > 
> > mutex_unlock();
> > 
> > ?
> 
> I think you also have to take the same lock in the uvc_ctrl_set() function to 
> fix the problem, otherwise the ctrl->handle = NULL line could still be 
> executed after the ctrl->handle assignment in uvc_ctrl_set(), resulting in 
> ctrl->handle being NULL while the control is being set.

Doesn't this mean, that you're attempting to send a new instance of the 
same control before the previous has completed? In which case also taking 
the lock in uvc_ctrl_set() wouldn't help either, because you can anyway do 
that immediately after the first instance, before the completion even has 
fired.

> > >> +	/* Resubmit the URB. */
> > >> +	w->urb->interval = dev->int_ep->desc.bInterval;
> > >> +	ret = usb_submit_urb(w->urb, GFP_KERNEL);
> > >> +	if (ret < 0)
> > >> +		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
> > >> +			   ret);
> > >> +}
> 
> [snip]
> 
> > >> diff --git a/drivers/media/usb/uvc/uvc_status.c
> > >> b/drivers/media/usb/uvc/uvc_status.c index 7b71041..a0f2fea 100644
> > >> --- a/drivers/media/usb/uvc/uvc_status.c
> > >> +++ b/drivers/media/usb/uvc/uvc_status.c
> 
> [snip]
> 
> > >> +static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
> > >> +					const struct uvc_control_status *status,
> > >> +					struct uvc_video_chain **chain)
> > >> +{
> > >> +	list_for_each_entry((*chain), &dev->chains, list) {
> > >> +		struct uvc_entity *entity;
> > >> +		struct uvc_control *ctrl;
> > >> +
> > >> +		list_for_each_entry(entity, &(*chain)->entities, chain) {
> > >> +			if (entity->id != status->bOriginator)
> > >> +				continue;
> > >> +
> > >> +			ctrl = uvc_event_entity_find_ctrl(entity,
> > >> +							  status->bSelector);
> > >> +			if (ctrl && (!ctrl->handle ||
> > >> +				     ctrl->handle->chain == *chain))
> > > 
> > > I'm afraid I still don't understand why you need the chain check :-(
> > > Unless I'm mistaken, ctrl->handle is set in uvc_ctrl_set(), where the
> > > control is looked up from the chain corresponding to handle->chain. How
> > > can the check be false here ?
> > 
> > I think you're right, the bOriginator check should be enough.
> > 
> > > Those are my two major concerns. Apart from that I have other small
> > > concerns that I propose addressing myself to avoid further delays. I've
> > > been slow enough when it comes to reviewing this series, if we can clear
> > > the two issues above, I'll handle the rest.
> > 
> > Once I get your review of patch #3, I'll fix these two issues and
> > resubmit, so you can also tell me your "minor concerns," since I'll be
> > resubmitting anyway.
> 
> Do you mind if I send them as a diff on top of your patch ? I'll of course add 
> explanations where they are needed.

No, I don't mind.

Thanks
Guennadi

> > >> +				return ctrl;
> > >> +		}
> > >> +	}
> > >> +
> > >> +	return NULL;
> > >> +}
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v8 2/3] uvcvideo: send a control event when a Control Change interrupt arrives

[Laurent Pinchart]

Hi Guennadi,

On Wednesday, 18 July 2018 00:30:45 EEST Guennadi Liakhovetski wrote:
> On Tue, 17 Jul 2018, Laurent Pinchart wrote:
> > On Thursday, 12 July 2018 10:30:46 EEST Guennadi Liakhovetski wrote:
> >> On Thu, 12 Jul 2018, Laurent Pinchart wrote:
> >>> On Tuesday, 8 May 2018 18:07:43 EEST Guennadi Liakhovetski wrote:
> >>>> UVC defines a method of handling asynchronous controls, which sends a
> >>>> USB packet over the interrupt pipe. This patch implements support for
> >>>> such packets by sending a control event to the user. Since this can
> >>>> involve USB traffic and, therefore, scheduling, this has to be done
> >>>> in a work queue.
> >>>> 
> >>>> Signed-off-by: Guennadi Liakhovetski
> >>>> <guennadi.liakhovetski@intel.com>
> >>>> ---
> >>>> 
> >>>> v8:
> >>>> 
> >>>> * avoid losing events by delaying the status URB resubmission until
> >>>>   after completion of the current event
> >>>> * extract control value calculation into __uvc_ctrl_get_value()
> >>>> * do not proactively return EBUSY if the previous control hasn't
> >>>>   completed yet, let the camera handle such cases
> >>>> * multiple cosmetic changes
> >>>> 
> >>>>  drivers/media/usb/uvc/uvc_ctrl.c   | 166 +++++++++++++++++++++------
> >>>>  drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++---
> >>>>  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
> >>>>  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
> >>>>  include/uapi/linux/uvcvideo.h      |   2 +
> >>>>  5 files changed, 255 insertions(+), 44 deletions(-)
> >>>> 
> >>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> >>>> b/drivers/media/usb/uvc/uvc_ctrl.c index 2a213c8..796f86a 100644
> >>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> >>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> >> 
> >> [snip]
> >> 
> >>>> +static void uvc_ctrl_status_event_work(struct work_struct *work)
> >>>> +{
> >>>> +	struct uvc_device *dev = container_of(work, struct uvc_device,
> >>>> +					      async_ctrl.work);
> >>>> +	struct uvc_ctrl_work *w = &dev->async_ctrl;
> >>>> +	struct uvc_control_mapping *mapping;
> >>>> +	struct uvc_control *ctrl = w->ctrl;
> >>>> +	unsigned int i;
> >>>> +	int ret;
> >>>> +
> >>>> +	mutex_lock(&w->chain->ctrl_mutex);
> >>>> +
> >>>> +	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> >>>> +		s32 value = __uvc_ctrl_get_value(mapping, w->data);
> >>>> +
> >>>> +		/*
> >>>> +		 * So far none of the auto-update controls in the uvc_ctrls[]
> >>>> +		 * table is mapped to a V4L control with slaves in the
> >>>> +		 * uvc_ctrl_mappings[] list, so slave controls so far never have
> >>>> +		 * handle == NULL, but this can change in the future
> >>>> +		 */
> >>>> +		for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
> >>>> +			if (!mapping->slave_ids[i])
> >>>> +				break;
> >>>> +
> >>>> +			__uvc_ctrl_send_slave_event(ctrl->handle, w->chain,
> >>>> +						ctrl, mapping->slave_ids[i]);
> >>>> +		}
> >>>> +
> >>>> +		uvc_ctrl_send_event(ctrl->handle, ctrl, mapping, value,
> >>>> +				    V4L2_EVENT_CTRL_CH_VALUE);
> >>>> +	}
> >>>> +
> >>>> +	mutex_unlock(&w->chain->ctrl_mutex);
> >>>> +
> >>>> +	ctrl->handle = NULL;
> >>> 
> >>> Can't this race with a uvc_ctrl_set() call, resulting in ctrl->handle
> >>> being NULL after the control gets set ?
> >> 
> >> Right, it's better to set .handle to NULL before sending events.
> >> Something like
> >> 
> >> mutex_lock();
> >> 
> >> handle = ctrl->handle;
> >> ctrl->handle = NULL;
> >> 
> >> list_for_each_entry() {
> >> 
> >> 	...
> >> 	uvc_ctrl_send_event(handle,...);
> >> 
> >> }
> >> 
> >> mutex_unlock();
> >> 
> >> ?
> > 
> > I think you also have to take the same lock in the uvc_ctrl_set() function
> > to fix the problem, otherwise the ctrl->handle = NULL line could still be
> > executed after the ctrl->handle assignment in uvc_ctrl_set(), resulting
> > in ctrl->handle being NULL while the control is being set.
> 
> Doesn't this mean, that you're attempting to send a new instance of the
> same control before the previous has completed? In which case also taking
> the lock in uvc_ctrl_set() wouldn't help either, because you can anyway do
> that immediately after the first instance, before the completion even has
> fired.

You're right that it won't solve the race completely, but wouldn't it at least 
prevent ctrl->handle from being NULL ? We can't guarantee which of the old and 
new handle will be used for events when multiple control set operations are 
invoked, but we should try to guarantee that the handle won't be NULL.

> >>>> +	/* Resubmit the URB. */
> >>>> +	w->urb->interval = dev->int_ep->desc.bInterval;
> >>>> +	ret = usb_submit_urb(w->urb, GFP_KERNEL);
> >>>> +	if (ret < 0)
> >>>> +		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
> >>>> +			   ret);
> >>>> +}

[snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 2/3] uvcvideo: send a control event when a Control Change interrupt arrives

[Guennadi Liakhovetski]

Hi Laurent,

On Wed, 18 Jul 2018, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Wednesday, 18 July 2018 00:30:45 EEST Guennadi Liakhovetski wrote:
> > On Tue, 17 Jul 2018, Laurent Pinchart wrote:
> > > On Thursday, 12 July 2018 10:30:46 EEST Guennadi Liakhovetski wrote:
> > >> On Thu, 12 Jul 2018, Laurent Pinchart wrote:
> > >>> On Tuesday, 8 May 2018 18:07:43 EEST Guennadi Liakhovetski wrote:
> > >>>> UVC defines a method of handling asynchronous controls, which sends a
> > >>>> USB packet over the interrupt pipe. This patch implements support for
> > >>>> such packets by sending a control event to the user. Since this can
> > >>>> involve USB traffic and, therefore, scheduling, this has to be done
> > >>>> in a work queue.
> > >>>> 
> > >>>> Signed-off-by: Guennadi Liakhovetski
> > >>>> <guennadi.liakhovetski@intel.com>
> > >>>> ---
> > >>>> 
> > >>>> v8:
> > >>>> 
> > >>>> * avoid losing events by delaying the status URB resubmission until
> > >>>>   after completion of the current event
> > >>>> * extract control value calculation into __uvc_ctrl_get_value()
> > >>>> * do not proactively return EBUSY if the previous control hasn't
> > >>>>   completed yet, let the camera handle such cases
> > >>>> * multiple cosmetic changes
> > >>>> 
> > >>>>  drivers/media/usb/uvc/uvc_ctrl.c   | 166 +++++++++++++++++++++------
> > >>>>  drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++---
> > >>>>  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
> > >>>>  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
> > >>>>  include/uapi/linux/uvcvideo.h      |   2 +
> > >>>>  5 files changed, 255 insertions(+), 44 deletions(-)
> > >>>> 
> > >>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > >>>> b/drivers/media/usb/uvc/uvc_ctrl.c index 2a213c8..796f86a 100644
> > >>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > >>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > >> 
> > >> [snip]
> > >> 
> > >>>> +static void uvc_ctrl_status_event_work(struct work_struct *work)
> > >>>> +{
> > >>>> +	struct uvc_device *dev = container_of(work, struct uvc_device,
> > >>>> +					      async_ctrl.work);
> > >>>> +	struct uvc_ctrl_work *w = &dev->async_ctrl;
> > >>>> +	struct uvc_control_mapping *mapping;
> > >>>> +	struct uvc_control *ctrl = w->ctrl;
> > >>>> +	unsigned int i;
> > >>>> +	int ret;
> > >>>> +
> > >>>> +	mutex_lock(&w->chain->ctrl_mutex);
> > >>>> +
> > >>>> +	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> > >>>> +		s32 value = __uvc_ctrl_get_value(mapping, w->data);
> > >>>> +
> > >>>> +		/*
> > >>>> +		 * So far none of the auto-update controls in the uvc_ctrls[]
> > >>>> +		 * table is mapped to a V4L control with slaves in the
> > >>>> +		 * uvc_ctrl_mappings[] list, so slave controls so far never have
> > >>>> +		 * handle == NULL, but this can change in the future
> > >>>> +		 */
> > >>>> +		for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
> > >>>> +			if (!mapping->slave_ids[i])
> > >>>> +				break;
> > >>>> +
> > >>>> +			__uvc_ctrl_send_slave_event(ctrl->handle, w->chain,
> > >>>> +						ctrl, mapping->slave_ids[i]);
> > >>>> +		}
> > >>>> +
> > >>>> +		uvc_ctrl_send_event(ctrl->handle, ctrl, mapping, value,
> > >>>> +				    V4L2_EVENT_CTRL_CH_VALUE);
> > >>>> +	}
> > >>>> +
> > >>>> +	mutex_unlock(&w->chain->ctrl_mutex);
> > >>>> +
> > >>>> +	ctrl->handle = NULL;
> > >>> 
> > >>> Can't this race with a uvc_ctrl_set() call, resulting in ctrl->handle
> > >>> being NULL after the control gets set ?
> > >> 
> > >> Right, it's better to set .handle to NULL before sending events.
> > >> Something like
> > >> 
> > >> mutex_lock();
> > >> 
> > >> handle = ctrl->handle;
> > >> ctrl->handle = NULL;
> > >> 
> > >> list_for_each_entry() {
> > >> 
> > >> 	...
> > >> 	uvc_ctrl_send_event(handle,...);
> > >> 
> > >> }
> > >> 
> > >> mutex_unlock();
> > >> 
> > >> ?
> > > 
> > > I think you also have to take the same lock in the uvc_ctrl_set() function
> > > to fix the problem, otherwise the ctrl->handle = NULL line could still be
> > > executed after the ctrl->handle assignment in uvc_ctrl_set(), resulting
> > > in ctrl->handle being NULL while the control is being set.
> > 
> > Doesn't this mean, that you're attempting to send a new instance of the
> > same control before the previous has completed? In which case also taking
> > the lock in uvc_ctrl_set() wouldn't help either, because you can anyway do
> > that immediately after the first instance, before the completion even has
> > fired.
> 
> You're right that it won't solve the race completely, but wouldn't it at least 
> prevent ctrl->handle from being NULL ? We can't guarantee which of the old and 
> new handle will be used for events when multiple control set operations are 
> invoked, but we should try to guarantee that the handle won't be NULL.

Sorry, I'm probably misunderstanding something. What exactly are you 
proposing to lock and what and how is it supposed to protect? Wouldn't the 
following flow still be possible, if you protect setting .handle = NULL in 
uvc_set_ctrl():

CPU 1                                 CPU 2

control completion interrupt
(.handle = HANDLE_1)
work scheduled
                                      uvc_set_ctrl()
                                      .handle = HANDLE_2
uvc_ctrl_status_event_work()
.handle = NULL
usb_submit_urb()

control completion interrupt
(.handle = NULL)

?

Thanks
Guennadi

> > >>>> +	/* Resubmit the URB. */
> > >>>> +	w->urb->interval = dev->int_ep->desc.bInterval;
> > >>>> +	ret = usb_submit_urb(w->urb, GFP_KERNEL);
> > >>>> +	if (ret < 0)
> > >>>> +		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
> > >>>> +			   ret);
> > >>>> +}
> 
> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

Re: [PATCH v8 2/3] uvcvideo: send a control event when a Control Change interrupt arrives

[Laurent Pinchart]

Hi Guennadi,

On Wednesday, 18 July 2018 09:55:27 EEST Guennadi Liakhovetski wrote:
> On Wed, 18 Jul 2018, Laurent Pinchart wrote:
> > On Wednesday, 18 July 2018 00:30:45 EEST Guennadi Liakhovetski wrote:
> >> On Tue, 17 Jul 2018, Laurent Pinchart wrote:
> >>> On Thursday, 12 July 2018 10:30:46 EEST Guennadi Liakhovetski wrote:
> >>>> On Thu, 12 Jul 2018, Laurent Pinchart wrote:
> >>>>> On Tuesday, 8 May 2018 18:07:43 EEST Guennadi Liakhovetski wrote:
> >>>>>> UVC defines a method of handling asynchronous controls, which sends
> >>>>>> a USB packet over the interrupt pipe. This patch implements support
> >>>>>> for such packets by sending a control event to the user. Since this
> >>>>>> can involve USB traffic and, therefore, scheduling, this has to be
> >>>>>> done in a work queue.
> >>>>>> 
> >>>>>> Signed-off-by: Guennadi Liakhovetski
> >>>>>> <guennadi.liakhovetski@intel.com>
> >>>>>> ---
> >>>>>> 
> >>>>>> v8:
> >>>>>> 
> >>>>>> * avoid losing events by delaying the status URB resubmission until
> >>>>>>   after completion of the current event
> >>>>>> * extract control value calculation into __uvc_ctrl_get_value()
> >>>>>> * do not proactively return EBUSY if the previous control hasn't
> >>>>>>   completed yet, let the camera handle such cases
> >>>>>> * multiple cosmetic changes
> >>>>>> 
> >>>>>>  drivers/media/usb/uvc/uvc_ctrl.c   | 166 ++++++++++++++++++++------
> >>>>>>  drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++---
> >>>>>>  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
> >>>>>>  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
> >>>>>>  include/uapi/linux/uvcvideo.h      |   2 +
> >>>>>>  5 files changed, 255 insertions(+), 44 deletions(-)
> >>>>>> 
> >>>>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> >>>>>> b/drivers/media/usb/uvc/uvc_ctrl.c index 2a213c8..796f86a 100644
> >>>>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> >>>>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> >>>> 
> >>>> [snip]
> >>>> 
> >>>>>> +static void uvc_ctrl_status_event_work(struct work_struct *work)
> >>>>>> +{
> >>>>>> +	struct uvc_device *dev = container_of(work, struct uvc_device,
> >>>>>> +					      async_ctrl.work);
> >>>>>> +	struct uvc_ctrl_work *w = &dev->async_ctrl;
> >>>>>> +	struct uvc_control_mapping *mapping;
> >>>>>> +	struct uvc_control *ctrl = w->ctrl;
> >>>>>> +	unsigned int i;
> >>>>>> +	int ret;
> >>>>>> +
> >>>>>> +	mutex_lock(&w->chain->ctrl_mutex);
> >>>>>> +
> >>>>>> +	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> >>>>>> +		s32 value = __uvc_ctrl_get_value(mapping, w->data);
> >>>>>> +
> >>>>>> +		/*
> >>>>>> +		 * So far none of the auto-update controls in the uvc_ctrls[]
> >>>>>> +		 * table is mapped to a V4L control with slaves in the
> >>>>>> +		 * uvc_ctrl_mappings[] list, so slave controls so far never have
> >>>>>> +		 * handle == NULL, but this can change in the future
> >>>>>> +		 */
> >>>>>> +		for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
> >>>>>> +			if (!mapping->slave_ids[i])
> >>>>>> +				break;
> >>>>>> +
> >>>>>> +			__uvc_ctrl_send_slave_event(ctrl->handle, w->chain,
> >>>>>> +						ctrl, mapping->slave_ids[i]);
> >>>>>> +		}
> >>>>>> +
> >>>>>> +		uvc_ctrl_send_event(ctrl->handle, ctrl, mapping, value,
> >>>>>> +				    V4L2_EVENT_CTRL_CH_VALUE);
> >>>>>> +	}
> >>>>>> +
> >>>>>> +	mutex_unlock(&w->chain->ctrl_mutex);
> >>>>>> +
> >>>>>> +	ctrl->handle = NULL;
> >>>>> 
> >>>>> Can't this race with a uvc_ctrl_set() call, resulting in
> >>>>> ctrl->handle being NULL after the control gets set ?
> >>>> 
> >>>> Right, it's better to set .handle to NULL before sending events.
> >>>> Something like
> >>>> 
> >>>> mutex_lock();
> >>>> 
> >>>> handle = ctrl->handle;
> >>>> ctrl->handle = NULL;
> >>>> 
> >>>> list_for_each_entry() {
> >>>> 	...
> >>>> 	uvc_ctrl_send_event(handle,...);
> >>>> }
> >>>> 
> >>>> mutex_unlock();
> >>>> 
> >>>> ?
> >>> 
> >>> I think you also have to take the same lock in the uvc_ctrl_set()
> >>> function to fix the problem, otherwise the ctrl->handle = NULL line
> >>> could still be executed after the ctrl->handle assignment in
> >>> uvc_ctrl_set(), resulting in ctrl->handle being NULL while the control
> >>> is being set.
> >> 
> >> Doesn't this mean, that you're attempting to send a new instance of the
> >> same control before the previous has completed? In which case also
> >> taking the lock in uvc_ctrl_set() wouldn't help either, because you can
> >> anyway do that immediately after the first instance, before the
> >> completion even has fired.
> > 
> > You're right that it won't solve the race completely, but wouldn't it at
> > least prevent ctrl->handle from being NULL ? We can't guarantee which of
> > the old and new handle will be used for events when multiple control set
> > operations are invoked, but we should try to guarantee that the handle
> > won't be NULL.
> 
> Sorry, I'm probably misunderstanding something. What exactly are you
> proposing to lock and what and how is it supposed to protect? Wouldn't the
> following flow still be possible, if you protect setting .handle = NULL in
> uvc_set_ctrl():
> 
> CPU 1                                 CPU 2
> 
> control completion interrupt
> (.handle = HANDLE_1)
> work scheduled
>                                       uvc_set_ctrl()
>                                       .handle = HANDLE_2
> uvc_ctrl_status_event_work()
> .handle = NULL
> usb_submit_urb()
> 
> control completion interrupt
> (.handle = NULL)
> 
> ?

You're absolutely right, there's no easy way to guard against this with a mere 
lock. I think we can ignore the issue for now and address it later if really 
needed, as the only adverse effect would be a spurious control change event 
sent to a file handle that hasn't set the V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK 
flag.

> >>>>>> +	/* Resubmit the URB. */
> >>>>>> +	w->urb->interval = dev->int_ep->desc.bInterval;
> >>>>>> +	ret = usb_submit_urb(w->urb, GFP_KERNEL);
> >>>>>> +	if (ret < 0)
> >>>>>> +		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
> >>>>>> +			   ret);
> >>>>>> +}
> > 
> > [snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 2/3] uvcvideo: send a control event when a Control Change interrupt arrives

[Guennadi Liakhovetski]

On Wed, 25 Jul 2018, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Wednesday, 18 July 2018 09:55:27 EEST Guennadi Liakhovetski wrote:
> > On Wed, 18 Jul 2018, Laurent Pinchart wrote:
> > > On Wednesday, 18 July 2018 00:30:45 EEST Guennadi Liakhovetski wrote:
> > >> On Tue, 17 Jul 2018, Laurent Pinchart wrote:
> > >>> On Thursday, 12 July 2018 10:30:46 EEST Guennadi Liakhovetski wrote:
> > >>>> On Thu, 12 Jul 2018, Laurent Pinchart wrote:
> > >>>>> On Tuesday, 8 May 2018 18:07:43 EEST Guennadi Liakhovetski wrote:
> > >>>>>> UVC defines a method of handling asynchronous controls, which sends
> > >>>>>> a USB packet over the interrupt pipe. This patch implements support
> > >>>>>> for such packets by sending a control event to the user. Since this
> > >>>>>> can involve USB traffic and, therefore, scheduling, this has to be
> > >>>>>> done in a work queue.
> > >>>>>> 
> > >>>>>> Signed-off-by: Guennadi Liakhovetski
> > >>>>>> <guennadi.liakhovetski@intel.com>
> > >>>>>> ---
> > >>>>>> 
> > >>>>>> v8:
> > >>>>>> 
> > >>>>>> * avoid losing events by delaying the status URB resubmission until
> > >>>>>>   after completion of the current event
> > >>>>>> * extract control value calculation into __uvc_ctrl_get_value()
> > >>>>>> * do not proactively return EBUSY if the previous control hasn't
> > >>>>>>   completed yet, let the camera handle such cases
> > >>>>>> * multiple cosmetic changes
> > >>>>>> 
> > >>>>>>  drivers/media/usb/uvc/uvc_ctrl.c   | 166 ++++++++++++++++++++------
> > >>>>>>  drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++---
> > >>>>>>  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
> > >>>>>>  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
> > >>>>>>  include/uapi/linux/uvcvideo.h      |   2 +
> > >>>>>>  5 files changed, 255 insertions(+), 44 deletions(-)
> > >>>>>> 
> > >>>>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > >>>>>> b/drivers/media/usb/uvc/uvc_ctrl.c index 2a213c8..796f86a 100644
> > >>>>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > >>>>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > >>>> 
> > >>>> [snip]
> > >>>> 
> > >>>>>> +static void uvc_ctrl_status_event_work(struct work_struct *work)
> > >>>>>> +{
> > >>>>>> +	struct uvc_device *dev = container_of(work, struct uvc_device,
> > >>>>>> +					      async_ctrl.work);
> > >>>>>> +	struct uvc_ctrl_work *w = &dev->async_ctrl;
> > >>>>>> +	struct uvc_control_mapping *mapping;
> > >>>>>> +	struct uvc_control *ctrl = w->ctrl;
> > >>>>>> +	unsigned int i;
> > >>>>>> +	int ret;
> > >>>>>> +
> > >>>>>> +	mutex_lock(&w->chain->ctrl_mutex);
> > >>>>>> +
> > >>>>>> +	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> > >>>>>> +		s32 value = __uvc_ctrl_get_value(mapping, w->data);
> > >>>>>> +
> > >>>>>> +		/*
> > >>>>>> +		 * So far none of the auto-update controls in the uvc_ctrls[]
> > >>>>>> +		 * table is mapped to a V4L control with slaves in the
> > >>>>>> +		 * uvc_ctrl_mappings[] list, so slave controls so far never have
> > >>>>>> +		 * handle == NULL, but this can change in the future
> > >>>>>> +		 */
> > >>>>>> +		for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
> > >>>>>> +			if (!mapping->slave_ids[i])
> > >>>>>> +				break;
> > >>>>>> +
> > >>>>>> +			__uvc_ctrl_send_slave_event(ctrl->handle, w->chain,
> > >>>>>> +						ctrl, mapping->slave_ids[i]);
> > >>>>>> +		}
> > >>>>>> +
> > >>>>>> +		uvc_ctrl_send_event(ctrl->handle, ctrl, mapping, value,
> > >>>>>> +				    V4L2_EVENT_CTRL_CH_VALUE);
> > >>>>>> +	}
> > >>>>>> +
> > >>>>>> +	mutex_unlock(&w->chain->ctrl_mutex);
> > >>>>>> +
> > >>>>>> +	ctrl->handle = NULL;
> > >>>>> 
> > >>>>> Can't this race with a uvc_ctrl_set() call, resulting in
> > >>>>> ctrl->handle being NULL after the control gets set ?
> > >>>> 
> > >>>> Right, it's better to set .handle to NULL before sending events.
> > >>>> Something like
> > >>>> 
> > >>>> mutex_lock();
> > >>>> 
> > >>>> handle = ctrl->handle;
> > >>>> ctrl->handle = NULL;
> > >>>> 
> > >>>> list_for_each_entry() {
> > >>>> 	...
> > >>>> 	uvc_ctrl_send_event(handle,...);
> > >>>> }
> > >>>> 
> > >>>> mutex_unlock();
> > >>>> 
> > >>>> ?
> > >>> 
> > >>> I think you also have to take the same lock in the uvc_ctrl_set()
> > >>> function to fix the problem, otherwise the ctrl->handle = NULL line
> > >>> could still be executed after the ctrl->handle assignment in
> > >>> uvc_ctrl_set(), resulting in ctrl->handle being NULL while the control
> > >>> is being set.
> > >> 
> > >> Doesn't this mean, that you're attempting to send a new instance of the
> > >> same control before the previous has completed? In which case also
> > >> taking the lock in uvc_ctrl_set() wouldn't help either, because you can
> > >> anyway do that immediately after the first instance, before the
> > >> completion even has fired.
> > > 
> > > You're right that it won't solve the race completely, but wouldn't it at
> > > least prevent ctrl->handle from being NULL ? We can't guarantee which of
> > > the old and new handle will be used for events when multiple control set
> > > operations are invoked, but we should try to guarantee that the handle
> > > won't be NULL.
> > 
> > Sorry, I'm probably misunderstanding something. What exactly are you
> > proposing to lock and what and how is it supposed to protect? Wouldn't the
> > following flow still be possible, if you protect setting .handle = NULL in
> > uvc_set_ctrl():
> > 
> > CPU 1                                 CPU 2
> > 
> > control completion interrupt
> > (.handle = HANDLE_1)
> > work scheduled
> >                                       uvc_set_ctrl()
> >                                       .handle = HANDLE_2
> > uvc_ctrl_status_event_work()
> > .handle = NULL
> > usb_submit_urb()
> > 
> > control completion interrupt
> > (.handle = NULL)
> > 
> > ?
> 
> You're absolutely right, there's no easy way to guard against this with a mere 
> lock. I think we can ignore the issue for now and address it later if really 
> needed, as the only adverse effect would be a spurious control change event 
> sent to a file handle that hasn't set the V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK 
> flag.

Ok, but I still think the above change - setting .handle to NULL before 
sending the event - should be useful?

Thanks
Guennadi

> 
> > >>>>>> +	/* Resubmit the URB. */
> > >>>>>> +	w->urb->interval = dev->int_ep->desc.bInterval;
> > >>>>>> +	ret = usb_submit_urb(w->urb, GFP_KERNEL);
> > >>>>>> +	if (ret < 0)
> > >>>>>> +		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
> > >>>>>> +			   ret);
> > >>>>>> +}
> > > 
> > > [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

Re: [PATCH v8 2/3] uvcvideo: send a control event when a Control Change interrupt arrives

[Laurent Pinchart]

Hi Guennadi,

On Wednesday, 25 July 2018 20:21:54 EEST Guennadi Liakhovetski wrote:
> On Wed, 25 Jul 2018, Laurent Pinchart wrote:
> > On Wednesday, 18 July 2018 09:55:27 EEST Guennadi Liakhovetski wrote:
> >> On Wed, 18 Jul 2018, Laurent Pinchart wrote:
> >>> On Wednesday, 18 July 2018 00:30:45 EEST Guennadi Liakhovetski wrote:
> >>>> On Tue, 17 Jul 2018, Laurent Pinchart wrote:
> >>>>> On Thursday, 12 July 2018 10:30:46 EEST Guennadi Liakhovetski wrote:
> >>>>>> On Thu, 12 Jul 2018, Laurent Pinchart wrote:
> >>>>>>> On Tuesday, 8 May 2018 18:07:43 EEST Guennadi Liakhovetski wrote:
> >>>>>>>> UVC defines a method of handling asynchronous controls, which
> >>>>>>>> sends a USB packet over the interrupt pipe. This patch implements
> >>>>>>>> support for such packets by sending a control event to the user.
> >>>>>>>> Since this can involve USB traffic and, therefore, scheduling, this
> >>>>>>>> has to be done in a work queue.
> >>>>>>>> 
> >>>>>>>> Signed-off-by: Guennadi Liakhovetski
> >>>>>>>> <guennadi.liakhovetski@intel.com>
> >>>>>>>> ---
> >>>>>>>> 
> >>>>>>>> v8:
> >>>>>>>> 
> >>>>>>>> * avoid losing events by delaying the status URB resubmission
> >>>>>>>>   until after completion of the current event
 >>>>>>>> * extract control value calculation into __uvc_ctrl_get_value()
> >>>>>>>> * do not proactively return EBUSY if the previous control hasn't
> >>>>>>>>   completed yet, let the camera handle such cases
> >>>>>>>> * multiple cosmetic changes
> >>>>>>>> 
> >>>>>>>>  drivers/media/usb/uvc/uvc_ctrl.c   | 166 +++++++++++++++++++------
> >>>>>>>>  drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++---
> >>>>>>>>  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
> >>>>>>>>  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
> >>>>>>>>  include/uapi/linux/uvcvideo.h      |   2 +
> >>>>>>>>  5 files changed, 255 insertions(+), 44 deletions(-)
> >>>>>>>> 
> >>>>>>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> >>>>>>>> b/drivers/media/usb/uvc/uvc_ctrl.c index 2a213c8..796f86a 100644
> >>>>>>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> >>>>>>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> >>>>>> 
> >>>>>> [snip]
> >>>>>> 
> >>>>>>>> +static void uvc_ctrl_status_event_work(struct work_struct *work)
> >>>>>>>> +{
> >>>>>>>> +	struct uvc_device *dev = container_of(work, struct uvc_device,
> >>>>>>>> +					      async_ctrl.work);
> >>>>>>>> +	struct uvc_ctrl_work *w = &dev->async_ctrl;
> >>>>>>>> +	struct uvc_control_mapping *mapping;
> >>>>>>>> +	struct uvc_control *ctrl = w->ctrl;
> >>>>>>>> +	unsigned int i;
> >>>>>>>> +	int ret;
> >>>>>>>> +
> >>>>>>>> +	mutex_lock(&w->chain->ctrl_mutex);
> >>>>>>>> +
> >>>>>>>> +	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> >>>>>>>> +		s32 value = __uvc_ctrl_get_value(mapping, w->data);
> >>>>>>>> +
> >>>>>>>> +		/*
> >>>>>>>> +		 * So far none of the auto-update controls in the uvc_ctrls[]
> >>>>>>>> +		 * table is mapped to a V4L control with slaves in the
> >>>>>>>> +		 * uvc_ctrl_mappings[] list, so slave controls so far never
> >>>>>>>> have
> >>>>>>>> +		 * handle == NULL, but this can change in the future
> >>>>>>>> +		 */
> >>>>>>>> +		for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
> >>>>>>>> +			if (!mapping->slave_ids[i])
> >>>>>>>> +				break;
> >>>>>>>> +
> >>>>>>>> +			__uvc_ctrl_send_slave_event(ctrl->handle, w->chain,
> >>>>>>>> +						ctrl, mapping->slave_ids[i]);
> >>>>>>>> +		}
> >>>>>>>> +
> >>>>>>>> +		uvc_ctrl_send_event(ctrl->handle, ctrl, mapping, value,
> >>>>>>>> +				    V4L2_EVENT_CTRL_CH_VALUE);
> >>>>>>>> +	}
> >>>>>>>> +
> >>>>>>>> +	mutex_unlock(&w->chain->ctrl_mutex);
> >>>>>>>> +
> >>>>>>>> +	ctrl->handle = NULL;
> >>>>>>> 
> >>>>>>> Can't this race with a uvc_ctrl_set() call, resulting in
> >>>>>>> ctrl->handle being NULL after the control gets set ?
> >>>>>> 
> >>>>>> Right, it's better to set .handle to NULL before sending events.
> >>>>>> Something like
> >>>>>> 
> >>>>>> mutex_lock();
> >>>>>> 
> >>>>>> handle = ctrl->handle;
> >>>>>> ctrl->handle = NULL;
> >>>>>> 
> >>>>>> list_for_each_entry() {
> >>>>>> 
> >>>>>> 	...
> >>>>>> 	uvc_ctrl_send_event(handle,...);
> >>>>>> 
> >>>>>> }
> >>>>>> 
> >>>>>> mutex_unlock();
> >>>>>> 
> >>>>>> ?
> >>>>> 
> >>>>> I think you also have to take the same lock in the uvc_ctrl_set()
> >>>>> function to fix the problem, otherwise the ctrl->handle = NULL line
> >>>>> could still be executed after the ctrl->handle assignment in
> >>>>> uvc_ctrl_set(), resulting in ctrl->handle being NULL while the
> >>>>> control is being set.
> >>>> 
> >>>> Doesn't this mean, that you're attempting to send a new instance of
> >>>> the same control before the previous has completed? In which case also
> >>>> taking the lock in uvc_ctrl_set() wouldn't help either, because you
> >>>> can anyway do that immediately after the first instance, before the
> >>>> completion even has fired.
> >>> 
> >>> You're right that it won't solve the race completely, but wouldn't it
> >>> at least prevent ctrl->handle from being NULL ? We can't guarantee
> >>> which of the old and new handle will be used for events when multiple
> >>> control set operations are invoked, but we should try to guarantee
> >>> that the handle won't be NULL.
> >> 
> >> Sorry, I'm probably misunderstanding something. What exactly are you
> >> proposing to lock and what and how is it supposed to protect? Wouldn't
> >> the following flow still be possible, if you protect setting .handle =
> >> NULL in uvc_set_ctrl():
> >> 
> >> CPU 1                                 CPU 2
> >> 
> >> control completion interrupt
> >> (.handle = HANDLE_1)
> >> work scheduled
> >> 
> >>                                       uvc_set_ctrl()
> >>                                       .handle = HANDLE_2
> >> 
> >> uvc_ctrl_status_event_work()
> >> .handle = NULL
> >> usb_submit_urb()
> >> 
> >> control completion interrupt
> >> (.handle = NULL)
> >> 
> >> ?
> > 
> > You're absolutely right, there's no easy way to guard against this with a
> > mere lock. I think we can ignore the issue for now and address it later
> > if really needed, as the only adverse effect would be a spurious control
> > change event sent to a file handle that hasn't set the
> > V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK flag.
> 
> Ok, but I still think the above change - setting .handle to NULL before
> sending the event - should be useful?

You're right, it's useful. It will help in case userspace calls S_CTRL on CPU 
2 in response to the event. I forgot to include this change in the patch I've 
just sent. Maybe something like

    mutex_lock(&chain->ctrl_mutex);
        
    /*
     * Set ctrl->handle to NULL before sending events, to avoid a race with
     * userspace setting the control in response to the event.
     */
    handle = ctrl->handle;
    ctrl->handle = NULL;
        
    list_for_each_entry(mapping, &ctrl->info.mappings, list) {
    ...

> >>>>>>>> +	/* Resubmit the URB. */
> >>>>>>>> +	w->urb->interval = dev->int_ep->desc.bInterval;
> >>>>>>>> +	ret = usb_submit_urb(w->urb, GFP_KERNEL);
> >>>>>>>> +	if (ret < 0)
> >>>>>>>> +		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
> >>>>>>>> +			   ret);
> >>>>>>>> +}
> >>> 
> >>> [snip]

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 2/3] uvcvideo: send a control event when a Control Change interrupt arrives

[Guennadi Liakhovetski]

Hi Laurent,

Thanks for the diff. I'm about to go over it and check your comments, but 
I cannot seem to find a git tree with your current stack. Particularly, 
you wanted to apply patches 1 and 3 from this series before applying this 
one, so, I'd prefer to work on top of the same tree as you to make sure to 
avoid rejects.

Thanks
Guennaddi

On Wed, 25 Jul 2018, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> On Wednesday, 25 July 2018 20:21:54 EEST Guennadi Liakhovetski wrote:
> > On Wed, 25 Jul 2018, Laurent Pinchart wrote:
> > > On Wednesday, 18 July 2018 09:55:27 EEST Guennadi Liakhovetski wrote:
> > >> On Wed, 18 Jul 2018, Laurent Pinchart wrote:
> > >>> On Wednesday, 18 July 2018 00:30:45 EEST Guennadi Liakhovetski wrote:
> > >>>> On Tue, 17 Jul 2018, Laurent Pinchart wrote:
> > >>>>> On Thursday, 12 July 2018 10:30:46 EEST Guennadi Liakhovetski wrote:
> > >>>>>> On Thu, 12 Jul 2018, Laurent Pinchart wrote:
> > >>>>>>> On Tuesday, 8 May 2018 18:07:43 EEST Guennadi Liakhovetski wrote:
> > >>>>>>>> UVC defines a method of handling asynchronous controls, which
> > >>>>>>>> sends a USB packet over the interrupt pipe. This patch implements
> > >>>>>>>> support for such packets by sending a control event to the user.
> > >>>>>>>> Since this can involve USB traffic and, therefore, scheduling, this
> > >>>>>>>> has to be done in a work queue.
> > >>>>>>>> 
> > >>>>>>>> Signed-off-by: Guennadi Liakhovetski
> > >>>>>>>> <guennadi.liakhovetski@intel.com>
> > >>>>>>>> ---
> > >>>>>>>> 
> > >>>>>>>> v8:
> > >>>>>>>> 
> > >>>>>>>> * avoid losing events by delaying the status URB resubmission
> > >>>>>>>>   until after completion of the current event
>  >>>>>>>> * extract control value calculation into __uvc_ctrl_get_value()
> > >>>>>>>> * do not proactively return EBUSY if the previous control hasn't
> > >>>>>>>>   completed yet, let the camera handle such cases
> > >>>>>>>> * multiple cosmetic changes
> > >>>>>>>> 
> > >>>>>>>>  drivers/media/usb/uvc/uvc_ctrl.c   | 166 +++++++++++++++++++------
> > >>>>>>>>  drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++---
> > >>>>>>>>  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
> > >>>>>>>>  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
> > >>>>>>>>  include/uapi/linux/uvcvideo.h      |   2 +
> > >>>>>>>>  5 files changed, 255 insertions(+), 44 deletions(-)
> > >>>>>>>> 
> > >>>>>>>> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > >>>>>>>> b/drivers/media/usb/uvc/uvc_ctrl.c index 2a213c8..796f86a 100644
> > >>>>>>>> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > >>>>>>>> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > >>>>>> 
> > >>>>>> [snip]
> > >>>>>> 
> > >>>>>>>> +static void uvc_ctrl_status_event_work(struct work_struct *work)
> > >>>>>>>> +{
> > >>>>>>>> +	struct uvc_device *dev = container_of(work, struct uvc_device,
> > >>>>>>>> +					      async_ctrl.work);
> > >>>>>>>> +	struct uvc_ctrl_work *w = &dev->async_ctrl;
> > >>>>>>>> +	struct uvc_control_mapping *mapping;
> > >>>>>>>> +	struct uvc_control *ctrl = w->ctrl;
> > >>>>>>>> +	unsigned int i;
> > >>>>>>>> +	int ret;
> > >>>>>>>> +
> > >>>>>>>> +	mutex_lock(&w->chain->ctrl_mutex);
> > >>>>>>>> +
> > >>>>>>>> +	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> > >>>>>>>> +		s32 value = __uvc_ctrl_get_value(mapping, w->data);
> > >>>>>>>> +
> > >>>>>>>> +		/*
> > >>>>>>>> +		 * So far none of the auto-update controls in the uvc_ctrls[]
> > >>>>>>>> +		 * table is mapped to a V4L control with slaves in the
> > >>>>>>>> +		 * uvc_ctrl_mappings[] list, so slave controls so far never
> > >>>>>>>> have
> > >>>>>>>> +		 * handle == NULL, but this can change in the future
> > >>>>>>>> +		 */
> > >>>>>>>> +		for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
> > >>>>>>>> +			if (!mapping->slave_ids[i])
> > >>>>>>>> +				break;
> > >>>>>>>> +
> > >>>>>>>> +			__uvc_ctrl_send_slave_event(ctrl->handle, w->chain,
> > >>>>>>>> +						ctrl, mapping->slave_ids[i]);
> > >>>>>>>> +		}
> > >>>>>>>> +
> > >>>>>>>> +		uvc_ctrl_send_event(ctrl->handle, ctrl, mapping, value,
> > >>>>>>>> +				    V4L2_EVENT_CTRL_CH_VALUE);
> > >>>>>>>> +	}
> > >>>>>>>> +
> > >>>>>>>> +	mutex_unlock(&w->chain->ctrl_mutex);
> > >>>>>>>> +
> > >>>>>>>> +	ctrl->handle = NULL;
> > >>>>>>> 
> > >>>>>>> Can't this race with a uvc_ctrl_set() call, resulting in
> > >>>>>>> ctrl->handle being NULL after the control gets set ?
> > >>>>>> 
> > >>>>>> Right, it's better to set .handle to NULL before sending events.
> > >>>>>> Something like
> > >>>>>> 
> > >>>>>> mutex_lock();
> > >>>>>> 
> > >>>>>> handle = ctrl->handle;
> > >>>>>> ctrl->handle = NULL;
> > >>>>>> 
> > >>>>>> list_for_each_entry() {
> > >>>>>> 
> > >>>>>> 	...
> > >>>>>> 	uvc_ctrl_send_event(handle,...);
> > >>>>>> 
> > >>>>>> }
> > >>>>>> 
> > >>>>>> mutex_unlock();
> > >>>>>> 
> > >>>>>> ?
> > >>>>> 
> > >>>>> I think you also have to take the same lock in the uvc_ctrl_set()
> > >>>>> function to fix the problem, otherwise the ctrl->handle = NULL line
> > >>>>> could still be executed after the ctrl->handle assignment in
> > >>>>> uvc_ctrl_set(), resulting in ctrl->handle being NULL while the
> > >>>>> control is being set.
> > >>>> 
> > >>>> Doesn't this mean, that you're attempting to send a new instance of
> > >>>> the same control before the previous has completed? In which case also
> > >>>> taking the lock in uvc_ctrl_set() wouldn't help either, because you
> > >>>> can anyway do that immediately after the first instance, before the
> > >>>> completion even has fired.
> > >>> 
> > >>> You're right that it won't solve the race completely, but wouldn't it
> > >>> at least prevent ctrl->handle from being NULL ? We can't guarantee
> > >>> which of the old and new handle will be used for events when multiple
> > >>> control set operations are invoked, but we should try to guarantee
> > >>> that the handle won't be NULL.
> > >> 
> > >> Sorry, I'm probably misunderstanding something. What exactly are you
> > >> proposing to lock and what and how is it supposed to protect? Wouldn't
> > >> the following flow still be possible, if you protect setting .handle =
> > >> NULL in uvc_set_ctrl():
> > >> 
> > >> CPU 1                                 CPU 2
> > >> 
> > >> control completion interrupt
> > >> (.handle = HANDLE_1)
> > >> work scheduled
> > >> 
> > >>                                       uvc_set_ctrl()
> > >>                                       .handle = HANDLE_2
> > >> 
> > >> uvc_ctrl_status_event_work()
> > >> .handle = NULL
> > >> usb_submit_urb()
> > >> 
> > >> control completion interrupt
> > >> (.handle = NULL)
> > >> 
> > >> ?
> > > 
> > > You're absolutely right, there's no easy way to guard against this with a
> > > mere lock. I think we can ignore the issue for now and address it later
> > > if really needed, as the only adverse effect would be a spurious control
> > > change event sent to a file handle that hasn't set the
> > > V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK flag.
> > 
> > Ok, but I still think the above change - setting .handle to NULL before
> > sending the event - should be useful?
> 
> You're right, it's useful. It will help in case userspace calls S_CTRL on CPU 
> 2 in response to the event. I forgot to include this change in the patch I've 
> just sent. Maybe something like
> 
>     mutex_lock(&chain->ctrl_mutex);
>         
>     /*
>      * Set ctrl->handle to NULL before sending events, to avoid a race with
>      * userspace setting the control in response to the event.
>      */
>     handle = ctrl->handle;
>     ctrl->handle = NULL;
>         
>     list_for_each_entry(mapping, &ctrl->info.mappings, list) {
>     ...
> 
> > >>>>>>>> +	/* Resubmit the URB. */
> > >>>>>>>> +	w->urb->interval = dev->int_ep->desc.bInterval;
> > >>>>>>>> +	ret = usb_submit_urb(w->urb, GFP_KERNEL);
> > >>>>>>>> +	if (ret < 0)
> > >>>>>>>> +		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
> > >>>>>>>> +			   ret);
> > >>>>>>>> +}
> > >>> 
> > >>> [snip]
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

[PATCH v9] uvcvideo: send a control event when a Control Change interrupt arrives

[Guennadi Liakhovetski]

From: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>

UVC defines a method of handling asynchronous controls, which sends a
USB packet over the interrupt pipe. This patch implements support for
such packets by sending a control event to the user. Since this can
involve USB traffic and, therefore, scheduling, this has to be done
in a work queue.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
---

v9:
- multiple optimisations and style improvements from Laurent - thanks
- avoid the handle rewriting race at least in cases, when the user only 
sends a new control after receiving an event for the previous one

Laurent, you added a couple of comments, using DocBook markup like 
"@param" but you didn't mark those comments for DocBook processing with 
"/**" - I don't care that much, just wanted to check, that that was 
intentional.

 drivers/media/usb/uvc/uvc_ctrl.c   | 211 ++++++++++++++++++++++++++++---------
 drivers/media/usb/uvc/uvc_status.c | 121 ++++++++++++++++++---
 drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
 drivers/media/usb/uvc/uvcvideo.h   |  15 ++-
 include/uapi/linux/uvcvideo.h      |   2 +
 5 files changed, 286 insertions(+), 67 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 2a213c8..ddd069b 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -20,6 +20,7 @@
 #include <linux/videodev2.h>
 #include <linux/vmalloc.h>
 #include <linux/wait.h>
+#include <linux/workqueue.h>
 #include <linux/atomic.h>
 #include <media/v4l2-ctrls.h>
 
@@ -971,12 +972,30 @@ static int uvc_ctrl_populate_cache(struct uvc_video_chain *chain,
 	return 0;
 }
 
+static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
+				const u8 *data)
+{
+	s32 value = mapping->get(mapping, UVC_GET_CUR, data);
+
+	if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
+		struct uvc_menu_info *menu = mapping->menu_info;
+		unsigned int i;
+
+		for (i = 0; i < mapping->menu_count; ++i, ++menu) {
+			if (menu->value == value) {
+				value = i;
+				break;
+			}
+		}
+	}
+
+	return value;
+}
+
 static int __uvc_ctrl_get(struct uvc_video_chain *chain,
 	struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
 	s32 *value)
 {
-	struct uvc_menu_info *menu;
-	unsigned int i;
 	int ret;
 
 	if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
@@ -993,18 +1012,8 @@ static int __uvc_ctrl_get(struct uvc_video_chain *chain,
 		ctrl->loaded = 1;
 	}
 
-	*value = mapping->get(mapping, UVC_GET_CUR,
-		uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
-
-	if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
-		menu = mapping->menu_info;
-		for (i = 0; i < mapping->menu_count; ++i, ++menu) {
-			if (menu->value == *value) {
-				*value = i;
-				break;
-			}
-		}
-	}
+	*value = __uvc_ctrl_get_value(mapping,
+				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
 
 	return 0;
 }
@@ -1216,53 +1225,135 @@ static void uvc_ctrl_fill_event(struct uvc_video_chain *chain,
 	ev->u.ctrl.default_value = v4l2_ctrl.default_value;
 }
 
-static void uvc_ctrl_send_event(struct uvc_fh *handle,
-	struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
-	s32 value, u32 changes)
+/*
+ * Send control change events to all subscribers for the @ctrl control. By
+ * default the subscriber that generated the event, as identified by @handle,
+ * is not notified unless it has set the V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK flag.
+ * @handle can be NULL for asynchronous events related to auto-update controls,
+ * in which case all subscribers are notified.
+ */
+static void uvc_ctrl_send_event(struct uvc_video_chain *chain,
+	struct uvc_fh *handle, struct uvc_control *ctrl,
+	struct uvc_control_mapping *mapping, s32 value, u32 changes)
 {
+	struct v4l2_fh *originator = handle ? &handle->vfh : NULL;
 	struct v4l2_subscribed_event *sev;
 	struct v4l2_event ev;
 
 	if (list_empty(&mapping->ev_subs))
 		return;
 
-	uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, value, changes);
+	uvc_ctrl_fill_event(chain, &ev, ctrl, mapping, value, changes);
 
 	list_for_each_entry(sev, &mapping->ev_subs, node) {
-		if (sev->fh != &handle->vfh ||
+		if (sev->fh != originator ||
 		    (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK) ||
 		    (changes & V4L2_EVENT_CTRL_CH_FLAGS))
 			v4l2_event_queue_fh(sev->fh, &ev);
 	}
 }
 
-static void uvc_ctrl_send_slave_event(struct uvc_fh *handle,
-	struct uvc_control *master, u32 slave_id,
-	const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
+/*
+ * Send control change events for the slave of the @master control identified
+ * by the V4L2 ID @slave_id. The @handle identifies the event subscriber that
+ * generated the event and may be NULL for auto-update events.
+ */
+static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
+	struct uvc_fh *handle, struct uvc_control *master, u32 slave_id)
 {
 	struct uvc_control_mapping *mapping = NULL;
 	struct uvc_control *ctrl = NULL;
 	u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
-	unsigned int i;
 	s32 val = 0;
 
-	/*
-	 * We can skip sending an event for the slave if the slave
-	 * is being modified in the same transaction.
-	 */
-	for (i = 0; i < xctrls_count; i++) {
-		if (xctrls[i].id == slave_id)
-			return;
-	}
-
 	__uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0);
 	if (ctrl == NULL)
 		return;
 
-	if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0)
+	if (__uvc_ctrl_get(chain, ctrl, mapping, &val) == 0)
 		changes |= V4L2_EVENT_CTRL_CH_VALUE;
 
-	uvc_ctrl_send_event(handle, ctrl, mapping, val, changes);
+	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
+}
+
+static void uvc_ctrl_status_event_work(struct work_struct *work)
+{
+	struct uvc_device *dev = container_of(work, struct uvc_device,
+					      async_ctrl.work);
+	struct uvc_ctrl_work *w = &dev->async_ctrl;
+	struct uvc_video_chain *chain = w->chain;
+	struct uvc_control_mapping *mapping;
+	struct uvc_control *ctrl = w->ctrl;
+	struct uvc_fh *handle;
+	unsigned int i;
+	int ret;
+
+	mutex_lock(&chain->ctrl_mutex);
+
+	handle = ctrl->handle;
+	ctrl->handle = NULL;
+
+	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
+		s32 value = __uvc_ctrl_get_value(mapping, w->data);
+
+		/*
+		 * handle may be NULL here if the device sends auto-update
+		 * events without a prior related control set from userspace.
+		 */
+		for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
+			if (!mapping->slave_ids[i])
+				break;
+
+			uvc_ctrl_send_slave_event(chain, handle, ctrl,
+						  mapping->slave_ids[i]);
+		}
+
+		uvc_ctrl_send_event(chain, handle, ctrl, mapping, value,
+				    V4L2_EVENT_CTRL_CH_VALUE);
+	}
+
+	mutex_unlock(&chain->ctrl_mutex);
+
+	/* Resubmit the URB. */
+	w->urb->interval = dev->int_ep->desc.bInterval;
+	ret = usb_submit_urb(w->urb, GFP_KERNEL);
+	if (ret < 0)
+		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
+			   ret);
+}
+
+bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,
+			   struct uvc_control *ctrl, const u8 *data)
+{
+	struct uvc_device *dev = chain->dev;
+	struct uvc_ctrl_work *w = &dev->async_ctrl;
+
+	if (list_empty(&ctrl->info.mappings)) {
+		ctrl->handle = NULL;
+		return false;
+	}
+
+	w->data = data;
+	w->urb = urb;
+	w->chain = chain;
+	w->ctrl = ctrl;
+
+	schedule_work(&w->work);
+
+	return true;
+}
+
+static bool uvc_ctrl_xctrls_has_control(const struct v4l2_ext_control *xctrls,
+					unsigned int xctrls_count, u32 id)
+{
+	unsigned int i;
+
+	for (i = 0; i < xctrls_count; ++i) {
+		if (xctrls[i].id == id)
+			return true;
+	}
+
+	return false;
 }
 
 static void uvc_ctrl_send_events(struct uvc_fh *handle,
@@ -1277,29 +1368,39 @@ static void uvc_ctrl_send_events(struct uvc_fh *handle,
 	for (i = 0; i < xctrls_count; ++i) {
 		ctrl = uvc_find_control(handle->chain, xctrls[i].id, &mapping);
 
+		if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
+			/* Notification will be sent from an Interrupt event. */
+			continue;
+
 		for (j = 0; j < ARRAY_SIZE(mapping->slave_ids); ++j) {
-			if (!mapping->slave_ids[j])
+			u32 slave_id = mapping->slave_ids[j];
+
+			if (!slave_id)
 				break;
-			uvc_ctrl_send_slave_event(handle, ctrl,
-						  mapping->slave_ids[j],
-						  xctrls, xctrls_count);
+
+			/*
+			 * We can skip sending an event for the slave if the
+			 * slave is being modified in the same transaction.
+			 */
+			if (uvc_ctrl_xctrls_has_control(xctrls, xctrls_count,
+							slave_id))
+				continue;
+
+			uvc_ctrl_send_slave_event(handle->chain, handle, ctrl,
+						  slave_id);
 		}
 
 		/*
 		 * If the master is being modified in the same transaction
 		 * flags may change too.
 		 */
-		if (mapping->master_id) {
-			for (j = 0; j < xctrls_count; j++) {
-				if (xctrls[j].id == mapping->master_id) {
-					changes |= V4L2_EVENT_CTRL_CH_FLAGS;
-					break;
-				}
-			}
-		}
+		if (mapping->master_id &&
+		    uvc_ctrl_xctrls_has_control(xctrls, xctrls_count,
+						mapping->master_id))
+			changes |= V4L2_EVENT_CTRL_CH_FLAGS;
 
-		uvc_ctrl_send_event(handle, ctrl, mapping, xctrls[i].value,
-				    changes);
+		uvc_ctrl_send_event(handle->chain, handle, ctrl, mapping,
+				    xctrls[i].value, changes);
 	}
 }
 
@@ -1472,9 +1573,10 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
 	return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value);
 }
 
-int uvc_ctrl_set(struct uvc_video_chain *chain,
+int uvc_ctrl_set(struct uvc_fh *handle,
 	struct v4l2_ext_control *xctrl)
 {
+	struct uvc_video_chain *chain = handle->chain;
 	struct uvc_control *ctrl;
 	struct uvc_control_mapping *mapping;
 	s32 value;
@@ -1581,6 +1683,9 @@ int uvc_ctrl_set(struct uvc_video_chain *chain,
 	mapping->set(mapping, value,
 		uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
 
+	if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
+		ctrl->handle = handle;
+
 	ctrl->dirty = 1;
 	ctrl->modified = 1;
 	return 0;
@@ -1612,7 +1717,9 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
 			    |  (data[0] & UVC_CONTROL_CAP_SET ?
 				UVC_CTRL_FLAG_SET_CUR : 0)
 			    |  (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
-				UVC_CTRL_FLAG_AUTO_UPDATE : 0);
+				UVC_CTRL_FLAG_AUTO_UPDATE : 0)
+			    |  (data[0] & UVC_CONTROL_CAP_ASYNCHRONOUS ?
+				UVC_CTRL_FLAG_ASYNCHRONOUS : 0);
 
 	kfree(data);
 	return ret;
@@ -2173,6 +2280,8 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
 	struct uvc_entity *entity;
 	unsigned int i;
 
+	INIT_WORK(&dev->async_ctrl.work, uvc_ctrl_status_event_work);
+
 	/* Walk the entities list and instantiate controls */
 	list_for_each_entry(entity, &dev->entities, list) {
 		struct uvc_control *ctrl;
@@ -2241,6 +2350,8 @@ void uvc_ctrl_cleanup_device(struct uvc_device *dev)
 	struct uvc_entity *entity;
 	unsigned int i;
 
+	cancel_work_sync(&dev->async_ctrl.work);
+
 	/* Free controls and control mappings for all entities. */
 	list_for_each_entry(entity, &dev->entities, list) {
 		for (i = 0; i < entity->ncontrols; ++i) {
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index 7b71041..0722dc6 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -78,7 +78,24 @@ static void uvc_input_report_key(struct uvc_device *dev, unsigned int code,
 /* --------------------------------------------------------------------------
  * Status interrupt endpoint
  */
-static void uvc_event_streaming(struct uvc_device *dev, u8 *data, int len)
+struct uvc_streaming_status {
+	u8	bStatusType;
+	u8	bOriginator;
+	u8	bEvent;
+	u8	bValue[];
+} __packed;
+
+struct uvc_control_status {
+	u8	bStatusType;
+	u8	bOriginator;
+	u8	bEvent;
+	u8	bSelector;
+	u8	bAttribute;
+	u8	bValue[];
+} __packed;
+
+static void uvc_event_streaming(struct uvc_device *dev,
+				struct uvc_streaming_status *status, int len)
 {
 	if (len < 3) {
 		uvc_trace(UVC_TRACE_STATUS, "Invalid streaming status event "
@@ -86,31 +103,97 @@ static void uvc_event_streaming(struct uvc_device *dev, u8 *data, int len)
 		return;
 	}
 
-	if (data[2] == 0) {
+	if (status->bEvent == 0) {
 		if (len < 4)
 			return;
 		uvc_trace(UVC_TRACE_STATUS, "Button (intf %u) %s len %d\n",
-			data[1], data[3] ? "pressed" : "released", len);
-		uvc_input_report_key(dev, KEY_CAMERA, data[3]);
+			  status->bOriginator,
+			  status->bValue[0] ? "pressed" : "released", len);
+		uvc_input_report_key(dev, KEY_CAMERA, status->bValue[0]);
 	} else {
 		uvc_trace(UVC_TRACE_STATUS,
 			  "Stream %u error event %02x len %d.\n",
-			  data[1], data[2], len);
+			  status->bOriginator, status->bEvent, len);
 	}
 }
 
-static void uvc_event_control(struct uvc_device *dev, u8 *data, int len)
+#define UVC_CTRL_VALUE_CHANGE	0
+#define UVC_CTRL_INFO_CHANGE	1
+#define UVC_CTRL_FAILURE_CHANGE	2
+#define UVC_CTRL_MIN_CHANGE	3
+#define UVC_CTRL_MAX_CHANGE	4
+
+static struct uvc_control *uvc_event_entity_find_ctrl(struct uvc_entity *entity,
+						      u8 selector)
 {
-	char *attrs[3] = { "value", "info", "failure" };
+	struct uvc_control *ctrl;
+	unsigned int i;
+
+	for (i = 0, ctrl = entity->controls; i < entity->ncontrols; i++, ctrl++)
+		if (ctrl->info.selector == selector)
+			return ctrl;
+
+	return NULL;
+}
 
-	if (len < 6 || data[2] != 0 || data[4] > 2) {
+static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
+					const struct uvc_control_status *status,
+					struct uvc_video_chain **chain)
+{
+	list_for_each_entry((*chain), &dev->chains, list) {
+		struct uvc_entity *entity;
+		struct uvc_control *ctrl;
+
+		list_for_each_entry(entity, &(*chain)->entities, chain) {
+			if (entity->id != status->bOriginator)
+				continue;
+
+			ctrl = uvc_event_entity_find_ctrl(entity,
+							  status->bSelector);
+			if (ctrl)
+				return ctrl;
+		}
+	}
+
+	return NULL;
+}
+
+static bool uvc_event_control(struct urb *urb,
+			      const struct uvc_control_status *status, int len)
+{
+	static const char *attrs[] = { "value", "info", "failure", "min", "max" };
+	struct uvc_device *dev = urb->context;
+	struct uvc_video_chain *chain;
+	struct uvc_control *ctrl;
+
+	if (len < 6 || status->bEvent != 0 ||
+	    status->bAttribute >= ARRAY_SIZE(attrs)) {
 		uvc_trace(UVC_TRACE_STATUS, "Invalid control status event "
 				"received.\n");
-		return;
+		return false;
 	}
 
 	uvc_trace(UVC_TRACE_STATUS, "Control %u/%u %s change len %d.\n",
-		data[1], data[3], attrs[data[4]], len);
+		  status->bOriginator, status->bSelector,
+		  attrs[status->bAttribute], len);
+
+	/* Find the control. */
+	ctrl = uvc_event_find_ctrl(dev, status, &chain);
+	if (!ctrl)
+		return false;
+
+	switch (status->bAttribute) {
+	case UVC_CTRL_VALUE_CHANGE:
+		return uvc_ctrl_status_event(urb, chain, ctrl, status->bValue);
+
+	case UVC_CTRL_INFO_CHANGE:
+	case UVC_CTRL_FAILURE_CHANGE:
+	case UVC_CTRL_MIN_CHANGE:
+	case UVC_CTRL_MAX_CHANGE:
+		break;
+	}
+
+	return false;
 }
 
 static void uvc_status_complete(struct urb *urb)
@@ -138,13 +221,23 @@ static void uvc_status_complete(struct urb *urb)
 	len = urb->actual_length;
 	if (len > 0) {
 		switch (dev->status[0] & 0x0f) {
-		case UVC_STATUS_TYPE_CONTROL:
-			uvc_event_control(dev, dev->status, len);
+		case UVC_STATUS_TYPE_CONTROL: {
+			struct uvc_control_status *status =
+				(struct uvc_control_status *)dev->status;
+
+			if (uvc_event_control(urb, status, len))
+				/* The URB will be resubmitted in work context. */
+				return;
 			break;
+		}
 
-		case UVC_STATUS_TYPE_STREAMING:
-			uvc_event_streaming(dev, dev->status, len);
+		case UVC_STATUS_TYPE_STREAMING: {
+			struct uvc_streaming_status *status =
+				(struct uvc_streaming_status *)dev->status;
+
+			uvc_event_streaming(dev, status, len);
 			break;
+		}
 
 		default:
 			uvc_trace(UVC_TRACE_STATUS, "Unknown status event "
diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
index bd32914..18a7384 100644
--- a/drivers/media/usb/uvc/uvc_v4l2.c
+++ b/drivers/media/usb/uvc/uvc_v4l2.c
@@ -994,7 +994,7 @@ static int uvc_ioctl_s_ctrl(struct file *file, void *fh,
 	if (ret < 0)
 		return ret;
 
-	ret = uvc_ctrl_set(chain, &xctrl);
+	ret = uvc_ctrl_set(handle, &xctrl);
 	if (ret < 0) {
 		uvc_ctrl_rollback(handle);
 		return ret;
@@ -1069,7 +1069,7 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh *handle,
 		return ret;
 
 	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
-		ret = uvc_ctrl_set(chain, ctrl);
+		ret = uvc_ctrl_set(handle, ctrl);
 		if (ret < 0) {
 			uvc_ctrl_rollback(handle);
 			ctrls->error_idx = commit ? ctrls->count : i;
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index be5cf17..8ae03a4 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -12,6 +12,7 @@
 #include <linux/usb/video.h>
 #include <linux/uvcvideo.h>
 #include <linux/videodev2.h>
+#include <linux/workqueue.h>
 #include <media/media-device.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
@@ -256,6 +257,8 @@ struct uvc_control {
 	   initialized:1;
 
 	u8 *uvc_data;
+
+	struct uvc_fh *handle;	/* File handle that last changed the control. */
 };
 
 struct uvc_format_desc {
@@ -600,6 +603,14 @@ struct uvc_device {
 	u8 *status;
 	struct input_dev *input;
 	char input_phys[64];
+
+	struct uvc_ctrl_work {
+		struct work_struct work;
+		struct urb *urb;
+		struct uvc_video_chain *chain;
+		struct uvc_control *ctrl;
+		const void *data;
+	} async_ctrl;
 };
 
 enum uvc_handle_state {
@@ -753,6 +764,8 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
 int uvc_ctrl_init_device(struct uvc_device *dev);
 void uvc_ctrl_cleanup_device(struct uvc_device *dev);
 int uvc_ctrl_restore_values(struct uvc_device *dev);
+bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,
+			   struct uvc_control *ctrl, const u8 *data);
 
 int uvc_ctrl_begin(struct uvc_video_chain *chain);
 int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
@@ -770,7 +783,7 @@ static inline int uvc_ctrl_rollback(struct uvc_fh *handle)
 }
 
 int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);
-int uvc_ctrl_set(struct uvc_video_chain *chain, struct v4l2_ext_control *xctrl);
+int uvc_ctrl_set(struct uvc_fh *handle, struct v4l2_ext_control *xctrl);
 
 int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
 		      struct uvc_xu_control_query *xqry);
diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
index 020714d..f80f05b 100644
--- a/include/uapi/linux/uvcvideo.h
+++ b/include/uapi/linux/uvcvideo.h
@@ -28,6 +28,8 @@
 #define UVC_CTRL_FLAG_RESTORE		(1 << 6)
 /* Control can be updated by the camera. */
 #define UVC_CTRL_FLAG_AUTO_UPDATE	(1 << 7)
+/* Control supports asynchronous reporting */
+#define UVC_CTRL_FLAG_ASYNCHRONOUS	(1 << 8)
 
 #define UVC_CTRL_FLAG_GET_RANGE \
 	(UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \
-- 
1.9.3

Re: [PATCH v9] uvcvideo: send a control event when a Control Change interrupt arrives

[Laurent Pinchart]

Hi Guennadi,

Thank you for the patch.

On Thursday, 26 July 2018 11:17:53 EEST Guennadi Liakhovetski wrote:
> From: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> 
> UVC defines a method of handling asynchronous controls, which sends a
> USB packet over the interrupt pipe. This patch implements support for
> such packets by sending a control event to the user. Since this can
> involve USB traffic and, therefore, scheduling, this has to be done
> in a work queue.
> 
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

applied to my tree and pushed to the uvc/next branch.

> ---
> 
> v9:
> - multiple optimisations and style improvements from Laurent - thanks
> - avoid the handle rewriting race at least in cases, when the user only
> sends a new control after receiving an event for the previous one
> 
> Laurent, you added a couple of comments, using DocBook markup like
> "@param" but you didn't mark those comments for DocBook processing with
> "/**" - I don't care that much, just wanted to check, that that was
> intentional.

Yes, it's not worth compiling that as kerneldoc, I just wanted to mark 
references to variable names for clarity.

>  drivers/media/usb/uvc/uvc_ctrl.c   | 211
> ++++++++++++++++++++++++++++--------- drivers/media/usb/uvc/uvc_status.c |
> 121 ++++++++++++++++++---
>  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
>  drivers/media/usb/uvc/uvcvideo.h   |  15 ++-
>  include/uapi/linux/uvcvideo.h      |   2 +
>  5 files changed, 286 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> b/drivers/media/usb/uvc/uvc_ctrl.c index 2a213c8..ddd069b 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -20,6 +20,7 @@
>  #include <linux/videodev2.h>
>  #include <linux/vmalloc.h>
>  #include <linux/wait.h>
> +#include <linux/workqueue.h>
>  #include <linux/atomic.h>
>  #include <media/v4l2-ctrls.h>
> 
> @@ -971,12 +972,30 @@ static int uvc_ctrl_populate_cache(struct
> uvc_video_chain *chain, return 0;
>  }
> 
> +static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
> +				const u8 *data)
> +{
> +	s32 value = mapping->get(mapping, UVC_GET_CUR, data);
> +
> +	if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
> +		struct uvc_menu_info *menu = mapping->menu_info;
> +		unsigned int i;
> +
> +		for (i = 0; i < mapping->menu_count; ++i, ++menu) {
> +			if (menu->value == value) {
> +				value = i;
> +				break;
> +			}
> +		}
> +	}
> +
> +	return value;
> +}
> +
>  static int __uvc_ctrl_get(struct uvc_video_chain *chain,
>  	struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
>  	s32 *value)
>  {
> -	struct uvc_menu_info *menu;
> -	unsigned int i;
>  	int ret;
> 
>  	if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> @@ -993,18 +1012,8 @@ static int __uvc_ctrl_get(struct uvc_video_chain
> *chain, ctrl->loaded = 1;
>  	}
> 
> -	*value = mapping->get(mapping, UVC_GET_CUR,
> -		uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> -
> -	if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
> -		menu = mapping->menu_info;
> -		for (i = 0; i < mapping->menu_count; ++i, ++menu) {
> -			if (menu->value == *value) {
> -				*value = i;
> -				break;
> -			}
> -		}
> -	}
> +	*value = __uvc_ctrl_get_value(mapping,
> +				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> 
>  	return 0;
>  }
> @@ -1216,53 +1225,135 @@ static void uvc_ctrl_fill_event(struct
> uvc_video_chain *chain, ev->u.ctrl.default_value = v4l2_ctrl.default_value;
>  }
> 
> -static void uvc_ctrl_send_event(struct uvc_fh *handle,
> -	struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> -	s32 value, u32 changes)
> +/*
> + * Send control change events to all subscribers for the @ctrl control. By
> + * default the subscriber that generated the event, as identified by
> @handle, + * is not notified unless it has set the
> V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK flag. + * @handle can be NULL for
> asynchronous events related to auto-update controls, + * in which case all
> subscribers are notified.
> + */
> +static void uvc_ctrl_send_event(struct uvc_video_chain *chain,
> +	struct uvc_fh *handle, struct uvc_control *ctrl,
> +	struct uvc_control_mapping *mapping, s32 value, u32 changes)
>  {
> +	struct v4l2_fh *originator = handle ? &handle->vfh : NULL;
>  	struct v4l2_subscribed_event *sev;
>  	struct v4l2_event ev;
> 
>  	if (list_empty(&mapping->ev_subs))
>  		return;
> 
> -	uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, value, changes);
> +	uvc_ctrl_fill_event(chain, &ev, ctrl, mapping, value, changes);
> 
>  	list_for_each_entry(sev, &mapping->ev_subs, node) {
> -		if (sev->fh != &handle->vfh ||
> +		if (sev->fh != originator ||
>  		    (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK) ||
>  		    (changes & V4L2_EVENT_CTRL_CH_FLAGS))
>  			v4l2_event_queue_fh(sev->fh, &ev);
>  	}
>  }
> 
> -static void uvc_ctrl_send_slave_event(struct uvc_fh *handle,
> -	struct uvc_control *master, u32 slave_id,
> -	const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
> +/*
> + * Send control change events for the slave of the @master control
> identified + * by the V4L2 ID @slave_id. The @handle identifies the event
> subscriber that + * generated the event and may be NULL for auto-update
> events.
> + */
> +static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
> +	struct uvc_fh *handle, struct uvc_control *master, u32 slave_id)
>  {
>  	struct uvc_control_mapping *mapping = NULL;
>  	struct uvc_control *ctrl = NULL;
>  	u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
> -	unsigned int i;
>  	s32 val = 0;
> 
> -	/*
> -	 * We can skip sending an event for the slave if the slave
> -	 * is being modified in the same transaction.
> -	 */
> -	for (i = 0; i < xctrls_count; i++) {
> -		if (xctrls[i].id == slave_id)
> -			return;
> -	}
> -
>  	__uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0);
>  	if (ctrl == NULL)
>  		return;
> 
> -	if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0)
> +	if (__uvc_ctrl_get(chain, ctrl, mapping, &val) == 0)
>  		changes |= V4L2_EVENT_CTRL_CH_VALUE;
> 
> -	uvc_ctrl_send_event(handle, ctrl, mapping, val, changes);
> +	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
> +}
> +
> +static void uvc_ctrl_status_event_work(struct work_struct *work)
> +{
> +	struct uvc_device *dev = container_of(work, struct uvc_device,
> +					      async_ctrl.work);
> +	struct uvc_ctrl_work *w = &dev->async_ctrl;
> +	struct uvc_video_chain *chain = w->chain;
> +	struct uvc_control_mapping *mapping;
> +	struct uvc_control *ctrl = w->ctrl;
> +	struct uvc_fh *handle;
> +	unsigned int i;
> +	int ret;
> +
> +	mutex_lock(&chain->ctrl_mutex);
> +
> +	handle = ctrl->handle;
> +	ctrl->handle = NULL;
> +
> +	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> +		s32 value = __uvc_ctrl_get_value(mapping, w->data);
> +
> +		/*
> +		 * handle may be NULL here if the device sends auto-update
> +		 * events without a prior related control set from userspace.
> +		 */
> +		for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
> +			if (!mapping->slave_ids[i])
> +				break;
> +
> +			uvc_ctrl_send_slave_event(chain, handle, ctrl,
> +						  mapping->slave_ids[i]);
> +		}
> +
> +		uvc_ctrl_send_event(chain, handle, ctrl, mapping, value,
> +				    V4L2_EVENT_CTRL_CH_VALUE);
> +	}
> +
> +	mutex_unlock(&chain->ctrl_mutex);
> +
> +	/* Resubmit the URB. */
> +	w->urb->interval = dev->int_ep->desc.bInterval;
> +	ret = usb_submit_urb(w->urb, GFP_KERNEL);
> +	if (ret < 0)
> +		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
> +			   ret);
> +}
> +
> +bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,
> +			   struct uvc_control *ctrl, const u8 *data)
> +{
> +	struct uvc_device *dev = chain->dev;
> +	struct uvc_ctrl_work *w = &dev->async_ctrl;
> +
> +	if (list_empty(&ctrl->info.mappings)) {
> +		ctrl->handle = NULL;
> +		return false;
> +	}
> +
> +	w->data = data;
> +	w->urb = urb;
> +	w->chain = chain;
> +	w->ctrl = ctrl;
> +
> +	schedule_work(&w->work);
> +
> +	return true;
> +}
> +
> +static bool uvc_ctrl_xctrls_has_control(const struct v4l2_ext_control
> *xctrls, +					unsigned int xctrls_count, u32 id)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < xctrls_count; ++i) {
> +		if (xctrls[i].id == id)
> +			return true;
> +	}
> +
> +	return false;
>  }
> 
>  static void uvc_ctrl_send_events(struct uvc_fh *handle,
> @@ -1277,29 +1368,39 @@ static void uvc_ctrl_send_events(struct uvc_fh
> *handle, for (i = 0; i < xctrls_count; ++i) {
>  		ctrl = uvc_find_control(handle->chain, xctrls[i].id, &mapping);
> 
> +		if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> +			/* Notification will be sent from an Interrupt event. */
> +			continue;
> +
>  		for (j = 0; j < ARRAY_SIZE(mapping->slave_ids); ++j) {
> -			if (!mapping->slave_ids[j])
> +			u32 slave_id = mapping->slave_ids[j];
> +
> +			if (!slave_id)
>  				break;
> -			uvc_ctrl_send_slave_event(handle, ctrl,
> -						  mapping->slave_ids[j],
> -						  xctrls, xctrls_count);
> +
> +			/*
> +			 * We can skip sending an event for the slave if the
> +			 * slave is being modified in the same transaction.
> +			 */
> +			if (uvc_ctrl_xctrls_has_control(xctrls, xctrls_count,
> +							slave_id))
> +				continue;
> +
> +			uvc_ctrl_send_slave_event(handle->chain, handle, ctrl,
> +						  slave_id);
>  		}
> 
>  		/*
>  		 * If the master is being modified in the same transaction
>  		 * flags may change too.
>  		 */
> -		if (mapping->master_id) {
> -			for (j = 0; j < xctrls_count; j++) {
> -				if (xctrls[j].id == mapping->master_id) {
> -					changes |= V4L2_EVENT_CTRL_CH_FLAGS;
> -					break;
> -				}
> -			}
> -		}
> +		if (mapping->master_id &&
> +		    uvc_ctrl_xctrls_has_control(xctrls, xctrls_count,
> +						mapping->master_id))
> +			changes |= V4L2_EVENT_CTRL_CH_FLAGS;
> 
> -		uvc_ctrl_send_event(handle, ctrl, mapping, xctrls[i].value,
> -				    changes);
> +		uvc_ctrl_send_event(handle->chain, handle, ctrl, mapping,
> +				    xctrls[i].value, changes);
>  	}
>  }
> 
> @@ -1472,9 +1573,10 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
>  	return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value);
>  }
> 
> -int uvc_ctrl_set(struct uvc_video_chain *chain,
> +int uvc_ctrl_set(struct uvc_fh *handle,
>  	struct v4l2_ext_control *xctrl)
>  {
> +	struct uvc_video_chain *chain = handle->chain;
>  	struct uvc_control *ctrl;
>  	struct uvc_control_mapping *mapping;
>  	s32 value;
> @@ -1581,6 +1683,9 @@ int uvc_ctrl_set(struct uvc_video_chain *chain,
>  	mapping->set(mapping, value,
>  		uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> 
> +	if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> +		ctrl->handle = handle;
> +
>  	ctrl->dirty = 1;
>  	ctrl->modified = 1;
>  	return 0;
> @@ -1612,7 +1717,9 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
> 
>  			    |  (data[0] & UVC_CONTROL_CAP_SET ?
> 
>  				UVC_CTRL_FLAG_SET_CUR : 0)
> 
>  			    |  (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> 
> -				UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> +				UVC_CTRL_FLAG_AUTO_UPDATE : 0)
> +			    |  (data[0] & UVC_CONTROL_CAP_ASYNCHRONOUS ?
> +				UVC_CTRL_FLAG_ASYNCHRONOUS : 0);
> 
>  	kfree(data);
>  	return ret;
> @@ -2173,6 +2280,8 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
>  	struct uvc_entity *entity;
>  	unsigned int i;
> 
> +	INIT_WORK(&dev->async_ctrl.work, uvc_ctrl_status_event_work);
> +
>  	/* Walk the entities list and instantiate controls */
>  	list_for_each_entry(entity, &dev->entities, list) {
>  		struct uvc_control *ctrl;
> @@ -2241,6 +2350,8 @@ void uvc_ctrl_cleanup_device(struct uvc_device *dev)
>  	struct uvc_entity *entity;
>  	unsigned int i;
> 
> +	cancel_work_sync(&dev->async_ctrl.work);
> +
>  	/* Free controls and control mappings for all entities. */
>  	list_for_each_entry(entity, &dev->entities, list) {
>  		for (i = 0; i < entity->ncontrols; ++i) {
> diff --git a/drivers/media/usb/uvc/uvc_status.c
> b/drivers/media/usb/uvc/uvc_status.c index 7b71041..0722dc6 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -78,7 +78,24 @@ static void uvc_input_report_key(struct uvc_device *dev,
> unsigned int code, /*
> --------------------------------------------------------------------------
> * Status interrupt endpoint
>   */
> -static void uvc_event_streaming(struct uvc_device *dev, u8 *data, int len)
> +struct uvc_streaming_status {
> +	u8	bStatusType;
> +	u8	bOriginator;
> +	u8	bEvent;
> +	u8	bValue[];
> +} __packed;
> +
> +struct uvc_control_status {
> +	u8	bStatusType;
> +	u8	bOriginator;
> +	u8	bEvent;
> +	u8	bSelector;
> +	u8	bAttribute;
> +	u8	bValue[];
> +} __packed;
> +
> +static void uvc_event_streaming(struct uvc_device *dev,
> +				struct uvc_streaming_status *status, int len)
>  {
>  	if (len < 3) {
>  		uvc_trace(UVC_TRACE_STATUS, "Invalid streaming status event "
> @@ -86,31 +103,97 @@ static void uvc_event_streaming(struct uvc_device *dev,
> u8 *data, int len) return;
>  	}
> 
> -	if (data[2] == 0) {
> +	if (status->bEvent == 0) {
>  		if (len < 4)
>  			return;
>  		uvc_trace(UVC_TRACE_STATUS, "Button (intf %u) %s len %d\n",
> -			data[1], data[3] ? "pressed" : "released", len);
> -		uvc_input_report_key(dev, KEY_CAMERA, data[3]);
> +			  status->bOriginator,
> +			  status->bValue[0] ? "pressed" : "released", len);
> +		uvc_input_report_key(dev, KEY_CAMERA, status->bValue[0]);
>  	} else {
>  		uvc_trace(UVC_TRACE_STATUS,
>  			  "Stream %u error event %02x len %d.\n",
> -			  data[1], data[2], len);
> +			  status->bOriginator, status->bEvent, len);
>  	}
>  }
> 
> -static void uvc_event_control(struct uvc_device *dev, u8 *data, int len)
> +#define UVC_CTRL_VALUE_CHANGE	0
> +#define UVC_CTRL_INFO_CHANGE	1
> +#define UVC_CTRL_FAILURE_CHANGE	2
> +#define UVC_CTRL_MIN_CHANGE	3
> +#define UVC_CTRL_MAX_CHANGE	4
> +
> +static struct uvc_control *uvc_event_entity_find_ctrl(struct uvc_entity
> *entity, +						      u8 selector)
>  {
> -	char *attrs[3] = { "value", "info", "failure" };
> +	struct uvc_control *ctrl;
> +	unsigned int i;
> +
> +	for (i = 0, ctrl = entity->controls; i < entity->ncontrols; i++, ctrl++)
> +		if (ctrl->info.selector == selector)
> +			return ctrl;
> +
> +	return NULL;
> +}
> 
> -	if (len < 6 || data[2] != 0 || data[4] > 2) {
> +static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
> +					const struct uvc_control_status *status,
> +					struct uvc_video_chain **chain)
> +{
> +	list_for_each_entry((*chain), &dev->chains, list) {
> +		struct uvc_entity *entity;
> +		struct uvc_control *ctrl;
> +
> +		list_for_each_entry(entity, &(*chain)->entities, chain) {
> +			if (entity->id != status->bOriginator)
> +				continue;
> +
> +			ctrl = uvc_event_entity_find_ctrl(entity,
> +							  status->bSelector);
> +			if (ctrl)
> +				return ctrl;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
> +static bool uvc_event_control(struct urb *urb,
> +			      const struct uvc_control_status *status, int len)
> +{
> +	static const char *attrs[] = { "value", "info", "failure", "min", "max" };
> +	struct uvc_device *dev = urb->context;
> +	struct uvc_video_chain *chain;
> +	struct uvc_control *ctrl;
> +
> +	if (len < 6 || status->bEvent != 0 ||
> +	    status->bAttribute >= ARRAY_SIZE(attrs)) {
>  		uvc_trace(UVC_TRACE_STATUS, "Invalid control status event "
>  				"received.\n");
> -		return;
> +		return false;
>  	}
> 
>  	uvc_trace(UVC_TRACE_STATUS, "Control %u/%u %s change len %d.\n",
> -		data[1], data[3], attrs[data[4]], len);
> +		  status->bOriginator, status->bSelector,
> +		  attrs[status->bAttribute], len);
> +
> +	/* Find the control. */
> +	ctrl = uvc_event_find_ctrl(dev, status, &chain);
> +	if (!ctrl)
> +		return false;
> +
> +	switch (status->bAttribute) {
> +	case UVC_CTRL_VALUE_CHANGE:
> +		return uvc_ctrl_status_event(urb, chain, ctrl, status->bValue);
> +
> +	case UVC_CTRL_INFO_CHANGE:
> +	case UVC_CTRL_FAILURE_CHANGE:
> +	case UVC_CTRL_MIN_CHANGE:
> +	case UVC_CTRL_MAX_CHANGE:
> +		break;
> +	}
> +
> +	return false;
>  }
> 
>  static void uvc_status_complete(struct urb *urb)
> @@ -138,13 +221,23 @@ static void uvc_status_complete(struct urb *urb)
>  	len = urb->actual_length;
>  	if (len > 0) {
>  		switch (dev->status[0] & 0x0f) {
> -		case UVC_STATUS_TYPE_CONTROL:
> -			uvc_event_control(dev, dev->status, len);
> +		case UVC_STATUS_TYPE_CONTROL: {
> +			struct uvc_control_status *status =
> +				(struct uvc_control_status *)dev->status;
> +
> +			if (uvc_event_control(urb, status, len))
> +				/* The URB will be resubmitted in work context. */
> +				return;
>  			break;
> +		}
> 
> -		case UVC_STATUS_TYPE_STREAMING:
> -			uvc_event_streaming(dev, dev->status, len);
> +		case UVC_STATUS_TYPE_STREAMING: {
> +			struct uvc_streaming_status *status =
> +				(struct uvc_streaming_status *)dev->status;
> +
> +			uvc_event_streaming(dev, status, len);
>  			break;
> +		}
> 
>  		default:
>  			uvc_trace(UVC_TRACE_STATUS, "Unknown status event "
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> b/drivers/media/usb/uvc/uvc_v4l2.c index bd32914..18a7384 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -994,7 +994,7 @@ static int uvc_ioctl_s_ctrl(struct file *file, void *fh,
> if (ret < 0)
>  		return ret;
> 
> -	ret = uvc_ctrl_set(chain, &xctrl);
> +	ret = uvc_ctrl_set(handle, &xctrl);
>  	if (ret < 0) {
>  		uvc_ctrl_rollback(handle);
>  		return ret;
> @@ -1069,7 +1069,7 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh
> *handle, return ret;
> 
>  	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> -		ret = uvc_ctrl_set(chain, ctrl);
> +		ret = uvc_ctrl_set(handle, ctrl);
>  		if (ret < 0) {
>  			uvc_ctrl_rollback(handle);
>  			ctrls->error_idx = commit ? ctrls->count : i;
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index be5cf17..8ae03a4 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -12,6 +12,7 @@
>  #include <linux/usb/video.h>
>  #include <linux/uvcvideo.h>
>  #include <linux/videodev2.h>
> +#include <linux/workqueue.h>
>  #include <media/media-device.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-event.h>
> @@ -256,6 +257,8 @@ struct uvc_control {
>  	   initialized:1;
> 
>  	u8 *uvc_data;
> +
> +	struct uvc_fh *handle;	/* File handle that last changed the control. */
>  };
> 
>  struct uvc_format_desc {
> @@ -600,6 +603,14 @@ struct uvc_device {
>  	u8 *status;
>  	struct input_dev *input;
>  	char input_phys[64];
> +
> +	struct uvc_ctrl_work {
> +		struct work_struct work;
> +		struct urb *urb;
> +		struct uvc_video_chain *chain;
> +		struct uvc_control *ctrl;
> +		const void *data;
> +	} async_ctrl;
>  };
> 
>  enum uvc_handle_state {
> @@ -753,6 +764,8 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
>  int uvc_ctrl_init_device(struct uvc_device *dev);
>  void uvc_ctrl_cleanup_device(struct uvc_device *dev);
>  int uvc_ctrl_restore_values(struct uvc_device *dev);
> +bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,
> +			   struct uvc_control *ctrl, const u8 *data);
> 
>  int uvc_ctrl_begin(struct uvc_video_chain *chain);
>  int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> @@ -770,7 +783,7 @@ static inline int uvc_ctrl_rollback(struct uvc_fh
> *handle) }
> 
>  int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control
> *xctrl); -int uvc_ctrl_set(struct uvc_video_chain *chain, struct
> v4l2_ext_control *xctrl); +int uvc_ctrl_set(struct uvc_fh *handle, struct
> v4l2_ext_control *xctrl);
> 
>  int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
>  		      struct uvc_xu_control_query *xqry);
> diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
> index 020714d..f80f05b 100644
> --- a/include/uapi/linux/uvcvideo.h
> +++ b/include/uapi/linux/uvcvideo.h
> @@ -28,6 +28,8 @@
>  #define UVC_CTRL_FLAG_RESTORE		(1 << 6)
>  /* Control can be updated by the camera. */
>  #define UVC_CTRL_FLAG_AUTO_UPDATE	(1 << 7)
> +/* Control supports asynchronous reporting */
> +#define UVC_CTRL_FLAG_ASYNCHRONOUS	(1 << 8)
> 
>  #define UVC_CTRL_FLAG_GET_RANGE \
>  	(UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \


-- 
Regards,

Laurent Pinchart

Re: [PATCH v9] uvcvideo: send a control event when a Control Change interrupt arrives

[Guennadi Liakhovetski]

Hi Laurent,

Thanks. Now we can get to the next one: 
https://patchwork.linuxtv.org/patch/46184/ - without that one nobody can 
get complete D4XX metadata. After I get your comments to it I'll address 
them together with Sakari's ones.

Thanks
Guennadi

On Thu, 26 Jul 2018, Laurent Pinchart wrote:

> Hi Guennadi,
> 
> Thank you for the patch.
> 
> On Thursday, 26 July 2018 11:17:53 EEST Guennadi Liakhovetski wrote:
> > From: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> > 
> > UVC defines a method of handling asynchronous controls, which sends a
> > USB packet over the interrupt pipe. This patch implements support for
> > such packets by sending a control event to the user. Since this can
> > involve USB traffic and, therefore, scheduling, this has to be done
> > in a work queue.
> > 
> > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> applied to my tree and pushed to the uvc/next branch.
> 
> > ---
> > 
> > v9:
> > - multiple optimisations and style improvements from Laurent - thanks
> > - avoid the handle rewriting race at least in cases, when the user only
> > sends a new control after receiving an event for the previous one
> > 
> > Laurent, you added a couple of comments, using DocBook markup like
> > "@param" but you didn't mark those comments for DocBook processing with
> > "/**" - I don't care that much, just wanted to check, that that was
> > intentional.
> 
> Yes, it's not worth compiling that as kerneldoc, I just wanted to mark 
> references to variable names for clarity.
> 
> >  drivers/media/usb/uvc/uvc_ctrl.c   | 211
> > ++++++++++++++++++++++++++++--------- drivers/media/usb/uvc/uvc_status.c |
> > 121 ++++++++++++++++++---
> >  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
> >  drivers/media/usb/uvc/uvcvideo.h   |  15 ++-
> >  include/uapi/linux/uvcvideo.h      |   2 +
> >  5 files changed, 286 insertions(+), 67 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> > b/drivers/media/usb/uvc/uvc_ctrl.c index 2a213c8..ddd069b 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/videodev2.h>
> >  #include <linux/vmalloc.h>
> >  #include <linux/wait.h>
> > +#include <linux/workqueue.h>
> >  #include <linux/atomic.h>
> >  #include <media/v4l2-ctrls.h>
> > 
> > @@ -971,12 +972,30 @@ static int uvc_ctrl_populate_cache(struct
> > uvc_video_chain *chain, return 0;
> >  }
> > 
> > +static s32 __uvc_ctrl_get_value(struct uvc_control_mapping *mapping,
> > +				const u8 *data)
> > +{
> > +	s32 value = mapping->get(mapping, UVC_GET_CUR, data);
> > +
> > +	if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
> > +		struct uvc_menu_info *menu = mapping->menu_info;
> > +		unsigned int i;
> > +
> > +		for (i = 0; i < mapping->menu_count; ++i, ++menu) {
> > +			if (menu->value == value) {
> > +				value = i;
> > +				break;
> > +			}
> > +		}
> > +	}
> > +
> > +	return value;
> > +}
> > +
> >  static int __uvc_ctrl_get(struct uvc_video_chain *chain,
> >  	struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> >  	s32 *value)
> >  {
> > -	struct uvc_menu_info *menu;
> > -	unsigned int i;
> >  	int ret;
> > 
> >  	if ((ctrl->info.flags & UVC_CTRL_FLAG_GET_CUR) == 0)
> > @@ -993,18 +1012,8 @@ static int __uvc_ctrl_get(struct uvc_video_chain
> > *chain, ctrl->loaded = 1;
> >  	}
> > 
> > -	*value = mapping->get(mapping, UVC_GET_CUR,
> > -		uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> > -
> > -	if (mapping->v4l2_type == V4L2_CTRL_TYPE_MENU) {
> > -		menu = mapping->menu_info;
> > -		for (i = 0; i < mapping->menu_count; ++i, ++menu) {
> > -			if (menu->value == *value) {
> > -				*value = i;
> > -				break;
> > -			}
> > -		}
> > -	}
> > +	*value = __uvc_ctrl_get_value(mapping,
> > +				uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> > 
> >  	return 0;
> >  }
> > @@ -1216,53 +1225,135 @@ static void uvc_ctrl_fill_event(struct
> > uvc_video_chain *chain, ev->u.ctrl.default_value = v4l2_ctrl.default_value;
> >  }
> > 
> > -static void uvc_ctrl_send_event(struct uvc_fh *handle,
> > -	struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> > -	s32 value, u32 changes)
> > +/*
> > + * Send control change events to all subscribers for the @ctrl control. By
> > + * default the subscriber that generated the event, as identified by
> > @handle, + * is not notified unless it has set the
> > V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK flag. + * @handle can be NULL for
> > asynchronous events related to auto-update controls, + * in which case all
> > subscribers are notified.
> > + */
> > +static void uvc_ctrl_send_event(struct uvc_video_chain *chain,
> > +	struct uvc_fh *handle, struct uvc_control *ctrl,
> > +	struct uvc_control_mapping *mapping, s32 value, u32 changes)
> >  {
> > +	struct v4l2_fh *originator = handle ? &handle->vfh : NULL;
> >  	struct v4l2_subscribed_event *sev;
> >  	struct v4l2_event ev;
> > 
> >  	if (list_empty(&mapping->ev_subs))
> >  		return;
> > 
> > -	uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, value, changes);
> > +	uvc_ctrl_fill_event(chain, &ev, ctrl, mapping, value, changes);
> > 
> >  	list_for_each_entry(sev, &mapping->ev_subs, node) {
> > -		if (sev->fh != &handle->vfh ||
> > +		if (sev->fh != originator ||
> >  		    (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK) ||
> >  		    (changes & V4L2_EVENT_CTRL_CH_FLAGS))
> >  			v4l2_event_queue_fh(sev->fh, &ev);
> >  	}
> >  }
> > 
> > -static void uvc_ctrl_send_slave_event(struct uvc_fh *handle,
> > -	struct uvc_control *master, u32 slave_id,
> > -	const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
> > +/*
> > + * Send control change events for the slave of the @master control
> > identified + * by the V4L2 ID @slave_id. The @handle identifies the event
> > subscriber that + * generated the event and may be NULL for auto-update
> > events.
> > + */
> > +static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
> > +	struct uvc_fh *handle, struct uvc_control *master, u32 slave_id)
> >  {
> >  	struct uvc_control_mapping *mapping = NULL;
> >  	struct uvc_control *ctrl = NULL;
> >  	u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
> > -	unsigned int i;
> >  	s32 val = 0;
> > 
> > -	/*
> > -	 * We can skip sending an event for the slave if the slave
> > -	 * is being modified in the same transaction.
> > -	 */
> > -	for (i = 0; i < xctrls_count; i++) {
> > -		if (xctrls[i].id == slave_id)
> > -			return;
> > -	}
> > -
> >  	__uvc_find_control(master->entity, slave_id, &mapping, &ctrl, 0);
> >  	if (ctrl == NULL)
> >  		return;
> > 
> > -	if (__uvc_ctrl_get(handle->chain, ctrl, mapping, &val) == 0)
> > +	if (__uvc_ctrl_get(chain, ctrl, mapping, &val) == 0)
> >  		changes |= V4L2_EVENT_CTRL_CH_VALUE;
> > 
> > -	uvc_ctrl_send_event(handle, ctrl, mapping, val, changes);
> > +	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
> > +}
> > +
> > +static void uvc_ctrl_status_event_work(struct work_struct *work)
> > +{
> > +	struct uvc_device *dev = container_of(work, struct uvc_device,
> > +					      async_ctrl.work);
> > +	struct uvc_ctrl_work *w = &dev->async_ctrl;
> > +	struct uvc_video_chain *chain = w->chain;
> > +	struct uvc_control_mapping *mapping;
> > +	struct uvc_control *ctrl = w->ctrl;
> > +	struct uvc_fh *handle;
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	mutex_lock(&chain->ctrl_mutex);
> > +
> > +	handle = ctrl->handle;
> > +	ctrl->handle = NULL;
> > +
> > +	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
> > +		s32 value = __uvc_ctrl_get_value(mapping, w->data);
> > +
> > +		/*
> > +		 * handle may be NULL here if the device sends auto-update
> > +		 * events without a prior related control set from userspace.
> > +		 */
> > +		for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
> > +			if (!mapping->slave_ids[i])
> > +				break;
> > +
> > +			uvc_ctrl_send_slave_event(chain, handle, ctrl,
> > +						  mapping->slave_ids[i]);
> > +		}
> > +
> > +		uvc_ctrl_send_event(chain, handle, ctrl, mapping, value,
> > +				    V4L2_EVENT_CTRL_CH_VALUE);
> > +	}
> > +
> > +	mutex_unlock(&chain->ctrl_mutex);
> > +
> > +	/* Resubmit the URB. */
> > +	w->urb->interval = dev->int_ep->desc.bInterval;
> > +	ret = usb_submit_urb(w->urb, GFP_KERNEL);
> > +	if (ret < 0)
> > +		uvc_printk(KERN_ERR, "Failed to resubmit status URB (%d).\n",
> > +			   ret);
> > +}
> > +
> > +bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,
> > +			   struct uvc_control *ctrl, const u8 *data)
> > +{
> > +	struct uvc_device *dev = chain->dev;
> > +	struct uvc_ctrl_work *w = &dev->async_ctrl;
> > +
> > +	if (list_empty(&ctrl->info.mappings)) {
> > +		ctrl->handle = NULL;
> > +		return false;
> > +	}
> > +
> > +	w->data = data;
> > +	w->urb = urb;
> > +	w->chain = chain;
> > +	w->ctrl = ctrl;
> > +
> > +	schedule_work(&w->work);
> > +
> > +	return true;
> > +}
> > +
> > +static bool uvc_ctrl_xctrls_has_control(const struct v4l2_ext_control
> > *xctrls, +					unsigned int xctrls_count, u32 id)
> > +{
> > +	unsigned int i;
> > +
> > +	for (i = 0; i < xctrls_count; ++i) {
> > +		if (xctrls[i].id == id)
> > +			return true;
> > +	}
> > +
> > +	return false;
> >  }
> > 
> >  static void uvc_ctrl_send_events(struct uvc_fh *handle,
> > @@ -1277,29 +1368,39 @@ static void uvc_ctrl_send_events(struct uvc_fh
> > *handle, for (i = 0; i < xctrls_count; ++i) {
> >  		ctrl = uvc_find_control(handle->chain, xctrls[i].id, &mapping);
> > 
> > +		if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> > +			/* Notification will be sent from an Interrupt event. */
> > +			continue;
> > +
> >  		for (j = 0; j < ARRAY_SIZE(mapping->slave_ids); ++j) {
> > -			if (!mapping->slave_ids[j])
> > +			u32 slave_id = mapping->slave_ids[j];
> > +
> > +			if (!slave_id)
> >  				break;
> > -			uvc_ctrl_send_slave_event(handle, ctrl,
> > -						  mapping->slave_ids[j],
> > -						  xctrls, xctrls_count);
> > +
> > +			/*
> > +			 * We can skip sending an event for the slave if the
> > +			 * slave is being modified in the same transaction.
> > +			 */
> > +			if (uvc_ctrl_xctrls_has_control(xctrls, xctrls_count,
> > +							slave_id))
> > +				continue;
> > +
> > +			uvc_ctrl_send_slave_event(handle->chain, handle, ctrl,
> > +						  slave_id);
> >  		}
> > 
> >  		/*
> >  		 * If the master is being modified in the same transaction
> >  		 * flags may change too.
> >  		 */
> > -		if (mapping->master_id) {
> > -			for (j = 0; j < xctrls_count; j++) {
> > -				if (xctrls[j].id == mapping->master_id) {
> > -					changes |= V4L2_EVENT_CTRL_CH_FLAGS;
> > -					break;
> > -				}
> > -			}
> > -		}
> > +		if (mapping->master_id &&
> > +		    uvc_ctrl_xctrls_has_control(xctrls, xctrls_count,
> > +						mapping->master_id))
> > +			changes |= V4L2_EVENT_CTRL_CH_FLAGS;
> > 
> > -		uvc_ctrl_send_event(handle, ctrl, mapping, xctrls[i].value,
> > -				    changes);
> > +		uvc_ctrl_send_event(handle->chain, handle, ctrl, mapping,
> > +				    xctrls[i].value, changes);
> >  	}
> >  }
> > 
> > @@ -1472,9 +1573,10 @@ int uvc_ctrl_get(struct uvc_video_chain *chain,
> >  	return __uvc_ctrl_get(chain, ctrl, mapping, &xctrl->value);
> >  }
> > 
> > -int uvc_ctrl_set(struct uvc_video_chain *chain,
> > +int uvc_ctrl_set(struct uvc_fh *handle,
> >  	struct v4l2_ext_control *xctrl)
> >  {
> > +	struct uvc_video_chain *chain = handle->chain;
> >  	struct uvc_control *ctrl;
> >  	struct uvc_control_mapping *mapping;
> >  	s32 value;
> > @@ -1581,6 +1683,9 @@ int uvc_ctrl_set(struct uvc_video_chain *chain,
> >  	mapping->set(mapping, value,
> >  		uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT));
> > 
> > +	if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS)
> > +		ctrl->handle = handle;
> > +
> >  	ctrl->dirty = 1;
> >  	ctrl->modified = 1;
> >  	return 0;
> > @@ -1612,7 +1717,9 @@ static int uvc_ctrl_get_flags(struct uvc_device *dev,
> > 
> >  			    |  (data[0] & UVC_CONTROL_CAP_SET ?
> > 
> >  				UVC_CTRL_FLAG_SET_CUR : 0)
> > 
> >  			    |  (data[0] & UVC_CONTROL_CAP_AUTOUPDATE ?
> > 
> > -				UVC_CTRL_FLAG_AUTO_UPDATE : 0);
> > +				UVC_CTRL_FLAG_AUTO_UPDATE : 0)
> > +			    |  (data[0] & UVC_CONTROL_CAP_ASYNCHRONOUS ?
> > +				UVC_CTRL_FLAG_ASYNCHRONOUS : 0);
> > 
> >  	kfree(data);
> >  	return ret;
> > @@ -2173,6 +2280,8 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
> >  	struct uvc_entity *entity;
> >  	unsigned int i;
> > 
> > +	INIT_WORK(&dev->async_ctrl.work, uvc_ctrl_status_event_work);
> > +
> >  	/* Walk the entities list and instantiate controls */
> >  	list_for_each_entry(entity, &dev->entities, list) {
> >  		struct uvc_control *ctrl;
> > @@ -2241,6 +2350,8 @@ void uvc_ctrl_cleanup_device(struct uvc_device *dev)
> >  	struct uvc_entity *entity;
> >  	unsigned int i;
> > 
> > +	cancel_work_sync(&dev->async_ctrl.work);
> > +
> >  	/* Free controls and control mappings for all entities. */
> >  	list_for_each_entry(entity, &dev->entities, list) {
> >  		for (i = 0; i < entity->ncontrols; ++i) {
> > diff --git a/drivers/media/usb/uvc/uvc_status.c
> > b/drivers/media/usb/uvc/uvc_status.c index 7b71041..0722dc6 100644
> > --- a/drivers/media/usb/uvc/uvc_status.c
> > +++ b/drivers/media/usb/uvc/uvc_status.c
> > @@ -78,7 +78,24 @@ static void uvc_input_report_key(struct uvc_device *dev,
> > unsigned int code, /*
> > --------------------------------------------------------------------------
> > * Status interrupt endpoint
> >   */
> > -static void uvc_event_streaming(struct uvc_device *dev, u8 *data, int len)
> > +struct uvc_streaming_status {
> > +	u8	bStatusType;
> > +	u8	bOriginator;
> > +	u8	bEvent;
> > +	u8	bValue[];
> > +} __packed;
> > +
> > +struct uvc_control_status {
> > +	u8	bStatusType;
> > +	u8	bOriginator;
> > +	u8	bEvent;
> > +	u8	bSelector;
> > +	u8	bAttribute;
> > +	u8	bValue[];
> > +} __packed;
> > +
> > +static void uvc_event_streaming(struct uvc_device *dev,
> > +				struct uvc_streaming_status *status, int len)
> >  {
> >  	if (len < 3) {
> >  		uvc_trace(UVC_TRACE_STATUS, "Invalid streaming status event "
> > @@ -86,31 +103,97 @@ static void uvc_event_streaming(struct uvc_device *dev,
> > u8 *data, int len) return;
> >  	}
> > 
> > -	if (data[2] == 0) {
> > +	if (status->bEvent == 0) {
> >  		if (len < 4)
> >  			return;
> >  		uvc_trace(UVC_TRACE_STATUS, "Button (intf %u) %s len %d\n",
> > -			data[1], data[3] ? "pressed" : "released", len);
> > -		uvc_input_report_key(dev, KEY_CAMERA, data[3]);
> > +			  status->bOriginator,
> > +			  status->bValue[0] ? "pressed" : "released", len);
> > +		uvc_input_report_key(dev, KEY_CAMERA, status->bValue[0]);
> >  	} else {
> >  		uvc_trace(UVC_TRACE_STATUS,
> >  			  "Stream %u error event %02x len %d.\n",
> > -			  data[1], data[2], len);
> > +			  status->bOriginator, status->bEvent, len);
> >  	}
> >  }
> > 
> > -static void uvc_event_control(struct uvc_device *dev, u8 *data, int len)
> > +#define UVC_CTRL_VALUE_CHANGE	0
> > +#define UVC_CTRL_INFO_CHANGE	1
> > +#define UVC_CTRL_FAILURE_CHANGE	2
> > +#define UVC_CTRL_MIN_CHANGE	3
> > +#define UVC_CTRL_MAX_CHANGE	4
> > +
> > +static struct uvc_control *uvc_event_entity_find_ctrl(struct uvc_entity
> > *entity, +						      u8 selector)
> >  {
> > -	char *attrs[3] = { "value", "info", "failure" };
> > +	struct uvc_control *ctrl;
> > +	unsigned int i;
> > +
> > +	for (i = 0, ctrl = entity->controls; i < entity->ncontrols; i++, ctrl++)
> > +		if (ctrl->info.selector == selector)
> > +			return ctrl;
> > +
> > +	return NULL;
> > +}
> > 
> > -	if (len < 6 || data[2] != 0 || data[4] > 2) {
> > +static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
> > +					const struct uvc_control_status *status,
> > +					struct uvc_video_chain **chain)
> > +{
> > +	list_for_each_entry((*chain), &dev->chains, list) {
> > +		struct uvc_entity *entity;
> > +		struct uvc_control *ctrl;
> > +
> > +		list_for_each_entry(entity, &(*chain)->entities, chain) {
> > +			if (entity->id != status->bOriginator)
> > +				continue;
> > +
> > +			ctrl = uvc_event_entity_find_ctrl(entity,
> > +							  status->bSelector);
> > +			if (ctrl)
> > +				return ctrl;
> > +		}
> > +	}
> > +
> > +	return NULL;
> > +}
> > +
> > +static bool uvc_event_control(struct urb *urb,
> > +			      const struct uvc_control_status *status, int len)
> > +{
> > +	static const char *attrs[] = { "value", "info", "failure", "min", "max" };
> > +	struct uvc_device *dev = urb->context;
> > +	struct uvc_video_chain *chain;
> > +	struct uvc_control *ctrl;
> > +
> > +	if (len < 6 || status->bEvent != 0 ||
> > +	    status->bAttribute >= ARRAY_SIZE(attrs)) {
> >  		uvc_trace(UVC_TRACE_STATUS, "Invalid control status event "
> >  				"received.\n");
> > -		return;
> > +		return false;
> >  	}
> > 
> >  	uvc_trace(UVC_TRACE_STATUS, "Control %u/%u %s change len %d.\n",
> > -		data[1], data[3], attrs[data[4]], len);
> > +		  status->bOriginator, status->bSelector,
> > +		  attrs[status->bAttribute], len);
> > +
> > +	/* Find the control. */
> > +	ctrl = uvc_event_find_ctrl(dev, status, &chain);
> > +	if (!ctrl)
> > +		return false;
> > +
> > +	switch (status->bAttribute) {
> > +	case UVC_CTRL_VALUE_CHANGE:
> > +		return uvc_ctrl_status_event(urb, chain, ctrl, status->bValue);
> > +
> > +	case UVC_CTRL_INFO_CHANGE:
> > +	case UVC_CTRL_FAILURE_CHANGE:
> > +	case UVC_CTRL_MIN_CHANGE:
> > +	case UVC_CTRL_MAX_CHANGE:
> > +		break;
> > +	}
> > +
> > +	return false;
> >  }
> > 
> >  static void uvc_status_complete(struct urb *urb)
> > @@ -138,13 +221,23 @@ static void uvc_status_complete(struct urb *urb)
> >  	len = urb->actual_length;
> >  	if (len > 0) {
> >  		switch (dev->status[0] & 0x0f) {
> > -		case UVC_STATUS_TYPE_CONTROL:
> > -			uvc_event_control(dev, dev->status, len);
> > +		case UVC_STATUS_TYPE_CONTROL: {
> > +			struct uvc_control_status *status =
> > +				(struct uvc_control_status *)dev->status;
> > +
> > +			if (uvc_event_control(urb, status, len))
> > +				/* The URB will be resubmitted in work context. */
> > +				return;
> >  			break;
> > +		}
> > 
> > -		case UVC_STATUS_TYPE_STREAMING:
> > -			uvc_event_streaming(dev, dev->status, len);
> > +		case UVC_STATUS_TYPE_STREAMING: {
> > +			struct uvc_streaming_status *status =
> > +				(struct uvc_streaming_status *)dev->status;
> > +
> > +			uvc_event_streaming(dev, status, len);
> >  			break;
> > +		}
> > 
> >  		default:
> >  			uvc_trace(UVC_TRACE_STATUS, "Unknown status event "
> > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c
> > b/drivers/media/usb/uvc/uvc_v4l2.c index bd32914..18a7384 100644
> > --- a/drivers/media/usb/uvc/uvc_v4l2.c
> > +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> > @@ -994,7 +994,7 @@ static int uvc_ioctl_s_ctrl(struct file *file, void *fh,
> > if (ret < 0)
> >  		return ret;
> > 
> > -	ret = uvc_ctrl_set(chain, &xctrl);
> > +	ret = uvc_ctrl_set(handle, &xctrl);
> >  	if (ret < 0) {
> >  		uvc_ctrl_rollback(handle);
> >  		return ret;
> > @@ -1069,7 +1069,7 @@ static int uvc_ioctl_s_try_ext_ctrls(struct uvc_fh
> > *handle, return ret;
> > 
> >  	for (i = 0; i < ctrls->count; ++ctrl, ++i) {
> > -		ret = uvc_ctrl_set(chain, ctrl);
> > +		ret = uvc_ctrl_set(handle, ctrl);
> >  		if (ret < 0) {
> >  			uvc_ctrl_rollback(handle);
> >  			ctrls->error_idx = commit ? ctrls->count : i;
> > diff --git a/drivers/media/usb/uvc/uvcvideo.h
> > b/drivers/media/usb/uvc/uvcvideo.h index be5cf17..8ae03a4 100644
> > --- a/drivers/media/usb/uvc/uvcvideo.h
> > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > @@ -12,6 +12,7 @@
> >  #include <linux/usb/video.h>
> >  #include <linux/uvcvideo.h>
> >  #include <linux/videodev2.h>
> > +#include <linux/workqueue.h>
> >  #include <media/media-device.h>
> >  #include <media/v4l2-device.h>
> >  #include <media/v4l2-event.h>
> > @@ -256,6 +257,8 @@ struct uvc_control {
> >  	   initialized:1;
> > 
> >  	u8 *uvc_data;
> > +
> > +	struct uvc_fh *handle;	/* File handle that last changed the control. */
> >  };
> > 
> >  struct uvc_format_desc {
> > @@ -600,6 +603,14 @@ struct uvc_device {
> >  	u8 *status;
> >  	struct input_dev *input;
> >  	char input_phys[64];
> > +
> > +	struct uvc_ctrl_work {
> > +		struct work_struct work;
> > +		struct urb *urb;
> > +		struct uvc_video_chain *chain;
> > +		struct uvc_control *ctrl;
> > +		const void *data;
> > +	} async_ctrl;
> >  };
> > 
> >  enum uvc_handle_state {
> > @@ -753,6 +764,8 @@ int uvc_ctrl_add_mapping(struct uvc_video_chain *chain,
> >  int uvc_ctrl_init_device(struct uvc_device *dev);
> >  void uvc_ctrl_cleanup_device(struct uvc_device *dev);
> >  int uvc_ctrl_restore_values(struct uvc_device *dev);
> > +bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,
> > +			   struct uvc_control *ctrl, const u8 *data);
> > 
> >  int uvc_ctrl_begin(struct uvc_video_chain *chain);
> >  int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback,
> > @@ -770,7 +783,7 @@ static inline int uvc_ctrl_rollback(struct uvc_fh
> > *handle) }
> > 
> >  int uvc_ctrl_get(struct uvc_video_chain *chain, struct v4l2_ext_control
> > *xctrl); -int uvc_ctrl_set(struct uvc_video_chain *chain, struct
> > v4l2_ext_control *xctrl); +int uvc_ctrl_set(struct uvc_fh *handle, struct
> > v4l2_ext_control *xctrl);
> > 
> >  int uvc_xu_ctrl_query(struct uvc_video_chain *chain,
> >  		      struct uvc_xu_control_query *xqry);
> > diff --git a/include/uapi/linux/uvcvideo.h b/include/uapi/linux/uvcvideo.h
> > index 020714d..f80f05b 100644
> > --- a/include/uapi/linux/uvcvideo.h
> > +++ b/include/uapi/linux/uvcvideo.h
> > @@ -28,6 +28,8 @@
> >  #define UVC_CTRL_FLAG_RESTORE		(1 << 6)
> >  /* Control can be updated by the camera. */
> >  #define UVC_CTRL_FLAG_AUTO_UPDATE	(1 << 7)
> > +/* Control supports asynchronous reporting */
> > +#define UVC_CTRL_FLAG_ASYNCHRONOUS	(1 << 8)
> > 
> >  #define UVC_CTRL_FLAG_GET_RANGE \
> >  	(UVC_CTRL_FLAG_GET_CUR | UVC_CTRL_FLAG_GET_MIN | \
> 
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 

Re: [PATCH v8 2/3] uvcvideo: send a control event when a Control Change interrupt arrives

[Laurent Pinchart]

Hi Guennadi,

On Tuesday, 8 May 2018 18:07:43 EEST Guennadi Liakhovetski wrote:
> UVC defines a method of handling asynchronous controls, which sends a
> USB packet over the interrupt pipe. This patch implements support for
> such packets by sending a control event to the user. Since this can
> involve USB traffic and, therefore, scheduling, this has to be done
> in a work queue.
> 
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> ---
> 
> v8:
> 
> * avoid losing events by delaying the status URB resubmission until
>   after completion of the current event
> * extract control value calculation into __uvc_ctrl_get_value()
> * do not proactively return EBUSY if the previous control hasn't
>   completed yet, let the camera handle such cases
> * multiple cosmetic changes
> 
>  drivers/media/usb/uvc/uvc_ctrl.c   | 166 ++++++++++++++++++++++++++++------
>  drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++---
>  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
>  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
>  include/uapi/linux/uvcvideo.h      |   2 +
>  5 files changed, 255 insertions(+), 44 deletions(-)

As mentioned in a previous e-mail, here's my review of small issues in the
form of diff on top of this patch. I'll reply inline with comments.

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index d57a176a03d5..04d779e3f039 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -1225,38 +1225,41 @@ static void uvc_ctrl_fill_event(struct uvc_video_chain *chain,
 	ev->u.ctrl.default_value = v4l2_ctrl.default_value;
 }
 
-static void uvc_ctrl_send_event(struct uvc_fh *handle,
-	struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
-	s32 value, u32 changes)
+/*
+ * Send control change events to all subscribers for the @ctrl control. By
+ * default the subscriber that generated the event, as identified by @handle,
+ * is not notified unless it has set the V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK flag.
+ * @handle can be NULL for asynchronous events related to auto-update controls,
+ * in which case all subscribers are notified.
+ */
+static void uvc_ctrl_send_event(struct uvc_video_chain *chain,
+	struct uvc_fh *handle, struct uvc_control *ctrl,
+	struct uvc_control_mapping *mapping, s32 value, u32 changes)
 {
+	struct v4l2_fh *originator = handle ? &handle->vfh : NULL;
 	struct v4l2_subscribed_event *sev;
 	struct v4l2_event ev;
-	bool autoupdate;
 
 	if (list_empty(&mapping->ev_subs))
 		return;
 
-	if (!handle) {
-		autoupdate = true;
-		sev = list_first_entry(&mapping->ev_subs,
-				       struct v4l2_subscribed_event, node);
-		handle = container_of(sev->fh, struct uvc_fh, vfh);
-	} else {
-		autoupdate = false;
-	}
-
-	uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, value, changes);
+	uvc_ctrl_fill_event(chain, &ev, ctrl, mapping, value, changes);
 
 	list_for_each_entry(sev, &mapping->ev_subs, node) {
-		if (sev->fh != &handle->vfh ||
+		if (sev->fh != originator ||
 		    (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK) ||
-		    (changes & V4L2_EVENT_CTRL_CH_FLAGS) || autoupdate)
+		    (changes & V4L2_EVENT_CTRL_CH_FLAGS))
 			v4l2_event_queue_fh(sev->fh, &ev);
 	}
 }
 
-static void __uvc_ctrl_send_slave_event(struct uvc_fh *handle,
-	struct uvc_video_chain *chain, struct uvc_control *master, u32 slave_id)
+/*
+ * Send control change events for the slave of the @master control identified
+ * by the V4L2 ID @slave_id. The @handle identifies the event subscriber that
+ * generated the event and may be NULL for auto-update events.
+ */
+static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
+	struct uvc_fh *handle, struct uvc_control *master, u32 slave_id)
 {
 	struct uvc_control_mapping *mapping = NULL;
 	struct uvc_control *ctrl = NULL;
@@ -1267,11 +1270,10 @@ static void __uvc_ctrl_send_slave_event(struct uvc_fh *handle,
 	if (ctrl == NULL)
 		return;
 
-	if (__uvc_ctrl_get(handle ? handle->chain : chain, ctrl, mapping,
-			   &val) == 0)
+	if (__uvc_ctrl_get(chain, ctrl, mapping, &val) == 0)
 		changes |= V4L2_EVENT_CTRL_CH_VALUE;
 
-	uvc_ctrl_send_event(handle, ctrl, mapping, val, changes);
+	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
 }
 
 static void uvc_ctrl_status_event_work(struct work_struct *work)
@@ -1279,38 +1281,37 @@ static void uvc_ctrl_status_event_work(struct work_struct *work)
 	struct uvc_device *dev = container_of(work, struct uvc_device,
 					      async_ctrl.work);
 	struct uvc_ctrl_work *w = &dev->async_ctrl;
+	struct uvc_video_chain *chain = w->chain;
 	struct uvc_control_mapping *mapping;
 	struct uvc_control *ctrl = w->ctrl;
 	unsigned int i;
 	int ret;
 
-	mutex_lock(&w->chain->ctrl_mutex);
+	mutex_lock(&chain->ctrl_mutex);
 
 	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
 		s32 value = __uvc_ctrl_get_value(mapping, w->data);
 
 		/*
-		 * So far none of the auto-update controls in the uvc_ctrls[]
-		 * table is mapped to a V4L control with slaves in the
-		 * uvc_ctrl_mappings[] list, so slave controls so far never have
-		 * handle == NULL, but this can change in the future
+		 * ctrl->handle may be NULL here if the device sends auto-update
+		 * events without a prior related control set from userspace.
 		 */
 		for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
 			if (!mapping->slave_ids[i])
 				break;
 
-			__uvc_ctrl_send_slave_event(ctrl->handle, w->chain,
-						ctrl, mapping->slave_ids[i]);
+			uvc_ctrl_send_slave_event(chain, ctrl->handle, ctrl,
+						  mapping->slave_ids[i]);
 		}
 
-		uvc_ctrl_send_event(ctrl->handle, ctrl, mapping, value,
+		uvc_ctrl_send_event(chain, ctrl->handle, ctrl, mapping, value,
 				    V4L2_EVENT_CTRL_CH_VALUE);
 	}
 
-	mutex_unlock(&w->chain->ctrl_mutex);
-
 	ctrl->handle = NULL;
 
+	mutex_unlock(&chain->ctrl_mutex);
+
 	/* Resubmit the URB. */
 	w->urb->interval = dev->int_ep->desc.bInterval;
 	ret = usb_submit_urb(w->urb, GFP_KERNEL);
@@ -1340,23 +1341,17 @@ bool uvc_ctrl_status_event(struct urb *urb, struct uvc_video_chain *chain,
 	return true;
 }
 
-static void uvc_ctrl_send_slave_event(struct uvc_fh *handle,
-	struct uvc_control *master, u32 slave_id,
-	const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
+static bool uvc_ctrl_xctrls_has_control(const struct v4l2_ext_control *xctrls,
+					unsigned int xctrls_count, u32 id)
 {
 	unsigned int i;
 
-	/*
-	 * We can skip sending an event for the slave if the slave
-	 * is being modified in the same transaction.
-	 */
-	for (i = 0; i < xctrls_count; i++) {
-		if (xctrls[i].id == slave_id)
-			return;
+	for (i = 0; i < xctrls_count; ++i) {
+		if (xctrls[i].id == id)
+			return true;
 	}
 
-	/* handle != NULL */
-	__uvc_ctrl_send_slave_event(handle, NULL, master, slave_id);
+	return false;
 }
 
 static void uvc_ctrl_send_events(struct uvc_fh *handle,
@@ -1376,28 +1371,34 @@ static void uvc_ctrl_send_events(struct uvc_fh *handle,
 			continue;
 
 		for (j = 0; j < ARRAY_SIZE(mapping->slave_ids); ++j) {
-			if (!mapping->slave_ids[j])
+			u32 slave_id = mapping->slave_ids[j];
+
+			if (!slave_id)
 				break;
-			uvc_ctrl_send_slave_event(handle, ctrl,
-						  mapping->slave_ids[j],
-						  xctrls, xctrls_count);
+
+			/*
+			 * We can skip sending an event for the slave if the
+			 * slave is being modified in the same transaction.
+			 */
+			if (uvc_ctrl_xctrls_has_control(xctrls, xctrls_count,
+							slave_id))
+				continue;
+
+			uvc_ctrl_send_slave_event(handle->chain, handle, ctrl,
+						  slave_id);
 		}
 
 		/*
 		 * If the master is being modified in the same transaction
 		 * flags may change too.
 		 */
-		if (mapping->master_id) {
-			for (j = 0; j < xctrls_count; j++) {
-				if (xctrls[j].id == mapping->master_id) {
-					changes |= V4L2_EVENT_CTRL_CH_FLAGS;
-					break;
-				}
-			}
-		}
+		if (mapping->master_id &&
+		    uvc_ctrl_xctrls_has_control(xctrls, xctrls_count,
+						mapping->master_id))
+			changes |= V4L2_EVENT_CTRL_CH_FLAGS;
 
-		uvc_ctrl_send_event(handle, ctrl, mapping, xctrls[i].value,
-				    changes);
+		uvc_ctrl_send_event(handle->chain, handle, ctrl, mapping,
+				    xctrls[i].value, changes);
 	}
 }
 
diff --git a/drivers/media/usb/uvc/uvc_status.c b/drivers/media/usb/uvc/uvc_status.c
index a0f2feaee7c6..0722dc684378 100644
--- a/drivers/media/usb/uvc/uvc_status.c
+++ b/drivers/media/usb/uvc/uvc_status.c
@@ -150,8 +150,7 @@ static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
 
 			ctrl = uvc_event_entity_find_ctrl(entity,
 							  status->bSelector);
-			if (ctrl && (!ctrl->handle ||
-				     ctrl->handle->chain == *chain))
+			if (ctrl)
 				return ctrl;
 		}
 	}
@@ -222,17 +221,23 @@ static void uvc_status_complete(struct urb *urb)
 	len = urb->actual_length;
 	if (len > 0) {
 		switch (dev->status[0] & 0x0f) {
-		case UVC_STATUS_TYPE_CONTROL:
-			if (uvc_event_control(urb,
-				(struct uvc_control_status *)dev->status, len))
-				/* The URB will be resubmitted in work context */
+		case UVC_STATUS_TYPE_CONTROL: {
+			struct uvc_control_status *status =
+				(struct uvc_control_status *)dev->status;
+
+			if (uvc_event_control(urb, status, len))
+				/* The URB will be resubmitted in work context. */
 				return;
 			break;
+		}
 
-		case UVC_STATUS_TYPE_STREAMING:
-			uvc_event_streaming(dev,
-				(struct uvc_streaming_status *)dev->status, len);
+		case UVC_STATUS_TYPE_STREAMING: {
+			struct uvc_streaming_status *status =
+				(struct uvc_streaming_status *)dev->status;
+
+			uvc_event_streaming(dev, status, len);
 			break;
+		}
 
 		default:
 			uvc_trace(UVC_TRACE_STATUS, "Unknown status event "
diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
index b36ad9eb8c80..e5f5d84f1d1d 100644
--- a/drivers/media/usb/uvc/uvcvideo.h
+++ b/drivers/media/usb/uvc/uvcvideo.h
@@ -261,7 +261,7 @@ struct uvc_control {
 
 	u8 *uvc_data;
 
-	struct uvc_fh *handle;	/* Used for asynchronous event delivery */
+	struct uvc_fh *handle;	/* File handle that last changed the control. */
 };
 
 struct uvc_format_desc {

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 2/3] uvcvideo: send a control event when a Control Change interrupt arrives

[Laurent Pinchart]

Hi Guennadi,

On Wednesday, 25 July 2018 20:25:16 EEST Laurent Pinchart wrote:
> On Tuesday, 8 May 2018 18:07:43 EEST Guennadi Liakhovetski wrote:
> > UVC defines a method of handling asynchronous controls, which sends a
> > USB packet over the interrupt pipe. This patch implements support for
> > such packets by sending a control event to the user. Since this can
> > involve USB traffic and, therefore, scheduling, this has to be done
> > in a work queue.
> > 
> > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> > ---
> > 
> > v8:
> > 
> > * avoid losing events by delaying the status URB resubmission until
> >   after completion of the current event
> > * extract control value calculation into __uvc_ctrl_get_value()
> > * do not proactively return EBUSY if the previous control hasn't
> >   completed yet, let the camera handle such cases
> > * multiple cosmetic changes
> > 
> >  drivers/media/usb/uvc/uvc_ctrl.c   | 166 ++++++++++++++++++++++++++------
> >  drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++---
> >  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
> >  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
> >  include/uapi/linux/uvcvideo.h      |   2 +
> >  5 files changed, 255 insertions(+), 44 deletions(-)
> 
> As mentioned in a previous e-mail, here's my review of small issues in the
> form of diff on top of this patch. I'll reply inline with comments.
> 
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c
> b/drivers/media/usb/uvc/uvc_ctrl.c index d57a176a03d5..04d779e3f039 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -1225,38 +1225,41 @@ static void uvc_ctrl_fill_event(struct
> uvc_video_chain *chain, ev->u.ctrl.default_value = v4l2_ctrl.default_value;
>  }
> 
> -static void uvc_ctrl_send_event(struct uvc_fh *handle,
> -	struct uvc_control *ctrl, struct uvc_control_mapping *mapping,
> -	s32 value, u32 changes)
> +/*
> + * Send control change events to all subscribers for the @ctrl control. By
> + * default the subscriber that generated the event, as identified by
> @handle,
> + * is not notified unless it has set the
> V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK flag.
> + * @handle can be NULL for asynchronous events related to auto-update
> controls,
> + * in which case all subscribers are notified.
> + */

As the number of functions related to event handling grew larger, I felt that 
adding a bit of documentation became necessary to avoid getting lost.

> +static void uvc_ctrl_send_event(struct uvc_video_chain *chain,
> +	struct uvc_fh *handle, struct uvc_control *ctrl,
> +	struct uvc_control_mapping *mapping, s32 value, u32 changes)
>  {
> +	struct v4l2_fh *originator = handle ? &handle->vfh : NULL;
>  	struct v4l2_subscribed_event *sev;
>  	struct v4l2_event ev;
> -	bool autoupdate;
> 
>  	if (list_empty(&mapping->ev_subs))
>  		return;
> 
> -	if (!handle) {
> -		autoupdate = true;
> -		sev = list_first_entry(&mapping->ev_subs,
> -				       struct v4l2_subscribed_event, node);
> -		handle = container_of(sev->fh, struct uvc_fh, vfh);
> -	} else {
> -		autoupdate = false;
> -	}
> -

By adding the chain pointer as a function argument, which is always available 
to the caller, we can remove the small hack that looks up the chain in the 
first handle if the handle is NULL.

> -	uvc_ctrl_fill_event(handle->chain, &ev, ctrl, mapping, value, changes);
> +	uvc_ctrl_fill_event(chain, &ev, ctrl, mapping, value, changes);
> 
>  	list_for_each_entry(sev, &mapping->ev_subs, node) {
> -		if (sev->fh != &handle->vfh ||
> +		if (sev->fh != originator ||
>  		    (sev->flags & V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK) ||
> -		    (changes & V4L2_EVENT_CTRL_CH_FLAGS) || autoupdate)
> +		    (changes & V4L2_EVENT_CTRL_CH_FLAGS))
>  			v4l2_event_queue_fh(sev->fh, &ev);
>  	}
>  }
> 
> -static void __uvc_ctrl_send_slave_event(struct uvc_fh *handle,
> -	struct uvc_video_chain *chain, struct uvc_control *master, u32 slave_id)
> +/*
> + * Send control change events for the slave of the @master control
> identified
> + * by the V4L2 ID @slave_id. The @handle identifies the event subscriber
> that
> + * generated the event and may be NULL for auto-update
> events.
> + */
> +static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain,
> +	struct uvc_fh *handle, struct uvc_control *master, u32 slave_id)

Passing the chain as the first argument matches the rest of the functions in 
this file (more or less). I also removed the starting double underscore, by 
removing the uvc_ctrl_send_slave_event() function below.

>  {
>  	struct uvc_control_mapping *mapping = NULL;
>  	struct uvc_control *ctrl = NULL;
> @@ -1267,11 +1270,10 @@ static void __uvc_ctrl_send_slave_event(struct
> uvc_fh *handle, if (ctrl == NULL)
>  		return;
> 
> -	if (__uvc_ctrl_get(handle ? handle->chain : chain, ctrl, mapping,
> -			   &val) == 0)
> +	if (__uvc_ctrl_get(chain, ctrl, mapping, &val) == 0)

If handle isn't NULL, handle->chain == chain, so we can use chain directly.

>  		changes |= V4L2_EVENT_CTRL_CH_VALUE;
> 
> -	uvc_ctrl_send_event(handle, ctrl, mapping, val, changes);
> +	uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes);
>  }
> 
>  static void uvc_ctrl_status_event_work(struct work_struct *work)
> @@ -1279,38 +1281,37 @@ static void uvc_ctrl_status_event_work(struct
> work_struct *work) struct uvc_device *dev = container_of(work, struct
> uvc_device,
>  					      async_ctrl.work);
>  	struct uvc_ctrl_work *w = &dev->async_ctrl;
> +	struct uvc_video_chain *chain = w->chain;

Just to reduce line lengths below.

>  	struct uvc_control_mapping *mapping;
>  	struct uvc_control *ctrl = w->ctrl;
>  	unsigned int i;
>  	int ret;
> 
> -	mutex_lock(&w->chain->ctrl_mutex);
> +	mutex_lock(&chain->ctrl_mutex);
> 
>  	list_for_each_entry(mapping, &ctrl->info.mappings, list) {
>  		s32 value = __uvc_ctrl_get_value(mapping, w->data);
> 
>  		/*
> -		 * So far none of the auto-update controls in the uvc_ctrls[]
> -		 * table is mapped to a V4L control with slaves in the
> -		 * uvc_ctrl_mappings[] list, so slave controls so far never have
> -		 * handle == NULL, but this can change in the future
> +		 * ctrl->handle may be NULL here if the device sends auto-update
> +		 * events without a prior related control set from userspace.

I don't see how having handle == NULL for a slave control would be a problem, 
so I've replaced the comment. Please let me know if that was a 
misunderstanding on my side.

>  		 */
>  		for (i = 0; i < ARRAY_SIZE(mapping->slave_ids); ++i) {
>  			if (!mapping->slave_ids[i])
>  				break;
> 
> -			__uvc_ctrl_send_slave_event(ctrl->handle, w->chain,
> -						ctrl, mapping->slave_ids[i]);
> +			uvc_ctrl_send_slave_event(chain, ctrl->handle, ctrl,
> +						  mapping->slave_ids[i]);
>  		}
> 
> -		uvc_ctrl_send_event(ctrl->handle, ctrl, mapping, value,
> +		uvc_ctrl_send_event(chain, ctrl->handle, ctrl, mapping, value,
>  				    V4L2_EVENT_CTRL_CH_VALUE);
>  	}
> 
> -	mutex_unlock(&w->chain->ctrl_mutex);
> -
>  	ctrl->handle = NULL;
> 
> +	mutex_unlock(&chain->ctrl_mutex);
> +

As per the locking and race condition discussion.

>  	/* Resubmit the URB. */
>  	w->urb->interval = dev->int_ep->desc.bInterval;
>  	ret = usb_submit_urb(w->urb, GFP_KERNEL);
> @@ -1340,23 +1341,17 @@ bool uvc_ctrl_status_event(struct urb *urb, struct
> uvc_video_chain *chain, return true;
>  }
> 
> -static void uvc_ctrl_send_slave_event(struct uvc_fh *handle,
> -	struct uvc_control *master, u32 slave_id,
> -	const struct v4l2_ext_control *xctrls, unsigned int xctrls_count)
> +static bool uvc_ctrl_xctrls_has_control(const struct v4l2_ext_control
> *xctrls,
> +					unsigned int xctrls_count, u32 id)
>  {
>  	unsigned int i;
> 
> -	/*
> -	 * We can skip sending an event for the slave if the slave
> -	 * is being modified in the same transaction.
> -	 */
> -	for (i = 0; i < xctrls_count; i++) {
> -		if (xctrls[i].id == slave_id)
> -			return;
> +	for (i = 0; i < xctrls_count; ++i) {
> +		if (xctrls[i].id == id)
> +			return true;
>  	}
> 
> -	/* handle != NULL */
> -	__uvc_ctrl_send_slave_event(handle, NULL, master, slave_id);
> +	return false;
>  }

We check twice below whether a control with a given ID is present in an xctrls 
list, so I've factored that out to a separate function. The 
uvc_ctrl_send_slave_event() function is now inlined in its single caller below 
with the help of the new uvc_ctrl_xctrls_has_control() function.

>  static void uvc_ctrl_send_events(struct uvc_fh *handle,
> @@ -1376,28 +1371,34 @@ static void uvc_ctrl_send_events(struct uvc_fh
> *handle, continue;
> 
>  		for (j = 0; j < ARRAY_SIZE(mapping->slave_ids); ++j) {
> -			if (!mapping->slave_ids[j])
> +			u32 slave_id = mapping->slave_ids[j];

To reduce line lengths.

> +
> +			if (!slave_id)
>  				break;
> -			uvc_ctrl_send_slave_event(handle, ctrl,
> -						  mapping->slave_ids[j],
> -						  xctrls, xctrls_count);
> +
> +			/*
> +			 * We can skip sending an event for the slave if the
> +			 * slave is being modified in the same transaction.
> +			 */
> +			if (uvc_ctrl_xctrls_has_control(xctrls, xctrls_count,
> +							slave_id))
> +				continue;
> +
> +			uvc_ctrl_send_slave_event(handle->chain, handle, ctrl,
> +						  slave_id);
>  		}
> 
>  		/*
>  		 * If the master is being modified in the same transaction
>  		 * flags may change too.
>  		 */
> -		if (mapping->master_id) {
> -			for (j = 0; j < xctrls_count; j++) {
> -				if (xctrls[j].id == mapping->master_id) {
> -					changes |= V4L2_EVENT_CTRL_CH_FLAGS;
> -					break;
> -				}
> -			}
> -		}
> +		if (mapping->master_id &&
> +		    uvc_ctrl_xctrls_has_control(xctrls, xctrls_count,
> +						mapping->master_id))
> +			changes |= V4L2_EVENT_CTRL_CH_FLAGS;
> 
> -		uvc_ctrl_send_event(handle, ctrl, mapping, xctrls[i].value,
> -				    changes);
> +		uvc_ctrl_send_event(handle->chain, handle, ctrl, mapping,
> +				    xctrls[i].value, changes);
>  	}
>  }
> 
> diff --git a/drivers/media/usb/uvc/uvc_status.c
> b/drivers/media/usb/uvc/uvc_status.c index a0f2feaee7c6..0722dc684378
> 100644
> --- a/drivers/media/usb/uvc/uvc_status.c
> +++ b/drivers/media/usb/uvc/uvc_status.c
> @@ -150,8 +150,7 @@ static struct uvc_control *uvc_event_find_ctrl(struct
> uvc_device *dev,
> 
>  			ctrl = uvc_event_entity_find_ctrl(entity,
>  							  status->bSelector);
> -			if (ctrl && (!ctrl->handle ||
> -				     ctrl->handle->chain == *chain))
> +			if (ctrl)

As per our discussion on this topic.

>  				return ctrl;
>  		}
>  	}
> @@ -222,17 +221,23 @@ static void uvc_status_complete(struct urb *urb)
>  	len = urb->actual_length;
>  	if (len > 0) {
>  		switch (dev->status[0] & 0x0f) {
> -		case UVC_STATUS_TYPE_CONTROL:
> -			if (uvc_event_control(urb,
> -				(struct uvc_control_status *)dev->status, len))
> -				/* The URB will be resubmitted in work context */
> +		case UVC_STATUS_TYPE_CONTROL: {
> +			struct uvc_control_status *status =
> +				(struct uvc_control_status *)dev->status;

To reduce line length in the function call. I find this a bit more readable, 
but I agree it is debatable.

> +
> +			if (uvc_event_control(urb, status, len))
> +				/* The URB will be resubmitted in work context. */
>  				return;
>  			break;
> +		}
> 
> -		case UVC_STATUS_TYPE_STREAMING:
> -			uvc_event_streaming(dev,
> -				(struct uvc_streaming_status *)dev->status, len);
> +		case UVC_STATUS_TYPE_STREAMING: {
> +			struct uvc_streaming_status *status =
> +				(struct uvc_streaming_status *)dev->status;
> +
> +			uvc_event_streaming(dev, status, len);
>  			break;
> +		}
> 
>  		default:
>  			uvc_trace(UVC_TRACE_STATUS, "Unknown status event "
> diff --git a/drivers/media/usb/uvc/uvcvideo.h
> b/drivers/media/usb/uvc/uvcvideo.h index b36ad9eb8c80..e5f5d84f1d1d 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -261,7 +261,7 @@ struct uvc_control {
> 
>  	u8 *uvc_data;
> 
> -	struct uvc_fh *handle;	/* Used for asynchronous event delivery */
> +	struct uvc_fh *handle;	/* File handle that last changed the control. */

I thought it would be more useful to document what the field stores instead of 
where it is used.

>  };
> 
>  struct uvc_format_desc {

-- 
Regards,

Laurent Pinchart

[PATCH v8 3/3] uvcvideo: handle control pipe protocol STALLs

[Guennadi Liakhovetski]

When a command ends up in a STALL on the control pipe, use the Request
Error Code control to provide a more precise error information to the
user. For example, if a camera is still busy processing a control,
when the same or an interrelated control set request arrives, the
camera can react with a STALL and then return the "Not ready" status
in response to a UVC_VC_REQUEST_ERROR_CODE_CONTROL command. With this
patch the user would then get an EBUSY error code instead of a
generic EPIPE.

Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
---

v8:

* restrict UVC_VC_REQUEST_ERROR_CODE_CONTROL to the control interface
* fix the wIndex value
* improve error codes
* cosmetic changes

 drivers/media/usb/uvc/uvc_video.c | 52 ++++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index aa0082f..ce65cd6 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -73,17 +73,57 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8 unit,
 			u8 intfnum, u8 cs, void *data, u16 size)
 {
 	int ret;
+	u8 error;
+	u8 tmp;
 
 	ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
 				UVC_CTRL_CONTROL_TIMEOUT);
-	if (ret != size) {
-		uvc_printk(KERN_ERR, "Failed to query (%s) UVC control %u on "
-			"unit %u: %d (exp. %u).\n", uvc_query_name(query), cs,
-			unit, ret, size);
-		return -EIO;
+	if (likely(ret == size))
+		return 0;
+
+	uvc_printk(KERN_ERR,
+		   "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
+		   uvc_query_name(query), cs, unit, ret, size);
+
+	if (ret != -EPIPE)
+		return ret;
+
+	tmp = *(u8 *)data;
+
+	ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum,
+			       UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1,
+			       UVC_CTRL_CONTROL_TIMEOUT);
+
+	error = *(u8 *)data;
+	*(u8 *)data = tmp;
+
+	if (ret != 1)
+		return ret < 0 ? ret : -EPIPE;
+
+	uvc_trace(UVC_TRACE_CONTROL, "Control error %u\n", error);
+
+	switch (error) {
+	case 0:
+		/* Cannot happen - we received a STALL */
+		return -EPIPE;
+	case 1: /* Not ready */
+		return -EBUSY;
+	case 2: /* Wrong state */
+		return -EILSEQ;
+	case 3: /* Power */
+		return -EREMOTE;
+	case 4: /* Out of range */
+		return -ERANGE;
+	case 5: /* Invalid unit */
+	case 6: /* Invalid control */
+	case 7: /* Invalid Request */
+	case 8: /* Invalid value within range */
+		return -EINVAL;
+	default: /* reserved or unknown */
+		break;
 	}
 
-	return 0;
+	return -EPIPE;
 }
 
 static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,
-- 
1.9.3

Re: [PATCH v8 3/3] uvcvideo: handle control pipe protocol STALLs

[Laurent Pinchart]

Hi Guennadi,

Thank you for the patch.

On Tuesday, 8 May 2018 18:07:44 EEST Guennadi Liakhovetski wrote:
> When a command ends up in a STALL on the control pipe, use the Request
> Error Code control to provide a more precise error information to the
> user. For example, if a camera is still busy processing a control,
> when the same or an interrelated control set request arrives, the
> camera can react with a STALL and then return the "Not ready" status
> in response to a UVC_VC_REQUEST_ERROR_CODE_CONTROL command. With this
> patch the user would then get an EBUSY error code instead of a
> generic EPIPE.
> 
> Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> 
> v8:
> 
> * restrict UVC_VC_REQUEST_ERROR_CODE_CONTROL to the control interface
> * fix the wIndex value
> * improve error codes
> * cosmetic changes
> 
>  drivers/media/usb/uvc/uvc_video.c | 52 +++++++++++++++++++++++++++++++-----
>  1 file changed, 46 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index aa0082f..ce65cd6 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -73,17 +73,57 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query, u8
> unit, u8 intfnum, u8 cs, void *data, u16 size)
>  {
>  	int ret;
> +	u8 error;
> +	u8 tmp;
> 
>  	ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
>  				UVC_CTRL_CONTROL_TIMEOUT);
> -	if (ret != size) {
> -		uvc_printk(KERN_ERR, "Failed to query (%s) UVC control %u on "
> -			"unit %u: %d (exp. %u).\n", uvc_query_name(query), cs,
> -			unit, ret, size);
> -		return -EIO;
> +	if (likely(ret == size))
> +		return 0;
> +
> +	uvc_printk(KERN_ERR,
> +		   "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).\n",
> +		   uvc_query_name(query), cs, unit, ret, size);
> +
> +	if (ret != -EPIPE)
> +		return ret;
> +
> +	tmp = *(u8 *)data;
> +
> +	ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum,
> +			       UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1,
> +			       UVC_CTRL_CONTROL_TIMEOUT);
> +
> +	error = *(u8 *)data;
> +	*(u8 *)data = tmp;
> +
> +	if (ret != 1)
> +		return ret < 0 ? ret : -EPIPE;
> +
> +	uvc_trace(UVC_TRACE_CONTROL, "Control error %u\n", error);
> +
> +	switch (error) {
> +	case 0:
> +		/* Cannot happen - we received a STALL */
> +		return -EPIPE;
> +	case 1: /* Not ready */
> +		return -EBUSY;
> +	case 2: /* Wrong state */
> +		return -EILSEQ;
> +	case 3: /* Power */
> +		return -EREMOTE;
> +	case 4: /* Out of range */
> +		return -ERANGE;
> +	case 5: /* Invalid unit */
> +	case 6: /* Invalid control */
> +	case 7: /* Invalid Request */
> +	case 8: /* Invalid value within range */
> +		return -EINVAL;
> +	default: /* reserved or unknown */
> +		break;
>  	}
> 
> -	return 0;
> +	return -EPIPE;
>  }
> 
>  static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,

-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 3/3] uvcvideo: handle control pipe protocol STALLs

[Laurent Pinchart]

Hi Guennadi,

On Tuesday, 17 July 2018 23:58:03 EEST Laurent Pinchart wrote:
> On Tuesday, 8 May 2018 18:07:44 EEST Guennadi Liakhovetski wrote:
> > When a command ends up in a STALL on the control pipe, use the Request
> > Error Code control to provide a more precise error information to the
> > user. For example, if a camera is still busy processing a control,
> > when the same or an interrelated control set request arrives, the
> > camera can react with a STALL and then return the "Not ready" status
> > in response to a UVC_VC_REQUEST_ERROR_CODE_CONTROL command. With this
> > patch the user would then get an EBUSY error code instead of a
> > generic EPIPE.
> > 
> > Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@intel.com>
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Can I push this patch already before 2/3 is ready ?

> > ---
> > 
> > v8:
> > 
> > * restrict UVC_VC_REQUEST_ERROR_CODE_CONTROL to the control interface
> > * fix the wIndex value
> > * improve error codes
> > * cosmetic changes
> > 
> >  drivers/media/usb/uvc/uvc_video.c | 52 +++++++++++++++++++++++++++++-----
> >  1 file changed, 46 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/media/usb/uvc/uvc_video.c
> > b/drivers/media/usb/uvc/uvc_video.c index aa0082f..ce65cd6 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -73,17 +73,57 @@ int uvc_query_ctrl(struct uvc_device *dev, u8 query,
> > u8
> > unit, u8 intfnum, u8 cs, void *data, u16 size)
> > 
> >  {
> >  
> >  	int ret;
> > 
> > +	u8 error;
> > +	u8 tmp;
> > 
> >  	ret = __uvc_query_ctrl(dev, query, unit, intfnum, cs, data, size,
> >  	
> >  				UVC_CTRL_CONTROL_TIMEOUT);
> > 
> > -	if (ret != size) {
> > -		uvc_printk(KERN_ERR, "Failed to query (%s) UVC control %u on "
> > -			"unit %u: %d (exp. %u).\n", uvc_query_name(query), cs,
> > -			unit, ret, size);
> > -		return -EIO;
> > +	if (likely(ret == size))
> > +		return 0;
> > +
> > +	uvc_printk(KERN_ERR,
> > +		   "Failed to query (%s) UVC control %u on unit %u: %d (exp. %u).
\n",
> > +		   uvc_query_name(query), cs, unit, ret, size);
> > +
> > +	if (ret != -EPIPE)
> > +		return ret;
> > +
> > +	tmp = *(u8 *)data;
> > +
> > +	ret = __uvc_query_ctrl(dev, UVC_GET_CUR, 0, intfnum,
> > +			       UVC_VC_REQUEST_ERROR_CODE_CONTROL, data, 1,
> > +			       UVC_CTRL_CONTROL_TIMEOUT);
> > +
> > +	error = *(u8 *)data;
> > +	*(u8 *)data = tmp;
> > +
> > +	if (ret != 1)
> > +		return ret < 0 ? ret : -EPIPE;
> > +
> > +	uvc_trace(UVC_TRACE_CONTROL, "Control error %u\n", error);
> > +
> > +	switch (error) {
> > +	case 0:
> > +		/* Cannot happen - we received a STALL */
> > +		return -EPIPE;
> > +	case 1: /* Not ready */
> > +		return -EBUSY;
> > +	case 2: /* Wrong state */
> > +		return -EILSEQ;
> > +	case 3: /* Power */
> > +		return -EREMOTE;
> > +	case 4: /* Out of range */
> > +		return -ERANGE;
> > +	case 5: /* Invalid unit */
> > +	case 6: /* Invalid control */
> > +	case 7: /* Invalid Request */
> > +	case 8: /* Invalid value within range */
> > +		return -EINVAL;
> > +	default: /* reserved or unknown */
> > +		break;
> > 
> >  	}
> > 
> > -	return 0;
> > +	return -EPIPE;
> > 
> >  }
> >  
> >  static void uvc_fixup_video_ctrl(struct uvc_streaming *stream,


-- 
Regards,

Laurent Pinchart

Re: [PATCH v8 0/3] uvcvideo: asynchronous controls

[Guennadi Liakhovetski]

Hi Laurent,

More than 3 weeks since v8 has been posted. Seems like we've missed 4.18. 
Could you please review them ASAP to make sure we merge them into 4.19?

Thanks
Guennadi

On Tue, 8 May 2018, Guennadi Liakhovetski wrote:

> Added a patch to remove a redundant check, addressed Laurent's
> comments.
> 
> Guennadi Liakhovetski (3):
>   uvcvideo: remove a redundant check
>   uvcvideo: send a control event when a Control Change interrupt arrives
>   uvcvideo: handle control pipe protocol STALLs
> 
>  drivers/media/usb/uvc/uvc_ctrl.c   | 168 ++++++++++++++++++++++++++++++-------
>  drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++---
>  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
>  drivers/media/usb/uvc/uvc_video.c  |  52 ++++++++++--
>  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
>  include/uapi/linux/uvcvideo.h      |   2 +
>  6 files changed, 302 insertions(+), 51 deletions(-)
> 
> -- 
> 1.9.3
> 
> Thanks
> Guennadi
> 

Re: [PATCH v8 0/3] uvcvideo: asynchronous controls

[Guennadi Liakhovetski]

Hi Laurent,

6.5 weeks and counting. Can we please schedule a review of these patches 
for the next week? Not much time is left to make it for 4.19.

Thanks
Guennadi

On Thu, 31 May 2018, Guennadi Liakhovetski wrote:

> Hi Laurent,
> 
> More than 3 weeks since v8 has been posted. Seems like we've missed 4.18. 
> Could you please review them ASAP to make sure we merge them into 4.19?
> 
> Thanks
> Guennadi
> 
> On Tue, 8 May 2018, Guennadi Liakhovetski wrote:
> 
> > Added a patch to remove a redundant check, addressed Laurent's
> > comments.
> > 
> > Guennadi Liakhovetski (3):
> >   uvcvideo: remove a redundant check
> >   uvcvideo: send a control event when a Control Change interrupt arrives
> >   uvcvideo: handle control pipe protocol STALLs
> > 
> >  drivers/media/usb/uvc/uvc_ctrl.c   | 168 ++++++++++++++++++++++++++++++-------
> >  drivers/media/usb/uvc/uvc_status.c | 112 ++++++++++++++++++++++---
> >  drivers/media/usb/uvc/uvc_v4l2.c   |   4 +-
> >  drivers/media/usb/uvc/uvc_video.c  |  52 ++++++++++--
> >  drivers/media/usb/uvc/uvcvideo.h   |  15 +++-
> >  include/uapi/linux/uvcvideo.h      |   2 +
> >  6 files changed, 302 insertions(+), 51 deletions(-)
> > 
> > -- 
> > 1.9.3
> > 
> > Thanks
> > Guennadi
> > 
>