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

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' a=
ttribute from DSMAS tables, but this logic is completely missing from the c=
ode.
- [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 partitio=
ns ---
commit 2a6d0666da69329a29cf2ac94ee05e10012974cc
Author: Ira Weiny <[email protected]>

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
>    =20
>     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_heade=
rs *header, void *arg,
>  		return -ENOMEM;
> =20
>  	dent->handle =3D dsmas->dsmad_handle;
> +	dent->shareable =3D dsmas->flags & ACPI_CDAT_DSMAS_SHAREABLE;

[Severity: Medium]
Is the parsing for ACPI_CDAT_DSMAS_READ_ONLY missing from cdat_dsmas_handle=
r()?
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 =3D false;
> =20
>  		for (int i =3D 0; i < cxlds->nr_partitions; i++) {
> +			enum cxl_partition_mode mode =3D cxlds->part[i].mode;
>  			struct resource *res =3D &cxlds->part[i].res;
> +			u8 handle =3D cxlds->part[i].handle;
>  			struct range range =3D {
>  				.start =3D res->start,
>  				.end =3D res->end,
>  			};
> =20
>  			if (range_contains(&range, &dent->dpa_range)) {
> +				if (mode =3D=3D CXL_PARTMODE_DYNAMIC_RAM_1 &&
> +				    dent->handle !=3D 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 handl=
es
to support multiple memory types or sharing characteristics for the same DPA
space.=20

If a compliant device provides multiple valid DSMAS entries that overlap
the partition's range but have different handles, won't iterating over them=
=20
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?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260731084901.1512=
[email protected]?part=3D3