1. Patchew
  2. linux
  3. [PATCH 0/2] PCI/ASPM: Allow quirks to avoid L0s and L1
  4. View patch

[PATCH 1/2] PCI/ASPM: Cache Link Capabilities so quirks can override them

Bjorn Helgaas posted 2 patches 1 month, 1 week ago
There is a newer version of this series
[PATCH 1/2] PCI/ASPM: Cache Link Capabilities so quirks can override them
Posted by Bjorn Helgaas 1 month, 1 week ago 
From: Bjorn Helgaas <bhelgaas@google.com>

Cache the PCIe Link Capabilities register in struct pci_dev so quirks can
remove features to avoid hardware defects.  The idea is:

  - set_pcie_port_type() reads PCIe Link Capabilities and caches it in
    dev->lnkcap

  - HEADER quirks can update the cached dev->lnkcap to remove advertised
    features that don't work correctly

  - pcie_aspm_cap_init() relies on dev->lnkcap and ignores any features not
    advertised there

Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
---
 drivers/pci/pcie/aspm.c | 42 ++++++++++++++++++++---------------------
 drivers/pci/probe.c     |  5 ++---
 include/linux/pci.h     |  1 +
 3 files changed, 24 insertions(+), 24 deletions(-)

diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
index 7cc8281e7011..07536891f1f6 100644
--- a/drivers/pci/pcie/aspm.c
+++ b/drivers/pci/pcie/aspm.c
@@ -391,15 +391,13 @@ static void pcie_clkpm_override_default_link_state(struct pcie_link_state *link,
 static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	int capable = 1, enabled = 1;
-	u32 reg32;
 	u16 reg16;
 	struct pci_dev *child;
 	struct pci_bus *linkbus = link->pdev->subordinate;
 
 	/* All functions should have the same cap and state, take the worst */
 	list_for_each_entry(child, &linkbus->devices, bus_list) {
-		pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &reg32);
-		if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
+		if (!(child->lnkcap & PCI_EXP_LNKCAP_CLKPM)) {
 			capable = 0;
 			enabled = 0;
 			break;
@@ -581,7 +579,7 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
 
 static void pcie_aspm_check_latency(struct pci_dev *endpoint)
 {
-	u32 latency, encoding, lnkcap_up, lnkcap_dw;
+	u32 latency, encoding;
 	u32 l1_switch_latency = 0, latency_up_l0s;
 	u32 latency_up_l1, latency_dw_l0s, latency_dw_l1;
 	u32 acceptable_l0s, acceptable_l1;
@@ -606,14 +604,10 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
 		struct pci_dev *dev = pci_function_0(link->pdev->subordinate);
 
 		/* Read direction exit latencies */
-		pcie_capability_read_dword(link->pdev, PCI_EXP_LNKCAP,
-					   &lnkcap_up);
-		pcie_capability_read_dword(dev, PCI_EXP_LNKCAP,
-					   &lnkcap_dw);
-		latency_up_l0s = calc_l0s_latency(lnkcap_up);
-		latency_up_l1 = calc_l1_latency(lnkcap_up);
-		latency_dw_l0s = calc_l0s_latency(lnkcap_dw);
-		latency_dw_l1 = calc_l1_latency(lnkcap_dw);
+		latency_up_l0s = calc_l0s_latency(link->pdev->lnkcap);
+		latency_up_l1 = calc_l1_latency(link->pdev->lnkcap);
+		latency_dw_l0s = calc_l0s_latency(dev->lnkcap);
+		latency_dw_l1 = calc_l1_latency(dev->lnkcap);
 
 		/* Check upstream direction L0s latency */
 		if ((link->aspm_capable & PCIE_LINK_STATE_L0S_UP) &&
@@ -830,7 +824,7 @@ static void pcie_aspm_override_default_link_state(struct pcie_link_state *link)
 static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 {
 	struct pci_dev *child = link->downstream, *parent = link->pdev;
-	u32 parent_lnkcap, child_lnkcap;
+	u32 lnkcap;
 	u16 parent_lnkctl, child_lnkctl;
 	struct pci_bus *linkbus = parent->subordinate;
 
@@ -845,9 +839,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	 * If ASPM not supported, don't mess with the clocks and link,
 	 * bail out now.
 	 */
-	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &parent_lnkcap);
-	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &child_lnkcap);
-	if (!(parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPMS))
+	if (!(parent->lnkcap & child->lnkcap & PCI_EXP_LNKCAP_ASPMS))
 		return;
 
 	/* Configure common clock before checking latencies */
@@ -857,10 +849,18 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	 * Re-read upstream/downstream components' register state after
 	 * clock configuration.  L0s & L1 exit latencies in the otherwise
 	 * read-only Link Capabilities may change depending on common clock
-	 * configuration (PCIe r5.0, sec 7.5.3.6).
+	 * configuration (PCIe r5.0, sec 7.5.3.6).  Update only the exit
+	 * latencies in the cached dev->lnkcap because quirks may have
+	 * removed broken features advertised by the device.
 	 */
-	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &parent_lnkcap);
-	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &child_lnkcap);
+	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &lnkcap);
+	parent->lnkcap &= ~(PCI_EXP_LNKCAP_L0SEL | PCI_EXP_LNKCAP_L1EL);
+	parent->lnkcap |= lnkcap & (PCI_EXP_LNKCAP_L0SEL | PCI_EXP_LNKCAP_L1EL);
+
+	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &lnkcap);
+	child->lnkcap &= ~(PCI_EXP_LNKCAP_L0SEL | PCI_EXP_LNKCAP_L1EL);
+	child->lnkcap |= lnkcap & (PCI_EXP_LNKCAP_L0SEL | PCI_EXP_LNKCAP_L1EL);
+
 	pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &parent_lnkctl);
 	pcie_capability_read_word(child, PCI_EXP_LNKCTL, &child_lnkctl);
 
@@ -880,7 +880,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 	 * given link unless components on both sides of the link each
 	 * support L0s.
 	 */
-	if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
+	if (parent->lnkcap & child->lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
 		link->aspm_support |= PCIE_LINK_STATE_L0S;
 
 	if (child_lnkctl & PCI_EXP_LNKCTL_ASPM_L0S)
@@ -889,7 +889,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
 		link->aspm_enabled |= PCIE_LINK_STATE_L0S_DW;
 
 	/* Setup L1 state */
-	if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L1)
+	if (parent->lnkcap & child->lnkcap & PCI_EXP_LNKCAP_ASPM_L1)
 		link->aspm_support |= PCIE_LINK_STATE_L1;
 
 	if (parent_lnkctl & child_lnkctl & PCI_EXP_LNKCTL_ASPM_L1)
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index c83e75a0ec12..db4635b1ec47 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -1640,7 +1640,6 @@ void set_pcie_port_type(struct pci_dev *pdev)
 {
 	int pos;
 	u16 reg16;
-	u32 reg32;
 	int type;
 	struct pci_dev *parent;
 
@@ -1659,8 +1658,8 @@ void set_pcie_port_type(struct pci_dev *pdev)
 	pci_read_config_dword(pdev, pos + PCI_EXP_DEVCAP, &pdev->devcap);
 	pdev->pcie_mpss = FIELD_GET(PCI_EXP_DEVCAP_PAYLOAD, pdev->devcap);
 
-	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &reg32);
-	if (reg32 & PCI_EXP_LNKCAP_DLLLARC)
+	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &pdev->lnkcap);
+	if (pdev->lnkcap & PCI_EXP_LNKCAP_DLLLARC)
 		pdev->link_active_reporting = 1;
 
 	parent = pci_upstream_bridge(pdev);
diff --git a/include/linux/pci.h b/include/linux/pci.h
index d1fdf81fbe1e..ec4133ae9cae 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -361,6 +361,7 @@ struct pci_dev {
 	struct pci_dev  *rcec;          /* Associated RCEC device */
 #endif
 	u32		devcap;		/* PCIe Device Capabilities */
+	u32		lnkcap;		/* PCIe Link Capabilities */
 	u16		rebar_cap;	/* Resizable BAR capability offset */
 	u8		pcie_cap;	/* PCIe capability offset */
 	u8		msi_cap;	/* MSI capability offset */
-- 
2.43.0
Re: [PATCH 1/2] PCI/ASPM: Cache Link Capabilities so quirks can override them
Posted by Lukas Wunner 1 month, 1 week ago 
On Thu, Nov 06, 2025 at 12:36:38PM -0600, Bjorn Helgaas wrote:
> Cache the PCIe Link Capabilities register in struct pci_dev so quirks can
> remove features to avoid hardware defects.  The idea is:
> 
>   - set_pcie_port_type() reads PCIe Link Capabilities and caches it in
>     dev->lnkcap
> 
>   - HEADER quirks can update the cached dev->lnkcap to remove advertised
>     features that don't work correctly
> 
>   - pcie_aspm_cap_init() relies on dev->lnkcap and ignores any features not
>     advertised there

I realize that memory is cheap, but it still feels a bit wasteful
to cache the entire 32-bit register wholesale.  It contains
reserved bits as of PCIe r7.0, various uninteresting bits and
portions of it are already cached elsewhere and thus now duplicated.
I'm wondering if it would make sense to instead only cache the ASPM bits
that are relevant here?  That's the approach we've followed so far.

You're initializing the link_active_reporting bit from the newly
cached lnkcap register, I'd prefer having a static inline instead
which extracts the bit from the cached register on demand,
thus obviating the need to have a duplicate cached copy of the bit.

pci_set_bus_speed() caches bus->max_bus_speed from the Link
Capabilities register and isn't converted by this patch to use
the cached register.  There are various others, e.g.
get_port_device_capability() in drivers/pci/pcie/portdrv.c
could also get PCI_EXP_LNKCAP_LBNC from the cached lnkcap
register.  Same for pcie_get_supported_speeds().  If the
intention is to convert these in a separate step in v6.19,
it would be good to mention that in the changelog.

Thanks,

Lukas
Re: [PATCH 1/2] PCI/ASPM: Cache Link Capabilities so quirks can override them
Posted by Bjorn Helgaas 1 month, 1 week ago 
On Fri, Nov 07, 2025 at 06:32:14AM +0100, Lukas Wunner wrote:
> On Thu, Nov 06, 2025 at 12:36:38PM -0600, Bjorn Helgaas wrote:
> > Cache the PCIe Link Capabilities register in struct pci_dev so quirks can
> > remove features to avoid hardware defects.  The idea is:
> > 
> >   - set_pcie_port_type() reads PCIe Link Capabilities and caches it in
> >     dev->lnkcap
> > 
> >   - HEADER quirks can update the cached dev->lnkcap to remove advertised
> >     features that don't work correctly
> > 
> >   - pcie_aspm_cap_init() relies on dev->lnkcap and ignores any features not
> >     advertised there
> 
> I realize that memory is cheap, but it still feels a bit wasteful
> to cache the entire 32-bit register wholesale.  It contains
> reserved bits as of PCIe r7.0, various uninteresting bits and
> portions of it are already cached elsewhere and thus now duplicated.
> I'm wondering if it would make sense to instead only cache the ASPM bits
> that are relevant here?  That's the approach we've followed so far.

My first try (which I didn't post) cached only the two bits we need
for this.  It's not awful, and the aspm.c patch was smaller, so maybe
it's the right approach, at least for v6.18.

One thing I didn't like about pci_disable_aspm_cap() (which I know you
said you *did* like :)) is that it adds a layer of indirection.  I
like having PCI_EXP_LNKCAP_ASPM_L0S in the quirk because it's more
directly connected to the spec and the hardware register, and grep
works better for code readers.

But if we only cache the ASPM cap bits, we would need
pci_disable_aspm_cap() to manage converting PCI_EXP_LNKCAP_ASPM_L0S or
PCIE_LINK_STATE_L0S to the right place.

(A bit of a tangent, but I've never liked the PCIE_LINK_STATE_* bits
because they look like they ought to be register bits, but they're
not.  I think the code would be improved overall if we could remove
them.)

> You're initializing the link_active_reporting bit from the newly
> cached lnkcap register, I'd prefer having a static inline instead
> which extracts the bit from the cached register on demand,
> thus obviating the need to have a duplicate cached copy of the bit.
> 
> pci_set_bus_speed() caches bus->max_bus_speed from the Link
> Capabilities register and isn't converted by this patch to use
> the cached register.  There are various others, e.g.
> get_port_device_capability() in drivers/pci/pcie/portdrv.c
> could also get PCI_EXP_LNKCAP_LBNC from the cached lnkcap
> register.  Same for pcie_get_supported_speeds().  If the
> intention is to convert these in a separate step in v6.19,
> it would be good to mention that in the changelog.

I agree with all of that, and there are several other PCI_EXP_LNKCAP
reads that could be replaced, but that would have to be for v6.19.

Bjorn
Re: [PATCH 1/2] PCI/ASPM: Cache Link Capabilities so quirks can override them
Posted by Shawn Lin 1 month, 1 week ago 
在 2025/11/07 星期五 2:36, Bjorn Helgaas 写道:
> From: Bjorn Helgaas <bhelgaas@google.com>
> 
> Cache the PCIe Link Capabilities register in struct pci_dev so quirks can
> remove features to avoid hardware defects.  The idea is:
> 
>    - set_pcie_port_type() reads PCIe Link Capabilities and caches it in
>      dev->lnkcap
> 
>    - HEADER quirks can update the cached dev->lnkcap to remove advertised
>      features that don't work correctly
> 
>    - pcie_aspm_cap_init() relies on dev->lnkcap and ignores any features not
>      advertised there
> 

Quick test with a NVMe shows it works.

Before this patch,  lspci -vvv dumps:

  LnkCap: Port #0, Speed 16GT/s, Width x4, ASPM L1, Exit Latency L1 <64us
          ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp+
  LnkCtl: ASPM L1 Enabled; RCB 64 bytes, LnkDisable- CommClk+


Capabilities: [21c v1] L1 PM Substates
          L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ 
L1_PM_Substates+
                    PortCommonModeRestoreTime=10us PortTPowerOnTime=10us
          L1SubCtl1: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
                     T_CommonMode=0us LTR1.2_Threshold=26016ns

After this patch + a local quirk patch like your patch 2, it shows:

  LnkCap: Port #0, Speed 16GT/s, Width x4, ASPM L1, Exit Latency L1 <64us
          ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp+
  LnkCtl: ASPM Disabled; RCB 64 bytes, LnkDisable- CommClk-

Capabilities: [21c v1] L1 PM Substates
           L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+ 
L1_PM_Substates+
                     PortCommonModeRestoreTime=10us PortTPowerOnTime=10us
           L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1-
                      T_CommonMode=0us LTR1.2_Threshold=0ns



One things I noticed is CommClk in LnkCtl is changed. Per the spec,

"A value of 0b indicates that this component and the component at the
opposite end of this Link are operating with asynchronous reference
clock.

Components utilize this Common Clock Configuration information to report
the correct L0s and L1 Exit Latencies. After changing the value in this
bit in both components on a Link, software must trigger the Link to
retrain by writing a 1b to the Retrain Link bit of the Downstream Port."

Obviously my NVMe and RC are operating with common reference clk. So
CommClk- looks wrong to me. And should we also perform Retrain Link if
we really wants to change it?


> Signed-off-by: Bjorn Helgaas <bhelgaas@google.com>
> ---
>   drivers/pci/pcie/aspm.c | 42 ++++++++++++++++++++---------------------
>   drivers/pci/probe.c     |  5 ++---
>   include/linux/pci.h     |  1 +
>   3 files changed, 24 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/pci/pcie/aspm.c b/drivers/pci/pcie/aspm.c
> index 7cc8281e7011..07536891f1f6 100644
> --- a/drivers/pci/pcie/aspm.c
> +++ b/drivers/pci/pcie/aspm.c
> @@ -391,15 +391,13 @@ static void pcie_clkpm_override_default_link_state(struct pcie_link_state *link,
>   static void pcie_clkpm_cap_init(struct pcie_link_state *link, int blacklist)
>   {
>   	int capable = 1, enabled = 1;
> -	u32 reg32;
>   	u16 reg16;
>   	struct pci_dev *child;
>   	struct pci_bus *linkbus = link->pdev->subordinate;
>   
>   	/* All functions should have the same cap and state, take the worst */
>   	list_for_each_entry(child, &linkbus->devices, bus_list) {
> -		pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &reg32);
> -		if (!(reg32 & PCI_EXP_LNKCAP_CLKPM)) {
> +		if (!(child->lnkcap & PCI_EXP_LNKCAP_CLKPM)) {
>   			capable = 0;
>   			enabled = 0;
>   			break;
> @@ -581,7 +579,7 @@ static void encode_l12_threshold(u32 threshold_us, u32 *scale, u32 *value)
>   
>   static void pcie_aspm_check_latency(struct pci_dev *endpoint)
>   {
> -	u32 latency, encoding, lnkcap_up, lnkcap_dw;
> +	u32 latency, encoding;
>   	u32 l1_switch_latency = 0, latency_up_l0s;
>   	u32 latency_up_l1, latency_dw_l0s, latency_dw_l1;
>   	u32 acceptable_l0s, acceptable_l1;
> @@ -606,14 +604,10 @@ static void pcie_aspm_check_latency(struct pci_dev *endpoint)
>   		struct pci_dev *dev = pci_function_0(link->pdev->subordinate);
>   
>   		/* Read direction exit latencies */
> -		pcie_capability_read_dword(link->pdev, PCI_EXP_LNKCAP,
> -					   &lnkcap_up);
> -		pcie_capability_read_dword(dev, PCI_EXP_LNKCAP,
> -					   &lnkcap_dw);
> -		latency_up_l0s = calc_l0s_latency(lnkcap_up);
> -		latency_up_l1 = calc_l1_latency(lnkcap_up);
> -		latency_dw_l0s = calc_l0s_latency(lnkcap_dw);
> -		latency_dw_l1 = calc_l1_latency(lnkcap_dw);
> +		latency_up_l0s = calc_l0s_latency(link->pdev->lnkcap);
> +		latency_up_l1 = calc_l1_latency(link->pdev->lnkcap);
> +		latency_dw_l0s = calc_l0s_latency(dev->lnkcap);
> +		latency_dw_l1 = calc_l1_latency(dev->lnkcap);
>   
>   		/* Check upstream direction L0s latency */
>   		if ((link->aspm_capable & PCIE_LINK_STATE_L0S_UP) &&
> @@ -830,7 +824,7 @@ static void pcie_aspm_override_default_link_state(struct pcie_link_state *link)
>   static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>   {
>   	struct pci_dev *child = link->downstream, *parent = link->pdev;
> -	u32 parent_lnkcap, child_lnkcap;
> +	u32 lnkcap;
>   	u16 parent_lnkctl, child_lnkctl;
>   	struct pci_bus *linkbus = parent->subordinate;
>   
> @@ -845,9 +839,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>   	 * If ASPM not supported, don't mess with the clocks and link,
>   	 * bail out now.
>   	 */
> -	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &parent_lnkcap);
> -	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &child_lnkcap);
> -	if (!(parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPMS))
> +	if (!(parent->lnkcap & child->lnkcap & PCI_EXP_LNKCAP_ASPMS))
>   		return;
>   
>   	/* Configure common clock before checking latencies */
> @@ -857,10 +849,18 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>   	 * Re-read upstream/downstream components' register state after
>   	 * clock configuration.  L0s & L1 exit latencies in the otherwise
>   	 * read-only Link Capabilities may change depending on common clock
> -	 * configuration (PCIe r5.0, sec 7.5.3.6).
> +	 * configuration (PCIe r5.0, sec 7.5.3.6).  Update only the exit
> +	 * latencies in the cached dev->lnkcap because quirks may have
> +	 * removed broken features advertised by the device.
>   	 */
> -	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &parent_lnkcap);
> -	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &child_lnkcap);
> +	pcie_capability_read_dword(parent, PCI_EXP_LNKCAP, &lnkcap);
> +	parent->lnkcap &= ~(PCI_EXP_LNKCAP_L0SEL | PCI_EXP_LNKCAP_L1EL);
> +	parent->lnkcap |= lnkcap & (PCI_EXP_LNKCAP_L0SEL | PCI_EXP_LNKCAP_L1EL);
> +
> +	pcie_capability_read_dword(child, PCI_EXP_LNKCAP, &lnkcap);
> +	child->lnkcap &= ~(PCI_EXP_LNKCAP_L0SEL | PCI_EXP_LNKCAP_L1EL);
> +	child->lnkcap |= lnkcap & (PCI_EXP_LNKCAP_L0SEL | PCI_EXP_LNKCAP_L1EL);
> +
>   	pcie_capability_read_word(parent, PCI_EXP_LNKCTL, &parent_lnkctl);
>   	pcie_capability_read_word(child, PCI_EXP_LNKCTL, &child_lnkctl);
>   
> @@ -880,7 +880,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>   	 * given link unless components on both sides of the link each
>   	 * support L0s.
>   	 */
> -	if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
> +	if (parent->lnkcap & child->lnkcap & PCI_EXP_LNKCAP_ASPM_L0S)
>   		link->aspm_support |= PCIE_LINK_STATE_L0S;
>   
>   	if (child_lnkctl & PCI_EXP_LNKCTL_ASPM_L0S)
> @@ -889,7 +889,7 @@ static void pcie_aspm_cap_init(struct pcie_link_state *link, int blacklist)
>   		link->aspm_enabled |= PCIE_LINK_STATE_L0S_DW;
>   
>   	/* Setup L1 state */
> -	if (parent_lnkcap & child_lnkcap & PCI_EXP_LNKCAP_ASPM_L1)
> +	if (parent->lnkcap & child->lnkcap & PCI_EXP_LNKCAP_ASPM_L1)
>   		link->aspm_support |= PCIE_LINK_STATE_L1;
>   
>   	if (parent_lnkctl & child_lnkctl & PCI_EXP_LNKCTL_ASPM_L1)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index c83e75a0ec12..db4635b1ec47 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1640,7 +1640,6 @@ void set_pcie_port_type(struct pci_dev *pdev)
>   {
>   	int pos;
>   	u16 reg16;
> -	u32 reg32;
>   	int type;
>   	struct pci_dev *parent;
>   
> @@ -1659,8 +1658,8 @@ void set_pcie_port_type(struct pci_dev *pdev)
>   	pci_read_config_dword(pdev, pos + PCI_EXP_DEVCAP, &pdev->devcap);
>   	pdev->pcie_mpss = FIELD_GET(PCI_EXP_DEVCAP_PAYLOAD, pdev->devcap);
>   
> -	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &reg32);
> -	if (reg32 & PCI_EXP_LNKCAP_DLLLARC)
> +	pcie_capability_read_dword(pdev, PCI_EXP_LNKCAP, &pdev->lnkcap);
> +	if (pdev->lnkcap & PCI_EXP_LNKCAP_DLLLARC)
>   		pdev->link_active_reporting = 1;
>   
>   	parent = pci_upstream_bridge(pdev);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index d1fdf81fbe1e..ec4133ae9cae 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -361,6 +361,7 @@ struct pci_dev {
>   	struct pci_dev  *rcec;          /* Associated RCEC device */
>   #endif
>   	u32		devcap;		/* PCIe Device Capabilities */
> +	u32		lnkcap;		/* PCIe Link Capabilities */
>   	u16		rebar_cap;	/* Resizable BAR capability offset */
>   	u8		pcie_cap;	/* PCIe capability offset */
>   	u8		msi_cap;	/* MSI capability offset */
Re: [PATCH 1/2] PCI/ASPM: Cache Link Capabilities so quirks can override them
Posted by Manivannan Sadhasivam 1 month, 1 week ago 
On Fri, Nov 07, 2025 at 09:17:09AM +0800, Shawn Lin wrote:
> 在 2025/11/07 星期五 2:36, Bjorn Helgaas 写道:
> > From: Bjorn Helgaas <bhelgaas@google.com>
> > 
> > Cache the PCIe Link Capabilities register in struct pci_dev so quirks can
> > remove features to avoid hardware defects.  The idea is:
> > 
> >    - set_pcie_port_type() reads PCIe Link Capabilities and caches it in
> >      dev->lnkcap
> > 
> >    - HEADER quirks can update the cached dev->lnkcap to remove advertised
> >      features that don't work correctly
> > 
> >    - pcie_aspm_cap_init() relies on dev->lnkcap and ignores any features not
> >      advertised there
> > 
> 
> Quick test with a NVMe shows it works.
> 
> Before this patch,  lspci -vvv dumps:
> 
>  LnkCap: Port #0, Speed 16GT/s, Width x4, ASPM L1, Exit Latency L1 <64us
>          ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp+
>  LnkCtl: ASPM L1 Enabled; RCB 64 bytes, LnkDisable- CommClk+
> 
> 
> Capabilities: [21c v1] L1 PM Substates
>          L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
> L1_PM_Substates+
>                    PortCommonModeRestoreTime=10us PortTPowerOnTime=10us
>          L1SubCtl1: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
>                     T_CommonMode=0us LTR1.2_Threshold=26016ns
> 
> After this patch + a local quirk patch like your patch 2, it shows:
> 
>  LnkCap: Port #0, Speed 16GT/s, Width x4, ASPM L1, Exit Latency L1 <64us
>          ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp+
>  LnkCtl: ASPM Disabled; RCB 64 bytes, LnkDisable- CommClk-
> 
> Capabilities: [21c v1] L1 PM Substates
>           L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
> L1_PM_Substates+
>                     PortCommonModeRestoreTime=10us PortTPowerOnTime=10us
>           L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1-
>                      T_CommonMode=0us LTR1.2_Threshold=0ns
> 
> 
> 
> One things I noticed is CommClk in LnkCtl is changed.

That's not because of this series, but because of your quirk that disables L0s
and L1. Common Clock Configuration happens only when ASPM is enabled, if it is
disabled, PCI core will not configure it (the value remains untouched). That's
why it was enabled before your quirk and disabled afterwards.

This bit is also only used to report the L0s and L1 Exit latencies by the
devices.

- Mani

-- 
மணிவண்ணன் சதாசிவம்
Re: [PATCH 1/2] PCI/ASPM: Cache Link Capabilities so quirks can override them
Posted by Shawn Lin 1 month, 1 week ago 
在 2025/11/07 星期五 14:03, Manivannan Sadhasivam 写道:
> On Fri, Nov 07, 2025 at 09:17:09AM +0800, Shawn Lin wrote:
>> 在 2025/11/07 星期五 2:36, Bjorn Helgaas 写道:
>>> From: Bjorn Helgaas <bhelgaas@google.com>
>>>
>>> Cache the PCIe Link Capabilities register in struct pci_dev so quirks can
>>> remove features to avoid hardware defects.  The idea is:
>>>
>>>     - set_pcie_port_type() reads PCIe Link Capabilities and caches it in
>>>       dev->lnkcap
>>>
>>>     - HEADER quirks can update the cached dev->lnkcap to remove advertised
>>>       features that don't work correctly
>>>
>>>     - pcie_aspm_cap_init() relies on dev->lnkcap and ignores any features not
>>>       advertised there
>>>
>>
>> Quick test with a NVMe shows it works.
>>
>> Before this patch,  lspci -vvv dumps:
>>
>>   LnkCap: Port #0, Speed 16GT/s, Width x4, ASPM L1, Exit Latency L1 <64us
>>           ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp+
>>   LnkCtl: ASPM L1 Enabled; RCB 64 bytes, LnkDisable- CommClk+
>>
>>
>> Capabilities: [21c v1] L1 PM Substates
>>           L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
>> L1_PM_Substates+
>>                     PortCommonModeRestoreTime=10us PortTPowerOnTime=10us
>>           L1SubCtl1: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
>>                      T_CommonMode=0us LTR1.2_Threshold=26016ns
>>
>> After this patch + a local quirk patch like your patch 2, it shows:
>>
>>   LnkCap: Port #0, Speed 16GT/s, Width x4, ASPM L1, Exit Latency L1 <64us
>>           ClockPM+ Surprise- LLActRep- BwNot- ASPMOptComp+
>>   LnkCtl: ASPM Disabled; RCB 64 bytes, LnkDisable- CommClk-
>>
>> Capabilities: [21c v1] L1 PM Substates
>>            L1SubCap: PCI-PM_L1.2+ PCI-PM_L1.1+ ASPM_L1.2+ ASPM_L1.1+
>> L1_PM_Substates+
>>                      PortCommonModeRestoreTime=10us PortTPowerOnTime=10us
>>            L1SubCtl1: PCI-PM_L1.2- PCI-PM_L1.1- ASPM_L1.2- ASPM_L1.1-
>>                       T_CommonMode=0us LTR1.2_Threshold=0ns
>>
>>
>>
>> One things I noticed is CommClk in LnkCtl is changed.
> 
> That's not because of this series, but because of your quirk that disables L0s
> and L1. Common Clock Configuration happens only when ASPM is enabled, if it is
> disabled, PCI core will not configure it (the value remains untouched). That's
> why it was enabled before your quirk and disabled afterwards.
> 

Thanks for the detailed explanation, I have no more questions now.

> This bit is also only used to report the L0s and L1 Exit latencies by the
> devices.
> 
> - Mani
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.