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

[Guennadi Liakhovetski <g.liakhovetski@gmx.de>]

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