[PATCH] PCI/ASPM: fix reverse ASPM L0s assignment of upstream and downstream

[PATCH] PCI/ASPM: fix reverse ASPM L0s assignment of upstream and downstream

[Ocean HY1 He]

In pcie_config_aspm_link(), when convert ASPM state to
upstream/downstream ASPM register state, the upstream variable and
dwsream variable are reversed. This causes PCI/E link enter ASPM L0s
even it should be disabled and PCI/E endpoint may reset randomly.

Signed-off-by: Ocean He <hehy1@lenovo.com>
---
 drivers/pci/pcie/aspm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 2dfe7fd..3f8a44d 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -439,9 +439,9 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
 		return;
 	/* Convert ASPM state to upstream/downstream ASPM register state */
 	if (state & ASPM_STATE_L0S_UP)
-		dwstream |= PCI_EXP_LNKCTL_ASPM_L0S;
-	if (state & ASPM_STATE_L0S_DW)
 		upstream |= PCI_EXP_LNKCTL_ASPM_L0S;
+	if (state & ASPM_STATE_L0S_DW)
+		dwstream |= PCI_EXP_LNKCTL_ASPM_L0S;
 	if (state & ASPM_STATE_L1) {
 		upstream |= PCI_EXP_LNKCTL_ASPM_L1;
 		dwstream |= PCI_EXP_LNKCTL_ASPM_L1;
-- 
1.8.3.1

Re: [PATCH] PCI/ASPM: fix reverse ASPM L0s assignment of upstream and downstream

[Bjorn Helgaas]

Hi Ocean,

On Tue, May 24, 2016 at 06:29:44AM +0000, Ocean HY1 He wrote:
> In pcie_config_aspm_link(), when convert ASPM state to
> upstream/downstream ASPM register state, the upstream variable and
> dwsream variable are reversed. This causes PCI/E link enter ASPM L0s
> even it should be disabled and PCI/E endpoint may reset randomly.

Random resets of an endpoint sounds like a pretty bad problem.  Do you
have a bug report?  We've had lots of issues with ASPM; I wonder if
this could account for some of them.

> Signed-off-by: Ocean He <hehy1@lenovo.com>
> ---
>  drivers/pci/pcie/aspm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 2dfe7fd..3f8a44d 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -439,9 +439,9 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
>  		return;
>  	/* Convert ASPM state to upstream/downstream ASPM register state */
>  	if (state & ASPM_STATE_L0S_UP)
> -		dwstream |= PCI_EXP_LNKCTL_ASPM_L0S;
> -	if (state & ASPM_STATE_L0S_DW)
>  		upstream |= PCI_EXP_LNKCTL_ASPM_L0S;
> +	if (state & ASPM_STATE_L0S_DW)
> +		dwstream |= PCI_EXP_LNKCTL_ASPM_L0S;
>  	if (state & ASPM_STATE_L1) {
>  		upstream |= PCI_EXP_LNKCTL_ASPM_L1;
>  		dwstream |= PCI_EXP_LNKCTL_ASPM_L1;
> -- 
> 1.8.3.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PCI/ASPM: fix reverse ASPM L0s assignment of upstream and downstream

[Sinan Kaya]

On 5/24/2016 7:53 AM, Bjorn Helgaas wrote:
> On Tue, May 24, 2016 at 06:29:44AM +0000, Ocean HY1 He wrote:
>> > In pcie_config_aspm_link(), when convert ASPM state to
>> > upstream/downstream ASPM register state, the upstream variable and
>> > dwsream variable are reversed. This causes PCI/E link enter ASPM L0s
>> > even it should be disabled and PCI/E endpoint may reset randomly.
> Random resets of an endpoint sounds like a pretty bad problem.  Do you
> have a bug report?  We've had lots of issues with ASPM; I wonder if
> this could account for some of them.
> 

I'm seeing this problem on Linux's ASPM code using powersave option where
each side of the link is having ASPM L0s enabled without coordination with
the other side. I'm wondering if you are hitting this too.

Legacy software (either operating system or firmware) that encounters 
the previously reserved value 00b (No ASPM Support), will most likely
refrain from enabling L1, which is intended behavior. 10 Legacy software
will also most likely refrain from enabling L0s for that component’s 
Transmitter (also intended behavior), but it is unclear if such software
will also refrain from enabling L0s for the component on the other side 
of the Link. 

If software enables L0s on one side when the component on 
the other side does not indicate that it supports L0s, the result is 
undefined. 

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH] PCI/ASPM: fix reverse ASPM L0s assignment of upstream and downstream

[Bjorn Helgaas]

On Tue, May 24, 2016 at 10:42:31AM -0400, Sinan Kaya wrote:
> On 5/24/2016 7:53 AM, Bjorn Helgaas wrote:
> > On Tue, May 24, 2016 at 06:29:44AM +0000, Ocean HY1 He wrote:
> >> > In pcie_config_aspm_link(), when convert ASPM state to
> >> > upstream/downstream ASPM register state, the upstream variable and
> >> > dwsream variable are reversed. This causes PCI/E link enter ASPM L0s
> >> > even it should be disabled and PCI/E endpoint may reset randomly.
> > Random resets of an endpoint sounds like a pretty bad problem.  Do you
> > have a bug report?  We've had lots of issues with ASPM; I wonder if
> > this could account for some of them.
> 
> I'm seeing this problem on Linux's ASPM code using powersave option where
> each side of the link is having ASPM L0s enabled without coordination with
> the other side. I'm wondering if you are hitting this too.

This would be really bad.  Can you fill in a few more details about
how this happens?

I thought you were talking about booting with
"pcie_aspm.policy=powersave", where pcie_aspm_set_policy() sets
aspm_policy = POLICY_POWERSAVE, then configures each link with
ASPM_STATE_ALL.  But pcie_config_aspm_link() does AND the desired
state (ASPM_STATE_ALL) with link->aspm_capable, which only has
ASPM_STATE_L0S set if both the upstream and downstream components
advertised PCIE_LINK_STATE_L0S.

This path is very complicated, but I don't *think* it will enable L0s
if the other end of the link doesn't support it.

> Legacy software (either operating system or firmware) that encounters 
> the previously reserved value 00b (No ASPM Support), will most likely
> refrain from enabling L1, which is intended behavior. 10 Legacy software
> will also most likely refrain from enabling L0s for that component’s 
> Transmitter (also intended behavior), but it is unclear if such software
> will also refrain from enabling L0s for the component on the other side 
> of the Link. 
> 
> If software enables L0s on one side when the component on 
> the other side does not indicate that it supports L0s, the result is 
> undefined. 

RE: [PATCH] PCI/ASPM: fix reverse ASPM L0s assignment of upstream and downstream

[Ocean HY1 He]

Hi Bjorn,

I file a bug today: https://bugzilla.kernel.org/show_bug.cgi?id=118941

My patch passed in my test machine, but it's sad that Lenovo PA team still find reset issue when do batch test even applied my patch.
So please ignore it at this time. I am sorry for it. But I will spend more time to find if the patch is still right but has no impact on above bug report.

The section "5.4.1.1.1. Entry into the L0s State" of PCIE spec says: "Software must not enable L0s in either direction on a given Link unless components on both sides of the Link each support L0s; otherwise, the result is undefined."
So I think the issue may be caused by different ASPM L0s setting in each side of the link. After disable both of them, the issue gone.

And I find both sides of the link always keep ASPM L1 the same. Why ASPM L0s is treated in different way even PCIE spec has concern?
Is it valuable and acceptable to let ASPM L0s always keep the same in both sides of a link?

Thanks!

Ocean He / 何海洋
SW Development Dept. 
Beijing Design Center
Enterprise Product Group
Mobile: 18911778926
E-mail: hehy1@lenovo.com
No.6 Chuang Ye Road, Haidian District, Beijing, China 100085


-----Original Message-----
From: Bjorn Helgaas [mailto:helgaas@kernel.org] 
Sent: Tuesday, May 24, 2016 7:54 PM
To: Ocean HY1 He
Cc: bhelgaas@google.com; wangyijing@huawei.com; luto@kernel.org; linux-pci@vger.kernel.org; linux-kernel@vger.kernel.org; prarit@redhat.com; jcm@redhat.com; Nagananda Chumbalkar
Subject: Re: [PATCH] PCI/ASPM: fix reverse ASPM L0s assignment of upstream and downstream

Hi Ocean,

On Tue, May 24, 2016 at 06:29:44AM +0000, Ocean HY1 He wrote:
> In pcie_config_aspm_link(), when convert ASPM state to
> upstream/downstream ASPM register state, the upstream variable and
> dwsream variable are reversed. This causes PCI/E link enter ASPM L0s
> even it should be disabled and PCI/E endpoint may reset randomly.

Random resets of an endpoint sounds like a pretty bad problem.  Do you
have a bug report?  We've had lots of issues with ASPM; I wonder if
this could account for some of them.

> Signed-off-by: Ocean He <hehy1@lenovo.com>
> ---
>  drivers/pci/pcie/aspm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 2dfe7fd..3f8a44d 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -439,9 +439,9 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
>  		return;
>  	/* Convert ASPM state to upstream/downstream ASPM register state */
>  	if (state & ASPM_STATE_L0S_UP)
> -		dwstream |= PCI_EXP_LNKCTL_ASPM_L0S;
> -	if (state & ASPM_STATE_L0S_DW)
>  		upstream |= PCI_EXP_LNKCTL_ASPM_L0S;
> +	if (state & ASPM_STATE_L0S_DW)
> +		dwstream |= PCI_EXP_LNKCTL_ASPM_L0S;
>  	if (state & ASPM_STATE_L1) {
>  		upstream |= PCI_EXP_LNKCTL_ASPM_L1;
>  		dwstream |= PCI_EXP_LNKCTL_ASPM_L1;
> -- 
> 1.8.3.1
> --
> To unsubscribe from this list: send the line "unsubscribe linux-pci" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] PCI/ASPM: fix reverse ASPM L0s assignment of upstream and downstream

[Bjorn Helgaas]

On Tue, May 24, 2016 at 06:29:44AM +0000, Ocean HY1 He wrote:
> In pcie_config_aspm_link(), when convert ASPM state to
> upstream/downstream ASPM register state, the upstream variable and
> dwsream variable are reversed. This causes PCI/E link enter ASPM L0s
> even it should be disabled and PCI/E endpoint may reset randomly.
> 
> Signed-off-by: Ocean He <hehy1@lenovo.com>
> ---
>  drivers/pci/pcie/aspm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 2dfe7fd..3f8a44d 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -439,9 +439,9 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
>  		return;
>  	/* Convert ASPM state to upstream/downstream ASPM register state */
>  	if (state & ASPM_STATE_L0S_UP)
> -		dwstream |= PCI_EXP_LNKCTL_ASPM_L0S;
> -	if (state & ASPM_STATE_L0S_DW)
>  		upstream |= PCI_EXP_LNKCTL_ASPM_L0S;
> +	if (state & ASPM_STATE_L0S_DW)
> +		dwstream |= PCI_EXP_LNKCTL_ASPM_L0S;
>  	if (state & ASPM_STATE_L1) {
>  		upstream |= PCI_EXP_LNKCTL_ASPM_L1;
>  		dwstream |= PCI_EXP_LNKCTL_ASPM_L1;

I think the current code is correct.  Here's my reasoning, please
check and see if you agree:

  #define ASPM_STATE_L0S_UP       (1)
  #define ASPM_STATE_L0S_DW       (2)
  #define ASPM_STATE_L0S          (ASPM_STATE_L0S_UP | ASPM_STATE_L0S_DW)

  pcie_aspm_cap_init
  {
    ...
    if (dwreg.support & upreg.support & PCIE_LINK_STATE_L0S)
      link->aspm_support |= ASPM_STATE_L0S;
    link->aspm_capable = link->aspm_support;

Now "aspm_capable" has ASPM_STATE_L0S set only if both ends support L0s.

  pcie_config_aspm_link(link, state)
  {
    ...
    state &= (link->aspm_capable & ...)

This clears ASPM_STATE_L0S in "state" unless both ends support L0s.

    if (state & ASPM_STATE_L0S_UP)
      dwstream |= PCI_EXP_LNKCTL_ASPM_L0S;

If the caller of pcie_config_aspm_link() requested ASPM_STATE_L0S_UP
*and* both ends support L0s, we set PCI_EXP_LNKCTL_ASPM_L0S in
"dwstream".

    pcie_config_aspm_dev(child, dwstream);

Now we enable the downstream component's transmitter to enter L0s.
Per PCIe spec r3.0, sec 7.8.7, the receiver, i.e., the upstream
component, must be capable of entering L0s even when its transmitter
is disabled from entering L0s.

The way I read this,

  - We can only enable L0s when both ends of the link support L0s, and
  - We only need to enable L0s on the transmitting end.

ASPM_STATE_L0S_UP refers to L0s in the upstream direction, so that
would mean enabling L0s in the downstream component's transmitter.

Bjorn

Re: [PATCH] PCI/ASPM: fix reverse ASPM L0s assignment of upstream and downstream

[Sinan Kaya]

Hi Bjorn,

OK. I see that we are dealing with two different questions.

> I thought you were talking about booting with
> "pcie_aspm.policy=powersave", where pcie_aspm_set_policy() sets
> aspm_policy = POLICY_POWERSAVE, then configures each link with
> ASPM_STATE_ALL.  But pcie_config_aspm_link() does AND the desired
> state (ASPM_STATE_ALL) with link->aspm_capable, which only has
> ASPM_STATE_L0S set if both the upstream and downstream components
> advertised PCIE_LINK_STATE_L0S.
> This path is very complicated, but I don't *think* it will enable L0s
> if the other end of the link doesn't support it.

Thanks for clarifying this. You are saying that if one side of the
link doesn't support L0s, Linux doesn't enable L0s on the other side.
This makes sense.

I think there is a confusion between what supported means vs. when
L0s can be enabled on which side of the link.

You clarified the supported case above that code will not enable
L0s if the other side doesn't support L0s.


> Now we enable the downstream component's transmitter to enter L0s.
> Per PCIe spec r3.0, sec 7.8.7, the receiver, i.e., the upstream
> component, must be capable of entering L0s even when its transmitter
> is disabled from entering L0s.

Let's assume that both sides actually support L0s but the link parameters
on one side is not compatible.

You are saying that it is OK to enable L0s on just one side of the
link as long as both sides support L0s. 

This part is a little bit misleading. I had HW people telling me
that both sides need to enable L0s at about the same time.

I'm actually seeing Linux enabling L0s on one side only as you
described and both supports L0s.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH] PCI/ASPM: fix reverse ASPM L0s assignment of upstream and downstream

[Bjorn Helgaas]

On Wed, May 25, 2016 at 01:21:12PM -0400, Sinan Kaya wrote:
> Hi Bjorn,
> 
> OK. I see that we are dealing with two different questions.
> 
> > I thought you were talking about booting with
> > "pcie_aspm.policy=powersave", where pcie_aspm_set_policy() sets
> > aspm_policy = POLICY_POWERSAVE, then configures each link with
> > ASPM_STATE_ALL.  But pcie_config_aspm_link() does AND the desired
> > state (ASPM_STATE_ALL) with link->aspm_capable, which only has
> > ASPM_STATE_L0S set if both the upstream and downstream components
> > advertised PCIE_LINK_STATE_L0S.
> > This path is very complicated, but I don't *think* it will enable L0s
> > if the other end of the link doesn't support it.
> 
> Thanks for clarifying this. You are saying that if one side of the
> link doesn't support L0s, Linux doesn't enable L0s on the other side.
> This makes sense.
> 
> I think there is a confusion between what supported means vs. when
> L0s can be enabled on which side of the link.
> 
> You clarified the supported case above that code will not enable
> L0s if the other side doesn't support L0s.
> 
> 
> > Now we enable the downstream component's transmitter to enter L0s.
> > Per PCIe spec r3.0, sec 7.8.7, the receiver, i.e., the upstream
> > component, must be capable of entering L0s even when its transmitter
> > is disabled from entering L0s.
> 
> Let's assume that both sides actually support L0s but the link parameters
> on one side is not compatible.
> 
> You are saying that it is OK to enable L0s on just one side of the
> link as long as both sides support L0s. 

I'm not sure what you mean by the link parameters not being
compatible, but I think it is legal to enable L0s on only one
direction.

> This part is a little bit misleading. I had HW people telling me
> that both sides need to enable L0s at about the same time.

I don't remember seeing anything like that in the spec.  Do they have
a pointer?  "At about the same time" is too hand-wavey to be useful to
software.

> I'm actually seeing Linux enabling L0s on one side only as you
> described and both supports L0s.

Re: [PATCH] PCI/ASPM: fix reverse ASPM L0s assignment of upstream and downstream

[Sinan Kaya]

On 5/25/2016 1:50 PM, Bjorn Helgaas wrote:
>> > You are saying that it is OK to enable L0s on just one side of the
>> > link as long as both sides support L0s. 
> I'm not sure what you mean by the link parameters not being
> compatible, but I think it is legal to enable L0s on only one
> direction.

I'm talking about L0s acceptable and entry latency times used to
determine when L0s can be enabled.

> 
>> > This part is a little bit misleading. I had HW people telling me
>> > that both sides need to enable L0s at about the same time.
> I don't remember seeing anything like that in the spec.  Do they have
> a pointer?  "At about the same time" is too hand-wavey to be useful to
> software.
> 

OK. Let me do some more push back. I wanted to understand the OS
behavior and its reasoning.

Your answers are sufficient.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project

Re: [PATCH] PCI/ASPM: fix reverse ASPM L0s assignment of upstream and downstream

[Bjorn Helgaas]

On Wed, May 25, 2016 at 02:19:01PM -0400, Sinan Kaya wrote:
> On 5/25/2016 1:50 PM, Bjorn Helgaas wrote:
> >> > You are saying that it is OK to enable L0s on just one side of the
> >> > link as long as both sides support L0s. 
> > I'm not sure what you mean by the link parameters not being
> > compatible, but I think it is legal to enable L0s on only one
> > direction.
> 
> I'm talking about L0s acceptable and entry latency times used to
> determine when L0s can be enabled.

Oh, I see.  My understanding (again, I'm not a hardware person or a
PCIe spec expert) is that the latency numbers are an internal device
issue, not a PCIe link issue.

>From a PCIe point of view, I think we *could* enable L0s even if the
device's latency requirements wouldn't be met.  The PCIe link itself
should work fine, but the device may have internal issues like FIFO
overflows.

Of course, we want the device to work correctly, so we *shouldn't*
enable L0s if it would cause us to exceed the device's latency
tolerance.

It looks like the code enforces this by clearing bits in
link->aspm_capable (effectively pretending L0s or L1 are unsupported)
if the latency is too high.

Bjorn

Re: [PATCH] PCI/ASPM: fix reverse ASPM L0s assignment of upstream and downstream

[Sinan Kaya]

On 5/25/2016 2:33 PM, Bjorn Helgaas wrote:
> It looks like the code enforces this by clearing bits in
> link->aspm_capable (effectively pretending L0s or L1 are unsupported)
> if the latency is too high.

Yes, this is what I was referring to. I think what Linux does is
the right thing.

-- 
Sinan Kaya
Qualcomm Technologies, Inc. on behalf of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project