[PATCH v2 0/3] usb: notify hcd when USB device suspend or resume

[PATCH v2 0/3] usb: notify hcd when USB device suspend or resume

[Lu Baolu]

This patch series try to meet a design requirement in xHCI spec.

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.

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.

Change log:
v1->v2:
- make the callback name specific to the activity in question
- no need to export hcd_notify
- put the callback in the right place

Lu Baolu (3):
  usb: notify hcd when USB device suspend or resume
  usb: xhci: implement device_suspend/device_resume entries
  usb: xhci: remove stop device and ring doorbell in hub control and bus
    suspend

 drivers/usb/core/hcd.c      | 29 +++++++++++++++++++++++++++
 drivers/usb/core/hub.c      | 16 +++++++++++++++
 drivers/usb/host/xhci-hub.c | 49 +--------------------------------------------
 drivers/usb/host/xhci.c     | 40 ++++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci.h     |  5 +++++
 include/linux/usb/hcd.h     | 10 ++++++++-
 6 files changed, 100 insertions(+), 49 deletions(-)

-- 
2.1.0

[PATCH v2 1/3] usb: notify hcd when USB device suspend or resume

[Lu Baolu]

This patch adds two new entries in hc_driver. With these new entries,
USB core can notify host driver when a USB device is about to suspend
or just resumed.

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 callback entries to notify xHCI driver whenever a USB
device suspends or resumes.

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/hcd.c  | 29 +++++++++++++++++++++++++++++
 drivers/usb/core/hub.c  | 16 ++++++++++++++++
 include/linux/usb/hcd.h | 10 +++++++++-
 3 files changed, 54 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 45a915c..a4cfc2d 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2289,6 +2289,35 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
 }
 EXPORT_SYMBOL_GPL(usb_hcd_resume_root_hub);
 
+/**
+ * hcd_suspend_notify - notify hcd driver when a device is going to suspend
+ * @udev: USB device to be suspended
+ * @msg: Power Management message describing this state transition
+ *
+ * Call back to hcd driver to notify that a USB device is going to suspend.
+ */
+void hcd_suspend_notify(struct usb_device *udev, pm_message_t msg)
+{
+	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
+
+	if (hcd->driver->device_suspend)
+		hcd->driver->device_suspend(hcd, udev, msg);
+}
+
+/**
+ * hcd_resume_notify - notify hcd driver when a device is just resumed
+ * @udev: USB device just resumed
+ * @msg: Power Management message describing this state transition
+ *
+ * Call back to hcd driver to notify that a USB device is just resumed.
+ */
+void hcd_resume_notify(struct usb_device *udev, pm_message_t msg)
+{
+	struct usb_hcd *hcd = bus_to_hcd(udev->bus);
+
+	if (hcd->driver->device_resume)
+		hcd->driver->device_resume(hcd, udev, msg);
+}
 #endif	/* CONFIG_PM */
 
 /*-------------------------------------------------------------------------*/
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index 3b71516..ed22b51 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -3144,6 +3144,14 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 			goto err_lpm3;
 	}
 
+	/*
+	 * Call back to hcd if it expects. xHCI compatible host controller
+	 * driver expects to be notified prior to selectively suspending a
+	 * device. xHCI hcd could optimize the endpoint cache for power
+	 * saving purpose. Refer to 4.15.1.1 of xHCI 1.1 for more information.
+	 */
+	hcd_suspend_notify(udev, msg);
+
 	/* see 7.1.7.6 */
 	if (hub_is_superspeed(hub->hdev))
 		status = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U3);
@@ -3169,6 +3177,8 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
 	if (status) {
 		dev_dbg(&port_dev->dev, "can't suspend, status %d\n", status);
 
+		hcd_resume_notify(udev, msg);
+
 		/* Try to enable USB3 LPM and LTM again */
 		usb_unlocked_enable_lpm(udev);
  err_lpm3:
@@ -3422,6 +3432,12 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
 	}
 
  SuspendCleared:
+	/* Call back to hcd if it expects. xHCI compatible host controller
+	 * driver expects to be notified after a device is resumed. xHCI
+	 * hcd could optimize the endpoint cache for latency reducing
+	 * purpose. Refer to 4.15.1.1 of xHCI 1.1 for more information.
+	 */
+	hcd_resume_notify(udev, msg);
 	if (status == 0) {
 		udev->port_is_suspended = 0;
 		if (hub_is_superspeed(hub->hdev)) {
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 68b1e83..621d9b7 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -383,7 +383,13 @@ struct hc_driver {
 	int	(*find_raw_port_number)(struct usb_hcd *, int);
 	/* Call for power on/off the port if necessary */
 	int	(*port_power)(struct usb_hcd *hcd, int portnum, bool enable);
-
+	/* Call back to hcd when a USB device is going to suspend or just
+	 * resumed.
+	 */
+	void	(*device_suspend)(struct usb_hcd *, struct usb_device *udev,
+			pm_message_t msg);
+	void	(*device_resume)(struct usb_hcd *, struct usb_device *udev,
+			pm_message_t msg);
 };
 
 static inline int hcd_giveback_urb_in_bh(struct usb_hcd *hcd)
@@ -632,6 +638,8 @@ extern void usb_root_hub_lost_power(struct usb_device *rhdev);
 extern int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg);
 extern int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg);
 extern void usb_hcd_resume_root_hub(struct usb_hcd *hcd);
+extern void hcd_suspend_notify(struct usb_device *udev, pm_message_t msg);
+extern void hcd_resume_notify(struct usb_device *udev, pm_message_t msg);
 #else
 static inline void usb_hcd_resume_root_hub(struct usb_hcd *hcd)
 {
-- 
2.1.0

Re: [PATCH v2 1/3] usb: notify hcd when USB device suspend or resume

[Alan Stern]

On Wed, 6 May 2015, Lu Baolu wrote:

> This patch adds two new entries in hc_driver. With these new entries,
> USB core can notify host driver when a USB device is about to suspend
> or just resumed.
> 
> 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 callback entries to notify xHCI driver whenever a USB
> device suspends or resumes.
> 
> 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>


> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -3144,6 +3144,14 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
>  			goto err_lpm3;
>  	}
>  
> +	/*
> +	 * Call back to hcd if it expects. xHCI compatible host controller
> +	 * driver expects to be notified prior to selectively suspending a
> +	 * device. xHCI hcd could optimize the endpoint cache for power
> +	 * saving purpose. Refer to 4.15.1.1 of xHCI 1.1 for more information.
> +	 */

Doesn't this comment belong in the xhci-hcd source code rather than the 
hub driver source code?

> +	hcd_suspend_notify(udev, msg);
> +
>  	/* see 7.1.7.6 */
>  	if (hub_is_superspeed(hub->hdev))
>  		status = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U3);
> @@ -3169,6 +3177,8 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
>  	if (status) {
>  		dev_dbg(&port_dev->dev, "can't suspend, status %d\n", status);
>  
> +		hcd_resume_notify(udev, msg);
> +
>  		/* Try to enable USB3 LPM and LTM again */
>  		usb_unlocked_enable_lpm(udev);
>   err_lpm3:
> @@ -3422,6 +3432,12 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
>  	}
>  
>   SuspendCleared:
> +	/* Call back to hcd if it expects. xHCI compatible host controller
> +	 * driver expects to be notified after a device is resumed. xHCI
> +	 * hcd could optimize the endpoint cache for latency reducing
> +	 * purpose. Refer to 4.15.1.1 of xHCI 1.1 for more information.
> +	 */

Same for this comment.

> +	hcd_resume_notify(udev, msg);
>  	if (status == 0) {
>  		udev->port_is_suspended = 0;
>  		if (hub_is_superspeed(hub->hdev)) {
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index 68b1e83..621d9b7 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -383,7 +383,13 @@ struct hc_driver {
>  	int	(*find_raw_port_number)(struct usb_hcd *, int);
>  	/* Call for power on/off the port if necessary */
>  	int	(*port_power)(struct usb_hcd *hcd, int portnum, bool enable);
> -
> +	/* Call back to hcd when a USB device is going to suspend or just
> +	 * resumed.
> +	 */
> +	void	(*device_suspend)(struct usb_hcd *, struct usb_device *udev,
> +			pm_message_t msg);
> +	void	(*device_resume)(struct usb_hcd *, struct usb_device *udev,
> +			pm_message_t msg);
>  };

Your callbacks don't use the msg argument.  What makes you think it is 
needed?

Alan Stern

Re: [PATCH v2 1/3] usb: notify hcd when USB device suspend or resume

[Lu, Baolu]

On 05/06/2015 10:35 PM, Alan Stern wrote:
> On Wed, 6 May 2015, Lu Baolu wrote:
>
>> This patch adds two new entries in hc_driver. With these new entries,
>> USB core can notify host driver when a USB device is about to suspend
>> or just resumed.
>>
>> 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 callback entries to notify xHCI driver whenever a USB
>> device suspends or resumes.
>>
>> 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>
>
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -3144,6 +3144,14 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
>>   			goto err_lpm3;
>>   	}
>>   
>> +	/*
>> +	 * Call back to hcd if it expects. xHCI compatible host controller
>> +	 * driver expects to be notified prior to selectively suspending a
>> +	 * device. xHCI hcd could optimize the endpoint cache for power
>> +	 * saving purpose. Refer to 4.15.1.1 of xHCI 1.1 for more information.
>> +	 */
> Doesn't this comment belong in the xhci-hcd source code rather than the
> hub driver source code?

Yes, I agree. I will move this and below comments to xhci driver.

>
>> +	hcd_suspend_notify(udev, msg);
>> +
>>   	/* see 7.1.7.6 */
>>   	if (hub_is_superspeed(hub->hdev))
>>   		status = hub_set_port_link_state(hub, port1, USB_SS_PORT_LS_U3);
>> @@ -3169,6 +3177,8 @@ int usb_port_suspend(struct usb_device *udev, pm_message_t msg)
>>   	if (status) {
>>   		dev_dbg(&port_dev->dev, "can't suspend, status %d\n", status);
>>   
>> +		hcd_resume_notify(udev, msg);
>> +
>>   		/* Try to enable USB3 LPM and LTM again */
>>   		usb_unlocked_enable_lpm(udev);
>>    err_lpm3:
>> @@ -3422,6 +3432,12 @@ int usb_port_resume(struct usb_device *udev, pm_message_t msg)
>>   	}
>>   
>>    SuspendCleared:
>> +	/* Call back to hcd if it expects. xHCI compatible host controller
>> +	 * driver expects to be notified after a device is resumed. xHCI
>> +	 * hcd could optimize the endpoint cache for latency reducing
>> +	 * purpose. Refer to 4.15.1.1 of xHCI 1.1 for more information.
>> +	 */
> Same for this comment.
>
>> +	hcd_resume_notify(udev, msg);
>>   	if (status == 0) {
>>   		udev->port_is_suspended = 0;
>>   		if (hub_is_superspeed(hub->hdev)) {
>> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
>> index 68b1e83..621d9b7 100644
>> --- a/include/linux/usb/hcd.h
>> +++ b/include/linux/usb/hcd.h
>> @@ -383,7 +383,13 @@ struct hc_driver {
>>   	int	(*find_raw_port_number)(struct usb_hcd *, int);
>>   	/* Call for power on/off the port if necessary */
>>   	int	(*port_power)(struct usb_hcd *hcd, int portnum, bool enable);
>> -
>> +	/* Call back to hcd when a USB device is going to suspend or just
>> +	 * resumed.
>> +	 */
>> +	void	(*device_suspend)(struct usb_hcd *, struct usb_device *udev,
>> +			pm_message_t msg);
>> +	void	(*device_resume)(struct usb_hcd *, struct usb_device *udev,
>> +			pm_message_t msg);
>>   };
> Your callbacks don't use the msg argument.  What makes you think it is
> needed?

This msg argument is valuable. XHCI spec defines a capability named FSC
(Force Save context Capability). When this capability is implemented, the
Save State operation (do during host suspend) shall save any cached Slot,
Endpoint, Stream or other Context information to memory. xHCI hcd could
use this "msg" to determine whether it needs to issue stop endpoint with
SP (suspend) bit set.

FSC is optional prior 1.1 and mandatory since 1.1. I have patches for 1.1
features in Mathias' gate. We could bring them up after we test them on
the real hardware.

>
> Alan Stern
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH v2 1/3] usb: notify hcd when USB device suspend or resume

[Alan Stern]

On Thu, 7 May 2015, Lu, Baolu wrote:

> >> +	void	(*device_suspend)(struct usb_hcd *, struct usb_device *udev,
> >> +			pm_message_t msg);
> >> +	void	(*device_resume)(struct usb_hcd *, struct usb_device *udev,
> >> +			pm_message_t msg);
> >>   };
> > Your callbacks don't use the msg argument.  What makes you think it is
> > needed?
> 
> This msg argument is valuable. XHCI spec defines a capability named FSC
> (Force Save context Capability). When this capability is implemented, the
> Save State operation (do during host suspend) shall save any cached Slot,
> Endpoint, Stream or other Context information to memory. xHCI hcd could
> use this "msg" to determine whether it needs to issue stop endpoint with
> SP (suspend) bit set.

I don't understand.  What is the advantage of using FSC?

How would xhci-hcd use "msg" to determine this?  And why doesn't your
2/3 patch already do it?

Alan Stern

Re: [PATCH v2 1/3] usb: notify hcd when USB device suspend or resume

[Lu, Baolu]

On 05/07/2015 10:34 PM, Alan Stern wrote:
> On Thu, 7 May 2015, Lu, Baolu wrote:
>
>>>> +	void	(*device_suspend)(struct usb_hcd *, struct usb_device *udev,
>>>> +			pm_message_t msg);
>>>> +	void	(*device_resume)(struct usb_hcd *, struct usb_device *udev,
>>>> +			pm_message_t msg);
>>>>    };
>>> Your callbacks don't use the msg argument.  What makes you think it is
>>> needed?
>> This msg argument is valuable. XHCI spec defines a capability named FSC
>> (Force Save context Capability). When this capability is implemented, the
>> Save State operation (do during host suspend) shall save any cached Slot,
>> Endpoint, Stream or other Context information to memory. xHCI hcd could
>> use this "msg" to determine whether it needs to issue stop endpoint with
>> SP (suspend) bit set.
> I don't understand.  What is the advantage of using FSC?

I'm sorry, I didn't make it clear.

As part of host suspend, controller save state will be issued to save
host internal state in xhci_suspend():

         /* step 4: set CSS flag */
         command = readl(&xhci->op_regs->command);
         command |= CMD_CSS;
         writel(command, &xhci->op_regs->command);
         if (xhci_handshake(&xhci->op_regs->status,
                                 STS_SAVE, 0, 10 * 1000)) {
                 xhci_warn(xhci, "WARN: xHC save state timeout\n");
                 spin_unlock_irq(&xhci->lock);
                 return -ETIMEDOUT;
         }

If FSC is supported,  the cached Slot, Endpoint, Stream, or other
Context information are also saved.

Hence, when FSC is supported, software does not have to issue Stop
Endpoint Command to push public and private endpoint state into
memory as part of system suspend process.

The logic in xhci_device_suspend() will look like:

if xhci_device_suspend() callback was called due to system suspend,
(mesg.event & PM_EVENT_SUSPEND is true) and FSC is supported by
the xHC implementation, xhci_device_suspend() could ignore stop
endpoint command, since CSS will be done in xhc_suspend() later and
where all the endpoint caches will be pushed to memory.

>
> How would xhci-hcd use "msg" to determine this?  And why doesn't your
> 2/3 patch already do it?

xhci-hcd can use "msg" to determine which case the callback is called,
run-time suspend or system suspend.

FSC needs a separate patch, I already have it in Mathias' gate. I didn't
bring it together with this patch series because it's still not tested yet.

>
> Alan Stern

Thank you!
Baolu

>
>
>

Re: [PATCH v2 1/3] usb: notify hcd when USB device suspend or resume

[Alan Stern]

On Fri, 8 May 2015, Lu, Baolu wrote:

> On 05/07/2015 10:34 PM, Alan Stern wrote:
> > On Thu, 7 May 2015, Lu, Baolu wrote:
> >
> >>>> +	void	(*device_suspend)(struct usb_hcd *, struct usb_device *udev,
> >>>> +			pm_message_t msg);
> >>>> +	void	(*device_resume)(struct usb_hcd *, struct usb_device *udev,
> >>>> +			pm_message_t msg);
> >>>>    };
> >>> Your callbacks don't use the msg argument.  What makes you think it is
> >>> needed?
> >> This msg argument is valuable. XHCI spec defines a capability named FSC
> >> (Force Save context Capability). When this capability is implemented, the
> >> Save State operation (do during host suspend) shall save any cached Slot,
> >> Endpoint, Stream or other Context information to memory. xHCI hcd could
> >> use this "msg" to determine whether it needs to issue stop endpoint with
> >> SP (suspend) bit set.
> > I don't understand.  What is the advantage of using FSC?
> 
> I'm sorry, I didn't make it clear.
> 
> As part of host suspend, controller save state will be issued to save
> host internal state in xhci_suspend():

...

> If FSC is supported,  the cached Slot, Endpoint, Stream, or other
> Context information are also saved.
> 
> Hence, when FSC is supported, software does not have to issue Stop
> Endpoint Command to push public and private endpoint state into
> memory as part of system suspend process.

Why do you have to push this state into memory at all?  Does the
controller hardware lose the cached state information when it is in low
power?

> The logic in xhci_device_suspend() will look like:
> 
> if xhci_device_suspend() callback was called due to system suspend,
> (mesg.event & PM_EVENT_SUSPEND is true) and FSC is supported by
> the xHC implementation, xhci_device_suspend() could ignore stop
> endpoint command, since CSS will be done in xhc_suspend() later and
> where all the endpoint caches will be pushed to memory.

I still don't understand this.  You said earlier that according
to section 4.15.1.1 of the xHCI spec, the endpoint rings should
_always_ be stopped with SP set when a device is suspended.  Now you're
saying that they don't need to be stopped during a system suspend if
the controller supports FSC.  (Or maybe you're saying they need to be 
stopped but SP doesn't need to be set -- it's hard to tell.)

So which is it?  Do you need to stop the endpoint rings?  Is it okay 
not to set SP?

Alan Stern

Re: [PATCH v2 1/3] usb: notify hcd when USB device suspend or resume

[Lu, Baolu]

On 05/08/2015 10:21 PM, Alan Stern wrote:
> On Fri, 8 May 2015, Lu, Baolu wrote:
>
>> On 05/07/2015 10:34 PM, Alan Stern wrote:
>>> On Thu, 7 May 2015, Lu, Baolu wrote:
>>>
>>>>>> +	void	(*device_suspend)(struct usb_hcd *, struct usb_device *udev,
>>>>>> +			pm_message_t msg);
>>>>>> +	void	(*device_resume)(struct usb_hcd *, struct usb_device *udev,
>>>>>> +			pm_message_t msg);
>>>>>>     };
>>>>> Your callbacks don't use the msg argument.  What makes you think it is
>>>>> needed?
>>>> This msg argument is valuable. XHCI spec defines a capability named FSC
>>>> (Force Save context Capability). When this capability is implemented, the
>>>> Save State operation (do during host suspend) shall save any cached Slot,
>>>> Endpoint, Stream or other Context information to memory. xHCI hcd could
>>>> use this "msg" to determine whether it needs to issue stop endpoint with
>>>> SP (suspend) bit set.
>>> I don't understand.  What is the advantage of using FSC?
>> I'm sorry, I didn't make it clear.
>>
>> As part of host suspend, controller save state will be issued to save
>> host internal state in xhci_suspend():
> ...
>
>> If FSC is supported,  the cached Slot, Endpoint, Stream, or other
>> Context information are also saved.
>>
>> Hence, when FSC is supported, software does not have to issue Stop
>> Endpoint Command to push public and private endpoint state into
>> memory as part of system suspend process.
> Why do you have to push this state into memory at all?  Does the
> controller hardware lose the cached state information when it is in low
> power?

I don't think controller hardware will lose the cached state information
when it is in low power. But since cache in controller consumes power
and resources, by pushing state into memory, hardware can power
off the cache logic during suspend. Hence more power saving gains.

>
>> The logic in xhci_device_suspend() will look like:
>>
>> if xhci_device_suspend() callback was called due to system suspend,
>> (mesg.event & PM_EVENT_SUSPEND is true) and FSC is supported by
>> the xHC implementation, xhci_device_suspend() could ignore stop
>> endpoint command, since CSS will be done in xhc_suspend() later and
>> where all the endpoint caches will be pushed to memory.
> I still don't understand this.  You said earlier that according
> to section 4.15.1.1 of the xHCI spec, the endpoint rings should
> _always_ be stopped with SP set when a device is suspended.  Now you're

The intention of stop endpoint with SP set is to tell hardware that
"a device is going to suspend, hardware don't need to contain the
endpoint state in internal cache anymore". Hardware _could_ use
this hint to push endpoint state into memory to reduce power
consumption.

> saying that they don't need to be stopped during a system suspend if
> the controller supports FSC.  (Or maybe you're saying they need to be
> stopped but SP doesn't need to be set -- it's hard to tell.)

Even FSC is supported, controller hardware still need to push cached
endpoint state into memory when a USB device is suspended. The
difference is when FSC is enforced, CSS command will push any
cached endpoint state into memory unconditionally.

So, when xhci_device_suspend() knows that CSS command will be
executed later and CSS command will push cached endpoint state
into memory (a.k.a. FSC is enforced), it could skip issuing stop
endpoint command with SP bit set to avoid duplication and reduce
the suspend time.

This is the case for system suspend since CSS command is part of
xhci_suspend() and xhci_suspend() will be executed after all USB
devices have been suspended. But it's not case for run-time suspend
(auto-pm) since USB device suspend and host controller suspend
are independent for run-time case.

That's the reason why I wanted to keep 'msg' parameter. But just as
Greg said, we don't need to keep a parameter when it's not used
and can add it later when it is required.

>
> So which is it?  Do you need to stop the endpoint rings?  Is it okay
> not to set SP?

"stop endpoint" and "stop endpoint with SP set" serve different purposes
in Linux xhci driver as my understanding. "stop endpoint" command is
used to stop a active ring when upper layer want to cancel a URB.
"stop endpoint with SP set" is used to hint hardware that a USB is going
to suspend. Hence "stop endpoint with SP set" must be executed in case
that the transfer ring is empty.

>
> Alan Stern

Thank you,
Baolu
>
>
>

Re: [PATCH v2 1/3] usb: notify hcd when USB device suspend or resume

[Alan Stern]

On Sat, 9 May 2015, Lu, Baolu wrote:

> >> If FSC is supported,  the cached Slot, Endpoint, Stream, or other
> >> Context information are also saved.
> >>
> >> Hence, when FSC is supported, software does not have to issue Stop
> >> Endpoint Command to push public and private endpoint state into
> >> memory as part of system suspend process.
> > Why do you have to push this state into memory at all?  Does the
> > controller hardware lose the cached state information when it is in low
> > power?
> 
> I don't think controller hardware will lose the cached state information
> when it is in low power. But since cache in controller consumes power
> and resources, by pushing state into memory, hardware can power
> off the cache logic during suspend. Hence more power saving gains.
> 
> >
> >> The logic in xhci_device_suspend() will look like:
> >>
> >> if xhci_device_suspend() callback was called due to system suspend,
> >> (mesg.event & PM_EVENT_SUSPEND is true) and FSC is supported by
> >> the xHC implementation, xhci_device_suspend() could ignore stop
> >> endpoint command, since CSS will be done in xhc_suspend() later and
> >> where all the endpoint caches will be pushed to memory.
> > I still don't understand this.  You said earlier that according
> > to section 4.15.1.1 of the xHCI spec, the endpoint rings should
> > _always_ be stopped with SP set when a device is suspended.  Now you're
> 
> The intention of stop endpoint with SP set is to tell hardware that
> "a device is going to suspend, hardware don't need to contain the
> endpoint state in internal cache anymore". Hardware _could_ use
> this hint to push endpoint state into memory to reduce power
> consumption.
> 
> > saying that they don't need to be stopped during a system suspend if
> > the controller supports FSC.  (Or maybe you're saying they need to be
> > stopped but SP doesn't need to be set -- it's hard to tell.)
> 
> Even FSC is supported, controller hardware still need to push cached
> endpoint state into memory when a USB device is suspended. The
> difference is when FSC is enforced, CSS command will push any
> cached endpoint state into memory unconditionally.

You said above that the hardware _could_ push endpoint state into
memory.  Now you're saying it _needs_ to do this!  Make up your mind.


> 
> So, when xhci_device_suspend() knows that CSS command will be
> executed later and CSS command will push cached endpoint state
> into memory (a.k.a. FSC is enforced), it could skip issuing stop
> endpoint command with SP bit set to avoid duplication and reduce
> the suspend time.
> 
> This is the case for system suspend since CSS command is part of
> xhci_suspend() and xhci_suspend() will be executed after all USB
> devices have been suspended. But it's not case for run-time suspend
> (auto-pm) since USB device suspend and host controller suspend
> are independent for run-time case.
> 
> That's the reason why I wanted to keep 'msg' parameter. But just as
> Greg said, we don't need to keep a parameter when it's not used
> and can add it later when it is required.
> 
> >
> > So which is it?  Do you need to stop the endpoint rings?  Is it okay
> > not to set SP?
> 
> "stop endpoint" and "stop endpoint with SP set" serve different purposes
> in Linux xhci driver as my understanding. "stop endpoint" command is
> used to stop a active ring when upper layer want to cancel a URB.
> "stop endpoint with SP set" is used to hint hardware that a USB is going
> to suspend. Hence "stop endpoint with SP set" must be executed in case
> that the transfer ring is empty.

(How does the contents of the transfer ring affect anything?  Besides, 
there are never any active URBs when a device gets suspended, so the 
transfer ring will _always_ be empty at such times.)

This is still extremely confusing.  You're not doing a good job of 
explaining the situation clearly and logically.

Let's see if I understand it correctly:

	When the controller goes into suspend, you want the cache to
	be pushed into memory so that the cache can be powered down,
	thereby saving additional energy.

	If the hardware supports FSC, this will happen automatically.

	If the hardware doesn't support FSC, the cached data won't get
	pushed to memory unless the driver tells the controller to do 
	so at the time the device is suspended.  But this will slow 
	things down, so the driver should avoid doing it when it's not 
	needed.

	During system suspend you know in advance that the controller
	will be suspended.  Therefore the driver should push the cache 
	to memory if the hardware doesn't support FSC.  During runtime 
	suspend you don't know in advance whether the controller will 
	be suspended, so the driver should not push the cache to 
	memory.

	But what happens in the case where all the devices have gone 
	into runtime suspend, so the controller also goes into runtime
	suspend, and the hardware doesn't support FSC?  It seems that
	in this case the cache would have to remain powered on.

Also, it's still not clear what the driver needs to do differently in
order to push out the cached data.  You have managed to imply both:

	Issuing Stop Endpoint with SP set is mandatory whenever
	a device is suspended, and

	The SP bit is what tells the controller to push the endpoint
	state to memory.

This doesn't make sense.

One last question: How much extra time does it take to push the cached 
data to memory?  Bear in mind that suspending a device is not a rapid 
operation; if pushing the data takes no more than a few microseconds, I 
think it would be worthwhile to do it always, even when it's not 
necessary.

Alan Stern

Re: [PATCH v2 1/3] usb: notify hcd when USB device suspend or resume

[Lu, Baolu]

On 05/11/2015 10:25 PM, Alan Stern wrote:
> On Sat, 9 May 2015, Lu, Baolu wrote:
>
>>>> If FSC is supported,  the cached Slot, Endpoint, Stream, or other
>>>> Context information are also saved.
>>>>
>>>> Hence, when FSC is supported, software does not have to issue Stop
>>>> Endpoint Command to push public and private endpoint state into
>>>> memory as part of system suspend process.
>>> Why do you have to push this state into memory at all?  Does the
>>> controller hardware lose the cached state information when it is in low
>>> power?
>> I don't think controller hardware will lose the cached state information
>> when it is in low power. But since cache in controller consumes power
>> and resources, by pushing state into memory, hardware can power
>> off the cache logic during suspend. Hence more power saving gains.
>>
>>>> The logic in xhci_device_suspend() will look like:
>>>>
>>>> if xhci_device_suspend() callback was called due to system suspend,
>>>> (mesg.event & PM_EVENT_SUSPEND is true) and FSC is supported by
>>>> the xHC implementation, xhci_device_suspend() could ignore stop
>>>> endpoint command, since CSS will be done in xhc_suspend() later and
>>>> where all the endpoint caches will be pushed to memory.
>>> I still don't understand this.  You said earlier that according
>>> to section 4.15.1.1 of the xHCI spec, the endpoint rings should
>>> _always_ be stopped with SP set when a device is suspended.  Now you're
>> The intention of stop endpoint with SP set is to tell hardware that
>> "a device is going to suspend, hardware don't need to contain the
>> endpoint state in internal cache anymore". Hardware _could_ use
>> this hint to push endpoint state into memory to reduce power
>> consumption.
>>
>>> saying that they don't need to be stopped during a system suspend if
>>> the controller supports FSC.  (Or maybe you're saying they need to be
>>> stopped but SP doesn't need to be set -- it's hard to tell.)
>> Even FSC is supported, controller hardware still need to push cached
>> endpoint state into memory when a USB device is suspended. The
>> difference is when FSC is enforced, CSS command will push any
>> cached endpoint state into memory unconditionally.
> You said above that the hardware _could_ push endpoint state into
> memory.  Now you're saying it _needs_ to do this!  Make up your mind.

I'm sorry that I confused you.

FSC is a different thing from what this patch series does.

I should say "software could ask hardware to push endpoint
state into memory even FSC is supported". But in some cases,
it can be optimized as I will describe it below.

>
>
>> So, when xhci_device_suspend() knows that CSS command will be
>> executed later and CSS command will push cached endpoint state
>> into memory (a.k.a. FSC is enforced), it could skip issuing stop
>> endpoint command with SP bit set to avoid duplication and reduce
>> the suspend time.
>>
>> This is the case for system suspend since CSS command is part of
>> xhci_suspend() and xhci_suspend() will be executed after all USB
>> devices have been suspended. But it's not case for run-time suspend
>> (auto-pm) since USB device suspend and host controller suspend
>> are independent for run-time case.
>>
>> That's the reason why I wanted to keep 'msg' parameter. But just as
>> Greg said, we don't need to keep a parameter when it's not used
>> and can add it later when it is required.
>>
>>> So which is it?  Do you need to stop the endpoint rings?  Is it okay
>>> not to set SP?
>> "stop endpoint" and "stop endpoint with SP set" serve different purposes
>> in Linux xhci driver as my understanding. "stop endpoint" command is
>> used to stop a active ring when upper layer want to cancel a URB.
>> "stop endpoint with SP set" is used to hint hardware that a USB is going
>> to suspend. Hence "stop endpoint with SP set" must be executed in case
>> that the transfer ring is empty.
> (How does the contents of the transfer ring affect anything?  Besides,
> there are never any active URBs when a device gets suspended, so the
> transfer ring will _always_ be empty at such times.)
>
> This is still extremely confusing.  You're not doing a good job of
> explaining the situation clearly and logically.

I'm sorry for that.

>
> Let's see if I understand it correctly:
>
> 	When the controller goes into suspend, you want the cache to
> 	be pushed into memory so that the cache can be powered down,
> 	thereby saving additional energy.

Not the controller goes into suspend, but USB devices.

In order to talking to USB devices, xHCI has an endpoint state for each
endpoint of a device. When a USB device goes into suspend, xHC driver
could ask the hardware to push the state from cache to the memory.
Thereby saving additional energy. This is the intention of this patch 
series.

>
> 	If the hardware supports FSC, this will happen automatically.
>
> 	If the hardware doesn't support FSC, the cached data won't get
> 	pushed to memory unless the driver tells the controller to do
> 	so at the time the device is suspended.  But this will slow
> 	things down, so the driver should avoid doing it when it's not
> 	needed.
>
> 	During system suspend you know in advance that the controller
> 	will be suspended.  Therefore the driver should push the cache
> 	to memory if the hardware doesn't support FSC.  During runtime
> 	suspend you don't know in advance whether the controller will
> 	be suspended, so the driver should not push the cache to
> 	memory.
>
> 	But what happens in the case where all the devices have gone
> 	into runtime suspend, so the controller also goes into runtime
> 	suspend, and the hardware doesn't support FSC?  It seems that
> 	in this case the cache would have to remain powered on.

FSC is not part of this patch series. It was introduced when I tried to
explain the reason why I kept 'msg' parameter in the callback.

If the hardware support FSC, another operation named "save context
operation" will push everything in hardware cache into memory. So when
a USB device is going to suspend and xhci_device_suspend() callback is
being called, software can do an optimization. That is, if "save context
operation" will push everything in hardware cache to memory later,
xhci_device_suspend() could skip asking hardware for cache operation
and let "save context operation" do it later.

One example of this situation is system suspend. During system suspend,
below process will be done for a USB3 fabric.

1) all USB devices suspend
2) root hub suspend
3) host controller suspend

In 1), xhci_device_suspend() call back will be called for each device 
suspend.
In 3) "save context operation" will be executed.

In this case, if FSC is supported, xhci_device_suspend() could skip asking
host hardware for cache operation.

>
> Also, it's still not clear what the driver needs to do differently in
> order to push out the cached data.  You have managed to imply both:
>
> 	Issuing Stop Endpoint with SP set is mandatory whenever
> 	a device is suspended, and
>
> 	The SP bit is what tells the controller to push the endpoint
> 	state to memory.
>
> This doesn't make sense.

Software doesn't handle the cache directly. XHC driver just tell hardware
that a device is going to suspend. XHC driver does this through stop 
endpoint
command by setting the SP (suspend) bit in the command trb.

During xHC handles stop endpoint command, it will check SP bit, if it's set,
xHC knows that the endpoint state could be pushed out to memory now,
since device is going to suspend. The operation of cache is very hardware
implementation dependent.

>
> One last question: How much extra time does it take to push the cached
> data to memory?  Bear in mind that suspending a device is not a rapid
> operation; if pushing the data takes no more than a few microseconds, I
> think it would be worthwhile to do it always, even when it's not
> necessary.

It depends on how much time software spends in xhci_device_suspend().
I don't have data in hand, but I agree with you that if this time is no more
than a few microseconds, we can let do it always.

>
> Alan Stern

Thank you!
-baolu

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

Re: [PATCH v2 1/3] usb: notify hcd when USB device suspend or resume

[Alan Stern]

On Tue, 12 May 2015, Lu, Baolu wrote:

> I'm sorry that I confused you.
> 
> FSC is a different thing from what this patch series does.

I know that.  The patch series, in its current form, is fine.  Now I'm
trying to understand what you originally wanted to do.

> > Let's see if I understand it correctly:
> >
> > 	When the controller goes into suspend, you want the cache to
> > 	be pushed into memory so that the cache can be powered down,
> > 	thereby saving additional energy.
> 
> Not the controller goes into suspend, but USB devices.

Suppose a USB device goes into runtime suspend, and suppose the
hardware pushes the cached information for the endpoints on that device
into memory.  The cache will still contain information for other,
non-suspended devices.  Consequently the cache _can't_ be powered down
at this time, right?  You seem to be saying:

	When a USB device goes into suspend, you want the cache to
	be pushed into memory so that the cache can be powered down,
	thereby saving additional energy.

But that is just wrong!  The only time you know the hardware can safely
power-down the cache is when the _controller_ goes into suspend.  At
that time you _know_ all the attached devices are suspended, so the
cache isn't needed.

Therefore, when the controller goes into suspend, you want to make sure 
that the cache has been pushed into memory.  The actual push operation 
can occur earlier, during xhci_device_suspend, but the time when the 
information's location matters is during xhci_suspend.

> In order to talking to USB devices, xHCI has an endpoint state for each
> endpoint of a device. When a USB device goes into suspend, xHC driver
> could ask the hardware to push the state from cache to the memory.
> Thereby saving additional energy. This is the intention of this patch 
> series.

You don't save any energy when the _device_ goes into suspend.  You
save energy only when the _controller_ goes into suspend, because 
that's the only time when the cache can be powered down.  Right?

> > 	If the hardware supports FSC, this will happen automatically.
> >
> > 	If the hardware doesn't support FSC, the cached data won't get
> > 	pushed to memory unless the driver tells the controller to do
> > 	so at the time the device is suspended.  But this will slow
> > 	things down, so the driver should avoid doing it when it's not
> > 	needed.
> >
> > 	During system suspend you know in advance that the controller
> > 	will be suspended.  Therefore the driver should push the cache
> > 	to memory if the hardware doesn't support FSC.  During runtime
> > 	suspend you don't know in advance whether the controller will
> > 	be suspended, so the driver should not push the cache to
> > 	memory.
> >
> > 	But what happens in the case where all the devices have gone
> > 	into runtime suspend, so the controller also goes into runtime
> > 	suspend, and the hardware doesn't support FSC?  It seems that
> > 	in this case the cache would have to remain powered on.
> 
> FSC is not part of this patch series. It was introduced when I tried to
> explain the reason why I kept 'msg' parameter in the callback.
> 
> If the hardware support FSC, another operation named "save context
> operation" will push everything in hardware cache into memory. So when
> a USB device is going to suspend and xhci_device_suspend() callback is
> being called, software can do an optimization. That is, if "save context
> operation" will push everything in hardware cache to memory later,
> xhci_device_suspend() could skip asking hardware for cache operation
> and let "save context operation" do it later.

That's more or less the same as what I wrote above, except that I said 
it would happen automatically if the hardware supports FSC.  It isn't 
automatic; it requires the driver to issue a "save context operation" 
command.

> One example of this situation is system suspend. During system suspend,
> below process will be done for a USB3 fabric.
> 
> 1) all USB devices suspend
> 2) root hub suspend
> 3) host controller suspend
> 
> In 1), xhci_device_suspend() call back will be called for each device 
> suspend.
> In 3) "save context operation" will be executed.
> 
> In this case, if FSC is supported, xhci_device_suspend() could skip asking
> host hardware for cache operation.

That's basically what I said.

But now why should "msg" matter?  It seems that xhci_device_suspend() 
should skip asking for the cache operation whenever FSC is supported, 
regardless of whether you're talking about runtime suspend or system 
suspend.

> > Also, it's still not clear what the driver needs to do differently in
> > order to push out the cached data.  You have managed to imply both:
> >
> > 	Issuing Stop Endpoint with SP set is mandatory whenever
> > 	a device is suspended, and
> >
> > 	The SP bit is what tells the controller to push the endpoint
> > 	state to memory.
> >
> > This doesn't make sense.
> 
> Software doesn't handle the cache directly. XHC driver just tell hardware
> that a device is going to suspend. XHC driver does this through stop 
> endpoint
> command by setting the SP (suspend) bit in the command trb.
> 
> During xHC handles stop endpoint command, it will check SP bit, if it's set,
> xHC knows that the endpoint state could be pushed out to memory now,
> since device is going to suspend. The operation of cache is very hardware
> implementation dependent.

Does that mean the driver has no control over whether "stop endpoint 
with SP set" will push the cached information into memory?  In that 
case, how can xhci_device_suspend skip asking the hardware for the 
cache operation?

> > One last question: How much extra time does it take to push the cached
> > data to memory?  Bear in mind that suspending a device is not a rapid
> > operation; if pushing the data takes no more than a few microseconds, I
> > think it would be worthwhile to do it always, even when it's not
> > necessary.
> 
> It depends on how much time software spends in xhci_device_suspend().
> I don't have data in hand, but I agree with you that if this time is no more
> than a few microseconds, we can let do it always.

Above you seem to be saying that there's no way to prevent it!  Your
exact words are: "The operation of cache is very hardware
implementation dependent."

Alan Stern

Re: [PATCH v2 1/3] usb: notify hcd when USB device suspend or resume

[Lu, Baolu]

On 05/12/2015 11:54 PM, Alan Stern wrote:
> On Tue, 12 May 2015, Lu, Baolu wrote:
>
>> I'm sorry that I confused you.
>>
>> FSC is a different thing from what this patch series does.
> I know that.  The patch series, in its current form, is fine.  Now I'm
> trying to understand what you originally wanted to do.
>
>>> Let's see if I understand it correctly:
>>>
>>> 	When the controller goes into suspend, you want the cache to
>>> 	be pushed into memory so that the cache can be powered down,
>>> 	thereby saving additional energy.
>> Not the controller goes into suspend, but USB devices.
> Suppose a USB device goes into runtime suspend, and suppose the
> hardware pushes the cached information for the endpoints on that device
> into memory.  The cache will still contain information for other,
> non-suspended devices.  Consequently the cache _can't_ be powered down
> at this time, right?  You seem to be saying:
>
> 	When a USB device goes into suspend, you want the cache to
> 	be pushed into memory so that the cache can be powered down,
> 	thereby saving additional energy.
>
> But that is just wrong!  The only time you know the hardware can safely
> power-down the cache is when the _controller_ goes into suspend.  At
> that time you _know_ all the attached devices are suspended, so the
> cache isn't needed.
>
> Therefore, when the controller goes into suspend, you want to make sure
> that the cache has been pushed into memory.  The actual push operation
> can occur earlier, during xhci_device_suspend, but the time when the
> information's location matters is during xhci_suspend.
>
>> In order to talking to USB devices, xHCI has an endpoint state for each
>> endpoint of a device. When a USB device goes into suspend, xHC driver
>> could ask the hardware to push the state from cache to the memory.
>> Thereby saving additional energy. This is the intention of this patch
>> series.
> You don't save any energy when the _device_ goes into suspend.  You
> save energy only when the _controller_ goes into suspend, because
> that's the only time when the cache can be powered down.  Right?

Yes, I agree with you.

Hardware could only power down the cache after all data has been
pushed out. That could happen during host suspend as far as I can see.

>
>>> 	If the hardware supports FSC, this will happen automatically.
>>>
>>> 	If the hardware doesn't support FSC, the cached data won't get
>>> 	pushed to memory unless the driver tells the controller to do
>>> 	so at the time the device is suspended.  But this will slow
>>> 	things down, so the driver should avoid doing it when it's not
>>> 	needed.
>>>
>>> 	During system suspend you know in advance that the controller
>>> 	will be suspended.  Therefore the driver should push the cache
>>> 	to memory if the hardware doesn't support FSC.  During runtime
>>> 	suspend you don't know in advance whether the controller will
>>> 	be suspended, so the driver should not push the cache to
>>> 	memory.
>>>
>>> 	But what happens in the case where all the devices have gone
>>> 	into runtime suspend, so the controller also goes into runtime
>>> 	suspend, and the hardware doesn't support FSC?  It seems that
>>> 	in this case the cache would have to remain powered on.
>> FSC is not part of this patch series. It was introduced when I tried to
>> explain the reason why I kept 'msg' parameter in the callback.
>>
>> If the hardware support FSC, another operation named "save context
>> operation" will push everything in hardware cache into memory. So when
>> a USB device is going to suspend and xhci_device_suspend() callback is
>> being called, software can do an optimization. That is, if "save context
>> operation" will push everything in hardware cache to memory later,
>> xhci_device_suspend() could skip asking hardware for cache operation
>> and let "save context operation" do it later.
> That's more or less the same as what I wrote above, except that I said
> it would happen automatically if the hardware supports FSC.  It isn't
> automatic; it requires the driver to issue a "save context operation"
> command.

Yes, exactly.

>
>> One example of this situation is system suspend. During system suspend,
>> below process will be done for a USB3 fabric.
>>
>> 1) all USB devices suspend
>> 2) root hub suspend
>> 3) host controller suspend
>>
>> In 1), xhci_device_suspend() call back will be called for each device
>> suspend.
>> In 3) "save context operation" will be executed.
>>
>> In this case, if FSC is supported, xhci_device_suspend() could skip asking
>> host hardware for cache operation.
> That's basically what I said.
>
> But now why should "msg" matter?  It seems that xhci_device_suspend()
> should skip asking for the cache operation whenever FSC is supported,
> regardless of whether you're talking about runtime suspend or system
> suspend.

In runtime case, a USB device could be selected to suspend while host
controller is busy with other USB devices. Since FSC only impacts the
behavior of save context operation, it's probably that a USB device
is selectively suspended while it's state is still kept in cache.

My thinking is that although cache management is hardware implementation
dependent, it's always a good behavior for software to ask for the cache
operation whenever it realizes that caching the state is unnecessary. This
is good not only for power saving, but also for better cache utilization.

>
>>> Also, it's still not clear what the driver needs to do differently in
>>> order to push out the cached data.  You have managed to imply both:
>>>
>>> 	Issuing Stop Endpoint with SP set is mandatory whenever
>>> 	a device is suspended, and
>>>
>>> 	The SP bit is what tells the controller to push the endpoint
>>> 	state to memory.
>>>
>>> This doesn't make sense.
>> Software doesn't handle the cache directly. XHC driver just tell hardware
>> that a device is going to suspend. XHC driver does this through stop
>> endpoint
>> command by setting the SP (suspend) bit in the command trb.
>>
>> During xHC handles stop endpoint command, it will check SP bit, if it's set,
>> xHC knows that the endpoint state could be pushed out to memory now,
>> since device is going to suspend. The operation of cache is very hardware
>> implementation dependent.
> Does that mean the driver has no control over whether "stop endpoint
> with SP set" will push the cached information into memory?  In that
> case, how can xhci_device_suspend skip asking the hardware for the
> cache operation?

XHCI spec doesn't define what hardware does when it sees SP bit set in
stop endpoint command. An implementation can even be designed without
an internal cache. But whenever cache is used, spec does recommend
and assume that hardware should push cached state into system memory
when SP bit set.

Spec (1.1) has an implementation note on page 253. It says,

If an implementation uses the Scratchpad or non-volatile internal memory
to cache endpoint state rather than Endpoint Contexts, then the Save State
operation can flush that state to the Scratchpad and software does not have
to issue Stop Endpoint Commands as part of the suspend process.

>
>>> One last question: How much extra time does it take to push the cached
>>> data to memory?  Bear in mind that suspending a device is not a rapid
>>> operation; if pushing the data takes no more than a few microseconds, I
>>> think it would be worthwhile to do it always, even when it's not
>>> necessary.
>> It depends on how much time software spends in xhci_device_suspend().
>> I don't have data in hand, but I agree with you that if this time is no more
>> than a few microseconds, we can let do it always.
> Above you seem to be saying that there's no way to prevent it!  Your
> exact words are: "The operation of cache is very hardware
> implementation dependent."

I explained my intention of "the operation of cache is very hardware
implementation dependent" above.

 From software point of view, the extra time it takes to ask for cache
operation can be measured in xhci_device_suspend(). I can measure
the data later when I complete my tasks in hand. If that data is ignorable
comparing to the suspend time, we can simply do it always.

>
> Alan Stern

Thank you!
-Baolu

>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>

Re: [PATCH v2 1/3] usb: notify hcd when USB device suspend or resume

[Alan Stern]

On Wed, 13 May 2015, Lu, Baolu wrote:

>  From software point of view, the extra time it takes to ask for cache
> operation can be measured in xhci_device_suspend(). I can measure
> the data later when I complete my tasks in hand. If that data is ignorable
> comparing to the suspend time, we can simply do it always.

Or do it whenever the hardware doesn't support FSC.  That seems 
reasonable.

Alan Stern

Re: [PATCH v2 1/3] usb: notify hcd when USB device suspend or resume

[Lu, Baolu]

On 05/06/2015 10:35 PM, Alan Stern wrote:
>> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
>> >index 68b1e83..621d9b7 100644
>> >--- a/include/linux/usb/hcd.h
>> >+++ b/include/linux/usb/hcd.h
>> >@@ -383,7 +383,13 @@ struct hc_driver {
>> >  	int	(*find_raw_port_number)(struct usb_hcd *, int);
>> >  	/* Call for power on/off the port if necessary */
>> >  	int	(*port_power)(struct usb_hcd *hcd, int portnum, bool enable);
>> >-
>> >+	/* Call back to hcd when a USB device is going to suspend or just
>> >+	 * resumed.
>> >+	 */
>> >+	void	(*device_suspend)(struct usb_hcd *, struct usb_device *udev,
>> >+			pm_message_t msg);
>> >+	void	(*device_resume)(struct usb_hcd *, struct usb_device *udev,
>> >+			pm_message_t msg);
>> >  };
> Your callbacks don't use the msg argument.  What makes you think it is
> needed?

'msg' arguments are not used in this patch series. I will remove them.

>
> Alan Stern

Thank you,
Baolu

[PATCH v2 2/3] usb: xhci: implement device_suspend/device_resume entries

[Lu Baolu]

This patch implements device_suspend/device_resume entries for xHC driver.
device_suspend will be called when a USB device is about to suspend. It
will issue a stop endpoint command for each endpoint in this device. The
Suspend(SP) bit in the command TRB will set which will give xHC a hint
about the suspend. device_resume will be called when a USB device is just
resumed. It will ring doorbells of all endpoint unconditionally. XHC may
use these suspend/resume hints to optimize its operation.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/usb/host/xhci-hub.c |  2 +-
 drivers/usb/host/xhci.c     | 40 ++++++++++++++++++++++++++++++++++++++++
 drivers/usb/host/xhci.h     |  5 +++++
 3 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 0827d7c..a83e82e 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -266,7 +266,7 @@ int xhci_find_slot_id_by_port(struct usb_hcd *hcd, struct xhci_hcd *xhci,
  * to complete.
  * suspend will set to 1, if suspend bit need to set in command.
  */
-static int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
+int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend)
 {
 	struct xhci_virt_device *virt_dev;
 	struct xhci_command *cmd;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index ec8ac16..b639627 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4680,6 +4680,30 @@ int xhci_disable_usb3_lpm_timeout(struct usb_hcd *hcd,
 		return ret;
 	return 0;
 }
+
+void xhci_device_suspend(struct usb_hcd *hcd,
+			struct usb_device *udev, pm_message_t msg)
+{
+	struct xhci_hcd	*xhci;
+
+	xhci = hcd_to_xhci(hcd);
+	if (!xhci || !xhci->devs[udev->slot_id])
+		return;
+
+	xhci_stop_device(xhci, udev->slot_id, 1);
+}
+
+void xhci_device_resume(struct usb_hcd *hcd,
+			struct usb_device *udev, pm_message_t msg)
+{
+	struct xhci_hcd	*xhci;
+
+	xhci = hcd_to_xhci(hcd);
+	if (!xhci || !xhci->devs[udev->slot_id])
+		return;
+
+	xhci_ring_device(xhci, udev->slot_id);
+}
 #else /* CONFIG_PM */
 
 int xhci_set_usb2_hardware_lpm(struct usb_hcd *hcd,
@@ -4704,6 +4728,16 @@ int xhci_disable_usb3_lpm_timeout(struct usb_hcd *hcd,
 {
 	return 0;
 }
+
+void xhci_device_suspend(struct usb_hcd *hcd,
+			struct usb_device *udev, pm_message_t msg)
+{
+}
+
+void xhci_device_resume(struct usb_hcd *hcd,
+			struct usb_device *udev, pm_message_t msg)
+{
+}
 #endif	/* CONFIG_PM */
 
 /*-------------------------------------------------------------------------*/
@@ -4976,6 +5010,12 @@ static const struct hc_driver xhci_hc_driver = {
 	.enable_usb3_lpm_timeout =	xhci_enable_usb3_lpm_timeout,
 	.disable_usb3_lpm_timeout =	xhci_disable_usb3_lpm_timeout,
 	.find_raw_port_number =	xhci_find_raw_port_number,
+
+	/*
+	 * call back when devices suspend or resume
+	 */
+	.device_suspend =	xhci_device_suspend,
+	.device_resume =	xhci_device_resume,
 };
 
 void xhci_init_driver(struct hc_driver *drv, int (*setup_fn)(struct usb_hcd *))
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 8e421b8..3d1f73b 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1867,10 +1867,15 @@ u32 xhci_port_state_to_neutral(u32 state);
 int xhci_find_slot_id_by_port(struct usb_hcd *hcd, struct xhci_hcd *xhci,
 		u16 port);
 void xhci_ring_device(struct xhci_hcd *xhci, int slot_id);
+int xhci_stop_device(struct xhci_hcd *xhci, int slot_id, int suspend);
 
 /* xHCI contexts */
 struct xhci_input_control_ctx *xhci_get_input_control_ctx(struct xhci_container_ctx *ctx);
 struct xhci_slot_ctx *xhci_get_slot_ctx(struct xhci_hcd *xhci, struct xhci_container_ctx *ctx);
 struct xhci_ep_ctx *xhci_get_ep_ctx(struct xhci_hcd *xhci, struct xhci_container_ctx *ctx, unsigned int ep_index);
 
+void xhci_device_suspend(struct usb_hcd *hcd,
+		struct usb_device *udev, pm_message_t msg);
+void xhci_device_resume(struct usb_hcd *hcd,
+		struct usb_device *udev, pm_message_t msg);
 #endif /* __LINUX_XHCI_HCD_H */
-- 
2.1.0

Re: [PATCH v2 2/3] usb: xhci: implement device_suspend/device_resume entries

[Alan Stern]

On Wed, 6 May 2015, Lu Baolu wrote:

> This patch implements device_suspend/device_resume entries for xHC driver.
> device_suspend will be called when a USB device is about to suspend. It
> will issue a stop endpoint command for each endpoint in this device. The
> Suspend(SP) bit in the command TRB will set which will give xHC a hint
> about the suspend. device_resume will be called when a USB device is just
> resumed. It will ring doorbells of all endpoint unconditionally. XHC may
> use these suspend/resume hints to optimize its operation.
> 
> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>

...

>  #else /* CONFIG_PM */
>  
>  int xhci_set_usb2_hardware_lpm(struct usb_hcd *hcd,
> @@ -4704,6 +4728,16 @@ int xhci_disable_usb3_lpm_timeout(struct usb_hcd *hcd,
>  {
>  	return 0;
>  }
> +
> +void xhci_device_suspend(struct usb_hcd *hcd,
> +			struct usb_device *udev, pm_message_t msg)
> +{
> +}
> +
> +void xhci_device_resume(struct usb_hcd *hcd,
> +			struct usb_device *udev, pm_message_t msg)
> +{
> +}

You don't need to have empty functions.  Just do this:

#define xhci_device_suspend	NULL
#define xhci_device_resume	NULL

in the appropriate place, when CONFIG_PM is not enabled.

Alan Stern

Re: [PATCH v2 2/3] usb: xhci: implement device_suspend/device_resume entries

[Lu, Baolu]

On 05/06/2015 10:30 PM, Alan Stern wrote:
> On Wed, 6 May 2015, Lu Baolu wrote:
>
>> This patch implements device_suspend/device_resume entries for xHC driver.
>> device_suspend will be called when a USB device is about to suspend. It
>> will issue a stop endpoint command for each endpoint in this device. The
>> Suspend(SP) bit in the command TRB will set which will give xHC a hint
>> about the suspend. device_resume will be called when a USB device is just
>> resumed. It will ring doorbells of all endpoint unconditionally. XHC may
>> use these suspend/resume hints to optimize its operation.
>>
>> Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
> ...
>
>>   #else /* CONFIG_PM */
>>   
>>   int xhci_set_usb2_hardware_lpm(struct usb_hcd *hcd,
>> @@ -4704,6 +4728,16 @@ int xhci_disable_usb3_lpm_timeout(struct usb_hcd *hcd,
>>   {
>>   	return 0;
>>   }
>> +
>> +void xhci_device_suspend(struct usb_hcd *hcd,
>> +			struct usb_device *udev, pm_message_t msg)
>> +{
>> +}
>> +
>> +void xhci_device_resume(struct usb_hcd *hcd,
>> +			struct usb_device *udev, pm_message_t msg)
>> +{
>> +}
> You don't need to have empty functions.  Just do this:
>
> #define xhci_device_suspend	NULL
> #define xhci_device_resume	NULL
>
> in the appropriate place, when CONFIG_PM is not enabled.

Yes, I agree. I will change it.

>
> Alan Stern

Thank you,
Baolu

>
>
>

[PATCH v2 3/3] usb: xhci: remove stop device and ring doorbell in hub control and bus suspend

[Lu Baolu]

There is no need to call xhci_stop_device() and xhci_ring_device() in
hub control and bus suspend functions since all device suspend and
resume have been notified through device_suspend/device_resume interfaces.

Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/usb/host/xhci-hub.c | 47 ---------------------------------------------
 1 file changed, 47 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index a83e82e..f12e1b7 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -704,7 +704,6 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 	u32 temp, status;
 	int retval = 0;
 	__le32 __iomem **port_array;
-	int slot_id;
 	struct xhci_bus_state *bus_state;
 	u16 link_state = 0;
 	u16 wake_mask = 0;
@@ -818,17 +817,6 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 				goto error;
 			}
 
-			slot_id = xhci_find_slot_id_by_port(hcd, xhci,
-					wIndex + 1);
-			if (!slot_id) {
-				xhci_warn(xhci, "slot_id is zero\n");
-				goto error;
-			}
-			/* unlock to execute stop endpoint commands */
-			spin_unlock_irqrestore(&xhci->lock, flags);
-			xhci_stop_device(xhci, slot_id, 1);
-			spin_lock_irqsave(&xhci->lock, flags);
-
 			xhci_set_link_state(xhci, port_array, wIndex, XDEV_U3);
 
 			spin_unlock_irqrestore(&xhci->lock, flags);
@@ -876,19 +864,6 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 				goto error;
 			}
 
-			if (link_state == USB_SS_PORT_LS_U3) {
-				slot_id = xhci_find_slot_id_by_port(hcd, xhci,
-						wIndex + 1);
-				if (slot_id) {
-					/* unlock to execute stop endpoint
-					 * commands */
-					spin_unlock_irqrestore(&xhci->lock,
-								flags);
-					xhci_stop_device(xhci, slot_id, 1);
-					spin_lock_irqsave(&xhci->lock, flags);
-				}
-			}
-
 			xhci_set_link_state(xhci, port_array, wIndex,
 						link_state);
 
@@ -994,14 +969,6 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 							XDEV_U0);
 			}
 			bus_state->port_c_suspend |= 1 << wIndex;
-
-			slot_id = xhci_find_slot_id_by_port(hcd, xhci,
-					wIndex + 1);
-			if (!slot_id) {
-				xhci_dbg(xhci, "slot_id is zero\n");
-				goto error;
-			}
-			xhci_ring_device(xhci, slot_id);
 			break;
 		case USB_PORT_FEAT_C_SUSPEND:
 			bus_state->port_c_suspend &= ~(1 << wIndex);
@@ -1133,20 +1100,12 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
 	while (port_index--) {
 		/* suspend the port if the port is not suspended */
 		u32 t1, t2;
-		int slot_id;
 
 		t1 = readl(port_array[port_index]);
 		t2 = xhci_port_state_to_neutral(t1);
 
 		if ((t1 & PORT_PE) && !(t1 & PORT_PLS_MASK)) {
 			xhci_dbg(xhci, "port %d not suspended\n", port_index);
-			slot_id = xhci_find_slot_id_by_port(hcd, xhci,
-					port_index + 1);
-			if (slot_id) {
-				spin_unlock_irqrestore(&xhci->lock, flags);
-				xhci_stop_device(xhci, slot_id, 1);
-				spin_lock_irqsave(&xhci->lock, flags);
-			}
 			t2 &= ~PORT_PLS_MASK;
 			t2 |= PORT_LINK_STROBE | XDEV_U3;
 			set_bit(port_index, &bus_state->bus_suspended);
@@ -1207,7 +1166,6 @@ int xhci_bus_resume(struct usb_hcd *hcd)
 		/* Check whether need resume ports. If needed
 		   resume port and disable remote wakeup */
 		u32 temp;
-		int slot_id;
 
 		temp = readl(port_array[port_index]);
 		if (DEV_SUPERSPEED(temp))
@@ -1240,11 +1198,6 @@ int xhci_bus_resume(struct usb_hcd *hcd)
 			/* Clear PLC */
 			xhci_test_and_clear_bit(xhci, port_array, port_index,
 						PORT_PLC);
-
-			slot_id = xhci_find_slot_id_by_port(hcd,
-					xhci, port_index + 1);
-			if (slot_id)
-				xhci_ring_device(xhci, slot_id);
 		} else
 			writel(temp, port_array[port_index]);
 	}
-- 
2.1.0