Re: [PATCH 1/2] PCI/CXL: Allow PM Init to complete on cxl_bus reset if ACS SV enabled

[Dave Jiang <dave.jiang@intel.com>]

On 4/28/26 11:24 AM, Fabio M. De Francesco wrote:
> CXL r4.0 sec 8.1.5.1 Implementation Note describes a scenario in which
> issuing a Secondary Bus Reset on a CXL Downstream Port leaves the
> Port Power Management Initialization Complete bit unset when the PCIe
> Access Control Service (ACS) Source Validation bit (SV) is enabled on
> the Downstream Port. The spec states that another SBR alone will not
> facilitate recovery and shows a software recovery sequence.
> 
> Implement the sequence by extending cxl_reset_bus_function() to save,
> clear, and restore ACS SV and Bus Master Enable (BME) on the Downstream
> Port around the SBR with the use of helpers.
> 
> The wait inside pci_bridge_secondary_bus_reset() covers the 100 ms
> referenced by the spec. The helpers return when ACS SV is not enabled on
> the Downstream Port.
> 
> Signed-off-by: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>
> ---
>  drivers/pci/pci.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 8f7cfcc00090..047d3b4508a5 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4930,10 +4930,55 @@ static int pci_reset_bus_function(struct pci_dev *dev, bool probe)
>  	return rc;
>  }
>  
> +static void cxl_disable_acs_sv_bme(struct pci_dev *bridge, u16 *saved_cmd,
> +				   u16 *saved_acs_ctrl)

Maybe you can call this 'cxl_bus_reset_prep' and the restore one 'cxl_bus_reset_done'?

> +{

Should you return int and check if the two ptr passed in are valid?

> +	if (!bridge->acs_cap)
> +		return;

Returning here and the output parameters are not set. That is a problem when you try to restore with invalid values. Maybe -EOPNOTSUPP needs to be returned. Errno from this function should prevent the restore from being called.

> +
> +	pci_read_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL,
> +			     saved_acs_ctrl);
> +	if (!(*saved_acs_ctrl & PCI_ACS_SV))
> +		return;

What is the caller suppose to do if PCI_ACS_SV is not set? Should you skip restore later?

> +
> +	pci_read_config_word(bridge, PCI_COMMAND, saved_cmd);
> +	if (*saved_cmd & PCI_COMMAND_MASTER)

If all you care is master bit enabled, maybe just write back a bool 'master_en'.

> +		pci_clear_master(bridge);
> +
> +	pci_write_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL,
> +			      *saved_acs_ctrl & ~PCI_ACS_SV);
> +}
> +
> +static void cxl_restore_acs_sv_bme(struct pci_dev *bridge, u16 saved_cmd,
> +				   u16 saved_acs_ctrl)

static void cxl_bus_reset_done(struct pci_dev *bridge, bool master_en, u16 acs_ctrl)

saved_ has no relevance to the function itself.

Come to think of it, why not create a local struct and pass that in as parameter for prep and done functions? That way in the future if there are other stuff that needs to be done, this can be more versatile.

struct cxl_reset_ctx {
	u16 acs_ctrl;
	bool master_en;
	bool acs_restore;
};

This way you can also set 'acs_restore' on completion of the prep function. And in the done function, you can check and return early if it's not set. That takes away the messiness of returning errno in the prep function and then needing to determine if you need to call the prep function or not.

DJ

> +{
> +	if (!bridge->acs_cap || !(saved_acs_ctrl & PCI_ACS_SV))
> +		return;
> +
> +	pci_write_config_word(bridge, bridge->acs_cap + PCI_ACS_CTRL,
> +			      saved_acs_ctrl);
> +	if (saved_cmd & PCI_COMMAND_MASTER)
> +		pci_set_master(bridge);
> +}
> +
> +/**
> + * cxl_reset_bus_function - SBR for a child of a CXL downstream port
> + * @dev: child device whose upstream bridge is a CXL downstream port
> + * @probe: if true, only check whether the reset is supported
> + *
> + * Issues an SBR on @dev's parent bus. Temporarily sets the CXL Port
> + * DVSEC Unmask SBR bit across the reset. When ACS Source Validation
> + * is enabled on the bridge, also temporarily clears Bus Master Enable
> + * and ACS Source Validation, per CXL r4.0 sec 8.1.5.1.
> + *
> + * Return: 0 on success, -ENOTTY if the reset cannot be issued, or an
> + * errno from the reset path.
> + */
>  static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
>  {
>  	struct pci_dev *bridge;
>  	u16 dvsec, reg, val;
> +	u16 saved_cmd = 0, saved_acs_ctrl = 0;
>  	int rc;
>  
>  	bridge = pci_upstream_bridge(dev);
> @@ -4957,6 +5002,8 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
>  		return rc;
>  	}
>  
> +	cxl_disable_acs_sv_bme(bridge, &saved_cmd, &saved_acs_ctrl);
> +
>  	if (reg & PCI_DVSEC_CXL_PORT_CTL_UNMASK_SBR) {
>  		val = reg;
>  	} else {
> @@ -4971,6 +5018,8 @@ static int cxl_reset_bus_function(struct pci_dev *dev, bool probe)
>  		pci_write_config_word(bridge, dvsec + PCI_DVSEC_CXL_PORT_CTL,
>  				      reg);
>  
> +	cxl_restore_acs_sv_bme(bridge, saved_cmd, saved_acs_ctrl);
> +
>  	pci_dev_reset_iommu_done(dev);
>  	return rc;
>  }