Re: [PATCH] usb: core: introduce per-port over-current counters

2018-02-19 Thread Greg KH 
On Mon, Feb 19, 2018 at 05:05:34PM +0100, Richard Leitner wrote:
> 
> On 02/19/2018 04:59 PM, Greg KH wrote:
> > On Mon, Feb 19, 2018 at 01:01:07PM +0100, Richard Leitner wrote:
> >> From: Richard Leitner 
> >>
> >> For some userspace applications information on the number of
> >> over-current conditions at specific USB hub ports is relevant. Therefore
> >> introduce a oc_counter in the usb port struct which is exported via
> >> sysfs.
> >>
> >> Signed-off-by: Richard Leitner 
> >> ---
> >> Tested on an i.MX6DL based board.
> >> ---
> >>  drivers/usb/core/hub.c  |  4 +++-
> >>  drivers/usb/core/hub.h  |  1 +
> >>  drivers/usb/core/port.c | 10 ++
> >>  3 files changed, 14 insertions(+), 1 deletion(-)
> > 
> > When you add/remove/modify a sysfs attribute, you always have to
> > document in Documentation/ABI/
> 
> Ok. Thank you. Seems I missed that, Sorry!
> 
> >>
> >> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> >> index c5c1f6cf3228..448fba1e1827 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->oc_count++;
> >>  
> >> -  dev_dbg(_dev->dev, "over-current change\n");
> >> +  dev_dbg(_dev->dev, "over-current change #%u\n",
> >> +  port_dev->oc_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..b5cf567bf9e2 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 oc_count;
> >>  };
> >>  
> >>  #define to_usb_port(_dev) \
> >> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> >> index 1a01e9ad3804..0bfe410eb8a7 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 oc_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->oc_count);
> >> +}
> >> +static DEVICE_ATTR_RO(oc_count);
> > 
> > I don't see what userspace can do with this number, as there's not much
> > it can do with it.
> 
> I've answered this question to Felipe already:



It's not described in the patch changelog, which is where it is required
:)

thanks,

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

Re: [PATCH] usb: core: introduce per-port over-current counters

2018-02-19 Thread Richard Leitner 

On 02/19/2018 04:59 PM, Greg KH wrote:
> On Mon, Feb 19, 2018 at 01:01:07PM +0100, Richard Leitner wrote:
>> From: Richard Leitner 
>>
>> For some userspace applications information on the number of
>> over-current conditions at specific USB hub ports is relevant. Therefore
>> introduce a oc_counter in the usb port struct which is exported via
>> sysfs.
>>
>> Signed-off-by: Richard Leitner 
>> ---
>> Tested on an i.MX6DL based board.
>> ---
>>  drivers/usb/core/hub.c  |  4 +++-
>>  drivers/usb/core/hub.h  |  1 +
>>  drivers/usb/core/port.c | 10 ++
>>  3 files changed, 14 insertions(+), 1 deletion(-)
> 
> When you add/remove/modify a sysfs attribute, you always have to
> document in Documentation/ABI/

Ok. Thank you. Seems I missed that, Sorry!

>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index c5c1f6cf3228..448fba1e1827 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->oc_count++;
>>  
>> -dev_dbg(_dev->dev, "over-current change\n");
>> +dev_dbg(_dev->dev, "over-current change #%u\n",
>> +port_dev->oc_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..b5cf567bf9e2 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 oc_count;
>>  };
>>  
>>  #define to_usb_port(_dev) \
>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>> index 1a01e9ad3804..0bfe410eb8a7 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 oc_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->oc_count);
>> +}
>> +static DEVICE_ATTR_RO(oc_count);
> 
> I don't see what userspace can do with this number, as there's not much
> it can do with it.

I've answered this question to Felipe already:

On 02/19/2018 03:12 PM, Richard Leitner wrote:
> 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 use reduce the max power always.
> 
> With this patch I want to give the user-space application the possibility to
> react adequately to such over-current situations.
> 
> I hope this explains my motivation for this patch in a comprehensible way.

I'm of course always open for a better/cleaner/safer way of doing this ;-)

Thank you!

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

Re: [PATCH] usb: core: introduce per-port over-current counters

2018-02-19 Thread Greg KH 
On Mon, Feb 19, 2018 at 01:01:07PM +0100, Richard Leitner wrote:
> From: Richard Leitner 
> 
> For some userspace applications information on the number of
> over-current conditions at specific USB hub ports is relevant. Therefore
> introduce a oc_counter in the usb port struct which is exported via
> sysfs.
> 
> Signed-off-by: Richard Leitner 
> ---
> Tested on an i.MX6DL based board.
> ---
>  drivers/usb/core/hub.c  |  4 +++-
>  drivers/usb/core/hub.h  |  1 +
>  drivers/usb/core/port.c | 10 ++
>  3 files changed, 14 insertions(+), 1 deletion(-)

When you add/remove/modify a sysfs attribute, you always have to
document in Documentation/ABI/


> 
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index c5c1f6cf3228..448fba1e1827 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->oc_count++;
>  
> - dev_dbg(_dev->dev, "over-current change\n");
> + dev_dbg(_dev->dev, "over-current change #%u\n",
> + port_dev->oc_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..b5cf567bf9e2 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 oc_count;
>  };
>  
>  #define to_usb_port(_dev) \
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index 1a01e9ad3804..0bfe410eb8a7 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 oc_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->oc_count);
> +}
> +static DEVICE_ATTR_RO(oc_count);

I don't see what userspace can do with this number, as there's not much
it can do with it.

thanks,

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

Re: [PATCH] usb: core: introduce per-port over-current counters

2018-02-19 Thread Richard Leitner 
Hi,

On 02/19/2018 02:53 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Richard Leitner  writes:
> 
>> From: Richard Leitner 
>>
>> For some userspace applications information on the number of
>> over-current conditions at specific USB hub ports is relevant. Therefore
>> introduce a oc_counter in the usb port struct which is exported via
>> sysfs.
> 
> relevant how? What can the application do with that knowledge?

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 use reduce the max power always.

With this patch I want to give the user-space application the possibility to
react adequately to such over-current situations.

I hope this explains my motivation for this patch in a comprehensible way.

> 
>> Signed-off-by: Richard Leitner 
>> ---
>> Tested on an i.MX6DL based board.
>> ---
>>  drivers/usb/core/hub.c  |  4 +++-
>>  drivers/usb/core/hub.h  |  1 +
>>  drivers/usb/core/port.c | 10 ++
>>  3 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index c5c1f6cf3228..448fba1e1827 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->oc_count++;
>>  
>> -dev_dbg(_dev->dev, "over-current change\n");
>> +dev_dbg(_dev->dev, "over-current change #%u\n",
>> +port_dev->oc_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..b5cf567bf9e2 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 oc_count;
>>  };
>>  
>>  #define to_usb_port(_dev) \
>> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
>> index 1a01e9ad3804..0bfe410eb8a7 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 oc_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->oc_count);
>> +}
>> +static DEVICE_ATTR_RO(oc_count);
> 
> I would actually spell this out human readable: over_current_count
> 

Would be fine for me.

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

Re: [PATCH] usb: core: introduce per-port over-current counters

2018-02-19 Thread Felipe Balbi 

Hi,

Richard Leitner  writes:

> From: Richard Leitner 
>
> For some userspace applications information on the number of
> over-current conditions at specific USB hub ports is relevant. Therefore
> introduce a oc_counter in the usb port struct which is exported via
> sysfs.

relevant how? What can the application do with that knowledge?

> Signed-off-by: Richard Leitner 
> ---
> Tested on an i.MX6DL based board.
> ---
>  drivers/usb/core/hub.c  |  4 +++-
>  drivers/usb/core/hub.h  |  1 +
>  drivers/usb/core/port.c | 10 ++
>  3 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index c5c1f6cf3228..448fba1e1827 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->oc_count++;
>  
> - dev_dbg(_dev->dev, "over-current change\n");
> + dev_dbg(_dev->dev, "over-current change #%u\n",
> + port_dev->oc_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..b5cf567bf9e2 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 oc_count;
>  };
>  
>  #define to_usb_port(_dev) \
> diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
> index 1a01e9ad3804..0bfe410eb8a7 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 oc_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->oc_count);
> +}
> +static DEVICE_ATTR_RO(oc_count);

I would actually spell this out human readable: over_current_count

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

[PATCH] usb: core: introduce per-port over-current counters

2018-02-19 Thread Richard Leitner 
From: Richard Leitner 

For some userspace applications information on the number of
over-current conditions at specific USB hub ports is relevant. Therefore
introduce a oc_counter in the usb port struct which is exported via
sysfs.

Signed-off-by: Richard Leitner 
---
Tested on an i.MX6DL based board.
---
 drivers/usb/core/hub.c  |  4 +++-
 drivers/usb/core/hub.h  |  1 +
 drivers/usb/core/port.c | 10 ++
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index c5c1f6cf3228..448fba1e1827 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->oc_count++;
 
-   dev_dbg(_dev->dev, "over-current change\n");
+   dev_dbg(_dev->dev, "over-current change #%u\n",
+   port_dev->oc_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..b5cf567bf9e2 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 oc_count;
 };
 
 #define to_usb_port(_dev) \
diff --git a/drivers/usb/core/port.c b/drivers/usb/core/port.c
index 1a01e9ad3804..0bfe410eb8a7 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 oc_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->oc_count);
+}
+static DEVICE_ATTR_RO(oc_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[] = {
_attr_connect_type.attr,
+   _attr_oc_count.attr,
NULL,
 };
 
-- 
2.11.0

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