| Message ID | 20241127-uvc-fix-async-v1-1-eb8722531b8c@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Two fixes for async controls | expand |
Hi Ricardo, Thank you for the patch. 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 may be an issue is that ctrl->handle seem to be accessed from different contexts without proper locking :-S > Cc: stable@vger.kernel.org > Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > 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:
On Wed, 27 Nov 2024 at 10:12, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. > > 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. > > 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? > > > Cc: stable@vger.kernel.org > > Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > 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
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@vger.kernel.org > > > Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > 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:
On Wed, 27 Nov 2024 at 10:42, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > 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 ? It is also required by the granular PM patchset. > > 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. The same app can set the control as many times as it wants, and if that app closes the next patch will clean out the handle. Maybe I should invert the patches? > > > > 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). ctrl->handle = NULL in uvc_ctrl_status_event_async() is completely redundant. The only place we set the value of the handle is in uvc_ctrl_set, and that can only happen for controls with mappings. I am sending another patch to remove that set to make it clear. > > 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@vger.kernel.org > > > > Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > > --- > > > > 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
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:
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. Cc: stable@vger.kernel.org Fixes: e5225c820c05 ("media: uvcvideo: Send a control event when a Control Change interrupt arrives") Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_ctrl.c | 4 ++++ 1 file changed, 4 insertions(+)