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

[Mattijs Korpershoek via U-Boot <u-boot@lists.u-boot-project.org>]

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