From patchwork Mon Sep 28 15:20:50 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alan Stern X-Patchwork-Id: 11803961 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 7E3FB618 for ; Mon, 28 Sep 2020 15:20:52 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 6E4B92100A for ; Mon, 28 Sep 2020 15:20:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726485AbgI1PUw (ORCPT ); Mon, 28 Sep 2020 11:20:52 -0400 Received: from netrider.rowland.org ([192.131.102.5]:60855 "HELO netrider.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726465AbgI1PUv (ORCPT ); Mon, 28 Sep 2020 11:20:51 -0400 Received: (qmail 136874 invoked by uid 1000); 28 Sep 2020 11:20:50 -0400 Date: Mon, 28 Sep 2020 11:20:50 -0400 From: Alan Stern To: Greg KH Cc: yasushi asano , andrew_gabbasov@mentor.com, "Rosca, Eugeniu \(ADITG/ESM1\)" , Baxter Jim , linux-usb@vger.kernel.org, "Nishiguchi, Naohiro \(ADITJ/SWG\)" , "Natsume, Wataru \(ADITJ/SWG\)" , =?utf-8?b?5rWF6YeO5oGt5Y+y?= Subject: [Patch 1/2]: USB: hub: Clean up use of port initialization schemes and retries Message-ID: <20200928152050.GA134701@rowland.harvard.edu> References: <20200920192114.GB1190206@rowland.harvard.edu> <20200921140342.3813-1-yazzep@gmail.com> <20200921144827.GC1213381@rowland.harvard.edu> <20200921150611.GD1213381@rowland.harvard.edu> <20200925184153.GA53451@rowland.harvard.edu> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org The SET_CONFIG_TRIES macro in hub.c is badly named; it controls the number of port-initialization retry attempts rather than the number of Set-Configuration attempts. Furthermore, the USE_NEW_SCHEME macro and use_new_scheme() function are written in a very confusing manner, making it almost impossible to figure out exactly what they do or check that they are correct. This patch renames SET_CONFIG_TRIES to PORT_INIT_TRIES, removes USE_NEW_SCHEME entirely, and rewrites use_new_scheme() to be much more transparent, with added comments explaining how it works. The patch also pulls the single call site of use_new_scheme() out from the Get-Descriptor retry loop (where it returns the same value each time) and renames the local variable used to store the result. The overall effect is a minor cleanup. However, there is one functional change: If the "use_both_schemes" module parameter isn't set (by default it is set), the existing code does only two retry iterations. After this patch it will always perform four, regardless of the parameter's value. Signed-off-by: Alan Stern --- [as1944] drivers/usb/core/hub.c | 49 ++++++++++++++++++++++++++----------------------- 1 file changed, 26 insertions(+), 23 deletions(-) Index: usb-devel/drivers/usb/core/hub.c =================================================================== --- usb-devel.orig/drivers/usb/core/hub.c +++ usb-devel/drivers/usb/core/hub.c @@ -2708,8 +2708,7 @@ static unsigned hub_is_wusb(struct usb_h #define PORT_RESET_TRIES 5 #define SET_ADDRESS_TRIES 2 #define GET_DESCRIPTOR_TRIES 2 -#define SET_CONFIG_TRIES (2 * (use_both_schemes + 1)) -#define USE_NEW_SCHEME(i, scheme) ((i) / 2 == (int)(scheme)) +#define PORT_INIT_TRIES 4 #define HUB_ROOT_RESET_TIME 60 /* times are in msec */ #define HUB_SHORT_RESET_TIME 10 @@ -2717,23 +2716,31 @@ static unsigned hub_is_wusb(struct usb_h #define HUB_LONG_RESET_TIME 200 #define HUB_RESET_TIMEOUT 800 -/* - * "New scheme" enumeration causes an extra state transition to be - * exposed to an xhci host and causes USB3 devices to receive control - * commands in the default state. This has been seen to cause - * enumeration failures, so disable this enumeration scheme for USB3 - * devices. - */ static bool use_new_scheme(struct usb_device *udev, int retry, struct usb_port *port_dev) { int old_scheme_first_port = - port_dev->quirks & USB_PORT_QUIRK_OLD_SCHEME; + (port_dev->quirks & USB_PORT_QUIRK_OLD_SCHEME) || + old_scheme_first; + /* + * "New scheme" enumeration causes an extra state transition to be + * exposed to an xhci host and causes USB3 devices to receive control + * commands in the default state. This has been seen to cause + * enumeration failures, so disable this enumeration scheme for USB3 + * devices. + */ if (udev->speed >= USB_SPEED_SUPER) return false; - return USE_NEW_SCHEME(retry, old_scheme_first_port || old_scheme_first); + /* + * If use_both_schemes is set, use the first scheme (whichever + * it is) for the larger half of the retries, then use the other + * scheme. Otherwise, use the first scheme for all the retries. + */ + if (use_both_schemes && retry >= (PORT_INIT_TRIES + 1) / 2) + return old_scheme_first_port; /* Second half */ + return !old_scheme_first_port; /* First half or all */ } /* Is a USB 3.0 port in the Inactive or Compliance Mode state? @@ -4545,6 +4552,7 @@ hub_port_init(struct usb_hub *hub, struc const char *speed; int devnum = udev->devnum; const char *driver_name; + bool do_new_scheme; /* root hub ports have a slightly longer reset period * (from USB 2.0 spec, section 7.1.7.5) @@ -4657,14 +4665,13 @@ hub_port_init(struct usb_hub *hub, struc * first 8 bytes of the device descriptor to get the ep0 maxpacket * value. */ - for (retries = 0; retries < GET_DESCRIPTOR_TRIES; (++retries, msleep(100))) { - bool did_new_scheme = false; + do_new_scheme = use_new_scheme(udev, retry_counter, port_dev); - if (use_new_scheme(udev, retry_counter, port_dev)) { + for (retries = 0; retries < GET_DESCRIPTOR_TRIES; (++retries, msleep(100))) { + if (do_new_scheme) { struct usb_device_descriptor *buf; int r = 0; - did_new_scheme = true; retval = hub_enable_device(udev); if (retval < 0) { dev_err(&udev->dev, @@ -4773,11 +4780,7 @@ hub_port_init(struct usb_hub *hub, struc * - read ep0 maxpacket even for high and low speed, */ msleep(10); - /* use_new_scheme() checks the speed which may have - * changed since the initial look so we cache the result - * in did_new_scheme - */ - if (did_new_scheme) + if (do_new_scheme) break; } @@ -5106,7 +5109,7 @@ static void hub_port_connect(struct usb_ unit_load = 100; status = 0; - for (i = 0; i < SET_CONFIG_TRIES; i++) { + for (i = 0; i < PORT_INIT_TRIES; i++) { /* reallocate for each attempt, since references * to the previous one can escape in various ways @@ -5239,7 +5242,7 @@ loop: break; /* When halfway through our retry count, power-cycle the port */ - if (i == (SET_CONFIG_TRIES / 2) - 1) { + if (i == (PORT_INIT_TRIES - 1) / 2) { dev_info(&port_dev->dev, "attempt power cycle\n"); usb_hub_set_port_power(hdev, hub, port1, false); msleep(2 * hub_power_on_good_delay(hub)); @@ -5770,7 +5773,7 @@ static int usb_reset_and_verify_device(s bos = udev->bos; udev->bos = NULL; - for (i = 0; i < SET_CONFIG_TRIES; ++i) { + for (i = 0; i < PORT_INIT_TRIES; ++i) { /* ep0 maxpacket size may change; let the HCD know about it. * Other endpoints will be handled by re-enumeration. */ From patchwork Mon Sep 28 15:22:17 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Alan Stern X-Patchwork-Id: 11803973 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id F2641618 for ; Mon, 28 Sep 2020 15:22:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id E421E208FE for ; Mon, 28 Sep 2020 15:22:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726589AbgI1PWT (ORCPT ); Mon, 28 Sep 2020 11:22:19 -0400 Received: from netrider.rowland.org ([192.131.102.5]:50347 "HELO netrider.rowland.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with SMTP id S1726424AbgI1PWT (ORCPT ); Mon, 28 Sep 2020 11:22:19 -0400 Received: (qmail 136930 invoked by uid 1000); 28 Sep 2020 11:22:17 -0400 Date: Mon, 28 Sep 2020 11:22:17 -0400 From: Alan Stern To: Greg KH Cc: yasushi asano , andrew_gabbasov@mentor.com, "Rosca, Eugeniu \(ADITG/ESM1\)" , Baxter Jim , linux-usb@vger.kernel.org, "Nishiguchi, Naohiro \(ADITJ/SWG\)" , "Natsume, Wataru \(ADITJ/SWG\)" , =?utf-8?b?5rWF6YeO5oGt5Y+y?= Subject: [Patch 2/2]: USB: hub: Add Kconfig option to reduce number of port initialization retries Message-ID: <20200928152217.GB134701@rowland.harvard.edu> References: <20200920192114.GB1190206@rowland.harvard.edu> <20200921140342.3813-1-yazzep@gmail.com> <20200921144827.GC1213381@rowland.harvard.edu> <20200921150611.GD1213381@rowland.harvard.edu> <20200925184153.GA53451@rowland.harvard.edu> <20200928152050.GA134701@rowland.harvard.edu> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20200928152050.GA134701@rowland.harvard.edu> User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org Description based on one by Yasushi Asano: According to 6.7.22 A-UUT “Device No Response” for connection timeout of USB OTG and EH automated compliance plan v1.2, enumeration failure has to be detected within 30 seconds. However, the old and new enumeration schemes each make a total of 12 attempts, and each attempt can take 5 seconds to time out, so the PET test fails. This patch adds a new Kconfig option (CONFIG_USB_FEW_INIT_RETRIES); when the option is set all the initialization retry loops except the outermost are reduced to a single iteration. This reduces the total number of attempts to four, allowing Linux hosts to pass the PET test. The new option is disabled by default to preserve the existing behavior. The reduced number of retries may fail to initialize a few devices that currently do work, but for the most part there should be no change. And in cases where the initialization does fail, it will fail much more quickly. Reported-and-tested-by: yasushi asano Signed-off-by: Alan Stern --- [as1945] drivers/usb/core/Kconfig | 14 ++++++++++++++ drivers/usb/core/hub.c | 13 ++++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-) Index: usb-devel/drivers/usb/core/Kconfig =================================================================== --- usb-devel.orig/drivers/usb/core/Kconfig +++ usb-devel/drivers/usb/core/Kconfig @@ -32,6 +32,20 @@ config USB_DEFAULT_PERSIST If you have any questions about this, say Y here, only say N if you know exactly what you are doing. +config USB_FEW_INIT_RETRIES + bool "Limit USB device initialization to only a few retries" + help + When a new USB device is detected, the kernel tries very hard + to initialize and enumerate it, with lots of nested retry loops. + This almost always works, but when it fails it can take a long time. + This option tells the kernel to make only a few retry attempts, + so that the total time required for a failed initialization is + no more than 30 seconds (as required by the USB OTG spec). + + Say N here unless you require new-device enumeration failure to + occur within 30 seconds (as might be needed in an embedded + application). + config USB_DYNAMIC_MINORS bool "Dynamic USB minor allocation" help Index: usb-devel/drivers/usb/core/hub.c =================================================================== --- usb-devel.orig/drivers/usb/core/hub.c +++ usb-devel/drivers/usb/core/hub.c @@ -2705,10 +2705,20 @@ static unsigned hub_is_wusb(struct usb_h } +#ifdef CONFIG_USB_FEW_INIT_RETRIES +#define PORT_RESET_TRIES 2 +#define SET_ADDRESS_TRIES 1 +#define GET_DESCRIPTOR_TRIES 1 +#define GET_MAXPACKET0_TRIES 1 +#define PORT_INIT_TRIES 4 + +#else #define PORT_RESET_TRIES 5 #define SET_ADDRESS_TRIES 2 #define GET_DESCRIPTOR_TRIES 2 +#define GET_MAXPACKET0_TRIES 3 #define PORT_INIT_TRIES 4 +#endif /* CONFIG_USB_FEW_INIT_RETRIES */ #define HUB_ROOT_RESET_TIME 60 /* times are in msec */ #define HUB_SHORT_RESET_TIME 10 @@ -4691,7 +4701,8 @@ hub_port_init(struct usb_hub *hub, struc * 255 is for WUSB devices, we actually need to use * 512 (WUSB1.0[4.8.1]). */ - for (operations = 0; operations < 3; ++operations) { + for (operations = 0; operations < GET_MAXPACKET0_TRIES; + ++operations) { buf->bMaxPacketSize0 = 0; r = usb_control_msg(udev, usb_rcvaddr0pipe(), USB_REQ_GET_DESCRIPTOR, USB_DIR_IN,