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

Jump to bottom

usbd: device_next: cdc: wait IN EP ready before enqueue data to EP #79104

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+26 −2
Open

usbd: device_next: cdc: wait IN EP ready before enqueue data to EP #79104

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
28 changes: 26 additions & 2 deletions subsys/usb/device_next/class/usbd_cdc_acm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ UDC_BUF_POOL_DEFINE(cdc_acm_ep_pool,
#define CDC_ACM_IRQ_RX_ENABLED 2
#define CDC_ACM_IRQ_TX_ENABLED 3
#define CDC_ACM_RX_FIFO_BUSY 4
#define CDC_ACM_TX_EP_BUSY 5

static struct k_work_q cdc_acm_work_q;
static K_KERNEL_STACK_DEFINE(cdc_acm_stack,
Expand Down Expand Up @@ -114,6 +115,8 @@ struct cdc_acm_uart_data {
struct k_work rx_fifo_work;
atomic_t state;
struct k_sem notif_sem;
/* USBD CDC ACM TX transaction deferred due to BULK IN EP busy */
bool tx_deferred;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why new variable instead of just using data->tx_fifo.altered like it was earlier?

Copy link
Contributor Author

@zeonchew zeonchew Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @tmon-nordic,

I was just about to add my report on what I have done here.

Since that #75439 has been merged into zephyr main, I have rebased my works and retested with the changes of #75439 applied. However, I am still able to reproduce this issue.

I have also run more stress test on my fixed ported over to the latest main, and found out that raising data->tx_fifo.altered wouldn't help in retrigger the TX transaction, as tx_fifo.altered is cleared in cdc_acm_irq_cb_handler().

zephyr/subsys/usb/device_next/class/usbd_cdc_acm.c

Lines 836 to 837 in a7aac8d

data->tx_fifo.altered = false;
data->rx_fifo.altered = false;

The previous testing wasn't rigorous enough, that the subsequent transaction was actually triggered by new data coming in from RX instead (and hence the stale data was flushed out together).

This being said, I have added another handling here to make sure that remaining data in tx_fifo, which the BULK IN endpoint is unable to send out in current transaction, gets sent out immediately after current TX is completed as well.

zephyr/subsys/usb/device_next/class/usbd_cdc_acm.c

Lines 571 to 580 in a7aac8d

/*
* If ring buffer contains more data than what the endpoint could
* send in one go, raise deferred flag for remaining data to be
* sent out once current transaction is completed.
*/
if (len == buf->size) {
if (!ring_buf_is_empty(data->tx_fifo.rb)) {
data->tx_deferred = true;
}
}

Please help to review whether this make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't understand cdc_acm_irq_cb_handler(). Isn't the data->tx_fifo.altered = false; going to make the if (data->tx_fifo.altered) effectively a dead code? Shouldn't this be fixed instead of adding new boolean?

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 don't understand cdc_acm_irq_cb_handler(). Isn't the data->tx_fifo.altered = false; going to make the if (data->tx_fifo.altered) effectively a dead code? Shouldn't this be fixed instead of adding new boolean?

I took me a while to understand this as well, from my understanding, in this handler, it will first clear all the altered flag, and then callback to the interrupt handler at application side: data->cb(...).

zephyr/subsys/usb/device_next/class/usbd_cdc_acm.c

Lines 841 to 844 in a7aac8d

if (atomic_test_bit(&data->state, CDC_ACM_IRQ_RX_ENABLED) ||
atomic_test_bit(&data->state, CDC_ACM_IRQ_TX_ENABLED)) {
data->cb(usbd_class_get_private(c_data), data->cb_data);
}

If there is more data to be transmitted by the application, the tx_fifo.altered flag will be raised by application before the callback returns. Then here we go, continue to send data out again.

I see that this part is checked in by @jfischer-no. @jfischer-no, can you please help to confirm whether this understanding is correct?
Thanks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't there a problem when there is more data in the tx ringbuffer? The sequence I think about is:

  • cdc_acm_irq_cb_handler() sets data->tx_fifo.altered = false;
  • data->cb(usbd_class_get_private(c_data), data->cb_data); is called, application writes to ring buffer more bytes than a CDC ACM TX buffer can fit
  • TX fifo work is triggered because data->tx_fifo.altered was set to true alongside ring buffer write
  • cdc_acm_tx_fifo_handler() calls len = ring_buf_get(data->tx_fifo.rb, buf->data, buf->size); and submits the transfer. The ring buffer still has pending TX data.
  • host reads all the data, usbd_cdc_acm_request() submits cdc_acm_work_submit(&data->irq_cb_work);
  • cdc_acm_irq_cb_handler() sets data->tx_fifo.altered = false;
  • data->cb(usbd_class_get_private(c_data), data->cb_data); is called, application doesn't have more data to write
  • TX fifo work is not triggered despite there being data in the ring buffer.

Is there anything preventing the above scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @tmon-nordic,

I added this for your case you mentioned in the latest commit just now ☺️

zephyr/subsys/usb/device_next/class/usbd_cdc_acm.c

Lines 571 to 580 in a7aac8d

/*
* If ring buffer contains more data than what the endpoint could
* send in one go, raise deferred flag for remaining data to be
* sent out once current transaction is completed.
*/
if (len == buf->size) {
if (!ring_buf_is_empty(data->tx_fifo.rb)) {
data->tx_deferred = true;
}
}

Copy link
Contributor

@tmon-nordic tmon-nordic Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Why to have two booleans for roughly the same? Wouldn't it be better to change the data->tx_fifo.altered logic completely? If we go with separate variable, I think bool tx_deferred; should be moved inside tx_fifo structure.

But is it really necessary to have tx_deferred? I think that the CDC_ACM_TX_EP_BUSY in combination with ring_buf_is_empty(data->tx_fifo.rb)) should be enough to determine whether the TX work has to be raised or not.

My idea would be to replace if (data->tx_fifo.altered) with if (!ring_buf_is_empty(data->tx_fifo.rb)) && !atomic_test_and_set_bit(&data->state, CDC_ACM_TX_EP_BUSY)). Then the CDC_ACM_TX_EP_BUSY would be cleared in usbd_cdc_acm_request() (and inside the TX handler if it fails to enqueue the buffer) when the IN request completes. This would essentially eliminate the need for data->tx_fifo.altered altogether and potentially make the code simpler to follow and more robust. Am I missing something?

Copy link
Contributor

Choose a reason for hiding this comment

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

Open Source development works on technical merit. I would say that there is no need to consult the original author when proposing a change that solves issues or otherwise enhances the code base.

While there may be times where you want to ask what was intended, there is also the possibility that the original author simply is no longer around.

Copy link
Contributor Author

@zeonchew zeonchew Oct 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

sorry, was going to replace my comment with another one just now but somehow I lost all of them.
(The comment on saying I am new to Open-Source and ZephyrOS, asking on how this community works, and whether it is okay to make such a change without consulting original author is correct way to do things.)

I am kind of agree that in this case, checking for ring_buf_is_empty() at cdc_acm_irq_cb_handler() plus checking for CDC_ACM_TX_EP_BUSY flag at cdc_acm_tx_fifo_handler() would be optimal as checking the ring buffer status directly gives us the most accurate status for actions to be taken.

It seems that the original intend of altered flag is to provide a fast access to the status whether application layer has accessed the ring buffer. However, it seems to me that the bit might not be good enough for the scenario in tx_fifo here anymore.

@jfischer-no , what do you think? Is @tmon-nordic suggestion here to check whether to trigger tx_fifo_work based on whether ring_buf_is_empty() sounds good to you as well?

};

static void cdc_acm_irq_rx_enable(const struct device *dev);
Expand Down Expand Up @@ -224,6 +227,8 @@ static int usbd_cdc_acm_request(struct usbd_class_data *const c_data,

if (bi->ep == cdc_acm_get_bulk_out(c_data)) {
atomic_clear_bit(&data->state, CDC_ACM_RX_FIFO_BUSY);
} else if (bi->ep == cdc_acm_get_bulk_in(c_data)) {
atomic_clear_bit(&data->state, CDC_ACM_TX_EP_BUSY);
}

goto ep_request_error;
Expand All @@ -245,6 +250,7 @@ static int usbd_cdc_acm_request(struct usbd_class_data *const c_data,

if (bi->ep == cdc_acm_get_bulk_in(c_data)) {
/* TX transfer completion */
atomic_clear_bit(&data->state, CDC_ACM_TX_EP_BUSY);
if (data->cb) {
cdc_acm_work_submit(&data->irq_cb_work);
}
Expand Down Expand Up @@ -522,7 +528,8 @@ static int cdc_acm_send_notification(const struct device *dev,
}

/*
* TX handler is triggered when the state of TX fifo has been altered.
* TX handler is triggered when the state of TX fifo has been altered or when
* there is deferred TX transaction
*/
static void cdc_acm_tx_fifo_handler(struct k_work *work)
{
Expand All @@ -535,6 +542,7 @@ static void cdc_acm_tx_fifo_handler(struct k_work *work)

data = CONTAINER_OF(dwork, struct cdc_acm_uart_data, tx_fifo_work);
c_data = data->c_data;
data->tx_deferred = false;

if (!atomic_test_bit(&data->state, CDC_ACM_CLASS_ENABLED)) {
LOG_DBG("USB configuration is not enabled");
Expand All @@ -546,6 +554,11 @@ static void cdc_acm_tx_fifo_handler(struct k_work *work)
return;
}

if (atomic_test_and_set_bit(&data->state, CDC_ACM_TX_EP_BUSY)) {
data->tx_deferred = true;
return;
}

buf = cdc_acm_buf_alloc(cdc_acm_get_bulk_in(c_data));
if (buf == NULL) {
cdc_acm_work_schedule(&data->tx_fifo_work, K_MSEC(1));
Expand All @@ -555,6 +568,17 @@ static void cdc_acm_tx_fifo_handler(struct k_work *work)
len = ring_buf_get(data->tx_fifo.rb, buf->data, buf->size);
net_buf_add(buf, len);

/*
* If ring buffer contains more data than what the endpoint could
* send in one go, raise deferred flag for remaining data to be
* sent out once current transaction is completed.
*/
if (len == buf->size) {
if (!ring_buf_is_empty(data->tx_fifo.rb)) {
data->tx_deferred = true;
}
}

ret = usbd_ep_enqueue(c_data, buf);
if (ret) {
LOG_ERR("Failed to enqueue");
Expand Down Expand Up @@ -824,7 +848,7 @@ static void cdc_acm_irq_cb_handler(struct k_work *work)
cdc_acm_work_submit(&data->rx_fifo_work);
}

if (data->tx_fifo.altered) {
if (data->tx_fifo.altered || data->tx_deferred) {
LOG_DBG("tx fifo altered, submit work");
cdc_acm_work_schedule(&data->tx_fifo_work, K_NO_WAIT);
}
Expand Down
Loading