From 322b4b72affdb165b15f10239183cb1fbbd68614 Mon Sep 17 00:00:00 2001 From: Russ Butler Date: Fri, 25 May 2018 12:00:04 -0500 Subject: [PATCH] Update USBDevice endpoint checks to fix asserts Only assert if disabled endpoints are used when USBDevice is configured. USBDevice can leave the configured state due to a reset at any time, which disables all endpoints. Because this can happen at any time, thread processing could be performing any endpoint operation. The endpoint operation should return failure and do nothing in this case, rather than asserting as this is not an application error. An assert should only be triggered when an invalid endpoint is used after the use of USBDevice acknoledges the switch to configured mode by complete_set_configuration. In specific this PR fixes the assert caused with the following sequence: -ISR: OUT event sent -ISR: USB reset event -ISR: USB configure request start -Thread: OUT event processed on thread and next read starts ***endpoint is used while disabled causing an invalid assert*** -Thread: reset event processed -Thread: configure event processed This patch fixes this problem by making the following changes: 1. Operations done on disabled endpoints only assert when in the configured state 2. Adding and removing endpoints is only allowed when the flag _endpoint_add_remove_allowed is set 3. The flag _endpoint_add_remove_allowed is set on the set configuration request and cleared if the request is aborted or fails --- usb/device/USBDevice/USBDevice.cpp | 85 ++++++++++++++++-------------- usb/device/USBDevice/USBDevice.h | 1 + 2 files changed, 45 insertions(+), 41 deletions(-) diff --git a/usb/device/USBDevice/USBDevice.cpp b/usb/device/USBDevice/USBDevice.cpp index 600520af4f..c4fc4e17f6 100644 --- a/usb/device/USBDevice/USBDevice.cpp +++ b/usb/device/USBDevice/USBDevice.cpp @@ -376,6 +376,7 @@ bool USBDevice::_request_set_configuration() _phy->unconfigure(); _change_state(Address); } else { + _endpoint_add_remove_allowed = true; _transfer.user_callback = SetConfiguration; callback_set_configuration(_device.configuration); } @@ -399,6 +400,14 @@ void USBDevice::_complete_set_configuration() assert_locked(); bool success = _transfer.args.status; + if ((_abort_control || !success) && !configured()) { + // The set configuration request was aborted or failed so + // reset any endpoints which may have been added. + memset(_endpoint_info, 0, sizeof(_endpoint_info)); + _device.configuration = 0; + _endpoint_add_remove_allowed = false; + } + _transfer.user_callback = None; if (_abort_control) { @@ -1054,6 +1063,11 @@ bool USBDevice::endpoint_add(usb_ep_t endpoint, uint32_t max_packet_size, usb_ep return false; } + if (!_endpoint_add_remove_allowed) { + unlock(); + return false; + } + endpoint_info_t *info = &_endpoint_info[EP_TO_INDEX(endpoint)]; MBED_ASSERT(!(info->flags & ENDPOINT_ENABLED)); MBED_ASSERT(max_packet_size <= 1024); @@ -1080,6 +1094,11 @@ void USBDevice::endpoint_remove(usb_ep_t endpoint) return; } + if (!_endpoint_add_remove_allowed) { + unlock(); + return; + } + endpoint_info_t *info = &_endpoint_info[EP_TO_INDEX(endpoint)]; MBED_ASSERT(info->flags & ENDPOINT_ENABLED); @@ -1122,7 +1141,12 @@ void USBDevice::endpoint_stall(usb_ep_t endpoint) } endpoint_info_t *info = &_endpoint_info[EP_TO_INDEX(endpoint)]; - MBED_ASSERT(info->flags & ENDPOINT_ENABLED); + if (!(info->flags & ENDPOINT_ENABLED)) { + // Invalid endpoint is being used + MBED_ASSERT(!configured()); + unlock(); + return; + } info->flags |= ENDPOINT_STALLED; _phy->endpoint_stall(endpoint); @@ -1141,7 +1165,12 @@ void USBDevice::endpoint_unstall(usb_ep_t endpoint) } endpoint_info_t *info = &_endpoint_info[EP_TO_INDEX(endpoint)]; - MBED_ASSERT(info->flags & ENDPOINT_ENABLED); + if (!(info->flags & ENDPOINT_ENABLED)) { + // Invalid endpoint is being used + MBED_ASSERT(!configured()); + unlock(); + return; + } info->flags &= ~ENDPOINT_STALLED; _phy->endpoint_unstall(endpoint); @@ -1232,6 +1261,7 @@ USBDevice::USBDevice(USBPhy *phy, uint16_t vendor_id, uint16_t product_id, uint1 _phy = phy; _initialized = false; _connected = false; + _endpoint_add_remove_allowed = false; _current_interface = 0; _current_alternate = 0; _locked = 0; @@ -1275,16 +1305,10 @@ void USBDevice::endpoint_abort(usb_ep_t endpoint) return; } - bool configuring = _transfer.user_callback == SetConfiguration; - if (!configured() && !configuring) { - unlock(); - return; - } - endpoint_info_t *info = &_endpoint_info[EP_TO_INDEX(endpoint)]; if (!(info->flags & ENDPOINT_ENABLED)) { - // Invalid endpoint is being used - MBED_ASSERT(0); + // Assert that only valid endpoints are used when in the configured state + MBED_ASSERT(!configured()); unlock(); return; } @@ -1307,16 +1331,10 @@ bool USBDevice::read_start(usb_ep_t endpoint, uint8_t *buffer, uint32_t max_size return false; } - bool configuring = _transfer.user_callback == SetConfiguration; - if (!configured() && !configuring) { - unlock(); - return false; - } - endpoint_info_t *info = &_endpoint_info[EP_TO_INDEX(endpoint)]; if (!(info->flags & ENDPOINT_ENABLED)) { - // Invalid endpoint is being used - MBED_ASSERT(0); + // Assert that only valid endpoints are used when in the configured state + MBED_ASSERT(!configured()); unlock(); return false; } @@ -1353,15 +1371,10 @@ uint32_t USBDevice::read_finish(usb_ep_t endpoint) return 0; } - if (!configured()) { - unlock(); - return 0; - } - endpoint_info_t *info = &_endpoint_info[EP_TO_INDEX(endpoint)]; if (!(info->flags & ENDPOINT_ENABLED)) { - // Invalid endpoint is being used - MBED_ASSERT(0); + // Assert that only valid endpoints are used when in the configured state + MBED_ASSERT(!configured()); unlock(); return 0; } @@ -1382,16 +1395,10 @@ bool USBDevice::write_start(usb_ep_t endpoint, uint8_t *buffer, uint32_t size) return false; } - bool configuring = _transfer.user_callback == SetConfiguration; - if (!configured() && !configuring) { - unlock(); - return false; - } - endpoint_info_t *info = &_endpoint_info[EP_TO_INDEX(endpoint)]; if (!(info->flags & ENDPOINT_ENABLED)) { - // Invalid endpoint is being used - MBED_ASSERT(0); + // Assert that only valid endpoints are used when in the configured state + MBED_ASSERT(!configured()); unlock(); return false; } @@ -1431,18 +1438,13 @@ uint32_t USBDevice::write_finish(usb_ep_t endpoint) if (!EP_INDEXABLE(endpoint)) { MBED_ASSERT(0); unlock(); - return false; - } - - if (!configured()) { - unlock(); - return false; + return 0; } endpoint_info_t *info = &_endpoint_info[EP_TO_INDEX(endpoint)]; if (!(info->flags & ENDPOINT_ENABLED)) { - // Invalid endpoint is being used - MBED_ASSERT(0); + // Assert that only valid endpoints are used when in the configured state + MBED_ASSERT(!configured()); unlock(); return 0; } @@ -1593,6 +1595,7 @@ void USBDevice::_change_state(DeviceState new_state) { if (leaving_configured_state) { memset(_endpoint_info, 0, sizeof(_endpoint_info)); _device.configuration = 0; + _endpoint_add_remove_allowed = false; } if (leaving_default_state) { diff --git a/usb/device/USBDevice/USBDevice.h b/usb/device/USBDevice/USBDevice.h index 40051ce9c0..8ef7008597 100644 --- a/usb/device/USBDevice/USBDevice.h +++ b/usb/device/USBDevice/USBDevice.h @@ -595,6 +595,7 @@ private: USBPhy *_phy; bool _initialized; bool _connected; + bool _endpoint_add_remove_allowed; control_transfer_t _transfer; usb_device_t _device; uint32_t _max_packet_size_ep0;