Re: [PATCH] PCI: Save/restore Device 3 control and clear stale 14-bit Tag enables

[sashiko-bot@kernel.org]

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