Re: bug: usb: gadget: FSL_UDC_CORE Corrupted request list leads to unrecoverable loop.

[Joakim Tjernlund <Joakim.Tjernlund@infinera.com>]

[-- Attachment #1: Type: text/plain, Size: 2314 bytes --]

On Fri, 2021-10-29 at 17:14 +0000, Eugene Bordenkircher wrote:
> Hello all,
> 
> We've discovered a situation where the FSL udc driver (drivers/usb/gadget/udc/fsl_udc_core.c) will enter a loop iterating over the request queue, but the queue has been corrupted at some point so it loops infinitely.  I believe we have narrowed into the offending code, but we are in need of assistance trying to find an appropriate fix for the problem.  The identified code appears to be in all versions of the Linux kernel the driver exists in.
> 
> The problem appears to be when handling a USB_REQ_GET_STATUS request.  The driver gets this request and then calls the ch9getstatus() function.  In this function, it starts a request by "borrowing" the per device status_req, filling it in, and then queuing it with a call to list_add_tail() to add the request to the endpoint queue.  Right before it exits the function however, it's calling ep0_prime_status(), which is filling out that same status_req structure and then queuing it with another call to list_add_tail() to add the request to the endpoint queue.  This adds two instances of the exact same LIST_HEAD to the endpoint queue, which breaks the list since the prev and next pointers end up pointing to the wrong things.  This ends up causing a hard loop the next time nuke() gets called, which happens on the next setup IRQ.
> 
> I'm not sure what the appropriate fix to this problem is, mostly due to my lack of expertise in USB and this driver stack.  The code has been this way in the kernel for a very long time, which suggests that it has been working, unless USB_REQ_GET_STATUS requests are never made.  This further suggests that there is something else going on that I don't understand.  Deleting the call to ep0_prime_status() and the following ep0stall() call appears, on the surface, to get the device working again, but may have side effects that I'm not seeing.
> 
> I'm hopeful someone in the community can help provide some information on what I may be missing or help come up with a solution to the problem.  A big thank you to anyone who would like to help out.
> 
> Eugene

Run into this to a while ago. Found the bug and a few more fixes.
This is against 4.19 so you may have to tweak them a bit.
Feel free to upstream them.

 Jocke 

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0005-fsl_udc_core-Init-max_pipes-for-reset_queues.patch --]
[-- Type: text/x-patch; name="0005-fsl_udc_core-Init-max_pipes-for-reset_queues.patch", Size: 989 bytes --]

From a7ed9cffbfc90371b570ebef698d96c39adbaf77 Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <joakim.tjernlund@infinera.com>
Date: Mon, 11 May 2020 11:18:14 +0200
Subject: [PATCH 5/5] fsl_udc_core: Init max_pipes for reset_queues()

Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
---
 drivers/usb/gadget/udc/fsl_udc_core.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c b/drivers/usb/gadget/udc/fsl_udc_core.c
index bd3825d9f1d2..92136dff8373 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -2441,6 +2441,7 @@ static int fsl_udc_probe(struct platform_device *pdev)
 	/* Get max device endpoints */
 	/* DEN is bidirectional ep number, max_ep doubles the number */
 	udc_controller->max_ep = (dccparams & DCCPARAMS_DEN_MASK) * 2;
+	udc_controller->max_pipes = udc_controller->max_ep;
 
 	udc_controller->irq = platform_get_irq(pdev, 0);
 	if (!udc_controller->irq) {
-- 
2.32.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0004-fsl_udc_stop-Use-list_for_each_entry_safe-when-delet.patch --]
[-- Type: text/x-patch; name="0004-fsl_udc_stop-Use-list_for_each_entry_safe-when-delet.patch", Size: 1422 bytes --]

From b98fa0dd384f17fee0c1283b91f855b97d1976f4 Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <joakim.tjernlund@infinera.com>
Date: Mon, 11 May 2020 10:38:07 +0200
Subject: [PATCH 4/5] fsl_udc_stop: Use list_for_each_entry_safe() when
 deleting

Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
---
 drivers/usb/gadget/udc/fsl_udc_core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c b/drivers/usb/gadget/udc/fsl_udc_core.c
index 4f835332af45..bd3825d9f1d2 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -1984,7 +1984,7 @@ static int fsl_udc_start(struct usb_gadget *g,
 /* Disconnect from gadget driver */
 static int fsl_udc_stop(struct usb_gadget *g)
 {
-	struct fsl_ep *loop_ep;
+	struct fsl_ep *loop_ep, *tmp_loop;
 	unsigned long flags;
 
 	if (!IS_ERR_OR_NULL(udc_controller->transceiver))
@@ -2002,8 +2002,8 @@ static int fsl_udc_stop(struct usb_gadget *g)
 	spin_lock_irqsave(&udc_controller->lock, flags);
 	udc_controller->gadget.speed = USB_SPEED_UNKNOWN;
 	nuke(&udc_controller->eps[0], -ESHUTDOWN);
-	list_for_each_entry(loop_ep, &udc_controller->gadget.ep_list,
-			ep.ep_list)
+	list_for_each_entry_safe(loop_ep, tmp_loop, &udc_controller->gadget.ep_list,
+				 ep.ep_list)
 		nuke(loop_ep, -ESHUTDOWN);
 	spin_unlock_irqrestore(&udc_controller->lock, flags);
 
-- 
2.32.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-fsl_ep_dequeue.patch --]
[-- Type: text/x-patch; name="0003-fsl_ep_dequeue.patch", Size: 1007 bytes --]

From a90a89d06bd008f606404ec613b4f2343b9dda1a Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <joakim.tjernlund@infinera.com>
Date: Thu, 7 May 2020 22:35:14 +0200
Subject: [PATCH 3/5] fsl_ep_dequeue

Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
---
 drivers/usb/gadget/udc/fsl_udc_core.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c b/drivers/usb/gadget/udc/fsl_udc_core.c
index 4b1591fa2e1c..4f835332af45 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -977,7 +977,13 @@ static int fsl_ep_dequeue(struct usb_ep *_ep, struct usb_request *_req)
 
 			/* prime with dTD of next request */
 			fsl_prime_ep(ep, next_req->head);
-		}
+		} else {
+			struct ep_queue_head *qh;
+
+			qh = ep->qh;
+			qh->next_dtd_ptr = 1;
+			qh->size_ioc_int_sts = 0;
+ 		}
 	/* The request hasn't been processed, patch up the TD chain */
 	} else {
 		struct fsl_req *prev_req;
-- 
2.32.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0002-fsl_udc-import-build_dtd-fixes.patch --]
[-- Type: text/x-patch; name="0002-fsl_udc-import-build_dtd-fixes.patch", Size: 2239 bytes --]

From b3f09747be2007be3a372fe80635b51df6ba71bd Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <joakim.tjernlund@infinera.com>
Date: Thu, 7 May 2020 22:32:26 +0200
Subject: [PATCH 2/5] fsl_udc: import build_dtd fixes

Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
---
 drivers/usb/gadget/udc/fsl_udc_core.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c b/drivers/usb/gadget/udc/fsl_udc_core.c
index 2546bc28f42a..4b1591fa2e1c 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -774,12 +774,20 @@ static void fsl_queue_td(struct fsl_ep *ep, struct fsl_req *req)
 static struct ep_td_struct *fsl_build_dtd(struct fsl_req *req, unsigned *length,
 		dma_addr_t *dma, int *is_last, gfp_t gfp_flags)
 {
-	u32 swap_temp;
+	u32 swap_temp, mult = 0;
 	struct ep_td_struct *dtd;
+	struct ep_queue_head *dqh;
 
 	/* how big will this transfer be? */
-	*length = min(req->req.length - req->req.actual,
-			(unsigned)EP_MAX_LENGTH_TRANSFER);
+	if (usb_endpoint_xfer_isoc(req->ep->ep.desc)) {
+		dqh = req->ep->qh;
+		mult = (dqh->max_pkt_length >> EP_QUEUE_HEAD_MULT_POS)
+			& 0x3;
+		*length = min(req->req.length - req->req.actual,
+			      (unsigned)(mult * req->ep->ep.maxpacket));
+	} else
+		*length = min(req->req.length - req->req.actual,
+			      (unsigned)EP_MAX_LENGTH_TRANSFER);
 
 	dtd = dma_pool_alloc(udc_controller->td_pool, gfp_flags, dma);
 	if (dtd == NULL)
@@ -794,6 +802,7 @@ static struct ep_td_struct *fsl_build_dtd(struct fsl_req *req, unsigned *length,
 	/* Init all of buffer page pointers */
 	swap_temp = (u32) (req->req.dma + req->req.actual);
 	dtd->buff_ptr0 = cpu_to_hc32(swap_temp);
+	swap_temp &= ~0xFFF;
 	dtd->buff_ptr1 = cpu_to_hc32(swap_temp + 0x1000);
 	dtd->buff_ptr2 = cpu_to_hc32(swap_temp + 0x2000);
 	dtd->buff_ptr3 = cpu_to_hc32(swap_temp + 0x3000);
@@ -820,6 +829,7 @@ static struct ep_td_struct *fsl_build_dtd(struct fsl_req *req, unsigned *length,
 	/* Enable interrupt for the last dtd of a request */
 	if (*is_last && !req->req.no_interrupt)
 		swap_temp |= DTD_IOC;
+	swap_temp |= mult << 10;
 
 	dtd->size_ioc_sts = cpu_to_hc32(swap_temp);
 
-- 
2.32.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #6: 0001-ch9getstatus-ep0_prime_status-fixes-RND-28770.patch --]
[-- Type: text/x-patch; name="0001-ch9getstatus-ep0_prime_status-fixes-RND-28770.patch", Size: 4367 bytes --]

From 17c684fdcd6152b7e504656b1711e24508c32f6e Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <joakim.tjernlund@infinera.com>
Date: Fri, 8 May 2020 17:12:53 +0200
Subject: [PATCH 1/5] ch9getstatus/ep0_prime_status, fixes RND-28770

USB driver added the same req twice to the same list.
This cause a endless loop while in IRQ context.
Fix by importing code from mv_udc_core.c, its sister driver.

Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com>
---
 drivers/usb/gadget/udc/fsl_udc_core.c | 56 ++++++++++-----------------
 1 file changed, 21 insertions(+), 35 deletions(-)

diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c b/drivers/usb/gadget/udc/fsl_udc_core.c
index 367697144cda..2546bc28f42a 100644
--- a/drivers/usb/gadget/udc/fsl_udc_core.c
+++ b/drivers/usb/gadget/udc/fsl_udc_core.c
@@ -1266,7 +1266,7 @@ static void ep0stall(struct fsl_udc *udc)
 }
 
 /* Prime a status phase for ep0 */
-static int ep0_prime_status(struct fsl_udc *udc, int direction)
+static int ep0_prime_status(struct fsl_udc *udc, int direction, u16 status, bool empty)
 {
 	struct fsl_req *req = udc->status_req;
 	struct fsl_ep *ep;
@@ -1281,8 +1281,14 @@ static int ep0_prime_status(struct fsl_udc *udc, int direction)
 	if (udc->ep0_state != DATA_STATE_XMIT)
 		udc->ep0_state = WAIT_FOR_OUT_STATUS;
 
+	/* fill in the reqest structure */
+	if (empty == false) {
+		*((u16 *) req->req.buf) = cpu_to_le16(status);
+		req->req.length = 2;
+	} else
+		req->req.length = 0;
+
 	req->ep = ep;
-	req->req.length = 0;
 	req->req.status = -EINPROGRESS;
 	req->req.actual = 0;
 	req->req.complete = fsl_noop_complete;
@@ -1292,14 +1298,19 @@ static int ep0_prime_status(struct fsl_udc *udc, int direction)
 	if (ret)
 		return ret;
 
+	ret = -ENOMEM;
 	if (fsl_req_to_dtd(req, GFP_ATOMIC) == 0)
 		fsl_queue_td(ep, req);
 	else
-		return -ENOMEM;
+		goto out;
 
 	list_add_tail(&req->queue, &ep->queue);
 
 	return 0;
+out:
+	usb_gadget_unmap_request(&udc->gadget, &req->req, ep_is_in(ep));
+
+	return ret;
 }
 
 static void udc_reset_ep_queue(struct fsl_udc *udc, u8 pipe)
@@ -1320,7 +1331,7 @@ static void ch9setaddress(struct fsl_udc *udc, u16 value, u16 index, u16 length)
 	/* Update usb state */
 	udc->usb_state = USB_STATE_ADDRESS;
 	/* Status phase */
-	if (ep0_prime_status(udc, EP_DIR_IN))
+	if (ep0_prime_status(udc, EP_DIR_IN, 0, true))
 		ep0stall(udc);
 }
 
@@ -1331,9 +1342,7 @@ static void ch9getstatus(struct fsl_udc *udc, u8 request_type, u16 value,
 		u16 index, u16 length)
 {
 	u16 tmp = 0;		/* Status, cpu endian */
-	struct fsl_req *req;
 	struct fsl_ep *ep;
-	int ret;
 
 	ep = &udc->eps[0];
 
@@ -1358,33 +1367,10 @@ static void ch9getstatus(struct fsl_udc *udc, u8 request_type, u16 value,
 				<< USB_ENDPOINT_HALT;
 	}
 
-	udc->ep0_dir = USB_DIR_IN;
-	/* Borrow the per device status_req */
-	req = udc->status_req;
-	/* Fill in the reqest structure */
-	*((u16 *) req->req.buf) = cpu_to_le16(tmp);
-
-	req->ep = ep;
-	req->req.length = 2;
-	req->req.status = -EINPROGRESS;
-	req->req.actual = 0;
-	req->req.complete = fsl_noop_complete;
-	req->dtd_count = 0;
-
-	ret = usb_gadget_map_request(&ep->udc->gadget, &req->req, ep_is_in(ep));
-	if (ret)
-		goto stall;
-
-	/* prime the data phase */
-	if ((fsl_req_to_dtd(req, GFP_ATOMIC) == 0))
-		fsl_queue_td(ep, req);
-	else			/* no mem */
-		goto stall;
-
-	list_add_tail(&req->queue, &ep->queue);
-	udc->ep0_state = DATA_STATE_XMIT;
-	if (ep0_prime_status(udc, EP_DIR_OUT))
+	if (ep0_prime_status(udc, EP_DIR_OUT, tmp, false))
 		ep0stall(udc);
+	else
+		udc->ep0_state = DATA_STATE_XMIT;
 
 	return;
 stall:
@@ -1465,7 +1451,7 @@ __acquires(udc->lock)
 			break;
 
 		if (rc == 0) {
-			if (ep0_prime_status(udc, EP_DIR_IN))
+			if (ep0_prime_status(udc, EP_DIR_IN, 0, true))
 				ep0stall(udc);
 		}
 		if (ptc) {
@@ -1501,7 +1487,7 @@ __acquires(udc->lock)
 		 * See 2.0 Spec chapter 8.5.3.3 for detail.
 		 */
 		if (udc->ep0_state == DATA_STATE_XMIT)
-			if (ep0_prime_status(udc, EP_DIR_OUT))
+			if (ep0_prime_status(udc, EP_DIR_OUT, 0, true))
 				ep0stall(udc);
 
 	} else {
@@ -1537,7 +1523,7 @@ static void ep0_req_complete(struct fsl_udc *udc, struct fsl_ep *ep0,
 		break;
 	case DATA_STATE_RECV:
 		/* send status phase */
-		if (ep0_prime_status(udc, EP_DIR_IN))
+		if (ep0_prime_status(udc, EP_DIR_IN, 0, true))
 			ep0stall(udc);
 		break;
 	case WAIT_FOR_OUT_STATUS:
-- 
2.32.0