Re: [PATCH] PCI/ASPM: Enable ASPM on external PCIe devices

[Bjorn Helgaas <helgaas@kernel.org>]

On Mon, Jun 19, 2023 at 11:16:35AM -0500, Limonciello, Mario wrote:
> On 6/15/2023 10:01 PM, Kai-Heng Feng wrote:
> > On Fri, Jun 16, 2023 at 1:12 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> > > On Thu, Jun 15, 2023 at 03:04:20PM +0800, Kai-Heng Feng wrote:
> > > > When a PCIe device is hotplugged to a Thunderbolt port, ASPM is not
> > > > enabled for that device. However, when the device is plugged preboot,
> > > > ASPM is enabled by default.
> > > > 
> > > > The disparity happens because BIOS doesn't have the ability to program
> > > > ASPM on hotplugged devices.
> > > > 
> > > > So enable ASPM by default for external connected PCIe devices so ASPM
> > > > settings are consitent between preboot and hotplugged.
> > > > 
> > > > On HP Thunderbolt Dock G4, enable ASPM can also fix BadDLLP error:
> > > > pcieport 0000:00:1d.0: AER: Corrected error received: 0000:07:04.0
> > > > pcieport 0000:07:04.0: PCIe Bus Error: severity=Corrected, type=Data Link Layer, (Receiver ID)
> > > > pcieport 0000:07:04.0:   device [8086:0b26] error status/mask=00000080/00002000
> > > > pcieport 0000:07:04.0:    [ 7] BadDLLP
> > > > 
> > > > The root cause is still unclear, but quite likely because the I225 on
> > > > the dock supports PTM, where ASPM timing is precalculated for the PTM.
> > > > 
> > > > Cc: Mario Limonciello <mario.limonciello@amd.com>
> > > > Cc: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=217557
> > > > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> > > > ---
> > > >   drivers/pci/pcie/aspm.c | 4 +++-
> > > >   1 file changed, 3 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > > > index 66d7514ca111..613b0754c9bb 100644
> > > > --- a/drivers/pci/pcie/aspm.c
> > > > +++ b/drivers/pci/pcie/aspm.c
> > > > @@ -119,7 +119,9 @@ static int policy_to_aspm_state(struct pcie_link_state *link)
> > > >                /* Enable Everything */
> > > >                return ASPM_STATE_ALL;
> > > >        case POLICY_DEFAULT:
> > > > -             return link->aspm_default;
> > > > +             return dev_is_removable(&link->downstream->dev) ?
> > > > +                     link->aspm_capable :
> > > > +                     link->aspm_default;
> > >
> > > I'm a little hesitant because dev_is_removable() is a convenient
> > > test that covers the current issue, but it doesn't seem tightly
> > > connected from a PCIe architecture perspective.
> > > 
> > > I think the current model of compile-time ASPM policy selection:
> > > 
> > >    CONFIG_PCIEASPM_DEFAULT          /* BIOS default setting */
> > >    CONFIG_PCIEASPM_PERFORMANCE      /* disable L0s and L1 */
> > >    CONFIG_PCIEASPM_POWERSAVE        /* enable L0s and L1 */
> > >    CONFIG_PCIEASPM_POWER_SUPERSAVE  /* enable L1 substates */
> > > 
> > > is flawed.  As far as I know, there's no technical reason we
> > > have to select this at kernel build-time.  I suspect the
> > > original reason was risk avoidance, i.e., we were worried that
> > > we might expose hardware defects if we enabled ASPM states that
> > > BIOS hadn't already enabled.
> > > 
> > > How do we get out of that model?  We do have sysfs knobs that
> > > should cover all the functionality (set overall policy as above
> > > via /sys/module/pcie_aspm/parameters/policy; set device-level
> > > exceptions via /sys/bus/pci/devices/.../link/*_aspm).
> >
> > Agree. Build-time policy can be obsoleted by boot-time argument.
>
> I agree as well.

This isn't strictly relevant to the current problem, so let's put this
on the back burner for now.

> > > In my opinion, the cleanest solution would be to enable all ASPM
> > > functionality whenever possible and let users disable it if they
> > > need to for performance.  If there are device defects when
> > > something is enabled, deal with it via quirks, as we do for
> > > other PCI features.
> > > 
> > > That feels a little risky, but let's have a conversation about
> > > where we want to go in the long term.  It's good to avoid risk,
> > > but too much avoidance leads to its own complexity and an
> > > inability to change things.
> >
> > I think we should separate the situation into two cases:
> > - When BIOS/system firmware has the ability to program ASPM, honor
> > it.  This applies to most "internal" PCI devices.
> > - When BIOS/system can't program ASPM, enable ASPM for whatever
> > it's capable of. Most notable case is Intel VMD controller, and
> > this patch for devices connected through TBT.
> > 
> > Enabling all ASPM functionality regardless of what's being
> > pre-programmed by BIOS is way too risky.  Disabling ASPM to
> > workaround issues and defects are still quite common among
> > hardware manufacturers.

It sounds like you have actual experience with this :)  Do you have
any concrete examples that we can use as "known breakage"?

This feels like a real problem to me.  There are existing mechanisms
(ACPI_FADT_NO_ASPM and _OSC PCIe cap ownership) the platform can use
to prevent the OS from using ASPM.

If vendors assume that *in addition*, the OS will pay attention to
whatever ASPM configuration BIOS did, that's a major disconnect.  We
don't do anything like that for other PCI features, and I'm not aware
of any restriction like that being documented.

> I think the pragmatic way to approach it is to (essentially) apply
> the policy as BIOS defaults and allow overrides from that.

Do you mean that when enumerating a device (at boot-time or hot-add
time), we would read the current ASPM config but not change it?  And
users could use the sysfs knobs to enable/disable ASPM as desired?
That wouldn't solve the problem Kai-Heng is trying to solve.

Or that we leave ASPM alone during boot-time enumeration, but enable
ASPM when we enumerate hot-added devices?  It doesn't sound right that
a device would be configured differently if present at boot vs
hot-added.

Bjorn