Re: [PATCH v12 3/8] cxl/cdat: Gather DSMAS data for DCD partitions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The commit message claims to extract and store the 'read only' attribute from DSMAS tables, but this logic is completely missing from the code.
- [Medium] Spurious `dev_warn` on valid overlapping CDAT DSMAS entries for DCD partitions.
--

--- Patch [3]: [PATCH v12 3/8] cxl/cdat: Gather DSMAS data for DCD partitions ---
commit 2a6d0666da69329a29cf2ac94ee05e10012974cc
Author: Ira Weiny <iweiny@kernel.org>

cxl/cdat: Gather DSMAS data for DCD partitions

This commit extracts additional DCD partition information from DSMAS CDAT
tables. It matches DCD partitions with DSMAS tables to store performance
and shareable attributes.

>     cxl/cdat: Gather DSMAS data for DCD partitions
>     
>     Additional DCD partition (AKA region) information is contained in the
>     DSMAS CDAT tables, including performance, read only, and shareable
>     attributes.

[Severity: Medium]
The commit message states that the read only attribute is being extracted,
but I am not seeing the logic for it in the patch below.

> diff --git a/drivers/cxl/core/cdat.c b/drivers/cxl/core/cdat.c
> index 5c9f072625135..37136b2cf7e48 100644
> --- a/drivers/cxl/core/cdat.c
> +++ b/drivers/cxl/core/cdat.c
[ ... ]
> @@ -74,6 +75,7 @@ static int cdat_dsmas_handler(union acpi_subtable_headers *header, void *arg,
>  		return -ENOMEM;
>  
>  	dent->handle = dsmas->dsmad_handle;
> +	dent->shareable = dsmas->flags & ACPI_CDAT_DSMAS_SHAREABLE;

[Severity: Medium]
Is the parsing for ACPI_CDAT_DSMAS_READ_ONLY missing from cdat_dsmas_handler()?
The code only appears to extract the shareable flag.

[ ... ]
> @@ -266,15 +268,26 @@ static void cxl_memdev_set_qos_class(struct cxl_dev_state *cxlds,
>  		bool found = false;
>  
>  		for (int i = 0; i < cxlds->nr_partitions; i++) {
> +			enum cxl_partition_mode mode = cxlds->part[i].mode;
>  			struct resource *res = &cxlds->part[i].res;
> +			u8 handle = cxlds->part[i].handle;
>  			struct range range = {
>  				.start = res->start,
>  				.end = res->end,
>  			};
>  
>  			if (range_contains(&range, &dent->dpa_range)) {
> +				if (mode == CXL_PARTMODE_DYNAMIC_RAM_1 &&
> +				    dent->handle != handle) {
> +					dev_warn(dev,
> +						 "Dynamic RAM perf mismatch; %pra (%u) vs %pra (%u)\n",
> +						 &range, handle, &dent->dpa_range,
> +						 dent->handle);
> +					continue;
> +				}

[Severity: Medium]
Could this trigger spurious warnings on valid hardware configurations?

The CDAT specification allows overlapping DSMAS ranges with different handles
to support multiple memory types or sharing characteristics for the same DPA
space. 

If a compliant device provides multiple valid DSMAS entries that overlap
the partition's range but have different handles, won't iterating over them 
incorrectly flag the unselected ones as a perf mismatch?

[ ... ]
> diff --git a/include/cxl/cxl.h b/include/cxl/cxl.h
> index 87c2bd73df21d..baf0132c70a34 100644
> --- a/include/cxl/cxl.h
> +++ b/include/cxl/cxl.h
> @@ -141,11 +141,15 @@ enum cxl_partition_mode {
>   * @res: shortcut to the partition in the DPA resource tree (cxlds->dpa_res)
>   * @perf: performance attributes of the partition from CDAT
>   * @mode: operation mode for the DPA capacity, e.g. ram, pmem, dynamic...
> + * @handle: DSMAS handle intended to represent this partition
> + * @shareable: Is the partition sharable (from its CDAT DSMAS entry)
>   */
>  struct cxl_dpa_partition {
>  	struct resource res;
>  	struct cxl_dpa_perf perf;
>  	enum cxl_partition_mode mode;
> +	u8 handle;
> +	bool shareable;
>  };

[Severity: Medium]
Does struct cxl_dpa_partition also need a read_only boolean field added here
to match the commit message description?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731084901.1512819-1-anisa.su@samsung.com?part=3