Re: [PATCH v6 5/6] cxl/test: Fixup hdm init for auto region to support type2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] mock_init_hdm_type2_cxled bypasses cxl_port_update_decoder_targets() and modifies the live decoder target array directly without the cxl_rwsem.region lock.
- [Medium] Missing return value check for devm_cxl_dpa_reserve().
- [Low] Unused parameter `pdev` in `mock_init_hdm_type2_cxled`.
--

commit 0f9c8e2fe22ac8d4fe6d8be6b13b11102337bb2b
Author: Dave Jiang <dave.jiang@intel.com>

cxl/test: Fixup hdm init for auto region to support type2

This commit adds support to set up the initialization of decoders
to support type 2 auto regions in the cxl test framework.

> diff --git a/tools/testing/cxl/test/cxl.c b/tools/testing/cxl/test/cxl.c
> index 1eac06ad46601..910c8b9abc003 100644
> --- a/tools/testing/cxl/test/cxl.c
> +++ b/tools/testing/cxl/test/cxl.c

[ ... ]

> +static void mock_init_hdm_type2_cxled(struct cxl_endpoint_decoder *cxled,
> +				      struct cxl_port *port,
> +				      struct platform_device *pdev)

[Severity: Low]
This isn't a bug, but it looks like pdev is never evaluated or used in
this function. Is this parameter necessary?

> +{
> +	struct acpi_cedt_cfmws *window = mock_cfmws[0];
> +	struct cxl_decoder *cxld = &cxled->cxld;

[ ... ]

> +	cxld->target_type = CXL_DECODER_DEVMEM;
> +	cxld->flags = CXL_DECODER_F_ENABLE;
> +	cxled->state = CXL_DECODER_STATE_AUTO;
> +	port->commit_end = cxld->id;
> +	devm_cxl_dpa_reserve(cxled, 0,
> +			     mock_auto_region_size / cxld->interleave_ways, 0);

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

If this call fails with an error like -ENOMEM or -EBUSY, the mock framework
will silently proceed with an invalid or unreserved DPA state, which could
mask bugs or cause test crashes later on.

> +	cxld->commit = mock_decoder_commit;
> +	cxld->reset = mock_decoder_reset;
> +
> +	WARN_ON_ONCE(!cxld_registry_new(cxld));
> +	/*
> +	 * Now that endpoint decoder is set up, walk up the hierarchy
> +	 * and setup the root port decoder targeting @cxlmd.
> +	 */
> +	dport = port->parent_dport;
> +	root_port = dport->port;
> +	dev = device_find_child(&root_port->dev, NULL, first_decoder);
> +	/*
> +	 * Ancestor ports are guaranteed to be enumerated before
> +	 * @port, and all ports have at least one decoder.
> +	 */
> +	if (WARN_ON(!dev))
> +		return;
> +
> +	cxlsd = to_cxl_switch_decoder(dev);
> +	cxlsd->target[0] = dport;
> +	cxlsd->cxld.target_map[0] = dport->port_id;

[Severity: Medium]
Is it safe to directly assign to the live decoder target array here
without holding the cxl_rwsem.region lock for synchronization?

Bypassing cxl_port_update_decoder_targets() and modifying the decoder target
array directly creates a data race against concurrent sysfs readers like
target_list_show() that rely on this lock. Should this use
cxl_port_update_decoder_targets() instead?

> +	cxld = &cxlsd->cxld;
> +	cxld->target_type = CXL_DECODER_DEVMEM;
> +	cxld->flags = CXL_DECODER_F_ENABLE;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629202720.3658117-1-dave.jiang@intel.com?part=5