Re: Query on ASPM driver design

Re: Query on ASPM driver design

[hemantk]

Hi Bjorn,
On 2021-07-23 13:32, Bjorn Helgaas wrote:
> On Fri, Jul 23, 2021 at 01:11:18PM -0700, hemantk@codeaurora.org wrote:
>> Hi Bjorn,
> 
> It's best if you can cc: linux-pci@vger.kernel.org so others can
> contribute and benefit.
Good idea. Added now.
> 
>> I have a question regarding PCIe ASPM driver in upstream. Looks like
>> current ASPM driver is going to enable ASPM L1 and L1SS based on
>> EP's config space capability register read. Why ASPM driver is
>> enabling L1SS based on capability, instead of that can ASPM honor
>> default control register value (in EP config space) and let pci
>> device driver probe (or later after probe) to make the decision if
>> ASPM needs to be enabled or not.
> 
> Are you asking why the PCI core makes the decision about enabling ASPM
> instead of having each device driver decide?
Yes.
> 
> If you want each driver to decide, what benefit would that have?
Basically if PCI EP has capability to support ASPM L1 and L1SS but
power on default control reg values are meant to enumerate with ASPM 
disabled.
Which means EP wants to keep ASPM disabled right from the enumeration, 
and at some
point of time later EP wants to enable the ASPM. Main benefit is to give 
control
to EP to enumerate with what ever its control reg's power on default 
value is. EP
does not want to enable ASPM during its boot up and after entering to 
mission
mode use case it would enable the ASPM.

> 
> Obviously ASPM involves a power vs performance tradeoff, but
> functionally, ASPM is designed so drivers don't need to be aware of
> it.
> 
>> As far as i know there is an option to update quirk.c for a given
>> device id and vendor id to disable L0s/L1/L1SS but that sounds like
>> adding a software workaround to a device specific HW bug.
> 
> Yes.  It is common practice to use software quirks to work around
> hardware defects.
> 
>> Also, i know 5.5 kernel added a patch to control aspm using sysfs
>> per link basis:-
> 
>> https://patchwork.kernel.org/project/linux-pci/patch/b1c83f8a-9bf6-eac5-82d0-cf5b90128fbf@gmail.com/
> 
> Yes.  This is intended for use by tools like powertop to tune the
> power vs performance tradeoff.
> 
>> Basically point is: it is possible to honor what device control reg
>> reflects power on default and let the pci ep driver running on host
>> to make the decision when to enable/disable the aspm in kernel space
>> pci driver.
> 
> There is a pci_disable_link_state() interface that drivers can use to
> disable certain link states.  Some drivers use this to work around
> hardware defects, but it would be better to use quirks in that
> situation.
Thanks for pointing this API, which quirk also uses. But we just have
disable ver which EP driver can call only after enumeration is done. i 
was
thinking of the other way round where EP enumerates and then calls 
enable
API at some point of time. Also, if it decides to again disable and then 
enable.

> 
>> Sorry if this was already well thought/discussed argument in the
>> design of ASPM driver, i would appreciate, if you can shed some
>> light on the reason for not taking that approach.
> 
> I don't know the history of that decision, but in general we try to do
> things in the core instead of endpoint drivers whenever possible
> because it reduces the complexity of drivers.
> 
> Bjorn

Thanks for taking your time to respond very promptly Bjorn!

-Hemant

---
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
Forum,
a Linux Foundation Collaborative Project

Re: Query on ASPM driver design

[Bjorn Helgaas]

On Fri, Jul 23, 2021 at 03:04:52PM -0700, hemantk@codeaurora.org wrote:
> On 2021-07-23 13:32, Bjorn Helgaas wrote:
> > On Fri, Jul 23, 2021 at 01:11:18PM -0700, hemantk@codeaurora.org wrote:
> > > I have a question regarding PCIe ASPM driver in upstream. Looks like
> > > current ASPM driver is going to enable ASPM L1 and L1SS based on
> > > EP's config space capability register read. Why ASPM driver is
> > > enabling L1SS based on capability, instead of that can ASPM honor
> > > default control register value (in EP config space) and let pci
> > > device driver probe (or later after probe) to make the decision if
> > > ASPM needs to be enabled or not.
> > 
> > Are you asking why the PCI core makes the decision about enabling ASPM
> > instead of having each device driver decide?
>
> Yes.
>
> > If you want each driver to decide, what benefit would that have?
>
> Basically if PCI EP has capability to support ASPM L1 and L1SS but
> power on default control reg values are meant to enumerate with ASPM
> disabled.  Which means EP wants to keep ASPM disabled right from the
> enumeration, and at some point of time later EP wants to enable the
> ASPM. Main benefit is to give control to EP to enumerate with what
> ever its control reg's power on default value is. EP does not want
> to enable ASPM during its boot up and after entering to mission mode
> use case it would enable the ASPM.

The power-on default value for the "ASPM Control" field in the Link
Control register is 00b, which means ASPM is disabled.  The current
Linux behavior is that when we enumerate the device, we evaluate the
L0s and L1 exit latencies and enable ASPM if the device can tolerate
them.

It sounds like you want to prevent ASPM from being enabled until the
driver explicitly enables it.  Why?  The device should not be active
until a driver claims it, so it should not be a problem to have ASPM
enabled.

> > > Basically point is: it is possible to honor what device control reg
> > > reflects power on default and let the pci ep driver running on host
> > > to make the decision when to enable/disable the aspm in kernel space
> > > pci driver.
> > 
> > There is a pci_disable_link_state() interface that drivers can use to
> > disable certain link states.  Some drivers use this to work around
> > hardware defects, but it would be better to use quirks in that
> > situation.
>
> Thanks for pointing this API, which quirk also uses. But we just
> have disable ver which EP driver can call only after enumeration is
> done. i was thinking of the other way round where EP enumerates and
> then calls enable API at some point of time. Also, if it decides to
> again disable and then enable.

There is currently no pci_enable_link_state() because nobody has
needed it and implemented it.  I would push back a little bit on
adding this because I don't want to encourage drivers to mess with
ASPM.

Bjorn

Re: Query on ASPM driver design

[Om Prakash Singh]

Hi Bjorn,
I think it makes sense to have the scope of keeping default ASPM policy 
disable and API pci_enable_link_state() to selectively enable by EP Driver.

sysfs interface for ASPM also does not allow enabling ASPM for a device 
if the default policy (policy_to_aspm_state()) does not allow it.

Consider a situation, for a platform one wants to utilize ASPM 
capability of an onboard PCIe device because it is well evaluated, at 
the same time they want to keep ASPM disabled for other PCIe devices 
that can be connected on open PCIe slot to avoid possible performance issue.

I see ASPM is broken on many devices, though the device shows ASPM 
capabilities but has performance issues when it is enabled.

Thanks,
Om


On 7/24/2021 3:58 AM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Fri, Jul 23, 2021 at 03:04:52PM -0700, hemantk@codeaurora.org wrote:
>> On 2021-07-23 13:32, Bjorn Helgaas wrote:
>>> On Fri, Jul 23, 2021 at 01:11:18PM -0700, hemantk@codeaurora.org wrote:
>>>> I have a question regarding PCIe ASPM driver in upstream. Looks like
>>>> current ASPM driver is going to enable ASPM L1 and L1SS based on
>>>> EP's config space capability register read. Why ASPM driver is
>>>> enabling L1SS based on capability, instead of that can ASPM honor
>>>> default control register value (in EP config space) and let pci
>>>> device driver probe (or later after probe) to make the decision if
>>>> ASPM needs to be enabled or not.
>>>
>>> Are you asking why the PCI core makes the decision about enabling ASPM
>>> instead of having each device driver decide?
>>
>> Yes.
>>
>>> If you want each driver to decide, what benefit would that have?
>>
>> Basically if PCI EP has capability to support ASPM L1 and L1SS but
>> power on default control reg values are meant to enumerate with ASPM
>> disabled.  Which means EP wants to keep ASPM disabled right from the
>> enumeration, and at some point of time later EP wants to enable the
>> ASPM. Main benefit is to give control to EP to enumerate with what
>> ever its control reg's power on default value is. EP does not want
>> to enable ASPM during its boot up and after entering to mission mode
>> use case it would enable the ASPM.
> 
> The power-on default value for the "ASPM Control" field in the Link
> Control register is 00b, which means ASPM is disabled.  The current
> Linux behavior is that when we enumerate the device, we evaluate the
> L0s and L1 exit latencies and enable ASPM if the device can tolerate
> them.
> 
> It sounds like you want to prevent ASPM from being enabled until the
> driver explicitly enables it.  Why?  The device should not be active
> until a driver claims it, so it should not be a problem to have ASPM
> enabled.
> 
>>>> Basically point is: it is possible to honor what device control reg
>>>> reflects power on default and let the pci ep driver running on host
>>>> to make the decision when to enable/disable the aspm in kernel space
>>>> pci driver.
>>>
>>> There is a pci_disable_link_state() interface that drivers can use to
>>> disable certain link states.  Some drivers use this to work around
>>> hardware defects, but it would be better to use quirks in that
>>> situation.
>>
>> Thanks for pointing this API, which quirk also uses. But we just
>> have disable ver which EP driver can call only after enumeration is
>> done. i was thinking of the other way round where EP enumerates and
>> then calls enable API at some point of time. Also, if it decides to
>> again disable and then enable.
> 
> There is currently no pci_enable_link_state() because nobody has
> needed it and implemented it.  I would push back a little bit on
> adding this because I don't want to encourage drivers to mess with
> ASPM.
> 
> Bjorn
> 

Re: Query on ASPM driver design

[Bjorn Helgaas]

On Tue, Jul 27, 2021 at 07:51:37AM +0530, Om Prakash Singh wrote:
> Hi Bjorn,
> I think it makes sense to have the scope of keeping default ASPM
> policy disable and API pci_enable_link_state() to selectively enable
> by EP Driver.
>
> sysfs interface for ASPM also does not allow enabling ASPM for a
> device if the default policy (policy_to_aspm_state()) does not allow
> it.

The ASPM policy implementation may require changes.  I think the
current setup where a policy is compiled into the kernel via Kconfig
options is seriously flawed.

We need a fail-safe kernel parameter, i.e., "pcie_aspm=off", for cases
where devices don't work at all with ASPM.  We need quirks to work
around devices known to be broken, e.g., those that advertise ASPM
support that doesn't actually work, or those that advertise incorrect
exit latencies.  I think most other configuration should be done via
sysfs.

> Consider a situation, for a platform one wants to utilize ASPM
> capability of an onboard PCIe device because it is well evaluated,
> at the same time they want to keep ASPM disabled for other PCIe
> devices that can be connected on open PCIe slot to avoid possible
> performance issue.
> 
> I see ASPM is broken on many devices, though the device shows ASPM
> capabilities but has performance issues when it is enabled.

I'll wait to see your proposal and use case before commenting on this.

Bjorn

Re: Query on ASPM driver design

[Om Prakash Singh]

On 7/27/2021 8:48 PM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
> 
> 
> On Tue, Jul 27, 2021 at 07:51:37AM +0530, Om Prakash Singh wrote:
>> Hi Bjorn,
>> I think it makes sense to have the scope of keeping default ASPM
>> policy disable and API pci_enable_link_state() to selectively enable
>> by EP Driver.
>>
>> sysfs interface for ASPM also does not allow enabling ASPM for a
>> device if the default policy (policy_to_aspm_state()) does not allow
>> it.
> 
> The ASPM policy implementation may require changes.  I think the
> current setup where a policy is compiled into the kernel via Kconfig
> options is seriously flawed.
> 
> We need a fail-safe kernel parameter, i.e., "pcie_aspm=off", for cases
> where devices don't work at all with ASPM.  We need quirks to work
> around devices known to be broken, e.g., those that advertise ASPM
> support that doesn't actually work, or those that advertise incorrect
> exit latencies.  I think most other configuration should be done via
> sysfs.
> 
>> Consider a situation, for a platform one wants to utilize ASPM
>> capability of an onboard PCIe device because it is well evaluated,
>> at the same time they want to keep ASPM disabled for other PCIe
>> devices that can be connected on open PCIe slot to avoid possible
>> performance issue.
>>
>> I see ASPM is broken on many devices, though the device shows ASPM
>> capabilities but has performance issues when it is enabled.
> 
> I'll wait to see your proposal and use case before commenting on this.

few suggestions:

1. We can have a device-tree property to disable ASPM capabilities of a 
Root port. Corresponding to my example, the device-tree can use be use 
to enable ASPM capabilities of Root ports that have known/good/onboard 
PCIe device, and keep ASPM disabled for opens slots

2. sysfs should allow overriding default ASPM policy.
    With below change system can boot with default policy ASPM disabled 
(performance). But sysfs will still allow to enable ASPM capability of 
selective device

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index a08e7d6..268c2c5 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -1278,7 +1278,7 @@
  		link->aspm_disable |= state;
  	}

-	pcie_config_aspm_link(link, policy_to_aspm_state(link));
+	pcie_config_aspm_link(link, state);

  	mutex_unlock(&aspm_lock);
  	up_read(&pci_bus_sem);


3. Add API pci_enable_link_state()
    This will give control to driver to change ASPM at runtime depending 
on user selected performance mode. For example Android has different 
performance modes for Wifi chip, for sleep and active state. We are 
using similar API to overcome some performance issue related to wifi on 
our internal platform.




> 
> Bjorn
> 

Re: Query on ASPM driver design

[Bjorn Helgaas]

[+cc Rafael, driver-specific vs generic PM at very end]

On Wed, Jul 28, 2021 at 06:48:50AM +0530, Om Prakash Singh wrote:
> On 7/27/2021 8:48 PM, Bjorn Helgaas wrote:
> > On Tue, Jul 27, 2021 at 07:51:37AM +0530, Om Prakash Singh wrote:
> > > Hi Bjorn,
> > > I think it makes sense to have the scope of keeping default ASPM
> > > policy disable and API pci_enable_link_state() to selectively enable
> > > by EP Driver.
> > > 
> > > sysfs interface for ASPM also does not allow enabling ASPM for a
> > > device if the default policy (policy_to_aspm_state()) does not allow
> > > it.
> > 
> > The ASPM policy implementation may require changes.  I think the
> > current setup where a policy is compiled into the kernel via Kconfig
> > options is seriously flawed.
> > 
> > We need a fail-safe kernel parameter, i.e., "pcie_aspm=off", for cases
> > where devices don't work at all with ASPM.  We need quirks to work
> > around devices known to be broken, e.g., those that advertise ASPM
> > support that doesn't actually work, or those that advertise incorrect
> > exit latencies.  I think most other configuration should be done via
> > sysfs.
> > 
> > > Consider a situation, for a platform one wants to utilize ASPM
> > > capability of an onboard PCIe device because it is well evaluated,
> > > at the same time they want to keep ASPM disabled for other PCIe
> > > devices that can be connected on open PCIe slot to avoid possible
> > > performance issue.
> > > 
> > > I see ASPM is broken on many devices, though the device shows ASPM
> > > capabilities but has performance issues when it is enabled.
> > 
> > I'll wait to see your proposal and use case before commenting on this.
> 
> few suggestions:
> 
> 1. We can have a device-tree property to disable ASPM capabilities of a Root
> port. Corresponding to my example, the device-tree can use be use to enable
> ASPM capabilities of Root ports that have known/good/onboard PCIe device,
> and keep ASPM disabled for opens slots

ASPM can only be enabled for links between two devices.  For empty
slots, there's an upstream device (Root Port or Switch Downstream
Port) but no downstream device, so ASPM won't be enabled anyway.

> 2. sysfs should allow overriding default ASPM policy.
>    With below change system can boot with default policy ASPM disabled
> (performance). But sysfs will still allow to enable ASPM capability of
> selective device

This sounds like a good idea.  If we could get rid of the Kconfig ASPM
policy selection, we'd likely make PCIEASPM_POWER_SUPERSAVE the
default.  Then userspace could use sysfs to disable ASPM states
selective as necessary for performance.

Even before removing the Kconfig policy selection (or if we can't do
that), I think it make sense to allow sysfs to override it.

> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index a08e7d6..268c2c5 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -1278,7 +1278,7 @@
>  		link->aspm_disable |= state;
>  	}
> 
> -	pcie_config_aspm_link(link, policy_to_aspm_state(link));
> +	pcie_config_aspm_link(link, state);
> 
>  	mutex_unlock(&aspm_lock);
>  	up_read(&pci_bus_sem);
> 
> 3. Add API pci_enable_link_state()
>    This will give control to driver to change ASPM at runtime depending on
> user selected performance mode. For example Android has different
> performance modes for Wifi chip, for sleep and active state. We are using
> similar API to overcome some performance issue related to wifi on our
> internal platform.

I'd prefer to keep performance modes out of the drivers as much as
possible because that's not really a scalable approach.  I'm not a
power management expert, but I suspect the most maintainable approach
is to do this in the generic PM core, with selectable overrides via
sysfs as necessary.  I cc'd Rafael in case he wants to chime in.

Maybe there's a place for drivers to tweak ASPM at runtime, but I'm
not convinced yet.  I think the intent of ASPM is that devices
advertise exit latencies that correspond with their internal
buffering, so the L0s and L1 states can be used with no significant
performance impact.  But without more specifics, we can talk about
this all day without getting anywhere.

Bjorn

Re: Query on ASPM driver design

[Om Prakash Singh]

On 7/28/2021 9:31 PM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
> 
> 
> [+cc Rafael, driver-specific vs generic PM at very end]
> 
> On Wed, Jul 28, 2021 at 06:48:50AM +0530, Om Prakash Singh wrote:
>> On 7/27/2021 8:48 PM, Bjorn Helgaas wrote:
>>> On Tue, Jul 27, 2021 at 07:51:37AM +0530, Om Prakash Singh wrote:
>>>> Hi Bjorn,
>>>> I think it makes sense to have the scope of keeping default ASPM
>>>> policy disable and API pci_enable_link_state() to selectively enable
>>>> by EP Driver.
>>>>
>>>> sysfs interface for ASPM also does not allow enabling ASPM for a
>>>> device if the default policy (policy_to_aspm_state()) does not allow
>>>> it.
>>>
>>> The ASPM policy implementation may require changes.  I think the
>>> current setup where a policy is compiled into the kernel via Kconfig
>>> options is seriously flawed.
>>>
>>> We need a fail-safe kernel parameter, i.e., "pcie_aspm=off", for cases
>>> where devices don't work at all with ASPM.  We need quirks to work
>>> around devices known to be broken, e.g., those that advertise ASPM
>>> support that doesn't actually work, or those that advertise incorrect
>>> exit latencies.  I think most other configuration should be done via
>>> sysfs.
>>>
>>>> Consider a situation, for a platform one wants to utilize ASPM
>>>> capability of an onboard PCIe device because it is well evaluated,
>>>> at the same time they want to keep ASPM disabled for other PCIe
>>>> devices that can be connected on open PCIe slot to avoid possible
>>>> performance issue.
>>>>
>>>> I see ASPM is broken on many devices, though the device shows ASPM
>>>> capabilities but has performance issues when it is enabled.
>>>
>>> I'll wait to see your proposal and use case before commenting on this.
>>
>> few suggestions:
>>
>> 1. We can have a device-tree property to disable ASPM capabilities of a Root
>> port. Corresponding to my example, the device-tree can use be use to enable
>> ASPM capabilities of Root ports that have known/good/onboard PCIe device,
>> and keep ASPM disabled for opens slots
> 
> ASPM can only be enabled for links between two devices.  For empty
> slots, there's an upstream device (Root Port or Switch Downstream
> Port) but no downstream device, so ASPM won't be enabled anyway.

Yes, I meant to say this device-tree property will allow to disable ASPM 
capabilities of selected root port, ASPM will be disabled even if 
downstream device has ASPM capability.

> 
>> 2. sysfs should allow overriding default ASPM policy.
>>     With below change system can boot with default policy ASPM disabled
>> (performance). But sysfs will still allow to enable ASPM capability of
>> selective device
> 
> This sounds like a good idea.  If we could get rid of the Kconfig ASPM
> policy selection, we'd likely make PCIEASPM_POWER_SUPERSAVE the
> default.  Then userspace could use sysfs to disable ASPM states
> selective as necessary for performance.
> 
> Even before removing the Kconfig policy selection (or if we can't do
> that), I think it make sense to allow sysfs to override it.

I see Kconfig policy will is useful as different platform owner may want 
to keep different default policy for their platform.

> 
>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>> index a08e7d6..268c2c5 100644
>> --- a/drivers/pci/pcie/aspm.c
>> +++ b/drivers/pci/pcie/aspm.c
>> @@ -1278,7 +1278,7 @@
>>                link->aspm_disable |= state;
>>        }
>>
>> -     pcie_config_aspm_link(link, policy_to_aspm_state(link));
>> +     pcie_config_aspm_link(link, state);
>>
>>        mutex_unlock(&aspm_lock);
>>        up_read(&pci_bus_sem);
>>
>> 3. Add API pci_enable_link_state()
>>     This will give control to driver to change ASPM at runtime depending on
>> user selected performance mode. For example Android has different
>> performance modes for Wifi chip, for sleep and active state. We are using
>> similar API to overcome some performance issue related to wifi on our
>> internal platform.
> 
> I'd prefer to keep performance modes out of the drivers as much as
> possible because that's not really a scalable approach.  I'm not a
> power management expert, but I suspect the most maintainable approach
> is to do this in the generic PM core, with selectable overrides via
> sysfs as necessary.  I cc'd Rafael in case he wants to chime in.
> 
> Maybe there's a place for drivers to tweak ASPM at runtime, but I'm
> not convinced yet.  I think the intent of ASPM is that devices
> advertise exit latencies that correspond with their internal
> buffering, so the L0s and L1 states can be used with no significant
> performance impact.  But without more specifics, we can talk about
> this all day without getting anywhere.

We are discussing about all these options because many times device 
doesn't performs well with ASPM capabilities what they advertise.

> 
> Bjorn
> 

Re: Query on ASPM driver design

[Om Prakash Singh]

On 7/28/2021 9:31 PM, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
> 
> 
> [+cc Rafael, driver-specific vs generic PM at very end]
> 
> On Wed, Jul 28, 2021 at 06:48:50AM +0530, Om Prakash Singh wrote:
>> On 7/27/2021 8:48 PM, Bjorn Helgaas wrote:
>>> On Tue, Jul 27, 2021 at 07:51:37AM +0530, Om Prakash Singh wrote:
>>>> Hi Bjorn,
>>>> I think it makes sense to have the scope of keeping default ASPM
>>>> policy disable and API pci_enable_link_state() to selectively enable
>>>> by EP Driver.
>>>>
>>>> sysfs interface for ASPM also does not allow enabling ASPM for a
>>>> device if the default policy (policy_to_aspm_state()) does not allow
>>>> it.
>>>
>>> The ASPM policy implementation may require changes.  I think the
>>> current setup where a policy is compiled into the kernel via Kconfig
>>> options is seriously flawed.
>>>
>>> We need a fail-safe kernel parameter, i.e., "pcie_aspm=off", for cases
>>> where devices don't work at all with ASPM.  We need quirks to work
>>> around devices known to be broken, e.g., those that advertise ASPM
>>> support that doesn't actually work, or those that advertise incorrect
>>> exit latencies.  I think most other configuration should be done via
>>> sysfs.
>>>
>>>> Consider a situation, for a platform one wants to utilize ASPM
>>>> capability of an onboard PCIe device because it is well evaluated,
>>>> at the same time they want to keep ASPM disabled for other PCIe
>>>> devices that can be connected on open PCIe slot to avoid possible
>>>> performance issue.
>>>>
>>>> I see ASPM is broken on many devices, though the device shows ASPM
>>>> capabilities but has performance issues when it is enabled.
>>>
>>> I'll wait to see your proposal and use case before commenting on this.
>>
>> few suggestions:
>>
>> 1. We can have a device-tree property to disable ASPM capabilities of a Root
>> port. Corresponding to my example, the device-tree can use be use to enable
>> ASPM capabilities of Root ports that have known/good/onboard PCIe device,
>> and keep ASPM disabled for opens slots
> 
> ASPM can only be enabled for links between two devices.  For empty
> slots, there's an upstream device (Root Port or Switch Downstream
> Port) but no downstream device, so ASPM won't be enabled anyway.
> 
>> 2. sysfs should allow overriding default ASPM policy.
>>     With below change system can boot with default policy ASPM disabled
>> (performance). But sysfs will still allow to enable ASPM capability of
>> selective device
> 
> This sounds like a good idea.  If we could get rid of the Kconfig ASPM
> policy selection, we'd likely make PCIEASPM_POWER_SUPERSAVE the
> default.  Then userspace could use sysfs to disable ASPM states
> selective as necessary for performance.
> 

want to clarify this point, my idea was to have to have all ASPM mode 
disable by default (PERFORMANCE). And through sysfs user should be 
enable selected ASPM mode for each device. With current upstream code 
this is not possible. We can introduce one more policy USER_SELECTABLE 
if it make more sense.

Keeping ASPM disabled as default policy is important as any rough device 
connected can impact on overall system performance.

Adding Heiner Kallweit to share his opinion as well

> Even before removing the Kconfig policy selection (or if we can't do
> that), I think it make sense to allow sysfs to override it.
> 
>> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
>> index a08e7d6..268c2c5 100644
>> --- a/drivers/pci/pcie/aspm.c
>> +++ b/drivers/pci/pcie/aspm.c
>> @@ -1278,7 +1278,7 @@
>>                link->aspm_disable |= state;
>>        }
>>
>> -     pcie_config_aspm_link(link, policy_to_aspm_state(link));
>> +     pcie_config_aspm_link(link, state);
>>
>>        mutex_unlock(&aspm_lock);
>>        up_read(&pci_bus_sem);
>>
>> 3. Add API pci_enable_link_state()
>>     This will give control to driver to change ASPM at runtime depending on
>> user selected performance mode. For example Android has different
>> performance modes for Wifi chip, for sleep and active state. We are using
>> similar API to overcome some performance issue related to wifi on our
>> internal platform.
> 
> I'd prefer to keep performance modes out of the drivers as much as
> possible because that's not really a scalable approach.  I'm not a
> power management expert, but I suspect the most maintainable approach
> is to do this in the generic PM core, with selectable overrides via
> sysfs as necessary.  I cc'd Rafael in case he wants to chime in.
> 
> Maybe there's a place for drivers to tweak ASPM at runtime, but I'm
> not convinced yet.  I think the intent of ASPM is that devices
> advertise exit latencies that correspond with their internal
> buffering, so the L0s and L1 states can be used with no significant
> performance impact.  But without more specifics, we can talk about
> this all day without getting anywhere.
> 
> Bjorn
>