Re: [Intel-gfx] [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body

[Dan Carpenter]

On Mon, Feb 28, 2022 at 10:20:28AM -0800, Joe Perches wrote:
> On Mon, 2022-02-28 at 14:24 +0300, Dan Carpenter wrote:
> 
> > a multi-line indent gets curly braces for readability even though
> > it's not required by C.  And then both sides would get curly braces.
> 
> That's more your personal preference than a coding style guideline.
> 

It's a USB patch.  I thought Greg prefered it that way.  Greg has some
specific style things which he likes and I have adopted and some I
pretend not to see.

regards,
dan carpenter

Re: [Intel-gfx] [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body

[Joe Perches]

On Mon, 2022-02-28 at 14:24 +0300, Dan Carpenter wrote:

> a multi-line indent gets curly braces for readability even though
> it's not required by C.  And then both sides would get curly braces.

That's more your personal preference than a coding style guideline.

Re: [Intel-gfx] [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body

[Jakob Koschel]

> On 28. Feb 2022, at 12:24, Dan Carpenter  wrote:
> 
> On Mon, Feb 28, 2022 at 12:08:17PM +0100, Jakob Koschel wrote:
>> diff --git a/drivers/usb/gadget/udc/at91_udc.c 
>> b/drivers/usb/gadget/udc/at91_udc.c
>> index 9040a0561466..0fd0307bc07b 100644
>> --- a/drivers/usb/gadget/udc/at91_udc.c
>> +++ b/drivers/usb/gadget/udc/at91_udc.c
>> @@ -150,13 +150,14 @@ static void proc_ep_show(struct seq_file *s, struct 
>> at91_ep *ep)
>>  if (list_empty (>queue))
>>  seq_printf(s, "\t(queue empty)\n");
>> 
>> -else list_for_each_entry (req, >queue, queue) {
>> -unsignedlength = req->req.actual;
>> +else
>> +list_for_each_entry(req, >queue, queue) {
>> +unsigned intlength = req->req.actual;
>> 
>> -seq_printf(s, "\treq %p len %d/%d buf %p\n",
>> ->req, length,
>> -req->req.length, req->req.buf);
>> -}
>> +seq_printf(s, "\treq %p len %d/%d buf %p\n",
>> +>req, length,
>> +req->req.length, req->req.buf);
>> +}
> 
> Don't make unrelated white space changes.  It just makes the patch
> harder to review.  As you're writing the patch make note of any
> additional changes and do them later in a separate patch.
> 
> Also a multi-line indent gets curly braces for readability even though
> it's not required by C.  And then both sides would get curly braces.
> 
>>  spin_unlock_irqrestore(>lock, flags);
>> }
>> 
>> @@ -226,7 +227,7 @@ static int proc_udc_show(struct seq_file *s, void 
>> *unused)
>> 
>>  if (udc->enabled && udc->vbus) {
>>  proc_ep_show(s, >ep[0]);
>> -list_for_each_entry (ep, >gadget.ep_list, ep.ep_list) {
>> +list_for_each_entry(ep, >gadget.ep_list, ep.ep_list) {
> 
> Another unrelated change.
> 
>>  if (ep->ep.desc)
>>  proc_ep_show(s, ep);
>>  }
> 
> 
> [ snip ]

Thanks for pointing out, I'll remove the changes here and note them down
to send them separately.

> 
>> diff --git a/drivers/usb/gadget/udc/net2272.c 
>> b/drivers/usb/gadget/udc/net2272.c
>> index 7c38057dcb4a..bb59200f1596 100644
>> --- a/drivers/usb/gadget/udc/net2272.c
>> +++ b/drivers/usb/gadget/udc/net2272.c
>> @@ -926,7 +926,8 @@ static int
>> net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>> {
>>  struct net2272_ep *ep;
>> -struct net2272_request *req;
>> +struct net2272_request *req = NULL;
>> +struct net2272_request *tmp;
>>  unsigned long flags;
>>  int stopped;
>> 
>> @@ -939,11 +940,13 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request 
>> *_req)
>>  ep->stopped = 1;
>> 
>>  /* make sure it's still queued on this endpoint */
>> -list_for_each_entry(req, >queue, queue) {
>> -if (>req == _req)
>> +list_for_each_entry(tmp, >queue, queue) {
>> +if (>req == _req) {
>> +req = tmp;
>>  break;
>> +}
>>  }
>> -if (>req != _req) {
>> +if (!req) {
>>  ep->stopped = stopped;
>>  spin_unlock_irqrestore(>dev->lock, flags);
>>  return -EINVAL;
>> @@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request 
>> *_req)
>>  dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name);
>>  net2272_done(ep, req, -ECONNRESET);
>>  }
>> -req = NULL;
> 
> Another unrelated change.  These are all good changes but send them as
> separate patches.

You are referring to the req = NULL, right?

I've changed the use of 'req' in the same function and assumed that I can
just remove the unnecessary statement. But if it's better to do separately
I'll do that.

> 
>>  ep->stopped = stopped;
>> 
>>  spin_unlock_irqrestore(>dev->lock, flags);
> 
> regards,
> dan carpenter

thanks,
Jakob Koschel

[Intel-gfx] [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body

[Jakob Koschel]

If the list representing the request queue does not contain the expected
request, the value of 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 list_for_each_entry() loop.

In preparation to limiting scope of a list iterator to the list traversal
loop, use a dedicated pointer to point to the found request object.

Signed-off-by: Jakob Koschel 
---
 drivers/usb/gadget/udc/aspeed-vhub/epn.c | 11 ++
 drivers/usb/gadget/udc/at91_udc.c| 26 ++--
 drivers/usb/gadget/udc/atmel_usba_udc.c  | 11 ++
 drivers/usb/gadget/udc/bdc/bdc_ep.c  | 11 +++---
 drivers/usb/gadget/udc/fsl_qe_udc.c  | 11 ++
 drivers/usb/gadget/udc/fsl_udc_core.c| 11 ++
 drivers/usb/gadget/udc/goku_udc.c| 11 ++
 drivers/usb/gadget/udc/gr_udc.c  | 11 ++
 drivers/usb/gadget/udc/lpc32xx_udc.c | 11 ++
 drivers/usb/gadget/udc/mv_u3d_core.c | 11 ++
 drivers/usb/gadget/udc/mv_udc_core.c | 11 ++
 drivers/usb/gadget/udc/net2272.c | 12 ++-
 drivers/usb/gadget/udc/net2280.c | 11 ++
 drivers/usb/gadget/udc/omap_udc.c| 11 ++
 drivers/usb/gadget/udc/pxa25x_udc.c  | 11 ++
 drivers/usb/gadget/udc/s3c-hsudc.c   | 11 ++
 drivers/usb/gadget/udc/udc-xilinx.c  | 11 ++
 17 files changed, 128 insertions(+), 75 deletions(-)

diff --git a/drivers/usb/gadget/udc/aspeed-vhub/epn.c 
b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
index 917892ca8753..cad874ee4472 100644
--- a/drivers/usb/gadget/udc/aspeed-vhub/epn.c
+++ b/drivers/usb/gadget/udc/aspeed-vhub/epn.c
@@ -466,19 +466,22 @@ static int ast_vhub_epn_dequeue(struct usb_ep* u_ep, 
struct usb_request *u_req)
 {
struct ast_vhub_ep *ep = to_ast_ep(u_ep);
struct ast_vhub *vhub = ep->vhub;
-   struct ast_vhub_req *req;
+   struct ast_vhub_req *req = NULL;
+   struct ast_vhub_req *tmp;
unsigned long flags;
int rc = -EINVAL;

spin_lock_irqsave(>lock, flags);

/* Make sure it's actually queued on this endpoint */
-   list_for_each_entry (req, >queue, queue) {
-   if (>req == u_req)
+   list_for_each_entry(tmp, >queue, queue) {
+   if (>req == u_req) {
+   req = tmp;
break;
+   }
}

-   if (>req == u_req) {
+   if (req) {
EPVDBG(ep, "dequeue req @%p active=%d\n",
   req, req->active);
if (req->active)
diff --git a/drivers/usb/gadget/udc/at91_udc.c 
b/drivers/usb/gadget/udc/at91_udc.c
index 9040a0561466..0fd0307bc07b 100644
--- a/drivers/usb/gadget/udc/at91_udc.c
+++ b/drivers/usb/gadget/udc/at91_udc.c
@@ -150,13 +150,14 @@ static void proc_ep_show(struct seq_file *s, struct 
at91_ep *ep)
if (list_empty (>queue))
seq_printf(s, "\t(queue empty)\n");

-   else list_for_each_entry (req, >queue, queue) {
-   unsignedlength = req->req.actual;
+   else
+   list_for_each_entry(req, >queue, queue) {
+   unsigned intlength = req->req.actual;

-   seq_printf(s, "\treq %p len %d/%d buf %p\n",
-   >req, length,
-   req->req.length, req->req.buf);
-   }
+   seq_printf(s, "\treq %p len %d/%d buf %p\n",
+   >req, length,
+   req->req.length, req->req.buf);
+   }
spin_unlock_irqrestore(>lock, flags);
 }

@@ -226,7 +227,7 @@ static int proc_udc_show(struct seq_file *s, void *unused)

if (udc->enabled && udc->vbus) {
proc_ep_show(s, >ep[0]);
-   list_for_each_entry (ep, >gadget.ep_list, ep.ep_list) {
+   list_for_each_entry(ep, >gadget.ep_list, ep.ep_list) {
if (ep->ep.desc)
proc_ep_show(s, ep);
}
@@ -704,7 +705,8 @@ static int at91_ep_queue(struct usb_ep *_ep,
 static int at91_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 {
struct at91_ep  *ep;
-   struct at91_request *req;
+   struct at91_request *req = NULL;
+   struct at91_request *tmp;
unsigned long   flags;
struct at91_udc *udc;

@@ -717,11 +719,13 @@ static int at91_ep_dequeue(struct usb_ep *_ep, struct 
usb_request *_req)
spin_lock_irqsave(>lock, flags);

/* make sure it's actually queued on this endpoint */
-   list_for_each_entry (req, >queue, queue) {
-   if (>req == _req)
+   list_for_each_entry(tmp, >queue, queue) {
+   if (>req == _req) {
+   req = tmp;
break;
+   }
}
-   if 

Re: [Intel-gfx] [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body

[Dan Carpenter]

On Mon, Feb 28, 2022 at 01:03:36PM +0100, Jakob Koschel wrote:
> >> @@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request 
> >> *_req)
> >>dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name);
> >>net2272_done(ep, req, -ECONNRESET);
> >>}
> >> -  req = NULL;
> > 
> > Another unrelated change.  These are all good changes but send them as
> > separate patches.
> 
> You are referring to the req = NULL, right?

Yes.

> 
> I've changed the use of 'req' in the same function and assumed that I can
> just remove the unnecessary statement. But if it's better to do separately
> I'll do that.
> 

These are all changes which made me pause during my review to figure out
why they were necessary.  The line between what is a related part of a
patch is a bit vague and some maintainers will ask you to add or subtract
from a patch depending on their individual tastes.  I don't really have
an exact answer, but I felt like this patch needs to be subtracted from.

Especially if there is a whole chunk of the patch which can be removed,
then to me, that obviously should be in a different patch.

regards,
dan carpenter

Re: [Intel-gfx] [PATCH 1/6] drivers: usb: remove usage of list iterator past the loop body

[Dan Carpenter]

On Mon, Feb 28, 2022 at 12:08:17PM +0100, Jakob Koschel wrote:
> diff --git a/drivers/usb/gadget/udc/at91_udc.c 
> b/drivers/usb/gadget/udc/at91_udc.c
> index 9040a0561466..0fd0307bc07b 100644
> --- a/drivers/usb/gadget/udc/at91_udc.c
> +++ b/drivers/usb/gadget/udc/at91_udc.c
> @@ -150,13 +150,14 @@ static void proc_ep_show(struct seq_file *s, struct 
> at91_ep *ep)
>   if (list_empty (>queue))
>   seq_printf(s, "\t(queue empty)\n");
> 
> - else list_for_each_entry (req, >queue, queue) {
> - unsignedlength = req->req.actual;
> + else
> + list_for_each_entry(req, >queue, queue) {
> + unsigned intlength = req->req.actual;
> 
> - seq_printf(s, "\treq %p len %d/%d buf %p\n",
> - >req, length,
> - req->req.length, req->req.buf);
> - }
> + seq_printf(s, "\treq %p len %d/%d buf %p\n",
> + >req, length,
> + req->req.length, req->req.buf);
> + }

Don't make unrelated white space changes.  It just makes the patch
harder to review.  As you're writing the patch make note of any
additional changes and do them later in a separate patch.

Also a multi-line indent gets curly braces for readability even though
it's not required by C.  And then both sides would get curly braces.

>   spin_unlock_irqrestore(>lock, flags);
>  }
> 
> @@ -226,7 +227,7 @@ static int proc_udc_show(struct seq_file *s, void *unused)
> 
>   if (udc->enabled && udc->vbus) {
>   proc_ep_show(s, >ep[0]);
> - list_for_each_entry (ep, >gadget.ep_list, ep.ep_list) {
> + list_for_each_entry(ep, >gadget.ep_list, ep.ep_list) {

Another unrelated change.

>   if (ep->ep.desc)
>   proc_ep_show(s, ep);
>   }


[ snip ]

> diff --git a/drivers/usb/gadget/udc/net2272.c 
> b/drivers/usb/gadget/udc/net2272.c
> index 7c38057dcb4a..bb59200f1596 100644
> --- a/drivers/usb/gadget/udc/net2272.c
> +++ b/drivers/usb/gadget/udc/net2272.c
> @@ -926,7 +926,8 @@ static int
>  net2272_dequeue(struct usb_ep *_ep, struct usb_request *_req)
>  {
>   struct net2272_ep *ep;
> - struct net2272_request *req;
> + struct net2272_request *req = NULL;
> + struct net2272_request *tmp;
>   unsigned long flags;
>   int stopped;
> 
> @@ -939,11 +940,13 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request 
> *_req)
>   ep->stopped = 1;
> 
>   /* make sure it's still queued on this endpoint */
> - list_for_each_entry(req, >queue, queue) {
> - if (>req == _req)
> + list_for_each_entry(tmp, >queue, queue) {
> + if (>req == _req) {
> + req = tmp;
>   break;
> + }
>   }
> - if (>req != _req) {
> + if (!req) {
>   ep->stopped = stopped;
>   spin_unlock_irqrestore(>dev->lock, flags);
>   return -EINVAL;
> @@ -954,7 +957,6 @@ net2272_dequeue(struct usb_ep *_ep, struct usb_request 
> *_req)
>   dev_dbg(ep->dev->dev, "unlink (%s) pio\n", _ep->name);
>   net2272_done(ep, req, -ECONNRESET);
>   }
> - req = NULL;

Another unrelated change.  These are all good changes but send them as
separate patches.

>   ep->stopped = stopped;
> 
>   spin_unlock_irqrestore(>dev->lock, flags);

regards,
dan carpenter