On Sat, Aug 17, 2024 at 6:28 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> 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@xxxxxxxxx/
> > Signed-off-by: Kai-Heng Feng <kai.heng.feng@xxxxxxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx/
>
> 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
> >
