From fd574baeb728f2da39b5b2ea366f12b23bd4db59 Mon Sep 17 00:00:00 2001 From: sakumisu <1203593632@qq.com> Date: Thu, 17 Mar 2022 16:40:54 +0800 Subject: [PATCH] fix critical section wrong use --- class/hub/usbh_hub.c | 6 +-- core/usbh_core.c | 95 +++++++++++++++------------------ core/usbh_core.h | 2 - port/ehci/usb_ehci.c | 34 ++++++------ port/synopsys/usb_hc_synopsys.c | 87 ++++++++++++------------------ 5 files changed, 95 insertions(+), 129 deletions(-) diff --git a/class/hub/usbh_hub.c b/class/hub/usbh_hub.c index 76642cb0..69b75dd2 100644 --- a/class/hub/usbh_hub.c +++ b/class/hub/usbh_hub.c @@ -29,7 +29,7 @@ static uint32_t g_devinuse = 0; usb_slist_t hub_class_head = USB_SLIST_OBJECT_INIT(hub_class_head); -USB_NOCACHE_RAM_SECTION uint8_t int_buffer[6][USBH_HUB_INTIN_BUFSIZE]; +USB_MEM_ALIGN32 uint8_t int_buffer[6][USBH_HUB_INTIN_BUFSIZE]; extern void usbh_external_hport_connect(struct usbh_hubport *hport); extern void usbh_external_hport_disconnect(struct usbh_hubport *hport); extern void usbh_hport_activate(struct usbh_hubport *hport); @@ -326,7 +326,6 @@ static void usbh_extern_hub_psc_event(void *arg) uint16_t change; uint16_t mask; uint16_t feat; - uint32_t flags; int ret; hub_class = (struct usbh_hub *)arg; @@ -484,11 +483,10 @@ static void usbh_extern_hub_psc_event(void *arg) /* Hub status changed */ USB_LOG_WRN("Hub status changed, not handled\n"); } - flags = usb_osal_enter_critical_section(); + if (hub_class->parent->connected) { ret = usbh_ep_intr_async_transfer(hub_class->intin, hub_class->int_buffer, USBH_HUB_INTIN_BUFSIZE, usbh_external_hub_callback, hub_class); } - usb_osal_leave_critical_section(flags); } static void usbh_external_hub_callback(void *arg, int nbytes) diff --git a/core/usbh_core.c b/core/usbh_core.c index 8d1de09e..8850e6f0 100644 --- a/core/usbh_core.c +++ b/core/usbh_core.c @@ -28,8 +28,6 @@ static const char *speed_table[] = { "error speed", "low speed", "full speed", "high speed" }; static const struct usbh_class_driver *usbh_find_class_driver(uint8_t class, uint8_t subcalss, uint8_t protocol, uint16_t vid, uint16_t pid); -void usbh_hport_activate(struct usbh_hubport *hport); -void usbh_hport_deactivate(struct usbh_hubport *hport); /* general descriptor field offsets */ #define DESC_bLength 0 /** Length offset */ @@ -154,6 +152,44 @@ static int usbh_devaddr_destroy(struct usbh_hubport *hport, uint8_t dev_addr) return usbh_free_devaddr(&rhport->devgen, dev_addr); } +void usbh_hport_activate(struct usbh_hubport *hport) +{ + struct usbh_endpoint_cfg ep0_cfg; + + memset(&ep0_cfg, 0, sizeof(struct usbh_endpoint_cfg)); + + ep0_cfg.ep_addr = 0x00; + ep0_cfg.ep_interval = 0x00; + ep0_cfg.ep_mps = 0x08; + ep0_cfg.ep_type = USB_ENDPOINT_TYPE_CONTROL; + ep0_cfg.hport = hport; + /* Allocate memory for roothub port control endpoint */ + usbh_ep_alloc(&hport->ep0, &ep0_cfg); +} + +void usbh_hport_deactivate(struct usbh_hubport *hport) +{ + uint32_t flags; + + /* Don't free the control pipe of root hub ports! */ + if (hport->parent != NULL && hport->ep0 != NULL) { + usb_ep_cancel(hport->ep0); + usbh_ep_free(hport->ep0); + hport->ep0 = NULL; + } + + flags = usb_osal_enter_critical_section(); + /* Free the device address if one has been assigned */ + usbh_devaddr_destroy(hport, hport->dev_addr); + hport->dev_addr = 0; + usb_osal_leave_critical_section(flags); + + if (hport->setup) + usb_iofree(hport->setup); + + hport->setup = NULL; +} + static int parse_device_descriptor(struct usbh_hubport *hport, struct usb_device_descriptor *desc, uint16_t length) { if (desc->bLength != USB_SIZEOF_DEVICE_DESC) { @@ -673,17 +709,14 @@ static int usbh_portchange_wait(struct usbh_hubport **hport) static void usbh_portchange_detect_thread(void *argument) { struct usbh_hubport *hport = NULL; - uint32_t flags; - - flags = usb_osal_enter_critical_section(); - usb_hc_init(); for (uint8_t port = USBH_HUB_PORT_START_INDEX; port <= CONFIG_USBHOST_RHPORTS; port++) { usbh_core_cfg.rhport[port - 1].hport.port = port; usbh_core_cfg.rhport[port - 1].devgen.next = 1; usbh_hport_activate(&usbh_core_cfg.rhport[port - 1].hport); } - usb_osal_leave_critical_section(flags); + + usb_hc_init(); while (1) { usbh_portchange_wait(&hport); @@ -699,9 +732,7 @@ static void usbh_portchange_detect_thread(void *argument) } else { USB_LOG_INFO("Hub %u, Port %u connected, %s\r\n", hport->parent->index, hport->port, speed_table[hport->speed]); } - usb_osal_thread_suspend(g_lpworkq.thread); usbh_enumerate(hport); - usb_osal_thread_resume(g_lpworkq.thread); } else { usbh_hport_deactivate(hport); for (uint8_t i = 0; i < hport->config.config_desc.bNumInterfaces; i++) { @@ -734,7 +765,9 @@ void usbh_external_hport_connect(struct usbh_hubport *hport) if (usbh_core_cfg.pscwait) { usbh_core_cfg.pscwait = false; + usb_osal_leave_critical_section(flags); usb_osal_sem_give(usbh_core_cfg.pscsem); + return; } usb_osal_leave_critical_section(flags); @@ -750,54 +783,14 @@ void usbh_external_hport_disconnect(struct usbh_hubport *hport) if (usbh_core_cfg.pscwait) { usbh_core_cfg.pscwait = false; + usb_osal_leave_critical_section(flags); usb_osal_sem_give(usbh_core_cfg.pscsem); + return; } usb_osal_leave_critical_section(flags); } -void usbh_hport_activate(struct usbh_hubport *hport) -{ - struct usbh_endpoint_cfg ep0_cfg; - uint32_t flags; - - flags = usb_osal_enter_critical_section(); - memset(&ep0_cfg, 0, sizeof(struct usbh_endpoint_cfg)); - - ep0_cfg.ep_addr = 0x00; - ep0_cfg.ep_interval = 0x00; - ep0_cfg.ep_mps = 0x08; - ep0_cfg.ep_type = USB_ENDPOINT_TYPE_CONTROL; - ep0_cfg.hport = hport; - /* Allocate memory for roothub port control endpoint */ - usbh_ep_alloc(&hport->ep0, &ep0_cfg); - - usb_osal_leave_critical_section(flags); -} - -void usbh_hport_deactivate(struct usbh_hubport *hport) -{ - uint32_t flags; - - flags = usb_osal_enter_critical_section(); - /* Don't free the control pipe of root hub ports! */ - if (hport->parent != NULL && hport->ep0 != NULL) { - usb_ep_cancel(hport->ep0); - usbh_ep_free(hport->ep0); - hport->ep0 = NULL; - } - /* Free the device address if one has been assigned */ - usbh_devaddr_destroy(hport, hport->dev_addr); - - if (hport->setup) - usb_iofree(hport->setup); - - hport->setup = NULL; - hport->dev_addr = 0; - - usb_osal_leave_critical_section(flags); -} - void usbh_event_notify_handler(uint8_t event, uint8_t rhport) { switch (event) { diff --git a/core/usbh_core.h b/core/usbh_core.h index df4ae26d..fb001771 100644 --- a/core/usbh_core.h +++ b/core/usbh_core.h @@ -119,8 +119,6 @@ typedef struct usbh_hub { void usbh_event_notify_handler(uint8_t event, uint8_t rhport); int usbh_initialize(void); -int usbh_lock(void); -void usbh_unlock(void); int lsusb(int argc, char **argv); struct usbh_hubport *usbh_find_hubport(uint8_t dev_addr); void *usbh_find_class_instance(const char *devname); diff --git a/port/ehci/usb_ehci.c b/port/ehci/usb_ehci.c index 8e6e1175..0da339ae 100644 --- a/port/ehci/usb_ehci.c +++ b/port/ehci/usb_ehci.c @@ -750,7 +750,7 @@ static struct usb_ehci_qh_s *usb_ehci_qh_create(struct usb_ehci_epinfo_s *epinfo #ifndef CONFIG_USBHOST_INT_DISABLE if (epinfo->xfrtype == USB_ENDPOINT_TYPE_INTERRUPT) { - regval |= ((uint32_t)epinfo->interval << QH_EPCAPS_SSMASK_SHIFT); + regval |= ((uint32_t)1 << QH_EPCAPS_SSMASK_SHIFT); } #endif @@ -1831,8 +1831,11 @@ static inline void usb_ehci_portsc_bottomhalf(void) /* Handle port connection status change (CSC) events */ if ((portsc & EHCI_PORTSC_CSC) != 0) { - /* Check current connect status */ - if ((portsc & (EHCI_PORTSC_CCS | EHCI_PORTSC_PE)) != 0) { + /* Debounce */ + usb_osal_msleep(25); + /* Check current connect status*/ + portsc = usb_ehci_getreg(&HCOR->portsc[rhpndx]); + if ((portsc & EHCI_PORTSC_CCS) == EHCI_PORTSC_CCS) { /* Connected ... Did we just become connected? */ if (!g_ehci.connected) { g_ehci.connected = 1; @@ -2032,7 +2035,12 @@ int usb_hc_init(void) uintptr_t physaddr1; uintptr_t physaddr2; - memset(&g_ehci, 0, sizeof(struct usb_ehci_s)); + g_ehci.connected = 0; + g_ehci.qhfree = NULL; + g_ehci.qtdfree = NULL; + + usb_slist_init(&g_ehci.epinfo_list); + /* Initialize the list of free Queue Head (QH) structures */ for (uint8_t i = 0; i < CONFIG_USB_EHCI_QH_NUM; i++) { @@ -2257,21 +2265,16 @@ int usbh_ep0_reconfigure(usbh_epinfo_t ep, uint8_t dev_addr, uint8_t ep_mps, uin int usbh_ep_alloc(usbh_epinfo_t *ep, const struct usbh_endpoint_cfg *ep_cfg) { - int ret; struct usb_ehci_epinfo_s *epinfo; struct usbh_hubport *hport; DEBUGASSERT(ep_cfg != NULL && ep_cfg->hport != NULL); - ret = usb_osal_mutex_take(g_ehci.exclsem); - if (ret < 0) { - return ret; - } - hport = ep_cfg->hport; /* new roothub ep info */ if (((ep_cfg->ep_type & USB_ENDPOINT_TYPE_MASK) == USB_ENDPOINT_TYPE_CONTROL) && (hport->parent == NULL)) { + memset(&g_ehci.ep0, 0, sizeof(struct usb_ehci_epinfo_s)); epinfo = &g_ehci.ep0[hport->port - 1]; } else { /* new exteranl hub ep info */ @@ -2294,28 +2297,21 @@ int usbh_ep_alloc(usbh_epinfo_t *ep, const struct usbh_endpoint_cfg *ep_cfg) epinfo->hport = hport; epinfo->iocsem = usb_osal_sem_create(0); + usb_slist_add_tail(&g_ehci.epinfo_list, &epinfo->list); *ep = epinfo; - usb_slist_add_tail(&g_ehci.epinfo_list, &epinfo->list); - usb_osal_mutex_give(g_ehci.exclsem); + return 0; } int usbh_ep_free(usbh_epinfo_t ep) { - int ret; struct usb_ehci_epinfo_s *epinfo = (struct usb_ehci_epinfo_s *)ep; - ret = usb_osal_mutex_take(g_ehci.exclsem); - if (ret < 0) { - return ret; - } - usb_osal_sem_delete(epinfo->iocsem); usb_slist_remove(&g_ehci.epinfo_list, &epinfo->list); usb_free(epinfo); - usb_osal_mutex_give(g_ehci.exclsem); return 0; } diff --git a/port/synopsys/usb_hc_synopsys.c b/port/synopsys/usb_hc_synopsys.c index 3a76ff30..ff5f97e5 100644 --- a/port/synopsys/usb_hc_synopsys.c +++ b/port/synopsys/usb_hc_synopsys.c @@ -73,7 +73,6 @@ struct usb_synopsys_priv { * Allocate a channel. * ****************************************************************************/ - static int usb_synopsys_chan_alloc(struct usb_synopsys_priv *priv) { int chidx; @@ -119,7 +118,6 @@ static void usb_synopsys_chan_free(struct usb_synopsys_priv *priv, int chidx) * Free all channels. * ****************************************************************************/ - static inline void usb_synopsys_chan_freeall(struct usb_synopsys_priv *priv) { uint8_t chidx; @@ -145,7 +143,6 @@ static inline void usb_synopsys_chan_freeall(struct usb_synopsys_priv *priv) * started. * ****************************************************************************/ - static int usb_synopsys_chan_waitsetup(struct usb_synopsys_priv *priv, struct usb_synopsys_chan *chan) { @@ -189,7 +186,6 @@ static int usb_synopsys_chan_waitsetup(struct usb_synopsys_priv *priv, * Might be called from the level of an interrupt handler * ****************************************************************************/ - #ifdef CONFIG_USBHOST_ASYNCH static int usb_synopsys_chan_asynchsetup(struct usb_synopsys_priv *priv, struct usb_synopsys_chan *chan, @@ -221,7 +217,7 @@ static int usb_synopsys_chan_asynchsetup(struct usb_synopsys_priv *priv, #endif /**************************************************************************** - * Name: stm32_chan_wait + * Name: usb_synopsys_chan_wait * * Description: * Wait for a transfer on a channel to complete. @@ -230,7 +226,6 @@ static int usb_synopsys_chan_asynchsetup(struct usb_synopsys_priv *priv, * Called from a normal thread context * ****************************************************************************/ - static int usb_synopsys_chan_wait(struct usb_synopsys_priv *priv, struct usb_synopsys_chan *chan) { int ret; @@ -254,7 +249,7 @@ static int usb_synopsys_chan_wait(struct usb_synopsys_priv *priv, struct usb_syn } /**************************************************************************** - * Name: stm32_chan_wakeup + * Name: usb_synopsys_chan_wakeup * * Description: * A channel transfer has completed... wakeup any threads waiting for the @@ -265,7 +260,6 @@ static int usb_synopsys_chan_wait(struct usb_synopsys_priv *priv, struct usb_syn * the channel. Interrupts are disabled. * ****************************************************************************/ - static void usb_synopsys_chan_wakeup(struct usb_synopsys_priv *priv, struct usb_synopsys_chan *chan) { usbh_asynch_callback_t callback; @@ -323,7 +317,9 @@ __WEAK void usb_hc_low_level_init(void) int usb_hc_init(void) { - memset(&g_usbhost, 0, sizeof(struct usb_synopsys_priv)); + g_usbhost.sof_timer = 0; + g_usbhost.connected = 0; + g_usbhost.pscwait = 0; #if defined(CONFIG_USB_HS) || defined(CONFIG_USB_HS_IN_FULL) g_usbhost.handle = &hhcd_USB_OTG_HS; g_usbhost.handle->Instance = USB_OTG_HS; @@ -334,15 +330,6 @@ int usb_hc_init(void) g_usbhost.exclsem = usb_osal_mutex_create(); - for (uint8_t i = 0; i < CONFIG_USBHOST_CHANNELS; i++) { - struct usb_synopsys_chan *chan = &g_usbhost.chan[i]; - - /* The waitsem semaphore is used for signaling and, hence, should not - * have priority inheritance enabled. - */ - chan->waitsem = usb_osal_sem_create(0); - } - g_usbhost.handle->Init.Host_channels = CONFIG_USBHOST_CHANNELS; g_usbhost.handle->Init.speed = HCD_SPEED_FULL; g_usbhost.handle->Init.dma_enable = DISABLE; @@ -405,32 +392,32 @@ int usbh_ep_alloc(usbh_epinfo_t *ep, const struct usbh_endpoint_cfg *ep_cfg) struct usb_synopsys_ctrlinfo *ep0; struct usbh_hubport *hport; int chidx; - int ret; uint8_t speed; - ret = usb_osal_mutex_take(g_usbhost.exclsem); - if (ret < 0) { - return ret; - } - hport = ep_cfg->hport; if (hport->speed == USB_SPEED_FULL) { speed = 1; } else if (hport->speed == USB_SPEED_LOW) { speed = 2; + } else if (hport->speed == USB_SPEED_HIGH) { + speed = 0; } if (ep_cfg->ep_type == USB_ENDPOINT_TYPE_CONTROL) { ep0 = usb_malloc(sizeof(struct usb_synopsys_ctrlinfo)); + memset(ep0, 0, sizeof(struct usb_synopsys_ctrlinfo)); ep0->outndx = usb_synopsys_chan_alloc(&g_usbhost); ep0->inndx = usb_synopsys_chan_alloc(&g_usbhost); chan = &priv->chan[ep0->outndx]; - chan->interval = 0; + memset(chan, 0, sizeof(struct usb_synopsys_chan)); + chan->waitsem = usb_osal_sem_create(0); + chan = &priv->chan[ep0->inndx]; - chan->interval = 0; + memset(chan, 0, sizeof(struct usb_synopsys_chan)); + chan->waitsem = usb_osal_sem_create(0); HAL_HCD_HC_Init(g_usbhost.handle, ep0->outndx, 0x00, hport->dev_addr, speed, USB_ENDPOINT_TYPE_CONTROL, ep_cfg->ep_mps); HAL_HCD_HC_Init(g_usbhost.handle, ep0->inndx, 0x80, hport->dev_addr, speed, USB_ENDPOINT_TYPE_CONTROL, ep_cfg->ep_mps); @@ -441,7 +428,9 @@ int usbh_ep_alloc(usbh_epinfo_t *ep, const struct usbh_endpoint_cfg *ep_cfg) chidx = usb_synopsys_chan_alloc(&g_usbhost); chan = &priv->chan[chidx]; + memset(chan, 0, sizeof(struct usb_synopsys_chan)); chan->interval = ep_cfg->ep_interval; + chan->waitsem = usb_osal_sem_create(0); HAL_HCD_HC_Init(g_usbhost.handle, chidx, ep_cfg->ep_addr, hport->dev_addr, speed, ep_cfg->ep_type, ep_cfg->ep_mps); @@ -450,27 +439,22 @@ int usbh_ep_alloc(usbh_epinfo_t *ep, const struct usbh_endpoint_cfg *ep_cfg) *ep = (usbh_epinfo_t)chidx; } - usb_osal_mutex_give(g_usbhost.exclsem); return 0; } int usbh_ep_free(usbh_epinfo_t ep) { - int ret; - - ret = usb_osal_mutex_take(g_usbhost.exclsem); - if (ret < 0) { - return ret; - } if ((uintptr_t)ep < CONFIG_USBHOST_CHANNELS) { usb_synopsys_chan_free(&g_usbhost, (int)ep); + usb_osal_sem_delete(g_usbhost.chan[ep].waitsem); } else { struct usb_synopsys_ctrlinfo *ep0 = (struct usb_synopsys_ctrlinfo *)ep; usb_synopsys_chan_free(&g_usbhost, ep0->inndx); usb_synopsys_chan_free(&g_usbhost, ep0->outndx); + usb_osal_sem_delete(g_usbhost.chan[ep0->inndx].waitsem); + usb_osal_sem_delete(g_usbhost.chan[ep0->outndx].waitsem); } - usb_osal_mutex_give(g_usbhost.exclsem); return 0; } @@ -770,6 +754,10 @@ int usb_ep_cancel(usbh_epinfo_t ep) uint32_t flags; struct usb_synopsys_chan *chan; struct usb_synopsys_priv *priv = &g_usbhost; +#ifdef CONFIG_USBHOST_ASYNCH + usbh_asynch_callback_t callback; + void *arg; +#endif uint8_t chidx = (uint8_t)ep; @@ -778,6 +766,16 @@ int usb_ep_cancel(usbh_epinfo_t ep) flags = usb_osal_enter_critical_section(); chan->result = -ESHUTDOWN; +#ifdef CONFIG_USBHOST_ASYNCH + /* Extract the callback information */ + callback = chan->callback; + arg = chan->arg; + chan->callback = NULL; + chan->arg = NULL; + chan->xfrd = 0; +#endif + usb_osal_leave_critical_section(flags); + /* Is there a thread waiting for this transfer to complete? */ if (chan->waiter) { @@ -786,29 +784,12 @@ int usb_ep_cancel(usbh_epinfo_t ep) usb_osal_sem_give(chan->waitsem); } #ifdef CONFIG_USBHOST_ASYNCH - /* No.. is an asynchronous callback expected when the transfer - * completes? - */ - - else if (chan->callback) { - usbh_asynch_callback_t callback; - void *arg; - - /* Extract the callback information */ - - callback = chan->callback; - arg = chan->arg; - - chan->callback = NULL; - chan->arg = NULL; - chan->xfrd = 0; - + /* No.. is an asynchronous callback expected when the transfer completes? */ + else if (callback) { /* Then perform the callback */ - callback(arg, -ESHUTDOWN); } #endif - usb_osal_leave_critical_section(flags); return 0; }