Hi Ye, On Fri, Jul 10, 2026 at 17:28, Ye Li <[email protected]> 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 <[email protected]> wrote: >> >>> From: Jason He <[email protected]> >>> >>> 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 <[email protected]> >>> Signed-off-by: Ye Li <[email protected]> >>> --- >>> 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 <[email protected]> > >>> -- >>> 2.37.1
