| 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
| ||
|
Message-ID: <20260108154200.1.I7beb66162d35904e7e05830a666de03ed75e6b76@changeid>
Date: Thu, 8 Jan 2026 15:42:01 -0800
From: Brian Norris <briannorris@...omium.org>
To: Bjorn Helgaas <bhelgaas@...gle.com>
Cc: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
Heiner Kallweit <hkallweit1@...il.com>,
linux-pci@...r.kernel.org,
Manivannan Sadhasivam <mani@...nel.org>,
Rajat Jain <rajatja@...gle.com>,
Ajay Agarwal <ajayagarwal@...gle.com>,
linux-kernel@...r.kernel.org,
Brian Norris <briannorris@...omium.org>
Subject: [PATCH] PCI/ASPM: Allow PCI PM L1 substates without ASPM
Configuration of ASPM and PCI PM should be mostly orthogonal, where PCI
PM L1 substates can be enabled/disabled independently of ASPM L1
substates. The main restriction is that one cannot enter L1 substates
without entering L1.0 first. In practice, this means we cannot enable
ASPM L1 substates if ASPM L1.0 is disabled. Notably, PCI PM L1.0 cannot
be disabled [*].
However, we're enforcing unexpected artificial dependencies between PCI
PM and ASPM.
Consider the following sequence on a given device:
1. Initial state:
L1.0, L1.1, L1.2 ASPM enabled
L1.1, L1.2 PCI PM enabled
2. We disable ASPM L1.0 via sysfs
echo 0 > /sys/bus/pci/devices/.../link/l1_aspm
Expected:
L1.0, L1.1, L1.2 ASPM disabled
L1.1, L1.2 PCI PM enabled
Actual:
L1.0, L1.1, L1.2 ASPM disabled
L1.1, L1.2 PCI PM disabled
I suspect this is an accidental misapplication of ASPM requirements to
the PCI PM configuration.
There are similar artificial dependencies:
1. enabling L1.x PCI PM unnecessarily implies enabling L1.0 ASPM
2. pci_{enable,disable}_link_state() have the same bug
Relax these dependencies by:
(a) narrowing "PCIE_LINK_STATE_L1SS" to only mean ASPM L1 substates, and
updating some corresponding naming/constants
(b) applying the restriction (that L1 substates require L1.0 support)
only to the ASPM variant.
---
[*] PCI PM L1.0 does not have a configuration bit to disable it; it is
entered whenever the downstream component leaves D0. (PCIe r6.1 5.3.2)
== Behavioral impact ==
Besides correcting sysfs behavior, this may have some impact on other
drivers. For instance, drivers that call
pci_disable_link_state(pdev, PCIE_LINK_STATE_L1);
will now only disable ASPM L1.{0,1,2}; L1.{1,2} PCI PM may still be
possible. I don't expect this to be a problem, since ASPM is typically
the problematic one -- PCI PM typically only takes effect when a device
is runtime suspended (D3).
== Historical note ==
It seems this mistake originates in commit aeda9adebab8 ("PCI/ASPM:
Configure L1 substate settings"), where PCIE_LINK_STATE_L1SS was
previously named ASPM_STATE_L1SS, although it represented both ASPM and
PCI PM substates. This error was propagated further in the other commits
marked with Fixes.
Fixes: aeda9adebab8 ("PCI/ASPM: Configure L1 substate settings") ++
Fixes: aff5d0552da4 ("PCI/ASPM: Add L1 PM substate support to pci_disable_link_state()") ++
Fixes: 72ea91afbfb0 ("PCI/ASPM: Add sysfs attributes for controlling ASPM link states") ++
Fixes: 80950a546089 ("PCI/ASPM: Set ASPM_STATE_L1 when driver enables L1.1 or L1.2") ++
Signed-off-by: Brian Norris <briannorris@...omium.org>
---
drivers/pci/pcie/aspm.c | 32 ++++++++++++++++----------------
include/linux/pci.h | 2 +-
2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index cedea47a3547..4d1e79886518 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -221,9 +221,8 @@ static_assert(PCIE_LINK_STATE_L0S == (PCIE_LINK_STATE_L0S_UP | PCIE_LINK_STATE_L
PCIE_LINK_STATE_L1_2_PCIPM)
#define PCIE_LINK_STATE_L1_2_MASK (PCIE_LINK_STATE_L1_2 |\
PCIE_LINK_STATE_L1_2_PCIPM)
-#define PCIE_LINK_STATE_L1SS (PCIE_LINK_STATE_L1_1 |\
- PCIE_LINK_STATE_L1_1_PCIPM |\
- PCIE_LINK_STATE_L1_2_MASK)
+#define PCIE_LINK_STATE_L1_SS_ASPM (PCIE_LINK_STATE_L1_1 |\
+ PCIE_LINK_STATE_L1_2)
struct pcie_link_state {
struct pci_dev *pdev; /* Upstream component of the Link */
@@ -902,8 +901,8 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
}
}
-/* Configure the ASPM L1 substates. Caller must disable L1 first. */
-static void pcie_config_aspm_l1ss(struct pcie_link_state *link, u32 state)
+/* Configure the L1 substates. Caller must disable L1 first. */
+static void pcie_config_l1ss(struct pcie_link_state *link, u32 state)
{
u32 val = 0;
struct pci_dev *child = link->downstream, *parent = link->pdev;
@@ -953,9 +952,9 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
/* Enable only the states that were not explicitly disabled */
state &= (link->aspm_capable & ~link->aspm_disable);
- /* Can't enable any substates if L1 is not enabled */
+ /* Can't enable any ASPM substates if L1 is not enabled */
if (!(state & PCIE_LINK_STATE_L1))
- state &= ~PCIE_LINK_STATE_L1SS;
+ state &= ~PCIE_LINK_STATE_L1_SS_ASPM;
/* Spec says both ports must be in D0 before enabling PCI PM substates*/
if (parent->current_state != PCI_D0 || child->current_state != PCI_D0) {
@@ -994,8 +993,9 @@ static void pcie_config_aspm_link(struct pcie_link_state *link, u32 state)
pcie_config_aspm_dev(child, 0);
pcie_config_aspm_dev(parent, 0);
- if (link->aspm_capable & PCIE_LINK_STATE_L1SS)
- pcie_config_aspm_l1ss(link, state);
+ if (link->aspm_capable & (PCIE_LINK_STATE_L1_SS_PCIPM |
+ PCIE_LINK_STATE_L1_SS_ASPM))
+ pcie_config_l1ss(link, state);
pcie_config_aspm_dev(parent, upstream);
list_for_each_entry(child, &linkbus->devices, bus_list)
@@ -1376,9 +1376,9 @@ static u8 pci_calc_aspm_disable_mask(int state)
{
state &= ~PCIE_LINK_STATE_CLKPM;
- /* L1 PM substates require L1 */
+ /* L1 ASPM substates require L1 ASPM */
if (state & PCIE_LINK_STATE_L1)
- state |= PCIE_LINK_STATE_L1SS;
+ state |= PCIE_LINK_STATE_L1_SS_ASPM;
return state;
}
@@ -1387,8 +1387,8 @@ static u8 pci_calc_aspm_enable_mask(int state)
{
state &= ~PCIE_LINK_STATE_CLKPM;
- /* L1 PM substates require L1 */
- if (state & PCIE_LINK_STATE_L1SS)
+ /* L1 ASPM substates require L1 ASPM */
+ if (state & PCIE_LINK_STATE_L1_SS_ASPM)
state |= PCIE_LINK_STATE_L1;
return state;
@@ -1626,13 +1626,13 @@ static ssize_t aspm_attr_store_common(struct device *dev,
if (state_enable) {
link->aspm_disable &= ~state;
- /* need to enable L1 for substates */
- if (state & PCIE_LINK_STATE_L1SS)
+ /* need to enable L1 for ASPM substates */
+ if (state & PCIE_LINK_STATE_L1_SS_ASPM)
link->aspm_disable &= ~PCIE_LINK_STATE_L1;
} else {
link->aspm_disable |= state;
if (state & PCIE_LINK_STATE_L1)
- link->aspm_disable |= PCIE_LINK_STATE_L1SS;
+ link->aspm_disable |= PCIE_LINK_STATE_L1_SS_ASPM;
}
pcie_config_aspm_link(link, policy_to_aspm_state(link));
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 864775651c6f..5781aff12748 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1865,7 +1865,7 @@ static inline int pcie_set_target_speed(struct pci_dev *port,
#endif
#define PCIE_LINK_STATE_L0S (BIT(0) | BIT(1)) /* Upstr/dwnstr L0s */
-#define PCIE_LINK_STATE_L1 BIT(2) /* L1 state */
+#define PCIE_LINK_STATE_L1 BIT(2) /* ASPM L1 state */
#define PCIE_LINK_STATE_L1_1 BIT(3) /* ASPM L1.1 state */
#define PCIE_LINK_STATE_L1_2 BIT(4) /* ASPM L1.2 state */
#define PCIE_LINK_STATE_L1_1_PCIPM BIT(5) /* PCI-PM L1.1 state */
--
2.52.0.457.g6b5491de43-goog
Powered by blists - more mailing lists