Re: [Resend PATCH V4 0/10] usb: usb port power off mechanism anc expose usb port connect type
On 2013年01月16日 23:45, Alan Stern wrote:
On Tue, 15 Jan 2013, Lan Tianyu wrote:
Hi GregAlan:
Do you have some more comments about this patchset? Thanks.
I don't have any more comments at this point. It looks okay to me.
Acked-by: Alan Stern st...@rowland.harvard.edu
By the way, have you checked whether the auto-power-off mechanism works
correctly when you do a system suspend?
Thanks for reminder. I test this today and find an issue. If usb device
was powered off during runtime, pm_runtime_get_sync() in the
usb_port_resume() will return -EACCES when do system resume. This is
because port's runtime pm was disabled during system suspend. Following
are some log. The device's runtime pm will be enabled after system
suspend. The port device is advance to be inserted in the dpm_list than
usb device(port device is created before creating usb device). So the
resume sequence is that resume usb device firstly and then usb port. In
the result, pm_runtime_get_sync() doesn't work.
[ 70.448028] power LNXPOWER:03: driver resume
[ 70.448029] usb usb4: runtime enable
[ 70.448031] usb 3-1: can't resume usb port, status -13
[ 70.448032] usb usb4: type resume
[ 70.448039] dpm_run_callback(): usb_dev_resume+0x0/0x15 returns -13
[ 70.448040] usb usb4: usb resume
[ 70.448041] usb 3-2: can't resume usb port, status -13
[ 70.448043] PM: Device 3-1 failed to resume async: error -13
[ 70.448047] dpm_run_callback(): usb_dev_resume+0x0/0x15 returns -13
[ 70.448048] PM: Device 3-2 failed to resume async: error -13
[ 70.448056] hub 4-0:1.0: hub_resume
[ 70.448088] acpi device:49: runtime enable
I found one solution of adding pm_runtime_enable/disable() in the
usb_port_resume(). This is simple sovlution.
if (port_dev-did_runtime_put) {
if (!PMSG_IS_AUTO(msg)) {
pm_runtime_enable(port_dev-dev);
status = pm_runtime_get_sync(port_dev-dev);
pm_runtime_disable(port_dev-dev);
} else
status = pm_runtime_get_sync(port_dev-dev);
port_dev-did_runtime_put = false;
if (status 0) {
dev_dbg(udev-dev, can't resume usb port, status %d\n,
status);
return status;
}
}
This can work but I am not sure that it is safe to do system resume() in
the system resume().
Another proposal is to export usb_port_runtime_resume() or do a wrapper.
Call it in the usb_port_resume() with pm_runtime_get_noresume() and
pm_runtime_set_active(). This also work.
if (port_dev-did_runtime_put) {
if (!PMSG_IS_AUTO(msg)) {
status = usb_port_runtime_resume(port_dev-dev);
pm_runtime_get_noresume(port_dev-dev);
pm_runtime_set_active(udev-dev);
} else
status = pm_runtime_get_sync(port_dev-dev);
port_dev-did_runtime_put = false;
if (status 0) {
dev_dbg(udev-dev, can't resume usb port, status %d\n,
status);
return status;
}
}
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: [Resend PATCH V4 0/10] usb: usb port power off mechanism anc expose usb port connect type
On Fri, 18 Jan 2013, Lan Tianyu wrote:
By the way, have you checked whether the auto-power-off mechanism works
correctly when you do a system suspend?
Thanks for reminder. I test this today and find an issue. If usb device
was powered off during runtime, pm_runtime_get_sync() in the
usb_port_resume() will return -EACCES when do system resume. This is
because port's runtime pm was disabled during system suspend.
Actually it's worse than that. The PM layer assumes that devices are
brought back to full power during system resume. But that's not true
for your usb_port devices, because they don't have a resume callback.
In addition, we need to enable async PM for the usb_ports, just like
everything else in the USB stack. This means you have to call
device_enable_async_suspend before the port is registered.
Following
are some log. The device's runtime pm will be enabled after system
suspend. The port device is advance to be inserted in the dpm_list than
usb device(port device is created before creating usb device). So the
resume sequence is that resume usb device firstly and then usb port. In
the result, pm_runtime_get_sync() doesn't work.
When async is enabled, there won't be any defined ordering. We will
have to enforce the proper ordering by hand. This means
usb_resume_device will have to call device_pm_wait_for_dev for the port
device (it already does this for the companion root hub). In fact, the
code to do this waiting probably should be moved to usb_resume, because
it applies only to system resume, not to runtime resume.
[ 70.448028] power LNXPOWER:03: driver resume
[ 70.448029] usb usb4: runtime enable
[ 70.448031] usb 3-1: can't resume usb port, status -13
[ 70.448032] usb usb4: type resume
[ 70.448039] dpm_run_callback(): usb_dev_resume+0x0/0x15 returns -13
[ 70.448040] usb usb4: usb resume
[ 70.448041] usb 3-2: can't resume usb port, status -13
[ 70.448043] PM: Device 3-1 failed to resume async: error -13
[ 70.448047] dpm_run_callback(): usb_dev_resume+0x0/0x15 returns -13
[ 70.448048] PM: Device 3-2 failed to resume async: error -13
[ 70.448056] hub 4-0:1.0: hub_resume
[ 70.448088] acpi device:49: runtime enable
I found one solution of adding pm_runtime_enable/disable() in the
usb_port_resume(). This is simple sovlution.
if (port_dev-did_runtime_put) {
if (!PMSG_IS_AUTO(msg)) {
pm_runtime_enable(port_dev-dev);
status = pm_runtime_get_sync(port_dev-dev);
pm_runtime_disable(port_dev-dev);
} else
status = pm_runtime_get_sync(port_dev-dev);
port_dev-did_runtime_put = false;
if (status 0) {
dev_dbg(udev-dev, can't resume usb port, status %d\n,
status);
return status;
}
}
This can work but I am not sure that it is safe to do system resume() in
the system resume().
Another proposal is to export usb_port_runtime_resume() or do a wrapper.
Call it in the usb_port_resume() with pm_runtime_get_noresume() and
pm_runtime_set_active(). This also work.
if (port_dev-did_runtime_put) {
if (!PMSG_IS_AUTO(msg)) {
status = usb_port_runtime_resume(port_dev-dev);
pm_runtime_get_noresume(port_dev-dev);
pm_runtime_set_active(udev-dev);
} else
status = pm_runtime_get_sync(port_dev-dev);
port_dev-did_runtime_put = false;
if (status 0) {
dev_dbg(udev-dev, can't resume usb port, status %d\n,
status);
return status;
}
}
No, neither of these. The right approach is to define a new
usb_port_system_resume routine. It should call usb_port_runtime_resume
directly and set the runtime PM status back to ACTIVE, similar to what
usb_resume() does. In the dev_pm_ops structure, both the resume and
the restore callbacks should point to this new routine.
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: [Resend PATCH V4 0/10] usb: usb port power off mechanism anc expose usb port connect type
HI Alan:
I just find Rafael's patch has resolved this issue. In this
patch, enable runtime PM
right after executing subsystem/driver .resume_early() callbacks. When do
resume(),
the device's runtime pm has been enabled. This patch now is already in the
v3.8-rc4.
So this patchset will not cause problem with Rafael patch and I have tested.
About the
port system suspend/resume() callback, Could I do this in the next step?
commit a8d3848dadc2fd41a9117cb08dc04fe6d7c551f9
Author: Rafael J. Wysocki rafael.j.wyso...@intel.com
Date: Sat Dec 22 23:59:01 2012 +0100
PM: Move disabling/enabling runtime PM to late suspend/early resume
Currently, the PM core disables runtime PM for all devices right
after executing subsystem/driver .suspend() callbacks for them
and re-enables it right before executing subsystem/driver .resume()
callbacks for them. This may lead to problems when there are
two devices such that the .suspend() callback executed for one of
them depends on runtime PM working for the other. In that case,
if runtime PM has already been disabled for the second device,
the first one's .suspend() won't work correctly (and analogously
for resume).
To make those issues go away, make the PM core disable runtime PM
for devices right before executing subsystem/driver .suspend_late()
callbacks for them and enable runtime PM for them right after
executing subsystem/driver .resume_early() callbacks for them. This
way the potential conflitcs between .suspend_late()/.resume_early()
and their runtime PM counterparts are still prevented from happening,
but the subtle ordering issues related to disabling/enabling runtime
PM for devices during system suspend/resume are much easier to avoid.
Best Regards
Tianyu Lan
-Original Message-
From: Alan Stern [mailto:st...@rowland.harvard.edu]
Sent: Saturday, January 19, 2013 12:21 AM
To: Lan, Tianyu
Cc: r...@sisk.pl; gre...@linuxfoundation.org; sarah.a.sh...@linux.intel.com;
oneu...@suse.de; linux-usb@vger.kernel.org
Subject: Re: [Resend PATCH V4 0/10] usb: usb port power off mechanism anc
expose usb port connect type
On Fri, 18 Jan 2013, Lan Tianyu wrote:
By the way, have you checked whether the auto-power-off mechanism
works correctly when you do a system suspend?
Thanks for reminder. I test this today and find an issue. If usb
device was powered off during runtime, pm_runtime_get_sync() in the
usb_port_resume() will return -EACCES when do system resume. This is
because port's runtime pm was disabled during system suspend.
Actually it's worse than that. The PM layer assumes that devices are brought
back to full power during system resume. But that's not true for your usb_port
devices, because they don't have a resume callback.
In addition, we need to enable async PM for the usb_ports, just like everything
else in the USB stack. This means you have to call device_enable_async_suspend
before the port is registered.
Following
are some log. The device's runtime pm will be enabled after system
suspend. The port device is advance to be inserted in the dpm_list
than usb device(port device is created before creating usb device). So
the resume sequence is that resume usb device firstly and then usb
port. In the result, pm_runtime_get_sync() doesn't work.
When async is enabled, there won't be any defined ordering. We will have to
enforce the proper ordering by hand. This means usb_resume_device will have to
call device_pm_wait_for_dev for the port device (it already does this for the
companion root hub). In fact, the code to do this waiting probably should be
moved to usb_resume, because it applies only to system resume, not to runtime
resume.
[ 70.448028] power LNXPOWER:03: driver resume
[ 70.448029] usb usb4: runtime enable
[ 70.448031] usb 3-1: can't resume usb port, status -13
[ 70.448032] usb usb4: type resume
[ 70.448039] dpm_run_callback(): usb_dev_resume+0x0/0x15 returns -13
[ 70.448040] usb usb4: usb resume
[ 70.448041] usb 3-2: can't resume usb port, status -13
[ 70.448043] PM: Device 3-1 failed to resume async: error -13
[ 70.448047] dpm_run_callback(): usb_dev_resume+0x0/0x15 returns -13
[ 70.448048] PM: Device 3-2 failed to resume async: error -13
[ 70.448056] hub 4-0:1.0: hub_resume
[ 70.448088] acpi device:49: runtime enable
I found one solution of adding pm_runtime_enable/disable() in the
usb_port_resume(). This is simple sovlution.
if (port_dev-did_runtime_put) {
if (!PMSG_IS_AUTO(msg)) {
pm_runtime_enable(port_dev-dev);
status = pm_runtime_get_sync(port_dev-dev);
pm_runtime_disable(port_dev-dev);
} else
status = pm_runtime_get_sync(port_dev-dev);
port_dev-did_runtime_put = false;
if (status 0) {
dev_dbg(udev-dev, can't resume usb port, status %d\n
RE: [Resend PATCH V4 0/10] usb: usb port power off mechanism anc expose usb port connect type
On Fri, 18 Jan 2013, Lan, Tianyu wrote: HI Alan: I just find Rafael's patch has resolved this issue. In this patch, enable runtime PM right after executing subsystem/driver .resume_early() callbacks. When do resume(), the device's runtime pm has been enabled. This patch now is already in the v3.8-rc4. So this patchset will not cause problem with Rafael patch and I have tested. About the port system suspend/resume() callback, Could I do this in the next step? Well, I guess you don't really need a resume callback now. Leaving the port in its old runtime PM state should be okay, because hub_activate will set the port power correctly. The important thing is that the hardware's actual power level should agree with the runtime PM status. However, you still need to enable async suspend for the port devices. The device_pm_wait_for_dev call isn't needed if the ports don't have a resume callback. Have you tested hibernation as well as system suspend? 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: [Resend PATCH V4 0/10] usb: usb port power off mechanism anc expose usb port connect type
On 2013年01月11日 20:10, Lan Tianyu wrote: Change since v1: optimize the export connect type patch and adjust the DeviceRemovalbe flag in the rh_call_control() after GetHubDescriptor request being processed. move all debounce operation to usb port's runtime resume callback(). Add did_runtime_put in the struct usb_port to call pm_runtime_get/put(portdev) paired. using pm_runtime_get/put(portdev) to allow or prohibit device to be power off inside of pm qos request in the kernel side. Change since v2: Correct some link breaks. Add did_runtime_put in the usb_disconnect() before calling pm_runtime_put(portdev). Provide two seperate functions usb_device_allow_power_off() and usb_device_prevent_power_off() instead of just one. Change since v3: Set did_runtime_put to false in usb_disconnect() when its value is true Add comment about not enable port runtime pm if fail to expose port's pm qos. and call pm_runtime_set_active(portdev) unconditionally. rename usb_device_allow_prevent_power_off with usb_device_control_power_off Modify be power off to be powered off Expose dev_pm_qos_flags() symbol in order to ensure usb core can compile as module. Resend v4: make patch PM/Qos: Expose dev_pm_qos_flags symbol as first patch to avoid compilation error during git bisect correct some comments. This Patchset is based on usb-next tree commit 102ee001912 Merge tag 'for-usb-next-2013-01-03' This patchset is to add usb port power off mechanism and merge with patchset usb: expose DeviceRemovable to user space via sysfs attribute. Patchset usb: expose DeviceRemovable to user space via sysfs attribute. http://marc.info/?l=linux-usbm=135279430410171w=2 with some link break corrects The main discussion about usb port power off mechanism is in the following link: http://marc.info/?l=linux-usbm=134818340017208w=2 PM/Qos: Expose dev_pm_qos_flags symbol usb: Add driver/usb/core/(port.c,hub.h) files USB: Set usb port's DeviceRemovable according acpi information usb: Add portX/connect_type attribute to expose usb port's connect type usb: Create link files between child device and usb port device. usb: Register usb port's acpi power resources usb: add runtime pm support for usb port device usb: add usb port auto power off mechanism usb: expose usb port's pm qos flags to user space usb: add usb_device_allow_power_off() and usb_device_prevent_power_off() function. Hi GregAlan: Do you have some more comments about this patchset? Thanks. -- 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
[Resend PATCH V4 0/10] usb: usb port power off mechanism anc expose usb port connect type
Change since v1: optimize the export connect type patch and adjust the DeviceRemovalbe flag in the rh_call_control() after GetHubDescriptor request being processed. move all debounce operation to usb port's runtime resume callback(). Add did_runtime_put in the struct usb_port to call pm_runtime_get/put(portdev) paired. using pm_runtime_get/put(portdev) to allow or prohibit device to be power off inside of pm qos request in the kernel side. Change since v2: Correct some link breaks. Add did_runtime_put in the usb_disconnect() before calling pm_runtime_put(portdev). Provide two seperate functions usb_device_allow_power_off() and usb_device_prevent_power_off() instead of just one. Change since v3: Set did_runtime_put to false in usb_disconnect() when its value is true Add comment about not enable port runtime pm if fail to expose port's pm qos. and call pm_runtime_set_active(portdev) unconditionally. rename usb_device_allow_prevent_power_off with usb_device_control_power_off Modify be power off to be powered off Expose dev_pm_qos_flags() symbol in order to ensure usb core can compile as module. Resend v4: make patch PM/Qos: Expose dev_pm_qos_flags symbol as first patch to avoid compilation error during git bisect correct some comments. This Patchset is based on usb-next tree commit 102ee001912 Merge tag 'for-usb-next-2013-01-03' This patchset is to add usb port power off mechanism and merge with patchset usb: expose DeviceRemovable to user space via sysfs attribute. Patchset usb: expose DeviceRemovable to user space via sysfs attribute. http://marc.info/?l=linux-usbm=135279430410171w=2 with some link break corrects The main discussion about usb port power off mechanism is in the following link: http://marc.info/?l=linux-usbm=134818340017208w=2 PM/Qos: Expose dev_pm_qos_flags symbol usb: Add driver/usb/core/(port.c,hub.h) files USB: Set usb port's DeviceRemovable according acpi information usb: Add portX/connect_type attribute to expose usb port's connect type usb: Create link files between child device and usb port device. usb: Register usb port's acpi power resources usb: add runtime pm support for usb port device usb: add usb port auto power off mechanism usb: expose usb port's pm qos flags to user space usb: add usb_device_allow_power_off() and usb_device_prevent_power_off() function. -- 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
