From 26ab78a710b4cb7abcace769768c4781332b17ba Mon Sep 17 00:00:00 2001 From: Karl Palsson Date: Mon, 27 Aug 2018 14:20:27 +0000 Subject: usb: fix race between setup complete and endpoint nak See https://github.com/libopencm3/libopencm3/issues/873 Commentary describing this patch originally by zyp: ``` After looking further into it, I've concluded that my preliminary analysis looks correct. The problem is that setting CNAK before the SETUP complete event is received causes a race condition. The SETUP callback is called when the SETUP packet event is received, which means that setting CNAK from the callback is too early. Originally the problem was that CNAK was set by ep_read() which is called by the callback. #672 solved this by moving CNAK out of ep_read() and calling it after the SETUP complete event is received instead. The regression by #785 is caused by the introduction of flow control calls into the SETUP callback. They also set CNAK. To solve this properly, I propose changing the event handling code to only call the SETUP callback after the SETUP complete event is received. Unfortunately, this implies that the callback can't call ep_read() itself anymore, because the packet has to be read out of the FIFO before the SETUP complete event arrives. This implies a change of the API between the hardware drivers and _usbd_control_setup(). ``` L1 (st_usbfs) works and passes tests as before change F4 (dwc_otg_fs) works and now passes tests. (yay) LM4f still compiles, and has had the same style of implementation as st_usbfs, however has not been tested on any hardware. --- lib/stm32/common/st_usbfs_core.c | 1 + lib/usb/usb_control.c | 5 ----- lib/usb/usb_dwc_common.c | 9 ++++++++- lib/usb/usb_lm4f.c | 4 +++- 4 files changed, 12 insertions(+), 7 deletions(-) diff --git a/lib/stm32/common/st_usbfs_core.c b/lib/stm32/common/st_usbfs_core.c index edc2808e..a4ab414a 100644 --- a/lib/stm32/common/st_usbfs_core.c +++ b/lib/stm32/common/st_usbfs_core.c @@ -251,6 +251,7 @@ void st_usbfs_poll(usbd_device *dev) /* OUT or SETUP? */ if (*USB_EP_REG(ep) & USB_EP_SETUP) { type = USB_TRANSACTION_SETUP; + st_usbfs_ep_read_packet(dev, ep, &dev->control_state.req, 8); } else { type = USB_TRANSACTION_OUT; } diff --git a/lib/usb/usb_control.c b/lib/usb/usb_control.c index dce1530f..531d43a5 100644 --- a/lib/usb/usb_control.c +++ b/lib/usb/usb_control.c @@ -228,11 +228,6 @@ void _usbd_control_setup(usbd_device *usbd_dev, uint8_t ea) usbd_ep_nak_set(usbd_dev, 0, 1); - if (usbd_ep_read_packet(usbd_dev, 0, req, 8) != 8) { - stall_transaction(usbd_dev); - return; - } - if (req->wLength == 0) { usb_control_setup_read(usbd_dev, req); } else if (req->bmRequestType & 0x80) { diff --git a/lib/usb/usb_dwc_common.c b/lib/usb/usb_dwc_common.c index 11af2609..162bfd7c 100644 --- a/lib/usb/usb_dwc_common.c +++ b/lib/usb/usb_dwc_common.c @@ -358,6 +358,11 @@ void dwc_poll(usbd_device *usbd_dev) uint32_t rxstsp = REBASE(OTG_GRXSTSP); uint32_t pktsts = rxstsp & OTG_GRXSTSP_PKTSTS_MASK; uint8_t ep = rxstsp & OTG_GRXSTSP_EPNUM_MASK; + + if (pktsts == OTG_GRXSTSP_PKTSTS_SETUP_COMP) { + usbd_dev->user_callback_ctr[ep][USB_TRANSACTION_SETUP] (usbd_dev, ep); + } + if (pktsts == OTG_GRXSTSP_PKTSTS_OUT_COMP || pktsts == OTG_GRXSTSP_PKTSTS_SETUP_COMP) { REBASE(OTG_DOEPTSIZ(ep)) = usbd_dev->doeptsiz[ep]; @@ -390,7 +395,9 @@ void dwc_poll(usbd_device *usbd_dev) /* Save packet size for dwc_ep_read_packet(). */ usbd_dev->rxbcnt = (rxstsp & OTG_GRXSTSP_BCNT_MASK) >> 4; - if (usbd_dev->user_callback_ctr[ep][type]) { + if (type == USB_TRANSACTION_SETUP) { + dwc_ep_read_packet(usbd_dev, ep, &usbd_dev->control_state.req, 8); + } else if (usbd_dev->user_callback_ctr[ep][type]) { usbd_dev->user_callback_ctr[ep][type] (usbd_dev, ep); } diff --git a/lib/usb/usb_lm4f.c b/lib/usb/usb_lm4f.c index e70e6047..1d0d688a 100644 --- a/lib/usb/usb_lm4f.c +++ b/lib/usb/usb_lm4f.c @@ -503,7 +503,9 @@ static void lm4f_poll(usbd_device *usbd_dev) usbd_dev->control_state.state != LAST_DATA_OUT) ? USB_TRANSACTION_SETUP : USB_TRANSACTION_OUT; - + if (type == USB_TRANSACTION_SETUP) { + lm4f_ep_read_packet(usbd_dev, 0, &usbd_dev->control_state.req, 8); + } if (usbd_dev->user_callback_ctr[0][type]) { usbd_dev-> user_callback_ctr[0][type](usbd_dev, 0); -- cgit v1.2.3