On 28. Feb 2022, at 12:24, Dan Carpenter [off-list ref] wrote:

On Mon, Feb 28, 2022 at 12:08:17PM +0100, Jakob Koschel wrote:

quoted

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 (&ep->queue))
		seq_printf(s, "\t(queue empty)\n");

-	else list_for_each_entry (req, &ep->queue, queue) {
-		unsigned	length = req->req.actual;
+	else
+		list_for_each_entry(req, &ep->queue, queue) {
+			unsigned int	length = req->req.actual;

-		seq_printf(s, "\treq %p len %d/%d buf %p\n",
-				&req->req, length,
-				req->req.length, req->req.buf);
-	}
+			seq_printf(s, "\treq %p len %d/%d buf %p\n",
+					&req->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.

quoted

	spin_unlock_irqrestore(&udc->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, &udc->ep[0]);
-		list_for_each_entry (ep, &udc->gadget.ep_list, ep.ep_list) {
+		list_for_each_entry(ep, &udc->gadget.ep_list, ep.ep_list) {

Another unrelated change.

quoted

			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.

quoted

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, &ep->queue, queue) {
-		if (&req->req == _req)
+	list_for_each_entry(tmp, &ep->queue, queue) {
+		if (&tmp->req == _req) {
+			req = tmp;
			break;
+		}
	}
-	if (&req->req != _req) {
+	if (!req) {
		ep->stopped = stopped;
		spin_unlock_irqrestore(&ep->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.

quoted

	ep->stopped = stopped;

	spin_unlock_irqrestore(&ep->dev->lock, flags);

regards,
dan carpenter

thanks,
Jakob Koschel