Skip to content

sys/usb/cdc_acm: fix garbage input when disconnected #21890

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

Merged
merged 2 commits into from
Nov 18, 2025
+12 −21
Merged

sys/usb/cdc_acm: fix garbage input when disconnected #21890

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
182c7ec
sys/usb/cdc_acm: fix garbage input when disconnected
benpicco Nov 17, 2025
494e171
sys/usb_cdc_acm_stdio: use isrpipe_write()
benpicco Nov 17, 2025
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
29 changes: 11 additions & 18 deletions sys/usb/usbus/cdc/acm/cdc_acm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,25 +152,14 @@ size_t usbus_cdc_acm_submit(usbus_cdcacm_device_t *cdcacm, const uint8_t *buf, s
{
size_t n;
unsigned old;
if (cdcacm->state != USBUS_CDC_ACM_LINE_STATE_DISCONNECTED) {
old = irq_disable();
n = turb_add(&cdcacm->tsrb, buf, len);
irq_restore(old);
return n;
if (cdcacm->state == USBUS_CDC_ACM_LINE_STATE_DISCONNECTED) {
return len;
Comment on lines +155 to +156
Copy link
Contributor

@mguetschow mguetschow Nov 18, 2025

Choose a reason for hiding this comment

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

Shouldn't this rather return 0, as no bytes were actually added to the ring buffer? See

RIOT/sys/include/usb/usbus/cdc/acm.h

Lines 194 to 204 in bcd09d3

/**
* @brief Submit bytes to the CDC ACM handler
*
* @param[in] cdcacm USBUS CDC ACM handler context
* @param[in] buf buffer to submit
* @param[in] len length of the submitted buffer
*
* @return Number of bytes added to the CDC ACM ring buffer
*/
size_t usbus_cdc_acm_submit(usbus_cdcacm_device_t *cdcacm,
const uint8_t *buf, size_t len);

Copy link
Member

@maribu maribu Nov 18, 2025

Choose a reason for hiding this comment

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

That would cause problems with stdio, as stdio_write() is expected to either return the number of bytes written in absence of an error, or an error if writing bytes won't work.

If it would return 0, the stdio implementation on top would just keep calling stdio_write() in a loop until all data has been written - which would be an endless loop here.

We should either change the doc to "number of bytes handled" with discarding being handled, or change the return type to ssize_t and return an error when discarding data.

Discarding the data here and not reporting an error doesn't really change the API contract, btw. Even if we would go for ssize_t, getting a success return code is no guarantee that the data will be transferred to the Linux host: They may have been successfully added to the ringbuffer and subsequently a disconnect may happen before the data left the ringbuffer.

Copy link
Contributor

@mguetschow mguetschow Nov 18, 2025

Choose a reason for hiding this comment

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

Thanks for the explanation. I think I'd favor a telling error code over a silent drop. "bytes handled" does not really tell the user much.

}
/* stuff as much data as possible into tsrb, discarding the oldest */

old = irq_disable();
n = turb_free(&cdcacm->tsrb);
if (len > n) {
n += turb_drop(&cdcacm->tsrb, len - n);
buf += len - n;
} else {
n = len;
}
turb_add(&cdcacm->tsrb, buf, n);
/* behave as if everything has been written correctly */
n = turb_add(&cdcacm->tsrb, buf, len);
irq_restore(old);
return len;
return n;
}

void usbus_cdc_acm_set_coding_cb(usbus_cdcacm_device_t *cdcacm,
Expand Down Expand Up @@ -357,8 +346,12 @@ static void _transfer_handler(usbus_t *usbus, usbus_handler_t *handler,
{
(void)usbus;
(void)event; /* Only receives TR_COMPLETE events */
if (ep->type != USB_EP_TYPE_BULK) {
return;
}

usbus_cdcacm_device_t *cdcacm = (usbus_cdcacm_device_t*)handler;
if ((ep->dir == USB_EP_DIR_OUT) && (ep->type == USB_EP_TYPE_BULK)) {
if (ep->dir == USB_EP_DIR_OUT) {
size_t len;
/* Retrieve incoming data */
usbdev_ep_get(ep, USBOPT_EP_AVAILABLE, &len, sizeof(size_t));
Expand All @@ -367,7 +360,7 @@ static void _transfer_handler(usbus_t *usbus, usbus_handler_t *handler,
}
usbdev_ep_xmit(ep, cdcacm->out_buf, CONFIG_USBUS_CDC_ACM_BULK_EP_SIZE);
}
if ((ep->dir == USB_EP_DIR_IN) && (ep->type == USB_EP_TYPE_BULK)) {
else if (ep->dir == USB_EP_DIR_IN) {
cdcacm->occupied = 0;
if (!tsrb_empty(&cdcacm->tsrb)) {
return _handle_in(cdcacm, ep);
Expand Down
4 changes: 1 addition & 3 deletions sys/usb/usbus/cdc/acm/cdc_acm_stdio.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,9 +51,7 @@ static void _cdc_acm_rx_pipe(usbus_cdcacm_device_t *cdcacm,
uint8_t *data, size_t len)
{
(void)cdcacm;
for (size_t i = 0; i < len; i++) {
isrpipe_write_one(&stdin_isrpipe, data[i]);
}
isrpipe_write(&stdin_isrpipe, data, len);
}

void usb_cdc_acm_stdio_init(usbus_t *usbus)
Expand Down