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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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