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

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

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

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

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

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

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

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

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

and

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

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

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

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

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

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

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

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