Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

usb: device_next: uac2: Double buffering on IN data endpoints #82135

Merged
merged 4 commits into from
Nov 30, 2024
Merged

usb: device_next: uac2: Double buffering on IN data endpoints #82135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
66c03af
drivers: udc_dwc2: Fix isochronous endpoint disable
tmon-nordic Nov 27, 2024
3b48350
drivers: udc_dwc2: Queue next packet on incomplete iso IN interrupt
tmon-nordic Nov 27, 2024
d55cc46
usb: device_next: uac2: Double buffering on IN data endpoints
tmon-nordic Nov 27, 2024
8df471b
drivers: udc_dwc2: Workaround endpoint disable race condition
tmon-nordic Nov 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 21 additions & 3 deletions drivers/usb/udc/udc_dwc2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1387,11 +1387,12 @@ static void udc_dwc2_ep_disable(const struct device *dev,
uint8_t ep_idx = USB_EP_GET_IDX(cfg->addr);
mem_addr_t dxepctl_reg;
uint32_t dxepctl;
const bool is_iso = dwc2_ep_is_iso(cfg);

dxepctl_reg = dwc2_get_dxepctl_reg(dev, cfg->addr);
dxepctl = sys_read32(dxepctl_reg);

if (dxepctl & USB_DWC2_DEPCTL_NAKSTS) {
if (!is_iso && (dxepctl & USB_DWC2_DEPCTL_NAKSTS)) {
/* Endpoint already sends forced NAKs. STALL if necessary. */
if (stall) {
dxepctl |= USB_DWC2_DEPCTL_STALL;
Expand All @@ -1401,6 +1402,19 @@ static void udc_dwc2_ep_disable(const struct device *dev,
return;
}

/* FIXME: This function needs to be changed to not synchronously wait
* for the events to happen because the actions here are racing against
* the USB host packets. It is possible that the IN token or OUT DATA
* gets sent shortly before this function disables the endpoint. If this
* happens, the XferCompl would be set and driver will incorrectly think
* that either:
* * never queued transfer finished, OR
* * transfer queued in incompisoin handler finished (before it really
* does and then it'll "double"-finish when it actually finishes)
*
* For the time being XferCompl is cleared as a workaround.
*/

if (USB_EP_DIR_IS_OUT(cfg->addr)) {
mem_addr_t dctl_reg, gintsts_reg, doepint_reg;
uint32_t dctl;
Expand Down Expand Up @@ -1439,7 +1453,7 @@ static void udc_dwc2_ep_disable(const struct device *dev,
}

/* Clear Endpoint Disabled interrupt */
sys_write32(USB_DWC2_DIEPINT_EPDISBLD, doepint_reg);
sys_write32(USB_DWC2_DOEPINT_EPDISBLD | USB_DWC2_DOEPINT_XFERCOMPL, doepint_reg);

dctl |= USB_DWC2_DCTL_CGOUTNAK;
sys_write32(dctl, dctl_reg);
Expand All @@ -1463,7 +1477,7 @@ static void udc_dwc2_ep_disable(const struct device *dev,
dwc2_wait_for_bit(dev, diepint_reg, USB_DWC2_DIEPINT_EPDISBLD);

/* Clear Endpoint Disabled interrupt */
sys_write32(USB_DWC2_DIEPINT_EPDISBLD, diepint_reg);
sys_write32(USB_DWC2_DIEPINT_EPDISBLD | USB_DWC2_DIEPINT_XFERCOMPL, diepint_reg);

/* TODO: Read DIEPTSIZn here? Programming Guide suggest it to
* let application know how many bytes of interrupted transfer
Expand Down Expand Up @@ -2571,7 +2585,11 @@ static void dwc2_handle_incompisoin(const struct device *dev)

buf = udc_buf_get(dev, cfg->addr);
if (buf) {
/* Data is no longer relevant */
udc_submit_ep_event(dev, buf, 0);

/* Try to queue next packet before SOF */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just found out that there is a race condition between the host, DWC2 otg core and SW. After incomplete iso IN is triggered, it is still possible for the IN to be sent and actually handled. The failed race condition here that results in double pulling data from net buf is as follows:

  1. Incomplete iso IN is raised
  2. if ((diepctl & mask) == val) condition above is true, driver takes path to call udc_dwc2_ep_disable()
  3. Before udc_dwc2_ep_disable() finishes, the host sends the IN token, IN transfer complete interrupt is raised
  4. Driver executes udc_dwc2_ep_disable()
  5. Driver submits next transfer here
  6. Host sends SOF
  7. IN transfer complete from point 3 is handled but it was for the previous transfer! The net buf is pulled the last tx data
  8. Host sends IN token, device responds and IN transfer complete interrupt is raised
  9. Driver on next isr loop (without reentering ISR, there is a while() in ISR handler until all interrupts are handled) notices IN transfer complete. The net buf is pulled the last tx data (second time, which triggers assertion failure)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workaround is to clear XferCompl interrupt on endpoint disable. This seems to solve not only the IN problem, but also occassional "udc_dwc2: No buffer for ep" for OUT endpoints.

dwc2_handle_xfer_next(dev, cfg);
}
}
}
Expand Down
30 changes: 18 additions & 12 deletions subsys/usb/device_next/class/usbd_uac2.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,15 @@ LOG_MODULE_REGISTER(usbd_uac2, CONFIG_USBD_UAC2_LOG_LEVEL);

#define DT_DRV_COMPAT zephyr_uac2

#define COUNT_UAC2_AS_ENDPOINTS(node) \
#define COUNT_UAC2_AS_ENDPOINT_BUFFERS(node) \
IF_ENABLED(DT_NODE_HAS_COMPAT(node, zephyr_uac2_audio_streaming), ( \
+ AS_HAS_ISOCHRONOUS_DATA_ENDPOINT(node) + \
+ AS_IS_USB_ISO_IN(node) /* ISO IN double buffering */ + \
AS_HAS_EXPLICIT_FEEDBACK_ENDPOINT(node)))
#define COUNT_UAC2_ENDPOINTS(i) \
#define COUNT_UAC2_EP_BUFFERS(i) \
+ DT_PROP(DT_DRV_INST(i), interrupt_endpoint) \
DT_INST_FOREACH_CHILD(i, COUNT_UAC2_AS_ENDPOINTS)
#define UAC2_NUM_ENDPOINTS DT_INST_FOREACH_STATUS_OKAY(COUNT_UAC2_ENDPOINTS)
DT_INST_FOREACH_CHILD(i, COUNT_UAC2_AS_ENDPOINT_BUFFERS)
#define UAC2_NUM_EP_BUFFERS DT_INST_FOREACH_STATUS_OKAY(COUNT_UAC2_EP_BUFFERS)

/* Net buf is used mostly with external data. The main reason behind external
* data is avoiding unnecessary isochronous data copy operations.
Expand All @@ -40,7 +41,7 @@ LOG_MODULE_REGISTER(usbd_uac2, CONFIG_USBD_UAC2_LOG_LEVEL);
* "wasted memory" here is likely to be smaller than the memory overhead for
* more complex "only as much as needed" schemes (e.g. heap).
*/
UDC_BUF_POOL_DEFINE(uac2_pool, UAC2_NUM_ENDPOINTS, 6,
UDC_BUF_POOL_DEFINE(uac2_pool, UAC2_NUM_EP_BUFFERS, 6,
sizeof(struct udc_buf_info), NULL);

/* 5.2.2 Control Request Layout */
Expand Down Expand Up @@ -80,6 +81,7 @@ struct uac2_ctx {
*/
atomic_t as_active;
atomic_t as_queued;
atomic_t as_double;
uint32_t fb_queued;
};

Expand Down Expand Up @@ -247,6 +249,7 @@ int usbd_uac2_send(const struct device *dev, uint8_t terminal,
struct uac2_ctx *ctx = dev->data;
struct net_buf *buf;
const struct usb_ep_descriptor *desc;
atomic_t *queued_bits = &ctx->as_queued;
uint8_t ep = 0;
int as_idx = terminal_to_as_interface(dev, terminal);
int ret;
Expand All @@ -267,9 +270,12 @@ int usbd_uac2_send(const struct device *dev, uint8_t terminal,
return 0;
}

if (atomic_test_and_set_bit(&ctx->as_queued, as_idx)) {
LOG_ERR("Previous send not finished yet on 0x%02x", ep);
return -EAGAIN;
if (atomic_test_and_set_bit(queued_bits, as_idx)) {
queued_bits = &ctx->as_double;
if (atomic_test_and_set_bit(queued_bits, as_idx)) {
LOG_DBG("Already double queued on 0x%02x", ep);
return -EAGAIN;
}
}

buf = uac2_buf_alloc(ep, data, size);
Expand All @@ -278,7 +284,7 @@ int usbd_uac2_send(const struct device *dev, uint8_t terminal,
* enough, but if it does all we loose is just single packet.
*/
LOG_ERR("No netbuf for send");
atomic_clear_bit(&ctx->as_queued, as_idx);
atomic_clear_bit(queued_bits, as_idx);
ctx->ops->buf_release_cb(dev, terminal, data, ctx->user_data);
return -ENOMEM;
}
Expand All @@ -287,7 +293,7 @@ int usbd_uac2_send(const struct device *dev, uint8_t terminal,
if (ret) {
LOG_ERR("Failed to enqueue net_buf for 0x%02x", ep);
net_buf_unref(buf);
atomic_clear_bit(&ctx->as_queued, as_idx);
atomic_clear_bit(queued_bits, as_idx);
ctx->ops->buf_release_cb(dev, terminal, data, ctx->user_data);
}

Expand Down Expand Up @@ -761,8 +767,8 @@ static int uac2_request(struct usbd_class_data *const c_data, struct net_buf *bu

if (is_feedback) {
ctx->fb_queued &= ~BIT(as_idx);
} else {
atomic_clear_bit(&ctx->as_queued, as_idx);
} else if (!atomic_test_and_clear_bit(&ctx->as_queued, as_idx) || buf->frags) {
atomic_clear_bit(&ctx->as_double, as_idx);
}

if (USB_EP_DIR_IS_OUT(ep)) {
Expand Down
Loading