Re: [PATCH 2/4] usb: introduce usb force power off mechanism

2013-04-08 Thread Lan Tianyu

On 2013/3/30 4:24, Alan Stern wrote:

On Fri, 29 Mar 2013, Sarah Sharp wrote:


Also, bear in mind that the proposed patch does not give userspace a
way to power off ports via usbfs.  What the new code does is a
power-off reset -- it turns off power to the port, waits a short time,
and then turns power back on.


I think we need two separate IOCTLs: turn off the port power and turn it
on.  Then we get the manual port power off for our QA testing, distros
can make interesting policies about manually powering off ports, and
userspace can choose how long they want the port to be off.  After all,
different devices may need a longer powered-off period than others.


Or one port-power IOCTL that takes the setting as an argument.  Yes,
this would be more flexible than a power-off reset.

Hi Alan  Sarah:
I just recall why I put power off and power on in one ioctl.
At first, I also tried to make power on and power off into two ioctls.
But I found after powering off a device, the usbfs device node will
be removed and so can't power on the port via the same usbfs node.

For this point, we should add usbfs node for usb port?



Alan Stern



--
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 2/4] usb: introduce usb force power off mechanism

2013-04-08 Thread Greg KH
On Mon, Apr 08, 2013 at 08:58:09PM +0800, Lan Tianyu wrote:
 On 2013/3/30 4:24, Alan Stern wrote:
 On Fri, 29 Mar 2013, Sarah Sharp wrote:
 
 Also, bear in mind that the proposed patch does not give userspace a
 way to power off ports via usbfs.  What the new code does is a
 power-off reset -- it turns off power to the port, waits a short time,
 and then turns power back on.
 
 I think we need two separate IOCTLs: turn off the port power and turn it
 on.  Then we get the manual port power off for our QA testing, distros
 can make interesting policies about manually powering off ports, and
 userspace can choose how long they want the port to be off.  After all,
 different devices may need a longer powered-off period than others.
 
 Or one port-power IOCTL that takes the setting as an argument.  Yes,
 this would be more flexible than a power-off reset.
 Hi Alan  Sarah:
   I just recall why I put power off and power on in one ioctl.
 At first, I also tried to make power on and power off into two ioctls.
 But I found after powering off a device, the usbfs device node will
 be removed and so can't power on the port via the same usbfs node.
 
 For this point, we should add usbfs node for usb port?

No.
--
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 2/4] usb: introduce usb force power off mechanism

2013-04-08 Thread Lan Tianyu

On 2013/4/1 23:12, Alan Stern wrote:

On Mon, 1 Apr 2013, Lan Tianyu wrote:


On 2013年03月30日 01:23, Alan Stern wrote:

On Fri, 29 Mar 2013, Sarah Sharp wrote:

However, what happens if you echo 0 to pm_qos_no_power_off, the
power/control is set to auto, and there's a suspended USB device
attached to the port with remote wakeup enabled?  Will the port be
powered off?  I don't think it will with the current patchset, but I
will have to double check.


It shouldn't be, because remote wakeup is enabled.  But what about the
case where remote wakeup is disabled?  Will writing a 0 to
pm_qos_no_power_off cause the power to be turned off?

Or what about the case where there's no device attached to the port?  I
guess both questions amount to the same thing: When the user writes to
a pm_qos_* file, does the code call pm_runtime_idle?



Usb port's initial usage count is 0. For port attached with usb port,
when usb device is connected, the usb port's usage count is increased by
one. During device's suspend, its usage count would be decreased to 0
and powered of only when remote wakeup disable, persist enable and PM
Qos NO_POWER_OFF unsetting.


I already know all this.


During pm_qos_* file changing, pm_runtime_get_sync/put(port_dev) will be
done in the dev_pm_qos_update_flags(). The port's  usage count would be
increased and decreased by 1. Whether the port's pm_runtime_idle would
be called depends on port's usage count to be set to 0.


That answers my question.


If the usb port has already been powered off(port usage count is already
set to 0), it would be powered on first and powered off depending the PM
Qos NO_POWER_OFF flag value.


Obviously.


If the usb port wasn't powered off, this mean the usb port didn't meet
power off condition() and  its usage count was still greater than 0 So
during changing flag, only usage count was changed and no actual operation.


No, because one of the power off conditions is that the NO_POWER_OFF
flag must be clear.  If that flag was set, the port won't be powered
off even though the usage count is equal to 0.  If the write to the
pm_qos_* file then clears the flag, the port _will_ get powered off.


Hi Alan:
I think current code can't achieve that power off port(whose
child device was already suspended but it was not powered off due to
NO_POWER_OFF flag setting.) via clearing NO_POWER_OFF flag. Because
at that moment, its usage count can't be 0. At last, it should be
1 or large. port's pm_runtime_idle will not be called.

Further thinking, now we call pm_runtime_put_sync() in the
usb_port_suspend() when persist enable, do_remote_wakeup disable and
PM Qos NO_POWER_OFF flag cleared. But PM Qos NO_POWER_OFF flag will
also will be checked in the usb_port_runtime_suspend(). This seems
redundant.

From my opinion, the PM Qos check in the usb_port_suspend()
should be removed and port usage count will reach 0 when persist enable
and remote_wakeup disable. Whether power off or not totally depends on
the PM Qos flag in the usb_port_runtime_suspend(). For NO_POWER_OFF flag
setting case during usb device being suspend, the port's usage count
will be 0 but its runtime status is still active due to flag being set
and usb_port_runtime_suspend() returns -EAGAIN. At this time, clearing
the flag and the port can be powered off. Does this make sense?




For empty port, Only when PM Qos NO_POWER_OFF flag value is set to 0,
the port will be power off.


Alan Stern




--
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 2/4] usb: introduce usb force power off mechanism

2013-04-08 Thread Sarah Sharp
On Mon, Apr 08, 2013 at 06:29:36AM -0700, Greg KH wrote:
 On Mon, Apr 08, 2013 at 08:58:09PM +0800, Lan Tianyu wrote:
  On 2013/3/30 4:24, Alan Stern wrote:
  On Fri, 29 Mar 2013, Sarah Sharp wrote:
  
  Also, bear in mind that the proposed patch does not give userspace a
  way to power off ports via usbfs.  What the new code does is a
  power-off reset -- it turns off power to the port, waits a short time,
  and then turns power back on.
  
  I think we need two separate IOCTLs: turn off the port power and turn it
  on.  Then we get the manual port power off for our QA testing, distros
  can make interesting policies about manually powering off ports, and
  userspace can choose how long they want the port to be off.  After all,
  different devices may need a longer powered-off period than others.
  
  Or one port-power IOCTL that takes the setting as an argument.  Yes,
  this would be more flexible than a power-off reset.
  Hi Alan  Sarah:
  I just recall why I put power off and power on in one ioctl.
  At first, I also tried to make power on and power off into two ioctls.
  But I found after powering off a device, the usbfs device node will
  be removed and so can't power on the port via the same usbfs node.
  
  For this point, we should add usbfs node for usb port?
 
 No.

I agree that we shouldn't add more usbfs files without thinking very
carefully about it, since lots of tools like libusb use them.  However,
we do need a way to manually power off a port, wait a variable length of
time (or perhaps for a distro-specific event like screen unblank), and
turn the port on.

So how do we turn the port power back on with the options we have?
Would userspace have to turn the port power off via usbfs, and then
manually back on by setting the port's sysfs power/control to 'on'?

Sarah Sharp
--
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 2/4] usb: introduce usb force power off mechanism

2013-04-08 Thread Greg KH
On Mon, Apr 08, 2013 at 08:57:43AM -0700, Sarah Sharp wrote:
 On Mon, Apr 08, 2013 at 06:29:36AM -0700, Greg KH wrote:
  On Mon, Apr 08, 2013 at 08:58:09PM +0800, Lan Tianyu wrote:
   On 2013/3/30 4:24, Alan Stern wrote:
   On Fri, 29 Mar 2013, Sarah Sharp wrote:
   
   Also, bear in mind that the proposed patch does not give userspace a
   way to power off ports via usbfs.  What the new code does is a
   power-off reset -- it turns off power to the port, waits a short time,
   and then turns power back on.
   
   I think we need two separate IOCTLs: turn off the port power and turn it
   on.  Then we get the manual port power off for our QA testing, distros
   can make interesting policies about manually powering off ports, and
   userspace can choose how long they want the port to be off.  After all,
   different devices may need a longer powered-off period than others.
   
   Or one port-power IOCTL that takes the setting as an argument.  Yes,
   this would be more flexible than a power-off reset.
   Hi Alan  Sarah:
 I just recall why I put power off and power on in one ioctl.
   At first, I also tried to make power on and power off into two ioctls.
   But I found after powering off a device, the usbfs device node will
   be removed and so can't power on the port via the same usbfs node.
   
   For this point, we should add usbfs node for usb port?
  
  No.
 
 I agree that we shouldn't add more usbfs files without thinking very
 carefully about it, since lots of tools like libusb use them.  However,
 we do need a way to manually power off a port, wait a variable length of
 time (or perhaps for a distro-specific event like screen unblank), and
 turn the port on.
 
 So how do we turn the port power back on with the options we have?
 Would userspace have to turn the port power off via usbfs, and then
 manually back on by setting the port's sysfs power/control to 'on'?

Whatever method we use, it should be the same interface for both on
and off, so I would prefer to just use the sysfs one, as usbfs does not
represent ports, only USB devices.

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 2/4] usb: introduce usb force power off mechanism

2013-04-08 Thread Alan Stern
On Mon, 8 Apr 2013, Greg KH wrote:

 On Mon, Apr 08, 2013 at 08:57:43AM -0700, Sarah Sharp wrote:
  On Mon, Apr 08, 2013 at 06:29:36AM -0700, Greg KH wrote:
   On Mon, Apr 08, 2013 at 08:58:09PM +0800, Lan Tianyu wrote:
On 2013/3/30 4:24, Alan Stern wrote:
On Fri, 29 Mar 2013, Sarah Sharp wrote:

Also, bear in mind that the proposed patch does not give userspace a
way to power off ports via usbfs.  What the new code does is a
power-off reset -- it turns off power to the port, waits a short 
time,
and then turns power back on.

I think we need two separate IOCTLs: turn off the port power and turn 
it
on.  Then we get the manual port power off for our QA testing, distros
can make interesting policies about manually powering off ports, and
userspace can choose how long they want the port to be off.  After 
all,
different devices may need a longer powered-off period than others.

Or one port-power IOCTL that takes the setting as an argument.  Yes,
this would be more flexible than a power-off reset.
Hi Alan  Sarah:
I just recall why I put power off and power on in one ioctl.
At first, I also tried to make power on and power off into two ioctls.
But I found after powering off a device, the usbfs device node will
be removed and so can't power on the port via the same usbfs node.

For this point, we should add usbfs node for usb port?
   
   No.
  
  I agree that we shouldn't add more usbfs files without thinking very
  carefully about it, since lots of tools like libusb use them.  However,
  we do need a way to manually power off a port, wait a variable length of
  time (or perhaps for a distro-specific event like screen unblank), and
  turn the port on.
  
  So how do we turn the port power back on with the options we have?
  Would userspace have to turn the port power off via usbfs, and then
  manually back on by setting the port's sysfs power/control to 'on'?
 
 Whatever method we use, it should be the same interface for both on
 and off, so I would prefer to just use the sysfs one, as usbfs does not
 represent ports, only USB devices.

There is a way we can do it using the existing usbfs framework.  The
new ioctls could be sent to the parent hub, instead of the device
attached to the port.  Rather like USBDEVFS_CLAIM_PORT and
USBDEVFS_RELEASE_PORT.

Alan Stern

--
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 2/4] usb: introduce usb force power off mechanism

2013-04-08 Thread Alan Stern
On Mon, 8 Apr 2013, Lan Tianyu wrote:

 Hi Alan:
   I think current code can't achieve that power off port(whose
 child device was already suspended but it was not powered off due to
 NO_POWER_OFF flag setting.) via clearing NO_POWER_OFF flag. Because
 at that moment, its usage count can't be 0. At last, it should be
 1 or large. port's pm_runtime_idle will not be called.
 
   Further thinking, now we call pm_runtime_put_sync() in the
 usb_port_suspend() when persist enable, do_remote_wakeup disable and
 PM Qos NO_POWER_OFF flag cleared. But PM Qos NO_POWER_OFF flag will
 also will be checked in the usb_port_runtime_suspend(). This seems
 redundant.
 
   From my opinion, the PM Qos check in the usb_port_suspend()
 should be removed and port usage count will reach 0 when persist enable
 and remote_wakeup disable. Whether power off or not totally depends on
 the PM Qos flag in the usb_port_runtime_suspend(). For NO_POWER_OFF flag
 setting case during usb device being suspend, the port's usage count
 will be 0 but its runtime status is still active due to flag being set
 and usb_port_runtime_suspend() returns -EAGAIN. At this time, clearing
 the flag and the port can be powered off. Does this make sense?

Yes, that's the right thing to do.

Alan Stern

--
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 2/4] usb: introduce usb force power off mechanism

2013-04-08 Thread Sarah Sharp
On Mon, Apr 08, 2013 at 09:01:44AM -0700, Greg KH wrote:
 On Mon, Apr 08, 2013 at 08:57:43AM -0700, Sarah Sharp wrote:
  On Mon, Apr 08, 2013 at 06:29:36AM -0700, Greg KH wrote:
   On Mon, Apr 08, 2013 at 08:58:09PM +0800, Lan Tianyu wrote:
Hi Alan  Sarah:
I just recall why I put power off and power on in one ioctl.
At first, I also tried to make power on and power off into two ioctls.
But I found after powering off a device, the usbfs device node will
be removed and so can't power on the port via the same usbfs node.

For this point, we should add usbfs node for usb port?
   
   No.
  
  I agree that we shouldn't add more usbfs files without thinking very
  carefully about it, since lots of tools like libusb use them.  However,
  we do need a way to manually power off a port, wait a variable length of
  time (or perhaps for a distro-specific event like screen unblank), and
  turn the port on.
  
  So how do we turn the port power back on with the options we have?
  Would userspace have to turn the port power off via usbfs, and then
  manually back on by setting the port's sysfs power/control to 'on'?
 
 Whatever method we use, it should be the same interface for both on
 and off, so I would prefer to just use the sysfs one, as usbfs does not
 represent ports, only USB devices.

I would prefer that as well.  However, the port power sysfs file
power/control is created by the runtime PM core, and I think it doesn't
have a way to manually turn power off.  All we get with the current
framework is 'on' or 'auto'.  There may be times when userspace wants to
override the kernel's policy for 'auto' and force a port power off.

Perhaps the runtime PM core needs to be extended to have a way to allow
userspace to manually force a device into a lower power state?

Sarah Sharp
--
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 2/4] usb: introduce usb force power off mechanism

2013-04-08 Thread Sarah Sharp
Cc-ing the linux-pm list and some Intel power devs, as I think this
specific discussion could benefit from a broader audience.

On Mon, Apr 08, 2013 at 12:33:00PM -0400, Alan Stern wrote:
 On Mon, 8 Apr 2013, Greg KH wrote:
 
  On Mon, Apr 08, 2013 at 08:57:43AM -0700, Sarah Sharp wrote:
   On Mon, Apr 08, 2013 at 06:29:36AM -0700, Greg KH wrote:
On Mon, Apr 08, 2013 at 08:58:09PM +0800, Lan Tianyu wrote:
 On 2013/3/30 4:24, Alan Stern wrote:
 On Fri, 29 Mar 2013, Sarah Sharp wrote:
 Hi Alan  Sarah:
   I just recall why I put power off and power on in one ioctl.
 At first, I also tried to make power on and power off into two ioctls.
 But I found after powering off a device, the usbfs device node will
 be removed and so can't power on the port via the same usbfs node.
 
 For this point, we should add usbfs node for usb port?

No.
   
   I agree that we shouldn't add more usbfs files without thinking very
   carefully about it, since lots of tools like libusb use them.  However,
   we do need a way to manually power off a port, wait a variable length of
   time (or perhaps for a distro-specific event like screen unblank), and
   turn the port on.
   
   So how do we turn the port power back on with the options we have?
   Would userspace have to turn the port power off via usbfs, and then
   manually back on by setting the port's sysfs power/control to 'on'?
  
  Whatever method we use, it should be the same interface for both on
  and off, so I would prefer to just use the sysfs one, as usbfs does not
  represent ports, only USB devices.
 
 There is a way we can do it using the existing usbfs framework.  The
 new ioctls could be sent to the parent hub, instead of the device
 attached to the port.  Rather like USBDEVFS_CLAIM_PORT and
 USBDEVFS_RELEASE_PORT.

That could work.  However, we have to think about future platform power
changes as well.  Coming up with a USB specific way to work around the
runtime PM core will hurt us in the long run, if we end up having to
change the runtime PM core for another kernel user.

Len, Rafael, and Kristen, is there a need from any of the future power
work to have an 'off' mechanism added to the runtime PM core, so that
power/control would have 'on', 'auto', and 'off' options?  It currently
only has 'on' and 'auto'.

The kernel is always going to be more conservative about what policies
cause the 'auto' option to turn off USB ports.  A Linux distro may want
to override those policies and force the port off, or power off a
misbehaving device for a hard reset.  That's why we need an 'off'
extension to power/control to bypass the runtime PM usage counts and
power something off.

Are there analogous needs for other users of power/control?

Sarah Sharp
--
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 2/4] usb: introduce usb force power off mechanism

2013-04-08 Thread Rafael J. Wysocki
On Monday, April 08, 2013 10:33:54 AM Sarah Sharp wrote:
 On Mon, Apr 08, 2013 at 09:01:44AM -0700, Greg KH wrote:
  On Mon, Apr 08, 2013 at 08:57:43AM -0700, Sarah Sharp wrote:
   On Mon, Apr 08, 2013 at 06:29:36AM -0700, Greg KH wrote:
On Mon, Apr 08, 2013 at 08:58:09PM +0800, Lan Tianyu wrote:
 Hi Alan  Sarah:
   I just recall why I put power off and power on in one ioctl.
 At first, I also tried to make power on and power off into two ioctls.
 But I found after powering off a device, the usbfs device node will
 be removed and so can't power on the port via the same usbfs node.
 
 For this point, we should add usbfs node for usb port?

No.
   
   I agree that we shouldn't add more usbfs files without thinking very
   carefully about it, since lots of tools like libusb use them.  However,
   we do need a way to manually power off a port, wait a variable length of
   time (or perhaps for a distro-specific event like screen unblank), and
   turn the port on.
   
   So how do we turn the port power back on with the options we have?
   Would userspace have to turn the port power off via usbfs, and then
   manually back on by setting the port's sysfs power/control to 'on'?
  
  Whatever method we use, it should be the same interface for both on
  and off, so I would prefer to just use the sysfs one, as usbfs does not
  represent ports, only USB devices.
 
 I would prefer that as well.  However, the port power sysfs file
 power/control is created by the runtime PM core, and I think it doesn't
 have a way to manually turn power off.  All we get with the current
 framework is 'on' or 'auto'.  There may be times when userspace wants to
 override the kernel's policy for 'auto' and force a port power off.
 
 Perhaps the runtime PM core needs to be extended to have a way to allow
 userspace to manually force a device into a lower power state?

We actually considered that when the PM core's runtime PM framework was
being implemented and the answer was no.  The reason being that user space
has no idea whether or not the device *can* be turned off at the given
time, so the kernel can't guarantee any requests to turn devices off to be
satisfied at any given time.  I believe this is the case for USB ports too.

I don't think anything has changed in that respect since then.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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 2/4] usb: introduce usb force power off mechanism

2013-04-08 Thread Alan Stern
On Mon, 8 Apr 2013, Sarah Sharp wrote:

 That could work.  However, we have to think about future platform power
 changes as well.  Coming up with a USB specific way to work around the
 runtime PM core will hurt us in the long run, if we end up having to
 change the runtime PM core for another kernel user.
 
 Len, Rafael, and Kristen, is there a need from any of the future power
 work to have an 'off' mechanism added to the runtime PM core, so that
 power/control would have 'on', 'auto', and 'off' options?  It currently
 only has 'on' and 'auto'.

I can't say anything about future power work -- Len, Rafael, et al.  
will have to speak to that -- but the current design of the runtime PM
core doesn't allow for a distinction between low power and no
power.  As far as the core is concerned, either the device is fully
active or else it isn't (i.e., it is suspended).  To change this would
be a major rewrite.

 The kernel is always going to be more conservative about what policies
 cause the 'auto' option to turn off USB ports.  A Linux distro may want
 to override those policies and force the port off, or power off a
 misbehaving device for a hard reset.  That's why we need an 'off'
 extension to power/control to bypass the runtime PM usage counts and
 power something off.
 
 Are there analogous needs for other users of power/control?

In fact, some other people have made similar requests.  I can't 
remember the exact contexts now...  One of them may have been related 
to the PCI D4cold stuff.

Alan Stern

--
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 2/4] usb: introduce usb force power off mechanism

2013-04-08 Thread Rafael J. Wysocki
On Monday, April 08, 2013 10:55:19 AM Sarah Sharp wrote:
 Cc-ing the linux-pm list and some Intel power devs, as I think this
 specific discussion could benefit from a broader audience.
 
 On Mon, Apr 08, 2013 at 12:33:00PM -0400, Alan Stern wrote:
  On Mon, 8 Apr 2013, Greg KH wrote:
  
   On Mon, Apr 08, 2013 at 08:57:43AM -0700, Sarah Sharp wrote:
On Mon, Apr 08, 2013 at 06:29:36AM -0700, Greg KH wrote:
 On Mon, Apr 08, 2013 at 08:58:09PM +0800, Lan Tianyu wrote:
  On 2013/3/30 4:24, Alan Stern wrote:
  On Fri, 29 Mar 2013, Sarah Sharp wrote:
  Hi Alan  Sarah:
  I just recall why I put power off and power on in one ioctl.
  At first, I also tried to make power on and power off into two 
  ioctls.
  But I found after powering off a device, the usbfs device node will
  be removed and so can't power on the port via the same usbfs node.
  
  For this point, we should add usbfs node for usb port?
 
 No.

I agree that we shouldn't add more usbfs files without thinking very
carefully about it, since lots of tools like libusb use them.  However,
we do need a way to manually power off a port, wait a variable length of
time (or perhaps for a distro-specific event like screen unblank), and
turn the port on.

So how do we turn the port power back on with the options we have?
Would userspace have to turn the port power off via usbfs, and then
manually back on by setting the port's sysfs power/control to 'on'?
   
   Whatever method we use, it should be the same interface for both on
   and off, so I would prefer to just use the sysfs one, as usbfs does not
   represent ports, only USB devices.
  
  There is a way we can do it using the existing usbfs framework.  The
  new ioctls could be sent to the parent hub, instead of the device
  attached to the port.  Rather like USBDEVFS_CLAIM_PORT and
  USBDEVFS_RELEASE_PORT.
 
 That could work.  However, we have to think about future platform power
 changes as well.  Coming up with a USB specific way to work around the
 runtime PM core will hurt us in the long run, if we end up having to
 change the runtime PM core for another kernel user.
 
 Len, Rafael, and Kristen, is there a need from any of the future power
 work to have an 'off' mechanism added to the runtime PM core, so that
 power/control would have 'on', 'auto', and 'off' options?  It currently
 only has 'on' and 'auto'.

There's no such work for the reason given in another message a while ago.

 The kernel is always going to be more conservative about what policies
 cause the 'auto' option to turn off USB ports.  A Linux distro may want
 to override those policies and force the port off, or power off a
 misbehaving device for a hard reset.  That's why we need an 'off'
 extension to power/control to bypass the runtime PM usage counts and
 power something off.

Then please make it USB-specific.  Although I believe it would be
dangerous, too, if used without care (say, for a storage device attached
via USB).

I *think* it might be better to have a force power cycle interface for
USB ports that would be clearly named and documented so that there's no
confusion as to what it is for.

 Are there analogous needs for other users of power/control?

No, there aren't.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
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 2/4] usb: introduce usb force power off mechanism

2013-04-08 Thread Sarah Sharp
On Mon, Apr 08, 2013 at 09:39:07PM +0200, Rafael J. Wysocki wrote:
 On Monday, April 08, 2013 10:55:19 AM Sarah Sharp wrote:
  Cc-ing the linux-pm list and some Intel power devs, as I think this
  specific discussion could benefit from a broader audience.
  
  On Mon, Apr 08, 2013 at 12:33:00PM -0400, Alan Stern wrote:
   On Mon, 8 Apr 2013, Greg KH wrote:
   
On Mon, Apr 08, 2013 at 08:57:43AM -0700, Sarah Sharp wrote:
 On Mon, Apr 08, 2013 at 06:29:36AM -0700, Greg KH wrote:
  On Mon, Apr 08, 2013 at 08:58:09PM +0800, Lan Tianyu wrote:
   On 2013/3/30 4:24, Alan Stern wrote:
   On Fri, 29 Mar 2013, Sarah Sharp wrote:
   Hi Alan  Sarah:
 I just recall why I put power off and power on in one ioctl.
   At first, I also tried to make power on and power off into two 
   ioctls.
   But I found after powering off a device, the usbfs device node 
   will
   be removed and so can't power on the port via the same usbfs node.
   
   For this point, we should add usbfs node for usb port?
  
  No.
 
 I agree that we shouldn't add more usbfs files without thinking very
 carefully about it, since lots of tools like libusb use them.  
 However,
 we do need a way to manually power off a port, wait a variable length 
 of
 time (or perhaps for a distro-specific event like screen unblank), and
 turn the port on.
 
 So how do we turn the port power back on with the options we have?
 Would userspace have to turn the port power off via usbfs, and then
 manually back on by setting the port's sysfs power/control to 'on'?

Whatever method we use, it should be the same interface for both on
and off, so I would prefer to just use the sysfs one, as usbfs does not
represent ports, only USB devices.
   
   There is a way we can do it using the existing usbfs framework.  The
   new ioctls could be sent to the parent hub, instead of the device
   attached to the port.  Rather like USBDEVFS_CLAIM_PORT and
   USBDEVFS_RELEASE_PORT.
  
  That could work.  However, we have to think about future platform power
  changes as well.  Coming up with a USB specific way to work around the
  runtime PM core will hurt us in the long run, if we end up having to
  change the runtime PM core for another kernel user.
  
  Len, Rafael, and Kristen, is there a need from any of the future power
  work to have an 'off' mechanism added to the runtime PM core, so that
  power/control would have 'on', 'auto', and 'off' options?  It currently
  only has 'on' and 'auto'.
 
 There's no such work for the reason given in another message a while ago.
 
  The kernel is always going to be more conservative about what policies
  cause the 'auto' option to turn off USB ports.  A Linux distro may want
  to override those policies and force the port off, or power off a
  misbehaving device for a hard reset.  That's why we need an 'off'
  extension to power/control to bypass the runtime PM usage counts and
  power something off.
 
 Then please make it USB-specific.  Although I believe it would be
 dangerous, too, if used without care (say, for a storage device attached
 via USB).

All right, then let's make this USB specific.  Alan's idea of making the
ioctls bind to the parent hub makes sense.

And yes, userspace will have to take care about which ports it powers
off.  I think we currently expose enough information about what devices
are attached to which ports to allow Linux distros to make smart
decisions about what to allow to be powered off.

Sarah Sharp
--
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 2/4] usb: introduce usb force power off mechanism

2013-04-01 Thread Lan Tianyu
On 2013年03月29日 22:11, Alan Stern wrote:
 On Fri, 29 Mar 2013, Lan Tianyu wrote:
 
 Actually, I exposed pm qos flags for usb port via dev_pm_qos_expose_flags().
 It creates power/pm_qos_no_power_off under usb port sysfs directory.
 User can echo 0  pm_qos_no_power_off to power off the empty port.
 
 Before it's too late, we should consider changing the flag's name.  
 Writing 0 to a file named pm_qos_no_power_off is a double negative, 
 which is always awkward to think about.
 
 Instead, how about calling the file pm_qos_allow_power_off?  Will the
 PM-QOS system allow something like that, i.e., something that is the
 opposite of a constraint?
 
 If not, how about naming it pm_qos_keep_power_on?
This is PM core sysfs attribute and I am not sure that this can be changed.

CC: Rafael.

 
 Alan Stern
 


-- 
Best regards
Tianyu Lan
--
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 2/4] usb: introduce usb force power off mechanism

2013-04-01 Thread Lan Tianyu
On 2013年03月30日 01:23, Alan Stern wrote:
 On Fri, 29 Mar 2013, Sarah Sharp wrote:
 However, what happens if you echo 0 to pm_qos_no_power_off, the
 power/control is set to auto, and there's a suspended USB device
 attached to the port with remote wakeup enabled?  Will the port be
 powered off?  I don't think it will with the current patchset, but I
 will have to double check.
 
 It shouldn't be, because remote wakeup is enabled.  But what about the
 case where remote wakeup is disabled?  Will writing a 0 to
 pm_qos_no_power_off cause the power to be turned off?
 
 Or what about the case where there's no device attached to the port?  I 
 guess both questions amount to the same thing: When the user writes to 
 a pm_qos_* file, does the code call pm_runtime_idle?
 

Usb port's initial usage count is 0. For port attached with usb port,
when usb device is connected, the usb port's usage count is increased by
one. During device's suspend, its usage count would be decreased to 0
and powered of only when remote wakeup disable, persist enable and PM
Qos NO_POWER_OFF unsetting.

During pm_qos_* file changing, pm_runtime_get_sync/put(port_dev) will be
done in the dev_pm_qos_update_flags(). The port's  usage count would be
increased and decreased by 1. Whether the port's pm_runtime_idle would
be called depends on port's usage count to be set to 0.

If the usb port has already been powered off(port usage count is already
set to 0), it would be powered on first and powered off depending the PM
Qos NO_POWER_OFF flag value.

If the usb port wasn't powered off, this mean the usb port didn't meet
power off condition() and  its usage count was still greater than 0 So
during changing flag, only usage count was changed and no actual operation.

For empty port, Only when PM Qos NO_POWER_OFF flag value is set to 0,
the port will be power off.

 
 Alan Stern
 


-- 
Best regards
Tianyu Lan
--
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 2/4] usb: introduce usb force power off mechanism

2013-04-01 Thread Alan Stern
On Mon, 1 Apr 2013, Lan Tianyu wrote:

 On 2013年03月30日 01:23, Alan Stern wrote:
  On Fri, 29 Mar 2013, Sarah Sharp wrote:
  However, what happens if you echo 0 to pm_qos_no_power_off, the
  power/control is set to auto, and there's a suspended USB device
  attached to the port with remote wakeup enabled?  Will the port be
  powered off?  I don't think it will with the current patchset, but I
  will have to double check.
  
  It shouldn't be, because remote wakeup is enabled.  But what about the
  case where remote wakeup is disabled?  Will writing a 0 to
  pm_qos_no_power_off cause the power to be turned off?
  
  Or what about the case where there's no device attached to the port?  I 
  guess both questions amount to the same thing: When the user writes to 
  a pm_qos_* file, does the code call pm_runtime_idle?
  
 
 Usb port's initial usage count is 0. For port attached with usb port,
 when usb device is connected, the usb port's usage count is increased by
 one. During device's suspend, its usage count would be decreased to 0
 and powered of only when remote wakeup disable, persist enable and PM
 Qos NO_POWER_OFF unsetting.

I already know all this.

 During pm_qos_* file changing, pm_runtime_get_sync/put(port_dev) will be
 done in the dev_pm_qos_update_flags(). The port's  usage count would be
 increased and decreased by 1. Whether the port's pm_runtime_idle would
 be called depends on port's usage count to be set to 0.

That answers my question.

 If the usb port has already been powered off(port usage count is already
 set to 0), it would be powered on first and powered off depending the PM
 Qos NO_POWER_OFF flag value.

Obviously.

 If the usb port wasn't powered off, this mean the usb port didn't meet
 power off condition() and  its usage count was still greater than 0 So
 during changing flag, only usage count was changed and no actual operation.

No, because one of the power off conditions is that the NO_POWER_OFF
flag must be clear.  If that flag was set, the port won't be powered
off even though the usage count is equal to 0.  If the write to the
pm_qos_* file then clears the flag, the port _will_ get powered off.

 For empty port, Only when PM Qos NO_POWER_OFF flag value is set to 0,
 the port will be power off.

Alan Stern


--
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 2/4] usb: introduce usb force power off mechanism

2013-03-29 Thread Lan Tianyu

On 2013/3/29 6:43, Sarah Sharp wrote:

On Thu, Mar 28, 2013 at 05:00:23PM -0400, Alan Stern wrote:

On Thu, 28 Mar 2013, Sarah Sharp wrote:


On Thu, Mar 28, 2013 at 01:11:02AM +0800, Lan Tianyu wrote:

Some devices' firmware will be broken at some points. Power down
and power on device can help device to rework in this case.

This patch is to add ioctl cmd USBDEVFS_POWER_RESET for usbfs node
to repower usb device. First, call hub_port_logical_disconnect() to
disconnect device. Second, Power down and up usb port.


I don't think this is the right approach.  We want to be able to power
off a port, even if no devices are attached.  One of the use case
scenarios we discussed was allowing distros to turn off empty USB ports
according to some policy (e.g. screen blank, lost bluetooth connection
to their phone that indicates the user walked away from the computer,
server admin wants to save power, etc).

With the current patches in 3.9, I don't see a way we can do that.  The
ports have power/control, which can only be set to 'on' or 'auto'.  You
should add an 'off' option to that file.


Won't empty ports naturally be powered off by runtime PM, so long as
there isn't a PM_QOS constraint to prevent it?  The user (or system
software) may need to write auto to the port's power/control file,
which may require us to put the ports on the usb_bus_type or to make
them class devices.  But however we do it, this should work
automatically.


Now I'm a bit confused and I'll have to look at the code again.  I can't
remember if Tianyu made the kernel add a PM_QOS constraint to set the no
power off flag for hot pluggable ports, or if we expect userspace to use
the connect type sysfs file to figure out it shouldn't write auto to the
file.

Actually, I exposed pm qos flags for usb port via dev_pm_qos_expose_flags().
It creates power/pm_qos_no_power_off under usb port sysfs directory.
User can echo 0  pm_qos_no_power_off to power off the empty port.



If the kernel isn't adding that constraint, then Tianyu *really* needs
to document that.

power/pm_qos_no_power_off is device's power attribute and it has been
described in the sysfs-devices-power. I think I should mention in the
usb/power-management.text.


Sarah Sharp



--
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 2/4] usb: introduce usb force power off mechanism

2013-03-29 Thread Alan Stern
On Fri, 29 Mar 2013, Lan Tianyu wrote:

 Actually, I exposed pm qos flags for usb port via dev_pm_qos_expose_flags().
 It creates power/pm_qos_no_power_off under usb port sysfs directory.
 User can echo 0  pm_qos_no_power_off to power off the empty port.

Before it's too late, we should consider changing the flag's name.  
Writing 0 to a file named pm_qos_no_power_off is a double negative, 
which is always awkward to think about.

Instead, how about calling the file pm_qos_allow_power_off?  Will the
PM-QOS system allow something like that, i.e., something that is the
opposite of a constraint?

If not, how about naming it pm_qos_keep_power_on?

Alan Stern

--
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 2/4] usb: introduce usb force power off mechanism

2013-03-29 Thread Sarah Sharp
On Fri, Mar 29, 2013 at 02:37:24PM +0800, Lan Tianyu wrote:
 On 2013/3/29 6:43, Sarah Sharp wrote:
 On Thu, Mar 28, 2013 at 05:00:23PM -0400, Alan Stern wrote:
 On Thu, 28 Mar 2013, Sarah Sharp wrote:
 On Thu, Mar 28, 2013 at 01:11:02AM +0800, Lan Tianyu wrote:
 Now I'm a bit confused and I'll have to look at the code again.  I can't
 remember if Tianyu made the kernel add a PM_QOS constraint to set the no
 power off flag for hot pluggable ports, or if we expect userspace to use
 the connect type sysfs file to figure out it shouldn't write auto to the
 file.
 Actually, I exposed pm qos flags for usb port via dev_pm_qos_expose_flags().
 It creates power/pm_qos_no_power_off under usb port sysfs directory.
 User can echo 0  pm_qos_no_power_off to power off the empty port.

Ugh.  I agree with Alan that the name should be pm_qos_keep_power_on.

However, what happens if you echo 0 to pm_qos_no_power_off, the
power/control is set to auto, and there's a suspended USB device
attached to the port with remote wakeup enabled?  Will the port be
powered off?  I don't think it will with the current patchset, but I
will have to double check.

Maybe you have an Android smartphone or tablet, the user pushes the
button to turn off the screen.  In that case, you want to physically
power off any USB devices, including ones that may be in USB device
suspend with remote wakeup on, like a USB touchscreen.

With the proposed patchset, you can do that through usbfs, but it feels
awkward to have the PM QOS constraints and the manual power off in two
separate places.  I think if we're adding the usbfs interface, we should
also add a manual 'off' to the port power/control file.

Sarah Sharp
--
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 2/4] usb: introduce usb force power off mechanism

2013-03-29 Thread Alan Stern
On Fri, 29 Mar 2013, Sarah Sharp wrote:

 However, what happens if you echo 0 to pm_qos_no_power_off, the
 power/control is set to auto, and there's a suspended USB device
 attached to the port with remote wakeup enabled?  Will the port be
 powered off?  I don't think it will with the current patchset, but I
 will have to double check.

It shouldn't be, because remote wakeup is enabled.  But what about the
case where remote wakeup is disabled?  Will writing a 0 to
pm_qos_no_power_off cause the power to be turned off?

Or what about the case where there's no device attached to the port?  I 
guess both questions amount to the same thing: When the user writes to 
a pm_qos_* file, does the code call pm_runtime_idle?

 Maybe you have an Android smartphone or tablet, the user pushes the
 button to turn off the screen.  In that case, you want to physically
 power off any USB devices, including ones that may be in USB device
 suspend with remote wakeup on, like a USB touchscreen.
 
 With the proposed patchset, you can do that through usbfs, but it feels
 awkward to have the PM QOS constraints and the manual power off in two
 separate places.  I think if we're adding the usbfs interface, we should
 also add a manual 'off' to the port power/control file.

That's not so easy to do.  power/control is owned by the PM core, not 
by USB.  From the PM core's perspective, power off is merely a 
variant of suspend.  The USB subsystem gets to choose which variant 
to use, based on various constraints that userspace has imposed.

In fact, if the user wants to, it is possible to set the port's pm_qos
file to allow power off and then manage the port's actual behavior
entirely by means of its power/control file.  I sort of think that 
would be a cleaner approach ... but there may have been some reason for 
using the separate pm_qos attribute; I can't remember.

Also, bear in mind that the proposed patch does not give userspace a 
way to power off ports via usbfs.  What the new code does is a 
power-off reset -- it turns off power to the port, waits a short time, 
and then turns power back on.

Alan Stern

--
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 2/4] usb: introduce usb force power off mechanism

2013-03-29 Thread Sarah Sharp
On Fri, Mar 29, 2013 at 01:23:14PM -0400, Alan Stern wrote:
 On Fri, 29 Mar 2013, Sarah Sharp wrote:
 
  However, what happens if you echo 0 to pm_qos_no_power_off, the
  power/control is set to auto, and there's a suspended USB device
  attached to the port with remote wakeup enabled?  Will the port be
  powered off?  I don't think it will with the current patchset, but I
  will have to double check.
 
 It shouldn't be, because remote wakeup is enabled.  But what about the
 case where remote wakeup is disabled?  Will writing a 0 to
 pm_qos_no_power_off cause the power to be turned off?
 
 Or what about the case where there's no device attached to the port?  I 
 guess both questions amount to the same thing: When the user writes to 
 a pm_qos_* file, does the code call pm_runtime_idle?

Tianyu?

  Maybe you have an Android smartphone or tablet, the user pushes the
  button to turn off the screen.  In that case, you want to physically
  power off any USB devices, including ones that may be in USB device
  suspend with remote wakeup on, like a USB touchscreen.
  
  With the proposed patchset, you can do that through usbfs, but it feels
  awkward to have the PM QOS constraints and the manual power off in two
  separate places.  I think if we're adding the usbfs interface, we should
  also add a manual 'off' to the port power/control file.
 
 That's not so easy to do.  power/control is owned by the PM core, not 
 by USB.  From the PM core's perspective, power off is merely a 
 variant of suspend.  The USB subsystem gets to choose which variant 
 to use, based on various constraints that userspace has imposed.

Hmm, that's unfortunate.

 In fact, if the user wants to, it is possible to set the port's pm_qos
 file to allow power off and then manage the port's actual behavior
 entirely by means of its power/control file.  I sort of think that 
 would be a cleaner approach ... but there may have been some reason for 
 using the separate pm_qos attribute; I can't remember.

I think the implementation would be cleaner as well.  I can't remember
why we don't have power/control set to 'auto' and
power/pm_qos_no_power_off set to '1' by default.  I think the argument
was about how to deal with multiple userspace programs that wanted to
prevent port power off?

It seems like you would want the opposite default (power/control set to
'on' and power/pm_qos_no_power_off set to '0').  That way, something
like a firmware loader can express they don't want to power off a device
by writing 1 to power/pm_qos_no_power_off, and programs like powertop
can respect those decisions by just setting power/control to 'auto'.
Right now, if powertop wanted to attempt to enable automatic port power
off, it would have to overwrite any preferences the previous userspace
programs expressed.

 Also, bear in mind that the proposed patch does not give userspace a 
 way to power off ports via usbfs.  What the new code does is a 
 power-off reset -- it turns off power to the port, waits a short time, 
 and then turns power back on.

I think we need two separate IOCTLs: turn off the port power and turn it
on.  Then we get the manual port power off for our QA testing, distros
can make interesting policies about manually powering off ports, and
userspace can choose how long they want the port to be off.  After all,
different devices may need a longer powered-off period than others.

Sarah Sharp
--
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 2/4] usb: introduce usb force power off mechanism

2013-03-29 Thread Alan Stern
On Fri, 29 Mar 2013, Sarah Sharp wrote:

  Also, bear in mind that the proposed patch does not give userspace a 
  way to power off ports via usbfs.  What the new code does is a 
  power-off reset -- it turns off power to the port, waits a short time, 
  and then turns power back on.
 
 I think we need two separate IOCTLs: turn off the port power and turn it
 on.  Then we get the manual port power off for our QA testing, distros
 can make interesting policies about manually powering off ports, and
 userspace can choose how long they want the port to be off.  After all,
 different devices may need a longer powered-off period than others.

Or one port-power IOCTL that takes the setting as an argument.  Yes, 
this would be more flexible than a power-off reset.

Alan Stern

--
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 2/4] usb: introduce usb force power off mechanism

2013-03-28 Thread Lan Tianyu

On 2013/3/28 2:45, Alan Stern wrote:

+int usb_hub_port_power_reset(struct usb_device *hdev, int port1)
+{
+   struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+   struct usb_interface *intf = to_usb_interface(hub-intfdev);
+   int ret;
+
+   usb_autopm_get_interface(intf);


What happens if hdev is NULL?

Yes, This will be a problem. The repower cmd to root
hub should be ignored.




+   hub_port_logical_disconnect(hub, port1);
+   ret = usb_hub_set_port_power(hdev, port1, false);


How long do you think the power should remain turned off?  This code
will leave it off for only a few milliseconds at most.  That may not
even be long enough for the voltage to drop all the way to 0.

The delay probably should be at least 100 ms.  Maybe more, I don't
know.

I think this depends on the device. I don't find the answer on spec.
I find the cool down delay of over-current for port is 100ms
and one for hub is 500ms. Could we reference these delay?
500ms would be safe.



+   ret |= usb_hub_set_port_power(hdev, port1, true);


Don't use |=.  Skip the second call if the first one fails.


OK.

+   usb_autopm_put_interface(intf);
+
+   return ret;
+}


If you placed this function later on in the source file, you wouldn't
need the forward declaration of hub_port_logical_disconnect().

Yes, I will do this.


Alan Stern



--
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 2/4] usb: introduce usb force power off mechanism

2013-03-28 Thread Alan Stern
On Thu, 28 Mar 2013, Lan Tianyu wrote:

  How long do you think the power should remain turned off?  This code
  will leave it off for only a few milliseconds at most.  That may not
  even be long enough for the voltage to drop all the way to 0.
 
  The delay probably should be at least 100 ms.  Maybe more, I don't
  know.
 I think this depends on the device. I don't find the answer on spec.
 I find the cool down delay of over-current for port is 100ms
 and one for hub is 500ms. Could we reference these delay?
 500ms would be safe.

Okay, then use 500 ms.

Alan Stern

--
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 2/4] usb: introduce usb force power off mechanism

2013-03-28 Thread Lan Tianyu

On 2013/3/28 22:46, Alan Stern wrote:

On Thu, 28 Mar 2013, Lan Tianyu wrote:


How long do you think the power should remain turned off?  This code
will leave it off for only a few milliseconds at most.  That may not
even be long enough for the voltage to drop all the way to 0.

The delay probably should be at least 100 ms.  Maybe more, I don't
know.

I think this depends on the device. I don't find the answer on spec.
I find the cool down delay of over-current for port is 100ms
and one for hub is 500ms. Could we reference these delay?
500ms would be safe.


Okay, then use 500 ms.

Alan Stern


Ok.

About the path usb: Add usb port system pm support, do you think it's
ok?

On 2013/3/28 2:47, Alan Stern wrote:

What happens if there's no device plugged in to the port, but the hub
is enabled for remote wakeup?  How will the hub be able to detect a
plug-in event if the port isn't powered?

Alan Stern


Hi Alan:
 Great thanks for your review.
 The hub will not detect the new devices. From my opinion, this
depends on the user space since the port only will be powered off when
pm qos NO_POWER_OFF flag is unset(it is default to be set). If unset
the flag, losing plug-in event should have been took into account.




--
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 2/4] usb: introduce usb force power off mechanism

2013-03-28 Thread Alan Stern
On Fri, 29 Mar 2013, Lan Tianyu wrote:

 About the path usb: Add usb port system pm support, do you think it's
 ok?

Generally yes.  But why doesn't usb_port_system_suspend check for any
PM_QOS constraints?  Either on the port itself or on the child device.

Alan Stern

--
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 2/4] usb: introduce usb force power off mechanism

2013-03-28 Thread Lan Tianyu

On 2013/3/29 0:50, Alan Stern wrote:

On Fri, 29 Mar 2013, Lan Tianyu wrote:


About the path usb: Add usb port system pm support, do you think it's
ok?


Generally yes.  But why doesn't usb_port_system_suspend check for any
PM_QOS constraints?  Either on the port itself or on the child device.

Because usb_port_runtime_suspend() will PM Qos flags. If add check in
usb_port_system_suspend(), PM Qos flags would be checked twice and
this seems redundant.


Alan Stern



--
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 2/4] usb: introduce usb force power off mechanism

2013-03-28 Thread Alan Stern
On Fri, 29 Mar 2013, Lan Tianyu wrote:

 On 2013/3/29 0:50, Alan Stern wrote:
  On Fri, 29 Mar 2013, Lan Tianyu wrote:
 
  About the path usb: Add usb port system pm support, do you think it's
  ok?
 
  Generally yes.  But why doesn't usb_port_system_suspend check for any
  PM_QOS constraints?  Either on the port itself or on the child device.
 Because usb_port_runtime_suspend() will PM Qos flags. If add check in
 usb_port_system_suspend(), PM Qos flags would be checked twice and
 this seems redundant.

Yes, that's right -- I noticed this the first time I read the patch and 
then forgot it again.

I don't see any other problems with that patch.

Alan Stern

--
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 2/4] usb: introduce usb force power off mechanism

2013-03-28 Thread Lan Tianyu

On 2013/3/29 1:49, Alan Stern wrote:

On Fri, 29 Mar 2013, Lan Tianyu wrote:


On 2013/3/29 0:50, Alan Stern wrote:

On Fri, 29 Mar 2013, Lan Tianyu wrote:


About the path usb: Add usb port system pm support, do you think it's
ok?


Generally yes.  But why doesn't usb_port_system_suspend check for any
PM_QOS constraints?  Either on the port itself or on the child device.

Because usb_port_runtime_suspend() will PM Qos flags. If add check in
usb_port_system_suspend(), PM Qos flags would be checked twice and
this seems redundant.


Yes, that's right -- I noticed this the first time I read the patch and
then forgot it again.

I don't see any other problems with that patch.

Alan Stern



Ok. I just refresh patch usb: introduce usb force power off mechanism
Please have a look.

From 16f5c7c6dd00830530a9ac758af25b575e0b8731 Mon Sep 17 00:00:00 2001
From: Lan Tianyu tianyu@intel.com
Date: Tue, 26 Feb 2013 11:12:09 +0800
Subject: [PATCH] usb: introduce usb force power off mechanism

Some devices' firmware will be broken at some points. Power down
and power on device can help device to rework in this case.

This patch is to add ioctl cmd USBDEVFS_POWER_RESET for usbfs node
to repower usb device. First, call hub_port_logical_disconnect() to
disconnect device. Second, Power down and up usb port.

This patch is also helpful fo some QAs who want to do hcd's memleak
test(Plug and unplug device thousand times.)

Signed-off-by: Lan Tianyu tianyu@intel.com
---
 drivers/usb/core/devio.c  |   16 
 drivers/usb/core/hub.c|   27 +++
 drivers/usb/core/usb.h|2 ++
 include/uapi/linux/usbdevice_fs.h |1 +
 4 files changed, 46 insertions(+)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 8823e98..951e904 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1102,6 +1102,17 @@ static int proc_resetdevice(struct dev_state *ps)
return usb_reset_device(ps-dev);
 }

+static int proc_resetdevicepower(struct dev_state *ps)
+{
+   struct usb_device *udev = ps-dev;
+   struct usb_device *hdev = udev-parent;
+
+   if (!hdev)
+   return -EINVAL;
+
+   return usb_hub_port_power_reset(hdev, udev-portnum);
+}
+
 static int proc_setintf(struct dev_state *ps, void __user *arg)
 {
struct usbdevfs_setinterface setintf;
@@ -2011,6 +2022,11 @@ static long usbdev_do_ioctl(struct file *file, unsigned 
int cmd,
ret = proc_resetdevice(ps);
break;

+   case USBDEVFS_POWER_RESET:
+   snoop(dev-dev, %s: RESET POWER\n, __func__);
+   ret = proc_resetdevicepower(ps);
+   break;
+
case USBDEVFS_CLEAR_HALT:
snoop(dev-dev, %s: CLEAR_HALT\n, __func__);
ret = proc_clearhalt(ps, p);
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 6b7f5b9..9729e36 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -948,6 +948,33 @@ static void hub_port_logical_disconnect(struct usb_hub 
*hub, int port1)
 }

 /**
+ * usb_hub_port_power_reset - repower hub port
+ * @hdev: USB device belonging to the usb hub
+ * @port1: port num to be repowered.
+ *
+ * This routine will try to disconnect the device on
+ * the port and then repower the port.
+ */
+int usb_hub_port_power_reset(struct usb_device *hdev, int port1)
+{
+   struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+   struct usb_interface *intf = to_usb_interface(hub-intfdev);
+   int ret;
+
+   usb_autopm_get_interface(intf);
+   hub_port_logical_disconnect(hub, port1);
+   ret = usb_hub_set_port_power(hdev, port1, false);
+   if (!ret) {
+   /* Wait for power down device */
+   msleep(500);
+   ret = usb_hub_set_port_power(hdev, port1, true);
+   }
+   usb_autopm_put_interface(intf);
+
+   return ret;
+}
+
+/**
  * usb_remove_device - disable a device's port on its parent hub
  * @udev: device to be disabled and removed
  * Context: @udev locked, must be able to sleep.
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
index a7f20bd..4e7785c 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -185,6 +185,8 @@ extern void usb_set_hub_port_connect_type(struct usb_device 
*hdev, int port1,
enum usb_port_connect_type type);
 extern void usb_hub_adjust_deviceremovable(struct usb_device *hdev,
struct usb_hub_descriptor *desc);
+extern int usb_hub_port_power_reset(struct usb_device *hdev,
+   int port1);

 #ifdef CONFIG_ACPI
 extern int usb_acpi_register(void);
diff --git a/include/uapi/linux/usbdevice_fs.h 
b/include/uapi/linux/usbdevice_fs.h
index 0c65e4b..b6e0d17 100644
--- a/include/uapi/linux/usbdevice_fs.h
+++ b/include/uapi/linux/usbdevice_fs.h
@@ -176,5 +176,6 @@ struct usbdevfs_disconnect_claim {
 #define USBDEVFS_RELEASE_PORT  _IOR('U', 25, unsigned int)
 #define 

Re: [PATCH 2/4] usb: introduce usb force power off mechanism

2013-03-28 Thread Alan Stern
On Fri, 29 Mar 2013, Lan Tianyu wrote:

 Ok. I just refresh patch usb: introduce usb force power off mechanism
 Please have a look.
 
  From 16f5c7c6dd00830530a9ac758af25b575e0b8731 Mon Sep 17 00:00:00 2001
 From: Lan Tianyu tianyu@intel.com
 Date: Tue, 26 Feb 2013 11:12:09 +0800
 Subject: [PATCH] usb: introduce usb force power off mechanism
 
 Some devices' firmware will be broken at some points. Power down
 and power on device can help device to rework in this case.
 
 This patch is to add ioctl cmd USBDEVFS_POWER_RESET for usbfs node
 to repower usb device. First, call hub_port_logical_disconnect() to
 disconnect device. Second, Power down and up usb port.
 
 This patch is also helpful fo some QAs who want to do hcd's memleak
 test(Plug and unplug device thousand times.)
 
 Signed-off-by: Lan Tianyu tianyu@intel.com

...

 --- a/drivers/usb/core/usb.h
 +++ b/drivers/usb/core/usb.h
 @@ -185,6 +185,8 @@ extern void usb_set_hub_port_connect_type(struct 
 usb_device *hdev, int port1,
   enum usb_port_connect_type type);
   extern void usb_hub_adjust_deviceremovable(struct usb_device *hdev,
   struct usb_hub_descriptor *desc);
 +extern int usb_hub_port_power_reset(struct usb_device *hdev,
 + int port1);

This can all go on one line.


It looks okay.  When you test it, does the attached device get detected 
and initialized properly?

Alan Stern

--
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 2/4] usb: introduce usb force power off mechanism

2013-03-28 Thread Lan Tianyu

On 2013/3/29 3:38, Alan Stern wrote:

On Fri, 29 Mar 2013, Lan Tianyu wrote:


Ok. I just refresh patch usb: introduce usb force power off mechanism
Please have a look.

  From 16f5c7c6dd00830530a9ac758af25b575e0b8731 Mon Sep 17 00:00:00 2001
From: Lan Tianyu tianyu@intel.com
Date: Tue, 26 Feb 2013 11:12:09 +0800
Subject: [PATCH] usb: introduce usb force power off mechanism

Some devices' firmware will be broken at some points. Power down
and power on device can help device to rework in this case.

This patch is to add ioctl cmd USBDEVFS_POWER_RESET for usbfs node
to repower usb device. First, call hub_port_logical_disconnect() to
disconnect device. Second, Power down and up usb port.

This patch is also helpful fo some QAs who want to do hcd's memleak
test(Plug and unplug device thousand times.)

Signed-off-by: Lan Tianyu tianyu@intel.com


...


--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -185,6 +185,8 @@ extern void usb_set_hub_port_connect_type(struct usb_device 
*hdev, int port1,
enum usb_port_connect_type type);
   extern void usb_hub_adjust_deviceremovable(struct usb_device *hdev,
struct usb_hub_descriptor *desc);
+extern int usb_hub_port_power_reset(struct usb_device *hdev,
+   int port1);


This can all go on one line.


It looks okay.  When you test it, does the attached device get detected
and initialized properly?

I test usb2.0 key on my machine. It works.


Alan Stern



--
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 2/4] usb: introduce usb force power off mechanism

2013-03-28 Thread Sarah Sharp
On Thu, Mar 28, 2013 at 01:11:02AM +0800, Lan Tianyu wrote:
 Some devices' firmware will be broken at some points. Power down
 and power on device can help device to rework in this case.
 
 This patch is to add ioctl cmd USBDEVFS_POWER_RESET for usbfs node
 to repower usb device. First, call hub_port_logical_disconnect() to
 disconnect device. Second, Power down and up usb port.

I don't think this is the right approach.  We want to be able to power
off a port, even if no devices are attached.  One of the use case
scenarios we discussed was allowing distros to turn off empty USB ports
according to some policy (e.g. screen blank, lost bluetooth connection
to their phone that indicates the user walked away from the computer,
server admin wants to save power, etc).

With the current patches in 3.9, I don't see a way we can do that.  The
ports have power/control, which can only be set to 'on' or 'auto'.  You
should add an 'off' option to that file.

I don't particularly care if usbfs gets a new port power ioctl, I just
want the port power sysfs files to allow power off of an empty port.

 This patch is also helpful fo some QAs who want to do hcd's memleak
 test(Plug and unplug device thousand times.)

What we'd like to also do in our internal Intel QA is measure the USB
host power, manually power off all ports, and ensure the host power
drops.  That way we can be sure the BIOS and your ACPI code is working
properly.

Sarah Sharp

 
 Signed-off-by: Lan Tianyu tianyu@intel.com
 ---
  drivers/usb/core/devio.c  |   13 +
  drivers/usb/core/hub.c|   16 
  drivers/usb/core/usb.h|2 ++
  include/uapi/linux/usbdevice_fs.h |1 +
  4 files changed, 32 insertions(+)
 
 diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
 index 8823e98..a76b326 100644
 --- a/drivers/usb/core/devio.c
 +++ b/drivers/usb/core/devio.c
 @@ -1102,6 +1102,14 @@ static int proc_resetdevice(struct dev_state *ps)
   return usb_reset_device(ps-dev);
  }
  
 +static int proc_resetdevicepower(struct dev_state *ps)
 +{
 + struct usb_device *udev = ps-dev;
 + struct usb_device *hdev = udev-parent;
 +
 + return usb_hub_port_power_reset(hdev, udev-portnum);
 +}
 +
  static int proc_setintf(struct dev_state *ps, void __user *arg)
  {
   struct usbdevfs_setinterface setintf;
 @@ -2011,6 +2019,11 @@ static long usbdev_do_ioctl(struct file *file, 
 unsigned int cmd,
   ret = proc_resetdevice(ps);
   break;
  
 + case USBDEVFS_POWER_RESET:
 + snoop(dev-dev, %s: RESET POWER\n, __func__);
 + ret = proc_resetdevicepower(ps);
 + break;
 +
   case USBDEVFS_CLEAR_HALT:
   snoop(dev-dev, %s: CLEAR_HALT\n, __func__);
   ret = proc_clearhalt(ps, p);
 diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
 index 6b7f5b9..c228e69 100644
 --- a/drivers/usb/core/hub.c
 +++ b/drivers/usb/core/hub.c
 @@ -114,6 +114,7 @@ EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
  #define HUB_DEBOUNCE_STABLE   100
  
  static int usb_reset_and_verify_device(struct usb_device *udev);
 +static void hub_port_logical_disconnect(struct usb_hub *hub, int port1);
  
  static inline char *portspeed(struct usb_hub *hub, int portstatus)
  {
 @@ -741,6 +742,21 @@ int usb_hub_set_port_power(struct usb_device *hdev, int 
 port1,
   return ret;
  }
  
 +int usb_hub_port_power_reset(struct usb_device *hdev, int port1)
 +{
 + struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
 + struct usb_interface *intf = to_usb_interface(hub-intfdev);
 + int ret;
 +
 + usb_autopm_get_interface(intf);
 + hub_port_logical_disconnect(hub, port1);
 + ret = usb_hub_set_port_power(hdev, port1, false);
 + ret |= usb_hub_set_port_power(hdev, port1, true);
 + usb_autopm_put_interface(intf);
 +
 + return ret;
 +}
 +
  /**
   * usb_hub_clear_tt_buffer - clear control/bulk TT state in high speed hub
   * @urb: an URB associated with the failed or incomplete split transaction
 diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h
 index a7f20bd..4e7785c 100644
 --- a/drivers/usb/core/usb.h
 +++ b/drivers/usb/core/usb.h
 @@ -185,6 +185,8 @@ extern void usb_set_hub_port_connect_type(struct 
 usb_device *hdev, int port1,
   enum usb_port_connect_type type);
  extern void usb_hub_adjust_deviceremovable(struct usb_device *hdev,
   struct usb_hub_descriptor *desc);
 +extern int usb_hub_port_power_reset(struct usb_device *hdev,
 + int port1);
  
  #ifdef CONFIG_ACPI
  extern int usb_acpi_register(void);
 diff --git a/include/uapi/linux/usbdevice_fs.h 
 b/include/uapi/linux/usbdevice_fs.h
 index 0c65e4b..b6e0d17 100644
 --- a/include/uapi/linux/usbdevice_fs.h
 +++ b/include/uapi/linux/usbdevice_fs.h
 @@ -176,5 +176,6 @@ struct usbdevfs_disconnect_claim {
  #define USBDEVFS_RELEASE_PORT  _IOR('U', 25, unsigned int)
  #define USBDEVFS_GET_CAPABILITIES  _IOR('U', 26, 

Re: [PATCH 2/4] usb: introduce usb force power off mechanism

2013-03-28 Thread Sarah Sharp
On Fri, Mar 29, 2013 at 03:51:50AM +0800, Lan Tianyu wrote:
 On 2013/3/29 3:38, Alan Stern wrote:
 On Fri, 29 Mar 2013, Lan Tianyu wrote:
 It looks okay.  When you test it, does the attached device get detected
 and initialized properly?
 I test usb2.0 key on my machine. It works.

Did you test USB 3.0 as well?

Sarah Sharp
--
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 2/4] usb: introduce usb force power off mechanism

2013-03-28 Thread Alan Stern
On Thu, 28 Mar 2013, Sarah Sharp wrote:

 On Thu, Mar 28, 2013 at 01:11:02AM +0800, Lan Tianyu wrote:
  Some devices' firmware will be broken at some points. Power down
  and power on device can help device to rework in this case.
  
  This patch is to add ioctl cmd USBDEVFS_POWER_RESET for usbfs node
  to repower usb device. First, call hub_port_logical_disconnect() to
  disconnect device. Second, Power down and up usb port.
 
 I don't think this is the right approach.  We want to be able to power
 off a port, even if no devices are attached.  One of the use case
 scenarios we discussed was allowing distros to turn off empty USB ports
 according to some policy (e.g. screen blank, lost bluetooth connection
 to their phone that indicates the user walked away from the computer,
 server admin wants to save power, etc).
 
 With the current patches in 3.9, I don't see a way we can do that.  The
 ports have power/control, which can only be set to 'on' or 'auto'.  You
 should add an 'off' option to that file.

Won't empty ports naturally be powered off by runtime PM, so long as 
there isn't a PM_QOS constraint to prevent it?  The user (or system 
software) may need to write auto to the port's power/control file, 
which may require us to put the ports on the usb_bus_type or to make 
them class devices.  But however we do it, this should work 
automatically.

The mechanism Tianyu is adding here is meant for testing and other 
types of manual intervention, not for normal operation.

Alan Stern

--
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 2/4] usb: introduce usb force power off mechanism

2013-03-28 Thread Sarah Sharp
On Thu, Mar 28, 2013 at 05:00:23PM -0400, Alan Stern wrote:
 On Thu, 28 Mar 2013, Sarah Sharp wrote:
 
  On Thu, Mar 28, 2013 at 01:11:02AM +0800, Lan Tianyu wrote:
   Some devices' firmware will be broken at some points. Power down
   and power on device can help device to rework in this case.
   
   This patch is to add ioctl cmd USBDEVFS_POWER_RESET for usbfs node
   to repower usb device. First, call hub_port_logical_disconnect() to
   disconnect device. Second, Power down and up usb port.
  
  I don't think this is the right approach.  We want to be able to power
  off a port, even if no devices are attached.  One of the use case
  scenarios we discussed was allowing distros to turn off empty USB ports
  according to some policy (e.g. screen blank, lost bluetooth connection
  to their phone that indicates the user walked away from the computer,
  server admin wants to save power, etc).
  
  With the current patches in 3.9, I don't see a way we can do that.  The
  ports have power/control, which can only be set to 'on' or 'auto'.  You
  should add an 'off' option to that file.
 
 Won't empty ports naturally be powered off by runtime PM, so long as 
 there isn't a PM_QOS constraint to prevent it?  The user (or system 
 software) may need to write auto to the port's power/control file, 
 which may require us to put the ports on the usb_bus_type or to make 
 them class devices.  But however we do it, this should work 
 automatically.

Now I'm a bit confused and I'll have to look at the code again.  I can't
remember if Tianyu made the kernel add a PM_QOS constraint to set the no
power off flag for hot pluggable ports, or if we expect userspace to use
the connect type sysfs file to figure out it shouldn't write auto to the
file.

If the kernel isn't adding that constraint, then Tianyu *really* needs
to document that.

Sarah Sharp
--
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 2/4] usb: introduce usb force power off mechanism

2013-03-27 Thread Lan, Tianyu
A small tool to test this patch.

#include stdio.h
#include unistd.h
#include fcntl.h
#include errno.h
#include sys/ioctl.h
#include linux/usbdevice_fs.h

#define USBDEVFS_POWER_RESET   _IO('U', 28)

int main(int argc, char **argv)
{
const char *filename;
int fd;
int rc;

if (argc != 2) {
fprintf(stderr, Usage: usbreset device-filename\n);
return 1;
}
filename = argv[1];

fd = open(filename, O_WRONLY);
if (fd  0) {
perror(Error opening output file);
return 1;
}

printf(Repowering USB device %s\n, filename);
rc = ioctl(fd, USBDEVFS_POWER_RESET, 0);
if (rc  0) {
perror(Error in ioctl);
return 1;
}
printf(Repower successful\n);

close(fd);
return 0;
}


Best Regards 
Tianyu Lan 


-Original Message-
From: Lan, Tianyu 
Sent: Thursday, March 28, 2013 1:11 AM
To: gre...@linuxfoundation.org; sarah.a.sh...@linux.intel.com; 
st...@rowland.harvard.edu
Cc: linux-usb@vger.kernel.org; Lan, Tianyu
Subject: [PATCH 2/4] usb: introduce usb force power off mechanism

Some devices' firmware will be broken at some points. Power down and power on 
device can help device to rework in this case.

This patch is to add ioctl cmd USBDEVFS_POWER_RESET for usbfs node to repower 
usb device. First, call hub_port_logical_disconnect() to disconnect device. 
Second, Power down and up usb port.

This patch is also helpful fo some QAs who want to do hcd's memleak test(Plug 
and unplug device thousand times.)

Signed-off-by: Lan Tianyu tianyu@intel.com
---
 drivers/usb/core/devio.c  |   13 +
 drivers/usb/core/hub.c|   16 
 drivers/usb/core/usb.h|2 ++
 include/uapi/linux/usbdevice_fs.h |1 +
 4 files changed, 32 insertions(+)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c index 
8823e98..a76b326 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1102,6 +1102,14 @@ static int proc_resetdevice(struct dev_state *ps)
return usb_reset_device(ps-dev);
 }
 
+static int proc_resetdevicepower(struct dev_state *ps) {
+   struct usb_device *udev = ps-dev;
+   struct usb_device *hdev = udev-parent;
+
+   return usb_hub_port_power_reset(hdev, udev-portnum); }
+
 static int proc_setintf(struct dev_state *ps, void __user *arg)  {
struct usbdevfs_setinterface setintf;
@@ -2011,6 +2019,11 @@ static long usbdev_do_ioctl(struct file *file, unsigned 
int cmd,
ret = proc_resetdevice(ps);
break;
 
+   case USBDEVFS_POWER_RESET:
+   snoop(dev-dev, %s: RESET POWER\n, __func__);
+   ret = proc_resetdevicepower(ps);
+   break;
+
case USBDEVFS_CLEAR_HALT:
snoop(dev-dev, %s: CLEAR_HALT\n, __func__);
ret = proc_clearhalt(ps, p);
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 
6b7f5b9..c228e69 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -114,6 +114,7 @@ EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
 #define HUB_DEBOUNCE_STABLE 100
 
 static int usb_reset_and_verify_device(struct usb_device *udev);
+static void hub_port_logical_disconnect(struct usb_hub *hub, int 
+port1);
 
 static inline char *portspeed(struct usb_hub *hub, int portstatus)  { @@ 
-741,6 +742,21 @@ int usb_hub_set_port_power(struct usb_device *hdev, int port1,
return ret;
 }
 
+int usb_hub_port_power_reset(struct usb_device *hdev, int port1) {
+   struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
+   struct usb_interface *intf = to_usb_interface(hub-intfdev);
+   int ret;
+
+   usb_autopm_get_interface(intf);
+   hub_port_logical_disconnect(hub, port1);
+   ret = usb_hub_set_port_power(hdev, port1, false);
+   ret |= usb_hub_set_port_power(hdev, port1, true);
+   usb_autopm_put_interface(intf);
+
+   return ret;
+}
+
 /**
  * usb_hub_clear_tt_buffer - clear control/bulk TT state in high speed hub
  * @urb: an URB associated with the failed or incomplete split transaction 
diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h index 
a7f20bd..4e7785c 100644
--- a/drivers/usb/core/usb.h
+++ b/drivers/usb/core/usb.h
@@ -185,6 +185,8 @@ extern void usb_set_hub_port_connect_type(struct usb_device 
*hdev, int port1,
enum usb_port_connect_type type);
 extern void usb_hub_adjust_deviceremovable(struct usb_device *hdev,
struct usb_hub_descriptor *desc);
+extern int usb_hub_port_power_reset(struct usb_device *hdev,
+   int port1);
 
 #ifdef CONFIG_ACPI
 extern int usb_acpi_register(void);
diff --git a/include/uapi/linux/usbdevice_fs.h 
b/include/uapi/linux/usbdevice_fs.h
index 0c65e4b..b6e0d17 100644
--- a/include/uapi/linux/usbdevice_fs.h
+++ b/include/uapi/linux/usbdevice_fs.h
@@ -176,5 +176,6 @@ 

Re: [PATCH 2/4] usb: introduce usb force power off mechanism

2013-03-27 Thread Alan Stern
On Thu, 28 Mar 2013, Lan Tianyu wrote:

 Some devices' firmware will be broken at some points. Power down
 and power on device can help device to rework in this case.
 
 This patch is to add ioctl cmd USBDEVFS_POWER_RESET for usbfs node
 to repower usb device. First, call hub_port_logical_disconnect() to
 disconnect device. Second, Power down and up usb port.
 
 This patch is also helpful fo some QAs who want to do hcd's memleak
 test(Plug and unplug device thousand times.)

This is not a bad idea, but it needs a little more work...

 --- a/drivers/usb/core/hub.c
 +++ b/drivers/usb/core/hub.c
 @@ -114,6 +114,7 @@ EXPORT_SYMBOL_GPL(ehci_cf_port_reset_rwsem);
  #define HUB_DEBOUNCE_STABLE   100
  
  static int usb_reset_and_verify_device(struct usb_device *udev);
 +static void hub_port_logical_disconnect(struct usb_hub *hub, int port1);
  
  static inline char *portspeed(struct usb_hub *hub, int portstatus)
  {
 @@ -741,6 +742,21 @@ int usb_hub_set_port_power(struct usb_device *hdev, int 
 port1,
   return ret;
  }
  
 +int usb_hub_port_power_reset(struct usb_device *hdev, int port1)
 +{
 + struct usb_hub *hub = usb_hub_to_struct_hub(hdev);
 + struct usb_interface *intf = to_usb_interface(hub-intfdev);
 + int ret;
 +
 + usb_autopm_get_interface(intf);

What happens if hdev is NULL?

 + hub_port_logical_disconnect(hub, port1);
 + ret = usb_hub_set_port_power(hdev, port1, false);

How long do you think the power should remain turned off?  This code
will leave it off for only a few milliseconds at most.  That may not
even be long enough for the voltage to drop all the way to 0.

The delay probably should be at least 100 ms.  Maybe more, I don't 
know.

 + ret |= usb_hub_set_port_power(hdev, port1, true);

Don't use |=.  Skip the second call if the first one fails.

 + usb_autopm_put_interface(intf);
 +
 + return ret;
 +}

If you placed this function later on in the source file, you wouldn't 
need the forward declaration of hub_port_logical_disconnect().

Alan Stern

--
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 2/4] usb: introduce usb force power off mechanism

2013-03-27 Thread Alan Stern
On Wed, 27 Mar 2013, Lan, Tianyu wrote:

 A small tool to test this patch.
 
 #include stdio.h
 #include unistd.h
 #include fcntl.h
 #include errno.h
 #include sys/ioctl.h
 #include linux/usbdevice_fs.h
 
 #define USBDEVFS_POWER_RESET _IO('U', 28)
 
 int main(int argc, char **argv)
 {
   const char *filename;
   int fd;
   int rc;
 
   if (argc != 2) {
   fprintf(stderr, Usage: usbreset device-filename\n);

The name should be usbrepower (or usb-repower, which is more readable).  
Not usbreset -- there's already a different program using that name.

Alan Stern

--
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