From 4b6d887a89b79de062a8780b1826b2119f7a1fb8 Mon Sep 17 00:00:00 2001 From: Johann Fischer Date: Wed, 20 Nov 2024 23:56:10 +0100 Subject: [PATCH 1/2] usb: device_next: fix cdc_acm_send_notification() Fix "warning: 'cdc_acm_send_notification' defined but not used" when Kconfig option UART_USE_RUNTIME_CONFIGURE is not used and properly handle enqueue error. Signed-off-by: Johann Fischer --- subsys/usb/device_next/class/usbd_cdc_acm.c | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/subsys/usb/device_next/class/usbd_cdc_acm.c b/subsys/usb/device_next/class/usbd_cdc_acm.c index ee293cab75e4a..d61cfb34d8fbd 100644 --- a/subsys/usb/device_next/class/usbd_cdc_acm.c +++ b/subsys/usb/device_next/class/usbd_cdc_acm.c @@ -495,8 +495,8 @@ static int usbd_cdc_acm_init(struct usbd_class_data *const c_data) return 0; } -static int cdc_acm_send_notification(const struct device *dev, - const uint16_t serial_state) +static inline int cdc_acm_send_notification(const struct device *dev, + const uint16_t serial_state) { struct cdc_acm_notification notification = { .bmRequestType = 0xA1, @@ -530,7 +530,11 @@ static int cdc_acm_send_notification(const struct device *dev, net_buf_add_mem(buf, ¬ification, sizeof(struct cdc_acm_notification)); ret = usbd_ep_enqueue(c_data, buf); - /* FIXME: support for sync transfers */ + if (ret) { + net_buf_unref(buf); + return ret; + } + k_sem_take(&data->notif_sem, K_FOREVER); return ret; From 8b9397df62e5f312bc21ecabc811b4f9b31907ab Mon Sep 17 00:00:00 2001 From: Johann Fischer Date: Wed, 20 Nov 2024 15:33:58 +0100 Subject: [PATCH 2/2] usb: device_next: prevent ECHO on Linux Kernel based host MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is similar to the approach implemented for the legacy CDC ACM class commit 0127d000a287 ("usb: device: cdc_acm: Use ZLP to detect initial host read") but it uses Set Line Coding request to detect client activity on the host side. Suggested-by: Tomasz Moń Signed-off-by: Johann Fischer --- subsys/usb/device_next/class/Kconfig.cdc_acm | 8 ++++ subsys/usb/device_next/class/usbd_cdc_acm.c | 46 +++++++++++++++++--- 2 files changed, 48 insertions(+), 6 deletions(-) diff --git a/subsys/usb/device_next/class/Kconfig.cdc_acm b/subsys/usb/device_next/class/Kconfig.cdc_acm index 997ded105f4eb..fb7cd1f1da59a 100644 --- a/subsys/usb/device_next/class/Kconfig.cdc_acm +++ b/subsys/usb/device_next/class/Kconfig.cdc_acm @@ -21,6 +21,14 @@ config USBD_CDC_ACM_STACK_SIZE help USB CDC ACM workqueue stack size. +config USBD_CDC_ACM_TX_DELAY + int + range 1 1000 + default 100 + help + Time in milliseconds to wait before sending actual payload to host. + This is needed to prevent tty ECHO on Linux. + module = USBD_CDC_ACM module-str = usbd cdc_acm default-count = 1 diff --git a/subsys/usb/device_next/class/usbd_cdc_acm.c b/subsys/usb/device_next/class/usbd_cdc_acm.c index d61cfb34d8fbd..d11d8a88539cd 100644 --- a/subsys/usb/device_next/class/usbd_cdc_acm.c +++ b/subsys/usb/device_next/class/usbd_cdc_acm.c @@ -111,6 +111,22 @@ struct cdc_acm_uart_data { * roughly emulating flow control. */ bool flow_ctrl; + /* + * This is used to indicate that a client has started using a serial + * device on the host side. We use it to enqueue the first transfer as + * ZLP and later to delay the first data packet. + * + * The reason for this is to wait for initialization timeout before + * sending actual payload to make it possible for application to + * disable ECHO. The echo is long known problem related to the fact + * that POSIX defaults to ECHO ON and thus every application that opens + * tty device (on Linux) will have ECHO enabled in the short window + * between open() and ioctl() that disables the echo (if application + * wishes to disable the echo). + * + * The variable is set on line coding request. + */ + bool first_tx_pkt; /* USBD CDC ACM TX fifo work */ struct k_work_delayable tx_fifo_work; /* USBD CDC ACM RX fifo work */ @@ -259,8 +275,15 @@ static int usbd_cdc_acm_request(struct usbd_class_data *const c_data, atomic_clear_bit(&data->state, CDC_ACM_TX_FIFO_BUSY); if (!ring_buf_is_empty(data->tx_fifo.rb)) { + k_timeout_t timeout = K_NO_WAIT; + + if (data->first_tx_pkt) { + timeout = K_MSEC(CONFIG_USBD_CDC_ACM_TX_DELAY); + data->first_tx_pkt = false; + } + /* Queue pending TX data on IN endpoint */ - cdc_acm_work_schedule(&data->tx_fifo_work, K_NO_WAIT); + cdc_acm_work_schedule(&data->tx_fifo_work, timeout); } } @@ -296,11 +319,17 @@ static void usbd_cdc_acm_enable(struct usbd_class_data *const c_data) if (ring_buf_space_get(data->tx_fifo.rb)) { /* Raise TX ready interrupt */ cdc_acm_work_submit(&data->irq_cb_work); - } else { - /* Queue pending TX data on IN endpoint */ - cdc_acm_work_schedule(&data->tx_fifo_work, K_NO_WAIT); } } + + if (!ring_buf_is_empty(data->tx_fifo.rb)) { + /* Queue pending TX data on the IN endpoint with a delay to + * allow first_tx_pkt to be set if there is a Set Line Coding + * request. + */ + cdc_acm_work_schedule(&data->tx_fifo_work, + K_MSEC(CONFIG_USBD_CDC_ACM_TX_DELAY)); + } } static void usbd_cdc_acm_disable(struct usbd_class_data *const c_data) @@ -463,6 +492,8 @@ static int usbd_cdc_acm_ctd(struct usbd_class_data *const c_data, memcpy(&data->line_coding, buf->data, len); cdc_acm_update_uart_cfg(data); usbd_msg_pub_device(uds_ctx, USBD_MSG_CDC_ACM_LINE_CODING, dev); + data->first_tx_pkt = true; + return 0; case SET_CONTROL_LINE_STATE: @@ -549,7 +580,7 @@ static void cdc_acm_tx_fifo_handler(struct k_work *work) struct cdc_acm_uart_data *data; struct usbd_class_data *c_data; struct net_buf *buf; - size_t len; + size_t len = 0; int ret; data = CONTAINER_OF(dwork, struct cdc_acm_uart_data, tx_fifo_work); @@ -577,7 +608,10 @@ static void cdc_acm_tx_fifo_handler(struct k_work *work) return; } - len = ring_buf_get(data->tx_fifo.rb, buf->data, buf->size); + if (!data->first_tx_pkt) { + len = ring_buf_get(data->tx_fifo.rb, buf->data, buf->size); + } + net_buf_add(buf, len); ret = usbd_ep_enqueue(c_data, buf);