| Message ID | 20220306175034.3084609-27-jakobkoschel@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: gadget: remove usage of list iterator past the loop | expand |
On 06/03/2022 18:50, Jakob Koschel wrote: > To move the list iterator variable into the list_for_each_entry_*() > macro in the future it should be avoided to use the list iterator > variable after the loop body. > > To *never* use the list iterator variable after the loop it was > concluded to use a separate iterator variable [1]. > > This removes the need to check the rc value to determine if the > break/goto was hit and can be made more obvious > by checking if the variable was set within the list traversal loop. > > Link: https://lore.kernel.org/all/YhdfEIwI4EdtHdym@kroah.com/ > Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com> > --- > drivers/usb/gadget/udc/s3c2410_udc.c | 11 ++++++----- > 1 file changed, 6 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/gadget/udc/s3c2410_udc.c b/drivers/usb/gadget/udc/s3c2410_udc.c > index e3931da24277..fe6ecb0d16d1 100644 > --- a/drivers/usb/gadget/udc/s3c2410_udc.c > +++ b/drivers/usb/gadget/udc/s3c2410_udc.c > @@ -1265,7 +1265,7 @@ static int s3c2410_udc_dequeue(struct usb_ep *_ep, struct usb_request *_req) > struct s3c2410_ep *ep = to_s3c2410_ep(_ep); > int retval = -EINVAL; > unsigned long flags; > - struct s3c2410_request *req = NULL; > + struct s3c2410_request *req = NULL, *tmp; > > dprintk(DEBUG_VERBOSE, "%s(%p,%p)\n", __func__, _ep, _req); > > @@ -1277,16 +1277,17 @@ static int s3c2410_udc_dequeue(struct usb_ep *_ep, struct usb_request *_req) > > local_irq_save(flags); > > - list_for_each_entry(req, &ep->queue, queue) { > - if (&req->req == _req) { > - list_del_init(&req->queue); > + list_for_each_entry(tmp, &ep->queue, queue) { > + if (&tmp->req == _req) { > + list_del_init(&tmp->queue); > _req->status = -ECONNRESET; > + req = tmp; > retval = 0; > break; > } > } > > - if (retval == 0) { > + if (req) { I saw comments from Greg about it, so please keep similar approach here as in patch 25. I propose to keep retval. With retval restored: Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> Best regards, Krzysztof
diff --git a/drivers/usb/gadget/udc/s3c2410_udc.c b/drivers/usb/gadget/udc/s3c2410_udc.c index e3931da24277..fe6ecb0d16d1 100644 --- a/drivers/usb/gadget/udc/s3c2410_udc.c +++ b/drivers/usb/gadget/udc/s3c2410_udc.c @@ -1265,7 +1265,7 @@ static int s3c2410_udc_dequeue(struct usb_ep *_ep, struct usb_request *_req) struct s3c2410_ep *ep = to_s3c2410_ep(_ep); int retval = -EINVAL; unsigned long flags; - struct s3c2410_request *req = NULL; + struct s3c2410_request *req = NULL, *tmp; dprintk(DEBUG_VERBOSE, "%s(%p,%p)\n", __func__, _ep, _req); @@ -1277,16 +1277,17 @@ static int s3c2410_udc_dequeue(struct usb_ep *_ep, struct usb_request *_req) local_irq_save(flags); - list_for_each_entry(req, &ep->queue, queue) { - if (&req->req == _req) { - list_del_init(&req->queue); + list_for_each_entry(tmp, &ep->queue, queue) { + if (&tmp->req == _req) { + list_del_init(&tmp->queue); _req->status = -ECONNRESET; + req = tmp; retval = 0; break; } } - if (retval == 0) { + if (req) { dprintk(DEBUG_VERBOSE, "dequeued req %p from %s, len %d buf %p\n", req, _ep->name, _req->length, _req->buf);
To move the list iterator variable into the list_for_each_entry_*() macro in the future it should be avoided to use the list iterator variable after the loop body. To *never* use the list iterator variable after the loop it was concluded to use a separate iterator variable [1]. This removes the need to check the rc value to determine if the break/goto was hit and can be made more obvious by checking if the variable was set within the list traversal loop. Link: https://lore.kernel.org/all/YhdfEIwI4EdtHdym@kroah.com/ Signed-off-by: Jakob Koschel <jakobkoschel@gmail.com> --- drivers/usb/gadget/udc/s3c2410_udc.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)