bartoszek/zephyr
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

drivers: udc_dwc2: Fix incomplete iso handling race

Browse Source 
Incomplete iso IN/OUT is just informative and its occurrence does not
prevent the endpoint from actually transmitting/receiving data. Such
"late" isochronous transfers, which are perfectly fine according to USB
specification, were observed on Windows host with nRF54H20 running
explicit feedback sample operating at High-Speed.

The incorrect handling manifested itself with "ISO RX buffer too small"
error message. The faulty scenario was:
  * incompISOIN handler does not find any matching endpoint
  * incompISOOUT handler disables endpoint, discards buffer and sets
    rearm flag
  * next DWC2 interrupt handler iteration after reading GINTSTS
  * XferCompl interrupt on iso IN endpoint
  * XferCompl interrupt on iso OUT endpoint
      - transfer was actually happening to the buffer discarded in
	incompISOOUT handler
      - XferCompl handler modified the next buffer
  * GOUTNakEff interrupt, iso OUT endpoint EPDIS bit is set
  * EPDisbld interrupt, rearm flag set
      - the buffer modified by XferCompl is used and fails because it is
	not large enough

Modify the sequence so it accounts for host actions and the above faulty
scenario no longer causes any problems.

Signed-off-by: Tomasz Moń <tomasz.mon@nordicsemi.no>
This commit is contained in:
Tomasz Moń 2025-07-17 10:20:34 +02:00 committed by Chris Friedt
parent 6eb2fa8edd
commit 8d1f7b3bef
1 changed files with 74 additions and 24 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

98
drivers/usb/udc/udc_dwc2.c
View File

@ -141,6 +141,7 @@ struct udc_dwc2_data {
unsigned int enumdone : 1;
unsigned int enumspd : 2;
unsigned int pending_dout_feed : 1;
unsigned int ignore_ep0_nakeff : 1;
enum dwc2_suspend_type suspend_type;
/* Number of endpoints including control endpoint */
uint8_t numdeveps;
@ -485,7 +486,6 @@ static int dwc2_tx_fifo_write(const struct device *dev,
mem_addr_t dieptsiz_reg = (mem_addr_t)&base->in_ep[ep_idx].dieptsiz;
/* TODO: use dwc2_get_dxepctl_reg() */
mem_addr_t diepctl_reg = (mem_addr_t)&base->in_ep[ep_idx].diepctl;
mem_addr_t diepint_reg = (mem_addr_t)&base->in_ep[ep_idx].diepint;
uint32_t diepctl;
uint32_t max_xfersize, max_pktcnt, pktcnt;
@ -617,9 +617,6 @@ static int dwc2_tx_fifo_write(const struct device *dev,
}
sys_write32(diepctl, diepctl_reg);
/* Clear IN Endpoint NAK Effective interrupt in case it was set */
sys_write32(USB_DWC2_DIEPINT_INEPNAKEFF, diepint_reg);
if (dwc2_in_completer_mode(dev)) {
const uint8_t *src = buf->data;
@ -811,7 +808,20 @@ static void dwc2_handle_xfer_next(const struct device *dev,
if (USB_EP_DIR_IS_OUT(cfg->addr)) {
dwc2_prep_rx(dev, buf, cfg);
} else {
int err = dwc2_tx_fifo_write(dev, cfg, buf);
int err;
if (cfg->addr == USB_CONTROL_EP_IN &&
udc_ctrl_stage_is_status_in(dev)) {
/* It was observed that EPENA results in INEPNAKEFF
* interrupt which leads to endpoint disable. It is not
* clear how to prevent this without violating sequence
* described in Programming Guide, so just set a flag
* for interrupt handler to ignore it.
*/
priv->ignore_ep0_nakeff = 1;
}
err = dwc2_tx_fifo_write(dev, cfg, buf);
if (cfg->addr == USB_CONTROL_EP_IN) {
/* Feed a buffer for the next setup packet after arming
@ -862,6 +872,7 @@ static int dwc2_handle_evt_setup(const struct device *dev)
* transfer beforehand. In Buffer DMA the SETUP can be copied to any EP0
* OUT buffer. If there is any buffer queued, it is obsolete now.
*/
priv->ignore_ep0_nakeff = 0;
udc_dwc2_ep_disable(dev, cfg_in, false, true);
atomic_and(&priv->xfer_finished, ~(BIT(0) | BIT(16)));
@ -2658,7 +2669,21 @@ static inline void dwc2_handle_iepint(const struct device *dev)
}
if (status & USB_DWC2_DIEPINT_INEPNAKEFF) {
sys_set_bits(diepctl_reg, USB_DWC2_DEPCTL_EPDIS);
uint32_t diepctl = sys_read32(diepctl_reg);
if (!(diepctl & USB_DWC2_DEPCTL_NAKSTS)) {
/* Ignore stale NAK effective interrupt */
} else if (n == 0 && priv->ignore_ep0_nakeff) {
/* Status stage enabled endpoint. NAK will be
* cleared in STSPHSERCVD interrupt.
*/
} else if (diepctl & USB_DWC2_DEPCTL_EPENA) {
diepctl &= ~USB_DWC2_DEPCTL_EPENA;
diepctl |= USB_DWC2_DEPCTL_EPDIS;
sys_write32(diepctl, diepctl_reg);
} else if (priv->iso_in_rearm & (BIT(n))) {
priv->iso_in_rearm &= ~BIT(n);
}
}
if (status & USB_DWC2_DIEPINT_EPDISBLD) {
@ -2676,6 +2701,13 @@ static inline void dwc2_handle_iepint(const struct device *dev)
if ((usb_dwc2_get_depctl_eptype(diepctl) == USB_DWC2_DEPCTL_EPTYPE_ISO) &&
(priv->iso_in_rearm & BIT(n))) {
struct udc_ep_config *cfg = udc_get_ep_cfg(dev, n | USB_EP_DIR_IN);
struct net_buf *buf;
/* Data is no longer relevant, discard it */
buf = udc_buf_get(cfg);
if (buf) {
udc_submit_ep_event(dev, buf, 0);
}
/* Try to queue next packet before SOF */
dwc2_handle_xfer_next(dev, cfg);
@ -2837,9 +2869,17 @@ static inline void dwc2_handle_oepint(const struct device *dev)
if ((usb_dwc2_get_depctl_eptype(doepctl) == USB_DWC2_DEPCTL_EPTYPE_ISO) &&
(priv->iso_out_rearm & BIT(n))) {
struct udc_ep_config *cfg;
struct net_buf *buf;
cfg = udc_get_ep_cfg(dev, n | USB_EP_DIR_OUT);
/* Discard disabled transfer buffer */
buf = udc_buf_get(cfg);
if (buf) {
udc_submit_ep_event(dev, buf, 0);
}
/* Try to queue next packet before SOF */
cfg = udc_get_ep_cfg(dev, n | USB_EP_DIR_OUT);
dwc2_handle_xfer_next(dev, cfg);
priv->iso_out_rearm &= ~BIT(n);
@ -2883,7 +2923,6 @@ static void dwc2_handle_incompisoin(const struct device *dev)
/* Check if endpoint didn't receive ISO IN data */
if ((diepctl & mask) == val) {
struct udc_ep_config *cfg;
struct net_buf *buf;
cfg = udc_get_ep_cfg(dev, i | USB_EP_DIR_IN);
__ASSERT_NO_MSG(cfg && cfg->stat.enabled &&
@ -2891,13 +2930,7 @@ static void dwc2_handle_incompisoin(const struct device *dev)
udc_dwc2_ep_disable(dev, cfg, false, false);
buf = udc_buf_get(cfg);
if (buf) {
/* Data is no longer relevant */
udc_submit_ep_event(dev, buf, 0);
rearm |= BIT(i);
}
rearm |= BIT(i);
}
eps &= ~BIT(i);
@ -2940,7 +2973,6 @@ static void dwc2_handle_incompisoout(const struct device *dev)
/* Check if endpoint didn't receive ISO OUT data */
if ((doepctl & mask) == val) {
struct udc_ep_config *cfg;
struct net_buf *buf;
cfg = udc_get_ep_cfg(dev, i);
__ASSERT_NO_MSG(cfg && cfg->stat.enabled &&
@ -2948,12 +2980,7 @@ static void dwc2_handle_incompisoout(const struct device *dev)
udc_dwc2_ep_disable(dev, cfg, false, false);
buf = udc_buf_get(cfg);
if (buf) {
udc_submit_ep_event(dev, buf, 0);
rearm |= BIT(i);
}
rearm |= BIT(i);
}
eps &= ~BIT(i);
@ -2984,8 +3011,31 @@ static void dwc2_handle_goutnakeff(const struct device *dev)
doepctl_reg = (mem_addr_t)&base->out_ep[ep_idx].doepctl;
doepctl = sys_read32(doepctl_reg);
/* The application cannot disable control OUT endpoint 0. */
if (ep_idx != 0) {
if (!(doepctl & USB_DWC2_DEPCTL_EPENA)) {
/* While endpoint was enabled when application requested
* to disable it, there is a race window between setting
* DCTL SGOUTNAK bit and it becoming effective. During
* the window, it is possible for host to actually send
* OUT DATA packet ending the transfer which disables
* the endpoint.
*
* If we arrived here due to INCOMPISOOUT, clearing
* the rearm flag is enough.
*/
if (priv->iso_out_rearm & BIT(ep_idx)) {
priv->iso_out_rearm &= ~BIT(ep_idx);
}
/* If application requested STALL then set it here, but
* otherwise there's nothing to do.
*/
if (!(priv->ep_out_stall & BIT(ep_idx))) {
continue;
}
} else if (ep_idx != 0) {
/* Only set EPDIS if EPENA is set, but do not set it for
* endpoint 0 which cannot be disabled by application.
*/
doepctl |= USB_DWC2_DEPCTL_EPDIS;
}