Re: Query on ASPM driver design

[hemantk@codeaurora.org]

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