From bd7b54a3c4f12793f855a7e2885b318edba7b06e Mon Sep 17 00:00:00 2001 From: Aidan MacDonald Date: Thu, 22 Dec 2022 21:52:31 +0000 Subject: usb: Attempt to handle overlapped control requests in legacy drivers It seems connecting an iPod Video to a Mac triggers the null request check, resulting in a panic. Ignoring the error with a bare return "fixes" it and allows the iPod to connect. This isn't ideal though, because it could silently introduce bugs on other targets. The likely cause of this is the host sending control requests too fast, or a driver problem (the Video uses the ARC driver, which is still on the legacy interface), with multiple requests getting queued at once. Since the USB core expects to deal with only one request at a time, the second response trips the check. Try to handle this situation a bit more gracefully by detecting overlapped requests and returning a STALL to the host when it occurs. At this point the USB stack is able to safely handle a new request. Link: https://forums.rockbox.org/index.php/topic,54414.0.html Change-Id: I9a2b7e35620ff540ebdb39f81671377062a4917d --- firmware/usbstack/usb_core.c | 43 ++++++++++++++++++++++++++++++++++++++----- 1 file changed, 38 insertions(+), 5 deletions(-) diff --git a/firmware/usbstack/usb_core.c b/firmware/usbstack/usb_core.c index 63df173033..5e50d21383 100644 --- a/firmware/usbstack/usb_core.c +++ b/firmware/usbstack/usb_core.c @@ -264,7 +264,9 @@ static struct usb_class_driver drivers[USB_NUM_DRIVERS] = }; #ifdef USB_LEGACY_CONTROL_API +static struct usb_ctrlrequest buffered_request; static struct usb_ctrlrequest* volatile active_request = NULL; +static volatile unsigned int num_active_requests = 0; static void* volatile control_write_data = NULL; static volatile bool control_write_data_done = false; #endif @@ -1019,21 +1021,52 @@ void usb_core_control_complete(int status) /* Only needed if the driver does not support the new API yet */ void usb_core_legacy_control_request(struct usb_ctrlrequest* req) { - active_request = req; - control_write_data = NULL; - control_write_data_done = false; + /* Only submit non-overlapping requests */ + if (num_active_requests++ == 0) + { + buffered_request = *req; + active_request = &buffered_request; + control_write_data = NULL; + control_write_data_done = false; - usb_core_control_request(req, NULL); + usb_core_control_request(req, NULL); + } } void usb_drv_control_response(enum usb_control_response resp, void* data, int length) { struct usb_ctrlrequest* req = active_request; + unsigned int num_active = num_active_requests--; - if(!req) + /* + * There must have been a prior request submission, at least. + */ + if (num_active == 0) panicf("null ctrl req"); + /* + * This can happen because an active request was already pending when + * the driver submitted a new one in usb_core_legacy_control_request(). + * This could mean two things: (a) a driver bug; or (b) the host sent + * another request because we were too slow in handling an earlier one. + * + * The USB spec requires we respond to the latest request and drop any + * earlier ones, but that's not easy to do with the current design of + * the USB stack. Thus, the host will be expecting a response for the + * latest request, but this response is for the _earliest_ request. + * + * Play it safe and return a STALL. At this point we've recovered from + * the error on our end and will be ready to handle the next request. + */ + if (num_active > 1) + { + active_request = NULL; + num_active_requests = 0; + usb_drv_stall(EP_CONTROL, true, true); + return; + } + if(req->wLength == 0) { active_request = NULL; -- cgit v1.2.3