usb cdc acm: can't (re-)connect after host usb reset if device sends infinite data

[metratec-mkoehler]

Description:

My device sends data out over CDC ACM. When the host PC reboots the device is not (power) reset so it stays in this state. The host is not able to reinit the device so it's missing from the device list. I use Linux as the host platform but was able to reproduce the issue an an Windows 10 host PC, too. The board used is a custom board with an RP2040 MCU but I was able to reproduce it on nrf52840dk eval board. I expect it to happen on others, too.

Reproduction:

In zephyr/samples/subsys/usb/cdc_acm/src/main.c:

Add uart_fifo_fill(uart_dev, "T",1);continue; after line int rb_len, send_len; and change uart_irq_rx_enable(uart_dev); at the end of main() to uart_irq_tx_enable(uart_dev);

west build -b nrf52840dk/nrf52840 zephyr/samples/subsys/usb/cdc_acm --pristine west flash

The device is now sending infinite 'T'

Device won't show up under lsusb. If the code gets changed to start sending after receiving the first data it shows up but triggering the data flow and resetting it with sudo usbreset 2fe3:0001 will remove it from the device list. The same is true for a host reboot.

Expected:

Device exists under lsusb and can be used as ACM device. Host should receive the data stream.

Environment

• OS: XUbuntu Linux for dev and testing, test also in Windows 10
• Toolchain: Zephyr SDK
• Commit: current, tested on 50c03e9bbe68db9944704e380802be44d45cfc42

Severity

• Was a showstopper for the device (sends data from a sensor subdevice) but the fix is already shipped
• Tests showed the device is stuck as acm_cdc API return values don't take connection state into account
• For further devices with comparable features a mainlined fix (and external code checking) is needed

[jfischer-no]

So with the changes,

diff --git a/samples/subsys/usb/cdc_acm/src/main.c b/samples/subsys/usb/cdc_acm/src/main.c
index ab1cbdc6a4c..b225b75ec5d 100644
--- a/samples/subsys/usb/cdc_acm/src/main.c
+++ b/samples/subsys/usb/cdc_acm/src/main.c
@@ -146,6 +146,11 @@ static void interrupt_handler(const struct device *dev, void *user_data)
                if (uart_irq_tx_ready(dev)) {
                        uint8_t buffer[64];
                        int rb_len, send_len;
+                       static uint8_t count = '0';
+
+                       uart_fifo_fill(uart_dev, &count, 1);
+                       count = count <= '9' ? count + 1 : '0';
+                       continue;

                        rb_len = ring_buf_get(&ringbuf, buffer, sizeof(buffer));
                        if (!rb_len) {
@@ -231,7 +236,7 @@ int main(void)
        uart_irq_callback_set(uart_dev, interrupt_handler);

        /* Enable rx interrupts */
-       uart_irq_rx_enable(uart_dev);
+       uart_irq_tx_enable(uart_dev);

        return 0;
 }

when the host reads the data once, disconnect device leads to cdc_acm_fifo_fill() call in the while loop inside the interrupt handler, because dev_data->tx_ready is always true. The workqueue thread has a cooperative priority and starves everything else. dev_data->tx_ready = false; could be moved before if (!dev_data->configured || dev_data->suspended) or if (!dev_data->configured || dev_data->suspended) remove entirely. https://github.com/zephyrproject-rtos/zephyr/blob/ac52bd629d705811858c8bb38ca7a43e4e81182b/subsys/usb/device/class/cdc_acm.c#L529-L535

[metratec-mkoehler]

I have the same diagnosis and fifo_fill works correvt. The LOG should not say it drops data though (it does not, it just can't send them, which the calling function can handle).

Additionally cdc_acm_irq_tx_ready needs to take this into account as cdc_acm_fifo_fill() can't take data (will return 0) in this state. This violates UART API description.

The same is true for cdc_acm_irq_is_pending()