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