Re: [PATCH RFC v2 2/5] PCI: pciehp: Add pci_hp_wait_link_change()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] pci: missing acquire barrier in pci_hp_wait_link_change()
--

commit 4a9f36e746ab6c768961f1d34f724895b17c9def
Author: Abhin Parekadan Jose <abhinjoses@gmail.com>

PCI: pciehp: Add pci_hp_wait_link_change()

pci_hp_spurious_link_change() awaits the end of a code section causing
spurious link changes, such as a Secondary Bus Reset. This patch splits the
wait out into a new function pci_hp_wait_link_change() to allow hotplug
drivers to wait for the link to settle without clearing the changed state.

> diff --git a/drivers/pci/hotplug/pci_hotplug_core.c b/drivers/pci/hotplug/pci_hotplug_core.c
> index fadcf98a8a660..c3e0ee8b36381 100644
> --- a/drivers/pci/hotplug/pci_hotplug_core.c
> +++ b/drivers/pci/hotplug/pci_hotplug_core.c

[ ... ]

> @@ -528,6 +528,24 @@ void pci_hp_unignore_link_change(struct pci_dev *pdev)
>  	wake_up_all(&pci_hp_link_change_wq);
>  }
>  
> +/**
> + * pci_hp_wait_link_change - await end of code section causing spurious link changes
> + * @pdev: PCI hotplug bridge
> + *
> + * Await the end of a concurrently executing code section which is causing
> + * spurious link changes on the Secondary Bus of @pdev, if there is one.
> + *
> + * Unlike pci_hp_spurious_link_change(), leave the record that such a code
> + * section has executed in place.  May be called by hotplug drivers which need
> + * the link to have settled, but not the cause of a link change, so that they
> + * don't take the answer away from the caller of pci_hp_spurious_link_change().
> + */
> +void pci_hp_wait_link_change(struct pci_dev *pdev)
> +{
> +	wait_event(pci_hp_link_change_wq,
> +		   !test_bit(PCI_LINK_CHANGING, &pdev->priv_flags));
> +}
> +

[Severity: High]
Does this wait_event() condition check need an explicit acquire barrier?

If !test_bit(PCI_LINK_CHANGING, ...) evaluates to true immediately,
wait_event() returns without executing any memory barriers.

On weakly ordered architectures like ARM64, could this allow the CPU or
compiler to reorder the caller's subsequent state reads before the test_bit()
load?

For instance, if thread A clears PCI_LINK_CHANGING and wakes waiters, and
thread B calls pci_hp_wait_link_change() and returns immediately without a
barrier, could thread B's subsequent PCIe config reads execute before observing
the link state change, even if inside an acquire lock like down_read_nested()?

Would using test_bit_acquire() or an explicit smp_mb() prevent stale reads
here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260927165459.829900-1-abhinjoses@gmail.com?part=2