| Message ID | 1464071269-79954-1-git-send-email-hehy1@lenovo.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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 -- 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
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.
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 -- 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
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. -- 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
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 -- 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
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.
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. -- 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
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.
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 -- 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
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.
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;
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(-)