Re: [PATCH v2 10/13] PCI/CXL: Clear ACS SV across an SBR of a CXL DPort

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unsynchronized access to the global `cxl_sbr_region_ops` struct pointer can result in a NULL pointer dereference or a Use-After-Free crash if the CXL core module unloads concurrently.
- [High] Missing link training wait for nested bridges during subordinate configuration space restoration.
- [High] Restoring the standard configuration space of subordinate devices re-enables Bus Master and Memory Space before their PCIe capabilities are restored, risking Malformed TLP fatal errors.
- [High] The completion function attempts to access the configuration space and memory regions of downstream devices even if the secondary bus reset fails.
- [Medium] The polling loop in `cxl_port_pm_init_is_complete()` can miss a successful completion if scheduling delays cause `msleep()` to overshoot the timeout.
--

--- Patch [10]: [PATCH v2 10/13] PCI/CXL: Clear ACS SV across an SBR of a CXL DPort ---
commit fae09a9aaf20d6e342f823e3752512b2c39cdd95
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

PCI/CXL: Clear ACS SV across an SBR of a CXL DPort

A Secondary Bus Reset (SBR) of a CXL Downstream Port clears the
Downstream Component's (DPort) captured Bus Number. This patch
clears ACS Source Validation and Bus Master Enable across the reset
so that Port Power Management Initialization completes at link-up.

> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 417b6b44473e5..f3781d1e8f6e1 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
[ ... ]
> +static int cxl_sbr_prepare(struct pci_dev *bridge, u16 dvsec,
> +			   struct cxl_sbr_ctx *ctx)
> +{
> +	int rc;
> +
> +	/* Abort before touching hardware if the regions cannot be disabled. */
> +	if (cxl_sbr_region_ops) {
> +		rc = cxl_sbr_region_ops->disable_regions(bridge);
> +		if (rc)
> +			return rc;
> +	}

[Severity: High]
Could accessing cxl_sbr_region_ops here result in a use-after-free or NULL
pointer dereference?

If a secondary bus reset is initiated via sysfs concurrently with the
cxl_core module unloading, the pointer might be cleared or freed before
disable_regions() is called. There does not appear to be any lock or RCU
protection here.

[ ... ]
> +static bool cxl_port_pm_init_is_complete(struct pci_dev *bridge, u16 dvsec)
> +{
> +	unsigned long start = jiffies;
> +	unsigned long timeout = start + msecs_to_jiffies(100);
> +	u16 status;
> +
> +	do {
> +		pci_read_config_word(bridge,
> +				     dvsec + PCI_DVSEC_CXL_PORT_EXT_STATUS,
> +				     &status);
> +		if (!PCI_POSSIBLE_ERROR(status) &&
> +		    (status & PCI_DVSEC_CXL_PORT_EXT_STATUS_PM_INIT_COMP)) {
> +			pci_dbg(bridge, "%s: PM Init Complete set after %u ms, ext status %#06x\n",
> +				__func__, jiffies_to_msecs(jiffies - start), status);
> +			return true;
> +		}
> +		msleep(10);
> +	} while (time_before(jiffies, timeout));
> +
> +	pci_warn(bridge, "%s: PM Init Complete not set after %u ms, ext status %#06x\n",

[Severity: Medium]
Will this loop miss successful completions if scheduling delays cause
msleep() to overshoot the timeout?

If the sleep significantly exceeds 10ms and crosses the 100ms timeout
boundary, the loop terminates and returns false without a final status
read. This might lead to spurious warnings and premature restoration of
ACS Source Validation, causing the Port to reject bus 0 traffic.

[ ... ]
> +static int cxl_sbr_restore_config_space(struct pci_dev *dev, void *userdata)
> +{
> +	pci_restore_config_space(dev);
> +	pci_dbg(dev, "%s: config space restored\n", __func__);
> +
> +	return 0;
> +}

[Severity: High]
Does this unconditionally re-enable Bus Master and Memory Space before
PCIe capabilities are restored?

When pci_restore_config_space() writes the first 64 bytes, it restores
PCI_COMMAND while the Max Payload Size (MPS) remains at its hardware
reset default of 128 bytes. This might trigger Malformed TLP fatal errors
if the device interacts with a host configured for a larger MPS during
this window.

[ ... ]
> +static void cxl_sbr_restore_subordinate(struct pci_dev *bridge)
> +{
> +	if (!bridge->subordinate)
> +		return;
> +
> +	/* Parents before children: a child answers once its parent forwards. */
> +	pci_walk_bus(bridge->subordinate, cxl_sbr_restore_config_space, NULL);
> +}

[Severity: High]
Does pci_walk_bus() safely handle nested PCI-to-PCI bridges on the
subordinate bus?

It seems to restore the nested bridge's standard configuration and then
immediately traverse to the grandchildren without waiting for the nested
bridge's secondary link to retrain. This means configuration writes might
be sent to deeper endpoints while their link is still down.

[ ... ]
>  int pci_bridge_secondary_bus_reset(struct pci_dev *dev)
>  {
> +	struct cxl_sbr_ctx ctx = {};
> +	u16 dvsec = 0;
> +	int rc;
> +
>  	if (!dev->block_cfg_access)
>  		pci_warn_once(dev, "unlocked secondary bus reset via: %pS\n",
>  			      __builtin_return_address(0));
> +
> +	if (is_cxl_dport(dev))
> +		dvsec = cxl_port_dvsec(dev);
> +	if (dvsec) {
> +		rc = cxl_sbr_prepare(dev, dvsec, &ctx);
> +		if (rc)
> +			return rc;
> +	}
> +
>  	pcibios_reset_secondary_bus(dev);
>  
> -	return pci_bridge_wait_for_secondary_bus(dev, "bus reset");
> +	rc = pci_bridge_wait_for_secondary_bus(dev, "bus reset");
> +
> +	if (dvsec)
> +		cxl_sbr_complete(dev, dvsec, &ctx);
> +
> +	return rc;
>  }

[Severity: High]
Is it safe to call cxl_sbr_complete() when
pci_bridge_wait_for_secondary_bus() times out or returns a non-zero error?

Accessing the configuration space and memory regions of downstream devices
while the secondary link is physically down might generate uncorrectable
PCIe AER errors like Unsupported Requests or Completion Timeouts.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825022628.3651434-1-fabio.m.de.francesco@linux.intel.com?part=10