USB Device next cdc acm can get in a deadlock when the host doesn't pull the endpoint data #78160
Conversation
sample application which connects a physical uart to a usb cdc acm uart The complete process in handled in a single interrupt handler without interference of the application code. Signed-off-by: Vincent van der Locht <[email protected]>
zephyrbot
requested review from
jfischer-no,
kartben,
nashif and
tmon-nordic
|
Hello @SynchronicIT, and thank you very much for your first pull request to the Zephyr project! |
In case the host doesn't pull the new data from the endpoint, the work task would schedule itself again (at the max. priority). When there is no terminal program or active applicaiton, this could result in an infinite loop blocking all other communications. By using a atomic flag for tx busy, the deadlock is solved. Signed-off-by: Vincent van der Locht <[email protected]>
SynchronicIT
force-pushed
the
device_next_cdc_acm_deadlock
branch
from
2738d32
to
5596694
Compare
|
@tmon-nordic : The fix does take in account that poll could also be used, but I haven't validated that this would fix this mixed usage. |
| @@ -0,0 +1,8 @@ | |||
| # SPDX-License-Identifier: Apache-2.0 | |||
There was a problem hiding this comment.
Thanks, but we do not need another CDC ACM sample.
| @@ -48,6 +48,7 @@ UDC_BUF_POOL_DEFINE(cdc_acm_ep_pool, | |||
| #define CDC_ACM_IRQ_TX_ENABLED 3 | |||
| #define CDC_ACM_RX_FIFO_BUSY 4 | |||
| #define CDC_ACM_LOCK 5 | |||
| #define CDC_ACM_TX_FIFO_BUSY 6 | |||
There was a problem hiding this comment.
I do not want to introduce TX_FIFO_BUSY, we can handle it by using delayed work, which is needed anyway, see 741d8a7.
| @@ -217,6 +218,11 @@ static int usbd_cdc_acm_request(struct usbd_class_data *const c_data, | |||
| atomic_clear_bit(&data->state, CDC_ACM_RX_FIFO_BUSY); | |||
| } | |||
|
|
|||
| if (bi->ep == cdc_acm_get_bulk_in(c_data)) { | |||
| atomic_clear_bit(&data->state, CDC_ACM_TX_FIFO_BUSY); | |||
| LOG_ERR("TX ERROR !!"); | |||
There was a problem hiding this comment.
There is no need to log an error here.
| if (ring_buf_is_empty(data->tx_fifo.rb)) { | ||
| /* Raise TX ready interrupt */ | ||
| if (data->cb) { | ||
| cdc_acm_work_submit(&data->irq_cb_work); | ||
| } | ||
| } else { | ||
| /* Queue pending TX data on IN endpoint */ | ||
| cdc_acm_work_submit(&data->tx_fifo_work); |
There was a problem hiding this comment.
No need to do anything other than trigger irq_cb_work.
Fixes #78104
Current situation before the fix:
When the endpoint is not read, due to no terminal application running, the last two steps doesn't occur and the cdc_acm simply occupies all available buffers, followed by rescheduling the same work task for tx_fifo directly, creating an endless loop.
By checking if a TX task is ongoing, the new work task scheduled as soon as the callback is given.
There is no timeout used in this case, as I couldn't find an interface to configure a timeout.