2018-02-20 11:51:52

by Richard Leitner

[permalink] [raw]
Subject: [PATCH v2] usb: core: introduce per-port over-current counters

From: Richard Leitner <[email protected]>

For some userspace applications information on the number of
over-current conditions at specific USB hub ports is relevant.

In our case we have a series of USB hardware (using the cp210x driver)
which communicates using a proprietary protocol. These devices sometimes
trigger an over-current situation on some hubs. In case of such an
over-current situation the USB devices offer an interface for reducing
the max used power. As these conditions are quite rare and imply
performance reductions of the device we don't want to reduce the max
power always.

Therefore give user-space applications the possibility to react
adequately by introducing an over_current_counter in the usb port struct
which is exported via sysfs.

Signed-off-by: Richard Leitner <[email protected]>
---
Tested on an i.MX6DL based board

Changes v2:
- rename oc_count to over_current_count
- add entry to Documentation/ABI
- add detailled description to commit message
---
Documentation/ABI/testing/sysfs-bus-usb | 9 +++++++++
drivers/usb/core/hub.c | 4 +++-
drivers/usb/core/hub.h | 1 +
drivers/usb/core/port.c | 10 ++++++++++
4 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
index 0bd731cbb50c..27020293c85b 100644
--- a/Documentation/ABI/testing/sysfs-bus-usb
+++ b/Documentation/ABI/testing/sysfs-bus-usb
@@ -189,6 +189,15 @@ Description:
The file will read "hotplug", "wired" and "not used" if the
information is available, and "unknown" otherwise.

+What: /sys/bus/usb/devices/.../(hub interface)/portX/over_current_count
+Date: February 2018
+Contact: Richard Leitner <[email protected]>
+Description:
+ Most hubs are able to detect over-current situations on their
+ ports and report them to the kernel. This attribute is to expose
+ the number of over-current situation occurred on a specific port
+ to user space. This file will contain an unsigned int.
+
What: /sys/bus/usb/devices/.../(hub interface)/portX/usb3_lpm_permit
Date: November 2015
Contact: Lu Baolu <[email protected]>
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c5c1f6cf3228..6f779b518e75 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -5104,8 +5104,10 @@ static void port_event(struct usb_hub *hub, int port1)

if (portchange & USB_PORT_STAT_C_OVERCURRENT) {
u16 status = 0, unused;
+ port_dev->over_current_count++;

- dev_dbg(&port_dev->dev, "over-current change\n");
+ dev_dbg(&port_dev->dev, "over-current change #%u\n",
+ port_dev->over_current_count);
usb_clear_port_feature(hdev, port1,
USB_PORT_FEAT_C_OVER_CURRENT);
msleep(100); /* Cool down */
diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
index 2a700ccc868c..78d7f4dad618 100644
--- a/drivers/usb/core/hub.h
+++ b/drivers/usb/core/hub.h
@@ -100,6 +100,7 @@ struct usb_port {
unsigned int is_superspeed:1;
unsigned int usb3_lpm_u1_permit:1;
unsigned int usb3_lpm_u2_permit:1;
+ unsigned int over_current_count;
};

#define to_usb_port(_dev) \
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 1a01e9ad3804..6979bde87d31 100644
--- a/drivers/usb/core/port.c
+++ b/drivers/usb/core/port.c
@@ -41,6 +41,15 @@ static ssize_t connect_type_show(struct device *dev,
}
static DEVICE_ATTR_RO(connect_type);

+static ssize_t over_current_count_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+ struct usb_port *port_dev = to_usb_port(dev);
+
+ return sprintf(buf, "%u\n", port_dev->over_current_count);
+}
+static DEVICE_ATTR_RO(over_current_count);
+
static ssize_t usb3_lpm_permit_show(struct device *dev,
struct device_attribute *attr, char *buf)
{
@@ -109,6 +118,7 @@ static DEVICE_ATTR_RW(usb3_lpm_permit);

static struct attribute *port_dev_attrs[] = {
&dev_attr_connect_type.attr,
+ &dev_attr_over_current_count.attr,
NULL,
};

--
2.11.0



2018-02-20 11:59:23

by Felipe Balbi

[permalink] [raw]
Subject: Re: [PATCH v2] usb: core: introduce per-port over-current counters


Hi,

Richard Leitner <[email protected]> writes:
> From: Richard Leitner <[email protected]>
>
> For some userspace applications information on the number of
> over-current conditions at specific USB hub ports is relevant.
>
> In our case we have a series of USB hardware (using the cp210x driver)
> which communicates using a proprietary protocol. These devices sometimes
> trigger an over-current situation on some hubs. In case of such an
> over-current situation the USB devices offer an interface for reducing
> the max used power. As these conditions are quite rare and imply
> performance reductions of the device we don't want to reduce the max
> power always.
>
> Therefore give user-space applications the possibility to react
> adequately by introducing an over_current_counter in the usb port struct
> which is exported via sysfs.

why don't you just provide more than one configuration with several
bMaxPower fields? Then host OS should choose correct configuration based
on power budget.

--
balbi

2018-02-20 12:25:21

by Richard Leitner

[permalink] [raw]
Subject: Re: [PATCH v2] usb: core: introduce per-port over-current counters

On 02/20/2018 12:55 PM, Felipe Balbi wrote:
>
> Hi,
>
> Richard Leitner <[email protected]> writes:
>> From: Richard Leitner <[email protected]>
>>
>> For some userspace applications information on the number of
>> over-current conditions at specific USB hub ports is relevant.
>>
>> In our case we have a series of USB hardware (using the cp210x driver)
>> which communicates using a proprietary protocol. These devices sometimes
>> trigger an over-current situation on some hubs. In case of such an
>> over-current situation the USB devices offer an interface for reducing
>> the max used power. As these conditions are quite rare and imply
>> performance reductions of the device we don't want to reduce the max
>> power always.
>>
>> Therefore give user-space applications the possibility to react
>> adequately by introducing an over_current_counter in the usb port struct
>> which is exported via sysfs.
>
> why don't you just provide more than one configuration with several
> bMaxPower fields? Then host OS should choose correct configuration based
> on power budget.

Thank you for that suggestion!
Generally speaking that would be possible. Nonetheless we
a) don't have the possibility to change the firmware or the USB
configuration of the device.
b) in those corner-cases (where the over-current situation occurs)
the device consumes more than 500mA (which couldn't be
configured in bMaxPower AFAIK?). Nonetheless most
hubs don't detect the over-current (AFAIK) due to
electrical tolerances of their components. Our problem are
the hubs which trigger the over-current situation (which
is fine from a USB perspective).

I know it's probably not a very nice solution, so if anybody has a better
proposal please let me know :-)

Thanks!

regards;Richard.L

2018-03-09 17:20:34

by Greg Kroah-Hartman

[permalink] [raw]
Subject: Re: [PATCH v2] usb: core: introduce per-port over-current counters

On Tue, Feb 20, 2018 at 12:50:33PM +0100, Richard Leitner wrote:
> From: Richard Leitner <[email protected]>
>
> For some userspace applications information on the number of
> over-current conditions at specific USB hub ports is relevant.
>
> In our case we have a series of USB hardware (using the cp210x driver)
> which communicates using a proprietary protocol. These devices sometimes
> trigger an over-current situation on some hubs. In case of such an
> over-current situation the USB devices offer an interface for reducing
> the max used power. As these conditions are quite rare and imply
> performance reductions of the device we don't want to reduce the max
> power always.
>
> Therefore give user-space applications the possibility to react
> adequately by introducing an over_current_counter in the usb port struct
> which is exported via sysfs.
>
> Signed-off-by: Richard Leitner <[email protected]>
> ---
> Tested on an i.MX6DL based board
>
> Changes v2:
> - rename oc_count to over_current_count
> - add entry to Documentation/ABI
> - add detailled description to commit message
> ---
> Documentation/ABI/testing/sysfs-bus-usb | 9 +++++++++
> drivers/usb/core/hub.c | 4 +++-
> drivers/usb/core/hub.h | 1 +
> drivers/usb/core/port.c | 10 ++++++++++
> 4 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
> index 0bd731cbb50c..27020293c85b 100644
> --- a/Documentation/ABI/testing/sysfs-bus-usb
> +++ b/Documentation/ABI/testing/sysfs-bus-usb
> @@ -189,6 +189,15 @@ Description:
> The file will read "hotplug", "wired" and "not used" if the
> information is available, and "unknown" otherwise.
>
> +What: /sys/bus/usb/devices/.../(hub interface)/portX/over_current_count
> +Date: February 2018
> +Contact: Richard Leitner <[email protected]>
> +Description:
> + Most hubs are able to detect over-current situations on their
> + ports and report them to the kernel. This attribute is to expose
> + the number of over-current situation occurred on a specific port
> + to user space. This file will contain an unsigned int.

"unsigned int" is very vague :)

How about "this is a 32bit value"?

And what happens when this wraps? Is that allowed?

thanks,

greg k-h

2018-03-13 10:32:52

by Richard Leitner

[permalink] [raw]
Subject: Re: [PATCH v2] usb: core: introduce per-port over-current counters

Hi Greg,

On 03/09/2018 06:19 PM, Greg KH wrote:
>> diff --git a/Documentation/ABI/testing/sysfs-bus-usb b/Documentation/ABI/testing/sysfs-bus-usb
>> index 0bd731cbb50c..27020293c85b 100644
>> --- a/Documentation/ABI/testing/sysfs-bus-usb
>> +++ b/Documentation/ABI/testing/sysfs-bus-usb
>> @@ -189,6 +189,15 @@ Description:
>> The file will read "hotplug", "wired" and "not used" if the
>> information is available, and "unknown" otherwise.
>>
>> +What: /sys/bus/usb/devices/.../(hub interface)/portX/over_current_count
>> +Date: February 2018
>> +Contact: Richard Leitner <[email protected]>
>> +Description:
>> + Most hubs are able to detect over-current situations on their
>> + ports and report them to the kernel. This attribute is to expose
>> + the number of over-current situation occurred on a specific port
>> + to user space. This file will contain an unsigned int.
> "unsigned int" is very vague :)
>
> How about "this is a 32bit value"?
>
> And what happens when this wraps? Is that allowed?

Thank you for your feedback. I'll improve the description for v3!

>
> thanks,
>
> greg k-h

regards;Richard.L