[PATCH] media: uvcvideo: avoid variable shadowing in uvc_ctrl_cleanup_fh

[PATCH] media: uvcvideo: avoid variable shadowing in uvc_ctrl_cleanup_fh

[Desnes Nunes]

This avoids a variable loop shadowing occurring between the local loop
iterating through the uvc_entity's controls and the global one going
through the pending async controls of the file handle

Fixes: 10acb9101355 ("media: uvcvideo: Increase/decrease the PM counter per IOCTL")
Signed-off-by: Desnes Nunes <desnesn@redhat.com>
---
 drivers/media/usb/uvc/uvc_ctrl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 44b6513c5264..91cc874da798 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -3260,7 +3260,6 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
 void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
 {
 	struct uvc_entity *entity;
-	int i;
 
 	guard(mutex)(&handle->chain->ctrl_mutex);
 
@@ -3278,7 +3277,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
 	if (!WARN_ON(handle->pending_async_ctrls))
 		return;
 
-	for (i = 0; i < handle->pending_async_ctrls; i++)
+	for (unsigned int j = 0; j < handle->pending_async_ctrls; j++)
 		uvc_pm_put(handle->stream->dev);
 }
 
-- 
2.49.0

Re: [PATCH] media: uvcvideo: avoid variable shadowing in uvc_ctrl_cleanup_fh

[Ricardo Ribalda]

Hi Desnes

On Tue, 1 Jul 2025 at 16:59, Desnes Nunes <desnesn@redhat.com> wrote:
>
> This avoids a variable loop shadowing occurring between the local loop
> iterating through the uvc_entity's controls and the global one going
> through the pending async controls of the file handle
>
> Fixes: 10acb9101355 ("media: uvcvideo: Increase/decrease the PM counter per IOCTL")
If you add a fixes you need to add
Cc: stable@kernel.org

Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> Signed-off-by: Desnes Nunes <desnesn@redhat.com>
> ---
>  drivers/media/usb/uvc/uvc_ctrl.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 44b6513c5264..91cc874da798 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -3260,7 +3260,6 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
>  void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
>  {
>         struct uvc_entity *entity;
> -       int i;
>
>         guard(mutex)(&handle->chain->ctrl_mutex);
>
> @@ -3278,7 +3277,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
>         if (!WARN_ON(handle->pending_async_ctrls))
>                 return;
>
> -       for (i = 0; i < handle->pending_async_ctrls; i++)

nitpick: I would have called the variable i, not j.  For me j usually
means nested loop. But up to you

I am also not against your first version with a different commit message.

Thanks!
> +       for (unsigned int j = 0; j < handle->pending_async_ctrls; j++)
>                 uvc_pm_put(handle->stream->dev);
>  }
>
> --
> 2.49.0
>
>


-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: avoid variable shadowing in uvc_ctrl_cleanup_fh

[Desnes Nunes]

Hello Ricardo,

On Tue, Jul 1, 2025 at 1:48 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
>
> Hi Desnes
>
> On Tue, 1 Jul 2025 at 16:59, Desnes Nunes <desnesn@redhat.com> wrote:
> >
> > This avoids a variable loop shadowing occurring between the local loop
> > iterating through the uvc_entity's controls and the global one going
> > through the pending async controls of the file handle
> >
> > Fixes: 10acb9101355 ("media: uvcvideo: Increase/decrease the PM counter per IOCTL")
> If you add a fixes you need to add
> Cc: stable@kernel.org

Thanks for letting me know

>
> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> > Signed-off-by: Desnes Nunes <desnesn@redhat.com>
> > ---
> >  drivers/media/usb/uvc/uvc_ctrl.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 44b6513c5264..91cc874da798 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -3260,7 +3260,6 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
> >  void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> >  {
> >         struct uvc_entity *entity;
> > -       int i;
> >
> >         guard(mutex)(&handle->chain->ctrl_mutex);
> >
> > @@ -3278,7 +3277,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> >         if (!WARN_ON(handle->pending_async_ctrls))
> >                 return;
> >
> > -       for (i = 0; i < handle->pending_async_ctrls; i++)
>
> nitpick: I would have called the variable i, not j.  For me j usually
> means nested loop. But up to you

Noted - I used a different variable name because I wanted to
differentiate the loops.

>
> I am also not against your first version with a different commit message.

Third time's a charm then!

Will send a v2 with the first version having this commit message.

Thanks for the review Ricardo,

-- 
Desnes Nunes

Re: [PATCH] media: uvcvideo: avoid variable shadowing in uvc_ctrl_cleanup_fh

[Laurent Pinchart]

On Tue, Jul 01, 2025 at 02:20:53PM -0300, Desnes Nunes wrote:
> On Tue, Jul 1, 2025 at 1:48 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
> > On Tue, 1 Jul 2025 at 16:59, Desnes Nunes <desnesn@redhat.com> wrote:
> > >
> > > This avoids a variable loop shadowing occurring between the local loop
> > > iterating through the uvc_entity's controls and the global one going
> > > through the pending async controls of the file handle
> > >
> > > Fixes: 10acb9101355 ("media: uvcvideo: Increase/decrease the PM counter per IOCTL")
> > If you add a fixes you need to add
> > Cc: stable@kernel.org
> 
> Thanks for letting me know
> 
> > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> > > Signed-off-by: Desnes Nunes <desnesn@redhat.com>
> > > ---
> > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > index 44b6513c5264..91cc874da798 100644
> > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > @@ -3260,7 +3260,6 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
> > >  void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> > >  {
> > >         struct uvc_entity *entity;
> > > -       int i;
> > >
> > >         guard(mutex)(&handle->chain->ctrl_mutex);
> > >
> > > @@ -3278,7 +3277,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> > >         if (!WARN_ON(handle->pending_async_ctrls))
> > >                 return;
> > >
> > > -       for (i = 0; i < handle->pending_async_ctrls; i++)
> >
> > nitpick: I would have called the variable i, not j.  For me j usually
> > means nested loop. But up to you
> 
> Noted - I used a different variable name because I wanted to
> differentiate the loops.

Variable declaration in the loop statement is relatively new in the
kernel, so there's no consensus yet (to my knowledge) on clear coding
styles, but I would have simply used the same variable name in both
loops, with two separate declarations:

diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
index 303b7509ec47..6b9486749c3f 100644
--- a/drivers/media/usb/uvc/uvc_ctrl.c
+++ b/drivers/media/usb/uvc/uvc_ctrl.c
@@ -3299,7 +3299,6 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
 void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
 {
 	struct uvc_entity *entity;
-	int i;

 	guard(mutex)(&handle->chain->ctrl_mutex);

@@ -3317,7 +3316,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
 	if (!WARN_ON(handle->pending_async_ctrls))
 		return;

-	for (i = 0; i < handle->pending_async_ctrls; i++)
+	for (unsigned int i = 0; i < handle->pending_async_ctrls; i++)
 		uvc_pm_put(handle->stream->dev);
 }

Is there a downside to this ?

> > I am also not against your first version with a different commit message.
> 
> Third time's a charm then!
> 
> Will send a v2 with the first version having this commit message.
> 
> Thanks for the review Ricardo,

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: uvcvideo: avoid variable shadowing in uvc_ctrl_cleanup_fh

[Ricardo Ribalda]

On Tue, 1 Jul 2025 at 20:42, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> On Tue, Jul 01, 2025 at 02:20:53PM -0300, Desnes Nunes wrote:
> > On Tue, Jul 1, 2025 at 1:48 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
> > > On Tue, 1 Jul 2025 at 16:59, Desnes Nunes <desnesn@redhat.com> wrote:
> > > >
> > > > This avoids a variable loop shadowing occurring between the local loop
> > > > iterating through the uvc_entity's controls and the global one going
> > > > through the pending async controls of the file handle
> > > >
> > > > Fixes: 10acb9101355 ("media: uvcvideo: Increase/decrease the PM counter per IOCTL")
> > > If you add a fixes you need to add
> > > Cc: stable@kernel.org
> >
> > Thanks for letting me know
> >
> > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > Signed-off-by: Desnes Nunes <desnesn@redhat.com>
> > > > ---
> > > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 +--
> > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > index 44b6513c5264..91cc874da798 100644
> > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > @@ -3260,7 +3260,6 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
> > > >  void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> > > >  {
> > > >         struct uvc_entity *entity;
> > > > -       int i;
> > > >
> > > >         guard(mutex)(&handle->chain->ctrl_mutex);
> > > >
> > > > @@ -3278,7 +3277,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> > > >         if (!WARN_ON(handle->pending_async_ctrls))
> > > >                 return;
> > > >
> > > > -       for (i = 0; i < handle->pending_async_ctrls; i++)
> > >
> > > nitpick: I would have called the variable i, not j.  For me j usually
> > > means nested loop. But up to you
> >
> > Noted - I used a different variable name because I wanted to
> > differentiate the loops.
>
> Variable declaration in the loop statement is relatively new in the
> kernel, so there's no consensus yet (to my knowledge) on clear coding
> styles, but I would have simply used the same variable name in both
> loops, with two separate declarations:
>
> diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> index 303b7509ec47..6b9486749c3f 100644
> --- a/drivers/media/usb/uvc/uvc_ctrl.c
> +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> @@ -3299,7 +3299,6 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
>  void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
>  {
>         struct uvc_entity *entity;
> -       int i;
>
>         guard(mutex)(&handle->chain->ctrl_mutex);
>
> @@ -3317,7 +3316,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
>         if (!WARN_ON(handle->pending_async_ctrls))
>                 return;
>
> -       for (i = 0; i < handle->pending_async_ctrls; i++)
> +       for (unsigned int i = 0; i < handle->pending_async_ctrls; i++)
>                 uvc_pm_put(handle->stream->dev);
>  }
>
> Is there a downside to this ?

The toolchain that they are using does not seem to like it, and there
is no benefit for having two initialization vs one.

(also makes the lines shorter... but that is just a matter of taste).

I don't really have a preference.

>
> > > I am also not against your first version with a different commit message.
> >
> > Third time's a charm then!
> >
> > Will send a v2 with the first version having this commit message.
> >
> > Thanks for the review Ricardo,
>
> --
> Regards,
>
> Laurent Pinchart



-- 
Ricardo Ribalda

Re: [PATCH] media: uvcvideo: avoid variable shadowing in uvc_ctrl_cleanup_fh

[Laurent Pinchart]

On Tue, Jul 01, 2025 at 09:25:59PM +0200, Ricardo Ribalda wrote:
> On Tue, 1 Jul 2025 at 20:42, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > On Tue, Jul 01, 2025 at 02:20:53PM -0300, Desnes Nunes wrote:
> > > On Tue, Jul 1, 2025 at 1:48 PM Ricardo Ribalda <ribalda@chromium.org> wrote:
> > > > On Tue, 1 Jul 2025 at 16:59, Desnes Nunes <desnesn@redhat.com> wrote:
> > > > >
> > > > > This avoids a variable loop shadowing occurring between the local loop
> > > > > iterating through the uvc_entity's controls and the global one going
> > > > > through the pending async controls of the file handle
> > > > >
> > > > > Fixes: 10acb9101355 ("media: uvcvideo: Increase/decrease the PM counter per IOCTL")
> > > > If you add a fixes you need to add
> > > > Cc: stable@kernel.org
> > >
> > > Thanks for letting me know
> > >
> > > > Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
> > > > > Signed-off-by: Desnes Nunes <desnesn@redhat.com>
> > > > > ---
> > > > >  drivers/media/usb/uvc/uvc_ctrl.c | 3 +--
> > > > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > index 44b6513c5264..91cc874da798 100644
> > > > > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > > > > @@ -3260,7 +3260,6 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
> > > > >  void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> > > > >  {
> > > > >         struct uvc_entity *entity;
> > > > > -       int i;
> > > > >
> > > > >         guard(mutex)(&handle->chain->ctrl_mutex);
> > > > >
> > > > > @@ -3278,7 +3277,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> > > > >         if (!WARN_ON(handle->pending_async_ctrls))
> > > > >                 return;
> > > > >
> > > > > -       for (i = 0; i < handle->pending_async_ctrls; i++)
> > > >
> > > > nitpick: I would have called the variable i, not j.  For me j usually
> > > > means nested loop. But up to you
> > >
> > > Noted - I used a different variable name because I wanted to
> > > differentiate the loops.
> >
> > Variable declaration in the loop statement is relatively new in the
> > kernel, so there's no consensus yet (to my knowledge) on clear coding
> > styles, but I would have simply used the same variable name in both
> > loops, with two separate declarations:
> >
> > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c
> > index 303b7509ec47..6b9486749c3f 100644
> > --- a/drivers/media/usb/uvc/uvc_ctrl.c
> > +++ b/drivers/media/usb/uvc/uvc_ctrl.c
> > @@ -3299,7 +3299,6 @@ int uvc_ctrl_init_device(struct uvc_device *dev)
> >  void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> >  {
> >         struct uvc_entity *entity;
> > -       int i;
> >
> >         guard(mutex)(&handle->chain->ctrl_mutex);
> >
> > @@ -3317,7 +3316,7 @@ void uvc_ctrl_cleanup_fh(struct uvc_fh *handle)
> >         if (!WARN_ON(handle->pending_async_ctrls))
> >                 return;
> >
> > -       for (i = 0; i < handle->pending_async_ctrls; i++)
> > +       for (unsigned int i = 0; i < handle->pending_async_ctrls; i++)
> >                 uvc_pm_put(handle->stream->dev);
> >  }
> >
> > Is there a downside to this ?
> 
> The toolchain that they are using does not seem to like it,

Well, this is mainline :-)

> and there
> is no benefit for having two initialization vs one.
> 
> (also makes the lines shorter... but that is just a matter of taste).
> 
> I don't really have a preference.

Declaring the variable in the loop statement removes the possibility of
using it outside of the loop. I think that's an improvement in general,
it makes reading the code simpler, and avoids accidental misuse.

> > > > I am also not against your first version with a different commit message.
> > >
> > > Third time's a charm then!
> > >
> > > Will send a v2 with the first version having this commit message.
> > >
> > > Thanks for the review Ricardo,

-- 
Regards,

Laurent Pinchart