Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Message-ID: <eb42db34f456c3a157cc574893fd73d877b85b75.camel@linux.intel.com>
Subject: Re: [PATCH v2 1/6] PCI/ASPM: Add locked helper for enabling link
 state
From:   "David E. Box" <david.e.box@linux.intel.com>
Reply-To: david.e.box@linux.intel.com
To:     Bjorn Helgaas <helgaas@kernel.org>
Cc:     Kai-Heng Feng <kai.heng.feng@canonical.com>,
        Johan Hovold <johan+linaro@kernel.org>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Krzysztof =?UTF-8?Q?Wilczy=C5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        Manivannan Sadhasivam <mani@kernel.org>,
        Rob Herring <robh@kernel.org>,
        Nirmal Patel <nirmal.patel@linux.intel.com>,
        Jonathan Derrick <jonathan.derrick@linux.dev>,
        linux-arm-msm@vger.kernel.org, linux-pci@vger.kernel.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org,
        Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Date:   Wed, 13 Dec 2023 15:39:24 -0800
In-Reply-To: <20231213204512.GA1056289@bhelgaas>
References: <20231213204512.GA1056289@bhelgaas>
Organization: David E. Box
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4-0ubuntu2 
MIME-Version: 1.0
Precedence: bulk

On Wed, 2023-12-13 at 14:45 -0600, Bjorn Helgaas wrote:
> On Wed, Dec 13, 2023 at 11:48:41AM -0800, David E. Box wrote:
> > On Tue, 2023-12-12 at 15:27 -0600, Bjorn Helgaas wrote:
> > > On Tue, Dec 12, 2023 at 11:48:27AM +0800, Kai-Heng Feng wrote:
> > > > On Fri, Dec 8, 2023 at 4:47=E2=80=AFAM Bjorn Helgaas <helgaas@kerne=
l.org> wrote:
> > > > ...
> > >=20
> > > > > I hope we can obsolete this whole idea someday.=C2=A0 Using pci_w=
alk_bus()
> > > > > in qcom and vmd to enable ASPM is an ugly hack to work around thi=
s
> > > > > weird idea that "the OS isn't allowed to enable more ASPM states =
than
> > > > > the BIOS did because the BIOS might have left ASPM disabled becau=
se it
> > > > > knows about hardware issues."=C2=A0 More history at
> > > > > https://lore.kernel.org/linux-pci/20230615070421.1704133-1-kai.he=
ng.feng@canonical.com/T/#u
> > > > >=20
> > > > > I think we need to get to a point where Linux enables all support=
ed
> > > > > ASPM features by default.=C2=A0 If we really think x86 BIOS assum=
es an
> > > > > implicit contract that the OS will never enable ASPM more
> > > > > aggressively, we might need some kind of arch quirk for that.
> > > >=20
> > > > The reality is that PC ODM toggles ASPM to workaround hardware
> > > > defects, assuming that OS will honor what's set by the BIOS.
> > > > If ASPM gets enabled for all devices, many devices will break.
> > >=20
> > > That's why I mentioned some kind of arch quirk.=C2=A0 Maybe we're for=
ced to
> > > do that for x86, for instance.=C2=A0 But even that is a stop-gap.
> > >=20
> > > The idea that the BIOS ASPM config is some kind of handoff protocol i=
s
> > > really unsupportable.
> >=20
> > To be clear, you are not talking about a situation where
> > ACPI_FADT_NO_ASPM or _OSC PCIe disallow OS ASPM control, right?
> > Everyone agrees that this should be honored? The question is what to
> > do by default when the OS is not restricted by these mechanisms?
>=20
> Exactly.=C2=A0 The OS should respect ACPI_FADT_NO_ASPM and _OSC.
>=20
> I think there are a couple exceptions where we want to disable ASPM
> even if the platform said the OS shouldn't touch ASPM at all, but
> that's a special case.
>=20
> > Reading the mentioned thread, I too think that using the BIOS config
> > as the default would be the safest option, but only to avoid
> > breaking systems, not because of an implied contract between the
> > BIOS and OS. However, enabling all capable ASPM features is the
> > ideal option. If the OS isn't limited by ACPI_FADT_NO_ASPM or _OSC
> > PCIe, then ASPM enabling is fully under its control.=C2=A0 If this
> > doesn't work for some devices then they are broken and need a quirk.
>=20
> Agreed.=C2=A0 It may not be practical to identify such devices, so we may
> need a broader arch-based and/or date-based quirk.
>=20
> I'd be shocked if Windows treated the BIOS config as a "do not exceed
> this" situation, so my secret hope is that some of these "broken"
> devices are really caused by defects in the Linux ASPM support or the
> driver, and that we can fix them if we find out about them.
>=20
> But I have no details about any of these alleged broken devices, so
> it's hard to make progress on them.=C2=A0=C2=A0

I don't have a sense of the scope either. But I could see BIOS not enabling
features that would provide no added power savings benefit. We use ASPM to
manage package power. There are Intel devices that certainly don't require =
L1SS
for the SoC to achieve the deepest power savings. L1 alone is fine for them=
. I
don't know what the test coveragae is for unenabled features. I've sent the=
se
questions to our BIOS folks.

> Maybe we should log a debug note
> if the device advertises ASPM support that BIOS didn't enable.

Good idea.

David

>=20
> Bjorn