Re: [PATCH 2/2] PCI: Replace pci_dev->broken_parity_status with accessors

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix: other sysfs store methods modify C bitfields without locks, leaving identical read-modify-write (RMW) tearing races unaddressed.
--

--- Patch [2]: [PATCH 2/2] PCI: Replace pci_dev->broken_parity_status with accessors ---
commit 0e6499959d43880d6cf73511e1681afc56300aa3
Author: Maurice Hieronymus <mhi@mailbox.org>

PCI: Replace pci_dev->broken_parity_status with accessors

This patch moves the broken_parity_status bit into a flags bitmap
modified with atomic bitops. This prevents read-modify-write tearing races
when sysfs writers race with other kernel updates to the same machine word.

> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 5ec0b245a69bd..5e094d1e23e3e 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c

[ ... ]

> @@ -93,7 +93,7 @@ static ssize_t broken_parity_status_store(struct device *dev,
>  	if (kstrtoul(buf, 0, &val) < 0)
>  		return -EINVAL;
>  
> -	pdev->broken_parity_status = !!val;
> +	pci_dev_assign_broken_parity_status(pdev, val);

[Severity: High]
This is a pre-existing issue, but does this leave other identical
read-modify-write tearing races unaddressed in this same file?

For example, in d3cold_allowed_store(), there is a lockless assignment:

drivers/pci/pci-sysfs.c:d3cold_allowed_store() {
    ...
    pdev->d3cold_allowed = !!val;
    ...
}

This bitfield shares a machine word with ignore_hotplug. If a sysfs write
races with a GPU driver calling pci_ignore_hotplug() during runtime PM,
could the kernel's update be lost? If the ignore_hotplug bit is torn, the
PCI hotplug controller might unexpectedly remove the GPU when it enters
D3cold, causing a kernel panic upon resume.

Similarly, msi_bus_store() locklessly assigns:

drivers/pci/pci-sysfs.c:msi_bus_store() {
    ...
    pdev->no_msi = !val;
    ...
}

This shares a word with msi_enabled. If this tears msi_enabled, could it
cause IRQ leaks when freeing vectors?

Another instance is pci_write_rom() locklessly assigning:

drivers/pci/pci-sysfs.c:pci_write_rom() {
    ...
    pdev->rom_attr_enabled = enable;
    ...
}

Would it make sense to also migrate d3cold_allowed, no_msi, and
rom_attr_enabled to this new atomic flags bitmap?

>  
>  	return count;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-pci-dev-flags-v1-0-2fcf2811138c@mailbox.org?part=2