[PATCH] usb: core: add usb3 lpm sysfs

[PATCH] usb: core: add usb3 lpm sysfs

[Zhuang Jin Can]

Some usb3 devices may not support usb3 lpm well.
The patch adds a sysfs to enable/disable u1 or u2 of the port.The
settings apply to both before and after device enumeration.
Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.

The interface is useful for testing some USB3 devices during
development, and provides a way to disable usb3 lpm if the issues can
not be fixed in final products.

Signed-off-by: Zhuang Jin Can <jin.can.zhuang@intel.com>
---
 Documentation/ABI/testing/sysfs-bus-usb |   10 ++++
 drivers/usb/core/hub.c                  |   16 +++++-
 drivers/usb/core/hub.h                  |    4 ++
 drivers/usb/core/port.c                 |   89 ++++++++++++++++++++++++++++++-
 4 files changed, 116 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
index e5cc763..32fc689 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -179,3 +179,13 @@ Description:
 		Supported values are 0 - 15.
 		More information on how besl values map to microseconds can be found in
 		USB 2.0 ECN Errata for Link Power Management, section 4.10)
+
+What:		/sys/bus/usb/devices/.../(hub interface)/portX/usb3_lpm
+Date:		March 2015
+Contact:	Zhuang Jin Can <jin.can.zhuang@intel.com>
+Description:
+		Some USB3.0 devices may not support usb3 lpm well.
+		usb3_lpm attribute allows enabling/disabling usb3 lpm of the port.
+		It takes effect both before and after a usb device is enumerated.
+		Supported values are "0" if u1 and u2 are disabled, "u1" if only u1 is
+		enabled, "u2" if only u2 is enabled, "u1_u2" if u1 and u2 are enabled.
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index d7c3d5a..7732a41 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3996,6 +3996,8 @@ EXPORT_SYMBOL_GPL(usb_unlocked_disable_lpm);
 void usb_enable_lpm(struct usb_device *udev)
 {
 	struct usb_hcd *hcd;
+	struct usb_hub *hub;
+	struct usb_port *port_dev;
 
 	if (!udev || !udev->parent ||
 			udev->speed != USB_SPEED_SUPER ||
@@ -4015,8 +4017,18 @@ void usb_enable_lpm(struct usb_device *udev)
 	if (udev->lpm_disable_count > 0)
 		return;
 
-	usb_enable_link_state(hcd, udev, USB3_LPM_U1);
-	usb_enable_link_state(hcd, udev, USB3_LPM_U2);
+	hub = usb_hub_to_struct_hub(udev->parent);
+	if (!hub) {
+		dev_err(&udev->dev, "can't enable lpm, usb_hub is null.\n");
+		return;
+	}
+	port_dev = hub->ports[udev->portnum - 1];
+
+	if (port_dev->u1_is_enabled)
+		usb_enable_link_state(hcd, udev, USB3_LPM_U1);
+
+	if (port_dev->u2_is_enabled)
+		usb_enable_link_state(hcd, udev, USB3_LPM_U2);
 }
 EXPORT_SYMBOL_GPL(usb_enable_lpm);
 
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 688817f..1ae060e 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -92,6 +92,8 @@ struct usb_hub {
  * @status_lock: synchronize port_event() vs usb_port_{suspend|resume}
  * @portnum: port index num based one
  * @is_superspeed cache super-speed status
+ * @u1_is_enabled: whether u1 should be enabled.
+ * @u2_is_enabled: whether u2 should be enabled.
  */
 struct usb_port {
 	struct usb_device *child;
@@ -104,6 +106,8 @@ struct usb_port {
 	struct mutex status_lock;
 	u8 portnum;
 	unsigned int is_superspeed:1;
+	unsigned int u1_is_enabled:1;
+	unsigned int u2_is_enabled:1;
 };
 
 #define to_usb_port(_dev) \
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 2106183..7f4e6c7 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -50,6 +50,72 @@ static ssize_t connect_type_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(connect_type);
 
+static ssize_t usb3_lpm_show(struct device *dev,
+			      struct device_attribute *attr, char *buf)
+{
+	struct usb_port *port_dev = to_usb_port(dev);
+	const char *p;
+
+	if (port_dev->u1_is_enabled) {
+		if (port_dev->u2_is_enabled)
+			p = "u1_u2";
+		else
+			p = "u1";
+	} else {
+		if (port_dev->u2_is_enabled)
+			p = "u2";
+		else
+			p = "0";
+	}
+
+	return sprintf(buf, "%s\n", p);
+}
+
+static ssize_t usb3_lpm_store(struct device *dev,
+			       struct device_attribute *attr,
+			       const char *buf, size_t count)
+{
+	struct usb_port *port_dev = to_usb_port(dev);
+	struct usb_device *udev = port_dev->child;
+	struct usb_hcd *hcd;
+
+	if (!strncmp(buf, "u1_u2", 5)) {
+		port_dev->u1_is_enabled = 1;
+		port_dev->u2_is_enabled = 1;
+
+	} else if (!strncmp(buf, "u1", 2)) {
+		port_dev->u1_is_enabled = 1;
+		port_dev->u2_is_enabled = 0;
+
+	} else if (!strncmp(buf, "u2", 2)) {
+		port_dev->u1_is_enabled = 0;
+		port_dev->u2_is_enabled = 1;
+
+	} else if (!strncmp(buf, "0", 1)) {
+		port_dev->u1_is_enabled = 0;
+		port_dev->u2_is_enabled = 0;
+	} else
+		return -EINVAL;
+
+	/* If device is connected to the port, disable & enable lpm
+	 * to make new u1 u2 setting take effect immediately
+	 */
+	if (udev) {
+		hcd = bus_to_hcd(udev->bus);
+		if (!hcd)
+			return -EINVAL;
+		usb_lock_device(udev);
+		mutex_lock(hcd->bandwidth_mutex);
+		if (!usb_disable_lpm(udev))
+			usb_enable_lpm(udev);
+		mutex_unlock(hcd->bandwidth_mutex);
+		usb_unlock_device(udev);
+	}
+
+	return count;
+}
+static DEVICE_ATTR_RW(usb3_lpm);
+
 static struct attribute *port_dev_attrs[] = {
 	&dev_attr_connect_type.attr,
 	NULL,
@@ -64,6 +130,21 @@ static const struct attribute_group *port_dev_group[] = {
 	NULL,
 };
 
+static struct attribute *port_dev_usb3_attrs[] = {
+	&dev_attr_usb3_lpm.attr,
+	NULL,
+};
+
+static struct attribute_group port_dev_usb3_attr_grp = {
+	.attrs = port_dev_usb3_attrs,
+};
+
+static const struct attribute_group *port_dev_usb3_group[] = {
+	&port_dev_attr_grp,
+	&port_dev_usb3_attr_grp,
+	NULL,
+};
+
 static void usb_port_device_release(struct device *dev)
 {
 	struct usb_port *port_dev = to_usb_port(dev);
@@ -401,6 +482,7 @@ static void find_and_link_peer(struct usb_hub *hub, int port1)
 int usb_hub_create_port_device(struct usb_hub *hub, int port1)
 {
 	struct usb_port *port_dev;
+	struct usb_device *hdev = hub->hdev;
 	int retval;
 
 	port_dev = kzalloc(sizeof(*port_dev), GFP_KERNEL);
@@ -417,7 +499,12 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
 	port_dev->portnum = port1;
 	set_bit(port1, hub->power_bits);
 	port_dev->dev.parent = hub->intfdev;
-	port_dev->dev.groups = port_dev_group;
+	if (hub_is_superspeed(hdev)) {
+		port_dev->u1_is_enabled = 1;
+		port_dev->u2_is_enabled = 1;
+		port_dev->dev.groups = port_dev_usb3_group;
+	} else
+		port_dev->dev.groups = port_dev_group;
 	port_dev->dev.type = &usb_port_device_type;
 	port_dev->dev.driver = &usb_port_driver;
 	if (hub_is_superspeed(hub->hdev))
-- 
1.7.9.5

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Zhuang Jin Can]

+ Mathias and David.

Hi Mathias,

Please help to review this patch.

Thanks
Jincan

On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> Some usb3 devices may not support usb3 lpm well.
> The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> settings apply to both before and after device enumeration.
> Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> 
> The interface is useful for testing some USB3 devices during
> development, and provides a way to disable usb3 lpm if the issues can
> not be fixed in final products.
> 
> Signed-off-by: Zhuang Jin Can <jin.can.zhuang@intel.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-usb |   10 ++++
>  drivers/usb/core/hub.c                  |   16 +++++-
>  drivers/usb/core/hub.h                  |    4 ++
>  drivers/usb/core/port.c                 |   89 ++++++++++++++++++++++++++++++-
>  4 files changed, 116 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
> index e5cc763..32fc689 100644
> --- a/Documentation/ABI/testing/sysfs-bus-usb
> +++ b/Documentation/ABI/testing/sysfs-bus-usb
> @@ -179,3 +179,13 @@ Description:
>  		Supported values are 0 - 15.
>  		More information on how besl values map to microseconds can be found in
>  		USB 2.0 ECN Errata for Link Power Management, section 4.10)
> +
> +What:		/sys/bus/usb/devices/.../(hub interface)/portX/usb3_lpm
> +Date:		March 2015
> +Contact:	Zhuang Jin Can <jin.can.zhuang@intel.com>
> +Description:
> +		Some USB3.0 devices may not support usb3 lpm well.
> +		usb3_lpm attribute allows enabling/disabling usb3 lpm of the port.
> +		It takes effect both before and after a usb device is enumerated.
> +		Supported values are "0" if u1 and u2 are disabled, "u1" if only u1 is
> +		enabled, "u2" if only u2 is enabled, "u1_u2" if u1 and u2 are enabled.
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index d7c3d5a..7732a41 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3996,6 +3996,8 @@ EXPORT_SYMBOL_GPL(usb_unlocked_disable_lpm);
>  void usb_enable_lpm(struct usb_device *udev)
>  {
>  	struct usb_hcd *hcd;
> +	struct usb_hub *hub;
> +	struct usb_port *port_dev;
>  
>  	if (!udev || !udev->parent ||
>  			udev->speed != USB_SPEED_SUPER ||
> @@ -4015,8 +4017,18 @@ void usb_enable_lpm(struct usb_device *udev)
>  	if (udev->lpm_disable_count > 0)
>  		return;
>  
> -	usb_enable_link_state(hcd, udev, USB3_LPM_U1);
> -	usb_enable_link_state(hcd, udev, USB3_LPM_U2);
> +	hub = usb_hub_to_struct_hub(udev->parent);
> +	if (!hub) {
> +		dev_err(&udev->dev, "can't enable lpm, usb_hub is null.\n");
> +		return;
> +	}
> +	port_dev = hub->ports[udev->portnum - 1];
> +
> +	if (port_dev->u1_is_enabled)
> +		usb_enable_link_state(hcd, udev, USB3_LPM_U1);
> +
> +	if (port_dev->u2_is_enabled)
> +		usb_enable_link_state(hcd, udev, USB3_LPM_U2);
>  }
>  EXPORT_SYMBOL_GPL(usb_enable_lpm);
>  
> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
> index 688817f..1ae060e 100644
> --- a/drivers/usb/core/hub.h
> +++ b/drivers/usb/core/hub.h
> @@ -92,6 +92,8 @@ struct usb_hub {
>   * @status_lock: synchronize port_event() vs usb_port_{suspend|resume}
>   * @portnum: port index num based one
>   * @is_superspeed cache super-speed status
> + * @u1_is_enabled: whether u1 should be enabled.
> + * @u2_is_enabled: whether u2 should be enabled.
>   */
>  struct usb_port {
>  	struct usb_device *child;
> @@ -104,6 +106,8 @@ struct usb_port {
>  	struct mutex status_lock;
>  	u8 portnum;
>  	unsigned int is_superspeed:1;
> +	unsigned int u1_is_enabled:1;
> +	unsigned int u2_is_enabled:1;
>  };
>  
>  #define to_usb_port(_dev) \
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index 2106183..7f4e6c7 100644
> --- a/drivers/usb/core/port.c
> +++ b/drivers/usb/core/port.c
> @@ -50,6 +50,72 @@ static ssize_t connect_type_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(connect_type);
>  
> +static ssize_t usb3_lpm_show(struct device *dev,
> +			      struct device_attribute *attr, char *buf)
> +{
> +	struct usb_port *port_dev = to_usb_port(dev);
> +	const char *p;
> +
> +	if (port_dev->u1_is_enabled) {
> +		if (port_dev->u2_is_enabled)
> +			p = "u1_u2";
> +		else
> +			p = "u1";
> +	} else {
> +		if (port_dev->u2_is_enabled)
> +			p = "u2";
> +		else
> +			p = "0";
> +	}
> +
> +	return sprintf(buf, "%s\n", p);
> +}
> +
> +static ssize_t usb3_lpm_store(struct device *dev,
> +			       struct device_attribute *attr,
> +			       const char *buf, size_t count)
> +{
> +	struct usb_port *port_dev = to_usb_port(dev);
> +	struct usb_device *udev = port_dev->child;
> +	struct usb_hcd *hcd;
> +
> +	if (!strncmp(buf, "u1_u2", 5)) {
> +		port_dev->u1_is_enabled = 1;
> +		port_dev->u2_is_enabled = 1;
> +
> +	} else if (!strncmp(buf, "u1", 2)) {
> +		port_dev->u1_is_enabled = 1;
> +		port_dev->u2_is_enabled = 0;
> +
> +	} else if (!strncmp(buf, "u2", 2)) {
> +		port_dev->u1_is_enabled = 0;
> +		port_dev->u2_is_enabled = 1;
> +
> +	} else if (!strncmp(buf, "0", 1)) {
> +		port_dev->u1_is_enabled = 0;
> +		port_dev->u2_is_enabled = 0;
> +	} else
> +		return -EINVAL;
> +
> +	/* If device is connected to the port, disable & enable lpm
> +	 * to make new u1 u2 setting take effect immediately
> +	 */
> +	if (udev) {
> +		hcd = bus_to_hcd(udev->bus);
> +		if (!hcd)
> +			return -EINVAL;
> +		usb_lock_device(udev);
> +		mutex_lock(hcd->bandwidth_mutex);
> +		if (!usb_disable_lpm(udev))
> +			usb_enable_lpm(udev);
> +		mutex_unlock(hcd->bandwidth_mutex);
> +		usb_unlock_device(udev);
> +	}
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(usb3_lpm);
> +
>  static struct attribute *port_dev_attrs[] = {
>  	&dev_attr_connect_type.attr,
>  	NULL,
> @@ -64,6 +130,21 @@ static const struct attribute_group *port_dev_group[] = {
>  	NULL,
>  };
>  
> +static struct attribute *port_dev_usb3_attrs[] = {
> +	&dev_attr_usb3_lpm.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group port_dev_usb3_attr_grp = {
> +	.attrs = port_dev_usb3_attrs,
> +};
> +
> +static const struct attribute_group *port_dev_usb3_group[] = {
> +	&port_dev_attr_grp,
> +	&port_dev_usb3_attr_grp,
> +	NULL,
> +};
> +
>  static void usb_port_device_release(struct device *dev)
>  {
>  	struct usb_port *port_dev = to_usb_port(dev);
> @@ -401,6 +482,7 @@ static void find_and_link_peer(struct usb_hub *hub, int port1)
>  int usb_hub_create_port_device(struct usb_hub *hub, int port1)
>  {
>  	struct usb_port *port_dev;
> +	struct usb_device *hdev = hub->hdev;
>  	int retval;
>  
>  	port_dev = kzalloc(sizeof(*port_dev), GFP_KERNEL);
> @@ -417,7 +499,12 @@ int usb_hub_create_port_device(struct usb_hub *hub, int port1)
>  	port_dev->portnum = port1;
>  	set_bit(port1, hub->power_bits);
>  	port_dev->dev.parent = hub->intfdev;
> -	port_dev->dev.groups = port_dev_group;
> +	if (hub_is_superspeed(hdev)) {
> +		port_dev->u1_is_enabled = 1;
> +		port_dev->u2_is_enabled = 1;
> +		port_dev->dev.groups = port_dev_usb3_group;
> +	} else
> +		port_dev->dev.groups = port_dev_group;
>  	port_dev->dev.type = &usb_port_device_type;
>  	port_dev->dev.driver = &usb_port_driver;
>  	if (hub_is_superspeed(hub->hdev))
> -- 
> 1.7.9.5
> 

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Greg KH]

On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> Some usb3 devices may not support usb3 lpm well.
> The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> settings apply to both before and after device enumeration.
> Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> 
> The interface is useful for testing some USB3 devices during
> development, and provides a way to disable usb3 lpm if the issues can
> not be fixed in final products.

How is a user supposed to "know" to make this setting for a device?  Why
can't the kernel automatically set this value properly?  Why does it
need to be a kernel issue at all?

And when you are doing development of broken devices, the kernel doesn't
have to support you, you can run with debugging patches of your own
until you fix your firmware :)

thanks,

greg k-h

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Greg KH]

On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> Some usb3 devices may not support usb3 lpm well.
> The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> settings apply to both before and after device enumeration.
> Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> 
> The interface is useful for testing some USB3 devices during
> development, and provides a way to disable usb3 lpm if the issues can
> not be fixed in final products.

How is a user supposed to "know" to make this setting for a device?  Why
can't the kernel automatically set this value properly?  Why does it
need to be a kernel issue at all?

And when you are doing development of broken devices, the kernel doesn't
have to support you, you can run with debugging patches of your own
until you fix your firmware :)

thanks,

greg k-h

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Zhuang Jin Can]

Hi Greg KH,

On Tue, Apr 28, 2015 at 12:42:24PM +0200, Greg KH wrote:
> On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> > Some usb3 devices may not support usb3 lpm well.
> > The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> > settings apply to both before and after device enumeration.
> > Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> > enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> > 
> > The interface is useful for testing some USB3 devices during
> > development, and provides a way to disable usb3 lpm if the issues can
> > not be fixed in final products.
> 
> How is a user supposed to "know" to make this setting for a device?  Why
> can't the kernel automatically set this value properly?  Why does it
> need to be a kernel issue at all?
> 
By default kernel enables u1 u2 of all USB3 devices. This interface
provides the user to change this policy. User may set the policy
according to PID/VID of uevent or according to the platform information
known by userspace.

It's not a kernel issue, as u1 u2 is mandatory by USB3 compliance. But
for some internal hardwired USB3 connection, e.g. SSIC, passing USB3
compliance is not mandatory. So the interface provides a way for vendor
to ship with u1 or u2 broken products. Of course, this is not encouraged :).

> And when you are doing development of broken devices, the kernel doesn't
> have to support you, you can run with debugging patches of your own
> until you fix your firmware :)
> 
Understood. But I think other vendor or developer may face the same
issue in final product shipment or during development. Moreover, the
interface provide the flexibility for developer to separately
disable/enable u1 or u2, e.g. If they're debugging an u2 issue, they
can disable u1 to simplify the situtation.

Thanks
Jincan

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Greg KH]

On Wed, Apr 29, 2015 at 12:51:27AM +0800, Zhuang Jin Can wrote:
> Hi Greg KH,
> 
> On Tue, Apr 28, 2015 at 12:42:24PM +0200, Greg KH wrote:
> > On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> > > Some usb3 devices may not support usb3 lpm well.
> > > The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> > > settings apply to both before and after device enumeration.
> > > Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> > > enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> > > 
> > > The interface is useful for testing some USB3 devices during
> > > development, and provides a way to disable usb3 lpm if the issues can
> > > not be fixed in final products.
> > 
> > How is a user supposed to "know" to make this setting for a device?  Why
> > can't the kernel automatically set this value properly?  Why does it
> > need to be a kernel issue at all?
> > 
> By default kernel enables u1 u2 of all USB3 devices. This interface
> provides the user to change this policy. User may set the policy
> according to PID/VID of uevent or according to the platform information
> known by userspace.

And why would they ever want to do that?

> It's not a kernel issue, as u1 u2 is mandatory by USB3 compliance. But
> for some internal hardwired USB3 connection, e.g. SSIC, passing USB3
> compliance is not mandatory. So the interface provides a way for vendor
> to ship with u1 or u2 broken products. Of course, this is not encouraged :).

If the state is broken for those devices, we can't require the user to
fix it for us, the kernel should do it automatically.

> > And when you are doing development of broken devices, the kernel doesn't
> > have to support you, you can run with debugging patches of your own
> > until you fix your firmware :)
> > 
> Understood. But I think other vendor or developer may face the same
> issue in final product shipment or during development. Moreover, the
> interface provide the flexibility for developer to separately
> disable/enable u1 or u2, e.g. If they're debugging an u2 issue, they
> can disable u1 to simplify the situtation.

For debugging only, perhaps, but for a "normal" user, please let's
handle this automatically and don't create a switch that never gets used
by anyone or anything.

thanks,

greg k-h

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Zhuang Jin Can]

On Tue, Apr 28, 2015 at 11:11:10PM +0200, Greg KH wrote:
> On Wed, Apr 29, 2015 at 12:51:27AM +0800, Zhuang Jin Can wrote:
> > Hi Greg KH,
> > 
> > On Tue, Apr 28, 2015 at 12:42:24PM +0200, Greg KH wrote:
> > > On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> > > > Some usb3 devices may not support usb3 lpm well.
> > > > The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> > > > settings apply to both before and after device enumeration.
> > > > Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> > > > enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> > > > 
> > > > The interface is useful for testing some USB3 devices during
> > > > development, and provides a way to disable usb3 lpm if the issues can
> > > > not be fixed in final products.
> > > 
> > > How is a user supposed to "know" to make this setting for a device?  Why
> > > can't the kernel automatically set this value properly?  Why does it
> > > need to be a kernel issue at all?
> > > 
> > By default kernel enables u1 u2 of all USB3 devices. This interface
> > provides the user to change this policy. User may set the policy
> > according to PID/VID of uevent or according to the platform information
> > known by userspace.
> 
> And why would they ever want to do that?
> 
> > It's not a kernel issue, as u1 u2 is mandatory by USB3 compliance. But
> > for some internal hardwired USB3 connection, e.g. SSIC, passing USB3
> > compliance is not mandatory. So the interface provides a way for vendor
> > to ship with u1 or u2 broken products. Of course, this is not encouraged :).
> 
> If the state is broken for those devices, we can't require the user to
> fix it for us, the kernel should do it automatically.
> 
> > > And when you are doing development of broken devices, the kernel doesn't
> > > have to support you, you can run with debugging patches of your own
> > > until you fix your firmware :)
> > > 
> > Understood. But I think other vendor or developer may face the same
> > issue in final product shipment or during development. Moreover, the
> > interface provide the flexibility for developer to separately
> > disable/enable u1 or u2, e.g. If they're debugging an u2 issue, they
> > can disable u1 to simplify the situtation.
> 
> For debugging only, perhaps, but for a "normal" user, please let's
> handle this automatically and don't create a switch that never gets used
> by anyone or anything.
> 
Thanks Greg. Since so far the patch has no interesting value to the
community, I'll drop the patch.

Regards
Jincan

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Zhuang Jin Can]

On Tue, Apr 28, 2015 at 11:11:10PM +0200, Greg KH wrote:
> On Wed, Apr 29, 2015 at 12:51:27AM +0800, Zhuang Jin Can wrote:
> > Hi Greg KH,
> > 
> > On Tue, Apr 28, 2015 at 12:42:24PM +0200, Greg KH wrote:
> > > On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> > > > Some usb3 devices may not support usb3 lpm well.
> > > > The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> > > > settings apply to both before and after device enumeration.
> > > > Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> > > > enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> > > > 
> > > > The interface is useful for testing some USB3 devices during
> > > > development, and provides a way to disable usb3 lpm if the issues can
> > > > not be fixed in final products.
> > > 
> > > How is a user supposed to "know" to make this setting for a device?  Why
> > > can't the kernel automatically set this value properly?  Why does it
> > > need to be a kernel issue at all?
> > > 
> > By default kernel enables u1 u2 of all USB3 devices. This interface
> > provides the user to change this policy. User may set the policy
> > according to PID/VID of uevent or according to the platform information
> > known by userspace.
> 
> And why would they ever want to do that?
> 
> > It's not a kernel issue, as u1 u2 is mandatory by USB3 compliance. But
> > for some internal hardwired USB3 connection, e.g. SSIC, passing USB3
> > compliance is not mandatory. So the interface provides a way for vendor
> > to ship with u1 or u2 broken products. Of course, this is not encouraged :).
> 
> If the state is broken for those devices, we can't require the user to
> fix it for us, the kernel should do it automatically.
> 
> > > And when you are doing development of broken devices, the kernel doesn't
> > > have to support you, you can run with debugging patches of your own
> > > until you fix your firmware :)
> > > 
> > Understood. But I think other vendor or developer may face the same
> > issue in final product shipment or during development. Moreover, the
> > interface provide the flexibility for developer to separately
> > disable/enable u1 or u2, e.g. If they're debugging an u2 issue, they
> > can disable u1 to simplify the situtation.
> 
> For debugging only, perhaps, but for a "normal" user, please let's
> handle this automatically and don't create a switch that never gets used
> by anyone or anything.
> 
Thanks Greg. Since so far the patch has no interesting value to the
community, I'll drop the patch.

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

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Greg KH]

On Wed, Apr 29, 2015 at 03:20:04PM +0800, Zhuang Jin Can wrote:
> On Tue, Apr 28, 2015 at 11:11:10PM +0200, Greg KH wrote:
> > On Wed, Apr 29, 2015 at 12:51:27AM +0800, Zhuang Jin Can wrote:
> > > Hi Greg KH,
> > > 
> > > On Tue, Apr 28, 2015 at 12:42:24PM +0200, Greg KH wrote:
> > > > On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> > > > > Some usb3 devices may not support usb3 lpm well.
> > > > > The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> > > > > settings apply to both before and after device enumeration.
> > > > > Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> > > > > enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> > > > > 
> > > > > The interface is useful for testing some USB3 devices during
> > > > > development, and provides a way to disable usb3 lpm if the issues can
> > > > > not be fixed in final products.
> > > > 
> > > > How is a user supposed to "know" to make this setting for a device?  Why
> > > > can't the kernel automatically set this value properly?  Why does it
> > > > need to be a kernel issue at all?
> > > > 
> > > By default kernel enables u1 u2 of all USB3 devices. This interface
> > > provides the user to change this policy. User may set the policy
> > > according to PID/VID of uevent or according to the platform information
> > > known by userspace.
> > 
> > And why would they ever want to do that?
> > 
> > > It's not a kernel issue, as u1 u2 is mandatory by USB3 compliance. But
> > > for some internal hardwired USB3 connection, e.g. SSIC, passing USB3
> > > compliance is not mandatory. So the interface provides a way for vendor
> > > to ship with u1 or u2 broken products. Of course, this is not encouraged :).
> > 
> > If the state is broken for those devices, we can't require the user to
> > fix it for us, the kernel should do it automatically.
> > 
> > > > And when you are doing development of broken devices, the kernel doesn't
> > > > have to support you, you can run with debugging patches of your own
> > > > until you fix your firmware :)
> > > > 
> > > Understood. But I think other vendor or developer may face the same
> > > issue in final product shipment or during development. Moreover, the
> > > interface provide the flexibility for developer to separately
> > > disable/enable u1 or u2, e.g. If they're debugging an u2 issue, they
> > > can disable u1 to simplify the situtation.
> > 
> > For debugging only, perhaps, but for a "normal" user, please let's
> > handle this automatically and don't create a switch that never gets used
> > by anyone or anything.
> > 
> Thanks Greg. Since so far the patch has no interesting value to the
> community, I'll drop the patch.

I didn't say that, I said it needed some more work to be accepted.

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Greg KH]

On Wed, Apr 29, 2015 at 03:20:04PM +0800, Zhuang Jin Can wrote:
> On Tue, Apr 28, 2015 at 11:11:10PM +0200, Greg KH wrote:
> > On Wed, Apr 29, 2015 at 12:51:27AM +0800, Zhuang Jin Can wrote:
> > > Hi Greg KH,
> > > 
> > > On Tue, Apr 28, 2015 at 12:42:24PM +0200, Greg KH wrote:
> > > > On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> > > > > Some usb3 devices may not support usb3 lpm well.
> > > > > The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> > > > > settings apply to both before and after device enumeration.
> > > > > Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> > > > > enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> > > > > 
> > > > > The interface is useful for testing some USB3 devices during
> > > > > development, and provides a way to disable usb3 lpm if the issues can
> > > > > not be fixed in final products.
> > > > 
> > > > How is a user supposed to "know" to make this setting for a device?  Why
> > > > can't the kernel automatically set this value properly?  Why does it
> > > > need to be a kernel issue at all?
> > > > 
> > > By default kernel enables u1 u2 of all USB3 devices. This interface
> > > provides the user to change this policy. User may set the policy
> > > according to PID/VID of uevent or according to the platform information
> > > known by userspace.
> > 
> > And why would they ever want to do that?
> > 
> > > It's not a kernel issue, as u1 u2 is mandatory by USB3 compliance. But
> > > for some internal hardwired USB3 connection, e.g. SSIC, passing USB3
> > > compliance is not mandatory. So the interface provides a way for vendor
> > > to ship with u1 or u2 broken products. Of course, this is not encouraged :).
> > 
> > If the state is broken for those devices, we can't require the user to
> > fix it for us, the kernel should do it automatically.
> > 
> > > > And when you are doing development of broken devices, the kernel doesn't
> > > > have to support you, you can run with debugging patches of your own
> > > > until you fix your firmware :)
> > > > 
> > > Understood. But I think other vendor or developer may face the same
> > > issue in final product shipment or during development. Moreover, the
> > > interface provide the flexibility for developer to separately
> > > disable/enable u1 or u2, e.g. If they're debugging an u2 issue, they
> > > can disable u1 to simplify the situtation.
> > 
> > For debugging only, perhaps, but for a "normal" user, please let's
> > handle this automatically and don't create a switch that never gets used
> > by anyone or anything.
> > 
> Thanks Greg. Since so far the patch has no interesting value to the
> community, I'll drop the patch.

I didn't say that, I said it needed some more work to be accepted.

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Zhuang Jin Can]

On Wed, Apr 29, 2015 at 11:01:48AM +0200, Greg KH wrote:
> On Wed, Apr 29, 2015 at 03:20:04PM +0800, Zhuang Jin Can wrote:
> > On Tue, Apr 28, 2015 at 11:11:10PM +0200, Greg KH wrote:
> > > On Wed, Apr 29, 2015 at 12:51:27AM +0800, Zhuang Jin Can wrote:
> > > > Hi Greg KH,
> > > > 
> > > > On Tue, Apr 28, 2015 at 12:42:24PM +0200, Greg KH wrote:
> > > > > On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> > > > > > Some usb3 devices may not support usb3 lpm well.
> > > > > > The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> > > > > > settings apply to both before and after device enumeration.
> > > > > > Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> > > > > > enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> > > > > > 
> > > > > > The interface is useful for testing some USB3 devices during
> > > > > > development, and provides a way to disable usb3 lpm if the issues can
> > > > > > not be fixed in final products.
> > > > > 
> > > > > How is a user supposed to "know" to make this setting for a device?  Why
> > > > > can't the kernel automatically set this value properly?  Why does it
> > > > > need to be a kernel issue at all?
> > > > > 
> > > > By default kernel enables u1 u2 of all USB3 devices. This interface
> > > > provides the user to change this policy. User may set the policy
> > > > according to PID/VID of uevent or according to the platform information
> > > > known by userspace.
> > > 
> > > And why would they ever want to do that?
> > > 
> > > > It's not a kernel issue, as u1 u2 is mandatory by USB3 compliance. But
> > > > for some internal hardwired USB3 connection, e.g. SSIC, passing USB3
> > > > compliance is not mandatory. So the interface provides a way for vendor
> > > > to ship with u1 or u2 broken products. Of course, this is not encouraged :).
> > > 
> > > If the state is broken for those devices, we can't require the user to
> > > fix it for us, the kernel should do it automatically.
> > > 
> > > > > And when you are doing development of broken devices, the kernel doesn't
> > > > > have to support you, you can run with debugging patches of your own
> > > > > until you fix your firmware :)
> > > > > 
> > > > Understood. But I think other vendor or developer may face the same
> > > > issue in final product shipment or during development. Moreover, the
> > > > interface provide the flexibility for developer to separately
> > > > disable/enable u1 or u2, e.g. If they're debugging an u2 issue, they
> > > > can disable u1 to simplify the situtation.
> > > 
> > > For debugging only, perhaps, but for a "normal" user, please let's
> > > handle this automatically and don't create a switch that never gets used
> > > by anyone or anything.
> > > 
> > Thanks Greg. Since so far the patch has no interesting value to the
> > community, I'll drop the patch.
> 
> I didn't say that, I said it needed some more work to be accepted.
Sorry for misunderstanding. Let me explain more why we need this interface.

We have a modem USB3 device (in stepping C) hardwired to one specific port of xHCI.
The device was expected to work with u1 u2, however, due to a HW issue, it doesn't
work stably. To workaround the issue, we let the init.rc script disable u1 u2 for
this specific port.

Then maybe we want to start debug u1 issue first, to avoid hitting u2 issue,
we can disable u2. After u1 issue is resolved, we can enable back u2 to continue to
debug u2 issue. This provides the flexibility to isolate u1 u2 debugging.
This is valuable I think :)

The HW issue will be fixed in stepping D, however C and D will have the same PID/VID.
There's no way for kernel to know the difference between C and D.
Even after fixing in D, C will still be used for development (to save money..)

If somehow finally we decide to ship stepping C (suppose HW issue can't be fixed in D in time),
we'll have to disable both u1 and u2.

If we fix only u1 issue, we can just disable u2, etc..

To summarize, it provide users an opptunity to change the u1 u2 policy to
debug and ship broken products.



Thanks
Jincan

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Zhuang Jin Can]

On Wed, Apr 29, 2015 at 11:01:48AM +0200, Greg KH wrote:
> On Wed, Apr 29, 2015 at 03:20:04PM +0800, Zhuang Jin Can wrote:
> > On Tue, Apr 28, 2015 at 11:11:10PM +0200, Greg KH wrote:
> > > On Wed, Apr 29, 2015 at 12:51:27AM +0800, Zhuang Jin Can wrote:
> > > > Hi Greg KH,
> > > > 
> > > > On Tue, Apr 28, 2015 at 12:42:24PM +0200, Greg KH wrote:
> > > > > On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> > > > > > Some usb3 devices may not support usb3 lpm well.
> > > > > > The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> > > > > > settings apply to both before and after device enumeration.
> > > > > > Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> > > > > > enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> > > > > > 
> > > > > > The interface is useful for testing some USB3 devices during
> > > > > > development, and provides a way to disable usb3 lpm if the issues can
> > > > > > not be fixed in final products.
> > > > > 
> > > > > How is a user supposed to "know" to make this setting for a device?  Why
> > > > > can't the kernel automatically set this value properly?  Why does it
> > > > > need to be a kernel issue at all?
> > > > > 
> > > > By default kernel enables u1 u2 of all USB3 devices. This interface
> > > > provides the user to change this policy. User may set the policy
> > > > according to PID/VID of uevent or according to the platform information
> > > > known by userspace.
> > > 
> > > And why would they ever want to do that?
> > > 
> > > > It's not a kernel issue, as u1 u2 is mandatory by USB3 compliance. But
> > > > for some internal hardwired USB3 connection, e.g. SSIC, passing USB3
> > > > compliance is not mandatory. So the interface provides a way for vendor
> > > > to ship with u1 or u2 broken products. Of course, this is not encouraged :).
> > > 
> > > If the state is broken for those devices, we can't require the user to
> > > fix it for us, the kernel should do it automatically.
> > > 
> > > > > And when you are doing development of broken devices, the kernel doesn't
> > > > > have to support you, you can run with debugging patches of your own
> > > > > until you fix your firmware :)
> > > > > 
> > > > Understood. But I think other vendor or developer may face the same
> > > > issue in final product shipment or during development. Moreover, the
> > > > interface provide the flexibility for developer to separately
> > > > disable/enable u1 or u2, e.g. If they're debugging an u2 issue, they
> > > > can disable u1 to simplify the situtation.
> > > 
> > > For debugging only, perhaps, but for a "normal" user, please let's
> > > handle this automatically and don't create a switch that never gets used
> > > by anyone or anything.
> > > 
> > Thanks Greg. Since so far the patch has no interesting value to the
> > community, I'll drop the patch.
> 
> I didn't say that, I said it needed some more work to be accepted.
Sorry for misunderstanding. Let me explain more why we need this interface.

We have a modem USB3 device (in stepping C) hardwired to one specific port of xHCI.
The device was expected to work with u1 u2, however, due to a HW issue, it doesn't
work stably. To workaround the issue, we let the init.rc script disable u1 u2 for
this specific port.

Then maybe we want to start debug u1 issue first, to avoid hitting u2 issue,
we can disable u2. After u1 issue is resolved, we can enable back u2 to continue to
debug u2 issue. This provides the flexibility to isolate u1 u2 debugging.
This is valuable I think :)

The HW issue will be fixed in stepping D, however C and D will have the same PID/VID.
There's no way for kernel to know the difference between C and D.
Even after fixing in D, C will still be used for development (to save money..)

If somehow finally we decide to ship stepping C (suppose HW issue can't be fixed in D in time),
we'll have to disable both u1 and u2.

If we fix only u1 issue, we can just disable u2, etc..

To summarize, it provide users an opptunity to change the u1 u2 policy to
debug and ship broken products.



Thanks
Jincan

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Greg KH]

On Wed, Apr 29, 2015 at 06:57:30PM +0800, Zhuang Jin Can wrote:
> On Wed, Apr 29, 2015 at 11:01:48AM +0200, Greg KH wrote:
> > On Wed, Apr 29, 2015 at 03:20:04PM +0800, Zhuang Jin Can wrote:
> > > On Tue, Apr 28, 2015 at 11:11:10PM +0200, Greg KH wrote:
> > > > On Wed, Apr 29, 2015 at 12:51:27AM +0800, Zhuang Jin Can wrote:
> > > > > Hi Greg KH,
> > > > > 
> > > > > On Tue, Apr 28, 2015 at 12:42:24PM +0200, Greg KH wrote:
> > > > > > On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> > > > > > > Some usb3 devices may not support usb3 lpm well.
> > > > > > > The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> > > > > > > settings apply to both before and after device enumeration.
> > > > > > > Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> > > > > > > enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> > > > > > > 
> > > > > > > The interface is useful for testing some USB3 devices during
> > > > > > > development, and provides a way to disable usb3 lpm if the issues can
> > > > > > > not be fixed in final products.
> > > > > > 
> > > > > > How is a user supposed to "know" to make this setting for a device?  Why
> > > > > > can't the kernel automatically set this value properly?  Why does it
> > > > > > need to be a kernel issue at all?
> > > > > > 
> > > > > By default kernel enables u1 u2 of all USB3 devices. This interface
> > > > > provides the user to change this policy. User may set the policy
> > > > > according to PID/VID of uevent or according to the platform information
> > > > > known by userspace.
> > > > 
> > > > And why would they ever want to do that?
> > > > 
> > > > > It's not a kernel issue, as u1 u2 is mandatory by USB3 compliance. But
> > > > > for some internal hardwired USB3 connection, e.g. SSIC, passing USB3
> > > > > compliance is not mandatory. So the interface provides a way for vendor
> > > > > to ship with u1 or u2 broken products. Of course, this is not encouraged :).
> > > > 
> > > > If the state is broken for those devices, we can't require the user to
> > > > fix it for us, the kernel should do it automatically.
> > > > 
> > > > > > And when you are doing development of broken devices, the kernel doesn't
> > > > > > have to support you, you can run with debugging patches of your own
> > > > > > until you fix your firmware :)
> > > > > > 
> > > > > Understood. But I think other vendor or developer may face the same
> > > > > issue in final product shipment or during development. Moreover, the
> > > > > interface provide the flexibility for developer to separately
> > > > > disable/enable u1 or u2, e.g. If they're debugging an u2 issue, they
> > > > > can disable u1 to simplify the situtation.
> > > > 
> > > > For debugging only, perhaps, but for a "normal" user, please let's
> > > > handle this automatically and don't create a switch that never gets used
> > > > by anyone or anything.
> > > > 
> > > Thanks Greg. Since so far the patch has no interesting value to the
> > > community, I'll drop the patch.
> > 
> > I didn't say that, I said it needed some more work to be accepted.
> Sorry for misunderstanding. Let me explain more why we need this interface.
> 
> We have a modem USB3 device (in stepping C) hardwired to one specific port of xHCI.
> The device was expected to work with u1 u2, however, due to a HW issue, it doesn't
> work stably. To workaround the issue, we let the init.rc script disable u1 u2 for
> this specific port.

Modern Linux systems don't have init.rc scripts anymore :)

> Then maybe we want to start debug u1 issue first, to avoid hitting u2 issue,
> we can disable u2. After u1 issue is resolved, we can enable back u2 to continue to
> debug u2 issue. This provides the flexibility to isolate u1 u2 debugging.
> This is valuable I think :)

I agree.

> The HW issue will be fixed in stepping D, however C and D will have the same PID/VID.
> There's no way for kernel to know the difference between C and D.
> Even after fixing in D, C will still be used for development (to save money..)

That sounds like a big design flaw, what about looking at the version of
the device?  That's what that field in the USB descriptors is for.

> If somehow finally we decide to ship stepping C (suppose HW issue can't be fixed in D in time),
> we'll have to disable both u1 and u2.
> 
> If we fix only u1 issue, we can just disable u2, etc..
> 
> To summarize, it provide users an opptunity to change the u1 u2 policy to
> debug and ship broken products.

That's good, but you need to do it somehow "automatically", as again,
systems don't have a init.rc file anymore :)

thanks,

greg k-h

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Greg KH]

On Wed, Apr 29, 2015 at 06:57:30PM +0800, Zhuang Jin Can wrote:
> On Wed, Apr 29, 2015 at 11:01:48AM +0200, Greg KH wrote:
> > On Wed, Apr 29, 2015 at 03:20:04PM +0800, Zhuang Jin Can wrote:
> > > On Tue, Apr 28, 2015 at 11:11:10PM +0200, Greg KH wrote:
> > > > On Wed, Apr 29, 2015 at 12:51:27AM +0800, Zhuang Jin Can wrote:
> > > > > Hi Greg KH,
> > > > > 
> > > > > On Tue, Apr 28, 2015 at 12:42:24PM +0200, Greg KH wrote:
> > > > > > On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> > > > > > > Some usb3 devices may not support usb3 lpm well.
> > > > > > > The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> > > > > > > settings apply to both before and after device enumeration.
> > > > > > > Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> > > > > > > enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> > > > > > > 
> > > > > > > The interface is useful for testing some USB3 devices during
> > > > > > > development, and provides a way to disable usb3 lpm if the issues can
> > > > > > > not be fixed in final products.
> > > > > > 
> > > > > > How is a user supposed to "know" to make this setting for a device?  Why
> > > > > > can't the kernel automatically set this value properly?  Why does it
> > > > > > need to be a kernel issue at all?
> > > > > > 
> > > > > By default kernel enables u1 u2 of all USB3 devices. This interface
> > > > > provides the user to change this policy. User may set the policy
> > > > > according to PID/VID of uevent or according to the platform information
> > > > > known by userspace.
> > > > 
> > > > And why would they ever want to do that?
> > > > 
> > > > > It's not a kernel issue, as u1 u2 is mandatory by USB3 compliance. But
> > > > > for some internal hardwired USB3 connection, e.g. SSIC, passing USB3
> > > > > compliance is not mandatory. So the interface provides a way for vendor
> > > > > to ship with u1 or u2 broken products. Of course, this is not encouraged :).
> > > > 
> > > > If the state is broken for those devices, we can't require the user to
> > > > fix it for us, the kernel should do it automatically.
> > > > 
> > > > > > And when you are doing development of broken devices, the kernel doesn't
> > > > > > have to support you, you can run with debugging patches of your own
> > > > > > until you fix your firmware :)
> > > > > > 
> > > > > Understood. But I think other vendor or developer may face the same
> > > > > issue in final product shipment or during development. Moreover, the
> > > > > interface provide the flexibility for developer to separately
> > > > > disable/enable u1 or u2, e.g. If they're debugging an u2 issue, they
> > > > > can disable u1 to simplify the situtation.
> > > > 
> > > > For debugging only, perhaps, but for a "normal" user, please let's
> > > > handle this automatically and don't create a switch that never gets used
> > > > by anyone or anything.
> > > > 
> > > Thanks Greg. Since so far the patch has no interesting value to the
> > > community, I'll drop the patch.
> > 
> > I didn't say that, I said it needed some more work to be accepted.
> Sorry for misunderstanding. Let me explain more why we need this interface.
> 
> We have a modem USB3 device (in stepping C) hardwired to one specific port of xHCI.
> The device was expected to work with u1 u2, however, due to a HW issue, it doesn't
> work stably. To workaround the issue, we let the init.rc script disable u1 u2 for
> this specific port.

Modern Linux systems don't have init.rc scripts anymore :)

> Then maybe we want to start debug u1 issue first, to avoid hitting u2 issue,
> we can disable u2. After u1 issue is resolved, we can enable back u2 to continue to
> debug u2 issue. This provides the flexibility to isolate u1 u2 debugging.
> This is valuable I think :)

I agree.

> The HW issue will be fixed in stepping D, however C and D will have the same PID/VID.
> There's no way for kernel to know the difference between C and D.
> Even after fixing in D, C will still be used for development (to save money..)

That sounds like a big design flaw, what about looking at the version of
the device?  That's what that field in the USB descriptors is for.

> If somehow finally we decide to ship stepping C (suppose HW issue can't be fixed in D in time),
> we'll have to disable both u1 and u2.
> 
> If we fix only u1 issue, we can just disable u2, etc..
> 
> To summarize, it provide users an opptunity to change the u1 u2 policy to
> debug and ship broken products.

That's good, but you need to do it somehow "automatically", as again,
systems don't have a init.rc file anymore :)

thanks,

greg k-h

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Zhuang Jin Can]

On Wed, Apr 29, 2015 at 01:06:22PM +0200, Greg KH wrote:
> On Wed, Apr 29, 2015 at 06:57:30PM +0800, Zhuang Jin Can wrote:
> > On Wed, Apr 29, 2015 at 11:01:48AM +0200, Greg KH wrote:
> > > On Wed, Apr 29, 2015 at 03:20:04PM +0800, Zhuang Jin Can wrote:
> > > > On Tue, Apr 28, 2015 at 11:11:10PM +0200, Greg KH wrote:
> > > > > On Wed, Apr 29, 2015 at 12:51:27AM +0800, Zhuang Jin Can wrote:
> > > > > > Hi Greg KH,
> > > > > > 
> > > > > > On Tue, Apr 28, 2015 at 12:42:24PM +0200, Greg KH wrote:
> > > > > > > On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> > > > > > > > Some usb3 devices may not support usb3 lpm well.
> > > > > > > > The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> > > > > > > > settings apply to both before and after device enumeration.
> > > > > > > > Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> > > > > > > > enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> > > > > > > > 
> > > > > > > > The interface is useful for testing some USB3 devices during
> > > > > > > > development, and provides a way to disable usb3 lpm if the issues can
> > > > > > > > not be fixed in final products.
> > > > > > > 
> > > > > > > How is a user supposed to "know" to make this setting for a device?  Why
> > > > > > > can't the kernel automatically set this value properly?  Why does it
> > > > > > > need to be a kernel issue at all?
> > > > > > > 
> > > > > > By default kernel enables u1 u2 of all USB3 devices. This interface
> > > > > > provides the user to change this policy. User may set the policy
> > > > > > according to PID/VID of uevent or according to the platform information
> > > > > > known by userspace.
> > > > > 
> > > > > And why would they ever want to do that?
> > > > > 
> > > > > > It's not a kernel issue, as u1 u2 is mandatory by USB3 compliance. But
> > > > > > for some internal hardwired USB3 connection, e.g. SSIC, passing USB3
> > > > > > compliance is not mandatory. So the interface provides a way for vendor
> > > > > > to ship with u1 or u2 broken products. Of course, this is not encouraged :).
> > > > > 
> > > > > If the state is broken for those devices, we can't require the user to
> > > > > fix it for us, the kernel should do it automatically.
> > > > > 
> > > > > > > And when you are doing development of broken devices, the kernel doesn't
> > > > > > > have to support you, you can run with debugging patches of your own
> > > > > > > until you fix your firmware :)
> > > > > > > 
> > > > > > Understood. But I think other vendor or developer may face the same
> > > > > > issue in final product shipment or during development. Moreover, the
> > > > > > interface provide the flexibility for developer to separately
> > > > > > disable/enable u1 or u2, e.g. If they're debugging an u2 issue, they
> > > > > > can disable u1 to simplify the situtation.
> > > > > 
> > > > > For debugging only, perhaps, but for a "normal" user, please let's
> > > > > handle this automatically and don't create a switch that never gets used
> > > > > by anyone or anything.
> > > > > 
> > > > Thanks Greg. Since so far the patch has no interesting value to the
> > > > community, I'll drop the patch.
> > > 
> > > I didn't say that, I said it needed some more work to be accepted.
> > Sorry for misunderstanding. Let me explain more why we need this interface.
> > 
> > We have a modem USB3 device (in stepping C) hardwired to one specific port of xHCI.
> > The device was expected to work with u1 u2, however, due to a HW issue, it doesn't
> > work stably. To workaround the issue, we let the init.rc script disable u1 u2 for
> > this specific port.
> 
> Modern Linux systems don't have init.rc scripts anymore :)
> 
In Android, the init process still reads an init.rc where vendor can
define their own policies. Vendors normally provides a whole reference
design (including HW, FW, Kernel, BSP, AOSP) to OEMs. BSP contains
vendor specific configurations including its own init.rc.

> > Then maybe we want to start debug u1 issue first, to avoid hitting u2 issue,
> > we can disable u2. After u1 issue is resolved, we can enable back u2 to continue to
> > debug u2 issue. This provides the flexibility to isolate u1 u2 debugging.
> > This is valuable I think :)
> 
> I agree.
> 
> > The HW issue will be fixed in stepping D, however C and D will have the same PID/VID.
> > There's no way for kernel to know the difference between C and D.
> > Even after fixing in D, C will still be used for development (to save money..)
> 
> That sounds like a big design flaw, what about looking at the version of
> the device?  That's what that field in the USB descriptors is for.
> 
You're right. bcdDevice should be used for this purpose.
However, since we need to live with what we have, we just used the init.rc to disable
u1/u2. Definitely, it's a ugly hack in userspace to make it "automatically" work.

A better solution is to use monitor uevent, reading bcdDevice + PID/VID, and define
a rule in to disable u1/u2 of this device. Android provides an uevent machanism to do this.

However, how to do it automatically, it's out of the scope of the patch.
Without the patch, the only choice is to add a quirk in usb core to do it automatically. And
this should be in another separate patch.
With the patch:
	1. Userspace can also do the quirk with the help of uevent and rules
	2. Developer can isolate u1 u2 debugging.

And I don't think it's necessary for kernel to support this broken modem. Because, the modem
is integrated with the SoC, and SoC goes with init.rc to OEMs. Thus, it doesn't make sense we
add a quirk in kernel to long term support it. The SoC/Modem is going to be replaced by its next
generation, especially in mobile area.

> > If somehow finally we decide to ship stepping C (suppose HW issue can't be fixed in D in time),
> > we'll have to disable both u1 and u2.
> > 
> > If we fix only u1 issue, we can just disable u2, etc..
> > 
> > To summarize, it provide users an opptunity to change the u1 u2 policy to
> > debug and ship broken products.
> 
> That's good, but you need to do it somehow "automatically", as again,
> systems don't have a init.rc file anymore :)
> 

Thanks
Jincan

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Zhuang Jin Can]

On Wed, Apr 29, 2015 at 01:06:22PM +0200, Greg KH wrote:
> On Wed, Apr 29, 2015 at 06:57:30PM +0800, Zhuang Jin Can wrote:
> > On Wed, Apr 29, 2015 at 11:01:48AM +0200, Greg KH wrote:
> > > On Wed, Apr 29, 2015 at 03:20:04PM +0800, Zhuang Jin Can wrote:
> > > > On Tue, Apr 28, 2015 at 11:11:10PM +0200, Greg KH wrote:
> > > > > On Wed, Apr 29, 2015 at 12:51:27AM +0800, Zhuang Jin Can wrote:
> > > > > > Hi Greg KH,
> > > > > > 
> > > > > > On Tue, Apr 28, 2015 at 12:42:24PM +0200, Greg KH wrote:
> > > > > > > On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> > > > > > > > Some usb3 devices may not support usb3 lpm well.
> > > > > > > > The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> > > > > > > > settings apply to both before and after device enumeration.
> > > > > > > > Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> > > > > > > > enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> > > > > > > > 
> > > > > > > > The interface is useful for testing some USB3 devices during
> > > > > > > > development, and provides a way to disable usb3 lpm if the issues can
> > > > > > > > not be fixed in final products.
> > > > > > > 
> > > > > > > How is a user supposed to "know" to make this setting for a device?  Why
> > > > > > > can't the kernel automatically set this value properly?  Why does it
> > > > > > > need to be a kernel issue at all?
> > > > > > > 
> > > > > > By default kernel enables u1 u2 of all USB3 devices. This interface
> > > > > > provides the user to change this policy. User may set the policy
> > > > > > according to PID/VID of uevent or according to the platform information
> > > > > > known by userspace.
> > > > > 
> > > > > And why would they ever want to do that?
> > > > > 
> > > > > > It's not a kernel issue, as u1 u2 is mandatory by USB3 compliance. But
> > > > > > for some internal hardwired USB3 connection, e.g. SSIC, passing USB3
> > > > > > compliance is not mandatory. So the interface provides a way for vendor
> > > > > > to ship with u1 or u2 broken products. Of course, this is not encouraged :).
> > > > > 
> > > > > If the state is broken for those devices, we can't require the user to
> > > > > fix it for us, the kernel should do it automatically.
> > > > > 
> > > > > > > And when you are doing development of broken devices, the kernel doesn't
> > > > > > > have to support you, you can run with debugging patches of your own
> > > > > > > until you fix your firmware :)
> > > > > > > 
> > > > > > Understood. But I think other vendor or developer may face the same
> > > > > > issue in final product shipment or during development. Moreover, the
> > > > > > interface provide the flexibility for developer to separately
> > > > > > disable/enable u1 or u2, e.g. If they're debugging an u2 issue, they
> > > > > > can disable u1 to simplify the situtation.
> > > > > 
> > > > > For debugging only, perhaps, but for a "normal" user, please let's
> > > > > handle this automatically and don't create a switch that never gets used
> > > > > by anyone or anything.
> > > > > 
> > > > Thanks Greg. Since so far the patch has no interesting value to the
> > > > community, I'll drop the patch.
> > > 
> > > I didn't say that, I said it needed some more work to be accepted.
> > Sorry for misunderstanding. Let me explain more why we need this interface.
> > 
> > We have a modem USB3 device (in stepping C) hardwired to one specific port of xHCI.
> > The device was expected to work with u1 u2, however, due to a HW issue, it doesn't
> > work stably. To workaround the issue, we let the init.rc script disable u1 u2 for
> > this specific port.
> 
> Modern Linux systems don't have init.rc scripts anymore :)
> 
In Android, the init process still reads an init.rc where vendor can
define their own policies. Vendors normally provides a whole reference
design (including HW, FW, Kernel, BSP, AOSP) to OEMs. BSP contains
vendor specific configurations including its own init.rc.

> > Then maybe we want to start debug u1 issue first, to avoid hitting u2 issue,
> > we can disable u2. After u1 issue is resolved, we can enable back u2 to continue to
> > debug u2 issue. This provides the flexibility to isolate u1 u2 debugging.
> > This is valuable I think :)
> 
> I agree.
> 
> > The HW issue will be fixed in stepping D, however C and D will have the same PID/VID.
> > There's no way for kernel to know the difference between C and D.
> > Even after fixing in D, C will still be used for development (to save money..)
> 
> That sounds like a big design flaw, what about looking at the version of
> the device?  That's what that field in the USB descriptors is for.
> 
You're right. bcdDevice should be used for this purpose.
However, since we need to live with what we have, we just used the init.rc to disable
u1/u2. Definitely, it's a ugly hack in userspace to make it "automatically" work.

A better solution is to use monitor uevent, reading bcdDevice + PID/VID, and define
a rule in to disable u1/u2 of this device. Android provides an uevent machanism to do this.

However, how to do it automatically, it's out of the scope of the patch.
Without the patch, the only choice is to add a quirk in usb core to do it automatically. And
this should be in another separate patch.
With the patch:
	1. Userspace can also do the quirk with the help of uevent and rules
	2. Developer can isolate u1 u2 debugging.

And I don't think it's necessary for kernel to support this broken modem. Because, the modem
is integrated with the SoC, and SoC goes with init.rc to OEMs. Thus, it doesn't make sense we
add a quirk in kernel to long term support it. The SoC/Modem is going to be replaced by its next
generation, especially in mobile area.

> > If somehow finally we decide to ship stepping C (suppose HW issue can't be fixed in D in time),
> > we'll have to disable both u1 and u2.
> > 
> > If we fix only u1 issue, we can just disable u2, etc..
> > 
> > To summarize, it provide users an opptunity to change the u1 u2 policy to
> > debug and ship broken products.
> 
> That's good, but you need to do it somehow "automatically", as again,
> systems don't have a init.rc file anymore :)
> 

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

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Greg KH]

On Thu, Apr 30, 2015 at 12:21:32AM +0800, Zhuang Jin Can wrote:
> On Wed, Apr 29, 2015 at 01:06:22PM +0200, Greg KH wrote:
> > On Wed, Apr 29, 2015 at 06:57:30PM +0800, Zhuang Jin Can wrote:
> > > On Wed, Apr 29, 2015 at 11:01:48AM +0200, Greg KH wrote:
> > > > On Wed, Apr 29, 2015 at 03:20:04PM +0800, Zhuang Jin Can wrote:
> > > > > On Tue, Apr 28, 2015 at 11:11:10PM +0200, Greg KH wrote:
> > > > > > On Wed, Apr 29, 2015 at 12:51:27AM +0800, Zhuang Jin Can wrote:
> > > > > > > Hi Greg KH,
> > > > > > > 
> > > > > > > On Tue, Apr 28, 2015 at 12:42:24PM +0200, Greg KH wrote:
> > > > > > > > On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> > > > > > > > > Some usb3 devices may not support usb3 lpm well.
> > > > > > > > > The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> > > > > > > > > settings apply to both before and after device enumeration.
> > > > > > > > > Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> > > > > > > > > enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> > > > > > > > > 
> > > > > > > > > The interface is useful for testing some USB3 devices during
> > > > > > > > > development, and provides a way to disable usb3 lpm if the issues can
> > > > > > > > > not be fixed in final products.
> > > > > > > > 
> > > > > > > > How is a user supposed to "know" to make this setting for a device?  Why
> > > > > > > > can't the kernel automatically set this value properly?  Why does it
> > > > > > > > need to be a kernel issue at all?
> > > > > > > > 
> > > > > > > By default kernel enables u1 u2 of all USB3 devices. This interface
> > > > > > > provides the user to change this policy. User may set the policy
> > > > > > > according to PID/VID of uevent or according to the platform information
> > > > > > > known by userspace.
> > > > > > 
> > > > > > And why would they ever want to do that?
> > > > > > 
> > > > > > > It's not a kernel issue, as u1 u2 is mandatory by USB3 compliance. But
> > > > > > > for some internal hardwired USB3 connection, e.g. SSIC, passing USB3
> > > > > > > compliance is not mandatory. So the interface provides a way for vendor
> > > > > > > to ship with u1 or u2 broken products. Of course, this is not encouraged :).
> > > > > > 
> > > > > > If the state is broken for those devices, we can't require the user to
> > > > > > fix it for us, the kernel should do it automatically.
> > > > > > 
> > > > > > > > And when you are doing development of broken devices, the kernel doesn't
> > > > > > > > have to support you, you can run with debugging patches of your own
> > > > > > > > until you fix your firmware :)
> > > > > > > > 
> > > > > > > Understood. But I think other vendor or developer may face the same
> > > > > > > issue in final product shipment or during development. Moreover, the
> > > > > > > interface provide the flexibility for developer to separately
> > > > > > > disable/enable u1 or u2, e.g. If they're debugging an u2 issue, they
> > > > > > > can disable u1 to simplify the situtation.
> > > > > > 
> > > > > > For debugging only, perhaps, but for a "normal" user, please let's
> > > > > > handle this automatically and don't create a switch that never gets used
> > > > > > by anyone or anything.
> > > > > > 
> > > > > Thanks Greg. Since so far the patch has no interesting value to the
> > > > > community, I'll drop the patch.
> > > > 
> > > > I didn't say that, I said it needed some more work to be accepted.
> > > Sorry for misunderstanding. Let me explain more why we need this interface.
> > > 
> > > We have a modem USB3 device (in stepping C) hardwired to one specific port of xHCI.
> > > The device was expected to work with u1 u2, however, due to a HW issue, it doesn't
> > > work stably. To workaround the issue, we let the init.rc script disable u1 u2 for
> > > this specific port.
> > 
> > Modern Linux systems don't have init.rc scripts anymore :)
> > 
> In Android, the init process still reads an init.rc where vendor can
> define their own policies. Vendors normally provides a whole reference
> design (including HW, FW, Kernel, BSP, AOSP) to OEMs. BSP contains
> vendor specific configurations including its own init.rc.

And that's generally not a good idea for companies to do, as they
shouldn't need special hardware workarounds in an init script, but I
understand :(

You are also going to be giving them a kernel patch that is not accepted
upstream which is really NOT the way to do things, and something that
many of us are working quite hard to keep from happening.

> > > Then maybe we want to start debug u1 issue first, to avoid hitting u2 issue,
> > > we can disable u2. After u1 issue is resolved, we can enable back u2 to continue to
> > > debug u2 issue. This provides the flexibility to isolate u1 u2 debugging.
> > > This is valuable I think :)
> > 
> > I agree.
> > 
> > > The HW issue will be fixed in stepping D, however C and D will have the same PID/VID.
> > > There's no way for kernel to know the difference between C and D.
> > > Even after fixing in D, C will still be used for development (to save money..)
> > 
> > That sounds like a big design flaw, what about looking at the version of
> > the device?  That's what that field in the USB descriptors is for.
> > 
> You're right. bcdDevice should be used for this purpose.

Why can't it?

> However, since we need to live with what we have, we just used the init.rc to disable
> u1/u2. Definitely, it's a ugly hack in userspace to make it "automatically" work.

Why can't you put a quirk in the kernel for that bcdDevice value and
then not need any userspace hacks?

> A better solution is to use monitor uevent, reading bcdDevice + PID/VID, and define
> a rule in to disable u1/u2 of this device. Android provides an uevent machanism to do this.

It's a horrid uevent mechanism in that it duplicates what udev did years
ago :(

> However, how to do it automatically, it's out of the scope of the patch.

Not at all, what if you don't want to run Android on your hardware?  You
still want it to work, so get the kernel fix upstream properly.

> Without the patch, the only choice is to add a quirk in usb core to do it automatically. And
> this should be in another separate patch.

Please send that patch.

> With the patch:
> 	1. Userspace can also do the quirk with the help of uevent and rules

But it has no idea how or when to do that.  Please don't provide hooks
that no one knows how to use.

> 	2. Developer can isolate u1 u2 debugging.

That only developer seems to be you, and you've already debugged this :)

> And I don't think it's necessary for kernel to support this broken modem. Because, the modem
> is integrated with the SoC, and SoC goes with init.rc to OEMs. Thus, it doesn't make sense we
> add a quirk in kernel to long term support it. The SoC/Modem is going to be replaced by its next
> generation, especially in mobile area.

Again, someone wants to run a mainline kernel.org release on that
hardware, like they should.  Some companies even are pushing to require
OEMs to have all of their changes upstream before they will buy their
chip, so please, make this a quirk and have it "just work" properly.  No
need to rely on a magic init.rc value that no one notices or
understands.

thanks,

greg k-h

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Greg KH]

On Thu, Apr 30, 2015 at 12:21:32AM +0800, Zhuang Jin Can wrote:
> On Wed, Apr 29, 2015 at 01:06:22PM +0200, Greg KH wrote:
> > On Wed, Apr 29, 2015 at 06:57:30PM +0800, Zhuang Jin Can wrote:
> > > On Wed, Apr 29, 2015 at 11:01:48AM +0200, Greg KH wrote:
> > > > On Wed, Apr 29, 2015 at 03:20:04PM +0800, Zhuang Jin Can wrote:
> > > > > On Tue, Apr 28, 2015 at 11:11:10PM +0200, Greg KH wrote:
> > > > > > On Wed, Apr 29, 2015 at 12:51:27AM +0800, Zhuang Jin Can wrote:
> > > > > > > Hi Greg KH,
> > > > > > > 
> > > > > > > On Tue, Apr 28, 2015 at 12:42:24PM +0200, Greg KH wrote:
> > > > > > > > On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> > > > > > > > > Some usb3 devices may not support usb3 lpm well.
> > > > > > > > > The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> > > > > > > > > settings apply to both before and after device enumeration.
> > > > > > > > > Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> > > > > > > > > enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> > > > > > > > > 
> > > > > > > > > The interface is useful for testing some USB3 devices during
> > > > > > > > > development, and provides a way to disable usb3 lpm if the issues can
> > > > > > > > > not be fixed in final products.
> > > > > > > > 
> > > > > > > > How is a user supposed to "know" to make this setting for a device?  Why
> > > > > > > > can't the kernel automatically set this value properly?  Why does it
> > > > > > > > need to be a kernel issue at all?
> > > > > > > > 
> > > > > > > By default kernel enables u1 u2 of all USB3 devices. This interface
> > > > > > > provides the user to change this policy. User may set the policy
> > > > > > > according to PID/VID of uevent or according to the platform information
> > > > > > > known by userspace.
> > > > > > 
> > > > > > And why would they ever want to do that?
> > > > > > 
> > > > > > > It's not a kernel issue, as u1 u2 is mandatory by USB3 compliance. But
> > > > > > > for some internal hardwired USB3 connection, e.g. SSIC, passing USB3
> > > > > > > compliance is not mandatory. So the interface provides a way for vendor
> > > > > > > to ship with u1 or u2 broken products. Of course, this is not encouraged :).
> > > > > > 
> > > > > > If the state is broken for those devices, we can't require the user to
> > > > > > fix it for us, the kernel should do it automatically.
> > > > > > 
> > > > > > > > And when you are doing development of broken devices, the kernel doesn't
> > > > > > > > have to support you, you can run with debugging patches of your own
> > > > > > > > until you fix your firmware :)
> > > > > > > > 
> > > > > > > Understood. But I think other vendor or developer may face the same
> > > > > > > issue in final product shipment or during development. Moreover, the
> > > > > > > interface provide the flexibility for developer to separately
> > > > > > > disable/enable u1 or u2, e.g. If they're debugging an u2 issue, they
> > > > > > > can disable u1 to simplify the situtation.
> > > > > > 
> > > > > > For debugging only, perhaps, but for a "normal" user, please let's
> > > > > > handle this automatically and don't create a switch that never gets used
> > > > > > by anyone or anything.
> > > > > > 
> > > > > Thanks Greg. Since so far the patch has no interesting value to the
> > > > > community, I'll drop the patch.
> > > > 
> > > > I didn't say that, I said it needed some more work to be accepted.
> > > Sorry for misunderstanding. Let me explain more why we need this interface.
> > > 
> > > We have a modem USB3 device (in stepping C) hardwired to one specific port of xHCI.
> > > The device was expected to work with u1 u2, however, due to a HW issue, it doesn't
> > > work stably. To workaround the issue, we let the init.rc script disable u1 u2 for
> > > this specific port.
> > 
> > Modern Linux systems don't have init.rc scripts anymore :)
> > 
> In Android, the init process still reads an init.rc where vendor can
> define their own policies. Vendors normally provides a whole reference
> design (including HW, FW, Kernel, BSP, AOSP) to OEMs. BSP contains
> vendor specific configurations including its own init.rc.

And that's generally not a good idea for companies to do, as they
shouldn't need special hardware workarounds in an init script, but I
understand :(

You are also going to be giving them a kernel patch that is not accepted
upstream which is really NOT the way to do things, and something that
many of us are working quite hard to keep from happening.

> > > Then maybe we want to start debug u1 issue first, to avoid hitting u2 issue,
> > > we can disable u2. After u1 issue is resolved, we can enable back u2 to continue to
> > > debug u2 issue. This provides the flexibility to isolate u1 u2 debugging.
> > > This is valuable I think :)
> > 
> > I agree.
> > 
> > > The HW issue will be fixed in stepping D, however C and D will have the same PID/VID.
> > > There's no way for kernel to know the difference between C and D.
> > > Even after fixing in D, C will still be used for development (to save money..)
> > 
> > That sounds like a big design flaw, what about looking at the version of
> > the device?  That's what that field in the USB descriptors is for.
> > 
> You're right. bcdDevice should be used for this purpose.

Why can't it?

> However, since we need to live with what we have, we just used the init.rc to disable
> u1/u2. Definitely, it's a ugly hack in userspace to make it "automatically" work.

Why can't you put a quirk in the kernel for that bcdDevice value and
then not need any userspace hacks?

> A better solution is to use monitor uevent, reading bcdDevice + PID/VID, and define
> a rule in to disable u1/u2 of this device. Android provides an uevent machanism to do this.

It's a horrid uevent mechanism in that it duplicates what udev did years
ago :(

> However, how to do it automatically, it's out of the scope of the patch.

Not at all, what if you don't want to run Android on your hardware?  You
still want it to work, so get the kernel fix upstream properly.

> Without the patch, the only choice is to add a quirk in usb core to do it automatically. And
> this should be in another separate patch.

Please send that patch.

> With the patch:
> 	1. Userspace can also do the quirk with the help of uevent and rules

But it has no idea how or when to do that.  Please don't provide hooks
that no one knows how to use.

> 	2. Developer can isolate u1 u2 debugging.

That only developer seems to be you, and you've already debugged this :)

> And I don't think it's necessary for kernel to support this broken modem. Because, the modem
> is integrated with the SoC, and SoC goes with init.rc to OEMs. Thus, it doesn't make sense we
> add a quirk in kernel to long term support it. The SoC/Modem is going to be replaced by its next
> generation, especially in mobile area.

Again, someone wants to run a mainline kernel.org release on that
hardware, like they should.  Some companies even are pushing to require
OEMs to have all of their changes upstream before they will buy their
chip, so please, make this a quirk and have it "just work" properly.  No
need to rely on a magic init.rc value that no one notices or
understands.

thanks,

greg k-h

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Zhuang Jin Can]

On Wed, Apr 29, 2015 at 09:57:33PM +0200, Greg KH wrote:
> On Thu, Apr 30, 2015 at 12:21:32AM +0800, Zhuang Jin Can wrote:
> > On Wed, Apr 29, 2015 at 01:06:22PM +0200, Greg KH wrote:
> > > On Wed, Apr 29, 2015 at 06:57:30PM +0800, Zhuang Jin Can wrote:
> > > > On Wed, Apr 29, 2015 at 11:01:48AM +0200, Greg KH wrote:
> > > > > On Wed, Apr 29, 2015 at 03:20:04PM +0800, Zhuang Jin Can wrote:
> > > > > > On Tue, Apr 28, 2015 at 11:11:10PM +0200, Greg KH wrote:
> > > > > > > On Wed, Apr 29, 2015 at 12:51:27AM +0800, Zhuang Jin Can wrote:
> > > > > > > > Hi Greg KH,
> > > > > > > > 
> > > > > > > > On Tue, Apr 28, 2015 at 12:42:24PM +0200, Greg KH wrote:
> > > > > > > > > On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> > > > > > > > > > Some usb3 devices may not support usb3 lpm well.
> > > > > > > > > > The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> > > > > > > > > > settings apply to both before and after device enumeration.
> > > > > > > > > > Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> > > > > > > > > > enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> > > > > > > > > > 
> > > > > > > > > > The interface is useful for testing some USB3 devices during
> > > > > > > > > > development, and provides a way to disable usb3 lpm if the issues can
> > > > > > > > > > not be fixed in final products.
> > > > > > > > > 
> > > > > > > > > How is a user supposed to "know" to make this setting for a device?  Why
> > > > > > > > > can't the kernel automatically set this value properly?  Why does it
> > > > > > > > > need to be a kernel issue at all?
> > > > > > > > > 
> > > > > > > > By default kernel enables u1 u2 of all USB3 devices. This interface
> > > > > > > > provides the user to change this policy. User may set the policy
> > > > > > > > according to PID/VID of uevent or according to the platform information
> > > > > > > > known by userspace.
> > > > > > > 
> > > > > > > And why would they ever want to do that?
> > > > > > > 
> > > > > > > > It's not a kernel issue, as u1 u2 is mandatory by USB3 compliance. But
> > > > > > > > for some internal hardwired USB3 connection, e.g. SSIC, passing USB3
> > > > > > > > compliance is not mandatory. So the interface provides a way for vendor
> > > > > > > > to ship with u1 or u2 broken products. Of course, this is not encouraged :).
> > > > > > > 
> > > > > > > If the state is broken for those devices, we can't require the user to
> > > > > > > fix it for us, the kernel should do it automatically.
> > > > > > > 
> > > > > > > > > And when you are doing development of broken devices, the kernel doesn't
> > > > > > > > > have to support you, you can run with debugging patches of your own
> > > > > > > > > until you fix your firmware :)
> > > > > > > > > 
> > > > > > > > Understood. But I think other vendor or developer may face the same
> > > > > > > > issue in final product shipment or during development. Moreover, the
> > > > > > > > interface provide the flexibility for developer to separately
> > > > > > > > disable/enable u1 or u2, e.g. If they're debugging an u2 issue, they
> > > > > > > > can disable u1 to simplify the situtation.
> > > > > > > 
> > > > > > > For debugging only, perhaps, but for a "normal" user, please let's
> > > > > > > handle this automatically and don't create a switch that never gets used
> > > > > > > by anyone or anything.
> > > > > > > 
> > > > > > Thanks Greg. Since so far the patch has no interesting value to the
> > > > > > community, I'll drop the patch.
> > > > > 
> > > > > I didn't say that, I said it needed some more work to be accepted.
> > > > Sorry for misunderstanding. Let me explain more why we need this interface.
> > > > 
> > > > We have a modem USB3 device (in stepping C) hardwired to one specific port of xHCI.
> > > > The device was expected to work with u1 u2, however, due to a HW issue, it doesn't
> > > > work stably. To workaround the issue, we let the init.rc script disable u1 u2 for
> > > > this specific port.
> > > 
> > > Modern Linux systems don't have init.rc scripts anymore :)
> > > 
> > In Android, the init process still reads an init.rc where vendor can
> > define their own policies. Vendors normally provides a whole reference
> > design (including HW, FW, Kernel, BSP, AOSP) to OEMs. BSP contains
> > vendor specific configurations including its own init.rc.
> 
> And that's generally not a good idea for companies to do, as they
> shouldn't need special hardware workarounds in an init script, but I
> understand :(
> 
> You are also going to be giving them a kernel patch that is not accepted
> upstream which is really NOT the way to do things, and something that
> many of us are working quite hard to keep from happening.
> 
> > > > Then maybe we want to start debug u1 issue first, to avoid hitting u2 issue,
> > > > we can disable u2. After u1 issue is resolved, we can enable back u2 to continue to
> > > > debug u2 issue. This provides the flexibility to isolate u1 u2 debugging.
> > > > This is valuable I think :)
> > > 
> > > I agree.
> > > 
> > > > The HW issue will be fixed in stepping D, however C and D will have the same PID/VID.
> > > > There's no way for kernel to know the difference between C and D.
> > > > Even after fixing in D, C will still be used for development (to save money..)
> > > 
> > > That sounds like a big design flaw, what about looking at the version of
> > > the device?  That's what that field in the USB descriptors is for.
> > > 
> > You're right. bcdDevice should be used for this purpose.
> 
> Why can't it?
> 
> > However, since we need to live with what we have, we just used the init.rc to disable
> > u1/u2. Definitely, it's a ugly hack in userspace to make it "automatically" work.
> 
> Why can't you put a quirk in the kernel for that bcdDevice value and
> then not need any userspace hacks?
> 
Let me double confirm with our modem HW engineers to see if anything we
can differentiate the modem variants, and add the quirks in kernel
accordingly.

> > A better solution is to use monitor uevent, reading bcdDevice + PID/VID, and define
> > a rule in to disable u1/u2 of this device. Android provides an uevent machanism to do this.
> 
> It's a horrid uevent mechanism in that it duplicates what udev did years
> ago :(
> 
OK. I think I got your point now: kernel should handle broken devices automatically without
userspace's attention.


> > However, how to do it automatically, it's out of the scope of the patch.
> 
> Not at all, what if you don't want to run Android on your hardware?  You
> still want it to work, so get the kernel fix upstream properly.
> 
Got it.

> > Without the patch, the only choice is to add a quirk in usb core to do it automatically. And
> > this should be in another separate patch.
> 
> Please send that patch.
> 
Sure. Will need some time before sending the patch.

> > With the patch:
> > 	1. Userspace can also do the quirk with the help of uevent and rules
> 
> But it has no idea how or when to do that.  Please don't provide hooks
> that no one knows how to use.
> 
OK.

> > 	2. Developer can isolate u1 u2 debugging.
> 
> That only developer seems to be you, and you've already debugged this :)
> 
Do you think the interface has no much value to other developers, and I should
remove it?

> > And I don't think it's necessary for kernel to support this broken modem. Because, the modem
> > is integrated with the SoC, and SoC goes with init.rc to OEMs. Thus, it doesn't make sense we
> > add a quirk in kernel to long term support it. The SoC/Modem is going to be replaced by its next
> > generation, especially in mobile area.
> 
> Again, someone wants to run a mainline kernel.org release on that
> hardware, like they should.  Some companies even are pushing to require
> OEMs to have all of their changes upstream before they will buy their
> chip, so please, make this a quirk and have it "just work" properly.  No
> need to rely on a magic init.rc value that no one notices or
> understands.
> 
Got it. Thanks your explainations Greg.

Regards
Jincan

Re: [PATCH] usb: core: add usb3 lpm sysfs

[Zhuang Jin Can]

On Wed, Apr 29, 2015 at 09:57:33PM +0200, Greg KH wrote:
> On Thu, Apr 30, 2015 at 12:21:32AM +0800, Zhuang Jin Can wrote:
> > On Wed, Apr 29, 2015 at 01:06:22PM +0200, Greg KH wrote:
> > > On Wed, Apr 29, 2015 at 06:57:30PM +0800, Zhuang Jin Can wrote:
> > > > On Wed, Apr 29, 2015 at 11:01:48AM +0200, Greg KH wrote:
> > > > > On Wed, Apr 29, 2015 at 03:20:04PM +0800, Zhuang Jin Can wrote:
> > > > > > On Tue, Apr 28, 2015 at 11:11:10PM +0200, Greg KH wrote:
> > > > > > > On Wed, Apr 29, 2015 at 12:51:27AM +0800, Zhuang Jin Can wrote:
> > > > > > > > Hi Greg KH,
> > > > > > > > 
> > > > > > > > On Tue, Apr 28, 2015 at 12:42:24PM +0200, Greg KH wrote:
> > > > > > > > > On Sun, Apr 19, 2015 at 11:46:12AM +0800, Zhuang Jin Can wrote:
> > > > > > > > > > Some usb3 devices may not support usb3 lpm well.
> > > > > > > > > > The patch adds a sysfs to enable/disable u1 or u2 of the port.The
> > > > > > > > > > settings apply to both before and after device enumeration.
> > > > > > > > > > Supported values are "0" - u1 and u2 are disabled, "u1" - only u1 is
> > > > > > > > > > enabled, "u2" - only u2 is enabled, "u1_u2" - u1 and u2 are enabled.
> > > > > > > > > > 
> > > > > > > > > > The interface is useful for testing some USB3 devices during
> > > > > > > > > > development, and provides a way to disable usb3 lpm if the issues can
> > > > > > > > > > not be fixed in final products.
> > > > > > > > > 
> > > > > > > > > How is a user supposed to "know" to make this setting for a device?  Why
> > > > > > > > > can't the kernel automatically set this value properly?  Why does it
> > > > > > > > > need to be a kernel issue at all?
> > > > > > > > > 
> > > > > > > > By default kernel enables u1 u2 of all USB3 devices. This interface
> > > > > > > > provides the user to change this policy. User may set the policy
> > > > > > > > according to PID/VID of uevent or according to the platform information
> > > > > > > > known by userspace.
> > > > > > > 
> > > > > > > And why would they ever want to do that?
> > > > > > > 
> > > > > > > > It's not a kernel issue, as u1 u2 is mandatory by USB3 compliance. But
> > > > > > > > for some internal hardwired USB3 connection, e.g. SSIC, passing USB3
> > > > > > > > compliance is not mandatory. So the interface provides a way for vendor
> > > > > > > > to ship with u1 or u2 broken products. Of course, this is not encouraged :).
> > > > > > > 
> > > > > > > If the state is broken for those devices, we can't require the user to
> > > > > > > fix it for us, the kernel should do it automatically.
> > > > > > > 
> > > > > > > > > And when you are doing development of broken devices, the kernel doesn't
> > > > > > > > > have to support you, you can run with debugging patches of your own
> > > > > > > > > until you fix your firmware :)
> > > > > > > > > 
> > > > > > > > Understood. But I think other vendor or developer may face the same
> > > > > > > > issue in final product shipment or during development. Moreover, the
> > > > > > > > interface provide the flexibility for developer to separately
> > > > > > > > disable/enable u1 or u2, e.g. If they're debugging an u2 issue, they
> > > > > > > > can disable u1 to simplify the situtation.
> > > > > > > 
> > > > > > > For debugging only, perhaps, but for a "normal" user, please let's
> > > > > > > handle this automatically and don't create a switch that never gets used
> > > > > > > by anyone or anything.
> > > > > > > 
> > > > > > Thanks Greg. Since so far the patch has no interesting value to the
> > > > > > community, I'll drop the patch.
> > > > > 
> > > > > I didn't say that, I said it needed some more work to be accepted.
> > > > Sorry for misunderstanding. Let me explain more why we need this interface.
> > > > 
> > > > We have a modem USB3 device (in stepping C) hardwired to one specific port of xHCI.
> > > > The device was expected to work with u1 u2, however, due to a HW issue, it doesn't
> > > > work stably. To workaround the issue, we let the init.rc script disable u1 u2 for
> > > > this specific port.
> > > 
> > > Modern Linux systems don't have init.rc scripts anymore :)
> > > 
> > In Android, the init process still reads an init.rc where vendor can
> > define their own policies. Vendors normally provides a whole reference
> > design (including HW, FW, Kernel, BSP, AOSP) to OEMs. BSP contains
> > vendor specific configurations including its own init.rc.
> 
> And that's generally not a good idea for companies to do, as they
> shouldn't need special hardware workarounds in an init script, but I
> understand :(
> 
> You are also going to be giving them a kernel patch that is not accepted
> upstream which is really NOT the way to do things, and something that
> many of us are working quite hard to keep from happening.
> 
> > > > Then maybe we want to start debug u1 issue first, to avoid hitting u2 issue,
> > > > we can disable u2. After u1 issue is resolved, we can enable back u2 to continue to
> > > > debug u2 issue. This provides the flexibility to isolate u1 u2 debugging.
> > > > This is valuable I think :)
> > > 
> > > I agree.
> > > 
> > > > The HW issue will be fixed in stepping D, however C and D will have the same PID/VID.
> > > > There's no way for kernel to know the difference between C and D.
> > > > Even after fixing in D, C will still be used for development (to save money..)
> > > 
> > > That sounds like a big design flaw, what about looking at the version of
> > > the device?  That's what that field in the USB descriptors is for.
> > > 
> > You're right. bcdDevice should be used for this purpose.
> 
> Why can't it?
> 
> > However, since we need to live with what we have, we just used the init.rc to disable
> > u1/u2. Definitely, it's a ugly hack in userspace to make it "automatically" work.
> 
> Why can't you put a quirk in the kernel for that bcdDevice value and
> then not need any userspace hacks?
> 
Let me double confirm with our modem HW engineers to see if anything we
can differentiate the modem variants, and add the quirks in kernel
accordingly.

> > A better solution is to use monitor uevent, reading bcdDevice + PID/VID, and define
> > a rule in to disable u1/u2 of this device. Android provides an uevent machanism to do this.
> 
> It's a horrid uevent mechanism in that it duplicates what udev did years
> ago :(
> 
OK. I think I got your point now: kernel should handle broken devices automatically without
userspace's attention.


> > However, how to do it automatically, it's out of the scope of the patch.
> 
> Not at all, what if you don't want to run Android on your hardware?  You
> still want it to work, so get the kernel fix upstream properly.
> 
Got it.

> > Without the patch, the only choice is to add a quirk in usb core to do it automatically. And
> > this should be in another separate patch.
> 
> Please send that patch.
> 
Sure. Will need some time before sending the patch.

> > With the patch:
> > 	1. Userspace can also do the quirk with the help of uevent and rules
> 
> But it has no idea how or when to do that.  Please don't provide hooks
> that no one knows how to use.
> 
OK.

> > 	2. Developer can isolate u1 u2 debugging.
> 
> That only developer seems to be you, and you've already debugged this :)
> 
Do you think the interface has no much value to other developers, and I should
remove it?

> > And I don't think it's necessary for kernel to support this broken modem. Because, the modem
> > is integrated with the SoC, and SoC goes with init.rc to OEMs. Thus, it doesn't make sense we
> > add a quirk in kernel to long term support it. The SoC/Modem is going to be replaced by its next
> > generation, especially in mobile area.
> 
> Again, someone wants to run a mainline kernel.org release on that
> hardware, like they should.  Some companies even are pushing to require
> OEMs to have all of their changes upstream before they will buy their
> chip, so please, make this a quirk and have it "just work" properly.  No
> need to rely on a magic init.rc value that no one notices or
> understands.
> 
Got it. Thanks your explainations Greg.

Regards
Jincan