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

[David Matlack <dmatlack@google.com>]

On 2026-05-14 02:37 AM, sashiko-bot@kernel.org wrote:
> 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?

Yes looks like it, I will account for Intel SPT PCH root ports in v6.

> 
> [ ... ]
> > @@ -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?

Yes, will fix.

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