Re: [PATCH v6 5/8] usb: add usb port auto power off mechanism

2013-01-22 Thread Lan Tianyu 

On 2013/1/22 5:30, Greg KH wrote:

On Mon, Jan 21, 2013 at 10:18:04PM +0800, Lan Tianyu wrote:

This patch is to add usb port auto power off mechanism.
When usb device is suspending, usb core will suspend usb port and
usb port runtime pm callback will clear PORT_POWER feature to
power off port if all conditions were met. These conditions are
remote wakeup disable, pm qos NO_POWER_OFF flag clear and persist
enable. When it resumes, power on port again. Add did_runtime_put
in the struct usb_port in order to call pm_runtime_get/put(portdev)
paired during suspending and resuming.


I don't understand what did_runtime_put is supposed to mean.  Care to
explain this better?

Ok.




Acked-by: Alan Stern st...@rowland.harvard.edu
Acked-by: Rafael J. Wysocki rafael.j.wyso...@intel.com
Signed-off-by: Lan Tianyu tianyu@intel.com
---
  drivers/usb/core/hub.c  |   67 ---
  drivers/usb/core/hub.h  |9 +++
  drivers/usb/core/port.c |   40 ++--
  3 files changed, 105 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 8c1f9a5..786db99 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -26,6 +26,7 @@
  #include linux/mutex.h
  #include linux/freezer.h
  #include linux/random.h
+#include linux/pm_qos.h

  #include asm/uaccess.h
  #include asm/byteorder.h
@@ -108,7 +109,7 @@ MODULE_PARM_DESC(use_both_schemes,
  DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
  EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);

-#define HUB_DEBOUNCE_TIMEOUT   1500
+#define HUB_DEBOUNCE_TIMEOUT   2000


Why did you increase this timeout?  You don't mention that above in the
changelog entry.

I will add this into change log.



  #define HUB_DEBOUNCE_STEP   25
  #define HUB_DEBOUNCE_STABLE100

@@ -127,7 +128,7 @@ static inline char *portspeed(struct usb_hub *hub, int 
portstatus)
  }

  /* Note that hdev or one of its children must be locked! */
-static struct usb_hub *hdev_to_hub(struct usb_device *hdev)
+struct usb_hub *hdev_to_hub(struct usb_device *hdev)


This needs a better name now that this is a global function.  No one
knows what a hdev is.
Actually the routine will only be used in the driver/usb/core/(hub.c, 
port.c). Port also should be the part of hub, right? Moving port related 
code to port.c is to make code more clear. I am not sure whether we 
should rename it.


If we renamed it, how about usb_hub_to_struct_hub? \
Should we do rename operation in a separate patch since hdev_to_hub() is 
called many places in the hub.c?





  {
if (!hdev || !hdev-actconfig || !hdev-maxchild)
return NULL;
@@ -393,7 +394,7 @@ static int clear_hub_feature(struct usb_device *hdev, int 
feature)
  /*
   * USB 2.0 spec Section 11.24.2.2
   */
-static int clear_port_feature(struct usb_device *hdev, int port1, int feature)
+int clear_port_feature(struct usb_device *hdev, int port1, int feature)


This is now a global function, please put usb_ infront of it.


Same above?

  {
return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
USB_REQ_CLEAR_FEATURE, USB_RT_PORT, feature, port1,
@@ -718,11 +719,16 @@ int usb_hub_set_port_power(struct usb_device *hdev, int 
port1,
bool set)
  {
int ret;
+   struct usb_hub *hub = hdev_to_hub(hdev);
+   struct usb_port *port_dev = hub-ports[port1 - 1];

if (set)
ret = set_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
else
ret = clear_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
+
+   if (!ret)
+   port_dev-power_is_on = set;
return ret;
  }

@@ -802,7 +808,11 @@ static unsigned hub_power_on(struct usb_hub *hub, bool 
do_delay)
dev_dbg(hub-intfdev, trying to enable port power on 
non-switchable hub\n);
for (port1 = 1; port1 = hub-descriptor-bNbrPorts; port1++)
-   set_port_feature(hub-hdev, port1, USB_PORT_FEAT_POWER);
+   if (hub-ports[port1 - 1]-power_is_on)
+   set_port_feature(hub-hdev, port1, USB_PORT_FEAT_POWER);
+   else
+   clear_port_feature(hub-hdev, port1,
+   USB_PORT_FEAT_POWER);

/* Wait at least 100 msec for power to become stable */
delay = max(pgood_delay, (unsigned) 100);
@@ -1134,10 +1144,16 @@ static void hub_activate(struct usb_hub *hub, enum 
hub_activation_type type)
set_bit(port1, hub-change_bits);

} else if (udev-persist_enabled) {
+   struct usb_port *port_dev = hub-ports[port1 - 1];
+
  #ifdef CONFIG_PM
udev-reset_resume = 1;
  #endif
-   set_bit(port1, hub-change_bits);
+   /* Don't set the change_bits when the device
+* was powered off.
+

Re: [PATCH v6 5/8] usb: add usb port auto power off mechanism

2013-01-22 Thread Greg KH 
On Tue, Jan 22, 2013 at 11:32:13PM +0800, Lan Tianyu wrote:
 On 2013/1/22 5:30, Greg KH wrote:
 On Mon, Jan 21, 2013 at 10:18:04PM +0800, Lan Tianyu wrote:
 @@ -127,7 +128,7 @@ static inline char *portspeed(struct usb_hub *hub, int 
 portstatus)
   }
 
   /* Note that hdev or one of its children must be locked! */
 -static struct usb_hub *hdev_to_hub(struct usb_device *hdev)
 +struct usb_hub *hdev_to_hub(struct usb_device *hdev)
 
 This needs a better name now that this is a global function.  No one
 knows what a hdev is.
 Actually the routine will only be used in the
 driver/usb/core/(hub.c, port.c). Port also should be the part of
 hub, right? Moving port related code to port.c is to make code more
 clear. I am not sure whether we should rename it.

It is now a global symbol in the kernel name space, so yes, it should be
renamed.

 If we renamed it, how about usb_hub_to_struct_hub? \
 Should we do rename operation in a separate patch since
 hdev_to_hub() is called many places in the hub.c?

That's fine with me.

   {
 if (!hdev || !hdev-actconfig || !hdev-maxchild)
 return NULL;
 @@ -393,7 +394,7 @@ static int clear_hub_feature(struct usb_device *hdev, 
 int feature)
   /*
* USB 2.0 spec Section 11.24.2.2
*/
 -static int clear_port_feature(struct usb_device *hdev, int port1, int 
 feature)
 +int clear_port_feature(struct usb_device *hdev, int port1, int feature)
 
 This is now a global function, please put usb_ infront of it.
 
 Same above?

Yes.

 -static int hub_port_debounce(struct usb_hub *hub, int port1)
 +int hub_port_debounce(struct usb_hub *hub, int port1, bool 
 must_be_connected)
 
 I hate boolean flags in functions that do different things depending on
 the value.  Can you just make two different functions here, and have
 them call this private one with the proper flag set?
 Sorry. I am not very sure I understand correctly.
 Do you mean to create two wraps which call hub_port_debounce()?
 Just like
 hub_port_debounce_be_counted()
 {
   hub_port_debounce(...,..., true);
 }
 
 hub_port_debounce_be_counted()
 {
   hub_port_debounce(...,..., false);
 }

If you name the functions differently, yes :)

 @@ -3758,7 +3805,9 @@ static int hub_port_debounce(struct usb_hub *hub, int 
 port1)
 
 if (!(portchange  USB_PORT_STAT_C_CONNECTION) 
  (portstatus  USB_PORT_STAT_CONNECTION) == connection) {
 -   stable_time += HUB_DEBOUNCE_STEP;
 +   if (!must_be_connected || (connection
 +   == USB_PORT_STAT_CONNECTION))
 
 Please do:
  if (!must_be_connected ||
  (connection == USB_PORT_STAT_CONNECTION))
 
 instead, that makes it much more readable.
 Oh. I originally do the same thing. But there is following code
 which requires this line to be more indention. This will cause this
 line over 80 characters. So I had to break this line.

The above line, as written is under 80 characters, so I don't understand
the issue.

 I finally find that if I just do following, it also can work.
   if (!must_be_connected || connection)
   stable_time += HUB_DEBOUNCE_STEP;
 Does this make sense? Since connection only will be assigned to the
 result of  portstatus  USB_PORT_STAT_CONNECTION.

Connection is an enumerated type, please be explicit when testing 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 v6 5/8] usb: add usb port auto power off mechanism

2013-01-22 Thread Lan Tianyu 

On 2013/1/23 0:02, Greg KH wrote:

On Tue, Jan 22, 2013 at 11:32:13PM +0800, Lan Tianyu wrote:

On 2013/1/22 5:30, Greg KH wrote:

On Mon, Jan 21, 2013 at 10:18:04PM +0800, Lan Tianyu wrote:

@@ -127,7 +128,7 @@ static inline char *portspeed(struct usb_hub *hub, int 
portstatus)
  }

  /* Note that hdev or one of its children must be locked! */
-static struct usb_hub *hdev_to_hub(struct usb_device *hdev)
+struct usb_hub *hdev_to_hub(struct usb_device *hdev)


This needs a better name now that this is a global function.  No one
knows what a hdev is.

Actually the routine will only be used in the
driver/usb/core/(hub.c, port.c). Port also should be the part of
hub, right? Moving port related code to port.c is to make code more
clear. I am not sure whether we should rename it.


It is now a global symbol in the kernel name space, so yes, it should be
renamed.


If we renamed it, how about usb_hub_to_struct_hub? \
Should we do rename operation in a separate patch since
hdev_to_hub() is called many places in the hub.c?


That's fine with me.


  {
if (!hdev || !hdev-actconfig || !hdev-maxchild)
return NULL;
@@ -393,7 +394,7 @@ static int clear_hub_feature(struct usb_device *hdev, int 
feature)
  /*
   * USB 2.0 spec Section 11.24.2.2
   */
-static int clear_port_feature(struct usb_device *hdev, int port1, int feature)
+int clear_port_feature(struct usb_device *hdev, int port1, int feature)


This is now a global function, please put usb_ infront of it.


Same above?


Yes.


-static int hub_port_debounce(struct usb_hub *hub, int port1)
+int hub_port_debounce(struct usb_hub *hub, int port1, bool must_be_connected)


I hate boolean flags in functions that do different things depending on
the value.  Can you just make two different functions here, and have
them call this private one with the proper flag set?

Sorry. I am not very sure I understand correctly.
Do you mean to create two wraps which call hub_port_debounce()?
Just like
hub_port_debounce_be_counted()
{
hub_port_debounce(...,..., true);
}

hub_port_debounce_be_counted()
{
hub_port_debounce(...,..., false);
}


If you name the functions differently, yes :)


@@ -3758,7 +3805,9 @@ static int hub_port_debounce(struct usb_hub *hub, int 
port1)

if (!(portchange  USB_PORT_STAT_C_CONNECTION) 
 (portstatus  USB_PORT_STAT_CONNECTION) == connection) {
-   stable_time += HUB_DEBOUNCE_STEP;
+   if (!must_be_connected || (connection
+   == USB_PORT_STAT_CONNECTION))


Please do:
if (!must_be_connected ||
(connection == USB_PORT_STAT_CONNECTION))

instead, that makes it much more readable.

Oh. I originally do the same thing. But there is following code
which requires this line to be more indention. This will cause this
line over 80 characters. So I had to break this line.


The above line, as written is under 80 characters, so I don't understand
the issue.

If I do following, scripts/checkpatch.pl will complain over 80 characters.

@@ -3767,7 +3814,9 @@

if (!(portchange  USB_PORT_STAT_C_CONNECTION) 
 (portstatus  USB_PORT_STAT_CONNECTION) == connection) {
-   stable_time += HUB_DEBOUNCE_STEP;
+   if (!must_be_connected ||
+   (connection == 
USB_PORT_STAT_CONNECTION))
+   stable_time += HUB_DEBOUNCE_STEP;
if (stable_time = HUB_DEBOUNCE_STABLE)
break;
} else {

WARNING: line over 80 characters
#203: FILE: drivers/usb/core/hub.c:3818:
+   (connection == 
USB_PORT_STAT_CONNECTION))

If I use (connection  USB_PORT_STAT_CONNECTION)), that will be ok.
I am curious about that checkpatch.pl considers a space or tab as a character?





I finally find that if I just do following, it also can work.
if (!must_be_connected || connection)
stable_time += HUB_DEBOUNCE_STEP;
Does this make sense? Since connection only will be assigned to the
result of  portstatus  USB_PORT_STAT_CONNECTION.


Connection is an enumerated type, please be explicit when testing it.

thanks,

greg k-h



--
Best Regards
Tianyu Lan
linux kernel enabling team
--
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 v6 5/8] usb: add usb port auto power off mechanism

2013-01-22 Thread Greg KH 
On Wed, Jan 23, 2013 at 12:58:09AM +0800, Lan Tianyu wrote:
 On 2013/1/23 0:02, Greg KH wrote:
 On Tue, Jan 22, 2013 at 11:32:13PM +0800, Lan Tianyu wrote:
 On 2013/1/22 5:30, Greg KH wrote:
 @@ -3758,7 +3805,9 @@ static int hub_port_debounce(struct usb_hub *hub, 
 int port1)
 
   if (!(portchange  USB_PORT_STAT_C_CONNECTION) 
(portstatus  USB_PORT_STAT_CONNECTION) == 
  connection) {
 - stable_time += HUB_DEBOUNCE_STEP;
 + if (!must_be_connected || (connection
 + == USB_PORT_STAT_CONNECTION))
 
 Please do:
if (!must_be_connected ||
(connection == USB_PORT_STAT_CONNECTION))
 
 instead, that makes it much more readable.
 Oh. I originally do the same thing. But there is following code
 which requires this line to be more indention. This will cause this
 line over 80 characters. So I had to break this line.
 
 The above line, as written is under 80 characters, so I don't understand
 the issue.
 If I do following, scripts/checkpatch.pl will complain over 80 characters.
 
 @@ -3767,7 +3814,9 @@
 
 if (!(portchange  USB_PORT_STAT_C_CONNECTION) 
  (portstatus  USB_PORT_STAT_CONNECTION) == connection) {
 -   stable_time += HUB_DEBOUNCE_STEP;
 +   if (!must_be_connected ||
 +   (connection == 
 USB_PORT_STAT_CONNECTION))
 +   stable_time += HUB_DEBOUNCE_STEP;
 if (stable_time = HUB_DEBOUNCE_STABLE)
 break;
 } else {
 
 WARNING: line over 80 characters
 #203: FILE: drivers/usb/core/hub.c:3818:
 +   (connection == 
 USB_PORT_STAT_CONNECTION))

Of course, which is why I didn't say to do that :)

See above for where to line up the line with the previous one, showing
that it all can fit just fine within 80 characters.

 If I use (connection  USB_PORT_STAT_CONNECTION)), that will be ok.
 I am curious about that checkpatch.pl considers a space or tab as a character?

It understands the difference between tabs and spaces properly.

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 v6 5/8] usb: add usb port auto power off mechanism

2013-01-22 Thread Lan Tianyu 

On 2013/1/23 1:18, Greg KH wrote:

On Wed, Jan 23, 2013 at 12:58:09AM +0800, Lan Tianyu wrote

The above line, as written is under 80 characters, so I don't understand
the issue.

If I do following, scripts/checkpatch.pl will complain over 80 characters.

@@ -3767,7 +3814,9 @@

 if (!(portchange  USB_PORT_STAT_C_CONNECTION) 
  (portstatus  USB_PORT_STAT_CONNECTION) == connection) {
-   stable_time += HUB_DEBOUNCE_STEP;
+   if (!must_be_connected ||
+   (connection == 
USB_PORT_STAT_CONNECTION))
+   stable_time += HUB_DEBOUNCE_STEP;
 if (stable_time = HUB_DEBOUNCE_STABLE)
 break;
 } else {

WARNING: line over 80 characters
#203: FILE: drivers/usb/core/hub.c:3818:
+   (connection == 
USB_PORT_STAT_CONNECTION))


Of course, which is why I didn't say to do that :)

See above for where to line up the line with the previous one, showing
that it all can fit just fine within 80 characters.


Ok. I get it. learn one more trick. :)

Now I have two solutions. Which one do you think more readable?
@@ -3740,7 +3814,9 @@ static int hub_port_debounce(struct usb_hub *hub, int 
port1)

if (!(portchange  USB_PORT_STAT_C_CONNECTION) 
 (portstatus  USB_PORT_STAT_CONNECTION) == connection) {
-   stable_time += HUB_DEBOUNCE_STEP;
+   if (!must_be_connected ||
+   (connection  USB_PORT_STAT_CONNECTION))
+   stable_time += HUB_DEBOUNCE_STEP;
if (stable_time = HUB_DEBOUNCE_STABLE)
break;


@@ -3740,7 +3814,9 @@ static int hub_port_debounce(struct usb_hub *hub, int 
port1)

if (!(portchange  USB_PORT_STAT_C_CONNECTION) 
 (portstatus  USB_PORT_STAT_CONNECTION) == connection) {
-   stable_time += HUB_DEBOUNCE_STEP;
+   if (!must_be_connected ||
+(connection == USB_PORT_STAT_CONNECTION))
+   stable_time += HUB_DEBOUNCE_STEP;
if (stable_time = HUB_DEBOUNCE_STABLE)









If I use (connection  USB_PORT_STAT_CONNECTION)), that will be ok.
I am curious about that checkpatch.pl considers a space or tab as a character?


It understands the difference between tabs and spaces properly.

thanks,

greg k-h



--
Best Regards
Tianyu Lan
linux kernel enabling team
--
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 v6 5/8] usb: add usb port auto power off mechanism

2013-01-21 Thread Greg KH 
On Mon, Jan 21, 2013 at 10:18:04PM +0800, Lan Tianyu wrote:
 This patch is to add usb port auto power off mechanism.
 When usb device is suspending, usb core will suspend usb port and
 usb port runtime pm callback will clear PORT_POWER feature to
 power off port if all conditions were met. These conditions are
 remote wakeup disable, pm qos NO_POWER_OFF flag clear and persist
 enable. When it resumes, power on port again. Add did_runtime_put
 in the struct usb_port in order to call pm_runtime_get/put(portdev)
 paired during suspending and resuming.

I don't understand what did_runtime_put is supposed to mean.  Care to
explain this better?

 
 Acked-by: Alan Stern st...@rowland.harvard.edu
 Acked-by: Rafael J. Wysocki rafael.j.wyso...@intel.com
 Signed-off-by: Lan Tianyu tianyu@intel.com
 ---
  drivers/usb/core/hub.c  |   67 
 ---
  drivers/usb/core/hub.h  |9 +++
  drivers/usb/core/port.c |   40 ++--
  3 files changed, 105 insertions(+), 11 deletions(-)
 
 diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
 index 8c1f9a5..786db99 100644
 --- a/drivers/usb/core/hub.c
 +++ b/drivers/usb/core/hub.c
 @@ -26,6 +26,7 @@
  #include linux/mutex.h
  #include linux/freezer.h
  #include linux/random.h
 +#include linux/pm_qos.h
  
  #include asm/uaccess.h
  #include asm/byteorder.h
 @@ -108,7 +109,7 @@ MODULE_PARM_DESC(use_both_schemes,
  DECLARE_RWSEM(ehci_cf_port_reset_rwsem);
  EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
  
 -#define HUB_DEBOUNCE_TIMEOUT 1500
 +#define HUB_DEBOUNCE_TIMEOUT 2000

Why did you increase this timeout?  You don't mention that above in the
changelog entry.

  #define HUB_DEBOUNCE_STEP  25
  #define HUB_DEBOUNCE_STABLE   100
  
 @@ -127,7 +128,7 @@ static inline char *portspeed(struct usb_hub *hub, int 
 portstatus)
  }
  
  /* Note that hdev or one of its children must be locked! */
 -static struct usb_hub *hdev_to_hub(struct usb_device *hdev)
 +struct usb_hub *hdev_to_hub(struct usb_device *hdev)

This needs a better name now that this is a global function.  No one
knows what a hdev is.

  {
   if (!hdev || !hdev-actconfig || !hdev-maxchild)
   return NULL;
 @@ -393,7 +394,7 @@ static int clear_hub_feature(struct usb_device *hdev, int 
 feature)
  /*
   * USB 2.0 spec Section 11.24.2.2
   */
 -static int clear_port_feature(struct usb_device *hdev, int port1, int 
 feature)
 +int clear_port_feature(struct usb_device *hdev, int port1, int feature)

This is now a global function, please put usb_ infront of it.

  {
   return usb_control_msg(hdev, usb_sndctrlpipe(hdev, 0),
   USB_REQ_CLEAR_FEATURE, USB_RT_PORT, feature, port1,
 @@ -718,11 +719,16 @@ int usb_hub_set_port_power(struct usb_device *hdev, int 
 port1,
   bool set)
  {
   int ret;
 + struct usb_hub *hub = hdev_to_hub(hdev);
 + struct usb_port *port_dev = hub-ports[port1 - 1];
  
   if (set)
   ret = set_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
   else
   ret = clear_port_feature(hdev, port1, USB_PORT_FEAT_POWER);
 +
 + if (!ret)
 + port_dev-power_is_on = set;
   return ret;
  }
  
 @@ -802,7 +808,11 @@ static unsigned hub_power_on(struct usb_hub *hub, bool 
 do_delay)
   dev_dbg(hub-intfdev, trying to enable port power on 
   non-switchable hub\n);
   for (port1 = 1; port1 = hub-descriptor-bNbrPorts; port1++)
 - set_port_feature(hub-hdev, port1, USB_PORT_FEAT_POWER);
 + if (hub-ports[port1 - 1]-power_is_on)
 + set_port_feature(hub-hdev, port1, USB_PORT_FEAT_POWER);
 + else
 + clear_port_feature(hub-hdev, port1,
 + USB_PORT_FEAT_POWER);
  
   /* Wait at least 100 msec for power to become stable */
   delay = max(pgood_delay, (unsigned) 100);
 @@ -1134,10 +1144,16 @@ static void hub_activate(struct usb_hub *hub, enum 
 hub_activation_type type)
   set_bit(port1, hub-change_bits);
  
   } else if (udev-persist_enabled) {
 + struct usb_port *port_dev = hub-ports[port1 - 1];
 +
  #ifdef CONFIG_PM
   udev-reset_resume = 1;
  #endif
 - set_bit(port1, hub-change_bits);
 + /* Don't set the change_bits when the device
 +  * was powered off.
 +  */
 + if (port_dev-power_is_on)
 + set_bit(port1, hub-change_bits);
  
   } else {
   /* The power session is gone; tell khubd */
 @@ -2012,7 +2028,10 @@ void usb_disconnect(struct usb_device **pdev)
   sysfs_remove_link(udev-dev.kobj, port);
   sysfs_remove_link(port_dev-dev.kobj, device);
  
 - pm_runtime_put(port_dev-dev);
 + if