Re: [RFC][PATCH 1/3] usb: add a hcd notify entry in hc_driver

["Lu, Baolu" <baolu.lu@linux.intel.com>]

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@linux.intel.com>
>> ---
>>   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