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