[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

[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

[Ocean HY1 He]

SGkgQmpvcm4sDQoNCkkgZmlsZSBhIGJ1ZyB0b2RheTogaHR0cHM6Ly9idWd6aWxsYS5rZXJuZWwu
b3JnL3Nob3dfYnVnLmNnaT9pZD0xMTg5NDENCg0KTXkgcGF0Y2ggcGFzc2VkIGluIG15IHRlc3Qg
bWFjaGluZSwgYnV0IGl0J3Mgc2FkIHRoYXQgTGVub3ZvIFBBIHRlYW0gc3RpbGwgZmluZCByZXNl
dCBpc3N1ZSB3aGVuIGRvIGJhdGNoIHRlc3QgZXZlbiBhcHBsaWVkIG15IHBhdGNoLg0KU28gcGxl
YXNlIGlnbm9yZSBpdCBhdCB0aGlzIHRpbWUuIEkgYW0gc29ycnkgZm9yIGl0LiBCdXQgSSB3aWxs
IHNwZW5kIG1vcmUgdGltZSB0byBmaW5kIGlmIHRoZSBwYXRjaCBpcyBzdGlsbCByaWdodCBidXQg
aGFzIG5vIGltcGFjdCBvbiBhYm92ZSBidWcgcmVwb3J0Lg0KDQpUaGUgc2VjdGlvbiAiNS40LjEu
MS4xLiBFbnRyeSBpbnRvIHRoZSBMMHMgU3RhdGUiIG9mIFBDSUUgc3BlYyBzYXlzOiAiU29mdHdh
cmUgbXVzdCBub3QgZW5hYmxlIEwwcyBpbiBlaXRoZXIgZGlyZWN0aW9uIG9uIGEgZ2l2ZW4gTGlu
ayB1bmxlc3MgY29tcG9uZW50cyBvbiBib3RoIHNpZGVzIG9mIHRoZSBMaW5rIGVhY2ggc3VwcG9y
dCBMMHM7IG90aGVyd2lzZSwgdGhlIHJlc3VsdCBpcyB1bmRlZmluZWQuIg0KU28gSSB0aGluayB0
aGUgaXNzdWUgbWF5IGJlIGNhdXNlZCBieSBkaWZmZXJlbnQgQVNQTSBMMHMgc2V0dGluZyBpbiBl
YWNoIHNpZGUgb2YgdGhlIGxpbmsuIEFmdGVyIGRpc2FibGUgYm90aCBvZiB0aGVtLCB0aGUgaXNz
dWUgZ29uZS4NCg0KQW5kIEkgZmluZCBib3RoIHNpZGVzIG9mIHRoZSBsaW5rIGFsd2F5cyBrZWVw
IEFTUE0gTDEgdGhlIHNhbWUuIFdoeSBBU1BNIEwwcyBpcyB0cmVhdGVkIGluIGRpZmZlcmVudCB3
YXkgZXZlbiBQQ0lFIHNwZWMgaGFzIGNvbmNlcm4/DQpJcyBpdCB2YWx1YWJsZSBhbmQgYWNjZXB0
YWJsZSB0byBsZXQgQVNQTSBMMHMgYWx3YXlzIGtlZXAgdGhlIHNhbWUgaW4gYm90aCBzaWRlcyBv
ZiBhIGxpbms/DQoNClRoYW5rcyENCg0KT2NlYW4gSGUgLyC6zrqj0fMNClNXIERldmVsb3BtZW50
IERlcHQuIA0KQmVpamluZyBEZXNpZ24gQ2VudGVyDQpFbnRlcnByaXNlIFByb2R1Y3QgR3JvdXAN
Ck1vYmlsZTogMTg5MTE3Nzg5MjYNCkUtbWFpbDogaGVoeTFAbGVub3ZvLmNvbQ0KTm8uNiBDaHVh
bmcgWWUgUm9hZCwgSGFpZGlhbiBEaXN0cmljdCwgQmVpamluZywgQ2hpbmEgMTAwMDg1DQoNCg0K
LS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCkZyb206IEJqb3JuIEhlbGdhYXMgW21haWx0bzpo
ZWxnYWFzQGtlcm5lbC5vcmddIA0KU2VudDogVHVlc2RheSwgTWF5IDI0LCAyMDE2IDc6NTQgUE0N
ClRvOiBPY2VhbiBIWTEgSGUNCkNjOiBiaGVsZ2Fhc0Bnb29nbGUuY29tOyB3YW5neWlqaW5nQGh1
YXdlaS5jb207IGx1dG9Aa2VybmVsLm9yZzsgbGludXgtcGNpQHZnZXIua2VybmVsLm9yZzsgbGlu
dXgta2VybmVsQHZnZXIua2VybmVsLm9yZzsgcHJhcml0QHJlZGhhdC5jb207IGpjbUByZWRoYXQu
Y29tOyBOYWdhbmFuZGEgQ2h1bWJhbGthcg0KU3ViamVjdDogUmU6IFtQQVRDSF0gUENJL0FTUE06
IGZpeCByZXZlcnNlIEFTUE0gTDBzIGFzc2lnbm1lbnQgb2YgdXBzdHJlYW0gYW5kIGRvd25zdHJl
YW0NCg0KSGkgT2NlYW4sDQoNCk9uIFR1ZSwgTWF5IDI0LCAyMDE2IGF0IDA2OjI5OjQ0QU0gKzAw
MDAsIE9jZWFuIEhZMSBIZSB3cm90ZToNCj4gSW4gcGNpZV9jb25maWdfYXNwbV9saW5rKCksIHdo
ZW4gY29udmVydCBBU1BNIHN0YXRlIHRvDQo+IHVwc3RyZWFtL2Rvd25zdHJlYW0gQVNQTSByZWdp
c3RlciBzdGF0ZSwgdGhlIHVwc3RyZWFtIHZhcmlhYmxlIGFuZA0KPiBkd3NyZWFtIHZhcmlhYmxl
IGFyZSByZXZlcnNlZC4gVGhpcyBjYXVzZXMgUENJL0UgbGluayBlbnRlciBBU1BNIEwwcw0KPiBl
dmVuIGl0IHNob3VsZCBiZSBkaXNhYmxlZCBhbmQgUENJL0UgZW5kcG9pbnQgbWF5IHJlc2V0IHJh
bmRvbWx5Lg0KDQpSYW5kb20gcmVzZXRzIG9mIGFuIGVuZHBvaW50IHNvdW5kcyBsaWtlIGEgcHJl
dHR5IGJhZCBwcm9ibGVtLiAgRG8geW91DQpoYXZlIGEgYnVnIHJlcG9ydD8gIFdlJ3ZlIGhhZCBs
b3RzIG9mIGlzc3VlcyB3aXRoIEFTUE07IEkgd29uZGVyIGlmDQp0aGlzIGNvdWxkIGFjY291bnQg
Zm9yIHNvbWUgb2YgdGhlbS4NCg0KPiBTaWduZWQtb2ZmLWJ5OiBPY2VhbiBIZSA8aGVoeTFAbGVu
b3ZvLmNvbT4NCj4gLS0tDQo+ICBkcml2ZXJzL3BjaS9wY2llL2FzcG0uYyB8IDQgKystLQ0KPiAg
MSBmaWxlIGNoYW5nZWQsIDIgaW5zZXJ0aW9ucygrKSwgMiBkZWxldGlvbnMoLSkNCj4gDQo+IGRp
ZmYgLS1naXQgYS9kcml2ZXJzL3BjaS9wY2llL2FzcG0uYyBiL2RyaXZlcnMvcGNpL3BjaWUvYXNw
bS5jDQo+IGluZGV4IDJkZmU3ZmQuLjNmOGE0NGQgMTAwNjQ0DQo+IC0tLSBhL2RyaXZlcnMvcGNp
L3BjaWUvYXNwbS5jDQo+ICsrKyBiL2RyaXZlcnMvcGNpL3BjaWUvYXNwbS5jDQo+IEBAIC00Mzks
OSArNDM5LDkgQEAgc3RhdGljIHZvaWQgcGNpZV9jb25maWdfYXNwbV9saW5rKHN0cnVjdCBwY2ll
X2xpbmtfc3RhdGUgKmxpbmssIHUzMiBzdGF0ZSkNCj4gIAkJcmV0dXJuOw0KPiAgCS8qIENvbnZl
cnQgQVNQTSBzdGF0ZSB0byB1cHN0cmVhbS9kb3duc3RyZWFtIEFTUE0gcmVnaXN0ZXIgc3RhdGUg
Ki8NCj4gIAlpZiAoc3RhdGUgJiBBU1BNX1NUQVRFX0wwU19VUCkNCj4gLQkJZHdzdHJlYW0gfD0g
UENJX0VYUF9MTktDVExfQVNQTV9MMFM7DQo+IC0JaWYgKHN0YXRlICYgQVNQTV9TVEFURV9MMFNf
RFcpDQo+ICAJCXVwc3RyZWFtIHw9IFBDSV9FWFBfTE5LQ1RMX0FTUE1fTDBTOw0KPiArCWlmIChz
dGF0ZSAmIEFTUE1fU1RBVEVfTDBTX0RXKQ0KPiArCQlkd3N0cmVhbSB8PSBQQ0lfRVhQX0xOS0NU
TF9BU1BNX0wwUzsNCj4gIAlpZiAoc3RhdGUgJiBBU1BNX1NUQVRFX0wxKSB7DQo+ICAJCXVwc3Ry
ZWFtIHw9IFBDSV9FWFBfTE5LQ1RMX0FTUE1fTDE7DQo+ICAJCWR3c3RyZWFtIHw9IFBDSV9FWFBf
TE5LQ1RMX0FTUE1fTDE7DQo+IC0tIA0KPiAxLjguMy4xDQo+IC0tDQo+IFRvIHVuc3Vic2NyaWJl
IGZyb20gdGhpcyBsaXN0OiBzZW5kIHRoZSBsaW5lICJ1bnN1YnNjcmliZSBsaW51eC1wY2kiIGlu
DQo+IHRoZSBib2R5IG9mIGEgbWVzc2FnZSB0byBtYWpvcmRvbW9Admdlci5rZXJuZWwub3JnDQo+
IE1vcmUgbWFqb3Jkb21vIGluZm8gYXQgIGh0dHA6Ly92Z2VyLmtlcm5lbC5vcmcvbWFqb3Jkb21v
LWluZm8uaHRtbA0K

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

[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