All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Patel, Mayurkumar" <mayurkumar.patel@intel.com>
To: Bjorn Helgaas <bhelgaas@google.com>
Cc: Sinan Kaya <okaya@codeaurora.org>,
	Bjorn Helgaas <helgaas@kernel.org>,
	Rajat Jain <rajatja@google.com>,
	Rajat Jain <rajatxjain@gmail.com>,
	David Daney <david.daney@cavium.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	Timur Tabi <timur@codeaurora.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Julia Lawall <Julia.Lawall@lip6.fr>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Yinghai Lu <yinghai@kernel.org>,
	Shawn Lin <shawn.lin@rock-chips.com>,
	linux-arm <linux-arm-kernel@lists.infradead.org>,
	Myron Stowe <myron.stowe@redhat.com>
Subject: RE: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init
Date: Tue, 2 May 2017 12:02:53 +0000	[thread overview]
Message-ID: <92EBB4272BF81E4089A7126EC1E7B284667759B1@IRSMSX101.ger.corp.intel.com> (raw)
In-Reply-To: <CAErSpo7TO1rfG6MDYuohyXL4WbaGCJdV8y3Ke-uL6VjGRb1hvw@mail.gmail.com>

Hi Bjorn

>
>On Fri, Apr 21, 2017 at 2:46 AM, Patel, Mayurkumar
><mayurkumar.patel@intel.com> wrote:
>> Hi Bjorn/Kaya,
>>
>>
>>>
>>>On 4/17/2017 12:38 PM, Bjorn Helgaas wrote:
>>>>> Like you said, what do we do by default is the question. Should we opt
>>>>> for safe like we are doing, or try to save some power.
>>>> I think safety is paramount.  Every user should be able to boot safely
>>>> without any kernel parameters.  We don't want users to have a problem
>>>> booting and then have to search for a workaround like booting with
>>>> "pcie_aspm=off".  Most users will never do that.
>>>>
>>>
>>>OK, no problem with leaving the behavior as it is.
>>>
>>>My initial approach was #2. We knew this way that user had full control
>>>over the ASPM policy by changing the BIOS option. Then, Mayurkumar
>>>complained that ASPM is not enabled following a hotplug insertion to an
>>>empty slot. That's when I switched to #3 as it sounded like a good thing
>>>to have for us.
>>>
>>>> Here's a long-term strawman proposal, see what you think:
>>>>
>>>>   - Deprecate CONFIG_PCIEASPM_DEFAULT, CONFIG_PCIEASPM_POWERSAVE, etc.
>>>>   - Default aspm_policy is POLICY_DEFAULT always.
>>>>   - POLICY_DEFAULT means Linux doesn't touch anything: if BIOS enabled
>>>> ASPM, we leave it that way; we leave ASPM disabled on hot-added
>>>> devices.
>>>
>> I am also ok with leaving the same behavior as now.
>> But still following is something open I feel besides, Which may be there in your comments redundantly.
>> The current problem is, pcie_aspm_exit_link_state() disables the ASPM configuration even
>> if POLICY_DEFAULT was set.
>
>We call pcie_aspm_exit_link_state() when removing an endpoint.  When
>we remove an endpoint, I think disabling ASPM is the right thing to
>do.  The spec (PCIe r3.1, sec 5.4.1.3) 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."
>

Yes, you are right and per spec also it makes sense that ASPM needs to be disabled.
But, if POLICY_DEFAULT is set then, shouldn't BIOS take care of disabling ASPM?



>> I am seeing already following problem(or may be influence) with it. The Endpoint I have does not have
>> does not have "Presence detect change" mechanism. Hot plug is working with Link status events.
>> When link is in L1 or L1SS and if EP is powered off, no Link status change event are triggered (It might be
>> the expected behavior in L1 or L1SS).  When next time EP is powered on there are link down and
>> link up events coming one after other. BIOS enables ASPM on Root port and Endpoint, but while
>> processing link status down, pcie_aspm_exit_link_state() clears the ASPM already which were enabled by BIOS.
>> If we want to follow above approach then shall we consider having something similar as following?
>
>The proposal was to leave ASPM disabled on hot-added devices.  If the
>endpoint was powered off and powered back on again, I think that
>device looks like a hot-added device, doesn't it?
>

Yes, it is hot-added device. Also, I understand, for POLICY_DEFAULT, OS would/should not touch ASPM(enable/disable),
but BIOS could still (enable/disable), right?

Currently, what happens in my system is as following, (each 2nd power cycle/hotplug of Endpoint disables ASPM):


First Power cycle (When ASPM L1 is already enabled):
device gets powered off -> there are no Link status events, so no pcie hotplug interrupt and pcie_aspm_exit_link_state() triggered.
When the device gets powered on again -> Link down/Link up events are coming back to back. 
First Link down is served. (BIOS checks for the Link status and enables ASPM already, as the device is
actually powered back). OS calls pcie_aspm_exit_link_state() and ASPM gets disabled by OS.

Second Power cycle (When ASPM L1 is disabled after above):
device gets powered off -> there are link status events, pcie hotplug interrupt is triggered and pcie_aspm_exit_link_state() triggered.
OS disables ASPM. BIOS checks Link status and disables ASPM too.
When the device gets powered on -> BIOS enables ASPM and as this is pcie hotplug insertion, OS
does not interfere and we have ASPM enabled.

The above sequence happens each 2nd power cycle of the hotplug device.

So One could still argue if POLICY_DEFAULT is set, then why OS disables ASPM if it is not meant to touch configuration.
This is why I proposed following kind of change, so that OS would not touch ASPM, if POLICY_DEFAULT is set.
Also, With the below change, everything relies on BIOS for ASPM when POLICY_DEFAULT is set and I see above problem
gets resolved. Also, the existing ASPM behavior does not have impact, unless specific BIOS does not disable ASPM on
Root Port when device gets removed.



>Bjorn
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

WARNING: multiple messages have this Message-ID (diff)
From: "Patel, Mayurkumar" <mayurkumar.patel@intel.com>
To: Bjorn Helgaas <bhelgaas@google.com>
Cc: Sinan Kaya <okaya@codeaurora.org>,
	Bjorn Helgaas <helgaas@kernel.org>,
	Rajat Jain <rajatja@google.com>,
	Rajat Jain <rajatxjain@gmail.com>,
	"David Daney" <david.daney@cavium.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	Timur Tabi <timur@codeaurora.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Julia Lawall <Julia.Lawall@lip6.fr>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Yinghai Lu <yinghai@kernel.org>,
	Shawn Lin <shawn.lin@rock-chips.com>,
	linux-arm <linux-arm-kernel@lists.infradead.org>,
	Myron Stowe <myron.stowe@redhat.com>
Subject: RE: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init
Date: Tue, 2 May 2017 12:02:53 +0000	[thread overview]
Message-ID: <92EBB4272BF81E4089A7126EC1E7B284667759B1@IRSMSX101.ger.corp.intel.com> (raw)
In-Reply-To: <CAErSpo7TO1rfG6MDYuohyXL4WbaGCJdV8y3Ke-uL6VjGRb1hvw@mail.gmail.com>

SGkgQmpvcm4NCg0KPg0KPk9uIEZyaSwgQXByIDIxLCAyMDE3IGF0IDI6NDYgQU0sIFBhdGVsLCBN
YXl1cmt1bWFyDQo+PG1heXVya3VtYXIucGF0ZWxAaW50ZWwuY29tPiB3cm90ZToNCj4+IEhpIEJq
b3JuL0theWEsDQo+Pg0KPj4NCj4+Pg0KPj4+T24gNC8xNy8yMDE3IDEyOjM4IFBNLCBCam9ybiBI
ZWxnYWFzIHdyb3RlOg0KPj4+Pj4gTGlrZSB5b3Ugc2FpZCwgd2hhdCBkbyB3ZSBkbyBieSBkZWZh
dWx0IGlzIHRoZSBxdWVzdGlvbi4gU2hvdWxkIHdlIG9wdA0KPj4+Pj4gZm9yIHNhZmUgbGlrZSB3
ZSBhcmUgZG9pbmcsIG9yIHRyeSB0byBzYXZlIHNvbWUgcG93ZXIuDQo+Pj4+IEkgdGhpbmsgc2Fm
ZXR5IGlzIHBhcmFtb3VudC4gIEV2ZXJ5IHVzZXIgc2hvdWxkIGJlIGFibGUgdG8gYm9vdCBzYWZl
bHkNCj4+Pj4gd2l0aG91dCBhbnkga2VybmVsIHBhcmFtZXRlcnMuICBXZSBkb24ndCB3YW50IHVz
ZXJzIHRvIGhhdmUgYSBwcm9ibGVtDQo+Pj4+IGJvb3RpbmcgYW5kIHRoZW4gaGF2ZSB0byBzZWFy
Y2ggZm9yIGEgd29ya2Fyb3VuZCBsaWtlIGJvb3Rpbmcgd2l0aA0KPj4+PiAicGNpZV9hc3BtPW9m
ZiIuICBNb3N0IHVzZXJzIHdpbGwgbmV2ZXIgZG8gdGhhdC4NCj4+Pj4NCj4+Pg0KPj4+T0ssIG5v
IHByb2JsZW0gd2l0aCBsZWF2aW5nIHRoZSBiZWhhdmlvciBhcyBpdCBpcy4NCj4+Pg0KPj4+TXkg
aW5pdGlhbCBhcHByb2FjaCB3YXMgIzIuIFdlIGtuZXcgdGhpcyB3YXkgdGhhdCB1c2VyIGhhZCBm
dWxsIGNvbnRyb2wNCj4+Pm92ZXIgdGhlIEFTUE0gcG9saWN5IGJ5IGNoYW5naW5nIHRoZSBCSU9T
IG9wdGlvbi4gVGhlbiwgTWF5dXJrdW1hcg0KPj4+Y29tcGxhaW5lZCB0aGF0IEFTUE0gaXMgbm90
IGVuYWJsZWQgZm9sbG93aW5nIGEgaG90cGx1ZyBpbnNlcnRpb24gdG8gYW4NCj4+PmVtcHR5IHNs
b3QuIFRoYXQncyB3aGVuIEkgc3dpdGNoZWQgdG8gIzMgYXMgaXQgc291bmRlZCBsaWtlIGEgZ29v
ZCB0aGluZw0KPj4+dG8gaGF2ZSBmb3IgdXMuDQo+Pj4NCj4+Pj4gSGVyZSdzIGEgbG9uZy10ZXJt
IHN0cmF3bWFuIHByb3Bvc2FsLCBzZWUgd2hhdCB5b3UgdGhpbms6DQo+Pj4+DQo+Pj4+ICAgLSBE
ZXByZWNhdGUgQ09ORklHX1BDSUVBU1BNX0RFRkFVTFQsIENPTkZJR19QQ0lFQVNQTV9QT1dFUlNB
VkUsIGV0Yy4NCj4+Pj4gICAtIERlZmF1bHQgYXNwbV9wb2xpY3kgaXMgUE9MSUNZX0RFRkFVTFQg
YWx3YXlzLg0KPj4+PiAgIC0gUE9MSUNZX0RFRkFVTFQgbWVhbnMgTGludXggZG9lc24ndCB0b3Vj
aCBhbnl0aGluZzogaWYgQklPUyBlbmFibGVkDQo+Pj4+IEFTUE0sIHdlIGxlYXZlIGl0IHRoYXQg
d2F5OyB3ZSBsZWF2ZSBBU1BNIGRpc2FibGVkIG9uIGhvdC1hZGRlZA0KPj4+PiBkZXZpY2VzLg0K
Pj4+DQo+PiBJIGFtIGFsc28gb2sgd2l0aCBsZWF2aW5nIHRoZSBzYW1lIGJlaGF2aW9yIGFzIG5v
dy4NCj4+IEJ1dCBzdGlsbCBmb2xsb3dpbmcgaXMgc29tZXRoaW5nIG9wZW4gSSBmZWVsIGJlc2lk
ZXMsIFdoaWNoIG1heSBiZSB0aGVyZSBpbiB5b3VyIGNvbW1lbnRzIHJlZHVuZGFudGx5Lg0KPj4g
VGhlIGN1cnJlbnQgcHJvYmxlbSBpcywgcGNpZV9hc3BtX2V4aXRfbGlua19zdGF0ZSgpIGRpc2Fi
bGVzIHRoZSBBU1BNIGNvbmZpZ3VyYXRpb24gZXZlbg0KPj4gaWYgUE9MSUNZX0RFRkFVTFQgd2Fz
IHNldC4NCj4NCj5XZSBjYWxsIHBjaWVfYXNwbV9leGl0X2xpbmtfc3RhdGUoKSB3aGVuIHJlbW92
aW5nIGFuIGVuZHBvaW50LiAgV2hlbg0KPndlIHJlbW92ZSBhbiBlbmRwb2ludCwgSSB0aGluayBk
aXNhYmxpbmcgQVNQTSBpcyB0aGUgcmlnaHQgdGhpbmcgdG8NCj5kby4gIFRoZSBzcGVjIChQQ0ll
IHIzLjEsIHNlYyA1LjQuMS4zKSBzYXlzICJTb2Z0d2FyZSBtdXN0IG5vdCBlbmFibGUNCj5MMHMg
aW4gZWl0aGVyIGRpcmVjdGlvbiBvbiBhIGdpdmVuIExpbmsgdW5sZXNzIGNvbXBvbmVudHMgb24g
Ym90aA0KPnNpZGVzIG9mIHRoZSBMaW5rIGVhY2ggc3VwcG9ydCBMMHM7IG90aGVyd2lzZSwgdGhl
IHJlc3VsdCBpcw0KPnVuZGVmaW5lZC4iDQo+DQoNClllcywgeW91IGFyZSByaWdodCBhbmQgcGVy
IHNwZWMgYWxzbyBpdCBtYWtlcyBzZW5zZSB0aGF0IEFTUE0gbmVlZHMgdG8gYmUgZGlzYWJsZWQu
DQpCdXQsIGlmIFBPTElDWV9ERUZBVUxUIGlzIHNldCB0aGVuLCBzaG91bGRuJ3QgQklPUyB0YWtl
IGNhcmUgb2YgZGlzYWJsaW5nIEFTUE0/DQoNCg0KDQo+PiBJIGFtIHNlZWluZyBhbHJlYWR5IGZv
bGxvd2luZyBwcm9ibGVtKG9yIG1heSBiZSBpbmZsdWVuY2UpIHdpdGggaXQuIFRoZSBFbmRwb2lu
dCBJIGhhdmUgZG9lcyBub3QgaGF2ZQ0KPj4gZG9lcyBub3QgaGF2ZSAiUHJlc2VuY2UgZGV0ZWN0
IGNoYW5nZSIgbWVjaGFuaXNtLiBIb3QgcGx1ZyBpcyB3b3JraW5nIHdpdGggTGluayBzdGF0dXMg
ZXZlbnRzLg0KPj4gV2hlbiBsaW5rIGlzIGluIEwxIG9yIEwxU1MgYW5kIGlmIEVQIGlzIHBvd2Vy
ZWQgb2ZmLCBubyBMaW5rIHN0YXR1cyBjaGFuZ2UgZXZlbnQgYXJlIHRyaWdnZXJlZCAoSXQgbWln
aHQgYmUNCj4+IHRoZSBleHBlY3RlZCBiZWhhdmlvciBpbiBMMSBvciBMMVNTKS4gIFdoZW4gbmV4
dCB0aW1lIEVQIGlzIHBvd2VyZWQgb24gdGhlcmUgYXJlIGxpbmsgZG93biBhbmQNCj4+IGxpbmsg
dXAgZXZlbnRzIGNvbWluZyBvbmUgYWZ0ZXIgb3RoZXIuIEJJT1MgZW5hYmxlcyBBU1BNIG9uIFJv
b3QgcG9ydCBhbmQgRW5kcG9pbnQsIGJ1dCB3aGlsZQ0KPj4gcHJvY2Vzc2luZyBsaW5rIHN0YXR1
cyBkb3duLCBwY2llX2FzcG1fZXhpdF9saW5rX3N0YXRlKCkgY2xlYXJzIHRoZSBBU1BNIGFscmVh
ZHkgd2hpY2ggd2VyZSBlbmFibGVkIGJ5IEJJT1MuDQo+PiBJZiB3ZSB3YW50IHRvIGZvbGxvdyBh
Ym92ZSBhcHByb2FjaCB0aGVuIHNoYWxsIHdlIGNvbnNpZGVyIGhhdmluZyBzb21ldGhpbmcgc2lt
aWxhciBhcyBmb2xsb3dpbmc/DQo+DQo+VGhlIHByb3Bvc2FsIHdhcyB0byBsZWF2ZSBBU1BNIGRp
c2FibGVkIG9uIGhvdC1hZGRlZCBkZXZpY2VzLiAgSWYgdGhlDQo+ZW5kcG9pbnQgd2FzIHBvd2Vy
ZWQgb2ZmIGFuZCBwb3dlcmVkIGJhY2sgb24gYWdhaW4sIEkgdGhpbmsgdGhhdA0KPmRldmljZSBs
b29rcyBsaWtlIGEgaG90LWFkZGVkIGRldmljZSwgZG9lc24ndCBpdD8NCj4NCg0KWWVzLCBpdCBp
cyBob3QtYWRkZWQgZGV2aWNlLiBBbHNvLCBJIHVuZGVyc3RhbmQsIGZvciBQT0xJQ1lfREVGQVVM
VCwgT1Mgd291bGQvc2hvdWxkIG5vdCB0b3VjaCBBU1BNKGVuYWJsZS9kaXNhYmxlKSwNCmJ1dCBC
SU9TIGNvdWxkIHN0aWxsIChlbmFibGUvZGlzYWJsZSksIHJpZ2h0Pw0KDQpDdXJyZW50bHksIHdo
YXQgaGFwcGVucyBpbiBteSBzeXN0ZW0gaXMgYXMgZm9sbG93aW5nLCAoZWFjaCAybmQgcG93ZXIg
Y3ljbGUvaG90cGx1ZyBvZiBFbmRwb2ludCBkaXNhYmxlcyBBU1BNKToNCg0KDQpGaXJzdCBQb3dl
ciBjeWNsZSAoV2hlbiBBU1BNIEwxIGlzIGFscmVhZHkgZW5hYmxlZCk6DQpkZXZpY2UgZ2V0cyBw
b3dlcmVkIG9mZiAtPiB0aGVyZSBhcmUgbm8gTGluayBzdGF0dXMgZXZlbnRzLCBzbyBubyBwY2ll
IGhvdHBsdWcgaW50ZXJydXB0IGFuZCBwY2llX2FzcG1fZXhpdF9saW5rX3N0YXRlKCkgdHJpZ2dl
cmVkLg0KV2hlbiB0aGUgZGV2aWNlIGdldHMgcG93ZXJlZCBvbiBhZ2FpbiAtPiBMaW5rIGRvd24v
TGluayB1cCBldmVudHMgYXJlIGNvbWluZyBiYWNrIHRvIGJhY2suIA0KRmlyc3QgTGluayBkb3du
IGlzIHNlcnZlZC4gKEJJT1MgY2hlY2tzIGZvciB0aGUgTGluayBzdGF0dXMgYW5kIGVuYWJsZXMg
QVNQTSBhbHJlYWR5LCBhcyB0aGUgZGV2aWNlIGlzDQphY3R1YWxseSBwb3dlcmVkIGJhY2spLiBP
UyBjYWxscyBwY2llX2FzcG1fZXhpdF9saW5rX3N0YXRlKCkgYW5kIEFTUE0gZ2V0cyBkaXNhYmxl
ZCBieSBPUy4NCg0KU2Vjb25kIFBvd2VyIGN5Y2xlIChXaGVuIEFTUE0gTDEgaXMgZGlzYWJsZWQg
YWZ0ZXIgYWJvdmUpOg0KZGV2aWNlIGdldHMgcG93ZXJlZCBvZmYgLT4gdGhlcmUgYXJlIGxpbmsg
c3RhdHVzIGV2ZW50cywgcGNpZSBob3RwbHVnIGludGVycnVwdCBpcyB0cmlnZ2VyZWQgYW5kIHBj
aWVfYXNwbV9leGl0X2xpbmtfc3RhdGUoKSB0cmlnZ2VyZWQuDQpPUyBkaXNhYmxlcyBBU1BNLiBC
SU9TIGNoZWNrcyBMaW5rIHN0YXR1cyBhbmQgZGlzYWJsZXMgQVNQTSB0b28uDQpXaGVuIHRoZSBk
ZXZpY2UgZ2V0cyBwb3dlcmVkIG9uIC0+IEJJT1MgZW5hYmxlcyBBU1BNIGFuZCBhcyB0aGlzIGlz
IHBjaWUgaG90cGx1ZyBpbnNlcnRpb24sIE9TDQpkb2VzIG5vdCBpbnRlcmZlcmUgYW5kIHdlIGhh
dmUgQVNQTSBlbmFibGVkLg0KDQpUaGUgYWJvdmUgc2VxdWVuY2UgaGFwcGVucyBlYWNoIDJuZCBw
b3dlciBjeWNsZSBvZiB0aGUgaG90cGx1ZyBkZXZpY2UuDQoNClNvIE9uZSBjb3VsZCBzdGlsbCBh
cmd1ZSBpZiBQT0xJQ1lfREVGQVVMVCBpcyBzZXQsIHRoZW4gd2h5IE9TIGRpc2FibGVzIEFTUE0g
aWYgaXQgaXMgbm90IG1lYW50IHRvIHRvdWNoIGNvbmZpZ3VyYXRpb24uDQpUaGlzIGlzIHdoeSBJ
IHByb3Bvc2VkIGZvbGxvd2luZyBraW5kIG9mIGNoYW5nZSwgc28gdGhhdCBPUyB3b3VsZCBub3Qg
dG91Y2ggQVNQTSwgaWYgUE9MSUNZX0RFRkFVTFQgaXMgc2V0Lg0KQWxzbywgV2l0aCB0aGUgYmVs
b3cgY2hhbmdlLCBldmVyeXRoaW5nIHJlbGllcyBvbiBCSU9TIGZvciBBU1BNIHdoZW4gUE9MSUNZ
X0RFRkFVTFQgaXMgc2V0IGFuZCBJIHNlZSBhYm92ZSBwcm9ibGVtDQpnZXRzIHJlc29sdmVkLiBB
bHNvLCB0aGUgZXhpc3RpbmcgQVNQTSBiZWhhdmlvciBkb2VzIG5vdCBoYXZlIGltcGFjdCwgdW5s
ZXNzIHNwZWNpZmljIEJJT1MgZG9lcyBub3QgZGlzYWJsZSBBU1BNIG9uDQpSb290IFBvcnQgd2hl
biBkZXZpY2UgZ2V0cyByZW1vdmVkLg0KDQoNCg0KPkJqb3JuDQpJbnRlbCBEZXV0c2NobGFuZCBH
bWJIClJlZ2lzdGVyZWQgQWRkcmVzczogQW0gQ2FtcGVvbiAxMC0xMiwgODU1NzkgTmV1YmliZXJn
LCBHZXJtYW55ClRlbDogKzQ5IDg5IDk5IDg4NTMtMCwgd3d3LmludGVsLmRlCk1hbmFnaW5nIERp
cmVjdG9yczogQ2hyaXN0aW4gRWlzZW5zY2htaWQsIENocmlzdGlhbiBMYW1wcmVjaHRlcgpDaGFp
cnBlcnNvbiBvZiB0aGUgU3VwZXJ2aXNvcnkgQm9hcmQ6IE5pY29sZSBMYXUKUmVnaXN0ZXJlZCBP
ZmZpY2U6IE11bmljaApDb21tZXJjaWFsIFJlZ2lzdGVyOiBBbXRzZ2VyaWNodCBNdWVuY2hlbiBI
UkIgMTg2OTI4Cg==

WARNING: multiple messages have this Message-ID (diff)
From: mayurkumar.patel@intel.com (Patel, Mayurkumar)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init
Date: Tue, 2 May 2017 12:02:53 +0000	[thread overview]
Message-ID: <92EBB4272BF81E4089A7126EC1E7B284667759B1@IRSMSX101.ger.corp.intel.com> (raw)
In-Reply-To: <CAErSpo7TO1rfG6MDYuohyXL4WbaGCJdV8y3Ke-uL6VjGRb1hvw@mail.gmail.com>

Hi Bjorn

>
>On Fri, Apr 21, 2017 at 2:46 AM, Patel, Mayurkumar
><mayurkumar.patel@intel.com> wrote:
>> Hi Bjorn/Kaya,
>>
>>
>>>
>>>On 4/17/2017 12:38 PM, Bjorn Helgaas wrote:
>>>>> Like you said, what do we do by default is the question. Should we opt
>>>>> for safe like we are doing, or try to save some power.
>>>> I think safety is paramount.  Every user should be able to boot safely
>>>> without any kernel parameters.  We don't want users to have a problem
>>>> booting and then have to search for a workaround like booting with
>>>> "pcie_aspm=off".  Most users will never do that.
>>>>
>>>
>>>OK, no problem with leaving the behavior as it is.
>>>
>>>My initial approach was #2. We knew this way that user had full control
>>>over the ASPM policy by changing the BIOS option. Then, Mayurkumar
>>>complained that ASPM is not enabled following a hotplug insertion to an
>>>empty slot. That's when I switched to #3 as it sounded like a good thing
>>>to have for us.
>>>
>>>> Here's a long-term strawman proposal, see what you think:
>>>>
>>>>   - Deprecate CONFIG_PCIEASPM_DEFAULT, CONFIG_PCIEASPM_POWERSAVE, etc.
>>>>   - Default aspm_policy is POLICY_DEFAULT always.
>>>>   - POLICY_DEFAULT means Linux doesn't touch anything: if BIOS enabled
>>>> ASPM, we leave it that way; we leave ASPM disabled on hot-added
>>>> devices.
>>>
>> I am also ok with leaving the same behavior as now.
>> But still following is something open I feel besides, Which may be there in your comments redundantly.
>> The current problem is, pcie_aspm_exit_link_state() disables the ASPM configuration even
>> if POLICY_DEFAULT was set.
>
>We call pcie_aspm_exit_link_state() when removing an endpoint.  When
>we remove an endpoint, I think disabling ASPM is the right thing to
>do.  The spec (PCIe r3.1, sec 5.4.1.3) 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."
>

Yes, you are right and per spec also it makes sense that ASPM needs to be disabled.
But, if POLICY_DEFAULT is set then, shouldn't BIOS take care of disabling ASPM?



>> I am seeing already following problem(or may be influence) with it. The Endpoint I have does not have
>> does not have "Presence detect change" mechanism. Hot plug is working with Link status events.
>> When link is in L1 or L1SS and if EP is powered off, no Link status change event are triggered (It might be
>> the expected behavior in L1 or L1SS).  When next time EP is powered on there are link down and
>> link up events coming one after other. BIOS enables ASPM on Root port and Endpoint, but while
>> processing link status down, pcie_aspm_exit_link_state() clears the ASPM already which were enabled by BIOS.
>> If we want to follow above approach then shall we consider having something similar as following?
>
>The proposal was to leave ASPM disabled on hot-added devices.  If the
>endpoint was powered off and powered back on again, I think that
>device looks like a hot-added device, doesn't it?
>

Yes, it is hot-added device. Also, I understand, for POLICY_DEFAULT, OS would/should not touch ASPM(enable/disable),
but BIOS could still (enable/disable), right?

Currently, what happens in my system is as following, (each 2nd power cycle/hotplug of Endpoint disables ASPM):


First Power cycle (When ASPM L1 is already enabled):
device gets powered off -> there are no Link status events, so no pcie hotplug interrupt and pcie_aspm_exit_link_state() triggered.
When the device gets powered on again -> Link down/Link up events are coming back to back. 
First Link down is served. (BIOS checks for the Link status and enables ASPM already, as the device is
actually powered back). OS calls pcie_aspm_exit_link_state() and ASPM gets disabled by OS.

Second Power cycle (When ASPM L1 is disabled after above):
device gets powered off -> there are link status events, pcie hotplug interrupt is triggered and pcie_aspm_exit_link_state() triggered.
OS disables ASPM. BIOS checks Link status and disables ASPM too.
When the device gets powered on -> BIOS enables ASPM and as this is pcie hotplug insertion, OS
does not interfere and we have ASPM enabled.

The above sequence happens each 2nd power cycle of the hotplug device.

So One could still argue if POLICY_DEFAULT is set, then why OS disables ASPM if it is not meant to touch configuration.
This is why I proposed following kind of change, so that OS would not touch ASPM, if POLICY_DEFAULT is set.
Also, With the below change, everything relies on BIOS for ASPM when POLICY_DEFAULT is set and I see above problem
gets resolved. Also, the existing ASPM behavior does not have impact, unless specific BIOS does not disable ASPM on
Root Port when device gets removed.



>Bjorn
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928

  reply	other threads:[~2017-05-02 12:02 UTC|newest]

Thread overview: 94+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-08  4:55 [PATCH V8 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT Sinan Kaya
2017-04-08  4:55 ` Sinan Kaya
2017-04-08  4:55 ` Sinan Kaya
2017-04-08  4:55 ` [PATCH V8 1/5] PCI/ASPM: introduce pci_aspm_init() and add to pci_init_capabilities() Sinan Kaya
2017-04-08  4:55   ` Sinan Kaya
2017-04-08  4:55   ` Sinan Kaya
2017-04-13 20:51   ` Bjorn Helgaas
2017-04-13 20:51     ` Bjorn Helgaas
2017-04-14 19:10     ` Sinan Kaya
2017-04-14 19:10       ` Sinan Kaya
2017-04-08  4:55 ` [PATCH V8 2/5] PCI/ASPM: split pci_aspm_init() into two Sinan Kaya
2017-04-08  4:55   ` Sinan Kaya
2017-04-08  4:55   ` Sinan Kaya
2017-04-12 19:16   ` Rajat Jain
2017-04-12 19:16     ` Rajat Jain
2017-04-13 18:25     ` Bjorn Helgaas
2017-04-13 18:25       ` Bjorn Helgaas
2017-04-13 18:25       ` Bjorn Helgaas
2017-04-14 19:10       ` Sinan Kaya
2017-04-14 19:10         ` Sinan Kaya
2017-04-14 19:10         ` Sinan Kaya
2017-04-08  4:55 ` [PATCH V8 3/5] PCI/ASPM: add init hook to device_add Sinan Kaya
2017-04-08  4:55   ` Sinan Kaya
2017-04-08  4:55   ` Sinan Kaya
2017-04-13 20:48   ` Bjorn Helgaas
2017-04-13 20:48     ` Bjorn Helgaas
2017-04-13 20:48     ` Bjorn Helgaas
2017-04-13 21:02     ` Bjorn Helgaas
2017-04-13 21:02       ` Bjorn Helgaas
2017-04-13 21:02       ` Bjorn Helgaas
2017-04-14  1:19       ` Sinan Kaya
2017-04-14  1:19         ` Sinan Kaya
2017-04-14  1:30         ` Bjorn Helgaas
2017-04-14  1:30           ` Bjorn Helgaas
2017-04-08  4:55 ` [PATCH V8 4/5] PCI/ASPM: save power on values during bridge init Sinan Kaya
2017-04-08  4:55   ` Sinan Kaya
2017-04-08  4:55   ` Sinan Kaya
2017-04-12 19:19   ` Rajat Jain
2017-04-12 19:19     ` Rajat Jain
2017-04-12 19:19     ` Rajat Jain
2017-04-14 19:12     ` Sinan Kaya
2017-04-14 19:12       ` Sinan Kaya
2017-04-14 19:12       ` Sinan Kaya
2017-04-14 21:44       ` Bjorn Helgaas
2017-04-14 21:44         ` Bjorn Helgaas
2017-04-14 21:44         ` Bjorn Helgaas
2017-04-14 22:17         ` Sinan Kaya
2017-04-14 22:17           ` Sinan Kaya
2017-04-17 16:38           ` Bjorn Helgaas
2017-04-17 16:38             ` Bjorn Helgaas
2017-04-17 17:50             ` Sinan Kaya
2017-04-17 17:50               ` Sinan Kaya
2017-04-21  7:46               ` Patel, Mayurkumar
2017-04-21  7:46                 ` Patel, Mayurkumar
2017-04-21  7:46                 ` Patel, Mayurkumar
2017-04-21  7:46                 ` Patel, Mayurkumar
2017-04-21 13:50                 ` Sinan Kaya
2017-04-21 13:50                   ` Sinan Kaya
2017-04-21 14:13                   ` Patel, Mayurkumar
2017-04-21 14:13                     ` Patel, Mayurkumar
2017-04-21 14:13                     ` Patel, Mayurkumar
2017-04-21 14:13                     ` Patel, Mayurkumar
2017-04-25 18:45                 ` Bjorn Helgaas
2017-04-25 18:45                   ` Bjorn Helgaas
2017-05-02 12:02                   ` Patel, Mayurkumar [this message]
2017-05-02 12:02                     ` Patel, Mayurkumar
2017-05-02 12:02                     ` Patel, Mayurkumar
2017-05-03 21:10                     ` Bjorn Helgaas
2017-05-03 21:10                       ` Bjorn Helgaas
2017-05-03 21:10                       ` Bjorn Helgaas
2017-05-15  9:10                       ` Patel, Mayurkumar
2017-05-15  9:10                         ` Patel, Mayurkumar
2017-05-15  9:10                         ` Patel, Mayurkumar
2017-05-15  9:10                         ` Patel, Mayurkumar
2017-04-08  4:55 ` [PATCH V8 5/5] PCI/ASPM: move link_state cleanup to bridge remove Sinan Kaya
2017-04-08  4:55   ` Sinan Kaya
2017-04-08  4:55   ` Sinan Kaya
2017-04-10 11:37 ` [PATCH V8 0/5] PCI/ASPM: reconfigure ASPM following hotplug for POLICY_DEFAULT Patel, Mayurkumar
2017-04-10 11:37   ` Patel, Mayurkumar
2017-04-10 11:37   ` Patel, Mayurkumar
2017-04-10 13:07   ` Sinan Kaya
2017-04-10 13:07     ` Sinan Kaya
2017-04-10 13:07     ` Sinan Kaya
2017-04-10 13:11     ` Patel, Mayurkumar
2017-04-10 13:11       ` Patel, Mayurkumar
2017-04-10 13:11       ` Patel, Mayurkumar
2017-04-11 21:19 ` Bjorn Helgaas
2017-04-11 21:19   ` Bjorn Helgaas
2017-04-11 21:19   ` Bjorn Helgaas
2017-04-11 21:27   ` Sinan Kaya
2017-04-11 21:27     ` Sinan Kaya
2017-04-11 22:41     ` Bjorn Helgaas
2017-04-11 22:41       ` Bjorn Helgaas
2017-04-11 22:41       ` Bjorn Helgaas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=92EBB4272BF81E4089A7126EC1E7B284667759B1@IRSMSX101.ger.corp.intel.com \
    --to=mayurkumar.patel@intel.com \
    --cc=Julia.Lawall@lip6.fr \
    --cc=bhelgaas@google.com \
    --cc=david.daney@cavium.com \
    --cc=helgaas@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-arm-msm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=myron.stowe@redhat.com \
    --cc=okaya@codeaurora.org \
    --cc=rajatja@google.com \
    --cc=rajatxjain@gmail.com \
    --cc=shawn.lin@rock-chips.com \
    --cc=timur@codeaurora.org \
    --cc=yinghai@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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.