Hi Guennadi,
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.
> >> + /* 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]
> >> diff --git a/drivers/media/usb/uvc/uvc_status.c
> >> b/drivers/media/usb/uvc/uvc_status.c index 7b71041..a0f2fea 100644
> >> --- a/drivers/media/usb/uvc/uvc_status.c
> >> +++ b/drivers/media/usb/uvc/uvc_status.c
[snip]
> >> +static struct uvc_control *uvc_event_find_ctrl(struct uvc_device *dev,
> >> + const struct uvc_control_status *status,
> >> + struct uvc_video_chain **chain)
> >> +{
> >> + list_for_each_entry((*chain), &dev->chains, list) {
> >> + struct uvc_entity *entity;
> >> + struct uvc_control *ctrl;
> >> +
> >> + list_for_each_entry(entity, &(*chain)->entities, chain) {
> >> + if (entity->id != status->bOriginator)
> >> + continue;
> >> +
> >> + ctrl = uvc_event_entity_find_ctrl(entity,
> >> + status->bSelector);
> >> + if (ctrl && (!ctrl->handle ||
> >> + ctrl->handle->chain == *chain))
> >
> > I'm afraid I still don't understand why you need the chain check :-(
> > Unless I'm mistaken, ctrl->handle is set in uvc_ctrl_set(), where the
> > control is looked up from the chain corresponding to handle->chain. How
> > can the check be false here ?
>
> I think you're right, the bOriginator check should be enough.
>
> > Those are my two major concerns. Apart from that I have other small
> > concerns that I propose addressing myself to avoid further delays. I've
> > been slow enough when it comes to reviewing this series, if we can clear
> > the two issues above, I'll handle the rest.
>
> Once I get your review of patch #3, I'll fix these two issues and
> resubmit, so you can also tell me your "minor concerns," since I'll be
> resubmitting anyway.
Do you mind if I send them as a diff on top of your patch ? I'll of course add
explanations where they are needed.
> >> + return ctrl;
> >> + }
> >> + }
> >> +
> >> + return NULL;
> >> +}
[snip]
--
Regards,
Laurent Pinchart
