[PATCH 0/3] usb: Reduce USB scanning time

[PATCH 0/3] usb: Reduce USB scanning time

[Primoz Fiser]

Hello, 

Recently we came across an embedded platform which has a very complex           
USB infrastructure. In total, 17 USB devices are connected over various         
USB hubs. Initial USB scanning on this platform takes an incredible             
amounts of time ~22 seconds. This huge delay is unacceptable if we are          
required to boot from USB and need to perform scanning on each boot-up.         
                                                                                
However this huge USB scanning delay is only apparent in Barebox as             
subsequent Linux drivers don't take as much time to enumerate the bus.          
Thus we decided to investigate and try to optimize USB scanning time as         
much as possible without loosing any functionality & retain stability.          
                                                                                
Code analysis of USB hub driver revealed many calls to mdelay(). Some of         
them unnecessary and some even longer than 1 second per port, statically           
placed without any checks if required at all. These delays significantly           
degrade USB scanning performance and must be reworked.                             
                                                                                   
Investigation lead us also to U-boot which was already tested by us on             
the same i.MX6 platform and didn't show such poor USB performance. We              
found out U-boot already implements quasi parallel USB scanning                    
strategy and thus we decided to port this functionality to Barebox.                
                                                                                   
We took great deal of ideas and code from U-boot patch series by Stefan            
Roese: https://lists.denx.de/pipermail/u-boot/2016-March/248493.html               
                                                                                   
Although we didn't achieve the same level of delay reduction as reported           
by Mr. Roese (28.4 to 1.8 seconds), USB scanning in our case now takes             
around 4.5 seconds (21.7 to 4.5 seconds) which is a significant                    
improvement!                                                                       
                                                                                   
We could take this patch series a step further by also porting patch 3/4           
"usb: Don't reset the USB hub a 2nd time" from U-boot. But according to            
comment in Barebox code: "This is a Windows scheme of initialization               
sequence, with double reset of the device (Linux uses the same                     
sequence)" we wanted to be safe and left the double device reset                   
functionality intact for the sake of retaining stability over speed                
performance.

All further details on changes are described in each patch description.

Best regards,
Primoz Fiser

Primoz Fiser (3):
  usb: hub_port_reset(): Speedup hub reset handling
  usb: usb_hub_port_connect_change(): Remove unnecessary delays
  usb: Change power-on / scanning timeout handling

 drivers/usb/core/hub.c | 184 +++++++++++++++++++++++++++++++++++------
 include/usb/usb.h      |   3 +
 2 files changed, 162 insertions(+), 25 deletions(-)

-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 1/3] usb: hub_port_reset(): Speedup hub reset handling

[Primoz Fiser]

Start with a short USB hub reset delay of 20ms. This can be enough for
some configurations. Switch to longer reset delay only if the short
delay hasn't been long enough.

This USB hub reset handling strategy is also used in the Linux kernel
USB hub driver, function hub_port_reset().

Without patch:

    $ time usb
    usb: USB: scanning bus for devices...
    usb: 17 USB Device(s) found
    time: 21750ms

With patch:

    $ time usb
    usb: USB: scanning bus for devices...
    usb: 17 USB Device(s) found
    time: 16355ms

Delta: ~5.5 seconds

Signed-off-by: Primoz Fiser <primoz.fiser@norik.com>
---
 drivers/usb/core/hub.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index d01a01607..91604e1ef 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -30,6 +30,9 @@
 
 #define USB_BUFSIZ  512
 
+#define HUB_SHORT_RESET_TIME	20
+#define HUB_LONG_RESET_TIME	200
+
 static int usb_get_hub_descriptor(struct usb_device *dev, void *data, int size)
 {
 	return usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
@@ -116,12 +119,13 @@ int hub_port_reset(struct usb_device *hub, int port, struct usb_device *usb)
 	int tries;
 	struct usb_port_status portsts;
 	unsigned short portstatus, portchange;
+	int delay = HUB_SHORT_RESET_TIME; /* start with short reset delay */
 
 	dev_dbg(&hub->dev, "hub_port_reset: resetting port %d...\n", port);
 	for (tries = 0; tries < MAX_TRIES; tries++) {
 
 		usb_set_port_feature(hub, port + 1, USB_PORT_FEAT_RESET);
-		mdelay(200);
+		mdelay(delay);
 
 		if (usb_get_port_status(hub, port + 1, &portsts) < 0) {
 			dev_dbg(&hub->dev, "get_port_status failed status %lX\n",
@@ -148,7 +152,8 @@ int hub_port_reset(struct usb_device *hub, int port, struct usb_device *usb)
 		if (portstatus & USB_PORT_STAT_ENABLE)
 			break;
 
-		mdelay(200);
+		/* Switch to long reset delay for the next round */
+		delay = HUB_LONG_RESET_TIME;
 	}
 
 	if (tries == MAX_TRIES) {
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 2/3] usb: usb_hub_port_connect_change(): Remove unnecessary delays

[Primoz Fiser]

Remove two mdelay(200) calls from usb_hub_port_connect_change() before
and after calling hub_port_reset().

These delays don't seem to be necessary since hub_port_reset() already
incorporates delays on its own.

Without patch:

    $ time usb
    usb: USB: scanning bus for devices...
    usb: 17 USB Device(s) found
    time: 16355ms

With patch:

    $ time usb
    usb: USB: scanning bus for devices...
    usb: 17 USB Device(s) found
    time: 10344ms

Delta: ~6 seconds

Signed-off-by: Primoz Fiser <primoz.fiser@norik.com>
---
 drivers/usb/core/hub.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 91604e1ef..7de6aedc4 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -211,8 +211,6 @@ static void usb_hub_port_connect_change(struct usb_device *dev, int port)
 	if (dev->children[port] && !(portstatus & USB_PORT_STAT_ENABLE))
 		usb_remove_device(dev->children[port]);
 
-	mdelay(200);
-
 	/* Allocate a new device struct for the port */
 	usb = usb_alloc_new_device();
 	usb->dev.parent = &dev->dev;
@@ -225,8 +223,6 @@ static void usb_hub_port_connect_change(struct usb_device *dev, int port)
 		return;
 	}
 
-	mdelay(200);
-
 	dev->children[port] = usb;
 	usb->parent = dev;
 	usb->portnr = port + 1;
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

[PATCH 3/3] usb: Change power-on / scanning timeout handling

[Primoz Fiser]

Change USB port scanning procedure and timeout handling in the
following ways:

1)
The power-on delay in usb_hub_power_on() is now reduced to a value of
max(100ms, "hub->desc.bPwrOn2PwrGood * 2"). The code does not wait
using mdelay, instead usb_hub_power_on() will wait before querying
the device in the scanning loop later. The total connection timeout for
this hub, which is (1 second + hub->desc.bPwrOn2PwrGood * 2) is
calculated and will be used in the following per-port scanning loop as
the timeout to detect active USB devices on this hub.

2)
Don't delay the minimum delay (for power to stabilize) in
usb_hub_power_on(). Instead skip querying these devices in the scanning
loop usb_scan_port() until the delay time is reached.

3)
The ports are now scanned in a quasi parallel way. The current code did
wait for each (unconnected) port to reach its timeout and only then
continue with the next port. This patch now changes this to scan all
ports of all USB hubs quasi simultaneously. For this, all ports are added
to a scanning list in usb_hub_configure_ports(). The list is later scanned
in usb_device_list_scan() until all ports are ready by either a) reaching
the connection timeout (calculated earlier), or by b) detecting a USB
device. This results in a faster USB scan time as the recursive scanning
of USB hubs connected to the hub that's currently being scanned will
start earlier.

4)
Ports with overcurrent detection will get scanned multiple times if OC
condition is detected (PORT_OVERCURRENT_MAX_SCAN_COUNT).

Without patch:

    $ time usb
    usb: USB: scanning bus for devices...
    usb: 17 USB Device(s) found
    time: 10344ms

With patch:

    $ time usb
    usb: USB: scanning bus for devices...
    usb: 17 USB Device(s) found
    time: 4529ms

Delta: ~6 seconds

Signed-off-by: Primoz Fiser <primoz.fiser@norik.com>
---
 drivers/usb/core/hub.c | 171 ++++++++++++++++++++++++++++++++++++-----
 include/usb/usb.h      |   3 +
 2 files changed, 155 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 7de6aedc4..8874775f1 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -33,6 +33,17 @@
 #define HUB_SHORT_RESET_TIME	20
 #define HUB_LONG_RESET_TIME	200
 
+#define PORT_OVERCURRENT_MAX_SCAN_COUNT         3
+
+struct usb_device_scan {
+	struct usb_device *dev;		/* USB hub device to scan */
+	struct usb_hub_device *hub;	/* USB hub struct */
+	int port;			/* USB port to scan */
+	struct list_head list;
+};
+
+static LIST_HEAD(usb_scan_list);
+
 static int usb_get_hub_descriptor(struct usb_device *dev, void *data, int size)
 {
 	return usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
@@ -87,9 +98,6 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
 		dev_dbg(&dev->dev, "port %d returns %lX\n", i + 1, dev->status);
 	}
 
-	/* Wait at least 2 * bPwrOn2PwrGood for PP to change */
-	mdelay(pgood_delay);
-
 	/* Enable power to the ports */
 	dev_dbg(&dev->dev, "enabling power on all ports\n");
 
@@ -98,8 +106,24 @@ static void usb_hub_power_on(struct usb_hub_device *hub)
 		dev_dbg(&dev->dev, "port %d returns %lX\n", i + 1, dev->status);
 	}
 
-	/* power on is encoded in 2ms increments -> times 2 for the actual delay */
-	mdelay(pgood_delay + 1000);
+	/*
+	 * Do a minimum delay of the larger value of 100ms or pgood_delay
+	 * so that the power can stabilize before the devices are queried
+	 */
+	hub->query_delay = get_time_ns() +
+		max(100, (int) pgood_delay) * MSECOND;
+
+	/*
+	 * Record the power-on timeout here. The max delay (timeout)
+	 * will be done based on this value in the USB port loop in
+	 * usb_hub_configure_ports() later.
+	 */
+	hub->connect_timeout = hub->query_delay + 1000 * MSECOND;
+
+	dev_dbg(&dev->dev, "devnum=%d poweron: query_delay=%d \
+			connect_timeout=%d\n",
+			dev->devnum, max(100, (int) pgood_delay),
+			max(100, (int) pgood_delay) + 1000);
 }
 
 #define MAX_TRIES 5
@@ -243,15 +267,33 @@ static void usb_hub_port_connect_change(struct usb_device *dev, int port)
 	device_detect(&usb->dev);
 }
 
-static int usb_hub_configure_port(struct usb_device *dev, int port)
+static int usb_scan_port(struct usb_device_scan *usb_scan)
 {
 	struct usb_port_status portsts;
 	unsigned short portstatus, portchange;
-	int connect_change = 0;
+	struct usb_device *dev;
+	struct usb_hub_device *hub;
+	int port;
+
+	dev = usb_scan->dev;
+	hub = usb_scan->hub;
+	port = usb_scan->port;
+
+	/*
+	 * Don't talk to the device before the query delay is expired.
+	 * This is needed for voltages to stabilize.
+	 */
+	if (get_time_ns() < hub->query_delay)
+		return 0;
 
 	if (usb_get_port_status(dev, port + 1, &portsts) < 0) {
 		dev_dbg(&dev->dev, "get_port_status failed\n");
-		return -EIO;
+		if(get_time_ns() >= hub->connect_timeout) {
+			dev_dbg(&dev->dev, "port=%d: timeout\n", port + 1);
+			/* Remove this device from scanning list */
+			goto remove;
+		}
+		return 0;
 	}
 
 	portstatus = le16_to_cpu(portsts.wPortStatus);
@@ -259,10 +301,24 @@ static int usb_hub_configure_port(struct usb_device *dev, int port)
 	dev_dbg(&dev->dev, "Port %d Status %X Change %X\n",
 			port + 1, portstatus, portchange);
 
-	if (portchange & USB_PORT_STAT_C_CONNECTION) {
-		dev_dbg(&dev->dev, "port %d connection change\n", port + 1);
-		connect_change = 1;
+	if (!(portchange & USB_PORT_STAT_C_CONNECTION)) {
+		if(get_time_ns() >= hub->connect_timeout) {
+			dev_dbg(&dev->dev, "port=%d: timeout\n", port + 1);
+			/* Remove this device from scanning list */
+			goto remove;
+		}
+		return 0;
 	}
+
+	/* Test if the connection came up, and if not exit */
+	if(!(portstatus & USB_PORT_STAT_CONNECTION))
+		return 0;
+
+	/* A new USB device is ready at this point */
+	dev_dbg(&dev->dev, "port=%d: USB dev found\n", port + 1);
+
+	usb_hub_port_connect_change(dev, port);
+
 	if (portchange & USB_PORT_STAT_C_ENABLE) {
 		dev_dbg(&dev->dev, "port %d enable change, status %x\n",
 				port + 1, portstatus);
@@ -278,13 +334,10 @@ static int usb_hub_configure_port(struct usb_device *dev, int port)
 			dev_dbg(&dev->dev, "already running port %i "  \
 					"disabled by hub (EMI?), " \
 					"re-enabling...\n", port + 1);
-				connect_change = 1;
+				usb_hub_port_connect_change(dev, port);
 		}
 	}
 
-	if (connect_change)
-		usb_hub_port_connect_change(dev, port);
-
 	if (portstatus & USB_PORT_STAT_SUSPEND) {
 		dev_dbg(&dev->dev, "port %d suspend change\n", port + 1);
 		usb_clear_port_feature(dev, port + 1,
@@ -295,7 +348,21 @@ static int usb_hub_configure_port(struct usb_device *dev, int port)
 		dev_dbg(&dev->dev, "port %d over-current change\n", port + 1);
 		usb_clear_port_feature(dev, port + 1,
 					USB_PORT_FEAT_C_OVER_CURRENT);
-		usb_hub_power_on(dev->hub);
+		/* Only power-on this one port */
+		usb_set_port_feature(dev, port + 1, USB_PORT_FEAT_POWER);
+		hub->overcurrent_count[port]++;
+
+		/*
+		 * If the max-scan-count is not reached, return without removing
+		 * the device from scan-list. This will re-issue a new scan.
+		 */
+		if (hub->overcurrent_count[port] <=
+				PORT_OVERCURRENT_MAX_SCAN_COUNT)
+			return 0;
+
+		/* Otherwise the device will get removed */
+		dev_dbg(&dev->dev,"Port %d over-current occurred %d times\n",
+				port + 1, hub->overcurrent_count[port]);
 	}
 
 	if (portchange & USB_PORT_STAT_C_RESET) {
@@ -304,9 +371,54 @@ static int usb_hub_configure_port(struct usb_device *dev, int port)
 					USB_PORT_FEAT_C_RESET);
 	}
 
+remove:
+	/*
+	 * We're done with this device, so let's remove this device from
+	 * scanning list
+	 */
+	list_del(&usb_scan->list);
+	free(usb_scan);
+
 	return 0;
 }
 
+static int usb_device_list_scan(void)
+{
+	struct usb_device_scan *usb_scan;
+	struct usb_device_scan *tmp;
+	static int running;
+	int ret = 0;
+
+	/* Only run this loop once for each controller */
+	if (running)
+		return 0;
+
+	running = 1;
+
+	while (1) {
+		/* We're done, once the list is empty again */
+		if (list_empty(&usb_scan_list))
+			goto out;
+
+		list_for_each_entry_safe(usb_scan, tmp, &usb_scan_list, list) {
+			/* Scan this port */
+			ret = usb_scan_port(usb_scan);
+			if (ret)
+				goto out;
+		}
+	}
+
+out:
+	/*
+	 * This USB controller has finished scanning all its connected
+	 * USB devices. Set "running" back to 0, so that other USB controllers
+	 * will scan their devices too.
+	 */
+	running = 0;
+
+	return ret;
+}
+
 static int usb_hub_configure(struct usb_device *dev)
 {
 	unsigned char buffer[USB_BUFSIZ], *bitmap;
@@ -431,12 +543,33 @@ static int usb_hub_configure(struct usb_device *dev)
 
 static int usb_hub_configure_ports(struct usb_device *dev)
 {
+	struct usb_device_scan *usb_scan;
 	int i;
 
-	for (i = 0; i < dev->maxchild; i++)
-		usb_hub_configure_port(dev, i);
+	/*
+	 * Only add the connected USB devices, including potential hubs,
+	 * to a scanning list. This list will get scanned and devices that
+	 * are detected (either via port connected or via port timeout)
+	 * will get removed from this list. Scanning of the devices on this
+	 * list will continue until all devices are removed.
+	 */
+	for (i = 0; i < dev->maxchild; i++) {
+		usb_scan = calloc(1, sizeof(*usb_scan));
+		if (!usb_scan) {
+			dev_err(&dev->dev, "Can't allocate memory for \
+					USB device scanning!\n");
+			return -ENOMEM;
+		}
+		usb_scan->dev = dev;
+		usb_scan->hub = dev->hub;
+		usb_scan->port = i;
+		list_add_tail(&usb_scan->list, &usb_scan_list);
+	}
 
-	return 0;
+	/*
+	 * And now call the scanning code which loops over the generated list
+	 */
+	return usb_device_list_scan();
 }
 
 static int usb_hub_detect(struct device_d *dev)
diff --git a/include/usb/usb.h b/include/usb/usb.h
index 4698308df..0045608d6 100644
--- a/include/usb/usb.h
+++ b/include/usb/usb.h
@@ -313,6 +313,9 @@ void usb_rescan(void);
 struct usb_hub_device {
 	struct usb_device *pusb_dev;
 	struct usb_hub_descriptor desc;
+	uint64_t connect_timeout; /* Device connection timeout in ns */
+	uint64_t query_delay; /* Device query delay in ns */
+	int overcurrent_count[USB_MAXCHILDREN]; /* Over-current counter */
 };
 
 /**
-- 
2.17.1


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH 0/3] usb: Reduce USB scanning time

[Sascha Hauer]

Hi Primoz,

On Tue, Oct 01, 2019 at 10:17:57AM +0200, Primoz Fiser wrote:
> Hello, 
> 
> Recently we came across an embedded platform which has a very complex           
> USB infrastructure. In total, 17 USB devices are connected over various         
> USB hubs. Initial USB scanning on this platform takes an incredible             
> amounts of time ~22 seconds. This huge delay is unacceptable if we are          
> required to boot from USB and need to perform scanning on each boot-up.         
>                                                                                 
> However this huge USB scanning delay is only apparent in Barebox as             
> subsequent Linux drivers don't take as much time to enumerate the bus.          
> Thus we decided to investigate and try to optimize USB scanning time as         
> much as possible without loosing any functionality & retain stability.          
>                                                                                 
> Code analysis of USB hub driver revealed many calls to mdelay(). Some of         
> them unnecessary and some even longer than 1 second per port, statically           
> placed without any checks if required at all. These delays significantly           
> degrade USB scanning performance and must be reworked.                             
>                                                                                    
> Investigation lead us also to U-boot which was already tested by us on             
> the same i.MX6 platform and didn't show such poor USB performance. We              
> found out U-boot already implements quasi parallel USB scanning                    
> strategy and thus we decided to port this functionality to Barebox.                
>                                                                                    
> We took great deal of ideas and code from U-boot patch series by Stefan            
> Roese: https://lists.denx.de/pipermail/u-boot/2016-March/248493.html               
>                                                                                    
> Although we didn't achieve the same level of delay reduction as reported           
> by Mr. Roese (28.4 to 1.8 seconds), USB scanning in our case now takes             
> around 4.5 seconds (21.7 to 4.5 seconds) which is a significant                    
> improvement!                                                                       
>                                                                                    
> We could take this patch series a step further by also porting patch 3/4           
> "usb: Don't reset the USB hub a 2nd time" from U-boot. But according to            
> comment in Barebox code: "This is a Windows scheme of initialization               
> sequence, with double reset of the device (Linux uses the same                     
> sequence)" we wanted to be safe and left the double device reset                   
> functionality intact for the sake of retaining stability over speed                
> performance.
> 
> All further details on changes are described in each patch description.

This is great stuff! I gave it a shot on an i.MX53 board with a 7port
USB hub and some devices connected. The probing time decreased from 9s
to 2s which is really a great improvement. Even without the hub and only
one device connected the time decreased from 2s to 800ms.

I looked at the patches and they look good to me, so I applied them.
Anyway, it's USB, so I'd be surprised if there are no regressions ;)
So please guys, test the result. And as usual, Other feedback to this
series is still appreciated and it's not too late to fix it up shall
we need to.

Thanks
 Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox