On 09/03/2022 18:36, Jakob Koschel wrote: > >> 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/). Not cleaner to me at all, but it's all personal opinion. :) >> >> ? >> 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. > Yeah, I understand. The code itself looks good, so: Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxxxxx> Best regards, Krzysztof
