On 08/03/2022 18:18, Jakob Koschel wrote: > If the list representing the request queue does not contain the expected > request, the value of the list_for_each_entry() iterator will not point > to a valid structure. To avoid type confusion in such case, the list > iterator scope will be limited to the list_for_each_entry() loop. > > In preparation to limiting scope of the list iterator to the list traversal > loop, use a dedicated pointer to point to the found request object [1]. > > Link: https://lore.kernel.org/all/YhdfEIwI4EdtHdym@xxxxxxxxx/ > Signed-off-by: Jakob Koschel <jakobkoschel@xxxxxxxxx> > --- > drivers/usb/gadget/udc/s3c-hsudc.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/drivers/usb/gadget/udc/s3c-hsudc.c b/drivers/usb/gadget/udc/s3c-hsudc.c > index 89f1f8c9f02e..bf803e013458 100644 > --- a/drivers/usb/gadget/udc/s3c-hsudc.c > +++ b/drivers/usb/gadget/udc/s3c-hsudc.c > @@ -877,7 +877,7 @@ static int s3c_hsudc_dequeue(struct usb_ep *_ep, struct usb_request *_req) > { > struct s3c_hsudc_ep *hsep = our_ep(_ep); > struct s3c_hsudc *hsudc = hsep->dev; > - struct s3c_hsudc_req *hsreq; > + struct s3c_hsudc_req *hsreq = NULL, *iter; > unsigned long flags; > > hsep = our_ep(_ep); > @@ -886,11 +886,13 @@ static int s3c_hsudc_dequeue(struct usb_ep *_ep, struct usb_request *_req) > > spin_lock_irqsave(&hsudc->lock, flags); > > - list_for_each_entry(hsreq, &hsep->queue, queue) { > - if (&hsreq->req == _req) > - break; > + list_for_each_entry(iter, &hsep->queue, queue) { > + if (&iter->req != _req) > + continue; > + hsreq = iter; > + break; You have in the loop both continue and break, looks a bit complicated. Why not simpler: if (&iter->req == _req) { hsreq = iter; break; } ? Less code, typical (expected) code flow when looking for something in the list. Is your approach related to some speculative execution? > } > - if (&hsreq->req != _req) { > + if (!hsreq) { > spin_unlock_irqrestore(&hsudc->lock, flags); > return -EINVAL; > } Best regards, Krzysztof
