Re: [RFC][PATCH 1/3] usb: add a hcd notify entry in hc_driver
From: Lu, Baolu
Date: Mon May 04 2015 - 21:05:59 EST
Hi Alan,
Thanks for your review comments. Below has my response inline.
On 05/04/2015 10:28 PM, Alan Stern wrote:
On Mon, 4 May 2015, Lu Baolu wrote:
This patch adds a new entry pointer in hc_driver. With this new entry,
USB core can notify host driver when something happens and host driver
is willing to be notified. One use case of this interface comes from
xHCI compatible host controller driver.
"Notify" is too generic. It's better to make the callback routine
specific to the activity in question. So this patch series should add
"device_suspend" and "device_resume" callbacks, not a general "notify"
callback.
Fair enough. I will make this change in v2 if there is no objection.
The xHCI spec is designed to allow an xHC implementation to cache the
endpoint state. Caching endpoint state allows an xHC to reduce latency
when handling ERDYs and other USB asynchronous events. However holding
this state in xHC consumes resources and power. The xHCI spec designs
some methods through which host controller driver can hint xHC about
how to optimize its operation, e.g. to determine when it holds state
internally or pushes it out to memory, when to power down logic, etc.
When a USB device is going to suspend, states of all endpoints cached
in the xHC should be pushed out to memory to save power and resources.
Vice versa, when a USB device resumes, those states should be brought
back to the cache. USB core suspends or resumes a USB device by sending
set/clear port feature requests to the parent hub where the USB device
is connected. Unfortunately, these operations are transparent to xHCI
driver unless the USB device is plugged in a root port. This patch
utilizes the notify interface to notify xHCI driver whenever a USB
device's power state is changed.
It is harmless if a USB devices under USB 3.0 host controller suspends
or resumes without a notification to hcd driver. However there may be
less opportunities for power savings and there may be increased latency
for restarting an endpoint. The precise impact will be different for
each xHC implementation. It all depends on what an implementation does
with the hints.
Signed-off-by: Lu Baolu <baolu.lu@xxxxxxxxxxxxxxx>
---
drivers/usb/core/generic.c | 10 ++++++++--
drivers/usb/core/hcd.c | 23 +++++++++++++++++++++++
include/linux/usb/hcd.h | 11 ++++++++++-
3 files changed, 41 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/core/generic.c b/drivers/usb/core/generic.c
index 358ca8d..92bee33 100644
--- a/drivers/usb/core/generic.c
+++ b/drivers/usb/core/generic.c
@@ -211,8 +211,12 @@ static int generic_suspend(struct usb_device *udev, pm_message_t msg)
/* Non-root devices don't need to do anything for FREEZE or PRETHAW */
else if (msg.event == PM_EVENT_FREEZE || msg.event == PM_EVENT_PRETHAW)
rc = 0;
- else
+ else {
+ hcd_notify(udev, HCD_MSG_DEV_SUSPEND, &msg);
rc = usb_port_suspend(udev, msg);
+ if (rc)
+ hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg);
+ }
return rc;
}
@@ -228,8 +232,10 @@ static int generic_resume(struct usb_device *udev, pm_message_t msg)
*/
if (!udev->parent)
rc = hcd_bus_resume(udev, msg);
- else
+ else {
rc = usb_port_resume(udev, msg);
+ hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg);
+ }
Don't you want to tell the HCD about the resume _before_ it happens?
After all, the devices endpoint will be used as soon as the resume
takes place.
The order that software should do during device suspend/resume is
defined in 4.15.1.1 of xHCI spec 1.1.
Spec 4.15.1.1:
Software shall stop all endpoints of a device using the Stop Endpoint
Command and setting the Suspend
(SP) flag to ‘1’ prior to selectively suspending a device. After the
device is resumed software shall ring an
endpoint’s doorbell to restart it.
--end--
So the order looks like:
tell hcd device suspend
usb_port_suspend()
usb_port_resume()
tell hcd device resume
And besides, this should be symmetrical with the code above, where you
tell the HCD about a suspend _after_ it happens.
This was done in above change in generic_suspend():
@@ -211,8 +211,12 @@ static int generic_suspend(struct usb_device *udev, pm_message_t msg)
/* Non-root devices don't need to do anything for FREEZE or PRETHAW */
else if (msg.event == PM_EVENT_FREEZE || msg.event == PM_EVENT_PRETHAW)
rc = 0;
- else
+ else {
+ hcd_notify(udev, HCD_MSG_DEV_SUSPEND, &msg);
rc = usb_port_suspend(udev, msg);
+ if (rc)
+ hcd_notify(udev, HCD_MSG_DEV_RESUME, &msg);
+ }
return rc;
}
If usb_port_suspend() returns error, we should roll back HCD to the previous state.
Alan Stern
Thanks again,
Baolu
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/