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
> >>>>>>>> <[email protected]>
> >>>>>>>> ---
> >>>>>>>>
> >>>>>>>> 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
