USB: Fix reconnect issues #16193

finikorg · 2019-05-16T08:07:30Z

I still need to check suspend / resume properly working.

Fixes #15917

subsys/usb/usb_device.c

loicpoulain

LGTM

barsok

Found an issue which rakon then reported -- fixed. Verified on an own piece of HW.

masz-nordic

This allows USB3CV tests to be executed again, but following cases from Chapter 9 Tests still fail:

TD 9.1, 9.2, 9.3, 9.4, 9.5, 9.7 in Configured state:
- Set Configuration failed for Configuration Value : 1
- (1.1.2) Devices must support being set to Addressed/Configured state.
TD 9.9, 9.13:
- Set Configuration failed for Configuration Value : 1
- (1.1.2) Devices must support being set to Addressed/Configured state.
TD 9.16:
- Enumeration failed at iteration 1 (General failure)

finikorg · 2019-05-20T07:09:59Z

This allows USB3CV tests to be executed again, but following cases from Chapter 9 Tests still fail:

* TD 9.1, 9.2, 9.3, 9.4, 9.5, 9.7 in Configured state:
  
  * `Set Configuration failed for Configuration Value : 1`
  * `(1.1.2) Devices must support being set to Addressed/Configured state.`

* TD 9.9, 9.13:
  
  * `Set Configuration failed for Configuration Value : 1`
  * `(1.1.2) Devices must support being set to Addressed/Configured state.`

* TD 9.16:
  
  * `Enumeration failed at iteration 1 (General failure)`

Are those failures resulting from the PR? Can they be reproduced on master?

masz-nordic · 2019-05-20T07:45:39Z

On master the USB3CV tests do not work at all:

SetConfiguration() failed.
  No device:  deleting node!
  Device configuration failed!
  SetConfiguration() failed.
  No device:  deleting node!
  Device configuration failed!
A device was not found.  
Check that device is plugged into correct port.
Couldn't initialize common subsystems.
Function CVCommon_InitializeTestSuite did not succeed.  Return value = 0x80000000

finikorg · 2019-05-20T07:47:52Z

On master the USB3CV tests do not work at all:

SetConfiguration() failed.
  No device:  deleting node!
  Device configuration failed!
  SetConfiguration() failed.
  No device:  deleting node!
  Device configuration failed!
A device was not found.  
Check that device is plugged into correct port.
Couldn't initialize common subsystems.
Function CVCommon_InitializeTestSuite did not succeed.  Return value = 0x80000000

OK, This is probably because of extra checks we have added. I think we should address this in a separate issue.

finikorg · 2019-05-20T08:28:12Z

@masz-nordic Does this fix the issues?

diff --git a/subsys/usb/usb_device.c b/subsys/usb/usb_device.c
index 9e70966d10..765766cd5d 100644
--- a/subsys/usb/usb_device.c
+++ b/subsys/usb/usb_device.c
@@ -500,7 +500,6 @@ static bool set_endpoint(const struct usb_ep_descriptor *ep_desc)
 
        if (usb_dc_ep_enable(ep_cfg.ep_addr) < 0) {
                LOG_ERR("Failed to enable endpoint %x", ep_cfg.ep_addr);
-               return false;
        }
 
        usb_dev.configured = true;

masz-nordic · 2019-05-20T08:38:21Z

@finikorg It fixes 9.1, 9.2, 9.3, 9.4, 9.5, 9.7 in Configured state and 9.9. But 9.13 and 9.16 still fail.
Additionally, there are new failures:

9.1, 9.2, 9.3, 9.4, 9.5, 9.7 in Default state
9.1, 9.2, 9.3, 9.4, 9.5, 9.7 in Addressed state

All of them fail with:

Set Configuration failed for Configuration Value : 0
(1.1.2) Devices must support being set to Addressed/Configured state.

finikorg · 2019-05-20T08:43:12Z

Then we need to "fix" another bug :)

        if (usb_dc_ep_configure(&ep_cfg) < 0) {
                LOG_ERR("Failed to configure endpoint %x", ep_cfg.ep_addr);
-               return false;
        }

masz-nordic · 2019-05-20T08:53:59Z

Still same failures as in previous case. Logs:

[00:01:11.285,858] <dbg> usb_hid.hid_custom_handle_req: Standard request: bRequest 0x9 bmRequestType 0x0 len 0
[00:01:11.285,858] <dbg> usb_device.usb_handle_std_device_req: REQ_SET_CONFIGURATION, conf 0x0
[00:01:11.285,858] <dbg> usb_device.usb_set_configuration: Device not configured - invalid configuration
[00:01:11.285,888] <dbg> usb_hid.hid_status_cb: cfg 0x200033a8 status 3
[00:01:11.285,888] <dbg> usb_hid.hid_do_status_cb: USB device configured
[00:01:11.285,888] <dbg> usb_device.usb_handle_std_device_req: USB Set Configuration failed
[00:01:11.285,888] <dbg> usb_device.usb_handle_request: Handler Error 0
[00:01:11.285,919] <dbg> usb_device.usb_print_setup: Setup: 0 9 0 0 0
[00:01:11.285,919] <dbg> usb_device.usb_handle_control_transfer: usb_handle_request failed

finikorg · 2019-05-20T09:30:55Z

@masz-nordic what sample are you using? I had tests with frdm_k64f and HID it was passing all tests.

masz-nordic · 2019-05-20T09:44:04Z

@finikorg hid-mouse with nrf52840_pca10056.

jfischer-no · 2019-05-20T09:54:18Z

@masz-nordic Can you please apply and test again? (https://gist.github.com/acsplayon/b594d6b2207f8923edb071fdee499b36)

diff --git a/drivers/usb/device/usb_dc_nrfx.c b/drivers/usb/device/usb_dc_nrfx.c
index e3ba244b9c..86c7e39351 100644
--- a/drivers/usb/device/usb_dc_nrfx.c
+++ b/drivers/usb/device/usb_dc_nrfx.c
@@ -637,6 +637,8 @@ static void ep_ctx_reset(struct nrf_usbd_ep_ctx *ep_ctx)
        ep_ctx->buf.curr = ep_ctx->buf.data;
        ep_ctx->buf.len  = 0U;
 
+       ep_ctx->cfg.en = false;
+
        ep_ctx->read_complete = true;
        ep_ctx->read_pending = false;
        ep_ctx->write_in_progress = false;

masz-nordic · 2019-05-20T10:18:25Z

@jfischer-phytec-iot This fixes 9.16. Other still fail, here is the trace from Default state tests:

If this is nRF specific issue and this PR fixes USB3CV for other platforms, let's merge it and I will investigate fails in a separate case.

finikorg · 2019-05-20T11:28:05Z

I have tested frdm_k64f, stm32f723e_disco, nucleo_f412zg they are all passing, I will check also reel_board.

jfischer-no

LGTM

finikorg · 2019-05-20T13:29:15Z

@masz-nordic Tests are passing for reel_board also if I disable USB_DEVICE_REMOTE_WAKEUP

Add usb_cancel_transfers() helper to cancel all ongoing transfers. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Only call callback when transfer is not cancelled. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Cancel all usb transfers when receiving USB_DC_DISCONNECTED. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Add extra logs catching problem with re-enabling endpoints. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Fix supended -> suspended. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Disable endpoints on following events: USB_DC_DISCONNECTED and USB_DC_SUSPEND. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Fixes USB3CV Tool tests. One case is enabling endpoints with Set Interface after Set Configuration. In this case we report warning and continue without returning error. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

Set false to enable flag on USB Reset. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

masz-nordic · 2019-05-21T08:59:32Z

@finikorg: Which test mode are you using? I have tested reel_board with Compliance Test on Chapter 9 Tests [USB 2 devices] and it still fails.

finikorg · 2019-05-21T09:58:39Z

@finikorg: Which test mode are you using? I have tested reel_board with Compliance Test on Chapter 9 Tests [USB 2 devices] and it still fails.

Yes Compliance "Chapter 9 Tests [USB 2 devices]", only Remote Wakeup TD 9.12 is failing.

I am on top of my above mentioned PR, my reel_board is the oldest version, probably you have newer one?

jfischer-no · 2019-05-21T13:24:41Z

@masz-nordic Anything against to merge it?

masz-nordic · 2019-05-21T13:30:05Z

@finikorg Mine is 1507.1. I disabled remote wakeup in Kconfig (apart from prj.conf) too and now it only fails 9.21 and 9.16.

@jfischer-phytec-iot No, if this fixes problems from the original issue it is fine. I will create a separate one for USB3CV testcases.

This reverts commit f206170 introduced as workaround for nRF USBD device controller in PR zephyrproject-rtos#16193. This commit may be reverted due to changes made in commit e326c58 ("usb: device: Do not cancel transfers on suspend"). Signed-off-by: Nickolas Lapp <nickolaslapp@gmail.com> Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

This reverts commit f206170 introduced as workaround for nRF USBD device controller in PR #16193. This commit may be reverted due to changes made in commit e326c58 ("usb: device: Do not cancel transfers on suspend"). Signed-off-by: Nickolas Lapp <nickolaslapp@gmail.com> Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

This reverts commit f206170 introduced as workaround for nRF USBD device controller in PR zephyrproject-rtos/zephyr#16193. This commit may be reverted due to changes made in commit e326c58 ("usb: device: Do not cancel transfers on suspend"). (cherry picked from commit 81e4934) Original-Signed-off-by: Nickolas Lapp <nickolaslapp@gmail.com> Original-Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no> GitOrigin-RevId: 81e4934 Change-Id: I1ee50ed7ec2f84e30c5c7df1c359277889b58b42 Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/zephyr/+/4432938 Tested-by: CopyBot Service Account <copybot.service@gmail.com> Commit-Queue: Keith Short <keithshort@chromium.org> Tested-by: Keith Short <keithshort@chromium.org> Reviewed-by: Keith Short <keithshort@chromium.org>

finikorg requested a review from jfischer-no as a code owner May 16, 2019 08:07

finikorg changed the title ~~USB: Fix reconnect issues~~ WIP: USB: Fix reconnect issues May 16, 2019

jfischer-no added area: USB Universal Serial Bus bug The issue is a bug, or the PR is fixing a bug labels May 16, 2019

jfischer-no requested a review from loicpoulain May 16, 2019 08:35

finikorg force-pushed the disconnect branch 2 times, most recently from 4c9413a to d53a683 Compare May 16, 2019 13:00

rakons suggested changes May 16, 2019

View reviewed changes

subsys/usb/usb_device.c Outdated Show resolved Hide resolved

finikorg force-pushed the disconnect branch from d53a683 to a6aff32 Compare May 16, 2019 13:22

loicpoulain reviewed May 16, 2019

View reviewed changes

masz-nordic mentioned this pull request May 17, 2019

stm32: Unable to send 64 byte packet over control endpoint #16223

Closed

carlescufi requested review from masz-nordic and barsok May 17, 2019 18:03

barsok approved these changes May 20, 2019

View reviewed changes

masz-nordic requested changes May 20, 2019

View reviewed changes

jfischer-no approved these changes May 20, 2019

View reviewed changes

jfischer-no changed the title ~~WIP: USB: Fix reconnect issues~~ USB: Fix reconnect issues May 20, 2019

jfischer-no approved these changes May 20, 2019

View reviewed changes

finikorg added 8 commits May 20, 2019 16:34

usb: device: Add transfer cancel helper

e2f0703

Add usb_cancel_transfers() helper to cancel all ongoing transfers. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

usb: device: Do not call callback when transfer is cancelled

ea62216

Only call callback when transfer is not cancelled. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

usb: device: Cancel all transfers on disconnect

657c329

Cancel all usb transfers when receiving USB_DC_DISCONNECTED. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

usb: device: Add debug logs catching re-enable failures

6a36b25

Add extra logs catching problem with re-enabling endpoints. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

usb: webusb: Trivial syntax fix

4ece9f7

Fix supended -> suspended. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

usb: device: Disable endpoints on disconnect and suspend

c5bb9a0

Disable endpoints on following events: USB_DC_DISCONNECTED and USB_DC_SUSPEND. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

drivers: usb: usb_dc_nrfx: Set cfg.en to false on Reset

b5aefbd

Set false to enable flag on USB Reset. Signed-off-by: Andrei Emeltchenko <andrei.emeltchenko@intel.com>

finikorg force-pushed the disconnect branch from 175f789 to b5aefbd Compare May 20, 2019 13:36

masz-nordic approved these changes May 21, 2019

View reviewed changes

nashif requested a review from rakons May 21, 2019 20:15

rakons approved these changes May 22, 2019

View reviewed changes

carlescufi merged commit 03ef375 into zephyrproject-rtos:master May 22, 2019

finikorg deleted the disconnect branch May 22, 2019 11:41

carlescufi mentioned this pull request Jun 3, 2019

Superfluous USB suspend after USB configured #16470

Closed

jfischer-no mentioned this pull request Jul 11, 2019

usb: do not disable endpoints at USB_DC_SUSPEND event #17492

Merged

stephanosio mentioned this pull request Jul 8, 2020

USB suspend-resume process is not properly handled #26725

Closed

NickolasLapp mentioned this pull request Jan 5, 2023

Revert "usb: device: Do not call callback when transfer is cancelled" #53540

Closed

jfischer-no mentioned this pull request Apr 13, 2023

usb: device: Revert "Do not call callback when transfer is cancelled" #56829

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

USB: Fix reconnect issues #16193

USB: Fix reconnect issues #16193

finikorg commented May 16, 2019 •

edited

Loading

loicpoulain left a comment

barsok left a comment

masz-nordic left a comment

finikorg commented May 20, 2019

masz-nordic commented May 20, 2019

finikorg commented May 20, 2019

finikorg commented May 20, 2019

masz-nordic commented May 20, 2019

finikorg commented May 20, 2019

masz-nordic commented May 20, 2019

finikorg commented May 20, 2019

masz-nordic commented May 20, 2019

jfischer-no commented May 20, 2019 •

edited

Loading

masz-nordic commented May 20, 2019

finikorg commented May 20, 2019

jfischer-no left a comment

finikorg commented May 20, 2019

masz-nordic commented May 21, 2019

finikorg commented May 21, 2019

jfischer-no commented May 21, 2019

masz-nordic commented May 21, 2019

USB: Fix reconnect issues #16193

USB: Fix reconnect issues #16193

Conversation

finikorg commented May 16, 2019 • edited Loading

loicpoulain left a comment

Choose a reason for hiding this comment

barsok left a comment

Choose a reason for hiding this comment

masz-nordic left a comment

Choose a reason for hiding this comment

finikorg commented May 20, 2019

masz-nordic commented May 20, 2019

finikorg commented May 20, 2019

finikorg commented May 20, 2019

masz-nordic commented May 20, 2019

finikorg commented May 20, 2019

masz-nordic commented May 20, 2019

finikorg commented May 20, 2019

masz-nordic commented May 20, 2019

jfischer-no commented May 20, 2019 • edited Loading

masz-nordic commented May 20, 2019

finikorg commented May 20, 2019

jfischer-no left a comment

Choose a reason for hiding this comment

finikorg commented May 20, 2019

masz-nordic commented May 21, 2019

finikorg commented May 21, 2019

jfischer-no commented May 21, 2019

masz-nordic commented May 21, 2019

finikorg commented May 16, 2019 •

edited

Loading

jfischer-no commented May 20, 2019 •

edited

Loading