diff mbox series

[v3] usb: host: xhci: Avoid XHCI resume delay if SSUSB device is not present

Message ID 20230919224327.29974-1-quic_wcheng@quicinc.com (mailing list archive)
State Superseded
Headers show
Series [v3] usb: host: xhci: Avoid XHCI resume delay if SSUSB device is not present | expand

Commit Message

Wesley Cheng Sept. 19, 2023, 10:43 p.m. UTC 
There is a 120ms delay implemented for allowing the XHCI host controller to
detect a U3 wakeup pulse.  The intention is to wait for the device to retry
the wakeup event if the USB3 PORTSC doesn't reflect the RESUME link status
by the time it is checked.  As per the USB3 specification:

  tU3WakeupRetryDelay ("Table 7-12. LTSSM State Transition Timeouts")

This would allow the XHCI resume sequence to determine if the root hub
needs to be also resumed.  However, in case there is no device connected,
or if there is only a HSUSB device connected, this delay would still affect
the overall resume timing.

Since this delay is solely for detecting U3 wake events (USB3 specific)
then ignore this delay for the disconnected case and the HSUSB connected
only case.

Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
---
Depends-on:
https://lore.kernel.org/linux-usb/20230915143108.1532163-3-mathias.nyman@linux.intel.com/

 drivers/usb/host/xhci.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Comments

Greg KH Sept. 20, 2023, 7:24 a.m. UTC | #1 
On Tue, Sep 19, 2023 at 03:43:27PM -0700, Wesley Cheng wrote:
> There is a 120ms delay implemented for allowing the XHCI host controller to
> detect a U3 wakeup pulse.  The intention is to wait for the device to retry
> the wakeup event if the USB3 PORTSC doesn't reflect the RESUME link status
> by the time it is checked.  As per the USB3 specification:
> 
>   tU3WakeupRetryDelay ("Table 7-12. LTSSM State Transition Timeouts")
> 
> This would allow the XHCI resume sequence to determine if the root hub
> needs to be also resumed.  However, in case there is no device connected,
> or if there is only a HSUSB device connected, this delay would still affect
> the overall resume timing.
> 
> Since this delay is solely for detecting U3 wake events (USB3 specific)
> then ignore this delay for the disconnected case and the HSUSB connected
> only case.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
> Depends-on:
> https://lore.kernel.org/linux-usb/20230915143108.1532163-3-mathias.nyman@linux.intel.com/
> 
>  drivers/usb/host/xhci.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index e1b1b64a0723..1855cab1be56 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -805,6 +805,18 @@ static void xhci_disable_hub_port_wake(struct xhci_hcd *xhci,
>  	spin_unlock_irqrestore(&xhci->lock, flags);
>  }
>  
> +/*
> + * Utilize suspended_ports and bus_suspended to determine if USB3 device is
> + * connected.  The bus state bits are set by XHCI hub when root hub udev is
> + * suspended.  Used to determine if USB3 remote wakeup considerations need to
> + * be accounted for during XHCI resume.
> + */
> +static bool xhci_usb3_dev_connected(struct xhci_hcd *xhci)
> +{
> +	return !!xhci->usb3_rhub.bus_state.suspended_ports ||
> +		!!xhci->usb3_rhub.bus_state.bus_suspended;
> +}
> +
>  static bool xhci_pending_portevent(struct xhci_hcd *xhci)
>  {
>  	struct xhci_port	**ports;
> @@ -968,6 +980,7 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
>  	int			retval = 0;
>  	bool			comp_timer_running = false;
>  	bool			pending_portevent = false;
> +	bool			usb3_connected = false;
>  	bool			reinit_xhc = false;
>  
>  	if (!hcd->state)
> @@ -1116,9 +1129,14 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
>  		 * Resume roothubs only if there are pending events.
>  		 * USB 3 devices resend U3 LFPS wake after a 100ms delay if
>  		 * the first wake signalling failed, give it that chance.
> +		 * Avoid this check if there are no devices connected to
> +		 * the SS root hub. (i.e. HS device connected or no device
> +		 * connected)
>  		 */
>  		pending_portevent = xhci_pending_portevent(xhci);
> -		if (!pending_portevent && msg.event == PM_EVENT_AUTO_RESUME) {
> +		usb3_connected = xhci_usb3_dev_connected(xhci);
> +		if (!pending_portevent && usb3_connected &&
> +		     msg.event == PM_EVENT_AUTO_RESUME) {
>  			msleep(120);
>  			pending_portevent = xhci_pending_portevent(xhci);
>  		}
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- This looks like a new version of a previously submitted patch, but you
  did not list below the --- line any changes from the previous version.
  Please read the section entitled "The canonical patch format" in the
  kernel file, Documentation/process/submitting-patches.rst for what
  needs to be done here to properly describe this.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot
Wesley Cheng Oct. 10, 2023, 7:01 p.m. UTC | #2 
Friendly ping to see if there are any updates/feedback on this patch?

Thanks
Wesley Cheng

On 9/19/2023 3:43 PM, Wesley Cheng wrote:
> There is a 120ms delay implemented for allowing the XHCI host controller to
> detect a U3 wakeup pulse.  The intention is to wait for the device to retry
> the wakeup event if the USB3 PORTSC doesn't reflect the RESUME link status
> by the time it is checked.  As per the USB3 specification:
> 
>    tU3WakeupRetryDelay ("Table 7-12. LTSSM State Transition Timeouts")
> 
> This would allow the XHCI resume sequence to determine if the root hub
> needs to be also resumed.  However, in case there is no device connected,
> or if there is only a HSUSB device connected, this delay would still affect
> the overall resume timing.
> 
> Since this delay is solely for detecting U3 wake events (USB3 specific)
> then ignore this delay for the disconnected case and the HSUSB connected
> only case.
> 
> Signed-off-by: Wesley Cheng <quic_wcheng@quicinc.com>
> ---
> Depends-on:
> https://lore.kernel.org/linux-usb/20230915143108.1532163-3-mathias.nyman@linux.intel.com/
> 
>   drivers/usb/host/xhci.c | 20 +++++++++++++++++++-
>   1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index e1b1b64a0723..1855cab1be56 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -805,6 +805,18 @@ static void xhci_disable_hub_port_wake(struct xhci_hcd *xhci,
>   	spin_unlock_irqrestore(&xhci->lock, flags);
>   }
>   
> +/*
> + * Utilize suspended_ports and bus_suspended to determine if USB3 device is
> + * connected.  The bus state bits are set by XHCI hub when root hub udev is
> + * suspended.  Used to determine if USB3 remote wakeup considerations need to
> + * be accounted for during XHCI resume.
> + */
> +static bool xhci_usb3_dev_connected(struct xhci_hcd *xhci)
> +{
> +	return !!xhci->usb3_rhub.bus_state.suspended_ports ||
> +		!!xhci->usb3_rhub.bus_state.bus_suspended;
> +}
> +
>   static bool xhci_pending_portevent(struct xhci_hcd *xhci)
>   {
>   	struct xhci_port	**ports;
> @@ -968,6 +980,7 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
>   	int			retval = 0;
>   	bool			comp_timer_running = false;
>   	bool			pending_portevent = false;
> +	bool			usb3_connected = false;
>   	bool			reinit_xhc = false;
>   
>   	if (!hcd->state)
> @@ -1116,9 +1129,14 @@ int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
>   		 * Resume roothubs only if there are pending events.
>   		 * USB 3 devices resend U3 LFPS wake after a 100ms delay if
>   		 * the first wake signalling failed, give it that chance.
> +		 * Avoid this check if there are no devices connected to
> +		 * the SS root hub. (i.e. HS device connected or no device
> +		 * connected)
>   		 */
>   		pending_portevent = xhci_pending_portevent(xhci);
> -		if (!pending_portevent && msg.event == PM_EVENT_AUTO_RESUME) {
> +		usb3_connected = xhci_usb3_dev_connected(xhci);
> +		if (!pending_portevent && usb3_connected &&
> +		     msg.event == PM_EVENT_AUTO_RESUME) {
>   			msleep(120);
>   			pending_portevent = xhci_pending_portevent(xhci);
>   		}
Greg KH Oct. 11, 2023, 9:24 a.m. UTC | #3 
On Tue, Oct 10, 2023 at 12:01:05PM -0700, Wesley Cheng wrote:
> Friendly ping to see if there are any updates/feedback on this patch?

Please do not top-post.

Anyway, did you not see my bot's response to this patch?  If so, why did
you ignore it, that's why we didn't do anything with it...

thanks,

greg k-h
Wesley Cheng Oct. 11, 2023, 9:58 p.m. UTC | #4 
Hi Greg,

On 10/11/2023 2:24 AM, Greg KH wrote:
> On Tue, Oct 10, 2023 at 12:01:05PM -0700, Wesley Cheng wrote:
>> Friendly ping to see if there are any updates/feedback on this patch?
> 
> Please do not top-post.
> 
> Anyway, did you not see my bot's response to this patch?  If so, why did
> you ignore it, that's why we didn't do anything with it...
> 

Got it, I was mainly just checking to see if the new way of determining 
if there is a SSUSB device connected is the way we want to implement it, 
since I changed to using the ports_suspended bitmask.  I'll send a new 
revision with the changelist.

Thanks
Wesley Cheng
diff mbox series

Patch

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index e1b1b64a0723..1855cab1be56 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -805,6 +805,18 @@  static void xhci_disable_hub_port_wake(struct xhci_hcd *xhci,
 	spin_unlock_irqrestore(&xhci->lock, flags);
 }
 
+/*
+ * Utilize suspended_ports and bus_suspended to determine if USB3 device is
+ * connected.  The bus state bits are set by XHCI hub when root hub udev is
+ * suspended.  Used to determine if USB3 remote wakeup considerations need to
+ * be accounted for during XHCI resume.
+ */
+static bool xhci_usb3_dev_connected(struct xhci_hcd *xhci)
+{
+	return !!xhci->usb3_rhub.bus_state.suspended_ports ||
+		!!xhci->usb3_rhub.bus_state.bus_suspended;
+}
+
 static bool xhci_pending_portevent(struct xhci_hcd *xhci)
 {
 	struct xhci_port	**ports;
@@ -968,6 +980,7 @@  int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
 	int			retval = 0;
 	bool			comp_timer_running = false;
 	bool			pending_portevent = false;
+	bool			usb3_connected = false;
 	bool			reinit_xhc = false;
 
 	if (!hcd->state)
@@ -1116,9 +1129,14 @@  int xhci_resume(struct xhci_hcd *xhci, pm_message_t msg)
 		 * Resume roothubs only if there are pending events.
 		 * USB 3 devices resend U3 LFPS wake after a 100ms delay if
 		 * the first wake signalling failed, give it that chance.
+		 * Avoid this check if there are no devices connected to
+		 * the SS root hub. (i.e. HS device connected or no device
+		 * connected)
 		 */
 		pending_portevent = xhci_pending_portevent(xhci);
-		if (!pending_portevent && msg.event == PM_EVENT_AUTO_RESUME) {
+		usb3_connected = xhci_usb3_dev_connected(xhci);
+		if (!pending_portevent && usb3_connected &&
+		     msg.event == PM_EVENT_AUTO_RESUME) {
 			msleep(120);
 			pending_portevent = xhci_pending_portevent(xhci);
 		}