[PATCH 1/3] usb: ci_udc: set correct ep type in ep_enable()

[PATCH 1/3] usb: ci_udc: set correct ep type in ep_enable()

[Ye Li]

From: Xu Yang <xu.yang_2@nxp.com>

The Endpoint Type in ENDPTCTRL register should be correctly set
according to endpoint descriptor, not fixed to bulk. Otherwise,
unexpected behaviors may happen.

Fixes: 26cc5129ee64 ("USB: gadaget: add Marvell controller support")
Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
Signed-off-by: Ye Li <ye.li@nxp.com>
---
 drivers/usb/gadget/ci_udc.c | 16 +++++++++-------
 drivers/usb/gadget/ci_udc.h |  4 ++--
 2 files changed, 11 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index 4729570c525..a62e5ea5658 100644
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -333,16 +333,16 @@ static void request_complete_list(struct usb_ep *ep, struct list_head *list, int
 	}
 }
 
-static void ep_enable(int num, int in, int maxpacket)
+static void ep_enable(int num, int in, int type, int maxpacket)
 {
 	struct ci_udc *udc = (struct ci_udc *)controller.ctrl->hcor;
 	unsigned n;
 
 	n = readl(&udc->epctrl[num]);
 	if (in)
-		n |= (CTRL_TXE | CTRL_TXR | CTRL_TXT_BULK);
+		n |= (CTRL_TXE | CTRL_TXR | CTRL_TXT(type));
 	else
-		n |= (CTRL_RXE | CTRL_RXR | CTRL_RXT_BULK);
+		n |= (CTRL_RXE | CTRL_RXR | CTRL_RXT(type));
 
 	if (num != 0) {
 		struct ept_queue_head *head = ci_get_qh(num, in);
@@ -357,7 +357,7 @@ static int ci_ep_enable(struct usb_ep *ep,
 		const struct usb_endpoint_descriptor *desc)
 {
 	struct ci_ep *ci_ep = container_of(ep, struct ci_ep, ep);
-	int num, in;
+	int num, in, type;
 	num = desc->bEndpointAddress & USB_ENDPOINT_NUMBER_MASK;
 	in = (desc->bEndpointAddress & USB_DIR_IN) != 0;
 
@@ -366,6 +366,7 @@ static int ci_ep_enable(struct usb_ep *ep,
 		return -EBUSY;
 	}
 
+	type = usb_endpoint_type(desc);
 	ci_ep->desc = desc;
 	ep->desc = desc;
 
@@ -380,7 +381,7 @@ static int ci_ep_enable(struct usb_ep *ep,
 			ep->maxpacket = max;
 		}
 	}
-	ep_enable(num, in, ep->maxpacket);
+	ep_enable(num, in, type, ep->maxpacket);
 	DBG("%s: num=%d maxpacket=%d\n", __func__, num, ep->maxpacket);
 	return 0;
 }
@@ -783,7 +784,7 @@ static void handle_setup(void)
 	struct ept_queue_head *head;
 	struct usb_ctrlrequest r;
 	int status = 0;
-	int num, in, _num, _in, i;
+	int num, in, _num, _in, i, type;
 	char *buf;
 
 	ci_req = controller.ep0_req;
@@ -837,8 +838,9 @@ static void handle_setup(void)
 						& USB_ENDPOINT_NUMBER_MASK;
 				in = (ep->desc->bEndpointAddress
 						& USB_DIR_IN) != 0;
+				type = usb_endpoint_type(ep->desc);
 				if ((num == _num) && (in == _in)) {
-					ep_enable(num, in, ep->ep.maxpacket);
+					ep_enable(num, in, type, ep->ep.maxpacket);
 					usb_ep_queue(controller.gadget.ep0,
 							req, 0);
 					break;
diff --git a/drivers/usb/gadget/ci_udc.h b/drivers/usb/gadget/ci_udc.h
index 807f2084c1e..f1c4537859e 100644
--- a/drivers/usb/gadget/ci_udc.h
+++ b/drivers/usb/gadget/ci_udc.h
@@ -74,8 +74,8 @@ struct ci_udc {
 #define CTRL_TXR	(1 << 22)
 #define CTRL_RXE	(1 << 7)
 #define CTRL_RXR	(1 << 6)
-#define CTRL_TXT_BULK	(2 << 18)
-#define CTRL_RXT_BULK	(2 << 2)
+#define CTRL_TXT(t)	((t) << 18)
+#define CTRL_RXT(t)	((t) << 2)
 
 struct ci_req {
 	struct usb_request	req;
-- 
2.37.1

[PATCH 2/3] usb: ci_udc: Update usb request status

[Ye Li]

usb request status is not set in ci_udc driver. However
in ci_ep_dequeue, it checks the request status for executing
complete callback, so upper layer is not able to free these
enqueued requests when calling usb_ep_dequeue.

Signed-off-by: Ye Li <ye.li@nxp.com>
---
 drivers/usb/gadget/ci_udc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index a62e5ea5658..0baad83ef90 100644
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -680,6 +680,8 @@ static int ci_ep_queue(struct usb_ep *ep,
 	if (ret)
 		return ret;
 
+	req->status = -EINPROGRESS;
+
 	DBG("ept%d %s pre-queue req %p, buffer %p\n",
 	    num, in ? "in" : "out", ci_req, ci_req->hw_buf);
 	list_add_tail(&ci_req->queue, &ci_ep->queue);
@@ -754,6 +756,7 @@ static void handle_ep_complete(struct ci_ep *ci_ep)
 		ci_ep_submit_next_request(ci_ep);
 
 	ci_req->req.actual = ci_req->req.length - len;
+	ci_req->req.status = 0;
 	ci_debounce(ci_req, in);
 
 	DBG("ept%d %s req %p, complete %x\n",
-- 
2.37.1

[PATCH 3/3] usb: ci_udc: verify all dtds are inactive before completing request

[Ye Li]

From: Jason He <jason.he_1@nxp.com>

According to device mode spec, the ACTIVE status field of dtds should
be check to determine whether the transfers completed successfully.
However, this is not implemented in handle_ep_complete.
When two EPs are enabled and transferring, EPa requests with multiple dtds
and EPb request with one dtd. Irq is triggred on EPb. The udc_irq handler
finds both EPb's and EPa's ENDPTCOMPLETE=1 while not all of EPa's dtds
have been completed. Because ACTIVE status is not checked, this case
causes crash in ci_udc driver.

Signed-off-by: Jason He <jason.he_1@nxp.com>
Signed-off-by: Ye Li <ye.li@nxp.com>
---
 drivers/usb/gadget/ci_udc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
index 0baad83ef90..53796887dac 100644
--- a/drivers/usb/gadget/ci_udc.c
+++ b/drivers/usb/gadget/ci_udc.c
@@ -733,6 +733,17 @@ static void handle_ep_complete(struct ci_ep *ci_ep)
 	ci_invalidate_qtd(num);
 	ci_req = list_first_entry(&ci_ep->queue, struct ci_req, queue);
 
+	/* Check all dtd are completed, otherwise return for next irq process */
+	next_td = item;
+	for (j = 0; j < ci_req->dtd_count; j++) {
+		ci_invalidate_td(next_td);
+		if (next_td->info & INFO_ACTIVE)
+			return;
+		if (j != ci_req->dtd_count - 1)
+			next_td = (struct ept_queue_item *)(unsigned long)
+				next_td->next;
+	}
+
 	next_td = item;
 	len = 0;
 	for (j = 0; j < ci_req->dtd_count; j++) {
-- 
2.37.1

Re: [PATCH 1/3] usb: ci_udc: set correct ep type in ep_enable()

[Mattijs Korpershoek]

Hi Ye,

Thank you for the patch.

On Fri, May 22, 2026 at 15:55, Ye Li <ye.li@nxp.com> wrote:

> From: Xu Yang <xu.yang_2@nxp.com>
>
> The Endpoint Type in ENDPTCTRL register should be correctly set
> according to endpoint descriptor, not fixed to bulk. Otherwise,
> unexpected behaviors may happen.
>
> Fixes: 26cc5129ee64 ("USB: gadaget: add Marvell controller support")
> Signed-off-by: Xu Yang <xu.yang_2@nxp.com>
> Signed-off-by: Ye Li <ye.li@nxp.com>

Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>

> ---

Re: [PATCH 2/3] usb: ci_udc: Update usb request status

[Mattijs Korpershoek]

Hi Ye,

Thank you for the patch.

On Fri, May 22, 2026 at 15:55, Ye Li <ye.li@nxp.com> wrote:

> usb request status is not set in ci_udc driver. However
> in ci_ep_dequeue, it checks the request status for executing
> complete callback, so upper layer is not able to free these
> enqueued requests when calling usb_ep_dequeue.
>
> Signed-off-by: Ye Li <ye.li@nxp.com>

Fixes: 26cc5129ee64 ("USB: gadaget: add Marvell controller support")

Reviewed-by: Mattijs Korpershoek <mkorpershoek@kernel.org>

> ---
>  drivers/usb/gadget/ci_udc.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
> index a62e5ea5658..0baad83ef90 100644
> --- a/drivers/usb/gadget/ci_udc.c
> +++ b/drivers/usb/gadget/ci_udc.c
> @@ -680,6 +680,8 @@ static int ci_ep_queue(struct usb_ep *ep,
>  	if (ret)
>  		return ret;
>  
> +	req->status = -EINPROGRESS;
> +
>  	DBG("ept%d %s pre-queue req %p, buffer %p\n",
>  	    num, in ? "in" : "out", ci_req, ci_req->hw_buf);
>  	list_add_tail(&ci_req->queue, &ci_ep->queue);
> @@ -754,6 +756,7 @@ static void handle_ep_complete(struct ci_ep *ci_ep)
>  		ci_ep_submit_next_request(ci_ep);
>  
>  	ci_req->req.actual = ci_req->req.length - len;
> +	ci_req->req.status = 0;
>  	ci_debounce(ci_req, in);
>  
>  	DBG("ept%d %s req %p, complete %x\n",
> -- 
> 2.37.1

Re: [PATCH 3/3] usb: ci_udc: verify all dtds are inactive before completing request

[Mattijs Korpershoek]

Hi Ye,

Thank you for the patch.

On Fri, May 22, 2026 at 15:55, Ye Li <ye.li@nxp.com> wrote:

> From: Jason He <jason.he_1@nxp.com>
>
> According to device mode spec, the ACTIVE status field of dtds should
> be check to determine whether the transfers completed successfully.
> However, this is not implemented in handle_ep_complete.
> When two EPs are enabled and transferring, EPa requests with multiple dtds
> and EPb request with one dtd. Irq is triggred on EPb. The udc_irq handler
> finds both EPb's and EPa's ENDPTCOMPLETE=1 while not all of EPa's dtds
> have been completed. Because ACTIVE status is not checked, this case
> causes crash in ci_udc driver.
>
> Signed-off-by: Jason He <jason.he_1@nxp.com>
> Signed-off-by: Ye Li <ye.li@nxp.com>
> ---
>  drivers/usb/gadget/ci_udc.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/usb/gadget/ci_udc.c b/drivers/usb/gadget/ci_udc.c
> index 0baad83ef90..53796887dac 100644
> --- a/drivers/usb/gadget/ci_udc.c
> +++ b/drivers/usb/gadget/ci_udc.c
> @@ -733,6 +733,17 @@ static void handle_ep_complete(struct ci_ep *ci_ep)
>  	ci_invalidate_qtd(num);
>  	ci_req = list_first_entry(&ci_ep->queue, struct ci_req, queue);
>  
> +	/* Check all dtd are completed, otherwise return for next irq process */
> +	next_td = item;
> +	for (j = 0; j < ci_req->dtd_count; j++) {
> +		ci_invalidate_td(next_td);
> +		if (next_td->info & INFO_ACTIVE)
> +			return;
> +		if (j != ci_req->dtd_count - 1)
> +			next_td = (struct ept_queue_item *)(unsigned long)
> +				next_td->next;
> +	}

A very similar loop (that walks all the dtds) is just below:

> +
>  	next_td = item;
>  	len = 0;
>  	for (j = 0; j < ci_req->dtd_count; j++) {

Can't we merge both loops together?

> -- 
> 2.37.1