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