Re: Query on ASPM driver design

[Bjorn Helgaas <helgaas@kernel.org>]

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