| Message ID | 20240530085227.91168-1-kai.heng.feng@canonical.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] PCI: ASPM: Allow OS to configure ASPM where BIOS is incapable of | expand |
I tested the patchset on a Dell machine (the testing result is positive). Without the patchset, the ASPM is disabled on the NVME controller: LnkCtl: ASPM Disabled; RCB 64 bytes, Disabled- CommClk+ ExtSynch- ClockPM- AutWidDis- BWInt- AutBWInt- After applying the patchset, the ASPM is enabled on the NVME controller: LnkCtl: ASPM L1 Enabled; RCB 64 bytes, Disabled- CommClk+ ExtSynch- ClockPM+ AutWidDis- BWInt- AutBWInt- Regards, Hui. On 5/30/24 16:52, 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@canonical.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; > + } > } > > 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? */ > pci_dev_flags_t dev_flags; > atomic_t enable_cnt; /* pci_enable_device has been called */ >
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@canonical.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. 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. 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? And what about pcie_aspm_init_link_state()? Why doesn't *it* pay attention to "aspm_disabled"? It's all very complicated. 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. > + } > } > > 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). > pci_dev_flags_t dev_flags; > atomic_t enable_cnt; /* pci_enable_device has been called */ > > -- > 2.43.0 >
On Sat, Aug 17, 2024 at 6:28 AM Bjorn Helgaas <helgaas@kernel.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@canonical.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 > >
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; + } } 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? */ pci_dev_flags_t dev_flags; atomic_t enable_cnt; /* pci_enable_device has been called */
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@canonical.com> --- drivers/pci/pcie/aspm.c | 8 ++++++-- include/linux/pci.h | 1 + 2 files changed, 7 insertions(+), 2 deletions(-)