[PATCH 0/3] usb: dwc2: gadget: descriptor DMA fixes for BNA, found on Sophgo SG2002

[PATCH 0/3] usb: dwc2: gadget: descriptor DMA fixes for BNA, found on Sophgo SG2002

[Vadim Yuzifovich]

These are the three patches offered in [1]. Chen-Yu asked for them as a
series and offered to test on a Milk-V Duo 256M.

We run a Sophgo SG2002 (T-Head C906, DWC_otg 4.20a) as a peripheral with
g_dma and g_dma_desc both on, as a composite gadget: mass storage, HID,
ACM and UAC1. Descriptor DMA works there, but only with these changes.

Patch 1: an isochronous OUT endpoint that takes a BNA is never restarted,
and the BNA interrupt then repeats until the host resets the port.

Patch 2: a bulk or interrupt endpoint that takes a BNA is only logged at
debug level. It stays disabled with its request current, and the
function waits until the host times out.

Patch 3: why bulk endpoints took BNAs at all on this SoC. The core
fetches the descriptor before the CPU's stores to it have reached DRAM,
although writel() orders them before the enable. Reading the last
descriptor back between two full barriers closes the window. The
measurements are in the patch.

Patches 1 and 2 look generic to dwc2 descriptor DMA. Patch 3 is the one
I am least sure where to put. The effect was seen on the C906 only, and
the read-back costs one uncached load per transfer on every platform. If
it should sit behind a quirk or a core parameter, or belongs in the
RISC-V T-Head errata code instead of dwc2, I will redo it that way.

Known limit of patch 2: it starts the current request again from its
beginning, which is right when nothing was transferred. That holds for a
chain of one descriptor, the only case we hit. A BNA in the middle of a
scatter-gather chain was not exercised.

Testing. The series as posted here, applied to 7.3, ran on the SG2002
under the load described in patch 3: 1.47 million TEST UNIT READY in
8 minutes (about 3060 a second) with a 1080p60 H.264 encode running, no
BNA and no port reset. The viewer's network link dropped several times,
so the encode ran for about three quarters of that time. An earlier
form of the patches, which our tree carries, saw no BNA in 6.2 million
transfer starts and none in a 60 minute run at 1080p60, where there were
about 6200 a minute before. In that form patch 2 also waited for the
core's write-back of the descriptor at XferCompl and changed the AHBErr
message; both are dropped here, the wait because it never triggered.
On 7.4-rc1 the series is build tested only (x86_64, W=1 clean for
gadget.c) and passes checkpatch --strict.

To try it on a CV18XX/SG200X board, gadget DMA has to be allowed, since
dwc2_set_cv1800_params() turns it off:

  --- a/drivers/usb/dwc2/params.c
  +++ b/drivers/usb/dwc2/params.c
  @@ static void dwc2_set_cv1800_params(struct dwc2_hsotg *hsotg)
   	p->host_dma = false;
  -	p->g_dma = false;
   	p->speed = DWC2_SPEED_PARAM_HIGH;

and "usb: dwc2: Disable gadget descriptor DMA for Sophgo CV18XX/SG200X"
must not be applied. That change is not part of this series. Whether
DMA should become the default on these SoCs is a separate question, and
host DMA was not tested.

An AI coding assistant (Claude) was used in the debugging sessions and
in drafting the code and the changelogs. The measurements were made on
real hardware, and I have reviewed the patches.

[1] https://lore.kernel.org/r/CAH_=YdDCDyFBn2k18w8Ck5jrmHHZme7TuAA71hSgcAa9ZPb38Q@mail.gmail.com

Vadim Yuzifovich (3):
  usb: dwc2: gadget: restart descriptor DMA isochronous OUT endpoints
    after BNA
  usb: dwc2: gadget: restart descriptor DMA bulk and interrupt endpoints
    after BNA
  usb: dwc2: gadget: make descriptors visible before the endpoint is
    enabled

 drivers/usb/dwc2/gadget.c | 112 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)


base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
-- 
2.56.0

[PATCH 1/3] usb: dwc2: gadget: restart descriptor DMA isochronous OUT endpoints after BNA

[Vadim Yuzifovich]

In descriptor DMA mode the core disables an isochronous endpoint when it
reaches a descriptor that is not ready (BNA), for example after an
interrupt latency longer than the queued requests cover.
dwc2_gadget_handle_isoc_bna() resets the endpoint and leaves the restart
to the next OUTTKNEPDIS. Two things keep that from happening.

First, the endpoint-disabled interrupt that comes with the BNA runs
dwc2_gadget_handle_ep_disabled(), whose isochronous part is written for
buffer DMA: it completes requests frame by frame and advances
target_frame up to the current frame. Run after the BNA reset, it moves
target_frame off TARGET_FRAME_INITIAL, and the OUT token handler then
takes the chain for running and never restarts it.

Second, after the BNA the core raises BNA again for the disabled
endpoint as fast as the handler clears it, and no OUTTKNEPDIS comes.

On a Sophgo SG2002 (DWC_otg 4.20a) with a UAC1 capture endpoint and
eight requests queued, one 8 ms interrupt latency (a line printed on the
115200 baud serial console) caused one BNA. The endpoint stayed
disabled and the BNA interrupt kept the CPU busy until the host gave up
and reset the port, which took the other functions of the composite
gadget down as well.

In descriptor DMA mode, do not run the buffer DMA completion for a
disabled isochronous endpoint; put it back in its initial state. For an
OUT endpoint, build the chain again from the queue and enable it at
once, both after the BNA and after an endpoint-disabled interrupt that
finds it disabled with requests queued. IN endpoints keep restarting on
the next NAK.

Signed-off-by: Vadim Yuzifovich <vadim@yuzi.co>
---
 drivers/usb/dwc2/gadget.c | 45 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 6c8a9f05f..b0e129061 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2259,6 +2259,28 @@ static void dwc2_gadget_complete_isoc_request_ddma(struct dwc2_hsotg_ep *hs_ep)
 	}
 }
 
+/*
+ * dwc2_gadget_restart_isoc_out_ddma - build the chain of a disabled ISOC OUT
+ * endpoint again from its queue and enable it.
+ * @hs_ep: The isochronous OUT endpoint, in its initial state.
+ *
+ * After a BNA the core keeps raising BNA for the endpoint, as fast as the
+ * handler clears it, and no OUTTKNEPDIS comes to restart the chain. So the
+ * chain is restarted here at once.
+ */
+static void dwc2_gadget_restart_isoc_out_ddma(struct dwc2_hsotg_ep *hs_ep)
+{
+	struct dwc2_hsotg *hsotg = hs_ep->parent;
+
+	if (hs_ep->dir_in || list_empty(&hs_ep->queue) ||
+	    hs_ep->target_frame != TARGET_FRAME_INITIAL ||
+	    dwc2_readl(hsotg, DOEPCTL(hs_ep->index)) & DXEPCTL_EPENA)
+		return;
+
+	hs_ep->target_frame = dwc2_hsotg_read_frameno(hsotg);
+	dwc2_gadget_start_isoc_ddma(hs_ep);
+}
+
 /*
  * dwc2_gadget_handle_isoc_bna - handle BNA interrupt for ISOC.
  * @hs_ep: The isochronous endpoint.
@@ -2279,6 +2301,8 @@ static void dwc2_gadget_handle_isoc_bna(struct dwc2_hsotg_ep *hs_ep)
 	hs_ep->target_frame = TARGET_FRAME_INITIAL;
 	hs_ep->next_desc = 0;
 	hs_ep->compl_desc = 0;
+
+	dwc2_gadget_restart_isoc_out_ddma(hs_ep);
 }
 
 /**
@@ -2902,6 +2926,27 @@ static void dwc2_gadget_handle_ep_disabled(struct dwc2_hsotg_ep *hs_ep)
 	if (!hs_ep->isochronous)
 		return;
 
+	/*
+	 * In descriptor DMA the requests sit in the descriptor chain rather
+	 * than one per frame, so the frame by frame completion below does not
+	 * apply. The core disables an isochronous endpoint by itself when it
+	 * meets a descriptor that was not filled in time (BNA). Put such an
+	 * endpoint back in its initial state and build the chain again from
+	 * the queue: at once for OUT, on the next NAK for IN. The loop below
+	 * would move target_frame off TARGET_FRAME_INITIAL, which tells the
+	 * token handlers that a chain runs: the endpoint then stays disabled
+	 * for good, and every request queued again goes into a dead chain.
+	 */
+	if (using_desc_dma(hsotg)) {
+		if (!(dwc2_readl(hsotg, epctl_reg) & DXEPCTL_EPENA)) {
+			hs_ep->target_frame = TARGET_FRAME_INITIAL;
+			hs_ep->next_desc = 0;
+			hs_ep->compl_desc = 0;
+			dwc2_gadget_restart_isoc_out_ddma(hs_ep);
+		}
+		return;
+	}
+
 	if (list_empty(&hs_ep->queue)) {
 		dev_dbg(hsotg->dev, "%s: complete_ep 0x%p, ep->queue empty!\n",
 			__func__, hs_ep);
-- 
2.56.0

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

[Vadim Yuzifovich]

In descriptor DMA mode the core raises BNA and disables an endpoint when
the descriptor it fetches is not marked host ready. For isochronous
endpoints dwc2 handles that; for bulk and interrupt endpoints it only
logs the interrupt at debug level. Nothing was transferred and no
XferCompl follows, so the request stays current, the requests queued
behind it never start, and the function driver waits for a completion
that never comes.

On a Sophgo SG2002 (DWC_otg 4.20a) with a video pipeline loading the
DRAM this happened every minute or two to the bulk IN endpoint of
f_mass_storage: debugfs showed the endpoint disabled with an 18-byte
REQUEST SENSE reply current and nothing done, the 13-byte CSW queued
behind it, and f_mass_storage asleep waiting for its buffer. The host's
command timed out after 30 s and it reset the port. Read in the BNA
handler, the descriptor is 0x0e000012: host ready, last, short,
interrupt on completion, 18 bytes. The chain is written completely
before the endpoint is enabled, so the core fetched the descriptor
before the CPU's stores to it reached memory. The next patch closes
that window on this SoC, but a BNA on such an endpoint should not leave
it dead on any platform.

When a bulk or interrupt endpoint other than EP0 raises BNA, is disabled
and not halted, start its current request again, which fills the
descriptor anew. Report the BNA with a rate limited warning instead of
at debug level: unlike on an isochronous endpoint it is not expected.

Signed-off-by: Vadim Yuzifovich <vadim@yuzi.co>
---
 drivers/usb/dwc2/gadget.c | 41 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index b0e129061..046b5b045 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));
+
+	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);
+}
+
 /**
  * dwc2_hsotg_rx_data - receive data from the FIFO for an endpoint
  * @hsotg: The device state.
@@ -3257,6 +3296,8 @@ static void dwc2_hsotg_epint(struct dwc2_hsotg *hsotg, unsigned int idx,
 		dev_dbg(hsotg->dev, "%s: BNA interrupt\n", __func__);
 		if (hs_ep->isochronous)
 			dwc2_gadget_handle_isoc_bna(hs_ep);
+		else if (using_desc_dma(hsotg) && idx)
+			dwc2_gadget_handle_nonisoc_bna(hs_ep, ints);
 	}
 
 	if (dir_in && !hs_ep->isochronous) {
-- 
2.56.0

[PATCH 3/3] usb: dwc2: gadget: make descriptors visible before the endpoint is enabled

[Vadim Yuzifovich]

In descriptor DMA mode the core fetches the first descriptor of a chain
as soon as the endpoint is enabled. dwc2 fills the chain in coherent
memory and then writes DxEPDMA and DxEPCTL.EPENA with writel(), whose
barrier orders the earlier stores before the MMIO write. On a Sophgo
SG2002 (T-Head C906, DWC_otg 4.20a) that is not enough: the core still
finds a descriptor that is not host ready, raises BNA and disables the
endpoint, although the descriptor reads host ready, last, short, IOC
when the interrupt is handled.

With a host sending TEST UNIT READY to a CD-ROM LUN as fast as it can
(about 2600 a second, each followed by REQUEST SENSE) and a 1080p60
H.264 encode loading the DRAM, about 0.8% of transfer starts raised BNA,
99.5% of them within 20 us of the enable (the latency of the interrupt
itself), with DxEPDMA still at the head of the chain. Without the
encode there was none in 254000 commands. BNAs per variant, measured in
turn on one boot with a diagnostic build, at about 780000 transfer
starts a minute each:

  unchanged                                   6215, 6510
  mb(), read the last descriptor, mb()           0
  th.sync.s                                      0
  udelay(2)                                      2
  descriptor written as two word stores,
    status last, dma_wmb() between            3728

Mapping coherent memory non-bufferable on the C906 did not change the
rate either. So the stores are still on their way to DRAM when the core
reads, and a load from the descriptor waits for them.

Read the last descriptor of a chain back between two full barriers
before the endpoint is enabled, for non-isochronous transfers and for
the start of an isochronous chain. It costs one uncached load per
transfer. With it there was no BNA in 6.2 million transfer starts under
the same load, and none in a 60 minute run at 1080p60.

Signed-off-by: Vadim Yuzifovich <vadim@yuzi.co>
---
 drivers/usb/dwc2/gadget.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 046b5b045..fe37250fe 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -854,6 +854,27 @@ static void dwc2_gadget_fill_nonisoc_xfer_ddma_one(struct dwc2_hsotg_ep *hs_ep,
 	}
 }
 
+/*
+ * dwc2_gadget_desc_visible - make descriptors the CPU has just written
+ * visible to the core before it is told to fetch them.
+ * @desc: The descriptor written last.
+ *
+ * The core fetches the first descriptor of a chain as soon as the endpoint is
+ * enabled. writel() orders the CPU's earlier stores before that MMIO write,
+ * but on the Sophgo SG2002 (T-Head C906) the stores to coherent memory can
+ * still be on their way to DRAM when the core reads it. The core then finds a
+ * descriptor that is not host ready and raises BNA. Reading the last
+ * descriptor back between two full barriers waits for the stores to land.
+ */
+static void dwc2_gadget_desc_visible(struct dwc2_dma_desc *desc)
+{
+	/* Order the stores to the chain before the load below. */
+	mb();
+	(void)READ_ONCE(desc->status);
+	/* Finish the load before the MMIO write that enables the endpoint. */
+	mb();
+}
+
 /*
  * dwc2_gadget_config_nonisoc_xfer_ddma - prepare non ISOC DMA desc chain.
  * @hs_ep: The endpoint
@@ -881,6 +902,7 @@ static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep,
 	if (!ureq || !ureq->num_sgs) {
 		dwc2_gadget_fill_nonisoc_xfer_ddma_one(hs_ep, &desc,
 			dma_buff, len, true);
+		dwc2_gadget_desc_visible(desc - 1);
 		return;
 	}
 
@@ -893,6 +915,7 @@ static void dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep,
 	}
 
 	hs_ep->desc_count = desc_count;
+	dwc2_gadget_desc_visible(desc - 1);
 }
 
 /*
@@ -1021,6 +1044,9 @@ static void dwc2_gadget_start_isoc_ddma(struct dwc2_hsotg_ep *hs_ep)
 	depctl = hs_ep->dir_in ? DIEPCTL(index) : DOEPCTL(index);
 	dma_reg = hs_ep->dir_in ? DIEPDMA(index) : DOEPDMA(index);
 
+	if (hs_ep->next_desc)
+		dwc2_gadget_desc_visible(&hs_ep->desc_list[hs_ep->next_desc - 1]);
+
 	/* write descriptor chain address to control register */
 	dwc2_writel(hsotg, hs_ep->desc_list_dma, dma_reg);
 
-- 
2.56.0

Re: [PATCH 3/3] usb: dwc2: gadget: make descriptors visible before the endpoint is enabled

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: dwc2: gadget: dwc2_gadget_desc_visible() skipped when
  isoc ring is full
--

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

usb: dwc2: gadget: make descriptors visible before the endpoint is enabled

This commit adds dwc2_gadget_desc_visible() to read back the last written
DMA descriptor between two full memory barriers before enabling an endpoint
in descriptor DMA mode.

> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 046b5b04503bd..fe37250fec6c7 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c

[ ... ]

> @@ -1021,6 +1044,9 @@ static void dwc2_gadget_start_isoc_ddma(struct dwc2_hsotg_ep *hs_ep)
>  	depctl = hs_ep->dir_in ? DIEPCTL(index) : DOEPCTL(index);
>  	dma_reg = hs_ep->dir_in ? DIEPDMA(index) : DOEPDMA(index);
>  
> +	if (hs_ep->next_desc)
> +		dwc2_gadget_desc_visible(&hs_ep->desc_list[hs_ep->next_desc - 1]);

[Severity: Medium]
Does this skip dwc2_gadget_desc_visible() when the isochronous descriptor
ring is full?

dwc2_gadget_start_isoc_ddma() returns early when hs_ep->queue is empty, so
at least one descriptor is always filled.  When MAX_DMA_DESC_NUM_HS_ISOC or
more requests are queued, dwc2_gadget_fill_isoc_desc() fills all 256
descriptors and wraps hs_ep->next_desc back to 0:

drivers/usb/dwc2/gadget.c:dwc2_gadget_fill_isoc_desc() {
    ...
	/* Update index of last configured entry in the chain */
	hs_ep->next_desc++;
	if (hs_ep->next_desc >= MAX_DMA_DESC_NUM_HS_ISOC)
		hs_ep->next_desc = 0;

	return 0;
}

When hs_ep->next_desc wraps to 0, the if (hs_ep->next_desc) check skips
dwc2_gadget_desc_visible() instead of reading back
hs_ep->desc_list[MAX_DMA_DESC_NUM_HS_ISOC - 1], leaving the descriptor
writes in flight when the endpoint is enabled and allowing the BNA error to
occur.

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

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

[sashiko-bot]

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

Re: [PATCH 1/3] usb: dwc2: gadget: restart descriptor DMA isochronous OUT endpoints after BNA

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [High] usb: dwc2: gadget: ep_disabled restart before
  dwc2_gadget_handle_isoc_bna()
- [Medium] usb: dwc2: gadget: empty queue in
  dwc2_gadget_restart_isoc_out_ddma() stalls
- [Low] usb: dwc2: gadget: missing Fixes trailer on BNA recovery fix
--

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

> usb: dwc2: gadget: restart descriptor DMA isochronous OUT endpoints after BNA
> 
> In descriptor DMA mode the core disables an isochronous endpoint when it
> reaches a descriptor that is not ready (BNA), for example after an
> interrupt latency longer than the queued requests cover.
> dwc2_gadget_handle_isoc_bna() resets the endpoint and leaves the restart
> to the next OUTTKNEPDIS. Two things keep that from happening.
> 
> First, the endpoint-disabled interrupt that comes with the BNA runs
> dwc2_gadget_handle_ep_disabled(), whose isochronous part is written for
> buffer DMA: it completes requests frame by frame and advances
> target_frame up to the current frame. Run after the BNA reset, it moves
> target_frame off TARGET_FRAME_INITIAL, and the OUT token handler then
> takes the chain for running and never restarts it.
> 
> Second, after the BNA the core raises BNA again for the disabled
> endpoint as fast as the handler clears it, and no OUTTKNEPDIS comes.
> 
> On a Sophgo SG2002 (DWC_otg 4.20a) with a UAC1 capture endpoint and
> eight requests queued, one 8 ms interrupt latency (a line printed on the
> 115200 baud serial console) caused one BNA. The endpoint stayed
> disabled and the BNA interrupt kept the CPU busy until the host gave up
> and reset the port, which took the other functions of the composite
> gadget down as well.
> 
> In descriptor DMA mode, do not run the buffer DMA completion for a
> disabled isochronous endpoint; put it back in its initial state. For an
> OUT endpoint, build the chain again from the queue and enable it at
> once, both after the BNA and after an endpoint-disabled interrupt that
> finds it disabled with requests queued. IN endpoints keep restarting on
> the next NAK.
> 
> Signed-off-by: Vadim Yuzifovich <vadim@yuzi.co>

[Severity: Low]
This fixes BNA recovery in descriptor DMA mode.  Should it include:

  Fixes: 729cac693eec ("usb: dwc2: Change ISOC DDMA flow")

> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 6c8a9f05fff00..b0e129061abb6 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -2259,6 +2259,28 @@ static void dwc2_gadget_complete_isoc_request_ddma(struct dwc2_hsotg_ep *hs_ep)
>  	}
>  }
>  
> +/*
> + * dwc2_gadget_restart_isoc_out_ddma - build the chain of a disabled ISOC OUT
> + * endpoint again from its queue and enable it.
> + * @hs_ep: The isochronous OUT endpoint, in its initial state.
> + *
> + * After a BNA the core keeps raising BNA for the endpoint, as fast as the
> + * handler clears it, and no OUTTKNEPDIS comes to restart the chain. So the
> + * chain is restarted here at once.
> + */
> +static void dwc2_gadget_restart_isoc_out_ddma(struct dwc2_hsotg_ep *hs_ep)
> +{
> +	struct dwc2_hsotg *hsotg = hs_ep->parent;
> +
> +	if (hs_ep->dir_in || list_empty(&hs_ep->queue) ||
> +	    hs_ep->target_frame != TARGET_FRAME_INITIAL ||
> +	    dwc2_readl(hsotg, DOEPCTL(hs_ep->index)) & DXEPCTL_EPENA)
> +		return;
> +
> +	hs_ep->target_frame = dwc2_hsotg_read_frameno(hsotg);
> +	dwc2_gadget_start_isoc_ddma(hs_ep);
> +}

[Severity: Medium]
What happens if hs_ep->queue is empty when dwc2_gadget_restart_isoc_out_ddma()
runs (for example, if dwc2_gadget_handle_isoc_bna() just completed the
only remaining request and the gadget function queues requests
asynchronously)?

Returning early leaves the endpoint disabled, so the core continues
raising BNA interrupts.  Even when dwc2_hsotg_ep_queue() is later called,
it sees hs_ep->target_frame == TARGET_FRAME_INITIAL and returns without
filling a descriptor or restarting the endpoint, because no OUTTKNEPDIS
will arrive after the BNA:

drivers/usb/dwc2/gadget.c:dwc2_hsotg_ep_queue() {
    ...
	if (using_desc_dma(hs) && hs_ep->isochronous) {
		if (hs_ep->target_frame != TARGET_FRAME_INITIAL) {
    ...
			dwc2_gadget_fill_isoc_desc(hs_ep, dma_addr,
						   hs_req->req.length);
		}
		return 0;
	}
    ...
}

> @@ -2902,6 +2926,27 @@ static void dwc2_gadget_handle_ep_disabled(struct dwc2_hsotg_ep *hs_ep)
>  	if (!hs_ep->isochronous)
>  		return;
>  
> +	/*
> +	 * In descriptor DMA the requests sit in the descriptor chain rather
> +	 * than one per frame, so the frame by frame completion below does not
> +	 * apply. The core disables an isochronous endpoint by itself when it
> +	 * meets a descriptor that was not filled in time (BNA). Put such an
> +	 * endpoint back in its initial state and build the chain again from
> +	 * the queue: at once for OUT, on the next NAK for IN. The loop below
> +	 * would move target_frame off TARGET_FRAME_INITIAL, which tells the
> +	 * token handlers that a chain runs: the endpoint then stays disabled
> +	 * for good, and every request queued again goes into a dead chain.
> +	 */
> +	if (using_desc_dma(hsotg)) {
> +		if (!(dwc2_readl(hsotg, epctl_reg) & DXEPCTL_EPENA)) {
> +			hs_ep->target_frame = TARGET_FRAME_INITIAL;
> +			hs_ep->next_desc = 0;
> +			hs_ep->compl_desc = 0;
> +			dwc2_gadget_restart_isoc_out_ddma(hs_ep);
> +		}
> +		return;
> +	}

[Severity: High]
Can dwc2_gadget_handle_isoc_bna() corrupt the newly started descriptor
chain when DXEPINT_EPDISBLD and DXEPINT_BNAINTR are handled in the same
dwc2_hsotg_epint() call?

In dwc2_hsotg_epint(), DXEPINT_EPDISBLD and DXEPINT_NAKINTRPT are
dispatched before DXEPINT_BNAINTR:

drivers/usb/dwc2/gadget.c:dwc2_hsotg_epint() {
    ...
	if (ints & DXEPINT_EPDISBLD)
		dwc2_gadget_handle_ep_disabled(hs_ep);

	if (ints & DXEPINT_OUTTKNEPDIS)
		dwc2_gadget_handle_out_token_ep_disabled(hs_ep);

	if (ints & DXEPINT_NAKINTRPT)
		dwc2_gadget_handle_nak(hs_ep);
    ...
	if (ints & DXEPINT_BNAINTR) {
		dev_dbg(hsotg->dev, "%s: BNA interrupt\n", __func__);
		if (hs_ep->isochronous)
			dwc2_gadget_handle_isoc_bna(hs_ep);
	}
    ...
}

When both bits are set after interrupt latency,
dwc2_gadget_handle_ep_disabled() (or dwc2_gadget_handle_nak() for IN)
rebuilds desc_list[] from hs_ep->queue and sets DXEPCTL_EPENA.
dwc2_gadget_handle_isoc_bna() then runs on the active endpoint: it
unmaps and completes get_ep_head(hs_ep) while hardware DMA is armed on
desc_list[0], shifts hs_ep->queue out of sync with desc_list[], and
resets hs_ep->target_frame to TARGET_FRAME_INITIAL while DXEPCTL_EPENA is
still set, so its call to dwc2_gadget_restart_isoc_out_ddma() returns
early and subsequent dwc2_hsotg_ep_queue() calls stop filling descriptors.

In addition, because dwc2_hsotg_epint() clears DOEPINT at the entry of the
handler before the endpoint is re-enabled and the core re-asserts
DXEPINT_BNAINTR while the endpoint is disabled, a newly latched BNA
interrupt can fire right after the restart and hit the running chain in
the same way.

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