* [PATCH 1/2] PCI: ASPM: Allow OS to configure ASPM where BIOS is incapable of
@ 2024-05-30 8:52 Kai-Heng Feng
2024-05-30 8:52 ` [PATCH 2/2] PCI: vmd: Let OS control ASPM for devices under VMD domain Kai-Heng Feng
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Kai-Heng Feng @ 2024-05-30 8:52 UTC (permalink / raw)
To: bhelgaas
Cc: linux-pci, linux-kernel, nirmal.patel, jonathan.derrick,
ilpo.jarvinen, david.e.box, Kai-Heng Feng
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 */
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH 2/2] PCI: vmd: Let OS control ASPM for devices under VMD domain
2024-05-30 8:52 [PATCH 1/2] PCI: ASPM: Allow OS to configure ASPM where BIOS is incapable of Kai-Heng Feng
@ 2024-05-30 8:52 ` Kai-Heng Feng
2024-08-02 0:04 ` Bjorn Helgaas
2024-07-26 7:22 ` [PATCH 1/2] PCI: ASPM: Allow OS to configure ASPM where BIOS is incapable of Hui Wang
2024-08-16 22:28 ` Bjorn Helgaas
2 siblings, 1 reply; 8+ messages in thread
From: Kai-Heng Feng @ 2024-05-30 8:52 UTC (permalink / raw)
To: bhelgaas
Cc: linux-pci, linux-kernel, nirmal.patel, jonathan.derrick,
ilpo.jarvinen, david.e.box, Kai-Heng Feng
Intel SoC cannot reach lower power states when mapped VMD PCIe bridges
and NVMe devices don't have ASPM configured.
So set aspm_os_control attribute to let OS really enable ASPM for those
devices.
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/controller/vmd.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
index 87b7856f375a..1dbc525c473f 100644
--- a/drivers/pci/controller/vmd.c
+++ b/drivers/pci/controller/vmd.c
@@ -751,6 +751,8 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
return 0;
+ pdev->aspm_os_control = 1;
+
pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
--
2.43.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] PCI: ASPM: Allow OS to configure ASPM where BIOS is incapable of
2024-05-30 8:52 [PATCH 1/2] PCI: ASPM: Allow OS to configure ASPM where BIOS is incapable of Kai-Heng Feng
2024-05-30 8:52 ` [PATCH 2/2] PCI: vmd: Let OS control ASPM for devices under VMD domain Kai-Heng Feng
@ 2024-07-26 7:22 ` Hui Wang
2024-08-16 22:28 ` Bjorn Helgaas
2 siblings, 0 replies; 8+ messages in thread
From: Hui Wang @ 2024-07-26 7:22 UTC (permalink / raw)
To: Kai-Heng Feng, bhelgaas
Cc: linux-pci, linux-kernel, nirmal.patel, jonathan.derrick,
ilpo.jarvinen, david.e.box, AceLan Kao
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 */
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] PCI: vmd: Let OS control ASPM for devices under VMD domain
2024-05-30 8:52 ` [PATCH 2/2] PCI: vmd: Let OS control ASPM for devices under VMD domain Kai-Heng Feng
@ 2024-08-02 0:04 ` Bjorn Helgaas
2024-08-02 1:04 ` Kai-Heng Feng
0 siblings, 1 reply; 8+ messages in thread
From: Bjorn Helgaas @ 2024-08-02 0:04 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: bhelgaas, linux-pci, linux-kernel, nirmal.patel, jonathan.derrick,
ilpo.jarvinen, david.e.box
On Thu, May 30, 2024 at 04:52:27PM +0800, Kai-Heng Feng wrote:
> Intel SoC cannot reach lower power states when mapped VMD PCIe bridges
> and NVMe devices don't have ASPM configured.
>
> So set aspm_os_control attribute to let OS really enable ASPM for those
> devices.
>
> Fixes: f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR")
I assume f492edb40b54 was tested and worked at the time. Is the
implication that newer Intel SoCs have added more requirements for
getting to low power states, since __pci_enable_link_state() would
have warned and done nothing even then?
Or maybe this is a new system that sets ACPI_FADT_NO_ASPM, and
f492edb40b54 was tested on systems that did *not* set
ACPI_FADT_NO_ASPM?
> 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/controller/vmd.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> index 87b7856f375a..1dbc525c473f 100644
> --- a/drivers/pci/controller/vmd.c
> +++ b/drivers/pci/controller/vmd.c
> @@ -751,6 +751,8 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
> return 0;
>
> + pdev->aspm_os_control = 1;
> +
> pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
>
> pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> --
> 2.43.0
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] PCI: vmd: Let OS control ASPM for devices under VMD domain
2024-08-02 0:04 ` Bjorn Helgaas
@ 2024-08-02 1:04 ` Kai-Heng Feng
2024-08-16 6:17 ` Kai-Heng Feng
0 siblings, 1 reply; 8+ messages in thread
From: Kai-Heng Feng @ 2024-08-02 1:04 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: bhelgaas, linux-pci, linux-kernel, nirmal.patel, jonathan.derrick,
ilpo.jarvinen, david.e.box
On Fri, Aug 2, 2024 at 8:04 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> On Thu, May 30, 2024 at 04:52:27PM +0800, Kai-Heng Feng wrote:
> > Intel SoC cannot reach lower power states when mapped VMD PCIe bridges
> > and NVMe devices don't have ASPM configured.
> >
> > So set aspm_os_control attribute to let OS really enable ASPM for those
> > devices.
> >
> > Fixes: f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR")
>
> I assume f492edb40b54 was tested and worked at the time. Is the
> implication that newer Intel SoCs have added more requirements for
> getting to low power states, since __pci_enable_link_state() would
> have warned and done nothing even then?
>
> Or maybe this is a new system that sets ACPI_FADT_NO_ASPM, and
> f492edb40b54 was tested on systems that did *not* set
> ACPI_FADT_NO_ASPM?
I believe it's the case here. Vendors may or may not set
ACPI_FADT_NO_ASPM, so f492edb40b54 works on some systems but not
others.
Kai-Heng
>
> > 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/controller/vmd.c | 2 ++
> > 1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > index 87b7856f375a..1dbc525c473f 100644
> > --- a/drivers/pci/controller/vmd.c
> > +++ b/drivers/pci/controller/vmd.c
> > @@ -751,6 +751,8 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> > if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
> > return 0;
> >
> > + pdev->aspm_os_control = 1;
> > +
> > pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
> >
> > pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> > --
> > 2.43.0
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] PCI: vmd: Let OS control ASPM for devices under VMD domain
2024-08-02 1:04 ` Kai-Heng Feng
@ 2024-08-16 6:17 ` Kai-Heng Feng
0 siblings, 0 replies; 8+ messages in thread
From: Kai-Heng Feng @ 2024-08-16 6:17 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: bhelgaas, linux-pci, linux-kernel, nirmal.patel, jonathan.derrick,
ilpo.jarvinen, david.e.box
Hi Bjorn,
On Fri, Aug 2, 2024 at 9:04 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> On Fri, Aug 2, 2024 at 8:04 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
> >
> > On Thu, May 30, 2024 at 04:52:27PM +0800, Kai-Heng Feng wrote:
> > > Intel SoC cannot reach lower power states when mapped VMD PCIe bridges
> > > and NVMe devices don't have ASPM configured.
> > >
> > > So set aspm_os_control attribute to let OS really enable ASPM for those
> > > devices.
> > >
> > > Fixes: f492edb40b54 ("PCI: vmd: Add quirk to configure PCIe ASPM and LTR")
> >
> > I assume f492edb40b54 was tested and worked at the time. Is the
> > implication that newer Intel SoCs have added more requirements for
> > getting to low power states, since __pci_enable_link_state() would
> > have warned and done nothing even then?
> >
> > Or maybe this is a new system that sets ACPI_FADT_NO_ASPM, and
> > f492edb40b54 was tested on systems that did *not* set
> > ACPI_FADT_NO_ASPM?
>
> I believe it's the case here. Vendors may or may not set
> ACPI_FADT_NO_ASPM, so f492edb40b54 works on some systems but not
> others.
Do you think this is code is ready? Or is there any improvement should be made?
Kai-Heng
>
> Kai-Heng
>
> >
> > > 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/controller/vmd.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/pci/controller/vmd.c b/drivers/pci/controller/vmd.c
> > > index 87b7856f375a..1dbc525c473f 100644
> > > --- a/drivers/pci/controller/vmd.c
> > > +++ b/drivers/pci/controller/vmd.c
> > > @@ -751,6 +751,8 @@ static int vmd_pm_enable_quirk(struct pci_dev *pdev, void *userdata)
> > > if (!(features & VMD_FEAT_BIOS_PM_QUIRK))
> > > return 0;
> > >
> > > + pdev->aspm_os_control = 1;
> > > +
> > > pci_enable_link_state_locked(pdev, PCIE_LINK_STATE_ALL);
> > >
> > > pos = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_LTR);
> > > --
> > > 2.43.0
> > >
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] PCI: ASPM: Allow OS to configure ASPM where BIOS is incapable of
2024-05-30 8:52 [PATCH 1/2] PCI: ASPM: Allow OS to configure ASPM where BIOS is incapable of Kai-Heng Feng
2024-05-30 8:52 ` [PATCH 2/2] PCI: vmd: Let OS control ASPM for devices under VMD domain Kai-Heng Feng
2024-07-26 7:22 ` [PATCH 1/2] PCI: ASPM: Allow OS to configure ASPM where BIOS is incapable of Hui Wang
@ 2024-08-16 22:28 ` Bjorn Helgaas
2024-08-19 5:31 ` Kai-Heng Feng
2 siblings, 1 reply; 8+ messages in thread
From: Bjorn Helgaas @ 2024-08-16 22:28 UTC (permalink / raw)
To: Kai-Heng Feng
Cc: bhelgaas, linux-pci, linux-kernel, nirmal.patel, jonathan.derrick,
ilpo.jarvinen, david.e.box
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
>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] PCI: ASPM: Allow OS to configure ASPM where BIOS is incapable of
2024-08-16 22:28 ` Bjorn Helgaas
@ 2024-08-19 5:31 ` Kai-Heng Feng
0 siblings, 0 replies; 8+ messages in thread
From: Kai-Heng Feng @ 2024-08-19 5:31 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: bhelgaas, linux-pci, linux-kernel, nirmal.patel, jonathan.derrick,
ilpo.jarvinen, david.e.box
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
> >
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-08-19 5:32 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-30 8:52 [PATCH 1/2] PCI: ASPM: Allow OS to configure ASPM where BIOS is incapable of Kai-Heng Feng
2024-05-30 8:52 ` [PATCH 2/2] PCI: vmd: Let OS control ASPM for devices under VMD domain Kai-Heng Feng
2024-08-02 0:04 ` Bjorn Helgaas
2024-08-02 1:04 ` Kai-Heng Feng
2024-08-16 6:17 ` Kai-Heng Feng
2024-07-26 7:22 ` [PATCH 1/2] PCI: ASPM: Allow OS to configure ASPM where BIOS is incapable of Hui Wang
2024-08-16 22:28 ` Bjorn Helgaas
2024-08-19 5:31 ` Kai-Heng Feng
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox