Re: [PATCH v5 07/11] PCI: liveupdate: Inherit ACS flags in incoming preserved devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Modifying pci_dev state while holding only a read lock in pci_liveupdate_init_acs().
- [High] Intel SPT PCH root ports bypass the ACS quirk rejection check, causing their custom ACS control registers to be permanently lost on the new kernel.
--

commit 9befbb904977419650ee202e0854fc1452e0a9de
Author: David Matlack <dmatlack@google.com>

PCI: liveupdate: Inherit ACS flags in incoming preserved devices

This commit updates the live update process to inherit Access Control
Services (ACS) flags on incoming preserved devices. It caches the
inherited ACS controls established by the previous kernel so they do
not change after a reset.

> diff --git a/drivers/pci/liveupdate.c b/drivers/pci/liveupdate.c
> index d8e06afde2c76..e3cd6d76636c3 100644
> --- a/drivers/pci/liveupdate.c
> +++ b/drivers/pci/liveupdate.c
[ ... ]
> @@ -361,6 +373,16 @@ static int pci_liveupdate_preserve_device_new(struct pci_ser *ser, struct pci_de
>  {
>  	int i;
>  
> +	/*
> +	 * Do not preserve a devices that rely on device-specific ACS
> +	 * equivalents (for now) since that would complicate keeping ACS
> +	 * flags constant across Live Update.
> +	 */
> +	if (dev->dev_flags & PCI_DEV_FLAGS_ACS_ENABLED_QUIRK) {
> +		pci_warn(dev, "Refusing to preserve device that relies on ACS quirks\n");
> +		return -EINVAL;
> +	}

Can Intel SPT PCH root ports bypass this check?

Looking at pci_quirk_enable_intel_spt_pch_acs(), it does not appear to set
the PCI_DEV_FLAGS_ACS_ENABLED_QUIRK flag.

If these ports bypass the check and are preserved, pci_liveupdate_init_acs()
will only cache the standard PCI_ACS_CTRL register, ignoring the
device-specific INTEL_SPT_ACS_CTRL register.

When the new kernel calls pci_enable_acs(), pci_liveupdate_inherit_acs()
will restore the standard register and return true, skipping the
pci_dev_specific_enable_acs() path entirely.

Will this cause a regression where the custom ACS configuration is lost?

[ ... ]
> @@ -671,6 +694,32 @@ void pci_liveupdate_finish(struct pci_dev *dev)
>  }
>  EXPORT_SYMBOL_GPL(pci_liveupdate_finish);
>  
> +void pci_liveupdate_init_acs(struct pci_dev *dev)
> +{
> +	guard(read_lock)(&dev->liveupdate.lock);
> +
> +	if (!dev->acs_cap || !dev->liveupdate.incoming)
> +		return;
> +
> +	pci_read_config_word(dev, dev->acs_cap + PCI_ACS_CTRL, &dev->liveupdate.acs_ctrl);
> +}

Does this mutate dev->liveupdate.acs_ctrl while holding a read lock?

Since pci_read_config_word() writes the read value into the acs_ctrl
pointer, should this use write_lock to avoid a data race regression?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512184846.119396-1-dmatlack@google.com?part=7