usb: device_next: Cherry-pick USB fixes

drivers/usb/udc/udc_dwc2.c

@@ -54,9 +54,12 @@
  */
 #define UDC_DWC2_GRXFSIZ_HS_DEFAULT	(13 + 1 + 774)
 
-/* TX FIFO0 depth in 32-bit words (used by control IN endpoint) */
-#define UDC_DWC2_FIFO0_DEPTH		16U
+/* TX FIFO0 depth in 32-bit words (used by control IN endpoint)
+ * Try 2 * bMaxPacketSize0 to allow simultaneous operation with a fallback to
+ * whatever is available when 2 * bMaxPacketSize0 is not possible.
+ */
+#define UDC_DWC2_FIFO0_DEPTH		(2 * 16U)
 
 /* Get Data FIFO access register */
 #define UDC_DWC2_EP_FIFO(base, idx)	((mem_addr_t)base + 0x1000 * (idx + 1))
 
@@ -99,9 +102,9 @@
 	struct k_thread thread_data;
 	/* Main events the driver thread waits for */
 	struct k_event drv_evt;
-	/* Transfer triggers (OUT on bits 0-15, IN on bits 16-31) */
+	/* Transfer triggers (IN on bits 0-15, OUT on bits 16-31) */
 	struct k_event xfer_new;
-	/* Finished transactions (OUT on bits 0-15, IN on bits 16-31) */
+	/* Finished transactions (IN on bits 0-15, OUT on bits 16-31) */
 	struct k_event xfer_finished;
 	struct dwc2_reg_backup backup;
 	uint32_t ghwcfg1;
@@ -1184,8 +1187,8 @@
 				  dwc2_get_txfaddr(dev, ep_idx - 2);
 		} else {
 			txfaddr = priv->rxfifo_depth +
-				MAX(UDC_DWC2_FIFO0_DEPTH, priv->max_txfifo_depth[0]);
+				MIN(UDC_DWC2_FIFO0_DEPTH, priv->max_txfifo_depth[0]);
 		}
 
 		/* Make sure to not set TxFIFO greater than hardware allows */
 		txfdep = reqdep;
@@ -1554,9 +1557,9 @@
 		uint32_t ep_bit;
 
 		if (USB_EP_DIR_IS_IN(cfg->addr)) {
-			ep_bit = BIT(16 + USB_EP_GET_IDX(cfg->addr));
-		} else {
 			ep_bit = BIT(USB_EP_GET_IDX(cfg->addr));
+		} else {
+			ep_bit = BIT(16 + USB_EP_GET_IDX(cfg->addr));
 		}
 
 		k_event_post(&priv->xfer_new, ep_bit);
@@ -1579,9 +1582,9 @@
 		uint32_t ep_bit;
 
 		if (USB_EP_DIR_IS_IN(cfg->addr)) {
-			ep_bit = BIT(16 + USB_EP_GET_IDX(cfg->addr));
-		} else {
 			ep_bit = BIT(USB_EP_GET_IDX(cfg->addr));
+		} else {
+			ep_bit = BIT(16 + USB_EP_GET_IDX(cfg->addr));
 		}
 
 		k_event_post(&priv->xfer_new, ep_bit);
@@ -1939,7 +1942,7 @@
 		sys_write32(usb_dwc2_set_grxfsiz(priv->rxfifo_depth), grxfsiz_reg);
 
 		/* Set TxFIFO 0 depth */
-		val = MAX(UDC_DWC2_FIFO0_DEPTH, priv->max_txfifo_depth[0]);
+		val = MIN(UDC_DWC2_FIFO0_DEPTH, priv->max_txfifo_depth[0]);
 		gnptxfsiz = usb_dwc2_set_gnptxfsiz_nptxfdep(val) |
 			    usb_dwc2_set_gnptxfsiz_nptxfstaddr(priv->rxfifo_depth);
 
@@ -2346,7 +2349,7 @@
 		return;
 	}
 
-	k_event_post(&priv->xfer_finished, BIT(16 + ep_idx));
+	k_event_post(&priv->xfer_finished, BIT(ep_idx));
 	k_event_post(&priv->drv_evt, BIT(DWC2_DRV_EVT_EP_FINISHED));
 }
 
@@ -2452,7 +2455,7 @@
 	    net_buf_tailroom(buf)) {
 		dwc2_prep_rx(dev, buf, ep_cfg);
 	} else {
-		k_event_post(&priv->xfer_finished, BIT(ep_idx));
+		k_event_post(&priv->xfer_finished, BIT(16 + ep_idx));
 		k_event_post(&priv->drv_evt, BIT(DWC2_DRV_EVT_EP_FINISHED));
 	}
 }
@@ -2803,9 +2806,9 @@
 	*bitmap &= ~BIT(bit);
 
 	if (bit >= 16) {
-		return USB_EP_DIR_IN | (bit - 16);
+		return USB_EP_DIR_OUT | (bit - 16);
 	} else {
-		return USB_EP_DIR_OUT | bit;
+		return USB_EP_DIR_IN | bit;
 	}
 }
 

drivers/usb/udc/udc_nrf.c

@@ -70,7 +70,8 @@
 
 static struct udc_ep_config ep_cfg_out[CFG_EPOUT_CNT + CFG_EP_ISOOUT_CNT + 1];
 static struct udc_ep_config ep_cfg_in[CFG_EPIN_CNT + CFG_EP_ISOIN_CNT + 1];
-static bool udc_nrf_setup_rcvd;
+static bool udc_nrf_setup_rcvd, udc_nrf_setup_set_addr, udc_nrf_fake_setup;
+static uint8_t udc_nrf_address;
 const static struct device *udc_nrf_dev;
 
 struct udc_nrf_config {
@@ -143,7 +144,9 @@
 	/* Update to next stage of control transfer */
 	udc_ctrl_update_stage(dev, buf);
 
-	nrf_usbd_common_setup_clear();
+	if (!udc_nrf_setup_set_addr) {
+		nrf_usbd_common_setup_clear();
+	}
 }
 
 static void udc_event_fake_status_in(const struct device *dev)
@@ -317,6 +320,7 @@
 
 static int udc_event_xfer_setup(const struct device *dev)
 {
+	nrf_usbd_common_setup_t *setup;
 	struct net_buf *buf;
 	int err;
 
@@ -328,7 +332,77 @@
 	}
 
 	udc_ep_buf_set_setup(buf);
-	nrf_usbd_common_setup_get((nrf_usbd_common_setup_t *)buf->data);
+	setup = (nrf_usbd_common_setup_t *)buf->data;
+	nrf_usbd_common_setup_get(setup);
+
+	/* USBD peripheral automatically handles Set Address in slightly
+	 * different manner than the USB stack.
+	 *
+	 * USBD peripheral doesn't care about wLength, but the peripheral
+	 * switches to new address only after status stage. The device won't
+	 * automatically accept Data Stage packets.
+	 *
+	 * However, in the case the host:
+	 *   * sends SETUP Set Address with non-zero wLength
+	 *   * does not send corresponding OUT DATA packets (to match wLength)
+	 *     or sends the packets but disregards NAK
+	 *     or sends the packets that device ACKs
+	 *   * sends IN token (either incorrectly proceeds to status stage, or
+	 *     manages to send IN before SW sets STALL)
+	 * then the USBD peripheral will accept the address and USB stack won't.
+	 * This will lead to state mismatch between the stack and peripheral.
+	 *
+	 * In cases where the USB stack would like to STALL the request there is
+	 * a race condition between host issuing Set Address status stage (IN
+	 * token) and SW setting STALL bit. If host wins the race, the device
+	 * ACKs status stage and use new address. If device wins the race, the
+	 * device STALLs status stage and address remains unchanged.
+	 */
+	udc_nrf_setup_set_addr =
+		setup->bmRequestType == 0 &&
+		setup->bRequest == USB_SREQ_SET_ADDRESS;
+	if (udc_nrf_setup_set_addr) {
+		if (setup->wLength) {
+			/* Currently USB stack only STALLs OUT Data Stage when
+			 * buffer allocation fails. To prevent the device from
+			 * ACKing the Data Stage, simply ignore the request
+			 * completely.
+			 *
+			 * If host incorrectly proceeds to status stage there
+			 * will be address mismatch (unless the new address is
+			 * equal to current device address). If host does not
+			 * issue IN token then the mismatch will be avoided.
+			 */
+			net_buf_unref(buf);
+			return 0;
+		}
+
+		/* nRF52/nRF53 USBD doesn't care about wValue bits 8..15 and
+		 * wIndex value but USB device stack does.
+		 *
+		 * Just clear the bits so stack will handle the request in the
+		 * same way as USBD peripheral does, avoiding the mismatch.
+		 */
+		setup->wValue &= 0x7F;
+		setup->wIndex = 0;
+	}
+
+	if (!udc_nrf_setup_set_addr && udc_nrf_address != NRF_USBD->USBADDR) {
+		/* Address mismatch detected. Fake Set Address handling to
+		 * correct the situation, then repeat handling.
+		 */
+		udc_nrf_fake_setup = true;
+		udc_nrf_setup_set_addr = true;
+
+		setup->bmRequestType = 0;
+		setup->bRequest = USB_SREQ_SET_ADDRESS;
+		setup->wValue = NRF_USBD->USBADDR;
+		setup->wIndex = 0;
+		setup->wLength = 0;
+	} else {
+		udc_nrf_fake_setup = false;
+	}
+
 	net_buf_add(buf, sizeof(nrf_usbd_common_setup_t));
 	udc_nrf_setup_rcvd = true;
 
@@ -487,17 +561,21 @@
 	}
 }
 
-static void udc_nrf_fake_status_in(const struct device *dev)
+static bool udc_nrf_fake_status_in(const struct device *dev)
 {
 	struct udc_nrf_evt evt = {
 		.type = UDC_NRF_EVT_STATUS_IN,
 		.ep = USB_CONTROL_EP_IN,
 	};
 
-	if (nrf_usbd_common_last_setup_dir_get() == USB_CONTROL_EP_OUT) {
+	if (nrf_usbd_common_last_setup_dir_get() == USB_CONTROL_EP_OUT ||
+	    udc_nrf_fake_setup) {
 		/* Let controller perform status IN stage */
 		k_msgq_put(&drv_msgq, &evt, K_NO_WAIT);
+		return true;
 	}
+
+	return false;
 }
 
 static int udc_nrf_ep_enqueue(const struct device *dev,
@@ -512,8 +590,9 @@
 	udc_buf_put(cfg, buf);
 
 	if (cfg->addr == USB_CONTROL_EP_IN && buf->len == 0) {
-		udc_nrf_fake_status_in(dev);
-		return 0;
+		if (udc_nrf_fake_status_in(dev)) {
+			return 0;
+		}
 	}
 
 	k_msgq_put(&drv_msgq, &evt, K_NO_WAIT);
@@ -608,12 +687,38 @@
 
 static int udc_nrf_set_address(const struct device *dev, const uint8_t addr)
 {
-	/**
-	 * Nothing to do here. The USBD HW already takes care of initiating
-	 * STATUS stage. Just double check the address for sanity.
+	/*
+	 * If the status stage already finished (which depends entirely on when
+	 * the host sends IN token) then NRF_USBD->USBADDR will have the same
+	 * address, otherwise it won't (unless new address is unchanged).
+	 *
+	 * Store the address so the driver can detect address mismatches
+	 * between USB stack and USBD peripheral. The mismatches can occur if:
+	 *   * SW has high enough latency in SETUP handling, or
+	 *   * Host did not issue Status Stage after Set Address request
+	 *
+	 * The SETUP handling latency is a problem because the Set Address is
+	 * automatically handled by device. Because whole Set Address handling
+	 * can finish in less than 21 us, the latency required (with perfect
+	 * timing) to hit the issue is relatively short (2 ms Set Address
+	 * recovery interval + negligible Set Address handling time). If host
+	 * sends new SETUP before SW had a chance to read the Set Address one,
+	 * the Set Address one will be overwritten without a trace.
 	 */
-	if (addr != (uint8_t)NRF_USBD->USBADDR) {
-		LOG_WRN("USB Address incorrect 0x%02x", addr);
+	udc_nrf_address = addr;
+
+	if (udc_nrf_fake_setup) {
+		struct udc_nrf_evt evt = {
+			.type = UDC_NRF_EVT_HAL,
+			.hal_evt = {
+				.type = NRF_USBD_COMMON_EVT_SETUP,
+			},
+		};
+
+		/* Finished handling lost Set Address, now handle the pending
+		 * SETUP transfer.
+		 */
+		k_msgq_put(&drv_msgq, &evt, K_NO_WAIT);
 	}
 
 	return 0;

subsys/usb/device_next/usbd_ch9.c

@@ -86,8 +86,8 @@ static int sreq_set_address(struct usbd_context *const uds_ctx)
 	struct usb_setup_packet *setup = usbd_get_setup_pkt(uds_ctx);
 	struct udc_device_caps caps = udc_caps(uds_ctx->dev);
 
-	/* Not specified if wLength is non-zero, treat as error */
-	if (setup->wValue > 127 || setup->wLength) {
+	/* Not specified if wIndex or wLength is non-zero, treat as error */
+	if (setup->wValue > 127 || setup->wIndex || setup->wLength) {
 		errno = -ENOTSUP;
 		return 0;
 	}