On Wed, Nov 27, 2024 at 10:25:48AM +0100, Ricardo Ribalda wrote:
> On Wed, 27 Nov 2024 at 10:12, Laurent Pinchart wrote:
> > On Wed, Nov 27, 2024 at 07:46:10AM +0000, Ricardo Ribalda wrote:
> > > If a file handle is waiting for a response from an async control, avoid
> > > that other file handle operate with it.
> > >
> > > Without this patch, the first file handle will never get the event
> > > associated to that operation.
> >
> > Please explain why that is an issue (both for the commit message and for
> > me, as I'm not sure what you're fixing here).
>
> What about something like this:
>
> Without this patch, the first file handle will never get the event
> associated with that operation, which can lead to endless loops in
> applications. Eg:
> If an application A wants to change the zoom and to know when the
> operation has completed:
> it will open the video node, subscribe to the zoom event, change the
> control and wait for zoom to finish.
> If before the zoom operation finishes, another application B changes
> the zoom, the first app A will loop forever.
So it's related to the userspace-visible behaviour, there are no issues
with this inside the kernel ?
Applications should in any case implement timeouts, as UVC devices are
fairly unreliable. What bothers me with this patch is that if the device
doesn't generate the event, ctrl->handle will never be reset to NULL,
and the control will never be settable again. I think the current
behaviour is a lesser evil.
> > What may be an issue is that ctrl->handle seem to be accessed from
> > different contexts without proper locking :-S
>
> Isn't it always protected by ctrl_mutex?
Not that I can tell. At least uvc_ctrl_status_event_async() isn't called
with that lock held. uvc_ctrl_set() seems OK (a lockedep assert at the
beginning of the function wouldn't hurt).
As uvc_ctrl_status_event_async() is the URB completion handler, a
spinlock would be nicer than a mutex to protect ctrl->handle.
> > > Cc: stable@xxxxxxxxxxxxxxx
> > > Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives")
> > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > > ---
> > > drivers/media/usb/uvc/uvc_ctrl.c | 4 ++++
> > > 1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 4fe26e82e3d1..5d3a28edf7f0 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -1950,6 +1950,10 @@ int uvc_ctrl_set(struct uvc_fh *handle,
> > > if (!(ctrl->info.flags & UVC_CTRL_FLAG_SET_CUR))
> > > return -EACCES;
> > >
> > > + /* Other file handle is waiting a response from this async control. */
> > > + if (ctrl->handle && ctrl->handle != handle)
> > > + return -EBUSY;
> > > +
> > > /* Clamp out of range values. */
> > > switch (mapping->v4l2_type) {
> > > case V4L2_CTRL_TYPE_INTEGER:
--
Regards,
Laurent Pinchart
