| Message ID | 20240415-fix-cocci-v1-3-477afb23728b@chromium.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: Fix coccinelle warning/errors | expand |
On (24/04/15 19:34), Ricardo Ribalda wrote: [..] > @@ -2165,7 +2167,7 @@ static int uvc_ctrl_init_xu_ctrl(struct uvc_device *dev, > int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > struct uvc_xu_control_query *xqry) > { > - struct uvc_entity *entity; > + struct uvc_entity *entity, *iter; > struct uvc_control *ctrl; > unsigned int i; > bool found; Is `found` still used? > @@ -2175,16 +2177,16 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > int ret; > > /* Find the extension unit. */ > - found = false; > - list_for_each_entry(entity, &chain->entities, chain) { > - if (UVC_ENTITY_TYPE(entity) == UVC_VC_EXTENSION_UNIT && > - entity->id == xqry->unit) { > - found = true; > + entity = NULL; > + list_for_each_entry(iter, &chain->entities, chain) { > + if (UVC_ENTITY_TYPE(iter) == UVC_VC_EXTENSION_UNIT && > + iter->id == xqry->unit) { > + entity = iter; > break; > } > } > > - if (!found) { > + if (!entity) { > uvc_dbg(chain->dev, CONTROL, "Extension unit %u not found\n", > xqry->unit); > return -ENOENT;
On (24/04/16 13:39), Sergey Senozhatsky wrote: > On (24/04/15 19:34), Ricardo Ribalda wrote: > [..] > > @@ -2165,7 +2167,7 @@ static int uvc_ctrl_init_xu_ctrl(struct uvc_device *dev, > > int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > > struct uvc_xu_control_query *xqry) > > { > > - struct uvc_entity *entity; > > + struct uvc_entity *entity, *iter; > > struct uvc_control *ctrl; > > unsigned int i; > > bool found; > > Is `found` still used? It is. Never mind. FWIW Reviewed-by: Sergey Senozhatsky <senozhatsky@chromium.org>
I would find a hint for a variable change more appropriate in the patch subject instead of the word “iterators”. … > +++ b/drivers/media/usb/uvc/uvc_ctrl.c … > @@ -2175,16 +2177,16 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > int ret; > > /* Find the extension unit. */ … > + entity = NULL; > + list_for_each_entry(iter, &chain->entities, chain) { … I suggest to move this assignment into the definition for the affected local variable. By the way: I see another source code adjustment opportunity in this function implementation. https://elixir.bootlin.com/linux/v6.9-rc4/source/drivers/media/usb/uvc/uvc_ctrl.c#L2165 Can it be nicer to use labels “free_data” and “unlock” (instead of “done”)? How do you think about to increase the application of scope-based resource management here? Regards, Markus
On Tue, Apr 16, 2024 at 09:03:36AM +0200, Markus Elfring wrote: > I would find a hint for a variable change more appropriate in the patch subject > instead of the word “iterators”. > > > … > > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > … > > @@ -2175,16 +2177,16 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > > int ret; > > > > /* Find the extension unit. */ > … > > + entity = NULL; > > + list_for_each_entry(iter, &chain->entities, chain) { > … > > I suggest to move this assignment into the definition for the affected local variable. > > > By the way: > I see another source code adjustment opportunity in this function implementation. > https://elixir.bootlin.com/linux/v6.9-rc4/source/drivers/media/usb/uvc/uvc_ctrl.c#L2165 > > Can it be nicer to use labels “free_data” and “unlock” (instead of “done”)? > How do you think about to increase the application of scope-based resource management here? Hi, This is the semi-friendly patch-bot of Greg Kroah-Hartman. Markus, you seem to have sent a nonsensical or otherwise pointless review comment to a patch submission on a Linux kernel developer mailing list. I strongly suggest that you not do this anymore. Please do not bother developers who are actively working to produce patches and features with comments that, in the end, are a waste of time. Patch submitter, please ignore Markus's suggestion; you do not need to follow it at all. The person/bot/AI that sent it is being ignored by almost all Linux kernel maintainers for having a persistent pattern of behavior of producing distracting and pointless commentary, and inability to adapt to feedback. Please feel free to also ignore emails from them. thanks, greg k-h's patch email bot
Hi Ricardo, Thank you for the patch. On Mon, Apr 15, 2024 at 07:34:20PM +0000, Ricardo Ribalda wrote: > Avoid using the iterators after the list_for_each() constructs. > This patch should be a NOP, but makes cocci, happier: > > drivers/media/usb/uvc/uvc_ctrl.c:1861:44-50: ERROR: invalid reference to the index variable of the iterator on line 1850 > drivers/media/usb/uvc/uvc_ctrl.c:2195:17-23: ERROR: invalid reference to the index variable of the iterator on line 2179 > > Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> > --- > drivers/media/usb/uvc/uvc_ctrl.c | 24 +++++++++++++----------- > 1 file changed, 13 insertions(+), 11 deletions(-) > > diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c > index e59a463c2761..a4a987913430 100644 > --- a/drivers/media/usb/uvc/uvc_ctrl.c > +++ b/drivers/media/usb/uvc/uvc_ctrl.c > @@ -1850,16 +1850,18 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, > list_for_each_entry(entity, &chain->entities, chain) { If we really want to ensure the iterator won't be used after the loop, it could be declared in the loop statement itself, now that the kernel has switched to a newer C version. > ret = uvc_ctrl_commit_entity(chain->dev, entity, rollback, > &err_ctrl); > - if (ret < 0) > + if (ret < 0) { > + if (ctrls) > + ctrls->error_idx = > + uvc_ctrl_find_ctrl_idx(entity, ctrls, > + err_ctrl); > goto done; > + } > } > > if (!rollback) > uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count); > done: > - if (ret < 0 && ctrls) > - ctrls->error_idx = uvc_ctrl_find_ctrl_idx(entity, ctrls, > - err_ctrl); > mutex_unlock(&chain->ctrl_mutex); > return ret; > } > @@ -2165,7 +2167,7 @@ static int uvc_ctrl_init_xu_ctrl(struct uvc_device *dev, > int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > struct uvc_xu_control_query *xqry) > { > - struct uvc_entity *entity; > + struct uvc_entity *entity, *iter; > struct uvc_control *ctrl; > unsigned int i; > bool found; > @@ -2175,16 +2177,16 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain, > int ret; > > /* Find the extension unit. */ > - found = false; > - list_for_each_entry(entity, &chain->entities, chain) { > - if (UVC_ENTITY_TYPE(entity) == UVC_VC_EXTENSION_UNIT && > - entity->id == xqry->unit) { > - found = true; > + entity = NULL; > + list_for_each_entry(iter, &chain->entities, chain) { Same here, iter could be declared in the loop. > + if (UVC_ENTITY_TYPE(iter) == UVC_VC_EXTENSION_UNIT && > + iter->id == xqry->unit) { > + entity = iter; > break; > } > } > > - if (!found) { > + if (!entity) { > uvc_dbg(chain->dev, CONTROL, "Extension unit %u not found\n", > xqry->unit); > return -ENOENT; >
diff --git a/drivers/media/usb/uvc/uvc_ctrl.c b/drivers/media/usb/uvc/uvc_ctrl.c index e59a463c2761..a4a987913430 100644 --- a/drivers/media/usb/uvc/uvc_ctrl.c +++ b/drivers/media/usb/uvc/uvc_ctrl.c @@ -1850,16 +1850,18 @@ int __uvc_ctrl_commit(struct uvc_fh *handle, int rollback, list_for_each_entry(entity, &chain->entities, chain) { ret = uvc_ctrl_commit_entity(chain->dev, entity, rollback, &err_ctrl); - if (ret < 0) + if (ret < 0) { + if (ctrls) + ctrls->error_idx = + uvc_ctrl_find_ctrl_idx(entity, ctrls, + err_ctrl); goto done; + } } if (!rollback) uvc_ctrl_send_events(handle, ctrls->controls, ctrls->count); done: - if (ret < 0 && ctrls) - ctrls->error_idx = uvc_ctrl_find_ctrl_idx(entity, ctrls, - err_ctrl); mutex_unlock(&chain->ctrl_mutex); return ret; } @@ -2165,7 +2167,7 @@ static int uvc_ctrl_init_xu_ctrl(struct uvc_device *dev, int uvc_xu_ctrl_query(struct uvc_video_chain *chain, struct uvc_xu_control_query *xqry) { - struct uvc_entity *entity; + struct uvc_entity *entity, *iter; struct uvc_control *ctrl; unsigned int i; bool found; @@ -2175,16 +2177,16 @@ int uvc_xu_ctrl_query(struct uvc_video_chain *chain, int ret; /* Find the extension unit. */ - found = false; - list_for_each_entry(entity, &chain->entities, chain) { - if (UVC_ENTITY_TYPE(entity) == UVC_VC_EXTENSION_UNIT && - entity->id == xqry->unit) { - found = true; + entity = NULL; + list_for_each_entry(iter, &chain->entities, chain) { + if (UVC_ENTITY_TYPE(iter) == UVC_VC_EXTENSION_UNIT && + iter->id == xqry->unit) { + entity = iter; break; } } - if (!found) { + if (!entity) { uvc_dbg(chain->dev, CONTROL, "Extension unit %u not found\n", xqry->unit); return -ENOENT;
Avoid using the iterators after the list_for_each() constructs. This patch should be a NOP, but makes cocci, happier: drivers/media/usb/uvc/uvc_ctrl.c:1861:44-50: ERROR: invalid reference to the index variable of the iterator on line 1850 drivers/media/usb/uvc/uvc_ctrl.c:2195:17-23: ERROR: invalid reference to the index variable of the iterator on line 2179 Signed-off-by: Ricardo Ribalda <ribalda@chromium.org> --- drivers/media/usb/uvc/uvc_ctrl.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)