Re: [PATCH v8 08/12] PCI: liveupdate: Adopt ACS controls in incoming preserved devices

[Bjorn Helgaas]

On Mon, Sep 14, 2026 at 04:45:58PM +0000, David Matlack wrote:
> On 2026-09-11 06:31 PM, David Matlack wrote:
> > On 2026-09-10 06:51 PM, Bjorn Helgaas wrote:
> > 
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index 77b17b13ee61..22001bdf4c97 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -34,6 +34,8 @@
> > > >  #include <linux/aer.h>
> > > >  #include <linux/bitfield.h>
> > > >  #include <linux/suspend.h>
> > > > +
> > > > +#include "liveupdate.h"
> > > >  #include "pci.h"
> > > >  
> > > >  DEFINE_MUTEX(pci_slot_mutex);
> > > > @@ -1008,6 +1010,9 @@ void pci_enable_acs(struct pci_dev *dev)
> > > >  	bool enable_acs = false;
> > > >  	int pos;
> > > >  
> > > > +	if (!pci_liveupdate_enable_adopted_acs_controls(dev))
> > > > +		return;
> > > 
> > > Ugh.  The asymmetry between pci_save_state(), which does nothing
> > > ACS-related, and pci_restore_state(), which enables it, is sort of
> > > sketchy to begin with.
> > > 
> > > The command-line parsing in this path feels like kind of a wart (not
> > > that you're touching it).
> > > 
> > > It just seems like this path is already hard to analyze, and
> > > liveupdate is making it harder.
> > > 
> > > If we could save/restore the ACS state around the reset, wouldn't that
> > > solve this without any liveupdate specials here?
> > 
> > Yeah that would simplify the liveupdate support greatly. Let me work on
> > that for v9.
> 
> Here's the patch I have prepped for v9 to save/restore ACS controls
> around reset:
> 
> From 0c69424454de0c569aca67bc69f496db915a9bcf Mon Sep 17 00:00:00 2001
> From: David Matlack <dmatlack@xxxxxxxxxx>
> Date: Fri, 11 Sep 2026 20:05:04 +0000
> Subject: [PATCH] PCI: Save and restore the ACS Control register
> 
> Save the ACS Control register in pci_save_state() and write it back
> in pci_restore_state(), instead of recomputing the ACS controls from
> scratch with pci_enable_acs().
> 
> This makes ACS symmetric with the rest of a device's saved state. Today
> pci_save_state() ignores ACS entirely and pci_restore_state() re-enables
> the ACS controls from the kernel's current ACS policy. As a result, a
> device can come out of a reset with different ACS controls than it went
> in with, e.g. any controls programmed outside of pci_enable_acs() are
> silently dropped.
> 
> pci_enable_acs() runs when a driver binds to a device
> (pci_dma_configure()), i.e. after pci_bus_add_device() has already saved
> the device's state. Refresh the saved ACS Control register there as
> well, otherwise a subsequent reset would revert ACS back to the
> configuration left behind by firmware.
> 
> Devices that rely on device-specific quirks to enable an ACS equivalent
> keep that configuration outside of the ACS Control register, so keep
> configuring ACS from scratch for them. Do the same for devices that have
> no saved ACS state at all.
> 
> Assisted-by: Claude:claude-opus-5
> Signed-off-by: David Matlack <dmatlack@xxxxxxxxxx>

I like this a lot, thanks!  It would be good to get Alex W's ack, too.

Reviewed-by: Bjorn Helgaas <bhelgaas@xxxxxxxxxx>

> ---
>  drivers/pci/pci.c    | 66 +++++++++++++++++++++++++++++++++++++++++++-
>  drivers/pci/pci.h    |  5 ++++
>  drivers/pci/quirks.c |  7 +++++
>  3 files changed, 77 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index b2879a6be5f8..dd25c01736b4 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -1021,6 +1021,55 @@ static void pci_std_enable_acs(struct pci_dev *dev, struct pci_acs *caps)
>  		caps->ctrl |= (dev->acs_capabilities & PCI_ACS_TB);
>  }
>  
> +/**
> + * pci_save_acs_state - save the ACS Control register
> + * @dev: the PCI device
> + *
> + * Record the ACS controls currently programmed in hardware so that
> + * pci_restore_acs_state() can reapply them after a reset.
> + */
> +static void pci_save_acs_state(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state;
> +
> +	if (!dev->acs_cap)
> +		return;
> +
> +	save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ACS);
> +	if (!save_state)
> +		return;
> +
> +	pci_read_config_word(dev, dev->acs_cap + PCI_ACS_CTRL,
> +			     (u16 *)&save_state->cap.data[0]);
> +}
> +
> +/**
> + * pci_restore_acs_state - restore the ACS Control register
> + * @dev: the PCI device
> + */
> +static void pci_restore_acs_state(struct pci_dev *dev)
> +{
> +	struct pci_cap_saved_state *save_state = NULL;
> +
> +	if (dev->acs_cap && !pci_need_dev_specific_enable_acs(dev))
> +		save_state = pci_find_saved_ext_cap(dev, PCI_EXT_CAP_ID_ACS);
> +
> +	/*
> +	 * Devices that rely on device-specific quirks to enable an ACS
> +	 * equivalent keep that configuration outside of the ACS Control
> +	 * register, so there is nothing useful to restore for them. Configure
> +	 * ACS from scratch instead, which also covers devices that have no
> +	 * saved ACS state at all.
> +	 */
> +	if (!save_state) {
> +		pci_enable_acs(dev);
> +		return;
> +	}
> +
> +	pci_write_config_word(dev, dev->acs_cap + PCI_ACS_CTRL,
> +			      *(u16 *)&save_state->cap.data[0]);
> +}
> +
>  /**
>   * pci_enable_acs - enable ACS if hardware support it
>   * @dev: the PCI device
> @@ -1057,6 +1106,15 @@ void pci_enable_acs(struct pci_dev *dev)
>  	__pci_config_acs(dev, &caps, config_acs_param, 0, 0);
>  
>  	pci_write_config_word(dev, pos + PCI_ACS_CTRL, caps.ctrl);
> +
> +	/*
> +	 * pci_enable_acs() runs when a driver binds to the device, i.e. after
> +	 * pci_bus_add_device() has already saved the device's state. Refresh
> +	 * the saved ACS Control register so that a subsequent reset restores
> +	 * the controls programmed here rather than the ones left behind by
> +	 * firmware.
> +	 */
> +	pci_save_acs_state(dev);
>  }
>  
>  /**
> @@ -1800,6 +1858,7 @@ int pci_save_state(struct pci_dev *dev)
>  	pci_save_aer_state(dev);
>  	pci_save_ptm_state(dev);
>  	pci_save_tph_state(dev);
> +	pci_save_acs_state(dev);
>  	return pci_save_vc_state(dev);
>  }
>  EXPORT_SYMBOL(pci_save_state);
> @@ -1877,7 +1936,7 @@ void pci_restore_state(struct pci_dev *dev)
>  	pci_restore_msi_state(dev);
>  
>  	/* Restore ACS and IOV configuration state */
> -	pci_enable_acs(dev);
> +	pci_restore_acs_state(dev);
>  	pci_restore_iov_state(dev);
>  
>  	dev->state_saved = false;
> @@ -3532,6 +3591,11 @@ void pci_allocate_cap_save_buffers(struct pci_dev *dev)
>  	if (error)
>  		pci_err(dev, "unable to allocate suspend buffer for LTR\n");
>  
> +	error = pci_add_ext_cap_save_buffer(dev, PCI_EXT_CAP_ID_ACS,
> +					    sizeof(u16));
> +	if (error)
> +		pci_err(dev, "unable to allocate suspend buffer for ACS\n");
> +
>  	pci_allocate_vc_save_buffers(dev);
>  }
>  
> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
> index ba3c3fddddc2..037c1674f164 100644
> --- a/drivers/pci/pci.h
> +++ b/drivers/pci/pci.h
> @@ -1095,6 +1095,7 @@ void pci_acs_init(struct pci_dev *dev);
>  void pci_enable_acs(struct pci_dev *dev);
>  #ifdef CONFIG_PCI_QUIRKS
>  int pci_dev_specific_acs_enabled(struct pci_dev *dev, u16 acs_flags);
> +bool pci_need_dev_specific_enable_acs(struct pci_dev *dev);
>  int pci_dev_specific_enable_acs(struct pci_dev *dev);
>  int pci_dev_specific_disable_acs_redir(struct pci_dev *dev);
>  void pci_disable_broken_acs_cap(struct pci_dev *pdev);
> @@ -1105,6 +1106,10 @@ static inline int pci_dev_specific_acs_enabled(struct pci_dev *dev,
>  {
>  	return -ENOTTY;
>  }
> +static inline bool pci_need_dev_specific_enable_acs(struct pci_dev *dev)
> +{
> +	return false;
> +}
>  static inline int pci_dev_specific_enable_acs(struct pci_dev *dev)
>  {
>  	return -ENOTTY;
> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c
> index 7aee30734303..e500c202d2ec 100644
> --- a/drivers/pci/quirks.c
> +++ b/drivers/pci/quirks.c
> @@ -5476,6 +5476,13 @@ static const struct pci_dev_acs_ops *pci_dev_acs_ops_get(struct pci_dev *dev)
>  	return NULL;
>  }
>  
> +bool pci_need_dev_specific_enable_acs(struct pci_dev *dev)
> +{
> +	const struct pci_dev_acs_ops *p = pci_dev_acs_ops_get(dev);
> +
> +	return p && p->enable_acs;
> +}
> +
>  int pci_dev_specific_enable_acs(struct pci_dev *dev)
>  {
>  	const struct pci_dev_acs_ops *p = pci_dev_acs_ops_get(dev);