linux-kernel - Re: [PATCH] PCI/ASPM: Should not report ASPM support to BIOS if FADT indicates ASPM is unsupported

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAAd53p7g2Md73=UU6Rp-TZkksc+H02KAX58bWCzsgQ__VwvJ+g@mail.gmail.com>
Date:   Thu, 14 Jul 2022 11:20:26 +0800
From:   Kai-Heng Feng <kai.heng.feng@...onical.com>
To:     Bjorn Helgaas <helgaas@...nel.org>
Cc:     Manyi Li <limanyi@...ontech.com>, bhelgaas@...gle.com,
        refactormyself@...il.com, kw@...ux.com, rajatja@...gle.com,
        linux-pci@...r.kernel.org, linux-kernel@...r.kernel.org,
        Vidya Sagar <vidyas@...dia.com>,
        Matthew Garrett <mjg59@...f.ucam.org>
Subject: Re: [PATCH] PCI/ASPM: Should not report ASPM support to BIOS if FADT
 indicates ASPM is unsupported

[+Cc Matthew]

On Thu, Jul 14, 2022 at 2:28 AM Bjorn Helgaas <helgaas@...nel.org> wrote:
>
> [+cc Kai-Heng, Vidya, who also have ASPM patches in flight]
>
> On Wed, Jul 13, 2022 at 07:26:12PM +0800, Manyi Li wrote:
> > Startup log of ASUSTeK X456UJ Notebook show:
> > [    0.130563] ACPI FADT declares the system doesn't support PCIe ASPM, so disable it
> > [   48.092472] pcieport 0000:00:1c.5: PCIe Bus Error: severity=Corrected, type=Physical Layer, (Receiver ID)
> > [   48.092479] pcieport 0000:00:1c.5:   device [8086:9d15] error status/mask=00000001/00002000
> > [   48.092481] pcieport 0000:00:1c.5:    [ 0] RxErr
> > [   48.092490] pcieport 0000:00:1c.5: AER: Corrected error received: 0000:00:1c.5
> > [   48.092504] pcieport 0000:00:1c.5: AER: can't find device of ID00e5
> > [   48.092506] pcieport 0000:00:1c.5: AER: Corrected error received: 0000:00:1c.5
>
> Can you elaborate on the connection between the FADT ASPM bit and the
> AER logs above?
>
> What problem are we solving here?  A single corrected error being
> logged?  An infinite stream of errors?  A device that doesn't work at
> all?

Agree, what's the real symptom of the issue?

>
> We don't need the dmesg timestamps unless they contribute to
> understanding the problem.  I don't think they do in this case.

According to commit 387d37577fdd ("PCI: Don't clear ASPM bits when the
FADT declares it's unsupported"), the bit means "just use the ASPM
bits handed over by BIOS".

However, I do wonder why both drivers/pci/pci-acpi.c and
drivers/acpi/pci_root.c are doing the ACPI_FADT_NO_ASPM check, maybe
one of them should be removed?

>
> > Signed-off-by: Manyi Li <limanyi@...ontech.com>
> > ---
> >  drivers/pci/pcie/aspm.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index a96b7424c9bc..b173d3c75ae7 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -1359,6 +1359,7 @@ void pcie_no_aspm(void)
> >       if (!aspm_force) {
> >               aspm_policy = POLICY_DEFAULT;
> >               aspm_disabled = 1;
> > +             aspm_support_enabled = false;
>
> This makes pcie_no_aspm() work the same as booting with
> "pcie_aspm=off".  That might be reasonable.
>
> I do wonder why we need both "aspm_disabled" and
> "aspm_support_enabled".  And I wonder why we need to set "aspm_policy"
> when we're disabling ASPM.  But those aren't really connected to your
> change here.

>From what I can understand "aspm_disabled" means "don't touch ASPM
left by BIOS", and "aspm_support_enabled" means "whether ASPM is
disabled via command line".
There seems to be some overlaps though.

Kai-Heng

>
> >       }
> >  }
> >
> > --
> > 2.20.1
> >
> >
> >