[PATCH] usb: hub: Per-port setting to use old enumeration scheme

[PATCH] usb: hub: Per-port setting to use old enumeration scheme

[Nicolas Boichat]

The "old" enumeration scheme is considerably faster (it takes
~294ms instead of ~439ms to get the descriptor).

It is currently only possible to use the old scheme globally
(/sys/module/usbcore/parameters/old_scheme_first), which is not
desirable as the new scheme was introduced to increase compatibility
with more devices.

However, in our case, we care about time-to-active for a specific
USB device (which we make the firmware for), on a specific port
(that is pogo-pin based: not a standard USB port). This new
sysfs option makes it possible to use the old scheme on a single
port only.

Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
---

There are other "quirks" that we could add to reduce further
enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
to 10ms instead of 50ms as used currently), but the logic is quite
similar, so it'd be good to have this reviewed first.

 drivers/usb/core/hub.c  | 13 +++++++++----
 drivers/usb/core/hub.h  |  1 +
 drivers/usb/core/port.c | 23 +++++++++++++++++++++++
 include/linux/usb.h     |  7 +++++++
 4 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c2d993d3816f0..f900f66a62856 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2636,7 +2636,7 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
 #define SET_ADDRESS_TRIES	2
 #define GET_DESCRIPTOR_TRIES	2
 #define SET_CONFIG_TRIES	(2 * (use_both_schemes + 1))
-#define USE_NEW_SCHEME(i)	((i) / 2 == (int)old_scheme_first)
+#define USE_NEW_SCHEME(i, scheme)	((i) / 2 == (int)scheme)
 
 #define HUB_ROOT_RESET_TIME	60	/* times are in msec */
 #define HUB_SHORT_RESET_TIME	10
@@ -2651,12 +2651,16 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
  * enumeration failures, so disable this enumeration scheme for USB3
  * devices.
  */
-static bool use_new_scheme(struct usb_device *udev, int retry)
+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;
+
 	if (udev->speed >= USB_SPEED_SUPER)
 		return false;
 
-	return USE_NEW_SCHEME(retry);
+	return USE_NEW_SCHEME(retry, old_scheme_first_port || old_scheme_first);
 }
 
 /* Is a USB 3.0 port in the Inactive or Compliance Mode state?
@@ -4392,6 +4396,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
 {
 	struct usb_device	*hdev = hub->hdev;
 	struct usb_hcd		*hcd = bus_to_hcd(hdev->bus);
+	struct usb_port		*port_dev = hub->ports[port1 - 1];
 	int			retries, operations, retval, i;
 	unsigned		delay = HUB_SHORT_RESET_TIME;
 	enum usb_device_speed	oldspeed = udev->speed;
@@ -4513,7 +4518,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
 	for (retries = 0; retries < GET_DESCRIPTOR_TRIES; (++retries, msleep(100))) {
 		bool did_new_scheme = false;
 
-		if (use_new_scheme(udev, retry_counter)) {
+		if (use_new_scheme(udev, retry_counter, port_dev)) {
 			struct usb_device_descriptor *buf;
 			int r = 0;
 
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 4dc769ee9c740..4accfb63f7dcb 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -98,6 +98,7 @@ struct usb_port {
 	struct mutex status_lock;
 	u32 over_current_count;
 	u8 portnum;
+	u32 quirks;
 	unsigned int is_superspeed:1;
 	unsigned int usb3_lpm_u1_permit:1;
 	unsigned int usb3_lpm_u2_permit:1;
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 6979bde87d310..4a21431953953 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -50,6 +50,28 @@ static ssize_t over_current_count_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(over_current_count);
 
+static ssize_t quirks_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct usb_port *port_dev = to_usb_port(dev);
+
+	return sprintf(buf, "%08x\n", port_dev->quirks);
+}
+
+static ssize_t quirks_store(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	struct usb_port *port_dev = to_usb_port(dev);
+	u32 value;
+
+	if (kstrtou32(buf, 16, &value))
+		return -EINVAL;
+
+	port_dev->quirks = value;
+	return count;
+}
+static DEVICE_ATTR_RW(quirks);
+
 static ssize_t usb3_lpm_permit_show(struct device *dev,
 			      struct device_attribute *attr, char *buf)
 {
@@ -118,6 +140,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit);
 
 static struct attribute *port_dev_attrs[] = {
 	&dev_attr_connect_type.attr,
+	&dev_attr_quirks.attr,
 	&dev_attr_over_current_count.attr,
 	NULL,
 };
diff --git a/include/linux/usb.h b/include/linux/usb.h
index beffceec49158..2ade17992ed66 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -489,6 +489,13 @@ enum usb_port_connect_type {
 	USB_PORT_NOT_USED,
 };
 
+/*
+ * USB port quirks.
+ */
+
+/* For the given port, prefer the old (faster) enumeration scheme. */
+#define USB_PORT_QUIRK_OLD_SCHEME	BIT(0)
+
 /*
  * USB 2.0 Link Power Management (LPM) parameters.
  */
-- 
2.17.0.441.gb46fe60e1d-goog

usb: hub: Per-port setting to use old enumeration scheme

[Nicolas Boichat]

The "old" enumeration scheme is considerably faster (it takes
~294ms instead of ~439ms to get the descriptor).

It is currently only possible to use the old scheme globally
(/sys/module/usbcore/parameters/old_scheme_first), which is not
desirable as the new scheme was introduced to increase compatibility
with more devices.

However, in our case, we care about time-to-active for a specific
USB device (which we make the firmware for), on a specific port
(that is pogo-pin based: not a standard USB port). This new
sysfs option makes it possible to use the old scheme on a single
port only.

Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
---

There are other "quirks" that we could add to reduce further
enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
to 10ms instead of 50ms as used currently), but the logic is quite
similar, so it'd be good to have this reviewed first.

 drivers/usb/core/hub.c  | 13 +++++++++----
 drivers/usb/core/hub.h  |  1 +
 drivers/usb/core/port.c | 23 +++++++++++++++++++++++
 include/linux/usb.h     |  7 +++++++
 4 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c2d993d3816f0..f900f66a62856 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -2636,7 +2636,7 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
 #define SET_ADDRESS_TRIES	2
 #define GET_DESCRIPTOR_TRIES	2
 #define SET_CONFIG_TRIES	(2 * (use_both_schemes + 1))
-#define USE_NEW_SCHEME(i)	((i) / 2 == (int)old_scheme_first)
+#define USE_NEW_SCHEME(i, scheme)	((i) / 2 == (int)scheme)
 
 #define HUB_ROOT_RESET_TIME	60	/* times are in msec */
 #define HUB_SHORT_RESET_TIME	10
@@ -2651,12 +2651,16 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
  * enumeration failures, so disable this enumeration scheme for USB3
  * devices.
  */
-static bool use_new_scheme(struct usb_device *udev, int retry)
+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;
+
 	if (udev->speed >= USB_SPEED_SUPER)
 		return false;
 
-	return USE_NEW_SCHEME(retry);
+	return USE_NEW_SCHEME(retry, old_scheme_first_port || old_scheme_first);
 }
 
 /* Is a USB 3.0 port in the Inactive or Compliance Mode state?
@@ -4392,6 +4396,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
 {
 	struct usb_device	*hdev = hub->hdev;
 	struct usb_hcd		*hcd = bus_to_hcd(hdev->bus);
+	struct usb_port		*port_dev = hub->ports[port1 - 1];
 	int			retries, operations, retval, i;
 	unsigned		delay = HUB_SHORT_RESET_TIME;
 	enum usb_device_speed	oldspeed = udev->speed;
@@ -4513,7 +4518,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
 	for (retries = 0; retries < GET_DESCRIPTOR_TRIES; (++retries, msleep(100))) {
 		bool did_new_scheme = false;
 
-		if (use_new_scheme(udev, retry_counter)) {
+		if (use_new_scheme(udev, retry_counter, port_dev)) {
 			struct usb_device_descriptor *buf;
 			int r = 0;
 
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 4dc769ee9c740..4accfb63f7dcb 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -98,6 +98,7 @@ struct usb_port {
 	struct mutex status_lock;
 	u32 over_current_count;
 	u8 portnum;
+	u32 quirks;
 	unsigned int is_superspeed:1;
 	unsigned int usb3_lpm_u1_permit:1;
 	unsigned int usb3_lpm_u2_permit:1;
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 6979bde87d310..4a21431953953 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -50,6 +50,28 @@ static ssize_t over_current_count_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(over_current_count);
 
+static ssize_t quirks_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct usb_port *port_dev = to_usb_port(dev);
+
+	return sprintf(buf, "%08x\n", port_dev->quirks);
+}
+
+static ssize_t quirks_store(struct device *dev, struct device_attribute *attr,
+			    const char *buf, size_t count)
+{
+	struct usb_port *port_dev = to_usb_port(dev);
+	u32 value;
+
+	if (kstrtou32(buf, 16, &value))
+		return -EINVAL;
+
+	port_dev->quirks = value;
+	return count;
+}
+static DEVICE_ATTR_RW(quirks);
+
 static ssize_t usb3_lpm_permit_show(struct device *dev,
 			      struct device_attribute *attr, char *buf)
 {
@@ -118,6 +140,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit);
 
 static struct attribute *port_dev_attrs[] = {
 	&dev_attr_connect_type.attr,
+	&dev_attr_quirks.attr,
 	&dev_attr_over_current_count.attr,
 	NULL,
 };
diff --git a/include/linux/usb.h b/include/linux/usb.h
index beffceec49158..2ade17992ed66 100644
--- a/include/linux/usb.h
+++ b/include/linux/usb.h
@@ -489,6 +489,13 @@ enum usb_port_connect_type {
 	USB_PORT_NOT_USED,
 };
 
+/*
+ * USB port quirks.
+ */
+
+/* For the given port, prefer the old (faster) enumeration scheme. */
+#define USB_PORT_QUIRK_OLD_SCHEME	BIT(0)
+
 /*
  * USB 2.0 Link Power Management (LPM) parameters.
  */

Re: [PATCH] usb: hub: Per-port setting to use old enumeration scheme

[Alan Stern]

On Wed, 23 May 2018, Nicolas Boichat wrote:

> The "old" enumeration scheme is considerably faster (it takes
> ~294ms instead of ~439ms to get the descriptor).
> 
> It is currently only possible to use the old scheme globally
> (/sys/module/usbcore/parameters/old_scheme_first), which is not
> desirable as the new scheme was introduced to increase compatibility
> with more devices.
> 
> However, in our case, we care about time-to-active for a specific
> USB device (which we make the firmware for), on a specific port
> (that is pogo-pin based: not a standard USB port). This new
> sysfs option makes it possible to use the old scheme on a single
> port only.
> 
> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> ---
> 
> There are other "quirks" that we could add to reduce further
> enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
> to 10ms instead of 50ms as used currently), but the logic is quite
> similar, so it'd be good to have this reviewed first.

I'm not opposed to the idea in principle, although I don't like your
implementation because it breaks the original old_scheme_first
parameter.

Let's see what some other people think.

Yours is a rather special case, because you know exactly what device
will be attached to a specific port.  Still, I can see that sort of
thing happening in constrained and special-purpose settings.

How do you arrange to set the new quirk before the device is 
discovered?

Alan Stern

usb: hub: Per-port setting to use old enumeration scheme

[Alan Stern]

On Wed, 23 May 2018, Nicolas Boichat wrote:

> The "old" enumeration scheme is considerably faster (it takes
> ~294ms instead of ~439ms to get the descriptor).
> 
> It is currently only possible to use the old scheme globally
> (/sys/module/usbcore/parameters/old_scheme_first), which is not
> desirable as the new scheme was introduced to increase compatibility
> with more devices.
> 
> However, in our case, we care about time-to-active for a specific
> USB device (which we make the firmware for), on a specific port
> (that is pogo-pin based: not a standard USB port). This new
> sysfs option makes it possible to use the old scheme on a single
> port only.
> 
> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> ---
> 
> There are other "quirks" that we could add to reduce further
> enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
> to 10ms instead of 50ms as used currently), but the logic is quite
> similar, so it'd be good to have this reviewed first.

I'm not opposed to the idea in principle, although I don't like your
implementation because it breaks the original old_scheme_first
parameter.

Let's see what some other people think.

Yours is a rather special case, because you know exactly what device
will be attached to a specific port.  Still, I can see that sort of
thing happening in constrained and special-purpose settings.

How do you arrange to set the new quirk before the device is 
discovered?

Alan Stern
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: hub: Per-port setting to use old enumeration scheme

[Greg Kroah-Hartman]

On Wed, May 23, 2018 at 10:03:55AM -0400, Alan Stern wrote:
> On Wed, 23 May 2018, Nicolas Boichat wrote:
> 
> > The "old" enumeration scheme is considerably faster (it takes
> > ~294ms instead of ~439ms to get the descriptor).
> > 
> > It is currently only possible to use the old scheme globally
> > (/sys/module/usbcore/parameters/old_scheme_first), which is not
> > desirable as the new scheme was introduced to increase compatibility
> > with more devices.
> > 
> > However, in our case, we care about time-to-active for a specific
> > USB device (which we make the firmware for), on a specific port
> > (that is pogo-pin based: not a standard USB port). This new
> > sysfs option makes it possible to use the old scheme on a single
> > port only.
> > 
> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > ---
> > 
> > There are other "quirks" that we could add to reduce further
> > enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
> > to 10ms instead of 50ms as used currently), but the logic is quite
> > similar, so it'd be good to have this reviewed first.
> 
> I'm not opposed to the idea in principle, although I don't like your
> implementation because it breaks the original old_scheme_first
> parameter.
> 
> Let's see what some other people think.
> 
> Yours is a rather special case, because you know exactly what device
> will be attached to a specific port.  Still, I can see that sort of
> thing happening in constrained and special-purpose settings.
> 
> How do you arrange to set the new quirk before the device is 
> discovered?

Yeah, this last question is what I had when looking at this.  Or does it
not matter at first boot and only matters for wake-up?

thanks,

greg k-h

usb: hub: Per-port setting to use old enumeration scheme

[Greg Kroah-Hartman]

On Wed, May 23, 2018 at 10:03:55AM -0400, Alan Stern wrote:
> On Wed, 23 May 2018, Nicolas Boichat wrote:
> 
> > The "old" enumeration scheme is considerably faster (it takes
> > ~294ms instead of ~439ms to get the descriptor).
> > 
> > It is currently only possible to use the old scheme globally
> > (/sys/module/usbcore/parameters/old_scheme_first), which is not
> > desirable as the new scheme was introduced to increase compatibility
> > with more devices.
> > 
> > However, in our case, we care about time-to-active for a specific
> > USB device (which we make the firmware for), on a specific port
> > (that is pogo-pin based: not a standard USB port). This new
> > sysfs option makes it possible to use the old scheme on a single
> > port only.
> > 
> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> > ---
> > 
> > There are other "quirks" that we could add to reduce further
> > enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
> > to 10ms instead of 50ms as used currently), but the logic is quite
> > similar, so it'd be good to have this reviewed first.
> 
> I'm not opposed to the idea in principle, although I don't like your
> implementation because it breaks the original old_scheme_first
> parameter.
> 
> Let's see what some other people think.
> 
> Yours is a rather special case, because you know exactly what device
> will be attached to a specific port.  Still, I can see that sort of
> thing happening in constrained and special-purpose settings.
> 
> How do you arrange to set the new quirk before the device is 
> discovered?

Yeah, this last question is what I had when looking at this.  Or does it
not matter at first boot and only matters for wake-up?

thanks,

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: hub: Per-port setting to use old enumeration scheme

[Nicolas Boichat]

On Thu, May 24, 2018 at 12:39 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, May 23, 2018 at 10:03:55AM -0400, Alan Stern wrote:
>> On Wed, 23 May 2018, Nicolas Boichat wrote:
>>
>> > The "old" enumeration scheme is considerably faster (it takes
>> > ~294ms instead of ~439ms to get the descriptor).
>> >
>> > It is currently only possible to use the old scheme globally
>> > (/sys/module/usbcore/parameters/old_scheme_first), which is not
>> > desirable as the new scheme was introduced to increase compatibility
>> > with more devices.
>> >
>> > However, in our case, we care about time-to-active for a specific
>> > USB device (which we make the firmware for), on a specific port
>> > (that is pogo-pin based: not a standard USB port). This new
>> > sysfs option makes it possible to use the old scheme on a single
>> > port only.
>> >
>> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
>> > ---
>> >
>> > There are other "quirks" that we could add to reduce further
>> > enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
>> > to 10ms instead of 50ms as used currently), but the logic is quite
>> > similar, so it'd be good to have this reviewed first.
>>
>> I'm not opposed to the idea in principle, although I don't like your
>> implementation because it breaks the original old_scheme_first
>> parameter.

I don't think it breaks the original parameter? I mean,
/sys/module/usbcore/parameters/old_scheme_first is still a global
default, while bit 0 of /sys/bus/usb/devices/x/y/z/quirks becomes a
port-specific override.

>> Let's see what some other people think.
>>
>> Yours is a rather special case, because you know exactly what device
>> will be attached to a specific port.  Still, I can see that sort of
>> thing happening in constrained and special-purpose settings.
>>
>> How do you arrange to set the new quirk before the device is
>> discovered?
>
> Yeah, this last question is what I had when looking at this.  Or does it
> not matter at first boot and only matters for wake-up?

It does not matter on boot, we have plenty of time to enumerate the
device. We use USB (auto-)suspend and remote wake, so no
re-enumeration there either. It only matters on unplug/replug where
the device needs to be re-enumerated.

Somewhere in an init script, we would do this (we know in advance that
usb1 port2 is the bus/port where we have our pogo-pin USB interface,
so we can hard-code the path):
echo 1 > /sys/bus/usb/devices/usb1/1-0:1.0/usb1-port2/quirks

We could try to add ACPI support (just like connect_type), but we
don't strictly need it for our application.

Thanks,

usb: hub: Per-port setting to use old enumeration scheme

[Nicolas Boichat]

On Thu, May 24, 2018 at 12:39 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Wed, May 23, 2018 at 10:03:55AM -0400, Alan Stern wrote:
>> On Wed, 23 May 2018, Nicolas Boichat wrote:
>>
>> > The "old" enumeration scheme is considerably faster (it takes
>> > ~294ms instead of ~439ms to get the descriptor).
>> >
>> > It is currently only possible to use the old scheme globally
>> > (/sys/module/usbcore/parameters/old_scheme_first), which is not
>> > desirable as the new scheme was introduced to increase compatibility
>> > with more devices.
>> >
>> > However, in our case, we care about time-to-active for a specific
>> > USB device (which we make the firmware for), on a specific port
>> > (that is pogo-pin based: not a standard USB port). This new
>> > sysfs option makes it possible to use the old scheme on a single
>> > port only.
>> >
>> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
>> > ---
>> >
>> > There are other "quirks" that we could add to reduce further
>> > enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
>> > to 10ms instead of 50ms as used currently), but the logic is quite
>> > similar, so it'd be good to have this reviewed first.
>>
>> I'm not opposed to the idea in principle, although I don't like your
>> implementation because it breaks the original old_scheme_first
>> parameter.

I don't think it breaks the original parameter? I mean,
/sys/module/usbcore/parameters/old_scheme_first is still a global
default, while bit 0 of /sys/bus/usb/devices/x/y/z/quirks becomes a
port-specific override.

>> Let's see what some other people think.
>>
>> Yours is a rather special case, because you know exactly what device
>> will be attached to a specific port.  Still, I can see that sort of
>> thing happening in constrained and special-purpose settings.
>>
>> How do you arrange to set the new quirk before the device is
>> discovered?
>
> Yeah, this last question is what I had when looking at this.  Or does it
> not matter at first boot and only matters for wake-up?

It does not matter on boot, we have plenty of time to enumerate the
device. We use USB (auto-)suspend and remote wake, so no
re-enumeration there either. It only matters on unplug/replug where
the device needs to be re-enumerated.

Somewhere in an init script, we would do this (we know in advance that
usb1 port2 is the bus/port where we have our pogo-pin USB interface,
so we can hard-code the path):
echo 1 > /sys/bus/usb/devices/usb1/1-0:1.0/usb1-port2/quirks

We could try to add ACPI support (just like connect_type), but we
don't strictly need it for our application.

Thanks,
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: hub: Per-port setting to use old enumeration scheme

[Alan Stern]

On Thu, 24 May 2018, Nicolas Boichat wrote:

> On Thu, May 24, 2018 at 12:39 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Wed, May 23, 2018 at 10:03:55AM -0400, Alan Stern wrote:
> >> On Wed, 23 May 2018, Nicolas Boichat wrote:
> >>
> >> > The "old" enumeration scheme is considerably faster (it takes
> >> > ~294ms instead of ~439ms to get the descriptor).
> >> >
> >> > It is currently only possible to use the old scheme globally
> >> > (/sys/module/usbcore/parameters/old_scheme_first), which is not
> >> > desirable as the new scheme was introduced to increase compatibility
> >> > with more devices.
> >> >
> >> > However, in our case, we care about time-to-active for a specific
> >> > USB device (which we make the firmware for), on a specific port
> >> > (that is pogo-pin based: not a standard USB port). This new
> >> > sysfs option makes it possible to use the old scheme on a single
> >> > port only.
> >> >
> >> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> >> > ---
> >> >
> >> > There are other "quirks" that we could add to reduce further
> >> > enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
> >> > to 10ms instead of 50ms as used currently), but the logic is quite
> >> > similar, so it'd be good to have this reviewed first.
> >>
> >> I'm not opposed to the idea in principle, although I don't like your
> >> implementation because it breaks the original old_scheme_first
> >> parameter.
> 
> I don't think it breaks the original parameter? I mean,
> /sys/module/usbcore/parameters/old_scheme_first is still a global
> default, while bit 0 of /sys/bus/usb/devices/x/y/z/quirks becomes a
> port-specific override.

Oops, sorry, my mistake.  My email client wrapped the last line of
use_new_scheme(), and as a result I missed the fact that
old_scheme_first was getting or'ed into the expression.

> >> How do you arrange to set the new quirk before the device is
> >> discovered?
> >
> > Yeah, this last question is what I had when looking at this.  Or does it
> > not matter at first boot and only matters for wake-up?
> 
> It does not matter on boot, we have plenty of time to enumerate the
> device. We use USB (auto-)suspend and remote wake, so no
> re-enumeration there either. It only matters on unplug/replug where
> the device needs to be re-enumerated.
> 
> Somewhere in an init script, we would do this (we know in advance that
> usb1 port2 is the bus/port where we have our pogo-pin USB interface,
> so we can hard-code the path):
> echo 1 > /sys/bus/usb/devices/usb1/1-0:1.0/usb1-port2/quirks
> 
> We could try to add ACPI support (just like connect_type), but we
> don't strictly need it for our application.

Okay, that makes sense.  So this change looks okay to me.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern

usb: hub: Per-port setting to use old enumeration scheme

[Alan Stern]

On Thu, 24 May 2018, Nicolas Boichat wrote:

> On Thu, May 24, 2018 at 12:39 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Wed, May 23, 2018 at 10:03:55AM -0400, Alan Stern wrote:
> >> On Wed, 23 May 2018, Nicolas Boichat wrote:
> >>
> >> > The "old" enumeration scheme is considerably faster (it takes
> >> > ~294ms instead of ~439ms to get the descriptor).
> >> >
> >> > It is currently only possible to use the old scheme globally
> >> > (/sys/module/usbcore/parameters/old_scheme_first), which is not
> >> > desirable as the new scheme was introduced to increase compatibility
> >> > with more devices.
> >> >
> >> > However, in our case, we care about time-to-active for a specific
> >> > USB device (which we make the firmware for), on a specific port
> >> > (that is pogo-pin based: not a standard USB port). This new
> >> > sysfs option makes it possible to use the old scheme on a single
> >> > port only.
> >> >
> >> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> >> > ---
> >> >
> >> > There are other "quirks" that we could add to reduce further
> >> > enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
> >> > to 10ms instead of 50ms as used currently), but the logic is quite
> >> > similar, so it'd be good to have this reviewed first.
> >>
> >> I'm not opposed to the idea in principle, although I don't like your
> >> implementation because it breaks the original old_scheme_first
> >> parameter.
> 
> I don't think it breaks the original parameter? I mean,
> /sys/module/usbcore/parameters/old_scheme_first is still a global
> default, while bit 0 of /sys/bus/usb/devices/x/y/z/quirks becomes a
> port-specific override.

Oops, sorry, my mistake.  My email client wrapped the last line of
use_new_scheme(), and as a result I missed the fact that
old_scheme_first was getting or'ed into the expression.

> >> How do you arrange to set the new quirk before the device is
> >> discovered?
> >
> > Yeah, this last question is what I had when looking at this.  Or does it
> > not matter at first boot and only matters for wake-up?
> 
> It does not matter on boot, we have plenty of time to enumerate the
> device. We use USB (auto-)suspend and remote wake, so no
> re-enumeration there either. It only matters on unplug/replug where
> the device needs to be re-enumerated.
> 
> Somewhere in an init script, we would do this (we know in advance that
> usb1 port2 is the bus/port where we have our pogo-pin USB interface,
> so we can hard-code the path):
> echo 1 > /sys/bus/usb/devices/usb1/1-0:1.0/usb1-port2/quirks
> 
> We could try to add ACPI support (just like connect_type), but we
> don't strictly need it for our application.

Okay, that makes sense.  So this change looks okay to me.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: hub: Per-port setting to use old enumeration scheme

[Greg Kroah-Hartman]

On Thu, May 24, 2018 at 07:42:00AM +0800, Nicolas Boichat wrote:
> On Thu, May 24, 2018 at 12:39 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Wed, May 23, 2018 at 10:03:55AM -0400, Alan Stern wrote:
> >> On Wed, 23 May 2018, Nicolas Boichat wrote:
> >>
> >> > The "old" enumeration scheme is considerably faster (it takes
> >> > ~294ms instead of ~439ms to get the descriptor).
> >> >
> >> > It is currently only possible to use the old scheme globally
> >> > (/sys/module/usbcore/parameters/old_scheme_first), which is not
> >> > desirable as the new scheme was introduced to increase compatibility
> >> > with more devices.
> >> >
> >> > However, in our case, we care about time-to-active for a specific
> >> > USB device (which we make the firmware for), on a specific port
> >> > (that is pogo-pin based: not a standard USB port). This new
> >> > sysfs option makes it possible to use the old scheme on a single
> >> > port only.
> >> >
> >> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> >> > ---
> >> >
> >> > There are other "quirks" that we could add to reduce further
> >> > enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
> >> > to 10ms instead of 50ms as used currently), but the logic is quite
> >> > similar, so it'd be good to have this reviewed first.
> >>
> >> I'm not opposed to the idea in principle, although I don't like your
> >> implementation because it breaks the original old_scheme_first
> >> parameter.
> 
> I don't think it breaks the original parameter? I mean,
> /sys/module/usbcore/parameters/old_scheme_first is still a global
> default, while bit 0 of /sys/bus/usb/devices/x/y/z/quirks becomes a
> port-specific override.
> 
> >> Let's see what some other people think.
> >>
> >> Yours is a rather special case, because you know exactly what device
> >> will be attached to a specific port.  Still, I can see that sort of
> >> thing happening in constrained and special-purpose settings.
> >>
> >> How do you arrange to set the new quirk before the device is
> >> discovered?
> >
> > Yeah, this last question is what I had when looking at this.  Or does it
> > not matter at first boot and only matters for wake-up?
> 
> It does not matter on boot, we have plenty of time to enumerate the
> device. We use USB (auto-)suspend and remote wake, so no
> re-enumeration there either. It only matters on unplug/replug where
> the device needs to be re-enumerated.

How does this device get unplugged/replugged if it is connected directly
to the device?

> Somewhere in an init script, we would do this (we know in advance that
> usb1 port2 is the bus/port where we have our pogo-pin USB interface,
> so we can hard-code the path):
> echo 1 > /sys/bus/usb/devices/usb1/1-0:1.0/usb1-port2/quirks
> 
> We could try to add ACPI support (just like connect_type), but we
> don't strictly need it for our application.

Isn't there an "internal" ACPI flag for USB ports, or is that
what connect_type is?  Why wouldn't that work here instead?

thanks,

greg k-h

usb: hub: Per-port setting to use old enumeration scheme

[Greg Kroah-Hartman]

On Thu, May 24, 2018 at 07:42:00AM +0800, Nicolas Boichat wrote:
> On Thu, May 24, 2018 at 12:39 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Wed, May 23, 2018 at 10:03:55AM -0400, Alan Stern wrote:
> >> On Wed, 23 May 2018, Nicolas Boichat wrote:
> >>
> >> > The "old" enumeration scheme is considerably faster (it takes
> >> > ~294ms instead of ~439ms to get the descriptor).
> >> >
> >> > It is currently only possible to use the old scheme globally
> >> > (/sys/module/usbcore/parameters/old_scheme_first), which is not
> >> > desirable as the new scheme was introduced to increase compatibility
> >> > with more devices.
> >> >
> >> > However, in our case, we care about time-to-active for a specific
> >> > USB device (which we make the firmware for), on a specific port
> >> > (that is pogo-pin based: not a standard USB port). This new
> >> > sysfs option makes it possible to use the old scheme on a single
> >> > port only.
> >> >
> >> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> >> > ---
> >> >
> >> > There are other "quirks" that we could add to reduce further
> >> > enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
> >> > to 10ms instead of 50ms as used currently), but the logic is quite
> >> > similar, so it'd be good to have this reviewed first.
> >>
> >> I'm not opposed to the idea in principle, although I don't like your
> >> implementation because it breaks the original old_scheme_first
> >> parameter.
> 
> I don't think it breaks the original parameter? I mean,
> /sys/module/usbcore/parameters/old_scheme_first is still a global
> default, while bit 0 of /sys/bus/usb/devices/x/y/z/quirks becomes a
> port-specific override.
> 
> >> Let's see what some other people think.
> >>
> >> Yours is a rather special case, because you know exactly what device
> >> will be attached to a specific port.  Still, I can see that sort of
> >> thing happening in constrained and special-purpose settings.
> >>
> >> How do you arrange to set the new quirk before the device is
> >> discovered?
> >
> > Yeah, this last question is what I had when looking at this.  Or does it
> > not matter at first boot and only matters for wake-up?
> 
> It does not matter on boot, we have plenty of time to enumerate the
> device. We use USB (auto-)suspend and remote wake, so no
> re-enumeration there either. It only matters on unplug/replug where
> the device needs to be re-enumerated.

How does this device get unplugged/replugged if it is connected directly
to the device?

> Somewhere in an init script, we would do this (we know in advance that
> usb1 port2 is the bus/port where we have our pogo-pin USB interface,
> so we can hard-code the path):
> echo 1 > /sys/bus/usb/devices/usb1/1-0:1.0/usb1-port2/quirks
> 
> We could try to add ACPI support (just like connect_type), but we
> don't strictly need it for our application.

Isn't there an "internal" ACPI flag for USB ports, or is that
what connect_type is?  Why wouldn't that work here instead?

thanks,

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: hub: Per-port setting to use old enumeration scheme

[Nicolas Boichat]

On Fri, May 25, 2018 at 12:21 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, May 24, 2018 at 07:42:00AM +0800, Nicolas Boichat wrote:
>> On Thu, May 24, 2018 at 12:39 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Wed, May 23, 2018 at 10:03:55AM -0400, Alan Stern wrote:
>> >> On Wed, 23 May 2018, Nicolas Boichat wrote:
>> >>
>> >> > The "old" enumeration scheme is considerably faster (it takes
>> >> > ~294ms instead of ~439ms to get the descriptor).
>> >> >
>> >> > It is currently only possible to use the old scheme globally
>> >> > (/sys/module/usbcore/parameters/old_scheme_first), which is not
>> >> > desirable as the new scheme was introduced to increase compatibility
>> >> > with more devices.
>> >> >
>> >> > However, in our case, we care about time-to-active for a specific
>> >> > USB device (which we make the firmware for), on a specific port
>> >> > (that is pogo-pin based: not a standard USB port). This new
>> >> > sysfs option makes it possible to use the old scheme on a single
>> >> > port only.
>> >> >
>> >> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
>> >> > ---
>> >> >
>> >> > There are other "quirks" that we could add to reduce further
>> >> > enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
>> >> > to 10ms instead of 50ms as used currently), but the logic is quite
>> >> > similar, so it'd be good to have this reviewed first.
>> >>
>> >> I'm not opposed to the idea in principle, although I don't like your
>> >> implementation because it breaks the original old_scheme_first
>> >> parameter.
>>
>> I don't think it breaks the original parameter? I mean,
>> /sys/module/usbcore/parameters/old_scheme_first is still a global
>> default, while bit 0 of /sys/bus/usb/devices/x/y/z/quirks becomes a
>> port-specific override.
>>
>> >> Let's see what some other people think.
>> >>
>> >> Yours is a rather special case, because you know exactly what device
>> >> will be attached to a specific port.  Still, I can see that sort of
>> >> thing happening in constrained and special-purpose settings.
>> >>
>> >> How do you arrange to set the new quirk before the device is
>> >> discovered?
>> >
>> > Yeah, this last question is what I had when looking at this.  Or does it
>> > not matter at first boot and only matters for wake-up?
>>
>> It does not matter on boot, we have plenty of time to enumerate the
>> device. We use USB (auto-)suspend and remote wake, so no
>> re-enumeration there either. It only matters on unplug/replug where
>> the device needs to be re-enumerated.
>
> How does this device get unplugged/replugged if it is connected directly
> to the device?

It is external. Essentially, this is a tablet with a detachable
keyboard/touchpad. The interface between tablet and base is USB, over
pogo pins. The port is non-standard (pogo, not normal USB), and we
fully control the firmware on the base side as well, which allows us
to take shortcuts like this: we know exactly what device will be
connected on that port.

>> Somewhere in an init script, we would do this (we know in advance that
>> usb1 port2 is the bus/port where we have our pogo-pin USB interface,
>> so we can hard-code the path):
>> echo 1 > /sys/bus/usb/devices/usb1/1-0:1.0/usb1-port2/quirks
>>
>> We could try to add ACPI support (just like connect_type), but we
>> don't strictly need it for our application.
>
> Isn't there an "internal" ACPI flag for USB ports, or is that
> what connect_type is?  Why wouldn't that work here instead?

What we may need here is something like "external but non-standard, so
we can ignore compatibility constraints when enumerating"...

Thanks,

usb: hub: Per-port setting to use old enumeration scheme

[Nicolas Boichat]

On Fri, May 25, 2018 at 12:21 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Thu, May 24, 2018 at 07:42:00AM +0800, Nicolas Boichat wrote:
>> On Thu, May 24, 2018 at 12:39 AM, Greg Kroah-Hartman
>> <gregkh@linuxfoundation.org> wrote:
>> > On Wed, May 23, 2018 at 10:03:55AM -0400, Alan Stern wrote:
>> >> On Wed, 23 May 2018, Nicolas Boichat wrote:
>> >>
>> >> > The "old" enumeration scheme is considerably faster (it takes
>> >> > ~294ms instead of ~439ms to get the descriptor).
>> >> >
>> >> > It is currently only possible to use the old scheme globally
>> >> > (/sys/module/usbcore/parameters/old_scheme_first), which is not
>> >> > desirable as the new scheme was introduced to increase compatibility
>> >> > with more devices.
>> >> >
>> >> > However, in our case, we care about time-to-active for a specific
>> >> > USB device (which we make the firmware for), on a specific port
>> >> > (that is pogo-pin based: not a standard USB port). This new
>> >> > sysfs option makes it possible to use the old scheme on a single
>> >> > port only.
>> >> >
>> >> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
>> >> > ---
>> >> >
>> >> > There are other "quirks" that we could add to reduce further
>> >> > enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
>> >> > to 10ms instead of 50ms as used currently), but the logic is quite
>> >> > similar, so it'd be good to have this reviewed first.
>> >>
>> >> I'm not opposed to the idea in principle, although I don't like your
>> >> implementation because it breaks the original old_scheme_first
>> >> parameter.
>>
>> I don't think it breaks the original parameter? I mean,
>> /sys/module/usbcore/parameters/old_scheme_first is still a global
>> default, while bit 0 of /sys/bus/usb/devices/x/y/z/quirks becomes a
>> port-specific override.
>>
>> >> Let's see what some other people think.
>> >>
>> >> Yours is a rather special case, because you know exactly what device
>> >> will be attached to a specific port.  Still, I can see that sort of
>> >> thing happening in constrained and special-purpose settings.
>> >>
>> >> How do you arrange to set the new quirk before the device is
>> >> discovered?
>> >
>> > Yeah, this last question is what I had when looking at this.  Or does it
>> > not matter at first boot and only matters for wake-up?
>>
>> It does not matter on boot, we have plenty of time to enumerate the
>> device. We use USB (auto-)suspend and remote wake, so no
>> re-enumeration there either. It only matters on unplug/replug where
>> the device needs to be re-enumerated.
>
> How does this device get unplugged/replugged if it is connected directly
> to the device?

It is external. Essentially, this is a tablet with a detachable
keyboard/touchpad. The interface between tablet and base is USB, over
pogo pins. The port is non-standard (pogo, not normal USB), and we
fully control the firmware on the base side as well, which allows us
to take shortcuts like this: we know exactly what device will be
connected on that port.

>> Somewhere in an init script, we would do this (we know in advance that
>> usb1 port2 is the bus/port where we have our pogo-pin USB interface,
>> so we can hard-code the path):
>> echo 1 > /sys/bus/usb/devices/usb1/1-0:1.0/usb1-port2/quirks
>>
>> We could try to add ACPI support (just like connect_type), but we
>> don't strictly need it for our application.
>
> Isn't there an "internal" ACPI flag for USB ports, or is that
> what connect_type is?  Why wouldn't that work here instead?

What we may need here is something like "external but non-standard, so
we can ignore compatibility constraints when enumerating"...

Thanks,
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: hub: Per-port setting to use old enumeration scheme

[Greg Kroah-Hartman]

On Fri, May 25, 2018 at 06:05:16AM +0800, Nicolas Boichat wrote:
> On Fri, May 25, 2018 at 12:21 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, May 24, 2018 at 07:42:00AM +0800, Nicolas Boichat wrote:
> >> On Thu, May 24, 2018 at 12:39 AM, Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >> > On Wed, May 23, 2018 at 10:03:55AM -0400, Alan Stern wrote:
> >> >> On Wed, 23 May 2018, Nicolas Boichat wrote:
> >> >>
> >> >> > The "old" enumeration scheme is considerably faster (it takes
> >> >> > ~294ms instead of ~439ms to get the descriptor).
> >> >> >
> >> >> > It is currently only possible to use the old scheme globally
> >> >> > (/sys/module/usbcore/parameters/old_scheme_first), which is not
> >> >> > desirable as the new scheme was introduced to increase compatibility
> >> >> > with more devices.
> >> >> >
> >> >> > However, in our case, we care about time-to-active for a specific
> >> >> > USB device (which we make the firmware for), on a specific port
> >> >> > (that is pogo-pin based: not a standard USB port). This new
> >> >> > sysfs option makes it possible to use the old scheme on a single
> >> >> > port only.
> >> >> >
> >> >> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> >> >> > ---
> >> >> >
> >> >> > There are other "quirks" that we could add to reduce further
> >> >> > enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
> >> >> > to 10ms instead of 50ms as used currently), but the logic is quite
> >> >> > similar, so it'd be good to have this reviewed first.
> >> >>
> >> >> I'm not opposed to the idea in principle, although I don't like your
> >> >> implementation because it breaks the original old_scheme_first
> >> >> parameter.
> >>
> >> I don't think it breaks the original parameter? I mean,
> >> /sys/module/usbcore/parameters/old_scheme_first is still a global
> >> default, while bit 0 of /sys/bus/usb/devices/x/y/z/quirks becomes a
> >> port-specific override.
> >>
> >> >> Let's see what some other people think.
> >> >>
> >> >> Yours is a rather special case, because you know exactly what device
> >> >> will be attached to a specific port.  Still, I can see that sort of
> >> >> thing happening in constrained and special-purpose settings.
> >> >>
> >> >> How do you arrange to set the new quirk before the device is
> >> >> discovered?
> >> >
> >> > Yeah, this last question is what I had when looking at this.  Or does it
> >> > not matter at first boot and only matters for wake-up?
> >>
> >> It does not matter on boot, we have plenty of time to enumerate the
> >> device. We use USB (auto-)suspend and remote wake, so no
> >> re-enumeration there either. It only matters on unplug/replug where
> >> the device needs to be re-enumerated.
> >
> > How does this device get unplugged/replugged if it is connected directly
> > to the device?
> 
> It is external. Essentially, this is a tablet with a detachable
> keyboard/touchpad. The interface between tablet and base is USB, over
> pogo pins. The port is non-standard (pogo, not normal USB), and we
> fully control the firmware on the base side as well, which allows us
> to take shortcuts like this: we know exactly what device will be
> connected on that port.

Ah, ok, that makes more sense, thanks for the explanation.  I'll go
queue this up in a bit.

greg k-h

usb: hub: Per-port setting to use old enumeration scheme

[Greg Kroah-Hartman]

On Fri, May 25, 2018 at 06:05:16AM +0800, Nicolas Boichat wrote:
> On Fri, May 25, 2018 at 12:21 AM, Greg Kroah-Hartman
> <gregkh@linuxfoundation.org> wrote:
> > On Thu, May 24, 2018 at 07:42:00AM +0800, Nicolas Boichat wrote:
> >> On Thu, May 24, 2018 at 12:39 AM, Greg Kroah-Hartman
> >> <gregkh@linuxfoundation.org> wrote:
> >> > On Wed, May 23, 2018 at 10:03:55AM -0400, Alan Stern wrote:
> >> >> On Wed, 23 May 2018, Nicolas Boichat wrote:
> >> >>
> >> >> > The "old" enumeration scheme is considerably faster (it takes
> >> >> > ~294ms instead of ~439ms to get the descriptor).
> >> >> >
> >> >> > It is currently only possible to use the old scheme globally
> >> >> > (/sys/module/usbcore/parameters/old_scheme_first), which is not
> >> >> > desirable as the new scheme was introduced to increase compatibility
> >> >> > with more devices.
> >> >> >
> >> >> > However, in our case, we care about time-to-active for a specific
> >> >> > USB device (which we make the firmware for), on a specific port
> >> >> > (that is pogo-pin based: not a standard USB port). This new
> >> >> > sysfs option makes it possible to use the old scheme on a single
> >> >> > port only.
> >> >> >
> >> >> > Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> >> >> > ---
> >> >> >
> >> >> > There are other "quirks" that we could add to reduce further
> >> >> > enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
> >> >> > to 10ms instead of 50ms as used currently), but the logic is quite
> >> >> > similar, so it'd be good to have this reviewed first.
> >> >>
> >> >> I'm not opposed to the idea in principle, although I don't like your
> >> >> implementation because it breaks the original old_scheme_first
> >> >> parameter.
> >>
> >> I don't think it breaks the original parameter? I mean,
> >> /sys/module/usbcore/parameters/old_scheme_first is still a global
> >> default, while bit 0 of /sys/bus/usb/devices/x/y/z/quirks becomes a
> >> port-specific override.
> >>
> >> >> Let's see what some other people think.
> >> >>
> >> >> Yours is a rather special case, because you know exactly what device
> >> >> will be attached to a specific port.  Still, I can see that sort of
> >> >> thing happening in constrained and special-purpose settings.
> >> >>
> >> >> How do you arrange to set the new quirk before the device is
> >> >> discovered?
> >> >
> >> > Yeah, this last question is what I had when looking at this.  Or does it
> >> > not matter at first boot and only matters for wake-up?
> >>
> >> It does not matter on boot, we have plenty of time to enumerate the
> >> device. We use USB (auto-)suspend and remote wake, so no
> >> re-enumeration there either. It only matters on unplug/replug where
> >> the device needs to be re-enumerated.
> >
> > How does this device get unplugged/replugged if it is connected directly
> > to the device?
> 
> It is external. Essentially, this is a tablet with a detachable
> keyboard/touchpad. The interface between tablet and base is USB, over
> pogo pins. The port is non-standard (pogo, not normal USB), and we
> fully control the firmware on the base side as well, which allows us
> to take shortcuts like this: we know exactly what device will be
> connected on that port.

Ah, ok, that makes more sense, thanks for the explanation.  I'll go
queue this up in a bit.

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: hub: Per-port setting to use old enumeration scheme

[Greg Kroah-Hartman]

On Wed, May 23, 2018 at 10:16:56AM +0800, Nicolas Boichat wrote:
> The "old" enumeration scheme is considerably faster (it takes
> ~294ms instead of ~439ms to get the descriptor).
> 
> It is currently only possible to use the old scheme globally
> (/sys/module/usbcore/parameters/old_scheme_first), which is not
> desirable as the new scheme was introduced to increase compatibility
> with more devices.
> 
> However, in our case, we care about time-to-active for a specific
> USB device (which we make the firmware for), on a specific port
> (that is pogo-pin based: not a standard USB port). This new
> sysfs option makes it possible to use the old scheme on a single
> port only.
> 
> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> ---
> 
> There are other "quirks" that we could add to reduce further
> enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
> to 10ms instead of 50ms as used currently), but the logic is quite
> similar, so it'd be good to have this reviewed first.
> 
>  drivers/usb/core/hub.c  | 13 +++++++++----
>  drivers/usb/core/hub.h  |  1 +
>  drivers/usb/core/port.c | 23 +++++++++++++++++++++++
>  include/linux/usb.h     |  7 +++++++
>  4 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index c2d993d3816f0..f900f66a62856 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2636,7 +2636,7 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
>  #define SET_ADDRESS_TRIES	2
>  #define GET_DESCRIPTOR_TRIES	2
>  #define SET_CONFIG_TRIES	(2 * (use_both_schemes + 1))
> -#define USE_NEW_SCHEME(i)	((i) / 2 == (int)old_scheme_first)
> +#define USE_NEW_SCHEME(i, scheme)	((i) / 2 == (int)scheme)
>  
>  #define HUB_ROOT_RESET_TIME	60	/* times are in msec */
>  #define HUB_SHORT_RESET_TIME	10
> @@ -2651,12 +2651,16 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
>   * enumeration failures, so disable this enumeration scheme for USB3
>   * devices.
>   */
> -static bool use_new_scheme(struct usb_device *udev, int retry)
> +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;
> +
>  	if (udev->speed >= USB_SPEED_SUPER)
>  		return false;
>  
> -	return USE_NEW_SCHEME(retry);
> +	return USE_NEW_SCHEME(retry, old_scheme_first_port || old_scheme_first);
>  }
>  
>  /* Is a USB 3.0 port in the Inactive or Compliance Mode state?
> @@ -4392,6 +4396,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
>  {
>  	struct usb_device	*hdev = hub->hdev;
>  	struct usb_hcd		*hcd = bus_to_hcd(hdev->bus);
> +	struct usb_port		*port_dev = hub->ports[port1 - 1];
>  	int			retries, operations, retval, i;
>  	unsigned		delay = HUB_SHORT_RESET_TIME;
>  	enum usb_device_speed	oldspeed = udev->speed;
> @@ -4513,7 +4518,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
>  	for (retries = 0; retries < GET_DESCRIPTOR_TRIES; (++retries, msleep(100))) {
>  		bool did_new_scheme = false;
>  
> -		if (use_new_scheme(udev, retry_counter)) {
> +		if (use_new_scheme(udev, retry_counter, port_dev)) {
>  			struct usb_device_descriptor *buf;
>  			int r = 0;
>  
> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
> index 4dc769ee9c740..4accfb63f7dcb 100644
> --- a/drivers/usb/core/hub.h
> +++ b/drivers/usb/core/hub.h
> @@ -98,6 +98,7 @@ struct usb_port {
>  	struct mutex status_lock;
>  	u32 over_current_count;
>  	u8 portnum;
> +	u32 quirks;
>  	unsigned int is_superspeed:1;
>  	unsigned int usb3_lpm_u1_permit:1;
>  	unsigned int usb3_lpm_u2_permit:1;
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index 6979bde87d310..4a21431953953 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -50,6 +50,28 @@ static ssize_t over_current_count_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(over_current_count);
>  
> +static ssize_t quirks_show(struct device *dev,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	struct usb_port *port_dev = to_usb_port(dev);
> +
> +	return sprintf(buf, "%08x\n", port_dev->quirks);
> +}
> +
> +static ssize_t quirks_store(struct device *dev, struct device_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	struct usb_port *port_dev = to_usb_port(dev);
> +	u32 value;
> +
> +	if (kstrtou32(buf, 16, &value))
> +		return -EINVAL;
> +
> +	port_dev->quirks = value;
> +	return count;
> +}
> +static DEVICE_ATTR_RW(quirks);
> +
>  static ssize_t usb3_lpm_permit_show(struct device *dev,
>  			      struct device_attribute *attr, char *buf)
>  {
> @@ -118,6 +140,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit);
>  
>  static struct attribute *port_dev_attrs[] = {
>  	&dev_attr_connect_type.attr,
> +	&dev_attr_quirks.attr,
>  	&dev_attr_over_current_count.attr,
>  	NULL,
>  };

Oops, you add a new sysfs file, but do not document it anywhere.  It
needs to go into Documentation/ABI/ along with the other files,
otherwise no one knows how to use it.

Please fix that up and resend.

thanks,

greg k-h

usb: hub: Per-port setting to use old enumeration scheme

[Greg Kroah-Hartman]

On Wed, May 23, 2018 at 10:16:56AM +0800, Nicolas Boichat wrote:
> The "old" enumeration scheme is considerably faster (it takes
> ~294ms instead of ~439ms to get the descriptor).
> 
> It is currently only possible to use the old scheme globally
> (/sys/module/usbcore/parameters/old_scheme_first), which is not
> desirable as the new scheme was introduced to increase compatibility
> with more devices.
> 
> However, in our case, we care about time-to-active for a specific
> USB device (which we make the firmware for), on a specific port
> (that is pogo-pin based: not a standard USB port). This new
> sysfs option makes it possible to use the old scheme on a single
> port only.
> 
> Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
> ---
> 
> There are other "quirks" that we could add to reduce further
> enumeration time (e.g. reduce USB debounce time, reduce TRSTRCY
> to 10ms instead of 50ms as used currently), but the logic is quite
> similar, so it'd be good to have this reviewed first.
> 
>  drivers/usb/core/hub.c  | 13 +++++++++----
>  drivers/usb/core/hub.h  |  1 +
>  drivers/usb/core/port.c | 23 +++++++++++++++++++++++
>  include/linux/usb.h     |  7 +++++++
>  4 files changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index c2d993d3816f0..f900f66a62856 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -2636,7 +2636,7 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
>  #define SET_ADDRESS_TRIES	2
>  #define GET_DESCRIPTOR_TRIES	2
>  #define SET_CONFIG_TRIES	(2 * (use_both_schemes + 1))
> -#define USE_NEW_SCHEME(i)	((i) / 2 == (int)old_scheme_first)
> +#define USE_NEW_SCHEME(i, scheme)	((i) / 2 == (int)scheme)
>  
>  #define HUB_ROOT_RESET_TIME	60	/* times are in msec */
>  #define HUB_SHORT_RESET_TIME	10
> @@ -2651,12 +2651,16 @@ static unsigned hub_is_wusb(struct usb_hub *hub)
>   * enumeration failures, so disable this enumeration scheme for USB3
>   * devices.
>   */
> -static bool use_new_scheme(struct usb_device *udev, int retry)
> +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;
> +
>  	if (udev->speed >= USB_SPEED_SUPER)
>  		return false;
>  
> -	return USE_NEW_SCHEME(retry);
> +	return USE_NEW_SCHEME(retry, old_scheme_first_port || old_scheme_first);
>  }
>  
>  /* Is a USB 3.0 port in the Inactive or Compliance Mode state?
> @@ -4392,6 +4396,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
>  {
>  	struct usb_device	*hdev = hub->hdev;
>  	struct usb_hcd		*hcd = bus_to_hcd(hdev->bus);
> +	struct usb_port		*port_dev = hub->ports[port1 - 1];
>  	int			retries, operations, retval, i;
>  	unsigned		delay = HUB_SHORT_RESET_TIME;
>  	enum usb_device_speed	oldspeed = udev->speed;
> @@ -4513,7 +4518,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,
>  	for (retries = 0; retries < GET_DESCRIPTOR_TRIES; (++retries, msleep(100))) {
>  		bool did_new_scheme = false;
>  
> -		if (use_new_scheme(udev, retry_counter)) {
> +		if (use_new_scheme(udev, retry_counter, port_dev)) {
>  			struct usb_device_descriptor *buf;
>  			int r = 0;
>  
> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
> index 4dc769ee9c740..4accfb63f7dcb 100644
> --- a/drivers/usb/core/hub.h
> +++ b/drivers/usb/core/hub.h
> @@ -98,6 +98,7 @@ struct usb_port {
>  	struct mutex status_lock;
>  	u32 over_current_count;
>  	u8 portnum;
> +	u32 quirks;
>  	unsigned int is_superspeed:1;
>  	unsigned int usb3_lpm_u1_permit:1;
>  	unsigned int usb3_lpm_u2_permit:1;
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index 6979bde87d310..4a21431953953 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -50,6 +50,28 @@ static ssize_t over_current_count_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(over_current_count);
>  
> +static ssize_t quirks_show(struct device *dev,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	struct usb_port *port_dev = to_usb_port(dev);
> +
> +	return sprintf(buf, "%08x\n", port_dev->quirks);
> +}
> +
> +static ssize_t quirks_store(struct device *dev, struct device_attribute *attr,
> +			    const char *buf, size_t count)
> +{
> +	struct usb_port *port_dev = to_usb_port(dev);
> +	u32 value;
> +
> +	if (kstrtou32(buf, 16, &value))
> +		return -EINVAL;
> +
> +	port_dev->quirks = value;
> +	return count;
> +}
> +static DEVICE_ATTR_RW(quirks);
> +
>  static ssize_t usb3_lpm_permit_show(struct device *dev,
>  			      struct device_attribute *attr, char *buf)
>  {
> @@ -118,6 +140,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit);
>  
>  static struct attribute *port_dev_attrs[] = {
>  	&dev_attr_connect_type.attr,
> +	&dev_attr_quirks.attr,
>  	&dev_attr_over_current_count.attr,
>  	NULL,
>  };

Oops, you add a new sysfs file, but do not document it anywhere.  It
needs to go into Documentation/ABI/ along with the other files,
otherwise no one knows how to use it.

Please fix that up and resend.

thanks,

greg k-h
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html