Re: [PATCH 11/15] cxl/core: Add support for HDM-D cache id programming

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] cxl/cache: HDM-D capability limit ignored for intermediate switches
- [Medium] cxl/cache: off-by-one error in HDMDMax capability limit check
--

commit bdfe2b9c776e24b9a52c8e7a8594ae19a2e9c210
Author: Ben Cheatham <Benjamin.Cheatham@amd.com>

cxl/core: Add support for HDM-D cache id programming

This commit adds cache id allocation and programming support to the
pre-existing cache id programming routines for HDM-D devices.

> diff --git a/drivers/cxl/core/cache.c b/drivers/cxl/core/cache.c
> --- a/drivers/cxl/core/cache.c
> +++ b/drivers/cxl/core/cache.c
[ ... ]
> @@ -290,6 +297,16 @@ static int cxl_cid_program_table_entry(struct cxl_port *port, int cid,
>  		return -EINVAL;
>  	}
>  
> +	if (is_cxl_root(parent_port_of(port)) && hdmd) {
> +		hdmd_max = FIELD_GET(CXL_CACHE_ID_RT_CAP_HDMD_MAX, cap);
> +
> +		if (port->num_hdmd > hdmd_max) {

[Severity: Medium]
Will this strict inequality reject valid configurations when the hardware
capability register is 0-based?

In the CXL capability registers, CXL_CACHE_ID_RT_CAP_HDMD_MAX is only 4 bits.
To support 16 HDM-D devices (the maximum), it is highly likely to be 0-based.
The strict inequality port->num_hdmd > hdmd_max incorrectly evaluates to 1 > 0
for the first device, which could erroneously reject valid HDM-D devices during
initialization.

[Severity: High]
Does this capability limit check silently ignore intermediate switches?

Because cxl_cid_program_table_entry() explicitly restricts the limit validation
only to the top-level switch (direct child of the root), intermediate cascaded
switches completely skip validation. In a nested CXL topology, probing multiple
HDM-D endpoints connected to an intermediate switch that has a lower HDMDMax
capability limit than the top-level switch could silently over-program the
intermediate switch. This could potentially cause incorrect hardware routing,
dropped cache snoops, or fatal errors.

> +			dev_err(&port->dev,
> +				"Maximum number of devices using HDM-D reached\n");
> +			return -EINVAL;
> +		}
> +	}
[ ... ]
> @@ -385,6 +402,22 @@ static struct ida *find_cache_id_ida(struct cxl_port *port)
>  	return NULL;
>  }
>  
> +static void cxl_port_add_hdmd(struct cxl_port *endpoint, int val)
> +{
> +	struct cxl_port *parent = parent_port_of(endpoint);
> +	struct cxl_port *port = endpoint;
> +
> +	if (!parent || !is_cxl_cachedev(endpoint->uport_dev))
> +		return;
> +
> +	while (parent && !is_cxl_root(parent)) {
> +		port = parent;
> +		parent = parent_port_of(port);
> +	}
> +
> +	port->num_hdmd += val;

[Severity: High]
Can this loop fail to track limits for nested topologies by only aggregating
the count on the top-level switch?

By skipping intermediate switches, the HDM-D device is attributed entirely
to the top-level switch. This causes the capability limit checks to be
bypassed for intermediate cascaded switches.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260923173342.5584-1-Benjamin.Cheatham@amd.com?part=11