Re: [PATCH v2 09/13] PCI/CXL: Disable and re-enable CXL regions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] `cxl_sbr_enable_regions()` silently ignores the return value of `cxl_sbr_collect_regions()`, leading to permanently disabled regions on allocation failure.
- [High] The global `cxl_sbr_region_ops` pointer can dangle if `cxl_driver_register()` fails, and lacks module pinning to protect against concurrent unloads.
- [High] `cxl_sbr_enable_regions()` unconditionally re-enables regions even if HDM decoder restoration fails.
- [Medium] The recommit path incorrectly treats legitimately missing HDM state (e.g., ports without devices, or emulated decoders) as a failure or logs spurious warnings.
--

--- Patch [9]: [PATCH v2 09/13] PCI/CXL: Disable and re-enable CXL regions ---
commit cfc12109c423de14f05ed1c759bffe4684c3373e
Author: Fabio M. De Francesco <fabio.m.de.francesco@linux.intel.com>

PCI/CXL: Disable and re-enable CXL regions

A Secondary Bus Reset of a CXL Downstream Port needs every CXL region
routed through that Port disabled for the duration. This patch adds 
callbacks to collect and disable those regions before the reset, and 
subsequently restore HDM decoders and re-enable the regions afterward.

> diff --git a/drivers/cxl/core/dport_sbr.c b/drivers/cxl/core/dport_sbr.c
> index 233499bc1fad9..2b6f840e22a7d 100644
> --- a/drivers/cxl/core/dport_sbr.c
> +++ b/drivers/cxl/core/dport_sbr.c
[ ... ]
> +static void cxl_sbr_enable_regions(struct pci_dev *dport_pci)
> +{
> +	struct xarray *hdm_state;
> +	struct cxl_region *cxlr;
> +	struct xarray regions;
> +	unsigned long index;
> +
> +	xa_init(&regions);
> +
> +	cxl_sbr_collect_regions(dport_pci, &regions);

[Severity: High]
Does this code need to check the return value of cxl_sbr_collect_regions()?

If xa_insert() fails inside cxl_sbr_collect_regions():

	rc = xa_insert(regions, (unsigned long)cxlr, cxlr, GFP_KERNEL);
	if (rc) {
		put_device(&cxlr->dev);
		return rc;
	}

it returns an error and stops collecting. By discarding the return value here,
could this silently skip some regions and leave them permanently offline and
unbound?

> +
> +	hdm_state = xa_load(&cxl_sbr_hdm_state, (unsigned long)dport_pci);
> +	if (hdm_state)
> +		cxl_sbr_recommit_decoders(dport_pci, hdm_state);
> +	else
> +		pci_warn(dport_pci, "no saved HDM state, decode not restored\n");

[Severity: Medium]
Is it possible for ports to legitimately have no HDM state (like ports without
devices or with emulated decoders)?

The save path in cxl_port_save_hdm_state() appears to correctly skip such
ports without saving state. If that happens, wouldn't this path log a spurious
warning? Furthermore, wouldn't the recommit path in
cxl_port_recommit_decoders() incorrectly treat missing states as an -ENODATA
failure?

[Severity: High]
Since cxl_sbr_recommit_decoders() returns void, how does this code handle a
hardware programming failure?

If cxl_sbr_recommit_decoders() fails to reprogram the HDM decoders (e.g., due
to device timeout), won't the subsequent loop unconditionally call
cxl_region_enable() and rebind the region driver while the hardware is still
misconfigured? Could this lead to invalid memory accesses?

> +
> +	xa_for_each(&regions, index, cxlr)
> +		cxl_region_enable(cxlr);
> +
> +	cxl_sbr_put_regions(&regions);
> +	cxl_sbr_drop_hdm_state(dport_pci);
> +}
[ ... ]
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index fc0bec991a693..d1dd4924fba17 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
[ ... ]
> @@ -4263,12 +4264,14 @@ static struct cxl_driver cxl_region_driver = {
>  
>  int cxl_region_init(void)
>  {
> +	pci_cxl_set_sbr_region_ops(&cxl_sbr_region_ops);
>  	return cxl_driver_register(&cxl_region_driver);
>  }

[Severity: High]
If cxl_driver_register() fails here, does the PCI subsystem retain a dangling
pointer to cxl_sbr_region_ops?

Also, since the PCI core doesn't hold a module reference, is there a risk of
the module being concurrently unloaded while a Secondary Bus Reset is in
progress?

>  
>  void cxl_region_exit(void)
>  {
>  	cxl_driver_unregister(&cxl_region_driver);
> +	pci_cxl_set_sbr_region_ops(NULL);
>  }

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