All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xhci: fix runtime PM imbalance in USB2 resume
@ 2022-02-25  5:53 Henry Lin
  2022-02-25  6:23 ` Greg Kroah-Hartman
  2022-02-25  7:15 ` [PATCH v2] " Henry Lin
  0 siblings, 2 replies; 15+ messages in thread
From: Henry Lin @ 2022-02-25  5:53 UTC (permalink / raw)
  To: mathias.nyman; +Cc: henryl, Greg Kroah-Hartman, linux-usb, linux-kernel

USB2 resume starts with usb_hcd_start_port_resume() in port status
change handling for RESUME link state. usb_hcd_end_port_resume() call is
needed to keep runtime PM balance.

Signed-off-by: Henry Lin <henryl@nvidia.com>
---
 drivers/usb/host/xhci-hub.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index df3522dab31b..4a8b07b8ee01 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1090,6 +1090,8 @@ static void xhci_get_usb2_port_status(struct xhci_port *port, u32 *status,
 		if (link_state == XDEV_U0) {
 			bus_state->resume_done[portnum] = 0;
 			clear_bit(portnum, &bus_state->resuming_ports);
+			usb_hcd_end_port_resume(&port->rhub->hcd->self,
+						portnum);
 			if (bus_state->suspended_ports & (1 << portnum)) {
 				bus_state->suspended_ports &= ~(1 << portnum);
 				bus_state->port_c_suspend |= 1 << portnum;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] xhci: fix runtime PM imbalance in USB2 resume
  2022-02-25  5:53 [PATCH] xhci: fix runtime PM imbalance in USB2 resume Henry Lin
@ 2022-02-25  6:23 ` Greg Kroah-Hartman
  2022-02-25  6:40   ` Henry Lin
  2022-02-25  7:15 ` [PATCH v2] " Henry Lin
  1 sibling, 1 reply; 15+ messages in thread
From: Greg Kroah-Hartman @ 2022-02-25  6:23 UTC (permalink / raw)
  To: Henry Lin; +Cc: mathias.nyman, linux-usb, linux-kernel

On Fri, Feb 25, 2022 at 01:53:11PM +0800, Henry Lin wrote:
> USB2 resume starts with usb_hcd_start_port_resume() in port status
> change handling for RESUME link state. usb_hcd_end_port_resume() call is
> needed to keep runtime PM balance.
> 
> Signed-off-by: Henry Lin <henryl@nvidia.com>
> ---
>  drivers/usb/host/xhci-hub.c | 2 ++
>  1 file changed, 2 insertions(+)

What commit does this fix?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] xhci: fix runtime PM imbalance in USB2 resume
  2022-02-25  6:23 ` Greg Kroah-Hartman
@ 2022-02-25  6:40   ` Henry Lin
  2022-02-25  6:49     ` Greg Kroah-Hartman
  0 siblings, 1 reply; 15+ messages in thread
From: Henry Lin @ 2022-02-25  6:40 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: mathias.nyman, linux-usb, linux-kernel

It fixes side effect from below change. 

commit a231ec41e6f6433daf4c693f169f6c5cfda8cb9d
Author: Mathias Nyman <mathias.nyman@linux.intel.com>
Date:   Fri Dec 7 16:19:35 2018 +0200

    xhci: refactor U0 link state handling in get_port_status

    Move U0 link state handing to USB3 and USB2 specific functions

    Note that
    bus_state->resuming_ports:
    bus_state->resume_done[]:
    are only used for USB2, and don't need to cleared for USB3 ports

    No functional changes

    Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
    Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

--nvpublic
________________________________________
From: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Sent: Friday, February 25, 2022 2:23 PM
To: Henry Lin
Cc: mathias.nyman@intel.com; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH] xhci: fix runtime PM imbalance in USB2 resume

External email: Use caution opening links or attachments


On Fri, Feb 25, 2022 at 01:53:11PM +0800, Henry Lin wrote:
> USB2 resume starts with usb_hcd_start_port_resume() in port status
> change handling for RESUME link state. usb_hcd_end_port_resume() call is
> needed to keep runtime PM balance.
>
> Signed-off-by: Henry Lin <henryl@nvidia.com>
> ---
>  drivers/usb/host/xhci-hub.c | 2 ++
>  1 file changed, 2 insertions(+)

What commit does this fix?

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] xhci: fix runtime PM imbalance in USB2 resume
  2022-02-25  6:40   ` Henry Lin
@ 2022-02-25  6:49     ` Greg Kroah-Hartman
  2022-02-25  7:02       ` Henry Lin
  0 siblings, 1 reply; 15+ messages in thread
From: Greg Kroah-Hartman @ 2022-02-25  6:49 UTC (permalink / raw)
  To: Henry Lin; +Cc: mathias.nyman, linux-usb, linux-kernel

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top

On Fri, Feb 25, 2022 at 06:40:53AM +0000, Henry Lin wrote:
> It fixes side effect from below change. 
> 
> commit a231ec41e6f6433daf4c693f169f6c5cfda8cb9d
> Author: Mathias Nyman <mathias.nyman@linux.intel.com>
> Date:   Fri Dec 7 16:19:35 2018 +0200
> 
>     xhci: refactor U0 link state handling in get_port_status
> 
>     Move U0 link state handing to USB3 and USB2 specific functions
> 
>     Note that
>     bus_state->resuming_ports:
>     bus_state->resume_done[]:
>     are only used for USB2, and don't need to cleared for USB3 ports
> 
>     No functional changes
> 
>     Signed-off-by: Mathias Nyman <mathias.nyman@linux.intel.com>
>     Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Great, please add a "Fixes:" tag to the patch when resubmitting it.

And the "no functional changes" seems not to have been true, right?
Does this need to go into the stable kernels?

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] xhci: fix runtime PM imbalance in USB2 resume
  2022-02-25  6:49     ` Greg Kroah-Hartman
@ 2022-02-25  7:02       ` Henry Lin
  0 siblings, 0 replies; 15+ messages in thread
From: Henry Lin @ 2022-02-25  7:02 UTC (permalink / raw)
  To: Greg Kroah-Hartman; +Cc: mathias.nyman, linux-usb, linux-kernel

> Great, please add a "Fixes:" tag to the patch when resubmitting it.
I will resubmit soon.

> And the "no functional changes" seems not to have been true, right?
Yes.
> Does this need to go into the stable kernels?
Yes, this need to go into stable kernels. 

Thanks,
Henry

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v2] xhci: fix runtime PM imbalance in USB2 resume
  2022-02-25  5:53 [PATCH] xhci: fix runtime PM imbalance in USB2 resume Henry Lin
  2022-02-25  6:23 ` Greg Kroah-Hartman
@ 2022-02-25  7:15 ` Henry Lin
  2022-02-25  9:16   ` Greg KH
  2022-02-28 10:56   ` Mathias Nyman
  1 sibling, 2 replies; 15+ messages in thread
From: Henry Lin @ 2022-02-25  7:15 UTC (permalink / raw)
  To: gregkh; +Cc: Henry Lin, Mathias Nyman, linux-usb, linux-kernel

USB2 resume starts with usb_hcd_start_port_resume() in port status
change handling for RESUME link state. usb_hcd_end_port_resume() call is
needed to keep runtime PM balance.

Fixes: a231ec41e6f6 ("xhci: refactor U0 link state handling in get_port_status")
Signed-off-by: Henry Lin <henryl@nvidia.com>
---
 drivers/usb/host/xhci-hub.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index df3522dab31b..4a8b07b8ee01 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1090,6 +1090,8 @@ static void xhci_get_usb2_port_status(struct xhci_port *port, u32 *status,
 		if (link_state == XDEV_U0) {
 			bus_state->resume_done[portnum] = 0;
 			clear_bit(portnum, &bus_state->resuming_ports);
+			usb_hcd_end_port_resume(&port->rhub->hcd->self,
+						portnum);
 			if (bus_state->suspended_ports & (1 << portnum)) {
 				bus_state->suspended_ports &= ~(1 << portnum);
 				bus_state->port_c_suspend |= 1 << portnum;
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] xhci: fix runtime PM imbalance in USB2 resume
  2022-02-25  7:15 ` [PATCH v2] " Henry Lin
@ 2022-02-25  9:16   ` Greg KH
  2022-02-26 16:06     ` Henry Lin
  2022-02-28 10:56   ` Mathias Nyman
  1 sibling, 1 reply; 15+ messages in thread
From: Greg KH @ 2022-02-25  9:16 UTC (permalink / raw)
  To: Henry Lin; +Cc: Mathias Nyman, linux-usb, linux-kernel

On Fri, Feb 25, 2022 at 03:15:06PM +0800, Henry Lin wrote:
> USB2 resume starts with usb_hcd_start_port_resume() in port status
> change handling for RESUME link state. usb_hcd_end_port_resume() call is
> needed to keep runtime PM balance.
> 
> Fixes: a231ec41e6f6 ("xhci: refactor U0 link state handling in get_port_status")
> Signed-off-by: Henry Lin <henryl@nvidia.com>
> ---
>  drivers/usb/host/xhci-hub.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index df3522dab31b..4a8b07b8ee01 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -1090,6 +1090,8 @@ static void xhci_get_usb2_port_status(struct xhci_port *port, u32 *status,
>  		if (link_state == XDEV_U0) {
>  			bus_state->resume_done[portnum] = 0;
>  			clear_bit(portnum, &bus_state->resuming_ports);
> +			usb_hcd_end_port_resume(&port->rhub->hcd->self,
> +						portnum);
>  			if (bus_state->suspended_ports & (1 << portnum)) {
>  				bus_state->suspended_ports &= ~(1 << portnum);
>  				bus_state->port_c_suspend |= 1 << portnum;
> -- 
> 2.17.1
> 

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/SubmittingPatches 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

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] xhci: fix runtime PM imbalance in USB2 resume
  2022-02-25  9:16   ` Greg KH
@ 2022-02-26 16:06     ` Henry Lin
  2022-02-26 16:20       ` Henry Lin
  0 siblings, 1 reply; 15+ messages in thread
From: Henry Lin @ 2022-02-26 16:06 UTC (permalink / raw)
  To: Greg KH; +Cc: Mathias Nyman, linux-usb, linux-kernel

USB2 resume starts with usb_hcd_start_port_resume() in port status
change handling for RESUME link state. usb_hcd_end_port_resume() call is
needed to keep runtime PM balance.

Fixes: a231ec41e6f6 ("xhci: refactor U0 link state handling in get_port_status")
Signed-off-by: Henry Lin <henryl@nvidia.com>
---
V1 -> V2: Added Fixes tag in changelog

 drivers/usb/host/xhci-hub.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index df3522dab31b..4a8b07b8ee01 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1090,6 +1090,8 @@ static void xhci_get_usb2_port_status(struct xhci_port *port, u32 *status,
                 if (link_state == XDEV_U0) {
                         bus_state->resume_done[portnum] = 0;
                         clear_bit(portnum, &bus_state->resuming_ports);
+                       usb_hcd_end_port_resume(&port->rhub->hcd->self,
+                                               portnum);
                         if (bus_state->suspended_ports & (1 << portnum)) {
                                 bus_state->suspended_ports &= ~(1 << portnum);
                                 bus_state->port_c_suspend |= 1 << portnum;
--
2.17.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] xhci: fix runtime PM imbalance in USB2 resume
  2022-02-26 16:06     ` Henry Lin
@ 2022-02-26 16:20       ` Henry Lin
  0 siblings, 0 replies; 15+ messages in thread
From: Henry Lin @ 2022-02-26 16:20 UTC (permalink / raw)
  To: Greg KH; +Cc: Mathias Nyman, linux-usb, linux-kernel

> +                       usb_hcd_end_port_resume(&port->rhub->hcd->self,
> +                                               portnum);
Tab indent is broken in this version. Will resend this.

________________________________________
From: Henry Lin <henryl@nvidia.com>
Sent: Sunday, February 27, 2022 12:06 AM
To: Greg KH
Cc: Mathias Nyman; linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] xhci: fix runtime PM imbalance in USB2 resume

USB2 resume starts with usb_hcd_start_port_resume() in port status
change handling for RESUME link state. usb_hcd_end_port_resume() call is
needed to keep runtime PM balance.

Fixes: a231ec41e6f6 ("xhci: refactor U0 link state handling in get_port_status")
Signed-off-by: Henry Lin <henryl@nvidia.com>
---
V1 -> V2: Added Fixes tag in changelog

 drivers/usb/host/xhci-hub.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index df3522dab31b..4a8b07b8ee01 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1090,6 +1090,8 @@ static void xhci_get_usb2_port_status(struct xhci_port *port, u32 *status,
                 if (link_state == XDEV_U0) {
                         bus_state->resume_done[portnum] = 0;
                         clear_bit(portnum, &bus_state->resuming_ports);
+                       usb_hcd_end_port_resume(&port->rhub->hcd->self,
+                                               portnum);
                         if (bus_state->suspended_ports & (1 << portnum)) {
                                 bus_state->suspended_ports &= ~(1 << portnum);
                                 bus_state->port_c_suspend |= 1 << portnum;
--
2.17.1

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] xhci: fix runtime PM imbalance in USB2 resume
  2022-02-25  7:15 ` [PATCH v2] " Henry Lin
  2022-02-25  9:16   ` Greg KH
@ 2022-02-28 10:56   ` Mathias Nyman
  2022-03-01 10:28     ` Henry Lin
  1 sibling, 1 reply; 15+ messages in thread
From: Mathias Nyman @ 2022-02-28 10:56 UTC (permalink / raw)
  To: Henry Lin, gregkh; +Cc: Mathias Nyman, linux-usb, linux-kernel

On 25.2.2022 9.15, Henry Lin wrote:
> USB2 resume starts with usb_hcd_start_port_resume() in port status
> change handling for RESUME link state. usb_hcd_end_port_resume() call is
> needed to keep runtime PM balance.

For normal usb2 port resume the usb_hcd_end_port_resume() is called when resume
has been signaled for long enough in xhci_handle_usb2_port_link_resume().
 
This is also where driver directs the port to go from Resume state to U0.
Port can't do this without driver directing it.

If there's a failure during resume signaling (disconnect, reset, error) then
stale resume variables are detected in xhci_get_port_status() and 
usb_hcd_end_port_resume() is called.

I do now see a231ec41e6f6 ("xhci: refactor U0 link state handling in get_port_status") 
does change order of checking and clearing stale resume variables, but this should
only happen if the first port state we read is a fully enabled functional U0 state after
a failed resume.

Could you expand a bit how this was detected? 

> 
> Fixes: a231ec41e6f6 ("xhci: refactor U0 link state handling in get_port_status")
> Signed-off-by: Henry Lin <henryl@nvidia.com>
> ---
>  drivers/usb/host/xhci-hub.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index df3522dab31b..4a8b07b8ee01 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -1090,6 +1090,8 @@ static void xhci_get_usb2_port_status(struct xhci_port *port, u32 *status,
>  		if (link_state == XDEV_U0) {
>  			bus_state->resume_done[portnum] = 0;
>  			clear_bit(portnum, &bus_state->resuming_ports);
> +			usb_hcd_end_port_resume(&port->rhub->hcd->self,
> +						portnum);

This will call usb_hcd_end_port_resume() every time port is in normal enabled U0 state
even if resume was never signaled, or port suspended.

-Mathias

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] xhci: fix runtime PM imbalance in USB2 resume
  2022-02-28 10:56   ` Mathias Nyman
@ 2022-03-01 10:28     ` Henry Lin
  2022-03-01 15:49       ` Mathias Nyman
  0 siblings, 1 reply; 15+ messages in thread
From: Henry Lin @ 2022-03-01 10:28 UTC (permalink / raw)
  To: Mathias Nyman, gregkh; +Cc: Mathias Nyman, linux-usb, linux-kernel

>> USB2 resume starts with usb_hcd_start_port_resume() in port status
>> change handling for RESUME link state. usb_hcd_end_port_resume() call is
>> needed to keep runtime PM balance.

> For normal usb2 port resume the usb_hcd_end_port_resume() is called when resume
> has been signaled for long enough in xhci_handle_usb2_port_link_resume().
>
> This is also where driver directs the port to go from Resume state to U0.
> Port can't do this without driver directing it.
>
> If there's a failure during resume signaling (disconnect, reset, error) then
> stale resume variables are detected in xhci_get_port_status() and
> usb_hcd_end_port_resume() is called.

> I do now see a231ec41e6f6 ("xhci: refactor U0 link state handling in get_port_status")
> does change order of checking and clearing stale resume variables, but this should
> only happen if the first port state we read is a fully enabled functional U0 state after
> a failed resume.

> Could you expand a bit how this was detected?
We observed the racing issue when usb2 device-initiated resume occurs in system resume.
If usb2 host-initiated resume for system resume directs U0 before xhci_get_usb2_port_status()
see RESUME state, xhci_get_usb2_port_status() will not finish resume process in
xhci_handle_usb2_port_link_resume(). Its scenario is as follows:

1. System resume starts. All driver system resume callbacks get called in order. XHCI controller
    is resumed by xhci_resume().
2. USB2 root hub is resuming. hcd_bus_resume() is being executed.
3. Before xhci_bus_resume() is finished, XHCI driver receives a port status change event for 
    an USB2 port with RESUME link state in xhci_irq(). XHCI driver starts the process to resume
    HS port for device-initiated resume.
4. In xhci_bus_resume(), host-initiated resume (direct U0) is performed on the same port that is
    resuming in step 3 in below loop:

                if (bus_state->bus_suspended) {
                        spin_unlock_irqrestore(&xhci->lock, flags);
                        msleep(USB_RESUME_TIMEOUT);
                        spin_lock_irqsave(&xhci->lock, flags);
                }
                for_each_set_bit(port_index, &bus_state->bus_suspended,
                                 BITS_PER_LONG) {
                        /* Clear PLC to poll it later for U0 transition */
                        xhci_test_and_clear_bit(xhci, ports[port_index],
                                                PORT_PLC);
                        xhci_set_link_state(xhci, ports[port_index], XDEV_U0);
                }
5. Then, link state of the resuming port is observed as U0 in following
    xhci_get_usb2_port_status(). xhci_handle_usb2_port_link_resume() has
    no chance to get called on the resuming port.

Thanks,
Henry

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] xhci: fix runtime PM imbalance in USB2 resume
  2022-03-01 10:28     ` Henry Lin
@ 2022-03-01 15:49       ` Mathias Nyman
  2022-03-01 19:18         ` Henry Lin
  2022-03-02  6:51         ` [PATCH v3] " Henry Lin
  0 siblings, 2 replies; 15+ messages in thread
From: Mathias Nyman @ 2022-03-01 15:49 UTC (permalink / raw)
  To: Henry Lin, gregkh; +Cc: Mathias Nyman, linux-usb, linux-kernel

On 1.3.2022 12.28, Henry Lin wrote:
>>> USB2 resume starts with usb_hcd_start_port_resume() in port status
>>> change handling for RESUME link state. usb_hcd_end_port_resume() call is
>>> needed to keep runtime PM balance.
> 
>> For normal usb2 port resume the usb_hcd_end_port_resume() is called when resume
>> has been signaled for long enough in xhci_handle_usb2_port_link_resume().
>>
>> This is also where driver directs the port to go from Resume state to U0.
>> Port can't do this without driver directing it.
>>
>> If there's a failure during resume signaling (disconnect, reset, error) then
>> stale resume variables are detected in xhci_get_port_status() and
>> usb_hcd_end_port_resume() is called.
> 
>> I do now see a231ec41e6f6 ("xhci: refactor U0 link state handling in get_port_status")
>> does change order of checking and clearing stale resume variables, but this should
>> only happen if the first port state we read is a fully enabled functional U0 state after
>> a failed resume.
> 
>> Could you expand a bit how this was detected?
> We observed the racing issue when usb2 device-initiated resume occurs in system resume.
> If usb2 host-initiated resume for system resume directs U0 before xhci_get_usb2_port_status()
> see RESUME state, xhci_get_usb2_port_status() will not finish resume process in
> xhci_handle_usb2_port_link_resume(). Its scenario is as follows:
> 
> 1. System resume starts. All driver system resume callbacks get called in order. XHCI controller
>     is resumed by xhci_resume().
> 2. USB2 root hub is resuming. hcd_bus_resume() is being executed.
> 3. Before xhci_bus_resume() is finished, XHCI driver receives a port status change event for 
>     an USB2 port with RESUME link state in xhci_irq(). XHCI driver starts the process to resume
>     HS port for device-initiated resume.
> 4. In xhci_bus_resume(), host-initiated resume (direct U0) is performed on the same port that is
>     resuming in step 3 in below loop:
> 
>                 if (bus_state->bus_suspended) {
>                         spin_unlock_irqrestore(&xhci->lock, flags);
>                         msleep(USB_RESUME_TIMEOUT);
>                         spin_lock_irqsave(&xhci->lock, flags);
>                 }
>                 for_each_set_bit(port_index, &bus_state->bus_suspended,
>                                  BITS_PER_LONG) {
>                         /* Clear PLC to poll it later for U0 transition */
>                         xhci_test_and_clear_bit(xhci, ports[port_index],
>                                                 PORT_PLC);
>                         xhci_set_link_state(xhci, ports[port_index], XDEV_U0);
>                 }
> 5. Then, link state of the resuming port is observed as U0 in following
>     xhci_get_usb2_port_status(). xhci_handle_usb2_port_link_resume() has
>     no chance to get called on the resuming port.
> 

True, thanks for the explanation.
If there's a race between system resume and device-initiated resume, and port is resumed
in xhci_bus_resume() then yes I see how this could happen.

Maybe only call usb_hcd_end_port_resume() if xhci_irq() detected the device-initiated
resume. i.e. set a value to resume_done[portnum] and called usb_hcd_start_port_resume()
something like: 

@@ -1088,6 +1088,8 @@ static void xhci_get_usb2_port_status(struct xhci_port *port, u32 *status,
                if (link_state == XDEV_U2)
                        *status |= USB_PORT_STAT_L1;
                if (link_state == XDEV_U0) {
+                       if (bus_state->resume_done[portnum])
+                               usb_hcd_end_port_resume(&port->rhub->hcd->self, portnum);
                        bus_state->resume_done[portnum] = 0;
                        clear_bit(portnum, &bus_state->resuming_ports);

Also xhci_bus_resume() only resumes ports that were forcefully suspended to U3 in xhci_bus_suspend().
Could be worth checking why that device wasn't already in U3 when suspend reached xhci_bus_suspend().

Thanks
Mathias


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH v2] xhci: fix runtime PM imbalance in USB2 resume
  2022-03-01 15:49       ` Mathias Nyman
@ 2022-03-01 19:18         ` Henry Lin
  2022-03-02  6:51         ` [PATCH v3] " Henry Lin
  1 sibling, 0 replies; 15+ messages in thread
From: Henry Lin @ 2022-03-01 19:18 UTC (permalink / raw)
  To: Mathias Nyman, gregkh; +Cc: Mathias Nyman, linux-usb, linux-kernel

> Maybe only call usb_hcd_end_port_resume() if xhci_irq() detected the device-initiated
> resume. i.e. set a value to resume_done[portnum] and called usb_hcd_start_port_resume()
> something like:
>
> @@ -1088,6 +1088,8 @@ static void xhci_get_usb2_port_status(struct xhci_port *port, u32 *status,
>                 if (link_state == XDEV_U2)
>                         *status |= USB_PORT_STAT_L1;
>                 if (link_state == XDEV_U0) {
> +                       if (bus_state->resume_done[portnum])
> +                               usb_hcd_end_port_resume(&port->rhub->hcd->self, portnum);
>                         bus_state->resume_done[portnum] = 0;
>                         clear_bit(portnum, &bus_state->resuming_ports);
This looks good. I can submit a new version based on this.

> Also xhci_bus_resume() only resumes ports that were forcefully suspended to U3 in xhci_bus_suspend().
> Could be worth checking why that device wasn't already in U3 when suspend reached xhci_bus_suspend().
It happens when we trigger USB device-initiated resume to bring system out of system suspend state.
For example, we can connect an USB2 external hub on root port and put system into suspend state. The USB2
external hub (the port it connects to) is in U3 after xhci_bus_suspend(). Once we connect a USB device
to downstream port of the USB2 external hub, the hub will trigger device initiated resume to wake up the
system. During system resume, XHCI controller will report the USB2 external hub is in RESUME state.

Thanks,
Henry

^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH v3] xhci: fix runtime PM imbalance in USB2 resume
  2022-03-01 15:49       ` Mathias Nyman
  2022-03-01 19:18         ` Henry Lin
@ 2022-03-02  6:51         ` Henry Lin
  2022-03-02  9:16           ` Mathias Nyman
  1 sibling, 1 reply; 15+ messages in thread
From: Henry Lin @ 2022-03-02  6:51 UTC (permalink / raw)
  To: mathias.nyman
  Cc: Henry Lin, Mathias Nyman, Greg Kroah-Hartman, linux-usb, linux-kernel

A race between system resume and device-initiated resume may result in
runtime PM imbalance on USB2 root hub. If a device-initiated resume
starts and system resume xhci_bus_resume() directs U0 before hub driver
sees the resuming device in RESUME state, device-initiated resume will
not be finished in xhci_handle_usb2_port_link_resume(). In this case,
usb_hcd_end_port_resume() call is missing.

This changes calls usb_hcd_end_port_resume() if resuming device reaches
U0 to keep runtime PM balance.

Fixes: a231ec41e6f6 ("xhci: refactor U0 link state handling in get_port_status")
Signed-off-by: Henry Lin <henryl@nvidia.com>
---

Changes in v2:
- Add Fixes tag in change log

Changes in v3:
- Revise changelog
- Only call usb_hcd_end_port_resume() if xhci_irq() detected the device-initiated resume

[v2] https://lore.kernel.org/all/20220225071506.22012-1-henryl@nvidia.com/
[v1] https://lore.kernel.org/all/20220225055311.92447-1-henryl@nvidia.com/
 
 drivers/usb/host/xhci-hub.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index df3522dab31b..5e7a4dfc59d2 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1088,6 +1088,9 @@ static void xhci_get_usb2_port_status(struct xhci_port *port, u32 *status,
 		if (link_state == XDEV_U2)
 			*status |= USB_PORT_STAT_L1;
 		if (link_state == XDEV_U0) {
+			if (bus_state->resume_done[portnum])
+				usb_hcd_end_port_resume(&port->rhub->hcd->self,
+							portnum);
 			bus_state->resume_done[portnum] = 0;
 			clear_bit(portnum, &bus_state->resuming_ports);
 			if (bus_state->suspended_ports & (1 << portnum)) {
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH v3] xhci: fix runtime PM imbalance in USB2 resume
  2022-03-02  6:51         ` [PATCH v3] " Henry Lin
@ 2022-03-02  9:16           ` Mathias Nyman
  0 siblings, 0 replies; 15+ messages in thread
From: Mathias Nyman @ 2022-03-02  9:16 UTC (permalink / raw)
  To: Henry Lin; +Cc: Mathias Nyman, Greg Kroah-Hartman, linux-usb, linux-kernel

On 2.3.2022 8.51, Henry Lin wrote:
> A race between system resume and device-initiated resume may result in
> runtime PM imbalance on USB2 root hub. If a device-initiated resume
> starts and system resume xhci_bus_resume() directs U0 before hub driver
> sees the resuming device in RESUME state, device-initiated resume will
> not be finished in xhci_handle_usb2_port_link_resume(). In this case,
> usb_hcd_end_port_resume() call is missing.
> 
> This changes calls usb_hcd_end_port_resume() if resuming device reaches
> U0 to keep runtime PM balance.
> 
> Fixes: a231ec41e6f6 ("xhci: refactor U0 link state handling in get_port_status")
> Signed-off-by: Henry Lin <henryl@nvidia.com>
> ---

Thanks, adding to queue

-Mathias

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2022-03-02  9:14 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-25  5:53 [PATCH] xhci: fix runtime PM imbalance in USB2 resume Henry Lin
2022-02-25  6:23 ` Greg Kroah-Hartman
2022-02-25  6:40   ` Henry Lin
2022-02-25  6:49     ` Greg Kroah-Hartman
2022-02-25  7:02       ` Henry Lin
2022-02-25  7:15 ` [PATCH v2] " Henry Lin
2022-02-25  9:16   ` Greg KH
2022-02-26 16:06     ` Henry Lin
2022-02-26 16:20       ` Henry Lin
2022-02-28 10:56   ` Mathias Nyman
2022-03-01 10:28     ` Henry Lin
2022-03-01 15:49       ` Mathias Nyman
2022-03-01 19:18         ` Henry Lin
2022-03-02  6:51         ` [PATCH v3] " Henry Lin
2022-03-02  9:16           ` Mathias Nyman

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.