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
From a7ed9cffbfc90371b570ebef698d96c39adbaf77 Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <joakim.tjernl...@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.tjernl...@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
From b98fa0dd384f17fee0c1283b91f855b97d1976f4 Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <joakim.tjernl...@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.tjernl...@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
From a90a89d06bd008f606404ec613b4f2343b9dda1a Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <joakim.tjernl...@infinera.com>
Date: Thu, 7 May 2020 22:35:14 +0200
Subject: [PATCH 3/5] fsl_ep_dequeue
Signed-off-by: Joakim Tjernlund <joakim.tjernl...@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
From b3f09747be2007be3a372fe80635b51df6ba71bd Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <joakim.tjernl...@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.tjernl...@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
From 17c684fdcd6152b7e504656b1711e24508c32f6e Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <joakim.tjernl...@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.tjernl...@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
