[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

[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

[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

[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

[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 3/5] PCI: Save and restore the Device 3 Control register

[Lukas Wunner]

On Sat, Aug 15, 2026 at 01:46:19AM +0530, Vidya Sagar wrote:
> +static void pci_restore_dev3_state(struct pci_dev *dev)
[...]
> +	/*
> +	 * 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.
> +	 */

This is quite verbose, can you condense it a bit?  E.g. it's not
necessary to say that every other bit is written back unchanged.
That's the default.  Just say e.g.:

       /* Clear 14-Bit Tag Requester Enable if link is no longer in Flit Mode */

> +	pci_read_config_dword(dev, pos + PCI_DEV3_CAP, &dev3_cap);
> +	if (dev3_cap & PCI_DEV3_CAP_14BIT_TAG_REQ) {

PCIe r7.0 sec 7.7.10.3 says the bit is "VF RsvdP", so you need
to check !dev->is_virtfn here.

> +		/*
> +		 * 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.
> +		 */

Why do we have to check both?

We usually spell out the register names fully or leave them out.
Using "LNKSTA2.Flit_Mode" and "DEV3_STA.Segment Captured" instead looks
odd to me.

> +		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;

When can dev->bus be NULL?

> +
> +		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_dbg() please.

> +	/*
> +	 * 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.
> +	 */

The code comment seems unnecessary because that policy applies
to every other Extended Capability as well.  We only save
mutable registers, that's the default.

Thanks,

Lukas

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

[Lukas Wunner]

On Sat, Aug 15, 2026 at 01:46:16AM +0530, Vidya Sagar wrote:
> 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.

Please include a pointer to the spec section.  Looking through
PCIe r7.0 sec 2.2.6.2.1, I don't quite see anything that says
"14-Bit Tag Requester Enable is only meaningful [...] in Flit Mode".

It does say:
"A Function that supports Flit Mode must support 14-Bit Tag Completer
capability"

But that's different from what you're claiming.

It also says:
"In Non-Flit Mode, Tag[8] and Tag[9], are not contiguous with other Tag
field bits in the TLP Header. These bits were Reserved prior to 10-Bit
Tags being architected. Requesters in Non-Flit Mode that do not support
10-Bit Tag Requester capability must set Tag[9:8] to 00b."

Also not quite what you're claiming.

So where is this coming from?

I think if the link reverts to Non-Flit Mode and 14-Bit Tags were
only meaningful in Flit Mode, then the hardware should automatically
stop using 14-Bit Tags.  Relying on software to clean up a protocol
inconsistency before issuing further requests seems dangerous and silly.

Thanks,

Lukas

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

[Lukas Wunner]

On Sat, Aug 15, 2026 at 01:46:20AM +0530, Vidya Sagar wrote:
> @@ -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;

pci_bridge_wait_for_secondary_bus() should just do what it says,
i.e. wait.  It shouldn't have side effects like mutating register
state.

I'm wondering if this is a quirk of the PCIe IP used on Nvidia's
arm64 CPUs, as I can't find a spec section that says software
needs to clean up 14 Bit Tag enablement when the link switches
from Flit Mode to Non-Flit Mode.

Thanks,

Lukas

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

[Lukas Wunner]

On Sat, Aug 15, 2026 at 01:46:21AM +0530, Vidya Sagar wrote:
> +++ 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));

pciehp_check_link_status() should only do what its name implies,
i.e. return whether the link is up.  It should not have side effects
like mutating register state.

I instead suggest clearing 10-Bit and 14-Bit enablement in
pciehp_unconfigure_device(), i.e. on removal.

Thanks,

Lukas