Re: [PATCH v2 09/10] PCI/LUO: Avoid write to bus master at boot

[Bjorn Helgaas <helgaas@kernel.org>]

On Tue, Sep 16, 2025 at 12:45:17AM -0700, Chris Li wrote:
> If the liveupdate flag has LU_BUSMASTER or LU_BUSMASTER_BRIDGE, the
> device is participating in the liveupdate preserving bus master bit in the
> PCI config space command register.
> 
> Avoid writing to the PCI command register for the bus master bit during
> boot up.
> 
> Signed-off-by: Chris Li <chrisl@kernel.org>
> ---
>  drivers/pci/liveupdate.c | 6 ++++++
>  drivers/pci/pci.c        | 7 +++++--
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pci/liveupdate.c b/drivers/pci/liveupdate.c
> index 1b12fc0649f479c6f45ffb26e6e3754f41054ea8..a09a166b6ee271b96bce763716c3b62b24f3edbb 100644
> --- a/drivers/pci/liveupdate.c
> +++ b/drivers/pci/liveupdate.c
> @@ -377,6 +377,12 @@ static void pci_dev_do_restore(struct pci_dev *dev, struct pci_dev_ser *s)
>  	pci_info(dev, "liveupdate restore flags %x driver: %s data: [%llx]\n",
>  		 s->flags, s->driver_name, s->driver_data);
>  	list_move_tail(&dev->dev.lu.lu_next, &probe_devices);
> +	if (s->flags & (LU_BUSMASTER | LU_BUSMASTER_BRIDGE)) {
> +		u16 pci_command;
> +
> +		pci_read_config_word(dev, PCI_COMMAND, &pci_command);
> +		WARN_ON(!(pci_command & PCI_COMMAND_MASTER));
> +	}
>  }
>  
>  void pci_liveupdate_restore(struct pci_dev *dev)
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 9e42090fb108920995ebe34bd2535a0e23fef7fd..2339ac1bd57616a78d2105ba3a4fc72bbf49973e 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -2248,7 +2248,8 @@ static void do_pci_disable_device(struct pci_dev *dev)
>  	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 (!(dev->dev.lu.flags & (LU_BUSMASTER | LU_BUSMASTER_BRIDGE)))
> +			pci_write_config_word(dev, PCI_COMMAND, pci_command);

I think changing the semantics of interfaces like this is a problem
because callers rely on the existing semantics, and it's hard to
reason about how this change would affect them.  How would you update
the kernel-doc to reflect this change?

do_pci_disable_device() is used in the PM suspend, freeze, and
poweroff paths.  I suppose those paths are allowed even when devices
have been marked with LU_BUSMASTER/LU_BUSMASTER_BRIDGE?  And I assume
you probably would want the existing semantics there?

I.e., if a device has been marked with LU_BUSMASTER, you want to keep
its bus mastering enabled across a liveupdate kexec.  But if we
suspend before doing the kexec, I assume we would still want to clear
bus mastering on suspend and restore bus mastering on resume?

The other path that uses do_pci_disable_device() is
pci_disable_device(), which is primarily used in driver .remove()
methods.  You have to modify drivers to support liveupdate anyway, so
if we call driver .remove() methods during a liveupdate kexec, I think
you should change the .remove() method so it only calls
pci_disable_device() when you want bus mastering disabled.

>  	}
>  
>  	pcibios_disable_device(dev);
> @@ -4276,7 +4277,9 @@ static void __pci_set_master(struct pci_dev *dev, bool enable)
>  	if (cmd != old_cmd) {
>  		pci_dbg(dev, "%s bus mastering\n",
>  			enable ? "enabling" : "disabling");
> -		pci_write_config_word(dev, PCI_COMMAND, cmd);
> +
> +		if (!(dev->dev.lu.flags & (LU_BUSMASTER | LU_BUSMASTER_BRIDGE)))
> +			pci_write_config_word(dev, PCI_COMMAND, cmd);
>  	}
>  	dev->is_busmaster = enable;
>  }
> 
> -- 
> 2.51.0.384.g4c02a37b29-goog
>