[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

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

[Ye Li]

Hi Mattijs,

On 6/16/2026 5:30 PM, Mattijs Korpershoek wrote:
> 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?
> 
This loop will free each dtd. If the ACTIVE flag is set in the middle of 
loop, previous dtds should NOT be freed since this transfer is not done. 
That's why we add a loop to check ACTIVE prior this loop.

If merge two loops together, another list to temporally record dtds for 
free is needed. Then have to traverse the list to free each one when no 
ACTIVE found. This traversing will add a loop too.

Best regards,
Ye Li

>> -- 
>> 2.37.1

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

[Mattijs Korpershoek via U-Boot]

Hi Ye,

On Fri, Jul 10, 2026 at 17:28, Ye Li <ye.li@oss.nxp.com> wrote:

> Hi Mattijs,
>
> On 6/16/2026 5:30 PM, Mattijs Korpershoek wrote:
>> 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?
>> 
> This loop will free each dtd. If the ACTIVE flag is set in the middle of 
> loop, previous dtds should NOT be freed since this transfer is not done. 
> That's why we add a loop to check ACTIVE prior this loop.
>
> If merge two loops together, another list to temporally record dtds for 
> free is needed. Then have to traverse the list to free each one when no 
> ACTIVE found. This traversing will add a loop too.
>
> Best regards,
> Ye Li

Ah yeah, you are right. Sorry about that.

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

>
>>> -- 
>>> 2.37.1

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

[Mattijs Korpershoek via U-Boot]

Hi,

On Fri, 22 May 2026 15:55:10 +0800, Ye Li wrote:
> The Endpoint Type in ENDPTCTRL register should be correctly set
> according to endpoint descriptor, not fixed to bulk. Otherwise,
> unexpected behaviors may happen.

Thanks, Applied to https://git.u-boot-project.org/u-boot/custodians/u-boot-dfu (u-boot-dfu)

[1/3] usb: ci_udc: set correct ep type in ep_enable()
      https://git.u-boot-project.org/u-boot/custodians/u-boot-dfu/-/commit/eed2ed75883bdf029be9325fb9dd9263d921372e
[2/3] usb: ci_udc: Update usb request status
      https://git.u-boot-project.org/u-boot/custodians/u-boot-dfu/-/commit/b79147bbd47cd2d50836dc0a57376f4820663637
[3/3] usb: ci_udc: verify all dtds are inactive before completing request
      https://git.u-boot-project.org/u-boot/custodians/u-boot-dfu/-/commit/b6056097247385c24c23ef39ebc04238336d11e2

--
Mattijs