| Message ID | 20250509-uvc-followup-v1-1-73bcde30d2b5@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series | media: uvcvideo: Follow-up patches for next-media-uvc-20250509 | expand |
Hi Ricardo, On Fri, May 09, 2025 at 06:24:13PM +0000, Ricardo Ribalda wrote: > Today uvc_ctrl_set_handle() covers two use-uses: setting the handle and > clearing the handle. The only common code between the two cases is the > lockdep_assert_held. > > The code looks cleaner if we split these two usecases in two functions. It does indeed. Thanks for pushing for this :-) > We also take this opportunity to use pending_async_ctrls from ctrl where > possible. > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 65 +++++++++++++++++++++------------------- > 1 file changed, 35 insertions(+), 30 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index 44b6513c526421943bb9841fb53dc5f8e9f93f02..26be1d0a1891cf3a9a7489f60f9a2931c78d3239 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1812,48 +1812,53 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, > uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); > } > > -static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl, > - struct uvc_fh *new_handle) > +static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl) > { > - lockdep_assert_held(&handle->chain->ctrl_mutex); > - > - if (new_handle) { > - int ret; > + int ret; > > - if (ctrl->handle) > - dev_warn_ratelimited(&handle->stream->dev->udev->dev, > - "UVC non compliance: Setting an async control with a pending operation."); > + lockdep_assert_held(&handle->chain->ctrl_mutex); > > - if (new_handle == ctrl->handle) > - return 0; > + if (ctrl->handle) { > + dev_warn_ratelimited(&handle->stream->dev->udev->dev, > + "UVC non compliance: Setting an async control with a pending operation."); > > - if (ctrl->handle) { > - WARN_ON(!ctrl->handle->pending_async_ctrls); > - if (ctrl->handle->pending_async_ctrls) > - ctrl->handle->pending_async_ctrls--; > - ctrl->handle = new_handle; > - handle->pending_async_ctrls++; > + if (ctrl->handle == handle) > return 0; > - } > - > - ret = uvc_pm_get(handle->chain->dev); > - if (ret) > - return ret; > > - ctrl->handle = new_handle; > - handle->pending_async_ctrls++; > + WARN_ON(!ctrl->handle->pending_async_ctrls); > + if (ctrl->handle->pending_async_ctrls) > + ctrl->handle->pending_async_ctrls--; > + ctrl->handle = handle; > + ctrl->handle->pending_async_ctrls++; > return 0; > } > > + ret = uvc_pm_get(handle->chain->dev); > + if (ret) > + return ret; > + > + ctrl->handle = handle; > + ctrl->handle->pending_async_ctrls++; > + return 0; > +} > + > +static int uvc_ctrl_clear_handle(struct uvc_fh *handle, > + struct uvc_control *ctrl) > +{ > + lockdep_assert_held(&handle->chain->ctrl_mutex); > + > /* Cannot clear the handle for a control not owned by us.*/ While at it, I'll add a space before */ when applying. > if (WARN_ON(ctrl->handle != handle)) > return -EINVAL; But actually, as the caller guarantees that handle == ctrl->handle in both call sites (renaming the function makes this clear), can we drop the handle argument to this function ? If that's fine with you, I'd like to also change the uvc_ctrl_set_handle() function to pass the ctrl argument first. > > - ctrl->handle = NULL; > - if (WARN_ON(!handle->pending_async_ctrls)) > + if (WARN_ON(!ctrl->handle->pending_async_ctrls)) { > + ctrl->handle = NULL; > return -EINVAL; > - handle->pending_async_ctrls--; > + } > + > + ctrl->handle->pending_async_ctrls--; > uvc_pm_put(handle->chain->dev); This will need to become uvc_pm_put(ctrl->handle->chain->dev); Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + ctrl->handle = NULL; > return 0; > } > > @@ -1871,7 +1876,7 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, > > handle = ctrl->handle; > if (handle) > - uvc_ctrl_set_handle(handle, ctrl, NULL); > + uvc_ctrl_clear_handle(handle, ctrl); > > list_for_each_entry(mapping, &ctrl->info.mappings, list) { > s32 value; > @@ -2161,7 +2166,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, > > if (!rollback && handle && !ret && > ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > - ret = uvc_ctrl_set_handle(handle, ctrl, handle); > + ret = uvc_ctrl_set_handle(handle, ctrl); > > if (ret < 0 && !rollback) { > if (err_ctrl) > @@ -3271,7 +3276,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) > for (unsigned int i = 0; i < entity->ncontrols; ++i) { > if (entity->controls[i].handle != handle) > continue; > - uvc_ctrl_set_handle(handle, &entity->controls[i], NULL); > + uvc_ctrl_clear_handle(handle, &entity->controls[i]); > } > } >
Hi Laurent On Fri, 23 May 2025 at 10:53, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Ricardo, > > On Fri, May 09, 2025 at 06:24:13PM +0000, Ricardo Ribalda wrote: > > Today uvc_ctrl_set_handle() covers two use-uses: setting the handle and > > clearing the handle. The only common code between the two cases is the > > lockdep_assert_held. > > > > The code looks cleaner if we split these two usecases in two functions. > > It does indeed. Thanks for pushing for this :-) > > > We also take this opportunity to use pending_async_ctrls from ctrl where > > possible. > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > --- > > drivers/media/usb/uvc/uvc_ctrl.c | 65 +++++++++++++++++++++------------------- > > 1 file changed, 35 insertions(+), 30 deletions(-) > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > index 44b6513c526421943bb9841fb53dc5f8e9f93f02..26be1d0a1891cf3a9a7489f60f9a2931c78d3239 100644 > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > @@ -1812,48 +1812,53 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, > > uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); > > } > > > > -static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl, > > - struct uvc_fh *new_handle) > > +static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl) > > { > > - lockdep_assert_held(&handle->chain->ctrl_mutex); > > - > > - if (new_handle) { > > - int ret; > > + int ret; > > > > - if (ctrl->handle) > > - dev_warn_ratelimited(&handle->stream->dev->udev->dev, > > - "UVC non compliance: Setting an async control with a pending operation."); > > + lockdep_assert_held(&handle->chain->ctrl_mutex); > > > > - if (new_handle == ctrl->handle) > > - return 0; > > + if (ctrl->handle) { > > + dev_warn_ratelimited(&handle->stream->dev->udev->dev, > > + "UVC non compliance: Setting an async control with a pending operation."); > > > > - if (ctrl->handle) { > > - WARN_ON(!ctrl->handle->pending_async_ctrls); > > - if (ctrl->handle->pending_async_ctrls) > > - ctrl->handle->pending_async_ctrls--; > > - ctrl->handle = new_handle; > > - handle->pending_async_ctrls++; > > + if (ctrl->handle == handle) > > return 0; > > - } > > - > > - ret = uvc_pm_get(handle->chain->dev); > > - if (ret) > > - return ret; > > > > - ctrl->handle = new_handle; > > - handle->pending_async_ctrls++; > > + WARN_ON(!ctrl->handle->pending_async_ctrls); > > + if (ctrl->handle->pending_async_ctrls) > > + ctrl->handle->pending_async_ctrls--; > > + ctrl->handle = handle; > > + ctrl->handle->pending_async_ctrls++; > > return 0; > > } > > > > + ret = uvc_pm_get(handle->chain->dev); > > + if (ret) > > + return ret; > > + > > + ctrl->handle = handle; > > + ctrl->handle->pending_async_ctrls++; > > + return 0; > > +} > > + > > +static int uvc_ctrl_clear_handle(struct uvc_fh *handle, > > + struct uvc_control *ctrl) > > +{ > > + lockdep_assert_held(&handle->chain->ctrl_mutex); > > + > > /* Cannot clear the handle for a control not owned by us.*/ > > While at it, I'll add a space before */ when applying. > > > if (WARN_ON(ctrl->handle != handle)) > > return -EINVAL; > > But actually, as the caller guarantees that handle == ctrl->handle in > both call sites (renaming the function makes this clear), can we drop > the handle argument to this function ? > > If that's fine with you, I'd like to also change the > uvc_ctrl_set_handle() function to pass the ctrl argument first. SGTM, let me know if you have time to do this or if you prefer that I do it. Cheers > > > > > - ctrl->handle = NULL; > > - if (WARN_ON(!handle->pending_async_ctrls)) > > + if (WARN_ON(!ctrl->handle->pending_async_ctrls)) { > > + ctrl->handle = NULL; > > return -EINVAL; > > - handle->pending_async_ctrls--; > > + } > > + > > + ctrl->handle->pending_async_ctrls--; > > uvc_pm_put(handle->chain->dev); > > This will need to become > > uvc_pm_put(ctrl->handle->chain->dev); > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + ctrl->handle = NULL; > > return 0; > > } > > > > @@ -1871,7 +1876,7 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, > > > > handle = ctrl->handle; > > if (handle) > > - uvc_ctrl_set_handle(handle, ctrl, NULL); > > + uvc_ctrl_clear_handle(handle, ctrl); > > > > list_for_each_entry(mapping, &ctrl->info.mappings, list) { > > s32 value; > > @@ -2161,7 +2166,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, > > > > if (!rollback && handle && !ret && > > ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > > - ret = uvc_ctrl_set_handle(handle, ctrl, handle); > > + ret = uvc_ctrl_set_handle(handle, ctrl); > > > > if (ret < 0 && !rollback) { > > if (err_ctrl) > > @@ -3271,7 +3276,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) > > for (unsigned int i = 0; i < entity->ncontrols; ++i) { > > if (entity->controls[i].handle != handle) > > continue; > > - uvc_ctrl_set_handle(handle, &entity->controls[i], NULL); > > + uvc_ctrl_clear_handle(handle, &entity->controls[i]); > > } > > } > > > > -- > Regards, > > Laurent Pinchart
On Fri, May 23, 2025 at 12:58:31PM +0200, Ricardo Ribalda wrote: > On Fri, 23 May 2025 at 10:53, Laurent Pinchart wrote: > > On Fri, May 09, 2025 at 06:24:13PM +0000, Ricardo Ribalda wrote: > > > Today uvc_ctrl_set_handle() covers two use-uses: setting the handle and > > > clearing the handle. The only common code between the two cases is the > > > lockdep_assert_held. > > > > > > The code looks cleaner if we split these two usecases in two functions. > > > > It does indeed. Thanks for pushing for this :-) > > > > > We also take this opportunity to use pending_async_ctrls from ctrl where > > > possible. > > > > > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > > > --- > > > drivers/media/usb/uvc/uvc_ctrl.c | 65 +++++++++++++++++++++------------------- > > > 1 file changed, 35 insertions(+), 30 deletions(-) > > > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > > > index 44b6513c526421943bb9841fb53dc5f8e9f93f02..26be1d0a1891cf3a9a7489f60f9a2931c78d3239 100644 > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > > > @@ -1812,48 +1812,53 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, > > > uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); > > > } > > > > > > -static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl, > > > - struct uvc_fh *new_handle) > > > +static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl) > > > { > > > - lockdep_assert_held(&handle->chain->ctrl_mutex); > > > - > > > - if (new_handle) { > > > - int ret; > > > + int ret; > > > > > > - if (ctrl->handle) > > > - dev_warn_ratelimited(&handle->stream->dev->udev->dev, > > > - "UVC non compliance: Setting an async control with a pending operation."); > > > + lockdep_assert_held(&handle->chain->ctrl_mutex); > > > > > > - if (new_handle == ctrl->handle) > > > - return 0; > > > + if (ctrl->handle) { > > > + dev_warn_ratelimited(&handle->stream->dev->udev->dev, > > > + "UVC non compliance: Setting an async control with a pending operation."); > > > > > > - if (ctrl->handle) { > > > - WARN_ON(!ctrl->handle->pending_async_ctrls); > > > - if (ctrl->handle->pending_async_ctrls) > > > - ctrl->handle->pending_async_ctrls--; > > > - ctrl->handle = new_handle; > > > - handle->pending_async_ctrls++; > > > + if (ctrl->handle == handle) > > > return 0; > > > - } > > > - > > > - ret = uvc_pm_get(handle->chain->dev); > > > - if (ret) > > > - return ret; > > > > > > - ctrl->handle = new_handle; > > > - handle->pending_async_ctrls++; > > > + WARN_ON(!ctrl->handle->pending_async_ctrls); > > > + if (ctrl->handle->pending_async_ctrls) > > > + ctrl->handle->pending_async_ctrls--; > > > + ctrl->handle = handle; > > > + ctrl->handle->pending_async_ctrls++; > > > return 0; > > > } > > > > > > + ret = uvc_pm_get(handle->chain->dev); > > > + if (ret) > > > + return ret; > > > + > > > + ctrl->handle = handle; > > > + ctrl->handle->pending_async_ctrls++; > > > + return 0; > > > +} > > > + > > > +static int uvc_ctrl_clear_handle(struct uvc_fh *handle, > > > + struct uvc_control *ctrl) > > > +{ > > > + lockdep_assert_held(&handle->chain->ctrl_mutex); > > > + > > > /* Cannot clear the handle for a control not owned by us.*/ > > > > While at it, I'll add a space before */ when applying. > > > > > if (WARN_ON(ctrl->handle != handle)) > > > return -EINVAL; > > > > But actually, as the caller guarantees that handle == ctrl->handle in > > both call sites (renaming the function makes this clear), can we drop > > the handle argument to this function ? > > > > If that's fine with you, I'd like to also change the > > uvc_ctrl_set_handle() function to pass the ctrl argument first. > > SGTM, let me know if you have time to do this or if you prefer that I do it. I'll send a new version of this patch. > > > - ctrl->handle = NULL; > > > - if (WARN_ON(!handle->pending_async_ctrls)) > > > + if (WARN_ON(!ctrl->handle->pending_async_ctrls)) { > > > + ctrl->handle = NULL; > > > return -EINVAL; > > > - handle->pending_async_ctrls--; > > > + } > > > + > > > + ctrl->handle->pending_async_ctrls--; > > > uvc_pm_put(handle->chain->dev); > > > > This will need to become > > > > uvc_pm_put(ctrl->handle->chain->dev); > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > + ctrl->handle = NULL; > > > return 0; > > > } > > > > > > @@ -1871,7 +1876,7 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, > > > > > > handle = ctrl->handle; > > > if (handle) > > > - uvc_ctrl_set_handle(handle, ctrl, NULL); > > > + uvc_ctrl_clear_handle(handle, ctrl); > > > > > > list_for_each_entry(mapping, &ctrl->info.mappings, list) { > > > s32 value; > > > @@ -2161,7 +2166,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, > > > > > > if (!rollback && handle && !ret && > > > ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) > > > - ret = uvc_ctrl_set_handle(handle, ctrl, handle); > > > + ret = uvc_ctrl_set_handle(handle, ctrl); > > > > > > if (ret < 0 && !rollback) { > > > if (err_ctrl) > > > @@ -3271,7 +3276,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) > > > for (unsigned int i = 0; i < entity->ncontrols; ++i) { > > > if (entity->controls[i].handle != handle) > > > continue; > > > - uvc_ctrl_set_handle(handle, &entity->controls[i], NULL); > > > + uvc_ctrl_clear_handle(handle, &entity->controls[i]); > > > } > > > } > > >
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index 44b6513c526421943bb9841fb53dc5f8e9f93f02..26be1d0a1891cf3a9a7489f60f9a2931c78d3239 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1812,48 +1812,53 @@ static void uvc_ctrl_send_slave_event(struct uvc_video_chain *chain, uvc_ctrl_send_event(chain, handle, ctrl, mapping, val, changes); } -static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl, - struct uvc_fh *new_handle) +static int uvc_ctrl_set_handle(struct uvc_fh *handle, struct uvc_control *ctrl) { - lockdep_assert_held(&handle->chain->ctrl_mutex); - - if (new_handle) { - int ret; + int ret; - if (ctrl->handle) - dev_warn_ratelimited(&handle->stream->dev->udev->dev, - "UVC non compliance: Setting an async control with a pending operation."); + lockdep_assert_held(&handle->chain->ctrl_mutex); - if (new_handle == ctrl->handle) - return 0; + if (ctrl->handle) { + dev_warn_ratelimited(&handle->stream->dev->udev->dev, + "UVC non compliance: Setting an async control with a pending operation."); - if (ctrl->handle) { - WARN_ON(!ctrl->handle->pending_async_ctrls); - if (ctrl->handle->pending_async_ctrls) - ctrl->handle->pending_async_ctrls--; - ctrl->handle = new_handle; - handle->pending_async_ctrls++; + if (ctrl->handle == handle) return 0; - } - - ret = uvc_pm_get(handle->chain->dev); - if (ret) - return ret; - ctrl->handle = new_handle; - handle->pending_async_ctrls++; + WARN_ON(!ctrl->handle->pending_async_ctrls); + if (ctrl->handle->pending_async_ctrls) + ctrl->handle->pending_async_ctrls--; + ctrl->handle = handle; + ctrl->handle->pending_async_ctrls++; return 0; } + ret = uvc_pm_get(handle->chain->dev); + if (ret) + return ret; + + ctrl->handle = handle; + ctrl->handle->pending_async_ctrls++; + return 0; +} + +static int uvc_ctrl_clear_handle(struct uvc_fh *handle, + struct uvc_control *ctrl) +{ + lockdep_assert_held(&handle->chain->ctrl_mutex); + /* Cannot clear the handle for a control not owned by us.*/ if (WARN_ON(ctrl->handle != handle)) return -EINVAL; - ctrl->handle = NULL; - if (WARN_ON(!handle->pending_async_ctrls)) + if (WARN_ON(!ctrl->handle->pending_async_ctrls)) { + ctrl->handle = NULL; return -EINVAL; - handle->pending_async_ctrls--; + } + + ctrl->handle->pending_async_ctrls--; uvc_pm_put(handle->chain->dev); + ctrl->handle = NULL; return 0; } @@ -1871,7 +1876,7 @@ void uvc_ctrl_status_event(struct uvc_video_chain *chain, handle = ctrl->handle; if (handle) - uvc_ctrl_set_handle(handle, ctrl, NULL); + uvc_ctrl_clear_handle(handle, ctrl); list_for_each_entry(mapping, &ctrl->info.mappings, list) { s32 value; @@ -2161,7 +2166,7 @@ static int uvc_ctrl_commit_entity(struct uvc_device *dev, if (!rollback && handle && !ret && ctrl->info.flags & UVC_CTRL_FLAG_ASYNCHRONOUS) - ret = uvc_ctrl_set_handle(handle, ctrl, handle); + ret = uvc_ctrl_set_handle(handle, ctrl); if (ret < 0 && !rollback) { if (err_ctrl) @@ -3271,7 +3276,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle) for (unsigned int i = 0; i < entity->ncontrols; ++i) { if (entity->controls[i].handle != handle) continue; - uvc_ctrl_set_handle(handle, &entity->controls[i], NULL); + uvc_ctrl_clear_handle(handle, &entity->controls[i]); } }
Today uvc_ctrl_set_handle() covers two use-uses: setting the handle and clearing the handle. The only common code between the two cases is the lockdep_assert_held. The code looks cleaner if we split these two usecases in two functions. We also take this opportunity to use pending_async_ctrls from ctrl where possible. Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_ctrl.c | 65 +++++++++++++++++++++------------------- 1 file changed, 35 insertions(+), 30 deletions(-)