Re: [PATCH 1/2] PCI: Replace pci_dev->is_busmaster 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] 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