From 2cab3a8243104ffd55983af7ba4f2d2a79e47bba Mon Sep 17 00:00:00 2001 From: Johann Fischer Date: Wed, 4 Jan 2023 10:51:51 +0100 Subject: [PATCH] drivers: usb: udc_kinetis: fix race condition in Kinetis USBFSOTG Periodic enqeueu of buffers can cause a attempt to start a new transfer even though an endpoint is already busy. Split usbfsotg_xfer_start() into two function, one to start next transfer and another to continue the transfers, and use busy state flags to explicitly mark an endpoint busy. Signed-off-by: Johann Fischer --- drivers/usb/udc/udc_kinetis.c | 47 +++++++++++++++++++++++------------ 1 file changed, 31 insertions(+), 16 deletions(-) diff --git a/drivers/usb/udc/udc_kinetis.c b/drivers/usb/udc/udc_kinetis.c index aa94222e387..91f93c1768b 100644 --- a/drivers/usb/udc/udc_kinetis.c +++ b/drivers/usb/udc/udc_kinetis.c @@ -170,25 +170,20 @@ static ALWAYS_INLINE void usbfsotg_resume_tx(const struct device *dev) base->CTL &= ~USB_CTL_TXSUSPENDTOKENBUSY_MASK; } -/* Initiate a new transfer, must not be used for control endpoint OUT */ -static int usbfsotg_xfer_start(const struct device *dev, - struct udc_ep_config *const cfg) +static int usbfsotg_xfer_continue(const struct device *dev, + struct udc_ep_config *const cfg, + struct net_buf *const buf) { const struct usbfsotg_config *config = dev->config; USB_Type *base = config->base; struct usbfsotg_bd *bd; - struct net_buf *buf; uint8_t *data_ptr; size_t len; - buf = udc_buf_peek(dev, cfg->addr); - if (buf == NULL) { - return -ENODATA; - } - bd = usbfsotg_get_ebd(dev, cfg, false); - if (usbfsotg_bd_is_busy(bd)) { - LOG_DBG("ep 0x%02x buf busy", cfg->addr); + if (unlikely(usbfsotg_bd_is_busy(bd))) { + LOG_ERR("ep 0x%02x buf busy", cfg->addr); + __ASSERT_NO_MSG(false); return -EBUSY; } @@ -213,6 +208,20 @@ static int usbfsotg_xfer_start(const struct device *dev, return 0; } +/* Initiate a new transfer, must not be used for control endpoint OUT */ +static int usbfsotg_xfer_next(const struct device *dev, + struct udc_ep_config *const cfg) +{ + struct net_buf *buf; + + buf = udc_buf_peek(dev, cfg->addr); + if (buf == NULL) { + return -ENODATA; + } + + return usbfsotg_xfer_continue(dev, cfg, buf); +} + static inline int usbfsotg_ctrl_feed_start(const struct device *dev, struct net_buf *const buf) { @@ -465,9 +474,11 @@ static void xfer_work_handler(struct k_work *item) break; case USBFSOTG_EVT_DOUT: err = work_handler_out(ev->dev, ev->ep); + udc_ep_set_busy(ev->dev, ev->ep, false); break; case USBFSOTG_EVT_DIN: err = work_handler_in(ev->dev, ev->ep); + udc_ep_set_busy(ev->dev, ev->ep, false); break; case USBFSOTG_EVT_CLEAR_HALT: err = usbfsotg_ep_clear_halt(ev->dev, ep_cfg); @@ -481,8 +492,10 @@ static void xfer_work_handler(struct k_work *item) } /* Peek next transer */ - if (ev->ep != USB_CONTROL_EP_OUT) { - usbfsotg_xfer_start(ev->dev, ep_cfg); + if (ev->ep != USB_CONTROL_EP_OUT && !udc_ep_is_busy(ev->dev, ev->ep)) { + if (usbfsotg_xfer_next(ev->dev, ep_cfg) == 0) { + udc_ep_set_busy(ev->dev, ev->ep, true); + } } xfer_work_error: @@ -576,7 +589,7 @@ static ALWAYS_INLINE void isr_handle_xfer_done(const struct device *dev, if (ep == USB_CONTROL_EP_OUT) { usbfsotg_ctrl_feed_start(dev, buf); } else { - usbfsotg_xfer_start(dev, ep_cfg); + usbfsotg_xfer_continue(dev, ep_cfg, buf); } } else { if (ep == USB_CONTROL_EP_OUT) { @@ -600,10 +613,10 @@ static ALWAYS_INLINE void isr_handle_xfer_done(const struct device *dev, net_buf_pull(buf, len); if (buf->len) { - usbfsotg_xfer_start(dev, ep_cfg); + usbfsotg_xfer_continue(dev, ep_cfg, buf); } else { if (udc_ep_buf_has_zlp(buf)) { - usbfsotg_xfer_start(dev, ep_cfg); + usbfsotg_xfer_continue(dev, ep_cfg, buf); udc_ep_buf_clear_zlp(buf); break; } @@ -718,6 +731,8 @@ static int usbfsotg_ep_dequeue(const struct device *dev, udc_submit_event(dev, UDC_EVT_EP_REQUEST, -ECONNABORTED, buf); } + udc_ep_set_busy(dev, cfg->addr, false); + return 0; }