Re: [PATCH 2/3] usb: dwc2: gadget: restart descriptor DMA bulk and interrupt endpoints after BNA

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] usb: dwc2: gadget: wrong DMA offset in
  dwc2_gadget_handle_nonisoc_bna()
- [Medium] usb: dwc2: delayed EPDISBLD after BNA skips restart or
  flushes active TxFIFO
--

commit 11858f40d5a168bb783c631975d168ab78609905
Author: Vadim Yuzifovich <vadim@yuzi.co>

usb: dwc2: gadget: restart descriptor DMA bulk and interrupt endpoints after BNA

This commit adds a BNA interrupt handler for non-isochronous endpoints in
descriptor DMA mode to restart the current request when the endpoint is
disabled and not halted.

> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index b0e129061abb6..046b5b04503bd 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -2305,6 +2305,45 @@ static void dwc2_gadget_handle_isoc_bna(struct dwc2_hsotg_ep *hs_ep)
>  	dwc2_gadget_restart_isoc_out_ddma(hs_ep);
>  }
>  
> +/*
> + * dwc2_gadget_handle_nonisoc_bna - handle BNA interrupt for bulk and
> + * interrupt endpoints in descriptor DMA mode.
> + * @hs_ep: The endpoint, not EP0 and not isochronous.
> + * @ints: The endpoint interrupts that came with the BNA, for the log.
> + *
> + * The core raises BNA and disables the endpoint when the descriptor it
> + * fetches is not marked host ready. The chain of a bulk or interrupt
> + * endpoint is complete before the endpoint is enabled, and afterwards the
> + * descriptor reads host ready, so the core read it before the CPU's writes
> + * to it reached memory. Nothing was transferred and no XferCompl
> + * follows: the request stays current, the ones queued behind it never
> + * start, and the function waits until the host gives up and resets the
> + * port. Start the request again; that fills the descriptor anew.
> + */
> +static void dwc2_gadget_handle_nonisoc_bna(struct dwc2_hsotg_ep *hs_ep,
> +					   u32 ints)
> +{
> +	struct dwc2_hsotg *hsotg = hs_ep->parent;
> +	struct dwc2_hsotg_req *hs_req = hs_ep->req;
> +	u32 epctl_reg = hs_ep->dir_in ? DIEPCTL(hs_ep->index) :
> +					DOEPCTL(hs_ep->index);
> +	u32 epctl = dwc2_readl(hsotg, epctl_reg);
> +	u32 status = hs_ep->desc_list ? READ_ONCE(hs_ep->desc_list[0].status) : 0;
> +	bool restart = hs_req && !hs_ep->halted &&
> +		       !(epctl & (DXEPCTL_EPENA | DXEPCTL_STALL));

[Severity: Medium]
What happens if DXEPINT_BNAINTR is serviced before the core finishes
clearing DXEPCTL_EPENA and latching DXEPINT_EPDISBLD?

As noted in 540ccba08599 ("usb: dwc2: gadget: Start and complete DDMA isoc
transfers") and commit "usb: dwc2: gadget: restart descriptor DMA
isochronous OUT endpoints after BNA", the endpoint disable can complete
slightly after DXEPINT_BNAINTR is first read:

1. If DXEPCTL_EPENA is still set when dwc2_gadget_handle_nonisoc_bna()
   reads epctl_reg, restart is false and the request is not restarted.
   When DXEPINT_EPDISBLD arrives later, dwc2_gadget_handle_ep_disabled()
   returns early for non-isochronous endpoints, leaving the endpoint stuck.

2. If DXEPCTL_EPENA clears after dwc2_hsotg_epint() reads and clears
   DIEPINT, but before dwc2_gadget_handle_nonisoc_bna() reads DIEPCTL,
   dwc2_gadget_handle_nonisoc_bna() restarts the IN endpoint while
   DXEPINT_EPDISBLD remains pending in DIEPINT.  On the next interrupt,
   dwc2_gadget_handle_ep_disabled() unconditionally calls
   dwc2_hsotg_txfifo_flush(), flushing the TxFIFO of the newly restarted
   IN transfer.

> +
> +	dev_warn_ratelimited(hsotg->dev,
> +			     "%s: BNA, DxEPINT %08x DxEPCTL %08x descriptor %08x, %s\n",
> +			     hs_ep->name, ints, epctl, status,
> +			     restart ? "request restarted" : "not restarted");
> +
> +	if (!restart)
> +		return;
> +
> +	hs_ep->req = NULL;
> +	dwc2_hsotg_start_req(hsotg, hs_ep, hs_req, false);
> +}

[Severity: Medium]
Does passing continuing = false to dwc2_hsotg_start_req() corrupt the DMA
offset when a BNA occurs on a continuation chunk of a multi-part request
(where hs_req->req.actual > 0)?

When a request exceeds dwc2_gadget_get_chain_limit(),
dwc2_hsotg_complete_in() or dwc2_hsotg_handle_outdone() starts the next
chunk with dwc2_hsotg_start_req(hsotg, hs_ep, hs_req, true).  In
dwc2_hsotg_start_req(), length and hs_ep->last_load always account for
ureq->actual, but the DDMA buffer offset only advances when continuing is
true:

drivers/usb/dwc2/gadget.c:dwc2_hsotg_start_req() {
    ...
	length = ureq->length - ureq->actual;
    ...
	if (using_desc_dma(hsotg)) {
		u32 offset = 0;
    ...
		if (continuing)
			offset = ureq->actual;

		/* Fill DDMA chain entries */
		dwc2_gadget_config_nonisoc_xfer_ddma(hs_ep, ureq->dma + offset,
						     length);
    ...
}

Restarting with continuing = false programs the descriptors at ureq->dma
instead of ureq->dma + ureq->actual for the remaining length, re-sending or
overwriting the start of the buffer.  Passing hs_req->req.actual > 0 as
continuing (and keeping hs_ep->req set when continuing is true) would
preserve the offset.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261115185335.782678-1-vadim@yuzi.co?part=2