* Re: [PATCH] PCI: Save/restore Device 3 control and clear stale 14-bit Tag enables
2026-05-12 16:30 [PATCH] PCI: Save/restore Device 3 control and clear stale 14-bit Tag enables Vidya Sagar
@ 2026-05-12 22:49 ` Bjorn Helgaas
2026-08-11 13:10 ` Vidya Sagar
2026-05-13 23:41 ` sashiko-bot
` (5 subsequent siblings)
6 siblings, 1 reply; 9+ messages in thread
From: Bjorn Helgaas @ 2026-05-12 22:49 UTC (permalink / raw)
To: Vidya Sagar
Cc: bhelgaas, vsethi, sdonthineni, kthota, sagar.tv, linux-pci,
linux-kernel
On Tue, May 12, 2026 at 10:00:12PM +0530, Vidya Sagar wrote:
> The Device 3 Extended Capability (DEV3_CTL) lets software enable a
> device's 14-Bit Tag Requester and 14-Bit Tag Completer behavior.
> Those enable bits, however, are only meaningful while the link
> operates in flit mode; in non-flit mode the upper 4 tag bits are not
> transmitted on the wire, so a requester that still has them enabled
> emits TLPs whose completions the requester can no longer match. The
> visible symptom on the first downstream TLP after such a reset is
> Completion Timeout plus Unexpected Completion on the root/switch
> port.
Being OCD, I'm going to style "Flit Mode" to match usage in the PCIe
base spec.
> The PCI core currently does not save/restore DEV3_CTL, and nothing
> re-evaluates the 14-Bit Tag enable bits when the link mode changes.
> Across a Secondary Bus Reset, DPC trigger/release, slot reset, AER
> bus reset, FLR, or D3cold->D0 bridge resume the link can come back
> in non-flit mode while DEV3_CTL still advertises 14-bit tagging,
> which breaks the very first config read the kernel issues to the
> endpoint.
>
> Address this with two complementary pieces of work in one patch:
Are these separable at all? This is a big chunk to digest all at
once.
> 1. Save and restore DEV3_CTL across pci_save_state() /
> pci_restore_state(). Allocate the save buffer in pci_dev3_init()
> for every PCIe device that exposes the Device 3 Extended
> Capability, save DEV3_CTL only (DEV3_STA is RW1C and not
> restored), and on restore sanitize the saved value: if the device
> advertises 14-Bit Tag support but flit mode is no longer active
> (checked via the live LNKSTA2.Flit_Mode and DEV3_STA.Segment
> Captured), drop PCI_DEV3_CTL_14BIT_TAG_REQ_EN and
> PCI_DEV3_CTL_14BIT_TAG_COMP_EN before writing DEV3_CTL back.
I don't really know how Flit Mode works or how we get in this
situation. What is the scenario where the link was in Flit Mode
before, but is not in Flit Mode after the reset? That sounds like it
wouldn't be the desired outcome. I assume the reset would leave the
Flit Mode Disable cleared, so if the link didn't come up in Flit
Mode something went wrong in link training?
> This covers every path that goes through pci_dev_restore(),
> including the endpoint side of SBR/slot reset/D3cold resume.
>
> 2. Add pci_bridge_refresh_14bit_tag() and call it from the two
> universal choke points where the link mode can change but
> pci_dev_restore() does not necessarily run on the bridge itself:
>
> - pci_bridge_wait_for_secondary_bus() runs after every family
> of bridge-mediated reset (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()).
> The helper is called there immediately after the link is
> known active but before pci_dev_wait() issues the first
> downstream config read, so the requester is fixed up before
> any TLP is sent.
- What about pcie_flr()? You mention it above but not in the patch.
- I guess pci_pm_reset() isn't an issue because it only does
D0->D3hot->D0uninitialized transitions, and the link stays up
and Flit Mode is unchanged?
> - __pcie_update_link_speed() is where bus->flit_mode is
> authoritatively updated whenever the kernel observes a link
> change: initial enumeration, manual pcie_retrain_link()
> (ASPM common-clock, target-speed change), bwctrl IRQ from
> autonomous HW speed changes, and the pciehp link-status
> poll. Calling the helper there covers retrain paths that
> never go through pci_bridge_wait_for_secondary_bus().
>
> The helper re-reads the bridge's own LNKSTA2.Flit_Mode, fixes the
> bridge's DEV3_CTL first (because the bridge is the requester for
> outbound config/MMIO TLPs and is what produces the post-reset
> error storm), refreshes bus->flit_mode, then walks the
> subordinate bus to fix every device that advertises 14-Bit Tag
> support. All bridge accesses are to the bridge's own config
> space on the primary bus, so the fix runs safely before any
> potentially-broken downstream TLP. Operation is idempotent:
> hardware is touched only when the enable bits are set and flit
> is no longer active.
Error storm? Sounds like something visible in dmesg where we might be
able to include a snippet here?
> To call the helper from __pcie_update_link_speed(), move that
> function's body from a static inline in drivers/pci/pci.h to an
> ordinary out-of-line definition in drivers/pci/probe.c (pure
> refactor, no functional change for existing callers).
Carving this out to a separate patch would help make this more
approachable to review.
> Also add the missing DEV3 14-Bit Tag bit names to
> include/uapi/linux/pci_regs.h:
> PCI_DEV3_CAP_14BIT_TAG_{COMP,REQ} and
> PCI_DEV3_CTL_14BIT_TAG_{COMP,REQ}_EN.
>
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> ---
> drivers/pci/pci.c | 213 ++++++++++++++++++++++++++++++++++
> drivers/pci/pci.h | 17 +--
> drivers/pci/probe.c | 32 +++++
> include/uapi/linux/pci_regs.h | 4 +
> 4 files changed, 254 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 0be4e263bca0..ad1a282a4e63 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1682,6 +1682,199 @@ 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];
> +
> + /*
> + * The 14-Bit Tag enable bits in DEV3_CTL are only valid in flit
> + * mode. On devices that advertise 14-Bit Tag 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. On other devices (and for any other
> + * DEV3_CTL bits, including future architected or vendor-specific
> + * ones), the saved value is written back unchanged.
> + *
> + * 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 bits 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 |
> + PCI_DEV3_CAP_14BIT_TAG_COMP)) {
> + /*
> + * 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.
> + */
> + 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 |
> + PCI_DEV3_CTL_14BIT_TAG_COMP_EN))) {
> + val &= ~(PCI_DEV3_CTL_14BIT_TAG_REQ_EN |
> + PCI_DEV3_CTL_14BIT_TAG_COMP_EN);
> + cap[0] = val;
> + pci_info(dev, "clearing 14-bit tag 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);
> +}
> +
> +/*
> + * Clear DEV3_CTL.14-Bit Tag {Req,Comp} 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 enables are 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 these bits set will emit TLPs the completer
> + * 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 |
> + PCI_DEV3_CAP_14BIT_TAG_COMP)))
> + 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 |
> + PCI_DEV3_CTL_14BIT_TAG_COMP_EN)))
> + return;
> +
> + dev3_ctl &= ~(PCI_DEV3_CTL_14BIT_TAG_REQ_EN |
> + PCI_DEV3_CTL_14BIT_TAG_COMP_EN);
> + pci_write_config_dword(dev, pos + PCI_DEV3_CTL, dev3_ctl);
> + pci_info(dev, "cleared 14-bit Tag enable: flit mode no longer active (DEV3_STA=%#010x)\n",
> + dev3_sta);
> +}
> +
> +/**
> + * pci_bridge_refresh_14bit_tag - Drop stale 14-Bit Tag enables across a link
> + * @bridge: PCIe bridge whose link may have changed mode
> + *
> + * Re-evaluate the bridge's own DEV3_CTL 14-Bit Tag enables 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 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 bits are
> + * 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 stale 14-bit Tag enables here are
> + * what produce 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;
> @@ -1738,6 +1931,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;
> @@ -1815,6 +2012,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);
> @@ -4745,6 +4943,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 enables
> + * 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;
>
> @@ -4772,6 +4978,13 @@ 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 enables 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 19660d068fb7..a47a0f3ad2e2 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -633,21 +633,14 @@ 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);
>
> +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 748c7a198262..27eb97b22c36 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -826,6 +826,30 @@ 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);
> +
> + /*
> + * Re-evaluate DEV3_CTL 14-Bit Tag enables 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 their 14-Bit Tag enables
> + * 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,
> enum pcie_link_change_reason reason)
> {
> @@ -2320,11 +2344,19 @@ 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; DEV3_STA is RW1C and is not restored. */
> + 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");
> }
>
> /**
> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
> index 14f634ab9350..6e00dbdf04eb 100644
> --- a/include/uapi/linux/pci_regs.h
> +++ b/include/uapi/linux/pci_regs.h
> @@ -1254,7 +1254,11 @@
>
> /* Device 3 Extended Capability */
> #define PCI_DEV3_CAP 0x04 /* Device 3 Capabilities Register */
> +#define PCI_DEV3_CAP_14BIT_TAG_COMP 0x00000004 /* 14-Bit Tag Completer Supported */
> +#define PCI_DEV3_CAP_14BIT_TAG_REQ 0x00000008 /* 14-Bit Tag Requester Supported */
> #define PCI_DEV3_CTL 0x08 /* Device 3 Control Register */
> +#define PCI_DEV3_CTL_14BIT_TAG_COMP_EN 0x00000002 /* 14-Bit Tag Completer Enable */
> +#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.25.1
>
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [PATCH] PCI: Save/restore Device 3 control and clear stale 14-bit Tag enables
2026-05-12 22:49 ` Bjorn Helgaas
@ 2026-08-11 13:10 ` Vidya Sagar
0 siblings, 0 replies; 9+ messages in thread
From: Vidya Sagar @ 2026-08-11 13:10 UTC (permalink / raw)
To: Bjorn Helgaas
Cc: bhelgaas@google.com, Vikram Sethi, Shanker Donthineni,
Krishna Thota, sagar.tv@gmail.com, linux-pci@vger.kernel.org,
linux-kernel@vger.kernel.org
On 13/05/26 04:19, Bjorn Helgaas wrote:
> External email: Use caution opening links or attachments
>
>
> On Tue, May 12, 2026 at 10:00:12PM +0530, Vidya Sagar wrote:
>> The Device 3 Extended Capability (DEV3_CTL) lets software enable a
>> device's 14-Bit Tag Requester and 14-Bit Tag Completer behavior.
>> Those enable bits, however, are only meaningful while the link
>> operates in flit mode; in non-flit mode the upper 4 tag bits are not
>> transmitted on the wire, so a requester that still has them enabled
>> emits TLPs whose completions the requester can no longer match. The
>> visible symptom on the first downstream TLP after such a reset is
>> Completion Timeout plus Unexpected Completion on the root/switch
>> port.
>
> Being OCD, I'm going to style "Flit Mode" to match usage in the PCIe
> base spec.
I'll address it in the next patch>
>> The PCI core currently does not save/restore DEV3_CTL, and nothing
>> re-evaluates the 14-Bit Tag enable bits when the link mode changes.
>> Across a Secondary Bus Reset, DPC trigger/release, slot reset, AER
>> bus reset, FLR, or D3cold->D0 bridge resume the link can come back
>> in non-flit mode while DEV3_CTL still advertises 14-bit tagging,
>> which breaks the very first config read the kernel issues to the
>> endpoint.
>>
>> Address this with two complementary pieces of work in one patch:
>
> Are these separable at all? This is a big chunk to digest all at
> once.
Yes. Let me split them into simpler patches.>
>> 1. Save and restore DEV3_CTL across pci_save_state() /
>> pci_restore_state(). Allocate the save buffer in pci_dev3_init()
>> for every PCIe device that exposes the Device 3 Extended
>> Capability, save DEV3_CTL only (DEV3_STA is RW1C and not
>> restored), and on restore sanitize the saved value: if the device
>> advertises 14-Bit Tag support but flit mode is no longer active
>> (checked via the live LNKSTA2.Flit_Mode and DEV3_STA.Segment
>> Captured), drop PCI_DEV3_CTL_14BIT_TAG_REQ_EN and
>> PCI_DEV3_CTL_14BIT_TAG_COMP_EN before writing DEV3_CTL back.
>
> I don't really know how Flit Mode works or how we get in this
> situation. What is the scenario where the link was in Flit Mode
> before, but is not in Flit Mode after the reset? That sounds like it
> wouldn't be the desired outcome. I assume the reset would leave the
> Flit Mode Disable cleared, so if the link didn't come up in Flit
> Mode something went wrong in link training?
Well, getting into this situation is straight forward in the scenario where
hot unplugging of a device that is currently operating at Gen6 speed (where
it is a must to have the link operating in Flit Mode) and replacing it with
a device that doesn't support Gen6 speed (hence the link never goes into
Flit Mode with this new device).>
>> This covers every path that goes through pci_dev_restore(),
>> including the endpoint side of SBR/slot reset/D3cold resume.
>>
>> 2. Add pci_bridge_refresh_14bit_tag() and call it from the two
>> universal choke points where the link mode can change but
>> pci_dev_restore() does not necessarily run on the bridge itself:
>>
>> - pci_bridge_wait_for_secondary_bus() runs after every family
>> of bridge-mediated reset (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()).
>> The helper is called there immediately after the link is
>> known active but before pci_dev_wait() issues the first
>> downstream config read, so the requester is fixed up before
>> any TLP is sent.
>
> - What about pcie_flr()? You mention it above but not in the patch.
Since FLR is scoped to a function and doesn't change the characteristics of
the link, mention of FLR should be removed from the list. That was a miss from
my side and I'll remove it.>
> - I guess pci_pm_reset() isn't an issue because it only does
> D0->D3hot->D0uninitialized transitions, and the link stays up
> and Flit Mode is unchanged?
Yes.>
>> - __pcie_update_link_speed() is where bus->flit_mode is
>> authoritatively updated whenever the kernel observes a link
>> change: initial enumeration, manual pcie_retrain_link()
>> (ASPM common-clock, target-speed change), bwctrl IRQ from
>> autonomous HW speed changes, and the pciehp link-status
>> poll. Calling the helper there covers retrain paths that
>> never go through pci_bridge_wait_for_secondary_bus().
>>
>> The helper re-reads the bridge's own LNKSTA2.Flit_Mode, fixes the
>> bridge's DEV3_CTL first (because the bridge is the requester for
>> outbound config/MMIO TLPs and is what produces the post-reset
>> error storm), refreshes bus->flit_mode, then walks the
>> subordinate bus to fix every device that advertises 14-Bit Tag
>> support. All bridge accesses are to the bridge's own config
>> space on the primary bus, so the fix runs safely before any
>> potentially-broken downstream TLP. Operation is idempotent:
>> hardware is touched only when the enable bits are set and flit
>> is no longer active.
>
> Error storm? Sounds like something visible in dmesg where we might be
> able to include a snippet here?
>
>> To call the helper from __pcie_update_link_speed(), move that
>> function's body from a static inline in drivers/pci/pci.h to an
>> ordinary out-of-line definition in drivers/pci/probe.c (pure
>> refactor, no functional change for existing callers).
>
> Carving this out to a separate patch would help make this more
> approachable to review.
>
>> Also add the missing DEV3 14-Bit Tag bit names to
>> include/uapi/linux/pci_regs.h:
>> PCI_DEV3_CAP_14BIT_TAG_{COMP,REQ} and
>> PCI_DEV3_CTL_14BIT_TAG_{COMP,REQ}_EN.
>>
>> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
>> ---
>> drivers/pci/pci.c | 213 ++++++++++++++++++++++++++++++++++
>> drivers/pci/pci.h | 17 +--
>> drivers/pci/probe.c | 32 +++++
>> include/uapi/linux/pci_regs.h | 4 +
>> 4 files changed, 254 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 0be4e263bca0..ad1a282a4e63 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -1682,6 +1682,199 @@ 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];
>> +
>> + /*
>> + * The 14-Bit Tag enable bits in DEV3_CTL are only valid in flit
>> + * mode. On devices that advertise 14-Bit Tag 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. On other devices (and for any other
>> + * DEV3_CTL bits, including future architected or vendor-specific
>> + * ones), the saved value is written back unchanged.
>> + *
>> + * 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 bits 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 |
>> + PCI_DEV3_CAP_14BIT_TAG_COMP)) {
>> + /*
>> + * 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.
>> + */
>> + 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 |
>> + PCI_DEV3_CTL_14BIT_TAG_COMP_EN))) {
>> + val &= ~(PCI_DEV3_CTL_14BIT_TAG_REQ_EN |
>> + PCI_DEV3_CTL_14BIT_TAG_COMP_EN);
>> + cap[0] = val;
>> + pci_info(dev, "clearing 14-bit tag 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);
>> +}
>> +
>> +/*
>> + * Clear DEV3_CTL.14-Bit Tag {Req,Comp} 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 enables are 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 these bits set will emit TLPs the completer
>> + * 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 |
>> + PCI_DEV3_CAP_14BIT_TAG_COMP)))
>> + 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 |
>> + PCI_DEV3_CTL_14BIT_TAG_COMP_EN)))
>> + return;
>> +
>> + dev3_ctl &= ~(PCI_DEV3_CTL_14BIT_TAG_REQ_EN |
>> + PCI_DEV3_CTL_14BIT_TAG_COMP_EN);
>> + pci_write_config_dword(dev, pos + PCI_DEV3_CTL, dev3_ctl);
>> + pci_info(dev, "cleared 14-bit Tag enable: flit mode no longer active (DEV3_STA=%#010x)\n",
>> + dev3_sta);
>> +}
>> +
>> +/**
>> + * pci_bridge_refresh_14bit_tag - Drop stale 14-Bit Tag enables across a link
>> + * @bridge: PCIe bridge whose link may have changed mode
>> + *
>> + * Re-evaluate the bridge's own DEV3_CTL 14-Bit Tag enables 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 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 bits are
>> + * 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 stale 14-bit Tag enables here are
>> + * what produce 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;
>> @@ -1738,6 +1931,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;
>> @@ -1815,6 +2012,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);
>> @@ -4745,6 +4943,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 enables
>> + * 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;
>>
>> @@ -4772,6 +4978,13 @@ 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 enables 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 19660d068fb7..a47a0f3ad2e2 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -633,21 +633,14 @@ 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);
>>
>> +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 748c7a198262..27eb97b22c36 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -826,6 +826,30 @@ 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);
>> +
>> + /*
>> + * Re-evaluate DEV3_CTL 14-Bit Tag enables 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 their 14-Bit Tag enables
>> + * 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,
>> enum pcie_link_change_reason reason)
>> {
>> @@ -2320,11 +2344,19 @@ 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; DEV3_STA is RW1C and is not restored. */
>> + 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");
>> }
>>
>> /**
>> diff --git a/include/uapi/linux/pci_regs.h b/include/uapi/linux/pci_regs.h
>> index 14f634ab9350..6e00dbdf04eb 100644
>> --- a/include/uapi/linux/pci_regs.h
>> +++ b/include/uapi/linux/pci_regs.h
>> @@ -1254,7 +1254,11 @@
>>
>> /* Device 3 Extended Capability */
>> #define PCI_DEV3_CAP 0x04 /* Device 3 Capabilities Register */
>> +#define PCI_DEV3_CAP_14BIT_TAG_COMP 0x00000004 /* 14-Bit Tag Completer Supported */
>> +#define PCI_DEV3_CAP_14BIT_TAG_REQ 0x00000008 /* 14-Bit Tag Requester Supported */
>> #define PCI_DEV3_CTL 0x08 /* Device 3 Control Register */
>> +#define PCI_DEV3_CTL_14BIT_TAG_COMP_EN 0x00000002 /* 14-Bit Tag Completer Enable */
>> +#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.25.1
>>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] PCI: Save/restore Device 3 control and clear stale 14-bit Tag enables
2026-05-12 16:30 [PATCH] PCI: Save/restore Device 3 control and clear stale 14-bit Tag enables Vidya Sagar
2026-05-12 22:49 ` Bjorn Helgaas
@ 2026-05-13 23:41 ` sashiko-bot
2026-08-11 16:46 ` [PATCH V2 0/4] PCI: Re-evaluate DEV3 14-Bit Tag Requester Enable on link mode changes Vidya Sagar
` (4 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: sashiko-bot @ 2026-05-13 23:41 UTC (permalink / raw)
To: Vidya Sagar; +Cc: linux-pci
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Premature downstream config accesses before child device readiness causes child fixups to be silently skipped.
- [High] Incomplete bus hierarchy traversal leaves endpoints behind switches with stale 14-bit tag enables.
- [High] Unsynchronized C bitfield assignments cause data races that corrupt device and bus state.
- [High] Incorrect bus flit_mode assignment for downstream ports corrupts primary bus state.
--
commit cfcee5167fd2cdd929f8d6d46c5515889aa39e42
Author: Vidya Sagar <vidyas@nvidia.com>
Subject: PCI: Save/restore Device 3 control and clear stale 14-bit Tag enables
This patch saves and restores DEV3_CTL across pci_save_state() and
pci_restore_state(). It also clears stale 14-bit Tag enables when the link
mode changes to non-flit.
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8f7cfcc000901..a146db4ca358c 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
[ ... ]
> @@ -1695,6 +1695,199 @@ static void pci_restore_pcie_state(struct pci_dev *dev)
[ ... ]
> +static void pci_restore_dev3_state(struct pci_dev *dev)
> +{
[ ... ]
> + pci_read_config_dword(dev, pos + PCI_DEV3_STA, &dev3_sta);
> + dev->fm_enabled = !!(dev3_sta & PCI_DEV3_STA_SEGMENT);
Is this bitfield assignment safe against concurrent modifications?
Compilers implement bitfield assignments as non-atomic read-modify-write
operations on the entire containing integer word.
If this runs concurrently with sibling resumes, or asynchronously during
probe or suspend (which modify adjacent bitfields like is_managed or
state_saved), could this clobber those other states and cause a data race?
> +
> + 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;
For a downstream port or root port, LNKSTA2 reflects the status of its
downstream link.
Would assigning this to dev->bus incorrectly update the port's upstream
primary bus instead of the secondary bus (dev->subordinate)?
[ ... ]
> +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);
This is another C bitfield assignment that might race if triggered
asynchronously.
Since pci_bridge_refresh_14bit_tag() holds only down_read(&pci_bus_sem)
during bandwidth notification events, could this also result in data
corruption?
[ ... ]
> +void pci_bridge_refresh_14bit_tag(struct pci_dev *bridge)
> +{
[ ... ]
> + /*
> + * 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);
> +}
Does this iterate deeply enough to catch endpoints behind PCIe switches?
If the bridge is a root port connected to a PCIe switch,
list_for_each_entry() will only visit the immediate child (the switch
upstream port).
Should this use pci_walk_bus() instead to recursively visit all downstream
devices, ensuring endpoints beneath a switch do not retain stale enables?
[ ... ]
> @@ -4758,6 +4956,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 enables
> + * 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;
Will calling pci_bridge_refresh_14bit_tag() here cause child devices to be
silently skipped if they are not yet ready?
pci_bridge_refresh_14bit_tag() walks the secondary bus and issues downstream
config reads via pci_find_ext_capability(). Since this occurs before
pci_dev_wait() verifies child recovery, the child may respond with CRS.
If it responds with CRS, pci_find_ext_capability() fails and skips the child.
When the child finally becomes ready, it will retain its stale 14-bit tag
enables.
Could the child traversal be deferred until after pci_dev_wait() guarantees
the devices are accessible?
> @@ -4785,6 +4991,13 @@ 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 enables 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);
> }
Similarly, could the child devices be missed here since pci_dev_wait() has
not yet completed?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260512163012.2336191-1-vidyas@nvidia.com?part=1
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH V2 0/4] PCI: Re-evaluate DEV3 14-Bit Tag Requester Enable on link mode changes
2026-05-12 16:30 [PATCH] PCI: Save/restore Device 3 control and clear stale 14-bit Tag enables Vidya Sagar
2026-05-12 22:49 ` Bjorn Helgaas
2026-05-13 23:41 ` sashiko-bot
@ 2026-08-11 16:46 ` Vidya Sagar
2026-08-11 16:46 ` [PATCH V2 1/4] PCI: Add DEV3 14-Bit Tag Requester register definitions Vidya Sagar
` (3 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Vidya Sagar @ 2026-08-11 16:46 UTC (permalink / raw)
To: bhelgaas
Cc: vsethi, sdonthineni, kthota, mmaddireddy, kumarahul, sagar.tv,
linux-pci, linux-kernel, 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 is what happens today on the very first config read after a
bridge-mediated reset when the link comes back 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 bridge: 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/4 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/4 moves __pcie_update_link_speed() out of line, which 4/4 needs. Pure
refactor.
3/4 saves DEV3_CTL and, on restore, drops 14-Bit Tag Requester Enable
from the saved value 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/4 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()).
3/4 and 4/4 are independent of each other; each is useful on its own.
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.
V2:
* Split the V1 monolithic patch into 4 patches
Vidya Sagar (4):
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
drivers/pci/pci.c | 208 ++++++++++++++++++++++++++++++++++
drivers/pci/pci.h | 17 +--
drivers/pci/probe.c | 36 ++++++
include/uapi/linux/pci_regs.h | 2 +
4 files changed, 251 insertions(+), 12 deletions(-)
--
2.43.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH V2 1/4] PCI: Add DEV3 14-Bit Tag Requester register definitions
2026-05-12 16:30 [PATCH] PCI: Save/restore Device 3 control and clear stale 14-bit Tag enables Vidya Sagar
` (2 preceding siblings ...)
2026-08-11 16:46 ` [PATCH V2 0/4] PCI: Re-evaluate DEV3 14-Bit Tag Requester Enable on link mode changes Vidya Sagar
@ 2026-08-11 16:46 ` Vidya Sagar
2026-08-11 16:46 ` [PATCH V2 2/4] PCI: Move __pcie_update_link_speed() out of line Vidya Sagar
` (2 subsequent siblings)
6 siblings, 0 replies; 9+ messages in thread
From: Vidya Sagar @ 2026-08-11 16:46 UTC (permalink / raw)
To: bhelgaas
Cc: vsethi, sdonthineni, kthota, mmaddireddy, kumarahul, sagar.tv,
linux-pci, linux-kernel, 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>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH V2 2/4] PCI: Move __pcie_update_link_speed() out of line
2026-05-12 16:30 [PATCH] PCI: Save/restore Device 3 control and clear stale 14-bit Tag enables Vidya Sagar
` (3 preceding siblings ...)
2026-08-11 16:46 ` [PATCH V2 1/4] PCI: Add DEV3 14-Bit Tag Requester register definitions Vidya Sagar
@ 2026-08-11 16:46 ` Vidya Sagar
2026-08-11 16:46 ` [PATCH V2 3/4] PCI: Save and restore the Device 3 Control register Vidya Sagar
2026-08-11 16:46 ` [PATCH V2 4/4] PCI: Clear stale 14-Bit Tag Requester Enable when a link leaves Flit Mode Vidya Sagar
6 siblings, 0 replies; 9+ messages in thread
From: Vidya Sagar @ 2026-08-11 16:46 UTC (permalink / raw)
To: bhelgaas
Cc: vsethi, sdonthineni, kthota, mmaddireddy, kumarahul, sagar.tv,
linux-pci, linux-kernel, 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>
---
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH V2 3/4] PCI: Save and restore the Device 3 Control register
2026-05-12 16:30 [PATCH] PCI: Save/restore Device 3 control and clear stale 14-bit Tag enables Vidya Sagar
` (4 preceding siblings ...)
2026-08-11 16:46 ` [PATCH V2 2/4] PCI: Move __pcie_update_link_speed() out of line Vidya Sagar
@ 2026-08-11 16:46 ` Vidya Sagar
2026-08-11 16:46 ` [PATCH V2 4/4] PCI: Clear stale 14-Bit Tag Requester Enable when a link leaves Flit Mode Vidya Sagar
6 siblings, 0 replies; 9+ messages in thread
From: Vidya Sagar @ 2026-08-11 16:46 UTC (permalink / raw)
To: bhelgaas
Cc: vsethi, sdonthineni, kthota, mmaddireddy, kumarahul, sagar.tv,
linux-pci, linux-kernel, 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>
---
V2:
* New patch as a result of splitting the V1 monolithic patch
drivers/pci/pci.c | 81 +++++++++++++++++++++++++++++++++++++++++++++
drivers/pci/probe.c | 12 +++++++
2 files changed, 93 insertions(+)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index ff6d5d059b21..3bbeed1ba20c 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1703,6 +1703,82 @@ 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.
+ */
+ 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;
+ cap[0] = val;
+ 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 +1835,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 +1906,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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH V2 4/4] PCI: Clear stale 14-Bit Tag Requester Enable when a link leaves Flit Mode
2026-05-12 16:30 [PATCH] PCI: Save/restore Device 3 control and clear stale 14-bit Tag enables Vidya Sagar
` (5 preceding siblings ...)
2026-08-11 16:46 ` [PATCH V2 3/4] PCI: Save and restore the Device 3 Control register Vidya Sagar
@ 2026-08-11 16:46 ` Vidya Sagar
6 siblings, 0 replies; 9+ messages in thread
From: Vidya Sagar @ 2026-08-11 16:46 UTC (permalink / raw)
To: bhelgaas
Cc: vsethi, sdonthineni, kthota, mmaddireddy, kumarahul, sagar.tv,
linux-pci, linux-kernel, 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>
---
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 3bbeed1ba20c..b53ca5f47f07 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -1750,12 +1750,22 @@ static void pci_restore_dev3_state(struct pci_dev *dev)
* 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.
+ *
+ * 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.
*/
@@ -1779,6 +1789,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;
@@ -4871,6 +4982,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;
@@ -4898,6 +5017,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
^ permalink raw reply related [flat|nested] 9+ messages in thread