| Message ID | 20241127-uvc-fix-async-v2-1-510aab9570dd@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 12:14:49PM +0000, Ricardo Ribalda wrote: > When an async control is written, we copy a pointer to the file handle > that started the operation. That pointer will be used when the device is > done. Which could be anytime in the future. > > If the user closes that file descriptor, its structure will be freed, > and there will be one dangling pointer per pending async control, that > the driver will try to use. > > Clean all the dangling pointers during release(). > > To avoid adding a performance penalty in the most common case (no async > operation). A counter has been introduced with some logic to make sure s/). A/), a/ > that it is properly handled. > > 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 | 38 ++++++++++++++++++++++++++++++++++++-- > drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ > drivers/media/usb/uvc/uvcvideo.h | 8 +++++++- > 3 files changed, 45 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 4fe26e82e3d1..b6af4ff92cbd 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1589,7 +1589,12 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, How about adding static void uvc_ctrl_set_handle(struct uvc_control *ctrl, uvc_fh *handle) { if (handle) { if (!ctrl->handle) handle->pending_async_ctrls++; ctrl->handle = handle; } else if (ctrl->handle) { ctrl->handle = NULL; if (!WARN_ON(!handle->pending_async_ctrls)) handle->pending_async_ctrls--; } } > mutex_lock(&chain->ctrl_mutex); > > handle = ctrl->handle; > - ctrl->handle = NULL; > + if (handle) { > + ctrl->handle = NULL; > + WARN_ON(!handle->pending_async_ctrls); > + if (handle->pending_async_ctrls) > + handle->pending_async_ctrls--; > + } This would become handle = ctrl->handle; uvc_ctrl_set_handle(ctrl, NULL); > > list_for_each_entry(mapping, &ctrl->info.mappings, list) { > s32 value = __uvc_ctrl_get_value(mapping, data); > @@ -2046,8 +2051,11 @@ int uvc_ctrl_set(struct uvc_fh *handle, > mapping->set(mapping, value, > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > - if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > + if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) { > + if (!ctrl->handle) > + handle->pending_async_ctrls++; > ctrl->handle = handle; > + } Here if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) uvc_ctrl_set_handle(ctrl, handle); > > ctrl->dirty = 1; > ctrl->modified = 1; > @@ -2770,6 +2778,32 @@ int uvc_ctrl_init_device(struct uvc_device *dev) > return 0; > } > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) > +{ > + struct uvc_entity *entity; > + > + guard(mutex)(&handle->chain->ctrl_mutex); > + > + if (!handle->pending_async_ctrls) > + return; > + > + list_for_each_entry(entity, &handle->chain->dev->entities, list) { > + for (unsigned int i = 0; i < entity->ncontrols; ++i) { > + struct uvc_control *ctrl = &entity->controls[i]; > + > + if (ctrl->handle != handle) > + continue; > + > + ctrl->handle = NULL; > + if (WARN_ON(!handle->pending_async_ctrls)) > + continue; > + handle->pending_async_ctrls--; And here uvc_ctrl_set_handle(ctrl, NULL); It seems less error-prone to centralize the logic. I'd add a lockdep_assert() in uvc_ctrl_set_handle(), but there's no easy access to the chain there. > + } > + } > + > + WARN_ON(handle->pending_async_ctrls); > +} > + > /* > * Cleanup device controls. > */ > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > index 97c5407f6603..b425306a3b8c 100644 > --- a/drivers/media/usb/uvc/uvc_v4l2.c > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > @@ -652,6 +652,8 @@ static int uvc_v4l2_release(struct file *file) > > uvc_dbg(stream->dev, CALLS, "%s\n", __func__); > > + uvc_ctrl_cleanup_fh(handle); > + > /* Only free resources if this is a privileged handle. */ > if (uvc_has_privileges(handle)) > uvc_queue_release(&stream->queue); > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > index 07f9921d83f2..c68659b70221 100644 > --- a/drivers/media/usb/uvc/uvcvideo.h > +++ b/drivers/media/usb/uvc/uvcvideo.h > @@ -337,7 +337,10 @@ struct uvc_video_chain { > struct uvc_entity *processing; /* Processing unit */ > struct uvc_entity *selector; /* Selector unit */ > > - struct mutex ctrl_mutex; /* Protects ctrl.info */ > + struct mutex ctrl_mutex; /* > + * Protects ctrl.info and > + * uvc_fh.pending_async_ctrls > + */ > > struct v4l2_prio_state prio; /* V4L2 priority state */ > u32 caps; /* V4L2 chain-wide caps */ > @@ -612,6 +615,7 @@ struct uvc_fh { > struct uvc_video_chain *chain; > struct uvc_streaming *stream; > enum uvc_handle_state state; > + unsigned int pending_async_ctrls; > }; > > struct uvc_driver { > @@ -797,6 +801,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, > int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > struct uvc_xu_control_query *xqry); > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle); > + > /* Utility functions */ > struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts, > u8 epaddr);
On Thu, 28 Nov 2024 at 23:17, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. > > On Wed, Nov 27, 2024 at 12:14:49PM +0000, Ricardo Ribalda wrote: > > When an async control is written, we copy a pointer to the file handle > > that started the operation. That pointer will be used when the device is > > done. Which could be anytime in the future. > > > > If the user closes that file descriptor, its structure will be freed, > > and there will be one dangling pointer per pending async control, that > > the driver will try to use. > > > > Clean all the dangling pointers during release(). > > > > To avoid adding a performance penalty in the most common case (no async > > operation). A counter has been introduced with some logic to make sure > > s/). A/), a/ > > > that it is properly handled. > > > > 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 | 38 ++++++++++++++++++++++++++++++++++++-- > > drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ > > drivers/media/usb/uvc/uvcvideo.h | 8 +++++++- > > 3 files changed, 45 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > index 4fe26e82e3d1..b6af4ff92cbd 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -1589,7 +1589,12 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, > > How about adding SGTM. > > static void uvc_ctrl_set_handle(struct uvc_control *ctrl, uvc_fh *handle) > { > if (handle) { > if (!ctrl->handle) > handle->pending_async_ctrls++; > ctrl->handle = handle; > } else if (ctrl->handle) { > ctrl->handle = NULL; > if (!WARN_ON(!handle->pending_async_ctrls)) Unless you think otherwise. I will make this: WARN_ON(!handle->pending_async_ctrls); if (handle->pending_async_ctrls) handle->pending_async_ctrls--; The double negation is difficult to read. I am pretty sure the compiler will do its magic and merge the two checks. > handle->pending_async_ctrls--; > } > } > > > mutex_lock(&chain->ctrl_mutex); > > > > handle = ctrl->handle; > > - ctrl->handle = NULL; > > + if (handle) { > > + ctrl->handle = NULL; > > + WARN_ON(!handle->pending_async_ctrls); > > + if (handle->pending_async_ctrls) > > + handle->pending_async_ctrls--; > > + } > > This would become > > handle = ctrl->handle; > uvc_ctrl_set_handle(ctrl, NULL); > > > > > list_for_each_entry(mapping, &ctrl->info.mappings, list) { > > s32 value = __uvc_ctrl_get_value(mapping, data); > > @@ -2046,8 +2051,11 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > mapping->set(mapping, value, > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > > > - if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > > + if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) { > > + if (!ctrl->handle) > > + handle->pending_async_ctrls++; > > ctrl->handle = handle; > > + } > > Here > > if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > uvc_ctrl_set_handle(ctrl, handle); > > > > > ctrl->dirty = 1; > > ctrl->modified = 1; > > @@ -2770,6 +2778,32 @@ int uvc_ctrl_init_device(struct uvc_device *dev) > > return 0; > > } > > > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) > > +{ > > + struct uvc_entity *entity; > > + > > + guard(mutex)(&handle->chain->ctrl_mutex); > > + > > + if (!handle->pending_async_ctrls) > > + return; > > + > > + list_for_each_entry(entity, &handle->chain->dev->entities, list) { > > + for (unsigned int i = 0; i < entity->ncontrols; ++i) { > > + struct uvc_control *ctrl = &entity->controls[i]; > > + > > + if (ctrl->handle != handle) > > + continue; > > + > > + ctrl->handle = NULL; > > + if (WARN_ON(!handle->pending_async_ctrls)) > > + continue; > > + handle->pending_async_ctrls--; > > And here > > uvc_ctrl_set_handle(ctrl, NULL); > > It seems less error-prone to centralize the logic. I'd add a > lockdep_assert() in uvc_ctrl_set_handle(), but there's no easy access to > the chain there. > > > + } > > + } > > + > > + WARN_ON(handle->pending_async_ctrls); > > +} > > + > > /* > > * Cleanup device controls. > > */ > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > > index 97c5407f6603..b425306a3b8c 100644 > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > @@ -652,6 +652,8 @@ static int uvc_v4l2_release(struct file *file) > > > > uvc_dbg(stream->dev, CALLS, "%s\n", __func__); > > > > + uvc_ctrl_cleanup_fh(handle); > > + > > /* Only free resources if this is a privileged handle. */ > > if (uvc_has_privileges(handle)) > > uvc_queue_release(&stream->queue); > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > index 07f9921d83f2..c68659b70221 100644 > > --- a/drivers/media/usb/uvc/uvcvideo.h > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > @@ -337,7 +337,10 @@ struct uvc_video_chain { > > struct uvc_entity *processing; /* Processing unit */ > > struct uvc_entity *selector; /* Selector unit */ > > > > - struct mutex ctrl_mutex; /* Protects ctrl.info */ > > + struct mutex ctrl_mutex; /* > > + * Protects ctrl.info and > > + * uvc_fh.pending_async_ctrls > > + */ > > > > struct v4l2_prio_state prio; /* V4L2 priority state */ > > u32 caps; /* V4L2 chain-wide caps */ > > @@ -612,6 +615,7 @@ struct uvc_fh { > > struct uvc_video_chain *chain; > > struct uvc_streaming *stream; > > enum uvc_handle_state state; > > + unsigned int pending_async_ctrls; > > }; > > > > struct uvc_driver { > > @@ -797,6 +801,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, > > int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > > struct uvc_xu_control_query *xqry); > > > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle); > > + > > /* Utility functions */ > > struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts, > > u8 epaddr); > > -- > Regards, > > Laurent Pinchart
On Thu, Nov 28, 2024 at 11:25:25PM +0100, Ricardo Ribalda wrote: > On Thu, 28 Nov 2024 at 23:17, Laurent Pinchart wrote: > > On Wed, Nov 27, 2024 at 12:14:49PM +0000, Ricardo Ribalda wrote: > > > When an async control is written, we copy a pointer to the file handle > > > that started the operation. That pointer will be used when the device is > > > done. Which could be anytime in the future. > > > > > > If the user closes that file descriptor, its structure will be freed, > > > and there will be one dangling pointer per pending async control, that > > > the driver will try to use. > > > > > > Clean all the dangling pointers during release(). > > > > > > To avoid adding a performance penalty in the most common case (no async > > > operation). A counter has been introduced with some logic to make sure > > > > s/). A/), a/ > > > > > that it is properly handled. > > > > > > 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 | 38 ++++++++++++++++++++++++++++++++++++-- > > > drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ > > > drivers/media/usb/uvc/uvcvideo.h | 8 +++++++- > > > 3 files changed, 45 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > > index 4fe26e82e3d1..b6af4ff92cbd 100644 > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > > @@ -1589,7 +1589,12 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, > > > > How about adding > > SGTM. > > > > > static void uvc_ctrl_set_handle(struct uvc_control *ctrl, uvc_fh *handle) > > { > > if (handle) { > > if (!ctrl->handle) > > handle->pending_async_ctrls++; > > ctrl->handle = handle; > > } else if (ctrl->handle) { > > ctrl->handle = NULL; > > if (!WARN_ON(!handle->pending_async_ctrls)) > > Unless you think otherwise. I will make this: > > WARN_ON(!handle->pending_async_ctrls); > if (handle->pending_async_ctrls) > handle->pending_async_ctrls--; I'm fine with that, I went back and forth between the two. > The double negation is difficult to read. I am pretty sure the > compiler will do its magic and merge the two checks. > > > handle->pending_async_ctrls--; > > } > > } > > > > > mutex_lock(&chain->ctrl_mutex); > > > > > > handle = ctrl->handle; > > > - ctrl->handle = NULL; > > > + if (handle) { > > > + ctrl->handle = NULL; > > > + WARN_ON(!handle->pending_async_ctrls); > > > + if (handle->pending_async_ctrls) > > > + handle->pending_async_ctrls--; > > > + } > > > > This would become > > > > handle = ctrl->handle; > > uvc_ctrl_set_handle(ctrl, NULL); > > > > > > > > list_for_each_entry(mapping, &ctrl->info.mappings, list) { > > > s32 value = __uvc_ctrl_get_value(mapping, data); > > > @@ -2046,8 +2051,11 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > > mapping->set(mapping, value, > > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > > > > > - if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > > > + if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) { > > > + if (!ctrl->handle) > > > + handle->pending_async_ctrls++; > > > ctrl->handle = handle; > > > + } > > > > Here > > > > if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > > uvc_ctrl_set_handle(ctrl, handle); > > > > > > > > ctrl->dirty = 1; > > > ctrl->modified = 1; > > > @@ -2770,6 +2778,32 @@ int uvc_ctrl_init_device(struct uvc_device *dev) > > > return 0; > > > } > > > > > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) > > > +{ > > > + struct uvc_entity *entity; > > > + > > > + guard(mutex)(&handle->chain->ctrl_mutex); > > > + > > > + if (!handle->pending_async_ctrls) > > > + return; > > > + > > > + list_for_each_entry(entity, &handle->chain->dev->entities, list) { > > > + for (unsigned int i = 0; i < entity->ncontrols; ++i) { > > > + struct uvc_control *ctrl = &entity->controls[i]; > > > + > > > + if (ctrl->handle != handle) > > > + continue; > > > + > > > + ctrl->handle = NULL; > > > + if (WARN_ON(!handle->pending_async_ctrls)) > > > + continue; > > > + handle->pending_async_ctrls--; > > > > And here > > > > uvc_ctrl_set_handle(ctrl, NULL); > > > > It seems less error-prone to centralize the logic. I'd add a > > lockdep_assert() in uvc_ctrl_set_handle(), but there's no easy access to > > the chain there. > > > > > + } > > > + } > > > + > > > + WARN_ON(handle->pending_async_ctrls); > > > +} > > > + > > > /* > > > * Cleanup device controls. > > > */ > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > > > index 97c5407f6603..b425306a3b8c 100644 > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > > @@ -652,6 +652,8 @@ static int uvc_v4l2_release(struct file *file) > > > > > > uvc_dbg(stream->dev, CALLS, "%s\n", __func__); > > > > > > + uvc_ctrl_cleanup_fh(handle); > > > + > > > /* Only free resources if this is a privileged handle. */ > > > if (uvc_has_privileges(handle)) > > > uvc_queue_release(&stream->queue); > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > > index 07f9921d83f2..c68659b70221 100644 > > > --- a/drivers/media/usb/uvc/uvcvideo.h > > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > > @@ -337,7 +337,10 @@ struct uvc_video_chain { > > > struct uvc_entity *processing; /* Processing unit */ > > > struct uvc_entity *selector; /* Selector unit */ > > > > > > - struct mutex ctrl_mutex; /* Protects ctrl.info */ > > > + struct mutex ctrl_mutex; /* > > > + * Protects ctrl.info and > > > + * uvc_fh.pending_async_ctrls > > > + */ > > > > > > struct v4l2_prio_state prio; /* V4L2 priority state */ > > > u32 caps; /* V4L2 chain-wide caps */ > > > @@ -612,6 +615,7 @@ struct uvc_fh { > > > struct uvc_video_chain *chain; > > > struct uvc_streaming *stream; > > > enum uvc_handle_state state; > > > + unsigned int pending_async_ctrls; > > > }; > > > > > > struct uvc_driver { > > > @@ -797,6 +801,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, > > > int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > > > struct uvc_xu_control_query *xqry); > > > > > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle); > > > + > > > /* Utility functions */ > > > struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts, > > > u8 epaddr);
On Thu, 28 Nov 2024 at 23:28, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > On Thu, Nov 28, 2024 at 11:25:25PM +0100, Ricardo Ribalda wrote: > > On Thu, 28 Nov 2024 at 23:17, Laurent Pinchart wrote: > > > On Wed, Nov 27, 2024 at 12:14:49PM +0000, Ricardo Ribalda wrote: > > > > When an async control is written, we copy a pointer to the file handle > > > > that started the operation. That pointer will be used when the device is > > > > done. Which could be anytime in the future. > > > > > > > > If the user closes that file descriptor, its structure will be freed, > > > > and there will be one dangling pointer per pending async control, that > > > > the driver will try to use. > > > > > > > > Clean all the dangling pointers during release(). > > > > > > > > To avoid adding a performance penalty in the most common case (no async > > > > operation). A counter has been introduced with some logic to make sure > > > > > > s/). A/), a/ > > > > > > > that it is properly handled. > > > > > > > > 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 | 38 ++++++++++++++++++++++++++++++++++++-- > > > > drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ > > > > drivers/media/usb/uvc/uvcvideo.h | 8 +++++++- > > > > 3 files changed, 45 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > > > index 4fe26e82e3d1..b6af4ff92cbd 100644 > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > > > @@ -1589,7 +1589,12 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, > > > > > > How about adding > > > > SGTM. > > > > > > > > static void uvc_ctrl_set_handle(struct uvc_control *ctrl, uvc_fh *handle) > > > { > > > if (handle) { > > > if (!ctrl->handle) > > > handle->pending_async_ctrls++; > > > ctrl->handle = handle; > > > } else if (ctrl->handle) { > > > ctrl->handle = NULL; > > > if (!WARN_ON(!handle->pending_async_ctrls)) > > > > Unless you think otherwise. I will make this: > > > > WARN_ON(!handle->pending_async_ctrls); > > if (handle->pending_async_ctrls) > > handle->pending_async_ctrls--; > > I'm fine with that, I went back and forth between the two. > > > The double negation is difficult to read. I am pretty sure the > > compiler will do its magic and merge the two checks. > > > > > handle->pending_async_ctrls--; > > > } > > > } Now that I think about it. Now that we have inverted the patches 1 and 2 we need to add the following change to your function: diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index d8d4bd2254ec..5ce9be812559 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1584,8 +1584,14 @@ static void uvc_ctrl_set_handle(struct uvc_control *ctrl, struct uvc_fh *handle) /* chain->ctrl_mutex must be held. */ if (handle) { - if (!ctrl->handle) - handle->pending_async_ctrls++; + if (ctrl->handle) { + if (ctrl->handle == handle) + return; + WARN_ON(!ctrl->handle->pending_async_ctrls); + if (ctrl->handle->pending_async_ctrls) + ctrl->handle->pending_async_ctrls--; + } + handle->pending_async_ctrls++; ctrl->handle = handle; } else if (ctrl->handle) { ctrl->handle = NULL; Otherwise, if the control is handled by another fh, pending_async_ctrls will be unbalanced. > > > > > > > mutex_lock(&chain->ctrl_mutex); > > > > > > > > handle = ctrl->handle; > > > > - ctrl->handle = NULL; > > > > + if (handle) { > > > > + ctrl->handle = NULL; > > > > + WARN_ON(!handle->pending_async_ctrls); > > > > + if (handle->pending_async_ctrls) > > > > + handle->pending_async_ctrls--; > > > > + } > > > > > > This would become > > > > > > handle = ctrl->handle; > > > uvc_ctrl_set_handle(ctrl, NULL); > > > > > > > > > > > list_for_each_entry(mapping, &ctrl->info.mappings, list) { > > > > s32 value = __uvc_ctrl_get_value(mapping, data); > > > > @@ -2046,8 +2051,11 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > > > mapping->set(mapping, value, > > > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > > > > > > > - if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > > > > + if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) { > > > > + if (!ctrl->handle) > > > > + handle->pending_async_ctrls++; > > > > ctrl->handle = handle; > > > > + } > > > > > > Here > > > > > > if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > > > uvc_ctrl_set_handle(ctrl, handle); > > > > > > > > > > > ctrl->dirty = 1; > > > > ctrl->modified = 1; > > > > @@ -2770,6 +2778,32 @@ int uvc_ctrl_init_device(struct uvc_device *dev) > > > > return 0; > > > > } > > > > > > > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) > > > > +{ > > > > + struct uvc_entity *entity; > > > > + > > > > + guard(mutex)(&handle->chain->ctrl_mutex); > > > > + > > > > + if (!handle->pending_async_ctrls) > > > > + return; > > > > + > > > > + list_for_each_entry(entity, &handle->chain->dev->entities, list) { > > > > + for (unsigned int i = 0; i < entity->ncontrols; ++i) { > > > > + struct uvc_control *ctrl = &entity->controls[i]; > > > > + > > > > + if (ctrl->handle != handle) > > > > + continue; > > > > + > > > > + ctrl->handle = NULL; > > > > + if (WARN_ON(!handle->pending_async_ctrls)) > > > > + continue; > > > > + handle->pending_async_ctrls--; > > > > > > And here > > > > > > uvc_ctrl_set_handle(ctrl, NULL); > > > > > > It seems less error-prone to centralize the logic. I'd add a > > > lockdep_assert() in uvc_ctrl_set_handle(), but there's no easy access to > > > the chain there. > > > > > > > + } > > > > + } > > > > + > > > > + WARN_ON(handle->pending_async_ctrls); > > > > +} > > > > + > > > > /* > > > > * Cleanup device controls. > > > > */ > > > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > > > > index 97c5407f6603..b425306a3b8c 100644 > > > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > > > @@ -652,6 +652,8 @@ static int uvc_v4l2_release(struct file *file) > > > > > > > > uvc_dbg(stream->dev, CALLS, "%s\n", __func__); > > > > > > > > + uvc_ctrl_cleanup_fh(handle); > > > > + > > > > /* Only free resources if this is a privileged handle. */ > > > > if (uvc_has_privileges(handle)) > > > > uvc_queue_release(&stream->queue); > > > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > > > index 07f9921d83f2..c68659b70221 100644 > > > > --- a/drivers/media/usb/uvc/uvcvideo.h > > > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > > > @@ -337,7 +337,10 @@ struct uvc_video_chain { > > > > struct uvc_entity *processing; /* Processing unit */ > > > > struct uvc_entity *selector; /* Selector unit */ > > > > > > > > - struct mutex ctrl_mutex; /* Protects ctrl.info */ > > > > + struct mutex ctrl_mutex; /* > > > > + * Protects ctrl.info and > > > > + * uvc_fh.pending_async_ctrls > > > > + */ > > > > > > > > struct v4l2_prio_state prio; /* V4L2 priority state */ > > > > u32 caps; /* V4L2 chain-wide caps */ > > > > @@ -612,6 +615,7 @@ struct uvc_fh { > > > > struct uvc_video_chain *chain; > > > > struct uvc_streaming *stream; > > > > enum uvc_handle_state state; > > > > + unsigned int pending_async_ctrls; > > > > }; > > > > > > > > struct uvc_driver { > > > > @@ -797,6 +801,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, > > > > int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > > > > struct uvc_xu_control_query *xqry); > > > > > > > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle); > > > > + > > > > /* Utility functions */ > > > > struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts, > > > > u8 epaddr); > > -- > Regards, > > Laurent Pinchart
On Thu, 28 Nov 2024 at 23:17, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > Thank you for the patch. > > On Wed, Nov 27, 2024 at 12:14:49PM +0000, Ricardo Ribalda wrote: > > When an async control is written, we copy a pointer to the file handle > > that started the operation. That pointer will be used when the device is > > done. Which could be anytime in the future. > > > > If the user closes that file descriptor, its structure will be freed, > > and there will be one dangling pointer per pending async control, that > > the driver will try to use. > > > > Clean all the dangling pointers during release(). > > > > To avoid adding a performance penalty in the most common case (no async > > operation). A counter has been introduced with some logic to make sure > > s/). A/), a/ > > > that it is properly handled. > > > > 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 | 38 ++++++++++++++++++++++++++++++++++++-- > > drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ > > drivers/media/usb/uvc/uvcvideo.h | 8 +++++++- > > 3 files changed, 45 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > index 4fe26e82e3d1..b6af4ff92cbd 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -1589,7 +1589,12 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, > > How about adding > > static void uvc_ctrl_set_handle(struct uvc_control *ctrl, uvc_fh *handle) > { > if (handle) { > if (!ctrl->handle) > handle->pending_async_ctrls++; > ctrl->handle = handle; > } else if (ctrl->handle) { > ctrl->handle = NULL; > if (!WARN_ON(!handle->pending_async_ctrls)) > handle->pending_async_ctrls--; handle is NULL here. Luckily smatch found it :) I am rewriting it a bit. > } > } > > > mutex_lock(&chain->ctrl_mutex); > > > > handle = ctrl->handle; > > - ctrl->handle = NULL; > > + if (handle) { > > + ctrl->handle = NULL; > > + WARN_ON(!handle->pending_async_ctrls); > > + if (handle->pending_async_ctrls) > > + handle->pending_async_ctrls--; > > + } > > This would become > > handle = ctrl->handle; > uvc_ctrl_set_handle(ctrl, NULL); > > > > > list_for_each_entry(mapping, &ctrl->info.mappings, list) { > > s32 value = __uvc_ctrl_get_value(mapping, data); > > @@ -2046,8 +2051,11 @@ int uvc_ctrl_set(struct uvc_fh *handle, > > mapping->set(mapping, value, > > uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); > > > > - if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > > + if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) { > > + if (!ctrl->handle) > > + handle->pending_async_ctrls++; > > ctrl->handle = handle; > > + } > > Here > > if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > uvc_ctrl_set_handle(ctrl, handle); > > > > > ctrl->dirty = 1; > > ctrl->modified = 1; > > @@ -2770,6 +2778,32 @@ int uvc_ctrl_init_device(struct uvc_device *dev) > > return 0; > > } > > > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) > > +{ > > + struct uvc_entity *entity; > > + > > + guard(mutex)(&handle->chain->ctrl_mutex); > > + > > + if (!handle->pending_async_ctrls) > > + return; > > + > > + list_for_each_entry(entity, &handle->chain->dev->entities, list) { > > + for (unsigned int i = 0; i < entity->ncontrols; ++i) { > > + struct uvc_control *ctrl = &entity->controls[i]; > > + > > + if (ctrl->handle != handle) > > + continue; > > + > > + ctrl->handle = NULL; > > + if (WARN_ON(!handle->pending_async_ctrls)) > > + continue; > > + handle->pending_async_ctrls--; > > And here > > uvc_ctrl_set_handle(ctrl, NULL); > > It seems less error-prone to centralize the logic. I'd add a > lockdep_assert() in uvc_ctrl_set_handle(), but there's no easy access to > the chain there. > > > + } > > + } > > + > > + WARN_ON(handle->pending_async_ctrls); > > +} > > + > > /* > > * Cleanup device controls. > > */ > > diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c > > index 97c5407f6603..b425306a3b8c 100644 > > --- a/drivers/media/usb/uvc/uvc_v4l2.c > > +++ b/drivers/media/usb/uvc/uvc_v4l2.c > > @@ -652,6 +652,8 @@ static int uvc_v4l2_release(struct file *file) > > > > uvc_dbg(stream->dev, CALLS, "%s\n", __func__); > > > > + uvc_ctrl_cleanup_fh(handle); > > + > > /* Only free resources if this is a privileged handle. */ > > if (uvc_has_privileges(handle)) > > uvc_queue_release(&stream->queue); > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h > > index 07f9921d83f2..c68659b70221 100644 > > --- a/drivers/media/usb/uvc/uvcvideo.h > > +++ b/drivers/media/usb/uvc/uvcvideo.h > > @@ -337,7 +337,10 @@ struct uvc_video_chain { > > struct uvc_entity *processing; /* Processing unit */ > > struct uvc_entity *selector; /* Selector unit */ > > > > - struct mutex ctrl_mutex; /* Protects ctrl.info */ > > + struct mutex ctrl_mutex; /* > > + * Protects ctrl.info and > > + * uvc_fh.pending_async_ctrls > > + */ > > > > struct v4l2_prio_state prio; /* V4L2 priority state */ > > u32 caps; /* V4L2 chain-wide caps */ > > @@ -612,6 +615,7 @@ struct uvc_fh { > > struct uvc_video_chain *chain; > > struct uvc_streaming *stream; > > enum uvc_handle_state state; > > + unsigned int pending_async_ctrls; > > }; > > > > struct uvc_driver { > > @@ -797,6 +801,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, > > int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > > struct uvc_xu_control_query *xqry); > > > > +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle); > > + > > /* Utility functions */ > > struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts, > > u8 epaddr); > > -- > Regards, > > Laurent Pinchart
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 4fe26e82e3d1..b6af4ff92cbd 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1589,7 +1589,12 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, mutex_lock(&chain->ctrl_mutex); handle = ctrl->handle; - ctrl->handle = NULL; + if (handle) { + ctrl->handle = NULL; + WARN_ON(!handle->pending_async_ctrls); + if (handle->pending_async_ctrls) + handle->pending_async_ctrls--; + } list_for_each_entry(mapping, &ctrl->info.mappings, list) { s32 value = __uvc_ctrl_get_value(mapping, data); @@ -2046,8 +2051,11 @@ int uvc_ctrl_set(struct uvc_fh *handle, mapping->set(mapping, value, uvc_ctrl_data(ctrl, UVC_CTRL_DATA_CURRENT)); - if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) + if (ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) { + if (!ctrl->handle) + handle->pending_async_ctrls++; ctrl->handle = handle; + } ctrl->dirty = 1; ctrl->modified = 1; @@ -2770,6 +2778,32 @@ int uvc_ctrl_init_device(struct uvc_device *dev) return 0; } +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) +{ + struct uvc_entity *entity; + + guard(mutex)(&handle->chain->ctrl_mutex); + + if (!handle->pending_async_ctrls) + return; + + list_for_each_entry(entity, &handle->chain->dev->entities, list) { + for (unsigned int i = 0; i < entity->ncontrols; ++i) { + struct uvc_control *ctrl = &entity->controls[i]; + + if (ctrl->handle != handle) + continue; + + ctrl->handle = NULL; + if (WARN_ON(!handle->pending_async_ctrls)) + continue; + handle->pending_async_ctrls--; + } + } + + WARN_ON(handle->pending_async_ctrls); +} + /* * Cleanup device controls. */ diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c index 97c5407f6603..b425306a3b8c 100644 --- a/drivers/media/usb/uvc/uvc_v4l2.c +++ b/drivers/media/usb/uvc/uvc_v4l2.c @@ -652,6 +652,8 @@ static int uvc_v4l2_release(struct file *file) uvc_dbg(stream->dev, CALLS, "%s\n", __func__); + uvc_ctrl_cleanup_fh(handle); + /* Only free resources if this is a privileged handle. */ if (uvc_has_privileges(handle)) uvc_queue_release(&stream->queue); diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h index 07f9921d83f2..c68659b70221 100644 --- a/drivers/media/usb/uvc/uvcvideo.h +++ b/drivers/media/usb/uvc/uvcvideo.h @@ -337,7 +337,10 @@ struct uvc_video_chain { struct uvc_entity *processing; /* Processing unit */ struct uvc_entity *selector; /* Selector unit */ - struct mutex ctrl_mutex; /* Protects ctrl.info */ + struct mutex ctrl_mutex; /* + * Protects ctrl.info and + * uvc_fh.pending_async_ctrls + */ struct v4l2_prio_state prio; /* V4L2 priority state */ u32 caps; /* V4L2 chain-wide caps */ @@ -612,6 +615,7 @@ struct uvc_fh { struct uvc_video_chain *chain; struct uvc_streaming *stream; enum uvc_handle_state state; + unsigned int pending_async_ctrls; }; struct uvc_driver { @@ -797,6 +801,8 @@ int uvc_ctrl_is_accessible(struct uvc_video_chain *chain, u32 v4l2_id, int uvc_xu_ctrl_query(struct uvc_video_chain *chain, struct uvc_xu_control_query *xqry); +void uvc_ctrl_cleanup_fh(struct uvc_fh *handle); + /* Utility functions */ struct usb_host_endpoint *uvc_find_endpoint(struct usb_host_interface *alts, u8 epaddr);
When an async control is written, we copy a pointer to the file handle that started the operation. That pointer will be used when the device is done. Which could be anytime in the future. If the user closes that file descriptor, its structure will be freed, and there will be one dangling pointer per pending async control, that the driver will try to use. Clean all the dangling pointers during release(). To avoid adding a performance penalty in the most common case (no async operation). A counter has been introduced with some logic to make sure that it is properly handled. 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 | 38 ++++++++++++++++++++++++++++++++++++-- drivers/media/usb/uvc/uvc_v4l2.c | 2 ++ drivers/media/usb/uvc/uvcvideo.h | 8 +++++++- 3 files changed, 45 insertions(+), 3 deletions(-)