[PATCH V3 0/5] PCI: Re-evaluate DEV3 14-Bit Tag Requester Enable on link mode changes

[PATCH V3 0/5] PCI: Re-evaluate DEV3 14-Bit Tag Requester Enable on link mode changes

[Vidya Sagar]

DEV3_CTL.14-Bit Tag Requester Enable is only meaningful while the link
operates in Flit Mode.  In Non-Flit Mode the upper tag bits are not
transmitted on the wire, so a requester that still has it set emits
requests whose completions it can no longer match: the completer does
answer, but the completion comes back with the upper tag bits zero.  The
requester therefore logs an Unexpected Completion for it and, once the
timer expires, a Completion Timeout as well.  The completer side needs no
handling, since a completer only reflects the Tag field of the request it
answers and the spec accordingly defines no completer enable.

This happens today on the very first config read a Port issues after the
link below it comes back in a different mode, and there are two ways to
get there: a reset of any flavour, and a hot swap that replaces a card
whose link ran in Flit Mode with one that trains in Non-Flit Mode.  Two
things are missing in the core:

  - DEV3_CTL is not part of the state the core saves and restores, so its
    contents are lost across a reset even for the devices that do get
    saved and restored, and

  - nothing re-evaluates the 14-Bit Tag Requester Enable when the link
    mode changes.  This matters most for the Port: it is the requester for
    the config read the core issues to the device below as soon as the
    link is back, but it is not itself saved and restored on these paths,
    so it keeps its stale enable.

The series:

  1/5 adds the DEV3_CAP.14-Bit Tag Requester Supported and
      DEV3_CTL.14-Bit Tag Requester Enable definitions, both bit 2 of
      their respective register.

  2/5 moves __pcie_update_link_speed() out of line, which 4/5 needs.  Pure
      refactor.

  3/5 saves DEV3_CTL and, on restore, drops 14-Bit Tag Requester Enable
      from the value written to hardware when the live LNKSTA2.Flit_Mode
      and DEV3_STA.Segment Captured say Flit Mode is gone.  This covers the
      devices reached via pci_dev_restore().  No other DEV3_CTL bit is
      modified.

  4/5 adds pci_bridge_refresh_14bit_tag(), which fixes the bridge itself
      and then walks its subordinate bus, and calls it from
      pci_bridge_wait_for_secondary_bus() (before the first downstream
      config read after SBR, DPC release, AER bus reset, slot reset or a
      bridge D3cold->D0 resume) and from __pcie_update_link_speed() (which
      covers retrain, bwctrl and hotplug paths that never reach
      pci_bridge_wait_for_secondary_bus()).

  5/5 calls it from pciehp_check_link_status() as well.  A surprise hot
      swap resets nothing and the new card has no saved state, so the only
      thing standing between the Port and a failed enumeration is a fixup
      before pci_bus_check_dev() probes the card.

3/5 and 4/5 are independent of each other; each is useful on its own.

One deliberate asymmetry: the helper only ever clears the enable, it never
sets it.  That matches how the core treats 10-Bit Tags, which it also never
enables on its own, and it means a Port that has been through a Non-Flit
link does not get 14-Bit Tags back when a Flit Mode card is plugged in
later.  Restoring that automatically needs a policy for checking every
completer below the Port, which seems better left to a separate series.
The saved DEV3_CTL value is deliberately left untouched in 3/5 so that the
originally programmed value remains available if we do want it back.

Tested on an arm64 platform with a Root Port whose link comes back in
Non-Flit Mode after a reset.  Resetting the subordinate bus from sysfs
(reset_subordinate, i.e. pci_reset_bridge() -> pci_try_reset_slot())
previously left the Root Port with 14-Bit Tag Requester Enable set, and
the first config read to the device below failed with a Completion Timeout
plus an Unexpected Completion.  With this series the Root Port is fixed up
first,

  pcieport 0002:80:00.0: cleared 14-Bit Tag Requester Enable: flit mode no longer active (DEV3_STA=0x00000008)

and the reset completes.  Note that DEV3_STA.Segment Captured was still
set at that point while LNKSTA2.Flit_Mode had already dropped, which is
why both are consulted.

V3:
* Add patch-5 to handle hot-swap scenario

V2:
* Split the V1 monolithic patch into 4 patches

Vidya Sagar (5):
  PCI: Add DEV3 14-Bit Tag Requester register definitions
  PCI: Move __pcie_update_link_speed() out of line
  PCI: Save and restore the Device 3 Control register
  PCI: Clear stale 14-Bit Tag Requester Enable when a link leaves Flit
    Mode
  PCI: pciehp: Clear stale 14-Bit Tag Requester Enable on hot add

 drivers/pci/hotplug/pciehp_hpc.c |   8 ++
 drivers/pci/pci.c                | 211 +++++++++++++++++++++++++++++++
 drivers/pci/pci.h                |  17 +--
 drivers/pci/probe.c              |  36 ++++++
 include/uapi/linux/pci_regs.h    |   2 +
 5 files changed, 262 insertions(+), 12 deletions(-)

-- 
2.43.0

[PATCH V3 1/5] PCI: Add DEV3 14-Bit Tag Requester register definitions

[Vidya Sagar]

The Device 3 Extended Capability advertises 14-Bit Tag Requester support
in bit 2 of the Device 3 Capabilities register and carries the matching
enable in bit 2 of the Device 3 Control register.  Only the register
offsets and DEV3_STA.Segment Captured are defined today.

Add both 14-Bit Tag Requester bit definitions.  There is deliberately no
completer counterpart: a completer reflects the Tag field of the request
it answers, so the spec defines a 14-Bit Tag Completer Supported bit but
no completer enable, and software has nothing to program there.

No functional change.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V3:
* No change

V2:
* New patch as a result of splitting the V1 monolithic patch

 include/uapi/linux/pci_regs.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
index facaa324bd86..b2ba5c2642f0 100644
--- a/include/uapi/linux/pci_regs.h
+++ b/include/uapi/linux/pci_regs.h
@@ -1254,7 +1254,9 @@
 
 /* Device 3 Extended Capability */
 #define PCI_DEV3_CAP		0x04	/* Device 3 Capabilities Register */
+#define  PCI_DEV3_CAP_14BIT_TAG_REQ	0x00000004 /* 14-Bit Tag Requester Supported */
 #define PCI_DEV3_CTL		0x08	/* Device 3 Control Register */
+#define  PCI_DEV3_CTL_14BIT_TAG_REQ_EN	0x00000004 /* 14-Bit Tag Requester Enable */
 #define PCI_DEV3_STA		0x0c	/* Device 3 Status Register */
 #define  PCI_DEV3_STA_SEGMENT	0x8	/* Segment Captured (end-to-end flit-mode detected) */
 
-- 
2.43.0

Re: [PATCH V3 1/5] PCI: Add DEV3 14-Bit Tag Requester register definitions

[sashiko-bot]

> The Device 3 Extended Capability advertises 14-Bit Tag Requester support
> in bit 2 of the Device 3 Capabilities register and carries the matching
> enable in bit 2 of the Device 3 Control register.  Only the register
> offsets and DEV3_STA.Segment Captured are defined today.
> 
> Add both 14-Bit Tag Requester bit definitions.  There is deliberately no
> completer counterpart: a completer reflects the Tag field of the request
> it answers, so the spec defines a 14-Bit Tag Completer Supported bit but
> no completer enable, and software has nothing to program there.
> 
> No functional change.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814201621.2281245-1-vidyas@nvidia.com?part=1

[PATCH V3 2/5] PCI: Move __pcie_update_link_speed() out of line

[Vidya Sagar]

__pcie_update_link_speed() records the current speed and Flit Mode state
of a bus and emits the link event trace point.  It is a static inline in
drivers/pci/pci.h, but it has only two callers, pcie_update_link_speed()
in drivers/pci/probe.c and pciehp_check_link_status(), and it is never on
a hot path, so inlining buys nothing.

A subsequent change makes it re-evaluate the Device 3 Control register of
the bridge, which is more of the PCI core than belongs in a header.  Move
the body to drivers/pci/probe.c and leave only a declaration behind.

No functional change intended.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V3:
* No change

V2:
* New patch as a result of splitting the V1 monolithic patch

 drivers/pci/pci.h   | 15 +++------------
 drivers/pci/probe.c | 13 +++++++++++++
 2 files changed, 16 insertions(+), 12 deletions(-)

diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 68b5fb675d1c..9f55868adac8 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -676,18 +676,9 @@ enum pcie_link_change_reason {
 	PCIE_HOTPLUG,
 };
 
-static inline void __pcie_update_link_speed(struct pci_bus *bus,
-					    enum pcie_link_change_reason reason,
-					    u16 linksta, u16 linksta2)
-{
-	bus->cur_bus_speed = pcie_link_speed[linksta & PCI_EXP_LNKSTA_CLS];
-	bus->flit_mode = (linksta2 & PCI_EXP_LNKSTA2_FLIT) ? 1 : 0;
-
-	trace_pcie_link_event(bus,
-			     reason,
-			     FIELD_GET(PCI_EXP_LNKSTA_NLW, linksta),
-			     linksta & PCI_EXP_LNKSTA_LINK_STATUS_MASK);
-}
+void __pcie_update_link_speed(struct pci_bus *bus,
+			      enum pcie_link_change_reason reason,
+			      u16 linksta, u16 linksta2);
 
 void pcie_update_link_speed(struct pci_bus *bus, enum pcie_link_change_reason reason);
 
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index dd0abbc63e18..60dd1efe9abb 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -827,6 +827,19 @@ const char *pci_speed_string(enum pci_bus_speed speed)
 }
 EXPORT_SYMBOL_GPL(pci_speed_string);
 
+void __pcie_update_link_speed(struct pci_bus *bus,
+			      enum pcie_link_change_reason reason,
+			      u16 linksta, u16 linksta2)
+{
+	bus->cur_bus_speed = pcie_link_speed[linksta & PCI_EXP_LNKSTA_CLS];
+	bus->flit_mode = (linksta2 & PCI_EXP_LNKSTA2_FLIT) ? 1 : 0;
+
+	trace_pcie_link_event(bus,
+			      reason,
+			      FIELD_GET(PCI_EXP_LNKSTA_NLW, linksta),
+			      linksta & PCI_EXP_LNKSTA_LINK_STATUS_MASK);
+}
+
 void pcie_update_link_speed(struct pci_bus *bus,
 			    enum pcie_link_change_reason reason)
 {
-- 
2.43.0

Re: [PATCH V3 2/5] PCI: Move __pcie_update_link_speed() out of line

[sashiko-bot]

> __pcie_update_link_speed() records the current speed and Flit Mode state
> of a bus and emits the link event trace point.  It is a static inline in
> drivers/pci/pci.h, but it has only two callers, pcie_update_link_speed()
> in drivers/pci/probe.c and pciehp_check_link_status(), and it is never on
> a hot path, so inlining buys nothing.
> 
> A subsequent change makes it re-evaluate the Device 3 Control register of
> the bridge, which is more of the PCI core than belongs in a header.  Move
> the body to drivers/pci/probe.c and leave only a declaration behind.
> 
> No functional change intended.
> 
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814201621.2281245-1-vidyas@nvidia.com?part=2

[PATCH V3 3/5] PCI: Save and restore the Device 3 Control register

[Vidya Sagar]

The Device 3 Extended Capability carries the 14-Bit Tag Requester Enable
bit, which platform firmware may have programmed before the PCI core takes
over.  The core neither saves nor restores DEV3_CTL, so its contents are
lost on every path that goes through pci_save_state() and
pci_restore_state(), e.g. a Secondary Bus Reset, a slot reset or a D3cold
resume.

Restoring the saved value verbatim is not correct either.  14-Bit Tag
Requester Enable is only meaningful while the link operates in Flit Mode;
in Non-Flit Mode the upper tag bits are not transmitted on the wire, so a
requester that still has it set emits TLPs whose completions it can no
longer match.  That shows up as a Completion Timeout together with an
Unexpected Completion on the very first transaction after the reset.  A
link that comes back in Non-Flit Mode must therefore come back with
14-Bit Tag Requester Enable cleared.  The completer side needs no such
handling: a completer reflects the Tag field of the request it answers, so
the spec defines no completer enable to fix up.

Allocate a save buffer in pci_dev3_init() for every device that exposes
the Device 3 Extended Capability and save DEV3_CTL from pci_save_state().
DEV3_STA needs no save buffer of its own because all of its fields
(Initial Link Width, Segment Captured and Remote L0p Supported) are
read-only status reported by hardware.

On restore, sanitize the saved value first: if the device advertises
14-Bit Tag Requester support but Flit Mode is no longer active, as
determined from the live LNKSTA2.Flit_Mode and DEV3_STA.Segment Captured,
drop PCI_DEV3_CTL_14BIT_TAG_REQ_EN before writing DEV3_CTL back and
refresh dev->fm_enabled and bus->flit_mode to match what the hardware now
reports.  Devices without 14-Bit Tag Requester support, and every other
DEV3_CTL bit, are restored unchanged.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V3:
* Modified to preserve the original DEV3_CTL value

V2:
* New patch as a result of splitting the V1 monolithic patch

 drivers/pci/pci.c   | 84 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/pci/probe.c | 12 +++++++
 2 files changed, 96 insertions(+)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ff6d5d059b21..c59329365ad6 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1703,6 +1703,85 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
 	pcie_capability_write_word(dev, PCI_EXP_SLTCTL2, cap[i++]);
 }
 
+static int pci_save_dev3_state(struct pci_dev *dev)
+{
+	struct pci_cap_saved_state *save_state;
+	u32 *cap;
+	int pos;
+
+	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DEV3);
+	if (!pos)
+		return 0;
+
+	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DEV3);
+	if (!save_state)
+		return -ENOMEM;
+
+	cap = (u32 *)&save_state->cap.data[0];
+	pci_read_config_dword(dev, pos + PCI_DEV3_CTL, &cap[0]);
+
+	return 0;
+}
+
+static void pci_restore_dev3_state(struct pci_dev *dev)
+{
+	struct pci_cap_saved_state *save_state;
+	u32 *cap, val, dev3_cap, dev3_sta;
+	u16 lnksta2 = 0;
+	bool flit_now;
+	int pos;
+
+	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DEV3);
+	if (!pos)
+		return;
+
+	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_DEV3);
+	if (!save_state)
+		return;
+
+	cap = (u32 *)&save_state->cap.data[0];
+	val = cap[0];
+
+	/*
+	 * DEV3_CTL.14-Bit Tag Requester Enable is only meaningful in flit
+	 * mode.  On devices that advertise 14-Bit Tag Requester support,
+	 * sanitize the saved value before writing it back, so that callers
+	 * that issue further TLPs through this device after restore see a
+	 * coherent enable state.  Every other bit of DEV3_CTL (DMWr
+	 * Requester Enable, DMWr Egress Blocking, L0p Enable, Target Link
+	 * Width and any future addition) is written back unchanged.
+	 *
+	 * Only the value written to hardware is adjusted.  The save buffer
+	 * keeps what was saved, so the decision is taken afresh on every
+	 * restore and the originally programmed value is not lost.
+	 */
+	pci_read_config_dword(dev, pos + PCI_DEV3_CAP, &dev3_cap);
+	if (dev3_cap & PCI_DEV3_CAP_14BIT_TAG_REQ) {
+		/*
+		 * Check both LNKSTA2.Flit_Mode (link-level) and
+		 * DEV3_STA.Segment Captured (end-to-end); both must be
+		 * active for 14-bit tags.  Refresh bus->flit_mode and
+		 * dev->fm_enabled in lock-step.
+		 */
+		pci_read_config_dword(dev, pos + PCI_DEV3_STA, &dev3_sta);
+		dev->fm_enabled = !!(dev3_sta & PCI_DEV3_STA_SEGMENT);
+
+		pcie_capability_read_word(dev, PCI_EXP_LNKSTA2, &lnksta2);
+		flit_now = !!(lnksta2 & PCI_EXP_LNKSTA2_FLIT);
+		if (dev->bus)
+			dev->bus->flit_mode = flit_now;
+
+		if ((!dev->fm_enabled || !flit_now) &&
+		    (val & PCI_DEV3_CTL_14BIT_TAG_REQ_EN)) {
+			val &= ~PCI_DEV3_CTL_14BIT_TAG_REQ_EN;
+			pci_info(dev, "clearing 14-Bit Tag Requester Enable: flit mode no longer active (LNKSTA2=%#06x, DEV3_STA=%#010x)\n",
+				 lnksta2, dev3_sta);
+		}
+	}
+
+	pci_write_config_dword(dev, pos + PCI_DEV3_CTL, val);
+}
+
 static int pci_save_pcix_state(struct pci_dev *dev)
 {
 	int pos;
@@ -1759,6 +1838,10 @@ int pci_save_state(struct pci_dev *dev)
 	if (i != 0)
 		return i;
 
+	i = pci_save_dev3_state(dev);
+	if (i != 0)
+		return i;
+
 	i = pci_save_pcix_state(dev);
 	if (i != 0)
 		return i;
@@ -1826,6 +1909,7 @@ static void pci_restore_config_space(struct pci_dev *pdev)
 void pci_restore_state(struct pci_dev *dev)
 {
 	pci_restore_pcie_state(dev);
+	pci_restore_dev3_state(dev);
 	pci_restore_pasid_state(dev);
 	pci_restore_pri_state(dev);
 	pci_restore_ats_state(dev);
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 60dd1efe9abb..810114029ee0 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2334,11 +2334,23 @@ static void pci_dev3_init(struct pci_dev *pdev)
 {
 	u16 cap = pci_find_ext_capability(pdev, PCI_EXT_CAP_ID_DEV3);
 	u32 val = 0;
+	int err;
 
 	if (!cap)
 		return;
 	pci_read_config_dword(pdev, cap + PCI_DEV3_STA, &val);
 	pdev->fm_enabled = !!(val & PCI_DEV3_STA_SEGMENT);
+
+	/*
+	 * Save buffer for DEV3_CTL only.  Every field in DEV3_STA is
+	 * read-only status reported by hardware, so there is nothing there
+	 * to restore.
+	 */
+	err = pci_add_ext_cap_save_buffer(pdev, PCI_EXT_CAP_ID_DEV3,
+					  sizeof(u32));
+	if (err)
+		pci_warn(pdev,
+			 "unable to preallocate Device 3 save buffer\n");
 }
 
 /**
-- 
2.43.0

Re: [PATCH V3 3/5] PCI: Save and restore the Device 3 Control register

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Topology Corruption in pci_restore_dev3_state().
- [High] Data Race and Concurrency violation in pci_restore_dev3_state() writing to bus bitfields.
--

--- Patch [3]: [PATCH V3 3/5] PCI: Save and restore the Device 3 Control register ---
commit 308945c2aad690ba20eb4cf961ec76f5391413d4
Author: Vidya Sagar <vidyas@nvidia.com>

PCI: Save and restore the Device 3 Control register

This commit saves and restores the Device 3 Control register across PCI power
management transitions to preserve the 14-Bit Tag Requester Enable state. It
dynamically sanitizes the saved value upon restore to clear the enable bit if
the hardware link is no longer operating in Flit Mode.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 77b17b13ee615..b881163febb6a 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c

[ ... ]

> @@ -1703,6 +1703,85 @@ static void pci_restore_pcie_state(struct pci_dev *dev)

[ ... ]

> +static void pci_restore_dev3_state(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state;
> +	u32 *cap, val, dev3_cap, dev3_sta;
> +	u16 lnksta2 = 0;
> +	bool flit_now;
> +	int pos;

[ ... ]

> +		pci_read_config_dword(dev, pos + PCI_DEV3_STA, &dev3_sta);
> +		dev->fm_enabled = !!(dev3_sta & PCI_DEV3_STA_SEGMENT);
> +
> +		pcie_capability_read_word(dev, PCI_EXP_LNKSTA2, &lnksta2);
> +		flit_now = !!(lnksta2 & PCI_EXP_LNKSTA2_FLIT);
> +		if (dev->bus)
> +			dev->bus->flit_mode = flit_now;

[Severity: High]
For bridge devices like Downstream Ports or Root Ports, does writing to
dev->bus modify the upstream bus link status instead of the downstream link?

When a Root Port resumes, dev->bus points to the shared Root Bus. Reading
LNKSTA2 (which reflects the downstream link) and applying it to dev->bus
could misrepresent the shared upstream topology state.

Would dev->subordinate be the correct target for bridge downstream link
status?

[Severity: High]
Could this assignment also cause a data race during concurrent resumes?

The PCI power management subsystem can use asynchronous resume. If multiple
sibling endpoints on the same primary bus, or multiple root ports on the
root bus, execute pci_restore_dev3_state() concurrently, they will write to
the shared dev->bus structure simultaneously.

Since flit_mode is a bitfield in struct pci_bus, writing to it without a
lock from an asynchronous context could result in an unsynchronized
read-modify-write operation, potentially corrupting adjacent bitfields.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814201621.2281245-1-vidyas@nvidia.com?part=3

[PATCH V3 4/5] PCI: Clear stale 14-Bit Tag Requester Enable when a link leaves Flit Mode

[Vidya Sagar]

Sanitizing DEV3_CTL on restore covers the devices that go through
pci_dev_restore(), but those are not the only devices whose 14-Bit Tag
Requester Enable can go stale, and for the case that hurts most they are
not even the right device.

For a reset mediated by a bridge, i.e. a Secondary Bus Reset, a DPC
trigger and release, an AER bus reset, a slot reset or a bridge
D3cold->D0 resume, the bridge itself is neither reset nor saved and
restored.  The bridge is however the requester for the config read the
core issues to the device below as soon as the link is back up.  If the
link returns in Non-Flit Mode while the bridge still has 14-Bit Tag
Requester Enable set, that very first config read fails with a Completion
Timeout and an Unexpected Completion, because the device does answer but
with a completion the bridge can no longer match.  Devices below the
bridge that are reached only by paths which never call pci_dev_restore(),
such as a DPC release or a hotplug link status update, have the same
problem.

Add pci_bridge_refresh_14bit_tag().  It re-reads the bridge's own
LNKSTA2.Flit_Mode, clears 14-Bit Tag Requester Enable in the bridge's
DEV3_CTL if Flit Mode is gone, refreshes bus->flit_mode and then walks the
subordinate bus and does the same for every device that advertises 14-Bit
Tag Requester support.  Fixing up the bridge only requires access to the
bridge's own config space on the primary bus, so it works even while the
link below is still unusable.  Hardware is touched only when the enable is
set and Flit Mode is no longer active, so the helper is idempotent and
does nothing on the common path.

Call it from the two choke points that together cover every
kernel-visible link mode change:

  - pci_bridge_wait_for_secondary_bus(), right after the link is known to
    be up and before pci_dev_wait() issues the first config read
    downstream.  This covers SBR via pci_bridge_secondary_bus_reset(),
    DPC release via dpc_reset_link(), AER bus reset via
    pci_bus_error_reset(), slot reset via pciehp_reset_slot() and
    D3cold->D0 resume via pci_pm_bridge_power_up_actions().

  - __pcie_update_link_speed(), where bus->flit_mode is authoritatively
    updated whenever the kernel observes a link change: initial
    enumeration, pcie_retrain_link() for an ASPM common clock
    configuration or a target speed change, the bwctrl IRQ for autonomous
    hardware speed changes and the pciehp link status check.  None of
    these go through pci_bridge_wait_for_secondary_bus().

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V3:
* No change

V2:
* New patch as a result of splitting the V1 monolithic patch

 drivers/pci/pci.c   | 131 +++++++++++++++++++++++++++++++++++++++++++-
 drivers/pci/pci.h   |   2 +
 drivers/pci/probe.c |  11 ++++
 3 files changed, 142 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index c59329365ad6..09265173cf07 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1754,12 +1754,22 @@ static void pci_restore_dev3_state(struct pci_dev *dev)
 	 * Only the value written to hardware is adjusted.  The save buffer
 	 * keeps what was saved, so the decision is taken afresh on every
 	 * restore and the originally programmed value is not lost.
+	 *
+	 * Note: bridge-side and link-event paths are handled separately by
+	 * pci_bridge_refresh_14bit_tag(), which runs from
+	 * pci_bridge_wait_for_secondary_bus() and __pcie_update_link_speed()
+	 * and clears the bit directly in hardware as soon as the link is
+	 * observed to leave flit mode.  This function's responsibility is
+	 * narrowed to the save-buffer-restore path.
 	 */
 	pci_read_config_dword(dev, pos + PCI_DEV3_CAP, &dev3_cap);
 	if (dev3_cap & PCI_DEV3_CAP_14BIT_TAG_REQ) {
 		/*
-		 * Check both LNKSTA2.Flit_Mode (link-level) and
-		 * DEV3_STA.Segment Captured (end-to-end); both must be
+		 * Re-check link state here too: pci_restore_state() may run
+		 * on paths where the link has changed mode but
+		 * pci_bridge_refresh_14bit_tag() has not yet been called for
+		 * this device.  Check both LNKSTA2.Flit_Mode (link-level)
+		 * and DEV3_STA.Segment Captured (end-to-end); both must be
 		 * active for 14-bit tags.  Refresh bus->flit_mode and
 		 * dev->fm_enabled in lock-step.
 		 */
@@ -1782,6 +1792,107 @@ static void pci_restore_dev3_state(struct pci_dev *dev)
 	pci_write_config_dword(dev, pos + PCI_DEV3_CTL, val);
 }
 
+/*
+ * Clear DEV3_CTL.14-Bit Tag Requester Enable on @dev if flit mode is no
+ * longer active.  Touches only @dev's own config space, so it is safe to
+ * call on a bridge before the first downstream TLP is issued after a
+ * reset.
+ *
+ * 14-Bit Tag Requester Enable is only meaningful in flit mode.  If the
+ * link came back as non-flit (e.g. after SBR, DPC, slot reset, or D3cold
+ * resume), a requester that still has it set will emit TLPs whose
+ * completions it cannot match, producing Completion Timeout plus
+ * Unexpected Completion on the first transaction.
+ */
+static void __pci_dev_clear_stale_14bit_tag(struct pci_dev *dev, bool flit_now)
+{
+	u32 dev3_cap, dev3_ctl, dev3_sta;
+	int pos;
+
+	if (!pci_is_pcie(dev))
+		return;
+
+	pos = pci_find_ext_capability(dev, PCI_EXT_CAP_ID_DEV3);
+	if (!pos)
+		return;
+
+	pci_read_config_dword(dev, pos + PCI_DEV3_CAP, &dev3_cap);
+	if (!(dev3_cap & PCI_DEV3_CAP_14BIT_TAG_REQ))
+		return;
+
+	pci_read_config_dword(dev, pos + PCI_DEV3_STA, &dev3_sta);
+	dev->fm_enabled = !!(dev3_sta & PCI_DEV3_STA_SEGMENT);
+
+	if (flit_now && dev->fm_enabled)
+		return;
+
+	pci_read_config_dword(dev, pos + PCI_DEV3_CTL, &dev3_ctl);
+	if (!(dev3_ctl & PCI_DEV3_CTL_14BIT_TAG_REQ_EN))
+		return;
+
+	dev3_ctl &= ~PCI_DEV3_CTL_14BIT_TAG_REQ_EN;
+	pci_write_config_dword(dev, pos + PCI_DEV3_CTL, dev3_ctl);
+	pci_info(dev, "cleared 14-Bit Tag Requester Enable: flit mode no longer active (DEV3_STA=%#010x)\n",
+		 dev3_sta);
+}
+
+/**
+ * pci_bridge_refresh_14bit_tag - Drop a stale 14-Bit Tag enable across a link
+ * @bridge: PCIe bridge whose link may have changed mode
+ *
+ * Re-evaluate the bridge's own DEV3_CTL.14-Bit Tag Requester Enable
+ * against the live LNKSTA2.Flit_Mode, then walk the bridge's subordinate
+ * bus and do the same for every device that advertises 14-Bit Tag
+ * Requester support.  Also refresh bus->flit_mode so the rest of the PCI
+ * core sees a consistent view of the link.
+ *
+ * Called from every kernel-visible link state change site:
+ *   - pci_bridge_wait_for_secondary_bus() (covers SBR, DPC release, slot
+ *     reset, AER bus reset, bridge D3cold->D0 resume).
+ *   - __pcie_update_link_speed() (covers manual retrain, bwctrl IRQ,
+ *     hotplug link status check, initial enumeration).
+ *
+ * Safe to call repeatedly; only writes hardware when the enable bit is
+ * set and flit mode is no longer active.
+ */
+void pci_bridge_refresh_14bit_tag(struct pci_dev *bridge)
+{
+	struct pci_bus *bus;
+	struct pci_dev *child;
+	u16 lnksta2 = 0;
+	bool flit_now;
+
+	if (!bridge || !pci_is_pcie(bridge))
+		return;
+
+	pcie_capability_read_word(bridge, PCI_EXP_LNKSTA2, &lnksta2);
+	flit_now = !!(lnksta2 & PCI_EXP_LNKSTA2_FLIT);
+
+	/*
+	 * Fix the bridge itself first.  The bridge is the requester for
+	 * outbound config/MMIO TLPs, so a stale 14-Bit Tag Requester Enable
+	 * here is what produces the post-reset Completion Timeout /
+	 * Unexpected Completion failure.
+	 */
+	__pci_dev_clear_stale_14bit_tag(bridge, flit_now);
+
+	bus = bridge->subordinate;
+	if (!bus)
+		return;
+
+	bus->flit_mode = flit_now;
+
+	/*
+	 * Walk the secondary bus.  pci_restore_dev3_state() only fires on
+	 * paths that go through pci_dev_restore(); DPC release, hotplug
+	 * link status updates, and similar paths do not.  Fix those too.
+	 */
+	down_read(&pci_bus_sem);
+	list_for_each_entry(child, &bus->devices, bus_list)
+		__pci_dev_clear_stale_14bit_tag(child, flit_now);
+	up_read(&pci_bus_sem);
+}
+
 static int pci_save_pcix_state(struct pci_dev *dev)
 {
 	int pos;
@@ -4874,6 +4985,14 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
 		pci_dbg(dev, "waiting %d ms for downstream link\n", delay);
 		msleep(delay);
 
+		/*
+		 * The link has had a chance to come back; refresh the
+		 * bridge's (and subtree's) DEV3_CTL.14-Bit Tag Requester
+		 * Enable against the live LNKSTA2.Flit_Mode before we issue
+		 * the first config TLP to the child.
+		 */
+		pci_bridge_refresh_14bit_tag(dev);
+
 		if (!pci_dev_wait(child, reset_type, PCI_RESET_WAIT - delay))
 			return 0;
 
@@ -4901,6 +5020,14 @@ int pci_bridge_wait_for_secondary_bus(struct pci_dev *dev, char *reset_type)
 		return -ENOTTY;
 	}
 
+	/*
+	 * Link is up.  Refresh the bridge's (and subtree's)
+	 * DEV3_CTL.14-Bit Tag Requester Enable against the live
+	 * LNKSTA2.Flit_Mode before we issue the first config TLP to the
+	 * child below.
+	 */
+	pci_bridge_refresh_14bit_tag(dev);
+
 	return pci_dev_wait(child, reset_type,
 			    PCIE_RESET_READY_POLL_MS - delay);
 }
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 9f55868adac8..0298e250e254 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -682,6 +682,8 @@ void __pcie_update_link_speed(struct pci_bus *bus,
 
 void pcie_update_link_speed(struct pci_bus *bus, enum pcie_link_change_reason reason);
 
+void pci_bridge_refresh_14bit_tag(struct pci_dev *bridge);
+
 /* Single Root I/O Virtualization */
 struct pci_sriov {
 	int		pos;		/* Capability position */
diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 810114029ee0..d13094baf487 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -838,6 +838,17 @@ void __pcie_update_link_speed(struct pci_bus *bus,
 			      reason,
 			      FIELD_GET(PCI_EXP_LNKSTA_NLW, linksta),
 			      linksta & PCI_EXP_LNKSTA_LINK_STATUS_MASK);
+
+	/*
+	 * Re-evaluate DEV3_CTL.14-Bit Tag Requester Enable on this bridge
+	 * and its subordinate bus.  Any time bus->flit_mode is updated, the
+	 * link has just changed state; if flit mode is no longer active, the
+	 * bridge and downstream devices must drop that enable before
+	 * further TLPs are issued, or the requester (the bridge) will tag
+	 * config/MMIO requests with 14-bit tags that the completer can no
+	 * longer echo back in non-flit mode.
+	 */
+	pci_bridge_refresh_14bit_tag(bus->self);
 }
 
 void pcie_update_link_speed(struct pci_bus *bus,
-- 
2.43.0

Re: [PATCH V3 4/5] PCI: Clear stale 14-Bit Tag Requester Enable when a link leaves Flit Mode

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Unprotected access to `bridge->subordinate` can result in a Use-After-Free of a `struct pci_bus`.
- [High] The patch introduces a call to `down_read(&pci_bus_sem)` inside `pci_bridge_refresh_14bit_tag()`, which can be reached from a hard interrupt handler, leading to a 'sleeping function called from invalid context' kernel panic.
- [High] `pci_bridge_refresh_14bit_tag()` only updates devices on the immediate secondary bus, failing to update endpoints deeper in the PCIe subtree.
- [High] `pci_restore_dev3_state()` incorrectly clears 14-Bit Tag Requester Enable on Virtual Functions (VFs) and Root Complex Integrated Endpoints (RCiEPs) and corrupts `bus->flit_mode`.
- [High] Concurrent unlocked modification of the `flit_mode` and `fm_enabled` bitfields can result in silent corruption of adjacent bitfields.
- [High] Recursive read lock of `pci_bus_sem` causes a deadlock during ASPM link training.
--

--- Patch [4]: [PATCH V3 4/5] PCI: Clear stale 14-Bit Tag Requester Enable when a link leaves Flit Mode ---
commit 862a293c463fc8879021277c4277adaf98825b8b
Author: Vidya Sagar <vidyas@nvidia.com>

PCI: Clear stale 14-Bit Tag Requester Enable when a link leaves Flit Mode

This patch clears the 14-Bit Tag Requester Enable state across PCIe bridges
and their subordinate devices when a link drops out of Flit Mode. It
introduces a new refresh function called during link state changes to avoid
Completion Timeouts and Unexpected Completions.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b881163febb6a..8481b9796bbc7 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
[ ... ]
> @@ -1754,12 +1754,22 @@ static void pci_restore_dev3_state(struct pci_dev *dev)
>  	 * Only the value written to hardware is adjusted.  The save buffer
>  	 * keeps what was saved, so the decision is taken afresh on every
>  	 * restore and the originally programmed value is not lost.
> +	 *
> +	 * Note: bridge-side and link-event paths are handled separately by
> +	 * pci_bridge_refresh_14bit_tag(), which runs from
> +	 * pci_bridge_wait_for_secondary_bus() and __pcie_update_link_speed()
> +	 * and clears the bit directly in hardware as soon as the link is
> +	 * observed to leave flit mode.  This function's responsibility is
> +	 * narrowed to the save-buffer-restore path.
>  	 */
>  	pci_read_config_dword(dev, pos + PCI_DEV3_CAP, &dev3_cap);
>  	if (dev3_cap & PCI_DEV3_CAP_14BIT_TAG_REQ) {
>  		/*
> -		 * Check both LNKSTA2.Flit_Mode (link-level) and
> -		 * DEV3_STA.Segment Captured (end-to-end); both must be
> +		 * Re-check link state here too: pci_restore_state() may run
> +		 * on paths where the link has changed mode but
> +		 * pci_bridge_refresh_14bit_tag() has not yet been called for
> +		 * this device.  Check both LNKSTA2.Flit_Mode (link-level)
> +		 * and DEV3_STA.Segment Captured (end-to-end); both must be
>  		 * active for 14-bit tags.  Refresh bus->flit_mode and
>  		 * dev->fm_enabled in lock-step.
>  		 */

[Severity: High]
Does this code corrupt the bus's flit_mode when restoring state for Virtual
Functions (VFs) or Root Complex Integrated Endpoints (RCiEPs)?

Since VFs have their link registers hardwired to 0 and RCiEPs do not implement
them, pcie_capability_read_word() will return 0 for LNKSTA2 on these devices.

It seems this would force flit_now = false, inappropriately dropping the
14-bit tag support and writing a 0 to dev->bus->flit_mode, thereby corrupting
the shared bus state.

[ ... ]
> +static void __pci_dev_clear_stale_14bit_tag(struct pci_dev *dev, bool flit_now)
> +{
[ ... ]
> +	pci_read_config_dword(dev, pos + PCI_DEV3_STA, &dev3_sta);
> +	dev->fm_enabled = !!(dev3_sta & PCI_DEV3_STA_SEGMENT);

[Severity: High]
Can this unlocked modification of the fm_enabled bitfield cause silent
corruption of adjacent bitfields?

Because this function can be executed in a hardirq context via the
pcie_bwnotif_irq() handler, an interrupt preempting another bitfield update
(such as is_added) might cause the preempted update to be silently
overwritten and lost.

[ ... ]
> +void pci_bridge_refresh_14bit_tag(struct pci_dev *bridge)
> +{
[ ... ]
> +	bus = bridge->subordinate;
> +	if (!bus)
> +		return;

[Severity: Critical]
Is there a potential Use-After-Free of the pci_bus structure here?

If pci_bridge_wait_for_secondary_bus() drops the lock and goes to sleep,
a concurrent hot-remove could free the bus and set bridge->subordinate to
NULL.

Since this reads bridge->subordinate without locks, it might fetch
a dangling pointer before it is set to NULL, which is then dereferenced.

> +
> +	bus->flit_mode = flit_now;

[Severity: High]
Similar to fm_enabled, could this unlocked assignment to the flit_mode
bitfield also silently corrupt adjacent bitfields on the pci_bus struct
when reached from hardirq context?

> +
> +	/*
> +	 * Walk the secondary bus.  pci_restore_dev3_state() only fires on
> +	 * paths that go through pci_dev_restore(); DPC release, hotplug
> +	 * link status updates, and similar paths do not.  Fix those too.
> +	 */
> +	down_read(&pci_bus_sem);

[Severity: High]
Will this cause a kernel panic by calling a sleeping function from an
invalid context?

The pcie_bwnotif_irq() interrupt handler runs in hardirq context and can
call down into this function via pcie_update_link_speed(). Taking the
pci_bus_sem read lock here will sleep.

[Severity: High]
Can this recursive read lock result in a deadlock during ASPM link training?

If ASPM initializes a link via pcie_aspm_init_link_state(), it already
holds down_read(&pci_bus_sem). The call chain can reach
pcie_update_link_speed() which unconditionally calls this function,
attempting to acquire the same lock again.

A pending concurrent writer would cause the recursive down_read() to block
indefinitely.

> +	list_for_each_entry(child, &bus->devices, bus_list)
> +		__pci_dev_clear_stale_14bit_tag(child, flit_now);

[Severity: High]
Does this loop miss endpoints located deeper in the PCIe subtree?

By iterating only over the immediate secondary bus, downstream ports and
endpoints below a PCIe switch will not have their 14-bit tag support cleared.

Could this result in Completion Timeouts if those devices continue to issue
14-bit tags upstream?

> +	up_read(&pci_bus_sem);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814201621.2281245-1-vidyas@nvidia.com?part=4

[PATCH V3 5/5] PCI: pciehp: Clear stale 14-Bit Tag Requester Enable on hot add

[Vidya Sagar]

pci_bridge_refresh_14bit_tag() runs from
pci_bridge_wait_for_secondary_bus() and from __pcie_update_link_speed(),
which between them cover every reset and every link mode change the core
observes.  A hot swap that changes the mode of the link reaches neither in
time.

Take a Downstream Port without DPC.  A card whose link runs in Flit Mode is
pulled out and one that trains in Non-Flit Mode is plugged in.  Nothing is
reset, so pci_bridge_wait_for_secondary_bus() never runs, and the new card
has no saved state, so pci_restore_state() does not run on it either.
pciehp does reach __pcie_update_link_speed() from
pciehp_check_link_status(), but only after pci_bus_check_dev() has polled
the new card's Vendor ID for up to a second in 20 ms steps.  The Port is
the requester for every one of those reads and still has 14-Bit Tag
Requester Enable set, so the card's completions cannot be matched, each
read times out and returns all ones, and the slot is reported as empty.
The card never enumerates.

Refresh the Port's 14-Bit Tag Requester Enable in
pciehp_check_link_status() once pcie_wait_for_link() has confirmed Data
Link Layer Link Active, and before the first config read to the card.
LNKSTA2.Flit_Mode is valid at that point because the link has finished
training.  The subordinate bus holds no devices yet, so the subtree walk in
the helper is a no-op, which is what we want: the new card comes out of
reset with its own 14-Bit Tag Requester Enable clear and is configured
during enumeration.

Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
---
V3:
* New patch in this series

 drivers/pci/hotplug/pciehp_hpc.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
index 4c62140a3cb4..95f9d5abdec2 100644
--- a/drivers/pci/hotplug/pciehp_hpc.c
+++ b/drivers/pci/hotplug/pciehp_hpc.c
@@ -302,6 +302,14 @@ int pciehp_check_link_status(struct controller *ctrl)
 	if (ctrl->inband_presence_disabled)
 		pcie_wait_for_presence(pdev);
 
+	/*
+	 * The link is up, but a newly inserted card may have trained it in a
+	 * different mode than the card that was removed left it in.  Drop a
+	 * now stale 14-Bit Tag Requester Enable on the Port before the first
+	 * config read below, which the Port issues as the requester.
+	 */
+	pci_bridge_refresh_14bit_tag(pdev);
+
 	found = pci_bus_check_dev(ctrl->pcie->port->subordinate,
 					PCI_DEVFN(0, 0));
 
-- 
2.43.0

Re: [PATCH V3 5/5] PCI: pciehp: Clear stale 14-Bit Tag Requester Enable on hot add

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Concurrent lockless updates to bitfields `bus->flit_mode`, `dev->fm_enabled`, and neighboring flags can cause silent data corruption.
--

--- Patch [5]: [PATCH V3 5/5] PCI: pciehp: Clear stale 14-Bit Tag Requester Enable on hot add ---
commit c3e9cc0a2fb59bb295c18b992a944ec4dd73a497
Author: Vidya Sagar <vidyas@nvidia.com>

PCI: pciehp: Clear stale 14-Bit Tag Requester Enable on hot add

This commit addresses an issue where a hot swap changing the link mode
leaves a stale 14-Bit Tag Requester Enable on a Downstream Port. It refreshes
the port's tag before the first config read to ensure completions can be
matched and the device enumerates properly.

> diff --git a/drivers/pci/hotplug/pciehp_hpc.c b/drivers/pci/hotplug/pciehp_hpc.c
> index 4c62140a3cb44..95f9d5abdec2f 100644
> --- a/drivers/pci/hotplug/pciehp_hpc.c
> +++ b/drivers/pci/hotplug/pciehp_hpc.c
> @@ -302,6 +302,14 @@ int pciehp_check_link_status(struct controller *ctrl)
>  	if (ctrl->inband_presence_disabled)
>  		pcie_wait_for_presence(pdev);
>  
> +	/*
> +	 * The link is up, but a newly inserted card may have trained it in a
> +	 * different mode than the card that was removed left it in.  Drop a
> +	 * now stale 14-Bit Tag Requester Enable on the Port before the first
> +	 * config read below, which the Port issues as the requester.
> +	 */
> +	pci_bridge_refresh_14bit_tag(pdev);

[Severity: High]
Does calling pci_bridge_refresh_14bit_tag() here introduce a data race with
other bitfield updates?

Inside pci_bridge_refresh_14bit_tag() and its helper 
__pci_dev_clear_stale_14bit_tag(), there are lockless bitfield updates on live
devices:

drivers/pci/pci.c:__pci_dev_clear_stale_14bit_tag() {
    ...
    dev->fm_enabled = !!(dev3_sta & PCI_DEV3_STA_SEGMENT);
    ...
}

and

drivers/pci/pci.c:pci_bridge_refresh_14bit_tag() {
    ...
    bus->flit_mode = flit_now;
    ...
}

In C, bitfield assignments compile to read-modify-write operations on the
underlying memory word. The dev->fm_enabled flag shares a 32-bit word with
critical flags like dev->state_saved and dev->is_msi_managed.

If the read-modify-write cycle of fm_enabled overlaps with another flag update
(like power management setting dev->state_saved = true, or driver teardown
updating dev->is_msi_managed), one of the updates can be lost.

Losing state_saved = true causes pci_restore_state() to silently skip restoring
device config space on resume. Losing is_msi_managed = true can cause devres
double-frees.

Additionally, bus->flit_mode shares a word with bus->unsafe_warn, which could
be concurrently updated via sysfs (e.g. pci_generic_config_write32).

Is it possible to add locking or change these bitfields to avoid the
read-modify-write races during link events?

> +
>  	found = pci_bus_check_dev(ctrl->pcie->port->subordinate,
>  					PCI_DEVFN(0, 0));
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814201621.2281245-1-vidyas@nvidia.com?part=5