Re: [PATCH v7 03/11] cxl: Cache endpoint decoder settings during PCI enumeration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Allocating devres resources during PCI device enumeration causes driver probing to fail permanently with -EBUSY.
- [High] devm_cxl_pci_setup_hdm_info() falsely rejects devices using DVSEC emulation when their DVSEC range count differs from their HDM decoder count.
- [Medium] Missing release/acquire memory barriers when publishing and consuming the `pdev->hdm` pointer allows concurrent readers to see uninitialized memory.
--

commit e3058b1fc7aea013a57ebe41db76dc923a188403
Author: Srirangan Madhavan <smadhavan@nvidia.com>

cxl: Cache endpoint decoder settings during PCI enumeration

Populate pci_dev->hdm from PCI capability initialization when a CXL.mem
function already has memory decoding enabled. This gives driver-free reset
paths an early HDM snapshot without enabling BAR decoding during
enumeration.

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 83cda63f76a5a..0230ebfada42b 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
[ ... ]
> @@ -107,9 +105,27 @@ static int devm_cxl_pci_setup_hdm_info(struct cxl_hdm *cxlhdm)
>  	}
>  
>  	if (!dev_is_pci(uport))
> +		return NULL;
> +
> +	return to_pci_dev(uport);
> +}
> +
> +static int devm_cxl_pci_setup_hdm_info(struct cxl_hdm *cxlhdm)
> +{
> +	struct cxl_hdm_info *info;
> +	struct pci_dev *pdev;
> +
> +	pdev = cxl_hdm_to_pci_dev(cxlhdm);
> +	if (!pdev)
> +		return 0;
> +
> +	info = READ_ONCE(pdev->hdm);
> +	if (info) {
> +		if (info->decoder_count != cxlhdm->decoder_count)
> +			return -ENXIO;

[Severity: High]
Will this strict equality check reject valid devices that fall back to DVSEC
emulation?

When the cxl_port driver probes, devm_cxl_setup_hdm() might fall back to
DVSEC emulation, which overwrites the local cxlhdm->decoder_count with
the number of DVSEC ranges. If that happens, this check against the raw
hardware HDM count will fail and return -ENXIO, breaking the probe.

>  		return 0;
> +	}
>  
> -	pdev = to_pci_dev(uport);
>  	info = devm_kzalloc(&pdev->dev,
>  			    struct_size(info, settings, cxlhdm->decoder_count),
>  			    GFP_KERNEL);
[ ... ]
> diff --git a/drivers/cxl/core/reset.c b/drivers/cxl/core/reset.c
> index 14f024098e826..fc52d3abdb5b4 100644
> --- a/drivers/cxl/core/reset.c
> +++ b/drivers/cxl/core/reset.c
[ ... ]
> +int pci_cxl_hdm_init(struct pci_dev *pdev)
> +{
[ ... ]
> +	if (!info) {
> +		info = devm_kzalloc(&pdev->dev,
> +				    struct_size(info, settings,
> +						CXL_HDM_DECODER_MAX_COUNT),
> +				    GFP_KERNEL);

[Severity: Critical]
Does allocating devres memory during PCI enumeration prevent the device
from being probed later?

The driver core requires a device's devres_head to be empty before a
driver can bind to it. Since pci_cxl_hdm_init() is called during the early
pci_init_capabilities() enumeration phase, this devm_kzalloc() will attach
resources before probing. 

Will really_probe() then abort with a resources present before probing
error and permanently prevent the cxl_pci driver from binding to the device?

> +		if (!info)
> +			return -ENOMEM;
> +		allocated_info = true;
> +	}
[ ... ]
> +	settings = info->settings;
> +	for (int i = 0; i < info->decoder_count; i++) {
> +		rc = cxl_pci_hdm_read_decoder(pdev, &settings[i],
> +					      regs.hdm_decoder, i);
> +		if (rc)
> +			goto out_unmap;
> +	}
> +
> +	WRITE_ONCE(pdev->hdm, info);

[Severity: Medium]
Are memory barriers needed here to ensure the initialized memory is visible
before the pointer is published?

Since WRITE_ONCE() and READ_ONCE() do not provide SMP memory barriers,
can the CPU reorder the devm_kzalloc() memory zeroing after the pointer
publication? If a concurrent sysfs reader accesses the pointer, it could
potentially see a non-zero garbage value for info->regs.hdm_decoder.

> +	if (allocated_info) {
> +		rc = devm_add_action(&pdev->dev, cxl_pci_hdm_clear, pdev);
> +		if (rc) {
> +			WRITE_ONCE(pdev->hdm, NULL);
> +			goto out_unmap;
> +		}
> +	}
> +	return 0;
[ ... ]
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b63cd0c310bc0..9e214446fd42f 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
[ ... ]
> @@ -2679,6 +2680,7 @@ static void pci_init_capabilities(struct pci_dev *dev)
>  	pci_rebar_init(dev);		/* Resizable BAR */
>  	pci_dev3_init(dev);		/* Device 3 capabilities */
>  	pci_ide_init(dev);		/* Link Integrity and Data Encryption */
> +	pci_cxl_hdm_init(dev);		/* CXL HDM Decoder Capability */
>  
>  	pcie_report_downtraining(dev);
>  	pci_init_reset_methods(dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623032453.3404772-1-smadhavan@nvidia.com?part=3