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

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

--- Patch [3]: [PATCH V3 3/5] PCI: Save and restore the Device 3 Control register ---
commit 308945c2aad690ba20eb4cf961ec76f5391413d4
Author: Vidya Sagar <[email protected]>

PCI: Save and restore the Device 3 Control register

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

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

[ ... ]

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

[ ... ]

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

[ ... ]

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

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

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

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

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

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

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3