drivers: usb_dc_mcux: Move mcux callback handler out of ISR context

MCUX usb ISR was making usb callbacks directly, which caused assertion failures when a callback attempted to lock a mutex. Move USB callback handler to separate thread, and make ISR notify thread via message queue. Fixes zephyrproject-rtos#40638 Signed-off-by: Daniel DeGrasse <daniel.degrasse@nxp.com>
nxp-zephyr · Jan 20, 2022 · f727594 · f727594
1 parent d175c18
commit f727594
Show file tree

Hide file tree

Showing 3 changed files with 70 additions and 29 deletions.
diff --git a/drivers/usb/device/Kconfig b/drivers/usb/device/Kconfig
@@ -142,6 +142,14 @@ config USB_DC_NXP_LPCIP3511
 
 endchoice
 
+config USB_DC_MSG_QUEUE_LEN
+	int
+	default 10
+	help
+	  Maximum number of messages USB device controller interrupt can queue
+	  for callback thread
+
+
 endif # USB_MCUX
 
 config USB_NATIVE_POSIX

diff --git a/drivers/usb/device/usb_dc_mcux.c b/drivers/usb/device/usb_dc_mcux.c
@@ -29,6 +29,7 @@
 LOG_MODULE_REGISTER(usb_dc_mcux);
 
 static void usb_isr_handler(void);
+static void usb_mcux_thread_main(void *arg1, void *arg2, void *arg3);
 
 /* the setup transfer state */
 #define SETUP_DATA_STAGE_DONE	(0)
@@ -76,6 +77,10 @@ K_HEAP_DEFINE(ep_buf_pool, 1024 * EP_BUF_NUMOF_BLOCKS);
 static struct usb_ep_ctrl_data s_ep_ctrl[NUM_OF_EP_MAX];
 static struct usb_device_struct dev_data;
 
+/* Message queue for the usb thread */
+K_MSGQ_DEFINE(usb_dc_msgq, sizeof(usb_device_callback_message_struct_t),
+	CONFIG_USB_DC_MSG_QUEUE_LEN, 4);
+
 #if defined(CONFIG_USB_DC_NXP_EHCI)
 /* EHCI device driver interface */
 static const usb_device_controller_interface_struct_t mcux_usb_iface = {
@@ -129,6 +134,12 @@ int usb_dc_attach(void)
 		return -EIO;
 	}
 
+	/* Create the usb callback handler thread */
+	k_thread_create(&dev_data.thread, dev_data.thread_stack,
+				USBD_MCUX_THREAD_STACK_SIZE,
+				usb_mcux_thread_main, NULL, NULL, NULL,
+				K_PRIO_COOP(2), 0, K_NO_WAIT);
+
 	/* Connect and enable USB interrupt */
 	IRQ_CONNECT(DT_INST_IRQN(0), DT_INST_IRQ(0, priority),
 		    usb_isr_handler, 0, 0);
@@ -768,40 +779,58 @@ static void handle_transfer_msg(usb_device_callback_message_struct_t *cb_msg)
 	}
 }
 
-/* Notify the up layer the KHCI status changed. */
-usb_status_t USB_DeviceNotificationTrigger(void *handle, void *msg)
+/**
+ * Similar to the kinetis driver, this thread is used to not run the USB device
+ * stack/endpoint callbacks in the ISR context. This is because callbacks from
+ * the USB stack may use mutexes, or other kernel functions not supported from
+ * an interrupt context.
+ */
+static void usb_mcux_thread_main(void *arg1, void *arg2, void *arg3)
 {
+	ARG_UNUSED(arg1);
+	ARG_UNUSED(arg2);
+	ARG_UNUSED(arg3);
+
 	uint8_t ep_abs_idx;
-	usb_device_callback_message_struct_t *cb_msg =
-		(usb_device_callback_message_struct_t *)msg;
-
-	switch (cb_msg->code) {
-	case kUSB_DeviceNotifyBusReset:
-		handle_bus_reset();
-		dev_data.status_callback(USB_DC_RESET, NULL);
-		break;
-	case kUSB_DeviceNotifyError:
-		dev_data.status_callback(USB_DC_ERROR, NULL);
-		break;
-	case kUSB_DeviceNotifySuspend:
-		dev_data.status_callback(USB_DC_SUSPEND, NULL);
-		break;
-	case kUSB_DeviceNotifyResume:
-		dev_data.status_callback(USB_DC_RESUME, NULL);
-		break;
-	default:
-		ep_abs_idx = EP_ABS_IDX(cb_msg->code);
-
-		if (ep_abs_idx >= NUM_OF_EP_MAX) {
-			LOG_ERR("Wrong endpoint index/address");
-			return kStatus_USB_Error;
-		}
+	usb_device_callback_message_struct_t msg;
+
+	while (1) {
+		k_msgq_get(&usb_dc_msgq, &msg, K_FOREVER);
+		switch (msg.code) {
+		case kUSB_DeviceNotifyBusReset:
+			handle_bus_reset();
+			dev_data.status_callback(USB_DC_RESET, NULL);
+			break;
+		case kUSB_DeviceNotifyError:
+			dev_data.status_callback(USB_DC_ERROR, NULL);
+			break;
+		case kUSB_DeviceNotifySuspend:
+			dev_data.status_callback(USB_DC_SUSPEND, NULL);
+			break;
+		case kUSB_DeviceNotifyResume:
+			dev_data.status_callback(USB_DC_RESUME, NULL);
+			break;
+		default:
+			ep_abs_idx = EP_ABS_IDX(msg.code);
+
+			if (ep_abs_idx >= NUM_OF_EP_MAX) {
+				LOG_ERR("Wrong endpoint index/address");
+				return;
+			}
 
-		memcpy(&dev_data.eps[ep_abs_idx].transfer_message, cb_msg,
-		       sizeof(usb_device_callback_message_struct_t));
-		handle_transfer_msg(&dev_data.eps[ep_abs_idx].transfer_message);
+			memcpy(&dev_data.eps[ep_abs_idx].transfer_message, &msg,
+			       sizeof(usb_device_callback_message_struct_t));
+			handle_transfer_msg(&dev_data.eps[ep_abs_idx].transfer_message);
+		}
 	}
+}
 
+/* Notify the up layer the KHCI status changed. */
+usb_status_t USB_DeviceNotificationTrigger(void *handle, void *msg)
+{
+	/* Submit to message queue */
+	k_msgq_put(&usb_dc_msgq,
+		(usb_device_callback_message_struct_t *)msg, K_NO_WAIT);
 	return kStatus_USB_Success;
 }
 

diff --git a/modules/hal_nxp/usb/usb_dc_mcux.h b/modules/hal_nxp/usb/usb_dc_mcux.h
@@ -107,6 +107,8 @@ struct usb_ep_ctrl_data {
 	uint8_t ep_occupied : 1;
 };
 
+#define USBD_MCUX_THREAD_STACK_SIZE		1024
+
 struct usb_device_struct {
 #if ((defined(USB_DEVICE_CONFIG_REMOTE_WAKEUP)) && \
 		(USB_DEVICE_CONFIG_REMOTE_WAKEUP > 0U)) || \
@@ -135,6 +137,8 @@ struct usb_device_struct {
 	/* Is doing device reset or not */
 	uint8_t isResetting;
 	uint8_t setupDataStage;
+	K_KERNEL_STACK_MEMBER(thread_stack, USBD_MCUX_THREAD_STACK_SIZE);
+	struct k_thread thread;
 };
 
 #endif /* __USB_DC_MCUX_H__ */