Skip to content

usb: device_next: wait for device to resume after remote wakeup signal #73362

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
+45 −40
Open

usb: device_next: wait for device to resume after remote wakeup signal #73362

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f88c864
usb: device_next: wait for device to resume after remote wakeup signal
jfischer-no Apr 18, 2024
f9b1e6f
usb: device_next: try to signal remote wakeup before enqueue
jfischer-no May 27, 2024
6e816d2
usb: device_next: remove the SUSPENDED flag from CDC implementations
jfischer-no Apr 22, 2024
af11d1b
usb: device_next: cdc_acm: unref buf if enqueue a notification failed
jfischer-no May 24, 2024
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
2 changes: 2 additions & 0 deletions include/zephyr/usb/usbd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,8 @@ struct usbd_contex {
const char *name;
/** Access mutex */
struct k_mutex mutex;
/** Remote wakeup semaphore */
struct k_sem rwup_sem;
/** Pointer to UDC device */
const struct device *dev;
/** Notification message recipient callback */
Expand Down
45 changes: 17 additions & 28 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 @@ -41,12 +41,13 @@ NET_BUF_POOL_FIXED_DEFINE(cdc_acm_ep_pool,
#define CDC_ACM_FS_INT_EP_INTERVAL USB_FS_INT_EP_INTERVAL(10000U)
#define CDC_ACM_HS_INT_EP_INTERVAL USB_HS_INT_EP_INTERVAL(10000U)

#define CDC_ACM_CLASS_ENABLED 0
#define CDC_ACM_CLASS_SUSPENDED 1
#define CDC_ACM_IRQ_RX_ENABLED 2
#define CDC_ACM_IRQ_TX_ENABLED 3
#define CDC_ACM_RX_FIFO_BUSY 4
#define CDC_ACM_LOCK 5
enum {
CDC_ACM_CLASS_ENABLED,
CDC_ACM_IRQ_RX_ENABLED,
CDC_ACM_IRQ_TX_ENABLED,
CDC_ACM_RX_FIFO_BUSY,
CDC_ACM_LOCK,
};

static struct k_work_q cdc_acm_work_q;
static K_KERNEL_STACK_DEFINE(cdc_acm_stack,
Expand Down Expand Up @@ -278,25 +279,21 @@ static void usbd_cdc_acm_disable(struct usbd_class_data *const c_data)
struct cdc_acm_uart_data *data = dev->data;

atomic_clear_bit(&data->state, CDC_ACM_CLASS_ENABLED);
atomic_clear_bit(&data->state, CDC_ACM_CLASS_SUSPENDED);
LOG_INF("Configuration disabled");
}

static void usbd_cdc_acm_suspended(struct usbd_class_data *const c_data)
{
const struct device *dev = usbd_class_get_private(c_data);
struct cdc_acm_uart_data *data = dev->data;

/* FIXME: filter stray suspended events earlier */
atomic_set_bit(&data->state, CDC_ACM_CLASS_SUSPENDED);
LOG_DBG("CDC ACM device %s suspended", dev->name);
}

static void usbd_cdc_acm_resumed(struct usbd_class_data *const c_data)
{
const struct device *dev = usbd_class_get_private(c_data);
struct cdc_acm_uart_data *data = dev->data;

atomic_clear_bit(&data->state, CDC_ACM_CLASS_SUSPENDED);
LOG_DBG("CDC ACM device %s resumed", dev->name);
}

static void *usbd_cdc_acm_get_desc(struct usbd_class_data *const c_data,
Expand Down Expand Up @@ -485,11 +482,6 @@ static int cdc_acm_send_notification(const struct device *dev,
return -EACCES;
}

if (atomic_test_bit(&data->state, CDC_ACM_CLASS_SUSPENDED)) {
LOG_INF("USB support is suspended (FIXME)");
return -EACCES;
}

ep = cdc_acm_get_int_in(c_data);
buf = usbd_ep_buf_alloc(c_data, ep, sizeof(struct cdc_acm_notification));
if (buf == NULL) {
Expand All @@ -498,10 +490,14 @@ static int cdc_acm_send_notification(const struct device *dev,

net_buf_add_mem(buf, &notification, 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;
return 0;
}

/*
Expand All @@ -523,11 +519,6 @@ static void cdc_acm_tx_fifo_handler(struct k_work *work)
return;
}

if (atomic_test_bit(&data->state, CDC_ACM_CLASS_SUSPENDED)) {
LOG_INF("USB support is suspended (FIXME: submit rwup)");
return;
}

if (atomic_test_and_set_bit(&data->state, CDC_ACM_LOCK)) {
cdc_acm_work_submit(&data->tx_fifo_work);
return;
Expand Down Expand Up @@ -558,7 +549,6 @@ static void cdc_acm_tx_fifo_handler(struct k_work *work)
* - (x) RX transfer completion
* - (x) the end of cdc_acm_irq_cb_handler
* - (x) USBD class API enable call
* - ( ) USBD class API resumed call (TODO)
*/
static void cdc_acm_rx_fifo_handler(struct k_work *work)
{
Expand All @@ -571,9 +561,8 @@ static void cdc_acm_rx_fifo_handler(struct k_work *work)
data = CONTAINER_OF(work, struct cdc_acm_uart_data, rx_fifo_work);
c_data = data->c_data;

if (!atomic_test_bit(&data->state, CDC_ACM_CLASS_ENABLED) ||
atomic_test_bit(&data->state, CDC_ACM_CLASS_SUSPENDED)) {
LOG_INF("USB configuration is not enabled or suspended");
if (!atomic_test_bit(&data->state, CDC_ACM_CLASS_ENABLED)) {
LOG_INF("USB configuration is not enabled");
return;
}

Expand Down
13 changes: 2 additions & 11 deletions subsys/usb/device_next/class/usbd_cdc_ecm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ LOG_MODULE_REGISTER(cdc_ecm, CONFIG_USBD_CDC_ECM_LOG_LEVEL);
enum {
CDC_ECM_IFACE_UP,
CDC_ECM_CLASS_ENABLED,
CDC_ECM_CLASS_SUSPENDED,
CDC_ECM_OUT_ENGAGED,
};

Expand Down Expand Up @@ -334,11 +333,6 @@ static int cdc_ecm_send_notification(const struct device *dev,
return 0;
}

if (atomic_test_bit(&data->state, CDC_ECM_CLASS_SUSPENDED)) {
LOG_INF("USB device is suspended (FIXME)");
return 0;
}

ep = cdc_ecm_get_int_in(c_data);
buf = usbd_ep_buf_alloc(c_data, ep, sizeof(struct cdc_ecm_notification));
if (buf == NULL) {
Expand Down Expand Up @@ -401,24 +395,21 @@ static void usbd_cdc_ecm_disable(struct usbd_class_data *const c_data)
net_if_carrier_off(data->iface);
}

atomic_clear_bit(&data->state, CDC_ECM_CLASS_SUSPENDED);
LOG_INF("Configuration disabled");
}

static void usbd_cdc_ecm_suspended(struct usbd_class_data *const c_data)
{
const struct device *dev = usbd_class_get_private(c_data);
struct cdc_ecm_eth_data *data = dev->data;

atomic_set_bit(&data->state, CDC_ECM_CLASS_SUSPENDED);
LOG_DBG("CDC ECM device %s suspended", dev->name);
}

static void usbd_cdc_ecm_resumed(struct usbd_class_data *const c_data)
{
const struct device *dev = usbd_class_get_private(c_data);
struct cdc_ecm_eth_data *data = dev->data;

atomic_clear_bit(&data->state, CDC_ECM_CLASS_SUSPENDED);
LOG_DBG("CDC ECM device %s resumed", dev->name);
}

static int usbd_cdc_ecm_ctd(struct usbd_class_data *const c_data,
Expand Down
2 changes: 2 additions & 0 deletions subsys/usb/device_next/usbd_core.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,8 @@ int usbd_device_init_core(struct usbd_contex *const uds_ctx)
{
int ret;

k_sem_init(&uds_ctx->rwup_sem, 0, 1);

ret = udc_init(uds_ctx->dev, usbd_event_carrier);
if (ret != 0) {
LOG_ERR("Failed to init device driver");
Expand Down
13 changes: 13 additions & 0 deletions subsys/usb/device_next/usbd_device.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@ int usbd_device_set_code_triple(struct usbd_contex *const uds_ctx,
int usbd_wakeup_request(struct usbd_contex *const uds_ctx)
{
struct udc_device_caps caps = udc_caps(uds_ctx->dev);
const k_timeout_t resume_timeout = K_MSEC(25);
int ret = 0;

usbd_device_lock(uds_ctx);
Expand All @@ -165,7 +166,19 @@ int usbd_wakeup_request(struct usbd_contex *const uds_ctx)
goto wakeup_request_error;
}

k_sem_reset(&uds_ctx->rwup_sem);
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 this prone to race conditions where the host resumes the device before usbd_wakeup_request() obtains the usbd lock?

Using k_event that would have a bit that signifies not suspended would be immune to such race. Then the event would be set and cleared in usbd_status_suspended() with k_event_set_masked(&uds_ctx->events, value ? 0 : UDS_NOT_SUSPENDED_BIT, UDS_NOT_SUSPENDED_BIT); and the wait here would be changed to k_event_wait(&uds_ctx->events, UDS_NOT_SUSPENDED_BIT, false, resume_timeout);.

The name not suspended might not be the best, an alternative could be active but it might not be that good either. It would be important to make it not suspended because k_event_wait only allows to wait until an event bit gets set (there is no wait function for event bit to become cleared).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Isn't this prone to race conditions where the host resumes the device before usbd_wakeup_request() obtains the usbd lock?

Yes, e.g. if the suspend time is 0.

Using k_event that would have a bit that signifies not suspended would be immune to such race. Then the event would be set and cleared in usbd_status_suspended() with k_event_set_masked(&uds_ctx->events, value ? 0 : UDS_NOT_SUSPENDED_BIT, UDS_NOT_SUSPENDED_BIT); and the wait here would be changed to k_event_wait(&uds_ctx->events, UDS_NOT_SUSPENDED_BIT, false, resume_timeout);.

I guessed you would suggest it, I will try it out.

The name not suspended might not be the best, an alternative could be active but it might not be that good either. It would be important to make it not suspended because k_event_wait only allows to wait until an event bit gets set (there is no wait function for event bit to become cleared).

ret = udc_host_wakeup(uds_ctx->dev);
if (ret) {
LOG_ERR("Failed to signal remote wakekup, %d", ret);
goto wakeup_request_error;
}

ret = k_sem_take(&uds_ctx->rwup_sem, resume_timeout);
if (ret) {
LOG_ERR("Remote wakeup timeout");
} else {
LOG_DBG("Resumed after remote wakeup");
}

wakeup_request_error:
usbd_device_unlock(uds_ctx);
Expand Down
3 changes: 3 additions & 0 deletions subsys/usb/device_next/usbd_device.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,9 @@ static inline void usbd_status_suspended(struct usbd_contex *const uds_ctx,
const bool value)
{
uds_ctx->status.suspended = value;
if (!value) {
k_sem_give(&uds_ctx->rwup_sem);
}
}

/**
Expand Down
7 changes: 6 additions & 1 deletion subsys/usb/device_next/usbd_endpoint.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,12 @@ int usbd_ep_enqueue(const struct usbd_class_data *const c_data,

if (USB_EP_DIR_IS_IN(bi->ep)) {
if (usbd_is_suspended(uds_ctx)) {
return -EPERM;
int ret;

ret = usbd_wakeup_request(uds_ctx);
if (ret != 0) {
return ret;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we really be ending enqueue with an error if device is suspended (and wakeup fails or feature is cleared by the host)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question, if it is okay to enqueue in suspended state, then should we call remote wakeup signaling here or from the driver?

Copy link
Contributor

Choose a reason for hiding this comment

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

Linux dwc2 gadget driver seems to allow queuing transfers only in active state (L0). If the device is in any other state (suspended), then it fails the queue with -EAGAIN. But it really depends on the gadget driver - some do fail when suspended, and some do not care.

}
}
}

Expand Down
Loading