> On 9. Mar 2022, at 18:25, Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxxxxx> wrote: > > 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; > } Ultimately I changed this based on the feedback from Linus (Link: https://lore.kernel.org/linux-kernel/CAHk-=wheru6rEfzC2wuO9k03PRF6s3nhxryCAnwR5bzKwPV2ww@xxxxxxxxxxxxxx/). > > ? > Less code, typical (expected) code flow when looking for something in > the list. Is your approach related to some speculative execution? no relation to speculative execution. I have no preference for one over the other, so I'll just change it to however it is desired. It would just be great to have a (somewhat) consistent way so I can prepare the rest of the patch sets accordingly. > >> } >> - if (&hsreq->req != _req) { >> + if (!hsreq) { >> spin_unlock_irqrestore(&hsudc->lock, flags); >> return -EINVAL; >> } > > > Best regards, > Krzysztof Jakob
