[PATCH 0/2] Handle PM events for pci resume

[PATCH 0/2] Handle PM events for pci resume

[Basavaraj Natikar]

This series includes enhancements to the PCI resume function that allow it
to handle PM events in order to improve the XHCI resume time.

Basavaraj Natikar (2):
  USB: Extend pci resume function to handle PM events
  xhci: Improve the XHCI resume time

 drivers/usb/core/hcd-pci.c  | 14 ++++++++------
 drivers/usb/host/ehci-pci.c |  3 ++-
 drivers/usb/host/ohci-pci.c |  8 +++++++-
 drivers/usb/host/uhci-pci.c | 10 +++++++---
 drivers/usb/host/xhci-pci.c |  4 ++--
 drivers/usb/host/xhci.c     |  5 +++--
 drivers/usb/host/xhci.h     |  2 +-
 include/linux/usb/hcd.h     |  2 +-
 8 files changed, 31 insertions(+), 17 deletions(-)

-- 
2.25.1

[PATCH 1/2] USB: Extend pci resume function to handle PM events

[Basavaraj Natikar]

Currently, the pci_resume method has only a flag indicating whether the
system is resuming from hibernation. In order to handle all PM events like
AUTO_RESUME, SUSPEND etc change the pci_resume method to handle all PM
events.

Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
 drivers/usb/core/hcd-pci.c  | 14 ++++++++------
 drivers/usb/host/ehci-pci.c |  3 ++-
 drivers/usb/host/ohci-pci.c |  8 +++++++-
 drivers/usb/host/uhci-pci.c | 10 +++++++---
 drivers/usb/host/xhci-pci.c |  4 ++--
 drivers/usb/host/xhci.c     |  3 ++-
 drivers/usb/host/xhci.h     |  2 +-
 include/linux/usb/hcd.h     |  2 +-
 8 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
index ab2f3737764e..bef092da477a 100644
--- a/drivers/usb/core/hcd-pci.c
+++ b/drivers/usb/core/hcd-pci.c
@@ -415,12 +415,15 @@ static int check_root_hub_suspended(struct device *dev)
 	return 0;
 }
 
-static int suspend_common(struct device *dev, bool do_wakeup)
+static int suspend_common(struct device *dev, int event)
 {
 	struct pci_dev		*pci_dev = to_pci_dev(dev);
 	struct usb_hcd		*hcd = pci_get_drvdata(pci_dev);
+	bool			do_wakeup;
 	int			retval;
 
+	do_wakeup = event == PM_EVENT_AUTO_SUSPEND ? true : device_may_wakeup(dev);
+
 	/* Root hub suspend should have stopped all downstream traffic,
 	 * and all bus master traffic.  And done so for both the interface
 	 * and the stub usb_device (which we check here).  But maybe it
@@ -447,7 +450,7 @@ static int suspend_common(struct device *dev, bool do_wakeup)
 				(retval == 0 && do_wakeup && hcd->shared_hcd &&
 				 HCD_WAKEUP_PENDING(hcd->shared_hcd))) {
 			if (hcd->driver->pci_resume)
-				hcd->driver->pci_resume(hcd, false);
+				hcd->driver->pci_resume(hcd, event);
 			retval = -EBUSY;
 		}
 		if (retval)
@@ -502,8 +505,7 @@ static int resume_common(struct device *dev, int event)
 			for_each_companion(pci_dev, hcd,
 					ehci_wait_for_companions);
 
-		retval = hcd->driver->pci_resume(hcd,
-				event == PM_EVENT_RESTORE);
+		retval = hcd->driver->pci_resume(hcd, event);
 		if (retval) {
 			dev_err(dev, "PCI post-resume error %d!\n", retval);
 			usb_hc_died(hcd);
@@ -516,7 +518,7 @@ static int resume_common(struct device *dev, int event)
 
 static int hcd_pci_suspend(struct device *dev)
 {
-	return suspend_common(dev, device_may_wakeup(dev));
+	return suspend_common(dev, PM_EVENT_SUSPEND);
 }
 
 static int hcd_pci_suspend_noirq(struct device *dev)
@@ -600,7 +602,7 @@ static int hcd_pci_runtime_suspend(struct device *dev)
 {
 	int	retval;
 
-	retval = suspend_common(dev, true);
+	retval = suspend_common(dev, PM_EVENT_AUTO_SUSPEND);
 	if (retval == 0)
 		powermac_set_asic(to_pci_dev(dev), 0);
 	dev_dbg(dev, "hcd_pci_runtime_suspend: %d\n", retval);
diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
index 4b148fe5e43b..1145c6e7fae5 100644
--- a/drivers/usb/host/ehci-pci.c
+++ b/drivers/usb/host/ehci-pci.c
@@ -354,10 +354,11 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
  * Also they depend on separate root hub suspend/resume.
  */
 
-static int ehci_pci_resume(struct usb_hcd *hcd, bool hibernated)
+static int ehci_pci_resume(struct usb_hcd *hcd, int event)
 {
 	struct ehci_hcd		*ehci = hcd_to_ehci(hcd);
 	struct pci_dev		*pdev = to_pci_dev(hcd->self.controller);
+	bool hibernated = event == PM_EVENT_RESTORE;
 
 	if (ehci_resume(hcd, hibernated) != 0)
 		(void) ehci_pci_reinit(ehci, pdev);
diff --git a/drivers/usb/host/ohci-pci.c b/drivers/usb/host/ohci-pci.c
index d7b4f40f9ff4..923ed502414b 100644
--- a/drivers/usb/host/ohci-pci.c
+++ b/drivers/usb/host/ohci-pci.c
@@ -301,6 +301,12 @@ static struct pci_driver ohci_pci_driver = {
 #endif
 };
 
+#ifdef CONFIG_PM
+static int ohci_pci_resume(struct usb_hcd *hcd, int event)
+{
+	return ohci_resume(hcd, event == PM_EVENT_RESTORE);
+}
+#endif
 static int __init ohci_pci_init(void)
 {
 	if (usb_disabled())
@@ -311,7 +317,7 @@ static int __init ohci_pci_init(void)
 #ifdef	CONFIG_PM
 	/* Entries for the PCI suspend/resume callbacks are special */
 	ohci_pci_hc_driver.pci_suspend = ohci_suspend;
-	ohci_pci_hc_driver.pci_resume = ohci_resume;
+	ohci_pci_hc_driver.pci_resume = ohci_pci_resume;
 #endif
 
 	return pci_register_driver(&ohci_pci_driver);
diff --git a/drivers/usb/host/uhci-pci.c b/drivers/usb/host/uhci-pci.c
index 3592f757fe05..9b90c3221bd8 100644
--- a/drivers/usb/host/uhci-pci.c
+++ b/drivers/usb/host/uhci-pci.c
@@ -167,7 +167,7 @@ static void uhci_shutdown(struct pci_dev *pdev)
 
 #ifdef CONFIG_PM
 
-static int uhci_pci_resume(struct usb_hcd *hcd, bool hibernated);
+static int uhci_resume(struct usb_hcd *hcd, bool hibernated);
 
 static int uhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 {
@@ -202,13 +202,13 @@ static int uhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 
 	/* Check for race with a wakeup request */
 	if (do_wakeup && HCD_WAKEUP_PENDING(hcd)) {
-		uhci_pci_resume(hcd, false);
+		uhci_resume(hcd, false);
 		rc = -EBUSY;
 	}
 	return rc;
 }
 
-static int uhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
+static int uhci_resume(struct usb_hcd *hcd, bool hibernated)
 {
 	struct uhci_hcd *uhci = hcd_to_uhci(hcd);
 
@@ -252,6 +252,10 @@ static int uhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
 	return 0;
 }
 
+static int uhci_pci_resume(struct usb_hcd *hcd, int event)
+{
+	return uhci_resume(hcd, event == PM_EVENT_RESTORE);
+}
 #endif
 
 static const struct hc_driver uhci_driver = {
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 6db07ca419c3..ebdf9f32d128 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -628,7 +628,7 @@ static int xhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
 	return ret;
 }
 
-static int xhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
+static int xhci_pci_resume(struct usb_hcd *hcd, int event)
 {
 	struct xhci_hcd		*xhci = hcd_to_xhci(hcd);
 	struct pci_dev		*pdev = to_pci_dev(hcd->self.controller);
@@ -663,7 +663,7 @@ static int xhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
 	if (xhci->quirks & XHCI_PME_STUCK_QUIRK)
 		xhci_pme_quirk(hcd);
 
-	retval = xhci_resume(xhci, hibernated);
+	retval = xhci_resume(xhci, event);
 	return retval;
 }
 
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 6307bae9cddf..a539e4dd54f7 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1157,8 +1157,9 @@ EXPORT_SYMBOL_GPL(xhci_suspend);
  * This is called when the machine transition from S3/S4 mode.
  *
  */
-int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
+int xhci_resume(struct xhci_hcd *xhci, int event)
 {
+	bool			hibernated = event == PM_EVENT_RESTORE;
 	u32			command, temp = 0;
 	struct usb_hcd		*hcd = xhci_to_hcd(xhci);
 	int			retval = 0;
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 786002bb35db..948fc2d7b1f0 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -2139,7 +2139,7 @@ int xhci_disable_slot(struct xhci_hcd *xhci, u32 slot_id);
 int xhci_ext_cap_init(struct xhci_hcd *xhci);
 
 int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup);
-int xhci_resume(struct xhci_hcd *xhci, bool hibernated);
+int xhci_resume(struct xhci_hcd *xhci, int event);
 
 irqreturn_t xhci_irq(struct usb_hcd *hcd);
 irqreturn_t xhci_msi_irq(int irq, void *hcd);
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index b51c07111729..337575dd8665 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -267,7 +267,7 @@ struct hc_driver {
 	int	(*pci_suspend)(struct usb_hcd *hcd, bool do_wakeup);
 
 	/* called after entering D0 (etc), before resuming the hub */
-	int	(*pci_resume)(struct usb_hcd *hcd, bool hibernated);
+	int	(*pci_resume)(struct usb_hcd *hcd, int event);
 
 	/* called just before hibernate final D3 state, allows host to poweroff parts */
 	int	(*pci_poweroff_late)(struct usb_hcd *hcd, bool do_wakeup);
-- 
2.25.1

Re: [PATCH 1/2] USB: Extend pci resume function to handle PM events

[Alan Stern]

On Tue, Apr 18, 2023 at 07:38:16PM +0530, Basavaraj Natikar wrote:
> Currently, the pci_resume method has only a flag indicating whether the
> system is resuming from hibernation. In order to handle all PM events like
> AUTO_RESUME, SUSPEND etc change the pci_resume method to handle all PM
> events.

You might want to make a different kind of distinction between the 
various sorts of resume.  For example, a resume from runtime suspend 
can occur either because of a request from the system (it needs to start 
using the device) or a remote wakeup request from an attached device.  
The different sorts of resume might have different requirements.


> diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
> index 4b148fe5e43b..1145c6e7fae5 100644
> --- a/drivers/usb/host/ehci-pci.c
> +++ b/drivers/usb/host/ehci-pci.c
> @@ -354,10 +354,11 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
>   * Also they depend on separate root hub suspend/resume.
>   */
>  
> -static int ehci_pci_resume(struct usb_hcd *hcd, bool hibernated)
> +static int ehci_pci_resume(struct usb_hcd *hcd, int event)
>  {
>  	struct ehci_hcd		*ehci = hcd_to_ehci(hcd);
>  	struct pci_dev		*pdev = to_pci_dev(hcd->self.controller);
> +	bool hibernated = event == PM_EVENT_RESTORE;

Please use the same indentation style as the surrounding code.  Also, 
when a boolean expression is used in an assignment, I prefer to put it 
in parentheses to help set it off from the assignment operator:

	bool			hibernated = (event == PM_EVENT_RESTORE);


> diff --git a/drivers/usb/host/uhci-pci.c b/drivers/usb/host/uhci-pci.c
> index 3592f757fe05..9b90c3221bd8 100644
> --- a/drivers/usb/host/uhci-pci.c
> +++ b/drivers/usb/host/uhci-pci.c
> @@ -167,7 +167,7 @@ static void uhci_shutdown(struct pci_dev *pdev)
>  
>  #ifdef CONFIG_PM
>  
> -static int uhci_pci_resume(struct usb_hcd *hcd, bool hibernated);
> +static int uhci_resume(struct usb_hcd *hcd, bool hibernated);

There's no need to change the function's name.  After all, it is static.

>  
>  static int uhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
>  {
> @@ -202,13 +202,13 @@ static int uhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
>  
>  	/* Check for race with a wakeup request */
>  	if (do_wakeup && HCD_WAKEUP_PENDING(hcd)) {
> -		uhci_pci_resume(hcd, false);
> +		uhci_resume(hcd, false);
>  		rc = -EBUSY;
>  	}
>  	return rc;
>  }
>  
> -static int uhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
> +static int uhci_resume(struct usb_hcd *hcd, bool hibernated)
>  {
>  	struct uhci_hcd *uhci = hcd_to_uhci(hcd);
>  
> @@ -252,6 +252,10 @@ static int uhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
>  	return 0;
>  }
>  
> +static int uhci_pci_resume(struct usb_hcd *hcd, int event)
> +{
> +	return uhci_resume(hcd, event == PM_EVENT_RESTORE);
> +}

Again, try to avoid this wrapper.

Alan Stern

Re: [PATCH 1/2] USB: Extend pci resume function to handle PM events

[Basavaraj Natikar]

On 4/18/2023 8:36 PM, Alan Stern wrote:
> On Tue, Apr 18, 2023 at 07:38:16PM +0530, Basavaraj Natikar wrote:
>> Currently, the pci_resume method has only a flag indicating whether the
>> system is resuming from hibernation. In order to handle all PM events like
>> AUTO_RESUME, SUSPEND etc change the pci_resume method to handle all PM
>> events.
> You might want to make a different kind of distinction between the 
> various sorts of resume.  For example, a resume from runtime suspend 
> can occur either because of a request from the system (it needs to start 
> using the device) or a remote wakeup request from an attached device.  
> The different sorts of resume might have different requirements.

yes correct.

>
>> diff --git a/drivers/usb/host/ehci-pci.c b/drivers/usb/host/ehci-pci.c
>> index 4b148fe5e43b..1145c6e7fae5 100644
>> --- a/drivers/usb/host/ehci-pci.c
>> +++ b/drivers/usb/host/ehci-pci.c
>> @@ -354,10 +354,11 @@ static int ehci_pci_setup(struct usb_hcd *hcd)
>>   * Also they depend on separate root hub suspend/resume.
>>   */
>>  
>> -static int ehci_pci_resume(struct usb_hcd *hcd, bool hibernated)
>> +static int ehci_pci_resume(struct usb_hcd *hcd, int event)
>>  {
>>  	struct ehci_hcd		*ehci = hcd_to_ehci(hcd);
>>  	struct pci_dev		*pdev = to_pci_dev(hcd->self.controller);
>> +	bool hibernated = event == PM_EVENT_RESTORE;
> Please use the same indentation style as the surrounding code.  Also, 
> when a boolean expression is used in an assignment, I prefer to put it 
> in parentheses to help set it off from the assignment operator:
>
> 	bool			hibernated = (event == PM_EVENT_RESTORE);

Sure will change it accordingly.

>
>> diff --git a/drivers/usb/host/uhci-pci.c b/drivers/usb/host/uhci-pci.c
>> index 3592f757fe05..9b90c3221bd8 100644
>> --- a/drivers/usb/host/uhci-pci.c
>> +++ b/drivers/usb/host/uhci-pci.c
>> @@ -167,7 +167,7 @@ static void uhci_shutdown(struct pci_dev *pdev)
>>  
>>  #ifdef CONFIG_PM
>>  
>> -static int uhci_pci_resume(struct usb_hcd *hcd, bool hibernated);
>> +static int uhci_resume(struct usb_hcd *hcd, bool hibernated);
> There's no need to change the function's name.  After all, it is static.

sure will keep same function name.

>
>>  
>>  static int uhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
>>  {
>> @@ -202,13 +202,13 @@ static int uhci_pci_suspend(struct usb_hcd *hcd, bool do_wakeup)
>>  
>>  	/* Check for race with a wakeup request */
>>  	if (do_wakeup && HCD_WAKEUP_PENDING(hcd)) {
>> -		uhci_pci_resume(hcd, false);
>> +		uhci_resume(hcd, false);
>>  		rc = -EBUSY;
>>  	}
>>  	return rc;
>>  }
>>  
>> -static int uhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
>> +static int uhci_resume(struct usb_hcd *hcd, bool hibernated)
>>  {
>>  	struct uhci_hcd *uhci = hcd_to_uhci(hcd);
>>  
>> @@ -252,6 +252,10 @@ static int uhci_pci_resume(struct usb_hcd *hcd, bool hibernated)
>>  	return 0;
>>  }
>>  
>> +static int uhci_pci_resume(struct usb_hcd *hcd, int event)
>> +{
>> +	return uhci_resume(hcd, event == PM_EVENT_RESTORE);
>> +}
> Again, try to avoid this wrapper.

Sure will avoid this change. 

Thanks,
--
Basavaraj

>
> Alan Stern

Re: [PATCH 1/2] USB: Extend pci resume function to handle PM events

[Greg KH]

On Tue, Apr 18, 2023 at 07:38:16PM +0530, Basavaraj Natikar wrote:
> Currently, the pci_resume method has only a flag indicating whether the
> system is resuming from hibernation. In order to handle all PM events like
> AUTO_RESUME, SUSPEND etc change the pci_resume method to handle all PM
> events.
> 
> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
> ---
>  drivers/usb/core/hcd-pci.c  | 14 ++++++++------
>  drivers/usb/host/ehci-pci.c |  3 ++-
>  drivers/usb/host/ohci-pci.c |  8 +++++++-
>  drivers/usb/host/uhci-pci.c | 10 +++++++---
>  drivers/usb/host/xhci-pci.c |  4 ++--
>  drivers/usb/host/xhci.c     |  3 ++-
>  drivers/usb/host/xhci.h     |  2 +-
>  include/linux/usb/hcd.h     |  2 +-
>  8 files changed, 30 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
> index ab2f3737764e..bef092da477a 100644
> --- a/drivers/usb/core/hcd-pci.c
> +++ b/drivers/usb/core/hcd-pci.c
> @@ -415,12 +415,15 @@ static int check_root_hub_suspended(struct device *dev)
>  	return 0;
>  }
>  
> -static int suspend_common(struct device *dev, bool do_wakeup)
> +static int suspend_common(struct device *dev, int event)

Shouldn't there be a PM_EVENT_* type for this so that we can properly
type-check that it is being used properly everywhere?  Much like we can
do for GFP_* flags?

Not the fault of this patch, just a general comment...

thanks,

greg k-h

Re: [PATCH 1/2] USB: Extend pci resume function to handle PM events

[Basavaraj Natikar]

On 4/18/2023 8:53 PM, Greg KH wrote:
> On Tue, Apr 18, 2023 at 07:38:16PM +0530, Basavaraj Natikar wrote:
>> Currently, the pci_resume method has only a flag indicating whether the
>> system is resuming from hibernation. In order to handle all PM events like
>> AUTO_RESUME, SUSPEND etc change the pci_resume method to handle all PM
>> events.
>>
>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>> ---
>>  drivers/usb/core/hcd-pci.c  | 14 ++++++++------
>>  drivers/usb/host/ehci-pci.c |  3 ++-
>>  drivers/usb/host/ohci-pci.c |  8 +++++++-
>>  drivers/usb/host/uhci-pci.c | 10 +++++++---
>>  drivers/usb/host/xhci-pci.c |  4 ++--
>>  drivers/usb/host/xhci.c     |  3 ++-
>>  drivers/usb/host/xhci.h     |  2 +-
>>  include/linux/usb/hcd.h     |  2 +-
>>  8 files changed, 30 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/usb/core/hcd-pci.c b/drivers/usb/core/hcd-pci.c
>> index ab2f3737764e..bef092da477a 100644
>> --- a/drivers/usb/core/hcd-pci.c
>> +++ b/drivers/usb/core/hcd-pci.c
>> @@ -415,12 +415,15 @@ static int check_root_hub_suspended(struct device *dev)
>>  	return 0;
>>  }
>>  
>> -static int suspend_common(struct device *dev, bool do_wakeup)
>> +static int suspend_common(struct device *dev, int event)
> Shouldn't there be a PM_EVENT_* type for this so that we can properly
> type-check that it is being used properly everywhere?  Much like we can
> do for GFP_* flags?

yes correct , will change in all place accordingly by using pm_message_t type.

Thanks,
--
Basavaraj

> Not the fault of this patch, just a general comment...
>
> thanks,
>
> greg k-h

[PATCH 2/2] xhci: Improve the XHCI resume time

[Basavaraj Natikar]

xHC system resume time may increase due to a 120ms delay. A PME# signal
will trigger the xHC host to resume runtime, and the host must wait for a
missed U3 LFPS wake signal before re-checking for port activity. It may
be necessary to wait only for auto-resume cases. Add a check only for
runtime resume to avoid the delay for other PM events so that the resume
time can be improved.

Fixes: 253f588c70f6 ("xhci: Improve detection of device initiated wake signal.")
Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
---
 drivers/usb/host/xhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index a539e4dd54f7..a3ee80ee5d1e 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1315,7 +1315,7 @@ int xhci_resume(struct xhci_hcd *xhci, int event)
 		 * the first wake signalling failed, give it that chance.
 		 */
 		pending_portevent = xhci_pending_portevent(xhci);
-		if (!pending_portevent) {
+		if (!pending_portevent && event == PM_EVENT_AUTO_RESUME) {
 			msleep(120);
 			pending_portevent = xhci_pending_portevent(xhci);
 		}
-- 
2.25.1

Re: [PATCH 2/2] xhci: Improve the XHCI resume time

[Greg KH]

On Tue, Apr 18, 2023 at 07:38:17PM +0530, Basavaraj Natikar wrote:
> xHC system resume time may increase due to a 120ms delay.

I'm sorry, but I can not understand this sentence.  Is the delay 120ms
too long?  Or too short?  Or will it change to be always at least 120ms?
Or something else?

> A PME# signal
> will trigger the xHC host to resume runtime, and the host must wait for a
> missed U3 LFPS wake signal before re-checking for port activity. It may
> be necessary to wait only for auto-resume cases. Add a check only for
> runtime resume to avoid the delay for other PM events so that the resume
> time can be improved.

I also can not understand these last two sentences, can you try to
reword it a bit differently?

thanks,

greg k-h

Re: [PATCH 2/2] xhci: Improve the XHCI resume time

[Basavaraj Natikar]

On 4/18/2023 8:55 PM, Greg KH wrote:
> On Tue, Apr 18, 2023 at 07:38:17PM +0530, Basavaraj Natikar wrote:
>> xHC system resume time may increase due to a 120ms delay.
> I'm sorry, but I can not understand this sentence.  Is the delay 120ms
> too long?  Or too short?  Or will it change to be always at least 120ms?
> Or something else?

Each USB controller while xhci_resumes by default takes 120 ms if there
is no activity on the ports or no ports connected. Therefore, if there
are more USB controllers on the system, 120 ms per controller will add
delay to system resume.

This patch will prevent 120 ms for each XHCI controller during system
resume.

>
>> A PME# signal
>> will trigger the xHC host to resume runtime, and the host must wait for a
>> missed U3 LFPS wake signal before re-checking for port activity. It may
>> be necessary to wait only for auto-resume cases. Add a check only for
>> runtime resume to avoid the delay for other PM events so that the resume
>> time can be improved.

Once XHCI controller in runtime suspended state (D3), on USB device hotplug
controller will resume (D0) and check for pending port events if no events,
wait for 120 ms to re-check for port activity i.e. the first wake signalling
failed is handled in below changes.

https://lore.kernel.org/all/20210311115353.2137560-3-mathias.nyman@linux.intel.com/

This check adds delay to system resume case as well, which may be only required for
runtime resume (auto resume) case.

So added an extra check to handle the runtime resume case only.

Will retry to reword accordingly as above.

If you have any input, please let me know.  

Thanks,
--
Basavaraj

> I also can not understand these last two sentences, can you try to
> reword it a bit differently?
>
> thanks,
>
> greg k-h

Re: [PATCH 2/2] xhci: Improve the XHCI resume time

[Mark Hasemeyer]

> It may be necessary to wait only for auto-resume cases.

I find this comment misleading as the patch assumes that it's only necessary to
wait for auto-resume cases. Are there any cases where the driver should wait
during system-resume?

Also, the commit title could specifically mention "system resume".

Re: [PATCH 2/2] xhci: Improve the XHCI resume time

[Basavaraj Natikar]

On 4/20/2023 10:33 PM, Mark Hasemeyer wrote:
>> It may be necessary to wait only for auto-resume cases.
> I find this comment misleading as the patch assumes that it's only necessary to
> wait for auto-resume cases. Are there any cases where the driver should wait
> during system-resume?

Only in case of auto-resume (runtime resume).

Rewording the commit message as follows.

Each XHCI controller while xhci_resumes by default takes 120 ms more if
there is no activity on the ports or no ports connected. Therefore, if
there are more USB controllers on the system, 120 ms more per controller
will add delay to system resume from suspended states like s2idle, S3 or
S4 states.

Once the XHCI controller is in runtime suspended state (D3 state), on USB
device hotplug controller will runtime resume (D0 state) and check for
pending port events if no events, wait for 120 ms to re-check for port
activity to handle missed wake signal. 

A delay of 120 ms more to re-check for port activity is needed only in
auto-resume (runtime resume) cases. Hence, add a check only for runtime
resume from runtime suspend (D3->D0) to avoid the 120ms more delay for
other PM events (system resume from suspend states like s2idle, S3 or S4
states) so that the system resume time can be improved.

Please let me know if any inputs.

>
> Also, the commit title could specifically mention "system resume".

Sure will change title "Improve the XHCI system resume time"

Thanks,
--
Basavaraj

Re: [PATCH 2/2] xhci: Improve the XHCI resume time

[Mathias Nyman]

On 21.4.2023 7.58, Basavaraj Natikar wrote:
> 
> On 4/20/2023 10:33 PM, Mark Hasemeyer wrote:
>>> It may be necessary to wait only for auto-resume cases.
>> I find this comment misleading as the patch assumes that it's only necessary to
>> wait for auto-resume cases. Are there any cases where the driver should wait
>> during system-resume?
> 
> Only in case of auto-resume (runtime resume).
> 
> Rewording the commit message as follows.

Thanks for fixing this extra system resume delay

Maybe some kind of big picture explanation could be added to the commit message,
such as:

Avoid extra 120ms delay during system resume.

xHC controller may signal wake up to 120ms before it shows which USB device
caused the wake on the xHC port registers.

The xhci driver therefore checks for port activity up to 120ms during resume,
making sure that the hub driver can see the port change, and won't immediately
runtime suspend back due to no port activity.

This is however only needed for runtime resume as system resume will resume
all child hubs and other child usb devices anyway.

> 
> Each XHCI controller while xhci_resumes by default takes 120 ms more if
> there is no activity on the ports or no ports connected. Therefore, if
> there are more USB controllers on the system, 120 ms more per controller
> will add delay to system resume from suspended states like s2idle, S3 or
> S4 states.
> 
> Once the XHCI controller is in runtime suspended state (D3 state), on USB
> device hotplug controller will runtime resume (D0 state) and check for
> pending port events if no events, wait for 120 ms to re-check for port
> activity to handle missed wake signal.
> 
> A delay of 120 ms more to re-check for port activity is needed only in
> auto-resume (runtime resume) cases. Hence, add a check only for runtime
> resume from runtime suspend (D3->D0) to avoid the 120ms more delay for
> other PM events (system resume from suspend states like s2idle, S3 or S4
> states) so that the system resume time can be improved.
> 
> Please let me know if any inputs.

I can only think of one minor side-effect that would be runtime suspending back
too early after system resume. This could happen when connecting the first
usb device to a roothub on a (system) suspended setup?

steps:
1. in system suspend, no usb devices connected, xhci in D3, can signal wake with PME#
2. connect first usb device, xHC signals PME# wake
3. system resumes, xhci resumes to D0, but no actity visible on xHC port registers
4. rootubs resumes, no other children on this bus.
5. roothubs sees no activity (due to 120ms max latency before visible on port registers)
6. roothubs runtime suspend
7. xhci runtime suspends
8. same device now causes xHC to PME# wake again,
9. runtime reusume xhci, do wait 120ms for port activity
10. see port activity, resume hub, enumerate device.

It might be that this really isn't an issue due to the the graceperiod fix:

33e321586e37 xhci: Add grace period after xHC start to prevent premature runtime suspend.

Thanks
-Mathias

Re: [PATCH 2/2] xhci: Improve the XHCI resume time

[Wesley Cheng]

Hi Mathias,

On 4/24/2023 8:05 AM, Mathias Nyman wrote:
> On 21.4.2023 7.58, Basavaraj Natikar wrote:
>>
>> On 4/20/2023 10:33 PM, Mark Hasemeyer wrote:
>>>> It may be necessary to wait only for auto-resume cases.
>>> I find this comment misleading as the patch assumes that it's only 
>>> necessary to
>>> wait for auto-resume cases. Are there any cases where the driver 
>>> should wait
>>> during system-resume?
>>
>> Only in case of auto-resume (runtime resume).
>>
>> Rewording the commit message as follows.
> 
> Thanks for fixing this extra system resume delay
> 
> Maybe some kind of big picture explanation could be added to the commit 
> message,
> such as:
> 
> Avoid extra 120ms delay during system resume.
> 
> xHC controller may signal wake up to 120ms before it shows which USB device
> caused the wake on the xHC port registers.
> 
> The xhci driver therefore checks for port activity up to 120ms during 
> resume,
> making sure that the hub driver can see the port change, and won't 
> immediately
> runtime suspend back due to no port activity.
> 
> This is however only needed for runtime resume as system resume will resume
> all child hubs and other child usb devices anyway.
> 
>>
>> Each XHCI controller while xhci_resumes by default takes 120 ms more if
>> there is no activity on the ports or no ports connected. Therefore, if
>> there are more USB controllers on the system, 120 ms more per controller
>> will add delay to system resume from suspended states like s2idle, S3 or
>> S4 states.
>>
>> Once the XHCI controller is in runtime suspended state (D3 state), on USB
>> device hotplug controller will runtime resume (D0 state) and check for
>> pending port events if no events, wait for 120 ms to re-check for port
>> activity to handle missed wake signal.
>>
>> A delay of 120 ms more to re-check for port activity is needed only in
>> auto-resume (runtime resume) cases. Hence, add a check only for runtime
>> resume from runtime suspend (D3->D0) to avoid the 120ms more delay for
>> other PM events (system resume from suspend states like s2idle, S3 or S4
>> states) so that the system resume time can be improved.
>>
>> Please let me know if any inputs.
> 
> I can only think of one minor side-effect that would be runtime 
> suspending back
> too early after system resume. This could happen when connecting the first
> usb device to a roothub on a (system) suspended setup?
> 
> steps:
> 1. in system suspend, no usb devices connected, xhci in D3, can signal 
> wake with PME#
> 2. connect first usb device, xHC signals PME# wake
> 3. system resumes, xhci resumes to D0, but no actity visible on xHC port 
> registers

Thanks for bringing up this topic Basavaraj.

Sorry for jumping into this thread, but was looking to optimize this 
resume timing as well, since it is affecting some of the host driven bus 
resume situations.  Just had a quick question about where the 120ms 
delay is required...

 From what I'm gathering from the USB3 spec, the 120ms timeout is the 
recommended time for tU3WakeupRetryDelay ("Table 7-12. LTSSM State 
Transition Timeouts").  This is the retry time that the device will wait 
before re-issuing another (potential) LFPS U3 wake.

My idea was to see if we could limit this delay only for when a SSUSB 
device is already connected to the root hub.  (ignore if HSUSB device 
connected)  We would be able to eliminate the delay for:
1.  No device connected to root hub
2.  Only HSUSB device connected

Is that a possibility we can add on top of what Basavaraj is adding?

Thanks
Wesley Cheng

Re: [PATCH 2/2] xhci: Improve the XHCI resume time

[Mathias Nyman]

On 25.4.2023 3.09, Wesley Cheng wrote:
> Hi Mathias,
> 
> On 4/24/2023 8:05 AM, Mathias Nyman wrote:
>> On 21.4.2023 7.58, Basavaraj Natikar wrote:
>>>
>>> On 4/20/2023 10:33 PM, Mark Hasemeyer wrote:
>>>>> It may be necessary to wait only for auto-resume cases.
>>>> I find this comment misleading as the patch assumes that it's only necessary to
>>>> wait for auto-resume cases. Are there any cases where the driver should wait
>>>> during system-resume?
>>>
>>> Only in case of auto-resume (runtime resume).
>>>
>>> Rewording the commit message as follows.
>>
>> Thanks for fixing this extra system resume delay
>>
>> Maybe some kind of big picture explanation could be added to the commit message,
>> such as:
>>
>> Avoid extra 120ms delay during system resume.
>>
>> xHC controller may signal wake up to 120ms before it shows which USB device
>> caused the wake on the xHC port registers.
>>
>> The xhci driver therefore checks for port activity up to 120ms during resume,
>> making sure that the hub driver can see the port change, and won't immediately
>> runtime suspend back due to no port activity.
>>
>> This is however only needed for runtime resume as system resume will resume
>> all child hubs and other child usb devices anyway.
>>
>>>
>>> Each XHCI controller while xhci_resumes by default takes 120 ms more if
>>> there is no activity on the ports or no ports connected. Therefore, if
>>> there are more USB controllers on the system, 120 ms more per controller
>>> will add delay to system resume from suspended states like s2idle, S3 or
>>> S4 states.
>>>
>>> Once the XHCI controller is in runtime suspended state (D3 state), on USB
>>> device hotplug controller will runtime resume (D0 state) and check for
>>> pending port events if no events, wait for 120 ms to re-check for port
>>> activity to handle missed wake signal.
>>>
>>> A delay of 120 ms more to re-check for port activity is needed only in
>>> auto-resume (runtime resume) cases. Hence, add a check only for runtime
>>> resume from runtime suspend (D3->D0) to avoid the 120ms more delay for
>>> other PM events (system resume from suspend states like s2idle, S3 or S4
>>> states) so that the system resume time can be improved.
>>>
>>> Please let me know if any inputs.
>>
>> I can only think of one minor side-effect that would be runtime suspending back
>> too early after system resume. This could happen when connecting the first
>> usb device to a roothub on a (system) suspended setup?
>>
>> steps:
>> 1. in system suspend, no usb devices connected, xhci in D3, can signal wake with PME#
>> 2. connect first usb device, xHC signals PME# wake
>> 3. system resumes, xhci resumes to D0, but no actity visible on xHC port registers
> 
> Thanks for bringing up this topic Basavaraj.
> 
> Sorry for jumping into this thread, but was looking to optimize this resume timing as well, since it is affecting some of the host driven bus resume situations.  Just had a quick question about where the 120ms delay is required...
> 
>  From what I'm gathering from the USB3 spec, the 120ms timeout is the recommended time for tU3WakeupRetryDelay ("Table 7-12. LTSSM State Transition Timeouts").  This is the retry time that the device will wait before re-issuing another (potential) LFPS U3 wake.
> 
> My idea was to see if we could limit this delay only for when a SSUSB device is already connected to the root hub.  (ignore if HSUSB device connected)  We would be able to eliminate the delay for:
> 1.  No device connected to root hub
> 2.  Only HSUSB device connected
> 
> Is that a possibility we can add on top of what Basavaraj is adding?
> 

Sounds reasonable,
Yes the 120ms was intended for the U3 wake delay for SuperSpeed devices.

We should probably also check for CAS bit in xhci_pending_portevent()
(I'll add that CAS check)

-Mathias

  

Re: [PATCH 2/2] xhci: Improve the XHCI resume time

[Wesley Cheng]

Hi Mathias,

On 4/25/2023 2:04 AM, Mathias Nyman wrote:
> On 25.4.2023 3.09, Wesley Cheng wrote:
>> Hi Mathias,
>>
>> On 4/24/2023 8:05 AM, Mathias Nyman wrote:
>>> On 21.4.2023 7.58, Basavaraj Natikar wrote:
>>>>
>>>> On 4/20/2023 10:33 PM, Mark Hasemeyer wrote:
>>>>>> It may be necessary to wait only for auto-resume cases.
>>>>> I find this comment misleading as the patch assumes that it's only 
>>>>> necessary to
>>>>> wait for auto-resume cases. Are there any cases where the driver 
>>>>> should wait
>>>>> during system-resume?
>>>>
>>>> Only in case of auto-resume (runtime resume).
>>>>
>>>> Rewording the commit message as follows.
>>>
>>> Thanks for fixing this extra system resume delay
>>>
>>> Maybe some kind of big picture explanation could be added to the 
>>> commit message,
>>> such as:
>>>
>>> Avoid extra 120ms delay during system resume.
>>>
>>> xHC controller may signal wake up to 120ms before it shows which USB 
>>> device
>>> caused the wake on the xHC port registers.
>>>
>>> The xhci driver therefore checks for port activity up to 120ms during 
>>> resume,
>>> making sure that the hub driver can see the port change, and won't 
>>> immediately
>>> runtime suspend back due to no port activity.
>>>
>>> This is however only needed for runtime resume as system resume will 
>>> resume
>>> all child hubs and other child usb devices anyway.
>>>
>>>>
>>>> Each XHCI controller while xhci_resumes by default takes 120 ms more if
>>>> there is no activity on the ports or no ports connected. Therefore, if
>>>> there are more USB controllers on the system, 120 ms more per 
>>>> controller
>>>> will add delay to system resume from suspended states like s2idle, 
>>>> S3 or
>>>> S4 states.
>>>>
>>>> Once the XHCI controller is in runtime suspended state (D3 state), 
>>>> on USB
>>>> device hotplug controller will runtime resume (D0 state) and check for
>>>> pending port events if no events, wait for 120 ms to re-check for port
>>>> activity to handle missed wake signal.
>>>>
>>>> A delay of 120 ms more to re-check for port activity is needed only in
>>>> auto-resume (runtime resume) cases. Hence, add a check only for runtime
>>>> resume from runtime suspend (D3->D0) to avoid the 120ms more delay for
>>>> other PM events (system resume from suspend states like s2idle, S3 
>>>> or S4
>>>> states) so that the system resume time can be improved.
>>>>
>>>> Please let me know if any inputs.
>>>
>>> I can only think of one minor side-effect that would be runtime 
>>> suspending back
>>> too early after system resume. This could happen when connecting the 
>>> first
>>> usb device to a roothub on a (system) suspended setup?
>>>
>>> steps:
>>> 1. in system suspend, no usb devices connected, xhci in D3, can 
>>> signal wake with PME#
>>> 2. connect first usb device, xHC signals PME# wake
>>> 3. system resumes, xhci resumes to D0, but no actity visible on xHC 
>>> port registers
>>
>> Thanks for bringing up this topic Basavaraj.
>>
>> Sorry for jumping into this thread, but was looking to optimize this 
>> resume timing as well, since it is affecting some of the host driven 
>> bus resume situations.  Just had a quick question about where the 
>> 120ms delay is required...
>>
>>  From what I'm gathering from the USB3 spec, the 120ms timeout is the 
>> recommended time for tU3WakeupRetryDelay ("Table 7-12. LTSSM State 
>> Transition Timeouts").  This is the retry time that the device will 
>> wait before re-issuing another (potential) LFPS U3 wake.
>>
>> My idea was to see if we could limit this delay only for when a SSUSB 
>> device is already connected to the root hub.  (ignore if HSUSB device 
>> connected)  We would be able to eliminate the delay for:
>> 1.  No device connected to root hub
>> 2.  Only HSUSB device connected
>>
>> Is that a possibility we can add on top of what Basavaraj is adding?
>>
> 
> Sounds reasonable,
> Yes the 120ms was intended for the U3 wake delay for SuperSpeed devices.
> 
> We should probably also check for CAS bit in xhci_pending_portevent()
> (I'll add that CAS check)
> 

Thanks for the info.  I'll make a change to add the checks I mentioned 
above and submit it as a separate patch for review.

Thanks
Wesley Cheng

Re: [PATCH 2/2] xhci: Improve the XHCI resume time

[Basavaraj Natikar]

On 4/24/2023 8:35 PM, Mathias Nyman wrote:
> On 21.4.2023 7.58, Basavaraj Natikar wrote:
>>
>> On 4/20/2023 10:33 PM, Mark Hasemeyer wrote:
>>>> It may be necessary to wait only for auto-resume cases.
>>> I find this comment misleading as the patch assumes that it's only
>>> necessary to
>>> wait for auto-resume cases. Are there any cases where the driver
>>> should wait
>>> during system-resume?
>>
>> Only in case of auto-resume (runtime resume).
>>
>> Rewording the commit message as follows.
>
> Thanks for fixing this extra system resume delay
>
> Maybe some kind of big picture explanation could be added to the
> commit message,
> such as:
>
> Avoid extra 120ms delay during system resume.
>
> xHC controller may signal wake up to 120ms before it shows which USB
> device
> caused the wake on the xHC port registers.
>
> The xhci driver therefore checks for port activity up to 120ms during
> resume,
> making sure that the hub driver can see the port change, and won't
> immediately
> runtime suspend back due to no port activity.
>
> This is however only needed for runtime resume as system resume will
> resume
> all child hubs and other child usb devices anyway.

Thanks for the explanation. I will change the commit message as stated above.

>
>>
>> Each XHCI controller while xhci_resumes by default takes 120 ms more if
>> there is no activity on the ports or no ports connected. Therefore, if
>> there are more USB controllers on the system, 120 ms more per controller
>> will add delay to system resume from suspended states like s2idle, S3 or
>> S4 states.
>>
>> Once the XHCI controller is in runtime suspended state (D3 state), on
>> USB
>> device hotplug controller will runtime resume (D0 state) and check for
>> pending port events if no events, wait for 120 ms to re-check for port
>> activity to handle missed wake signal.
>>
>> A delay of 120 ms more to re-check for port activity is needed only in
>> auto-resume (runtime resume) cases. Hence, add a check only for runtime
>> resume from runtime suspend (D3->D0) to avoid the 120ms more delay for
>> other PM events (system resume from suspend states like s2idle, S3 or S4
>> states) so that the system resume time can be improved.
>>
>> Please let me know if any inputs.
>
> I can only think of one minor side-effect that would be runtime
> suspending back
> too early after system resume. This could happen when connecting the
> first
> usb device to a roothub on a (system) suspended setup?
>
> steps:
> 1. in system suspend, no usb devices connected, xhci in D3, can signal
> wake with PME#
> 2. connect first usb device, xHC signals PME# wake
> 3. system resumes, xhci resumes to D0, but no actity visible on xHC
> port registers
> 4. rootubs resumes, no other children on this bus.
> 5. roothubs sees no activity (due to 120ms max latency before visible
> on port registers)
> 6. roothubs runtime suspend
> 7. xhci runtime suspends
> 8. same device now causes xHC to PME# wake again,
> 9. runtime reusume xhci, do wait 120ms for port activity
> 10. see port activity, resume hub, enumerate device.
>
> It might be that this really isn't an issue due to the the graceperiod
> fix:
>
> 33e321586e37 xhci: Add grace period after xHC start to prevent
> premature runtime suspend.

Yes correct above changes helps to prevent premature runtime suspend.

Thanks,
--
Basavaraj 

>
> Thanks
> -Mathias
>
>