[PATCH 0/2] PCI: Convert bitfield flags to atomic accessors

[PATCH 0/2] PCI: Convert bitfield flags to atomic accessors

[Maurice Hieronymus]

`struct pci_dev` keeps ~60 flags in one C bitfield. Bits sharing a
word must not be modified concurrently, but several writers take no
common lock: `pci_set_master()` writes `is_busmaster` and can run
without the device lock (e.g. runtime PM resume paths),
`pci_disable_device()` clears it, and `broken_parity_status_store()`
writes the same word from sysfs at any time without any lock.

Convert these two bits to a new public `flags` bitmap accessed with
atomic bitops, mirroring how the driver core replaced its
`offline`/`offline_disabled` bitfield in commit a7cc262a1135 ("driver
core: Replace dev->offline + ->offline_disabled with accessors").
More bits can follow the same pattern later.

An alternative would be to reuse `priv_flags`, but its bit definitions
and accessors are deliberately private to drivers/pci, while
`is_busmaster` is accessed by xen-pciback, lpfc and sfc. Happy to
respin that way if preferred.

This is also a prerequisite for the Rust device enabling API rework
[1]: the guard object planned there calls `pci_disable_device()` from
contexts that may run concurrently with `pci_set_master()`, which
requires `is_busmaster` to not be part of a shared bitfield word.

Link: https://lore.kernel.org/rust-for-linux/DJOEYVBS17MJ.1YD3TNGQBWHNK@kernel.org/ [1]
Signed-off-by: Maurice Hieronymus <mhi@mailbox.org>
---
Maurice Hieronymus (2):
      PCI: Replace pci_dev->is_busmaster with accessors
      PCI: Replace pci_dev->broken_parity_status with accessors

 drivers/edac/edac_pci_sysfs.c               |  4 +--
 drivers/net/ethernet/sfc/falcon/farch.c     |  2 +-
 drivers/net/ethernet/sfc/siena/farch.c      |  2 +-
 drivers/pci/pci-driver.c                    |  2 +-
 drivers/pci/pci-sysfs.c                     |  4 +--
 drivers/pci/pci.c                           |  6 ++---
 drivers/scsi/lpfc/lpfc_init.c               |  4 +--
 drivers/xen/xen-pciback/conf_space_header.c |  4 +--
 drivers/xen/xen-pciback/pciback_ops.c       |  4 +--
 include/linux/pci.h                         | 42 +++++++++++++++++++++++++++--
 10 files changed, 56 insertions(+), 18 deletions(-)
---
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
change-id: 20260711-pci-dev-flags-fbbcf4ff9031

Best regards,
-- 
Maurice Hieronymus <mhi@mailbox.org>

[PATCH 1/2] PCI: Replace pci_dev->is_busmaster with accessors

[Maurice Hieronymus]

`is_busmaster` is one bit of a ~60-bit C bitfield in `struct pci_dev`.
Bits sharing a bitfield word must not be modified concurrently, but its
writers take no common lock: `pci_set_master()` can run without the
device lock (e.g. from runtime PM resume paths), `pci_disable_device()`
clears the bit, and other bits in the same word are written from
entirely different contexts, e.g. `broken_parity_status` from sysfs.
Concurrent read-modify-write cycles of the shared word can then lose
updates.

Move `is_busmaster` into a new `flags` bitmap modified with atomic
bitops and accessed through generated accessor functions, following the
example of commit a7cc262a1135 ("driver core: Replace dev->offline +
->offline_disabled with accessors"). More bitfield flags can follow the
same pattern later.

This also unblocks the Rust device enabling API rework [1], where a
guard object calls `pci_disable_device()` from contexts that may run
concurrently with `pci_set_master()`.

Link: https://lore.kernel.org/rust-for-linux/DJOEYVBS17MJ.1YD3TNGQBWHNK@kernel.org/ [1]
Suggested-by: Danilo Krummrich <dakr@kernel.org>
Cc: rust-for-linux@vger.kernel.org
Signed-off-by: Maurice Hieronymus <mhi@mailbox.org>
---
 drivers/net/ethernet/sfc/falcon/farch.c     |  2 +-
 drivers/net/ethernet/sfc/siena/farch.c      |  2 +-
 drivers/pci/pci-driver.c                    |  2 +-
 drivers/pci/pci.c                           |  6 ++---
 drivers/scsi/lpfc/lpfc_init.c               |  4 ++--
 drivers/xen/xen-pciback/conf_space_header.c |  4 ++--
 drivers/xen/xen-pciback/pciback_ops.c       |  4 ++--
 include/linux/pci.h                         | 37 ++++++++++++++++++++++++++++-
 8 files changed, 48 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/sfc/falcon/farch.c b/drivers/net/ethernet/sfc/falcon/farch.c
index 23d507a3820d..42594bd7e818 100644
--- a/drivers/net/ethernet/sfc/falcon/farch.c
+++ b/drivers/net/ethernet/sfc/falcon/farch.c
@@ -724,7 +724,7 @@ int ef4_farch_fini_dmaq(struct ef4_nic *efx)
 	/* Do not attempt to write to the NIC during EEH recovery */
 	if (efx->state != STATE_RECOVERY) {
 		/* Only perform flush if DMA is enabled */
-		if (efx->pci_dev->is_busmaster) {
+		if (pci_dev_busmaster(efx->pci_dev)) {
 			efx->type->prepare_flush(efx);
 			rc = ef4_farch_do_flush(efx);
 			efx->type->finish_flush(efx);
diff --git a/drivers/net/ethernet/sfc/siena/farch.c b/drivers/net/ethernet/sfc/siena/farch.c
index 7613d7988894..f673af4c77b6 100644
--- a/drivers/net/ethernet/sfc/siena/farch.c
+++ b/drivers/net/ethernet/sfc/siena/farch.c
@@ -723,7 +723,7 @@ int efx_farch_fini_dmaq(struct efx_nic *efx)
 	/* Do not attempt to write to the NIC during EEH recovery */
 	if (efx->state != STATE_RECOVERY) {
 		/* Only perform flush if DMA is enabled */
-		if (efx->pci_dev->is_busmaster) {
+		if (pci_dev_busmaster(efx->pci_dev)) {
 			efx->type->prepare_flush(efx);
 			rc = efx_farch_do_flush(efx);
 			efx->type->finish_flush(efx);
diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
index f36778e62ac1..412afa12a285 100644
--- a/drivers/pci/pci-driver.c
+++ b/drivers/pci/pci-driver.c
@@ -649,7 +649,7 @@ static int pci_pm_reenable_device(struct pci_dev *pci_dev)
 	 * if the device was busmaster before the suspend, make it busmaster
 	 * again
 	 */
-	if (pci_dev->is_busmaster)
+	if (pci_dev_busmaster(pci_dev))
 		pci_set_master(pci_dev);
 
 	return retval;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 77b17b13ee61..c4fd6fe6098d 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2045,7 +2045,7 @@ static void pci_enable_bridge(struct pci_dev *dev)
 		pci_enable_bridge(bridge);
 
 	if (pci_is_enabled(dev)) {
-		if (!dev->is_busmaster)
+		if (!pci_dev_busmaster(dev))
 			pci_set_master(dev);
 		return;
 	}
@@ -2205,7 +2205,7 @@ void pci_disable_device(struct pci_dev *dev)
 
 	do_pci_disable_device(dev);
 
-	dev->is_busmaster = 0;
+	pci_dev_assign_busmaster(dev, false);
 }
 EXPORT_SYMBOL(pci_disable_device);
 
@@ -4120,7 +4120,7 @@ static void __pci_set_master(struct pci_dev *dev, bool enable)
 			enable ? "enabling" : "disabling");
 		pci_write_config_word(dev, PCI_COMMAND, cmd);
 	}
-	dev->is_busmaster = enable;
+	pci_dev_assign_busmaster(dev, enable);
 }
 
 /**
diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index 82af59c913e9..08dc06e7dfc2 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -14398,7 +14398,7 @@ lpfc_io_slot_reset_s3(struct pci_dev *pdev)
 
 	pci_restore_state(pdev);
 
-	if (pdev->is_busmaster)
+	if (pci_dev_busmaster(pdev))
 		pci_set_master(pdev);
 
 	spin_lock_irq(&phba->hbalock);
@@ -15251,7 +15251,7 @@ lpfc_io_slot_reset_s4(struct pci_dev *pdev)
 	 */
 	pci_save_state(pdev);
 
-	if (pdev->is_busmaster)
+	if (pci_dev_busmaster(pdev))
 		pci_set_master(pdev);
 
 	spin_lock_irq(&phba->hbalock);
diff --git a/drivers/xen/xen-pciback/conf_space_header.c b/drivers/xen/xen-pciback/conf_space_header.c
index 8b50cbcbdfe1..59a89f915916 100644
--- a/drivers/xen/xen-pciback/conf_space_header.c
+++ b/drivers/xen/xen-pciback/conf_space_header.c
@@ -81,10 +81,10 @@ static int command_write(struct pci_dev *dev, int offset, u16 value, void *data)
 			dev_data->enable_intx = 0;
 	}
 
-	if (!dev->is_busmaster && is_master_cmd(value)) {
+	if (!pci_dev_busmaster(dev) && is_master_cmd(value)) {
 		dev_dbg(&dev->dev, "set bus master\n");
 		pci_set_master(dev);
-	} else if (dev->is_busmaster && !is_master_cmd(value)) {
+	} else if (pci_dev_busmaster(dev) && !is_master_cmd(value)) {
 		dev_dbg(&dev->dev, "clear bus master\n");
 		pci_clear_master(dev);
 	}
diff --git a/drivers/xen/xen-pciback/pciback_ops.c b/drivers/xen/xen-pciback/pciback_ops.c
index bfc186bf05bc..01f4705421c9 100644
--- a/drivers/xen/xen-pciback/pciback_ops.c
+++ b/drivers/xen/xen-pciback/pciback_ops.c
@@ -125,14 +125,14 @@ void xen_pcibk_reset_device(struct pci_dev *dev)
 		if (pci_is_enabled(dev))
 			pci_disable_device(dev);
 
-		dev->is_busmaster = 0;
+		pci_dev_assign_busmaster(dev, false);
 	} else {
 		pci_read_config_word(dev, PCI_COMMAND, &cmd);
 		if (cmd & (PCI_COMMAND_INVALIDATE)) {
 			cmd &= ~(PCI_COMMAND_INVALIDATE);
 			pci_write_config_word(dev, PCI_COMMAND, cmd);
 
-			dev->is_busmaster = 0;
+			pci_dev_assign_busmaster(dev, false);
 		}
 	}
 }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index ebb5b9d76360..9964646bdd46 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -336,6 +336,25 @@ struct pci_sriov;
 struct pci_p2pdma;
 struct rcec_ea;
 
+/**
+ * enum struct_pci_dev_flags - Flags in struct pci_dev
+ *
+ * Each flag has a set of accessor functions created via
+ * __create_pci_dev_flag_accessors() and must only be accessed through
+ * them.
+ *
+ * @PCI_DEV_FLAG_BUSMASTER: Bus mastering is enabled on the device. Pure
+ *		bookkeeping state, maintained by pci_set_master(),
+ *		pci_clear_master() and pci_disable_device(); modifying it
+ *		does not itself change the hardware state.
+ * @PCI_DEV_FLAG_COUNT: Number of defined struct_pci_dev_flags.
+ */
+enum struct_pci_dev_flags {
+	PCI_DEV_FLAG_BUSMASTER = 0,
+
+	PCI_DEV_FLAG_COUNT
+};
+
 /* struct pci_dev - describes a PCI device
  *
  * @supported_speeds:	PCIe Supported Link Speeds Vector (+ reserved 0 at
@@ -461,7 +480,6 @@ struct pci_dev {
 	unsigned int	pref_64_window:1;	/* Pref mem window is 64-bit */
 	unsigned int	multifunction:1;	/* Multi-function device */
 
-	unsigned int	is_busmaster:1;		/* Is busmaster */
 	unsigned int	no_msi:1;		/* May not use MSI */
 	unsigned int	block_cfg_access:1;	/* Config space access blocked */
 	unsigned int	broken_parity_status:1;	/* Generates false positive parity */
@@ -592,8 +610,25 @@ struct pci_dev {
 	u8		tph_mode;	/* TPH mode */
 	u8		tph_req_type;	/* TPH requester type */
 #endif
+
+	/* PCI_DEV_FLAG_XXX flags. Use atomic bitfield operations to modify. */
+	DECLARE_BITMAP(flags, PCI_DEV_FLAG_COUNT);
 };
 
+#define __create_pci_dev_flag_accessors(accessor_name, flag_name) \
+static inline bool pci_dev_##accessor_name(const struct pci_dev *pdev) \
+{ \
+	return test_bit(flag_name, pdev->flags); \
+} \
+static inline void pci_dev_assign_##accessor_name(struct pci_dev *pdev, bool value) \
+{ \
+	assign_bit(flag_name, pdev->flags, value); \
+}
+
+__create_pci_dev_flag_accessors(busmaster, PCI_DEV_FLAG_BUSMASTER);
+
+#undef __create_pci_dev_flag_accessors
+
 static inline struct pci_dev *pci_physfn(struct pci_dev *dev)
 {
 #ifdef CONFIG_PCI_IOV

-- 
2.51.2

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

[Lukas Wunner]

On Sat, Jul 11, 2026 at 05:21:06PM +0200, Maurice Hieronymus wrote:
> `is_busmaster` is one bit of a ~60-bit C bitfield in `struct pci_dev`.
> Bits sharing a bitfield word must not be modified concurrently, but its
> writers take no common lock: `pci_set_master()` can run without the
> device lock (e.g. from runtime PM resume paths), `pci_disable_device()`
> clears the bit, and other bits in the same word are written from
> entirely different contexts, e.g. `broken_parity_status` from sysfs.
> Concurrent read-modify-write cycles of the shared word can then lose
> updates.
> 
> Move `is_busmaster` into a new `flags` bitmap modified with atomic
> bitops and accessed through generated accessor functions, following the
> example of commit a7cc262a1135 ("driver core: Replace dev->offline +
> ->offline_disabled with accessors"). More bitfield flags can follow the
> same pattern later.

We already have the priv_flags member in struct pci_dev,
please use that instead of adding another one for the same purpose.

Thanks,

Lukas

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Concurrent read-modify-write of the hardware PCI_COMMAND register can lose updates.
--

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

PCI: Replace pci_dev->is_busmaster with accessors

This commit moves the is_busmaster bit from a shared bitfield into a
new flags bitmap using atomic bitops. This prevents concurrent
read-modify-write cycles on the shared software bitfield, explicitly
enabling safe concurrent usage of pci_disable_device() and pci_set_master().

Link: https://lore.kernel.org/rust-for-linux/DJOEYVBS17MJ.1YD3TNGQBWHNK@kernel.org/ [1]

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

[ ... ]

> @@ -2205,7 +2205,7 @@ void pci_disable_device(struct pci_dev *dev)
>  
>  	do_pci_disable_device(dev);
>  
> -	dev->is_busmaster = 0;
> +	pci_dev_assign_busmaster(dev, false);
>  }

[ ... ]

> @@ -4120,7 +4120,7 @@ static void __pci_set_master(struct pci_dev *dev, bool enable)
>  			enable ? "enabling" : "disabling");
>  		pci_write_config_word(dev, PCI_COMMAND, cmd);
>  	}
> -	dev->is_busmaster = enable;
> +	pci_dev_assign_busmaster(dev, enable);
>  }

[Severity: High]
This is a pre-existing issue, but since the commit message notes that
pci_set_master() and pci_disable_device() can run concurrently to justify
this change, can those concurrent calls also lose updates to the hardware
PCI_COMMAND register?

In __pci_set_master(), there is an unprotected read-modify-write cycle on
the hardware register:

	pci_read_config_word(dev, PCI_COMMAND, &old_cmd);
	if (enable)
		cmd = old_cmd | PCI_COMMAND_MASTER;
	else
		cmd = old_cmd & ~PCI_COMMAND_MASTER;
	if (cmd != old_cmd) {
		pci_dbg(dev, "%s bus mastering\n",
			enable ? "enabling" : "disabling");
		pci_write_config_word(dev, PCI_COMMAND, cmd);
	}

Similarly, do_pci_disable_device() (called just before the change in
pci_disable_device() above) performs its own unprotected update:

	pci_read_config_word(dev, PCI_COMMAND, &pci_command);
	if (pci_command & PCI_COMMAND_MASTER) {
		pci_command &= ~PCI_COMMAND_MASTER;
		pci_write_config_word(dev, PCI_COMMAND, pci_command);
	}

If a Rust guard object calls pci_disable_device() concurrently with
pci_set_master(), could we accidentally clear or set other critical flags
like PCI_COMMAND_INTX_DISABLE or PCI_COMMAND_MEMORY due to the hardware
read-modify-write race?

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

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

[Maurice Hieronymus]

`broken_parity_status` shares a C bitfield word in `struct pci_dev`
with many other bits. `broken_parity_status_store()` writes it from
sysfs at any time without taking any lock, so userspace can make it
race with every other writer of the same word, e.g. `pci_set_master()`
from a runtime PM resume path, and updates of neighboring bits can be
lost.

Move the bit into the `flags` bitmap modified with atomic bitops,
using the accessor pattern introduced by the previous commit.

Signed-off-by: Maurice Hieronymus <mhi@mailbox.org>
---
 drivers/edac/edac_pci_sysfs.c | 4 ++--
 drivers/pci/pci-sysfs.c       | 4 ++--
 include/linux/pci.h           | 5 ++++-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/edac/edac_pci_sysfs.c b/drivers/edac/edac_pci_sysfs.c
index 9f437f648e4e..fadc61235f1f 100644
--- a/drivers/edac/edac_pci_sysfs.c
+++ b/drivers/edac/edac_pci_sysfs.c
@@ -554,7 +554,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
 	/* check the status reg for errors on boards NOT marked as broken
 	 * if broken, we cannot trust any of the status bits
 	 */
-	if (status && !dev->broken_parity_status) {
+	if (status && !pci_dev_broken_parity_status(dev)) {
 		if (status & (PCI_STATUS_SIG_SYSTEM_ERROR)) {
 			edac_printk(KERN_CRIT, EDAC_PCI,
 				"Signaled System Error on %s\n",
@@ -593,7 +593,7 @@ static void edac_pci_dev_parity_test(struct pci_dev *dev)
 		/* check the secondary status reg for errors,
 		 * on NOT broken boards
 		 */
-		if (status && !dev->broken_parity_status) {
+		if (status && !pci_dev_broken_parity_status(dev)) {
 			if (status & (PCI_STATUS_SIG_SYSTEM_ERROR)) {
 				edac_printk(KERN_CRIT, EDAC_PCI, "Bridge "
 					"Signaled System Error on %s\n",
diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 5ec0b245a69b..5e094d1e23e3 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -80,7 +80,7 @@ static ssize_t broken_parity_status_show(struct device *dev,
 					 char *buf)
 {
 	struct pci_dev *pdev = to_pci_dev(dev);
-	return sysfs_emit(buf, "%u\n", pdev->broken_parity_status);
+	return sysfs_emit(buf, "%u\n", pci_dev_broken_parity_status(pdev));
 }
 
 static ssize_t broken_parity_status_store(struct device *dev,
@@ -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);
 
 	return count;
 }
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 9964646bdd46..fdcd9b1b7371 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -347,10 +347,13 @@ struct rcec_ea;
  *		bookkeeping state, maintained by pci_set_master(),
  *		pci_clear_master() and pci_disable_device(); modifying it
  *		does not itself change the hardware state.
+ * @PCI_DEV_FLAG_BROKEN_PARITY_STATUS: Device generates false positive
+ *		parity errors; set via sysfs.
  * @PCI_DEV_FLAG_COUNT: Number of defined struct_pci_dev_flags.
  */
 enum struct_pci_dev_flags {
 	PCI_DEV_FLAG_BUSMASTER = 0,
+	PCI_DEV_FLAG_BROKEN_PARITY_STATUS = 1,
 
 	PCI_DEV_FLAG_COUNT
 };
@@ -482,7 +485,6 @@ struct pci_dev {
 
 	unsigned int	no_msi:1;		/* May not use MSI */
 	unsigned int	block_cfg_access:1;	/* Config space access blocked */
-	unsigned int	broken_parity_status:1;	/* Generates false positive parity */
 	unsigned int	irq_reroute_variant:2;	/* Needs IRQ rerouting variant */
 	unsigned int	msi_enabled:1;
 	unsigned int	msix_enabled:1;
@@ -626,6 +628,7 @@ static inline void pci_dev_assign_##accessor_name(struct pci_dev *pdev, bool val
 }
 
 __create_pci_dev_flag_accessors(busmaster, PCI_DEV_FLAG_BUSMASTER);
+__create_pci_dev_flag_accessors(broken_parity_status, PCI_DEV_FLAG_BROKEN_PARITY_STATUS);
 
 #undef __create_pci_dev_flag_accessors
 

-- 
2.51.2

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

[Lukas Wunner]

On Sat, Jul 11, 2026 at 05:21:07PM +0200, Maurice Hieronymus wrote:
> `broken_parity_status` shares a C bitfield word in `struct pci_dev`
> with many other bits. `broken_parity_status_store()` writes it from
> sysfs at any time without taking any lock, so userspace can make it
> race with every other writer of the same word, e.g. `pci_set_master()`
> from a runtime PM resume path, and updates of neighboring bits can be
> lost.

For static bits in struct pci_dev, i.e. ones that are mostly read
and almost never written, and in particular ones that are only
written on device enumeration, it's perfectly fine and more convenient
to keep them as bitfields.  broken_parity_status seems to fit that bill.

For other bits which are modified more frequently, move them to the existing
priv_flags member if you believe they can be updated concurrently.
I'm not sure is_busmaster fits that bill, it isn't updated that often.

Quite honestly I'm wondering if there is anything to fix here.
Yes I get it, userspace may interfere with adjacent bits.
But broken_parity_status is only used for certain broken devices
on EDAC-capable platforms.  That's a fringe use case.
Is it really worth refactoring this?

Perhaps a better approach is to enclose dev_attr_broken_parity_status.attr
in "#ifdef CONFIG_EDAC" so that the attribute isn't shown unless it's used.

We shouldn't have used a sysfs attribute for this in the first place
but rather a quirk.  Unfortunately 6b09ff9d7879 does not betray for which
device this was needed, so it's difficult to convert it to a quirk now.

Bjorn introduced a pci_disable_parity() API in 2021 which is used in a
quirk for certain Mellanox products:

https://lore.kernel.org/all/20210330174318.1289680-1-helgaas@kernel.org/

Perhaps we can deprecate the sysfs attribute in favor of using quirks
for broken devices?

Thanks,

Lukas

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

[sashiko-bot]

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