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(-)
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]);
}
}
--
2.49.0.1015.ga840276032-googHi 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]);
> }
> }
>
--
Regards,
Laurent Pinchart
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
--
Ricardo Ribalda
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]);
> > > }
> > > }
> > >
--
Regards,
Laurent Pinchart
© 2016 - 2025 Red Hat, Inc.