Message-ID: <CAAd53p7bdcJNH_XNNvSjon_=S_q-xaUBuctxXGabb7BKKh9eZA@mail.gmail.com>
Date: Mon, 19 Aug 2024 13:31:55 +0800
From: Kai-Heng Feng <kai.heng.feng@...onical.com>
To: Bjorn Helgaas <helgaas@...nel.org>
Cc: bhelgaas@...gle.com, linux-pci@...r.kernel.org, 
	linux-kernel@...r.kernel.org, nirmal.patel@...ux.intel.com, 
	jonathan.derrick@...ux.dev, ilpo.jarvinen@...ux.intel.com, 
	david.e.box@...ux.intel.com
Subject: Re: [PATCH 1/2] PCI: ASPM: Allow OS to configure ASPM where BIOS is
 incapable of

On Sat, Aug 17, 2024 at 6:28 AM Bjorn Helgaas <helgaas@...nel.org> wrote:
>
> On Thu, May 30, 2024 at 04:52:26PM +0800, Kai-Heng Feng wrote:
> > Since commit f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM
> > and LTR"), ASPM is configured for NVMe devices enabled in VMD domain.
> >
> > However, that doesn't cover the case when FADT has ACPI_FADT_NO_ASPM
> > set.
> >
> > So add a new attribute to bypass aspm_disabled so OS can configure ASPM.
> >
> > Fixes: f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR")
> > Link: https://lore.kernel.org/linux-pm/218aa81f-9c6-5929-578d-8dc15f83dd48@panix.com/
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@...onical.com>
> > ---
> >  drivers/pci/pcie/aspm.c | 8 ++++++--
> >  include/linux/pci.h     | 1 +
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> > index cee2365e54b8..e719605857b1 100644
> > --- a/drivers/pci/pcie/aspm.c
> > +++ b/drivers/pci/pcie/aspm.c
> > @@ -1416,8 +1416,12 @@ static int __pci_enable_link_state(struct pci_dev *pdev, int state, bool locked)
> >        * the _OSC method), we can't honor that request.
> >        */
> >       if (aspm_disabled) {
> > -             pci_warn(pdev, "can't override BIOS ASPM; OS doesn't have ASPM control\n");
> > -             return -EPERM;
> > +             if (aspm_support_enabled && pdev->aspm_os_control)
> > +                     pci_info(pdev, "BIOS can't program ASPM, let OS control it\n");
> > +             else {
> > +                     pci_warn(pdev, "can't override BIOS ASPM; OS doesn't have ASPM control\n");
> > +                     return -EPERM;
>
> 1) I dislike having this VMD-specific special case in the generic
> code.

This can be generalized to "FDAT doesn't want OS to touch ASPM but
exceptions should be made" like external PCIe devices connected via
Thunderbolt:
https://lore.kernel.org/linux-pci/20230615070421.1704133-1-kai.heng.feng@canonical.com/

>
> 2) I think the "BIOS can't program ASPM ..." message is a little bit
> misleading.  We're making the assumption that BIOS doesn't know about
> devices below the VMD bridge, but we really don't know that.  BIOS
> *could* have a VMD driver, and it could configure ASPM below the VMD.
> We're just assuming that it doesn't.
>
> It's also a little bit too verbose -- I think we get this message for
> *every* device below VMD?  Maybe the vmd driver could print something
> about ignoring the ACPI FADT "PCIe ASPM Controls" bit once per VMD?
> Then it's clearly connected to something firmware folks know about.

Will do in next revision.

>
> 3) The code ends up looking like this:
>
>   if (aspm_disabled) {
>     if (aspm_support_enabled && pdev->aspm_os_control)
>       pci_info(pdev, "BIOS can't program ASPM, let OS control it\n");
>     else {
>       pci_warn(pdev, "can't override BIOS ASPM; OS doesn't have ASPM control\n");
>       return -EPERM;
>     }
>   }
>
> and I think it's confusing to check "aspm_support_enabled" and
> "pdev->aspm_os_control" after we've already decided that ASPM is
> sort of disabled by "aspm_disabled".
>
> Plus, we're left with questions about all the *other* tests of
> "aspm_disabled" in pcie_aspm_sanity_check(),
> pcie_aspm_pm_state_change(), pcie_aspm_powersave_config_link(),
> __pci_disable_link_state(), etc.  Why do they *not* need this change?

They all need similar change, yes.

>
> And what about pcie_aspm_init_link_state()?  Why doesn't *it* pay
> attention to "aspm_disabled"?  It's all very complicated.

It's already very complicated by aspm_disabled, aspm_force and
aspm_support_enabled.
We should define the relation between _OSC/FADT/driver/user override, etc.

Probably some helper functions to determine the ASPM status, instead
of checking aspm_disabled and aspm_support_enabled directly.

>
> This is similar in some ways to native_aer, native_pme, etc., which we
> negotiate with _OSC.  I wonder if we could make something similar for
> this, since it's another case where we want to make something specific
> to a host bridge instead of global.

I thinks it's possible by adding a new flag, but how should
pcie_aspm_init_link_state() check it? Adding a new parameter or find
the host bridge to check the new flag?

>
> > +             }
> >       }
> >
> >       if (!locked)
> > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > index fb004fd4e889..58cbd4bea320 100644
> > --- a/include/linux/pci.h
> > +++ b/include/linux/pci.h
> > @@ -467,6 +467,7 @@ struct pci_dev {
> >       unsigned int    no_command_memory:1;    /* No PCI_COMMAND_MEMORY */
> >       unsigned int    rom_bar_overlap:1;      /* ROM BAR disable broken */
> >       unsigned int    rom_attr_enabled:1;     /* Display of ROM attribute enabled? */
> > +     unsigned int    aspm_os_control:1;      /* Display of ROM attribute enabled? */
>
> Comment is wrong (but I hope we can avoid a per-device bit anyway).

Will make it right in next revision.

Kai-Heng

>
> >       pci_dev_flags_t dev_flags;
> >       atomic_t        enable_cnt;     /* pci_enable_device has been called */
> >
> > --
> > 2.43.0
> >

Powered by blists - more mailing lists