Re: [PATCH v9 2/3] cxl/hdm: Allow zero sized HDM decoders

Thank you for your contribution! Sashiko AI review found 3 potential issue(=
s) to consider:

New issues:
- [High] The teardown logic for 0-sized decoders is broken by an early retu=
rn, causing a resource leak and permanently breaking the DPA LIFO state mac=
hine.
- [Medium] The poison scan for the skip range of an out-of-bounds empty dec=
oder is bypassed, missing hardware error queries.

Pre-existing issues:
- [High] The unmapped poison scan aborts prematurely, missing hardware erro=
r queries for subsequent partitions.
--

--- Patch [2]: [PATCH v9 2/3] cxl/hdm: Allow zero sized HDM decoders ---
commit 129bd5c21519972c027d6ca212a8591b5cea7517
Author: Richard Cheng <[email protected]>

    cxl/hdm: Allow zero sized HDM decoders

This commit allows the kernel to accept and process zero-sized HDM decoders=
=20
instead of rejecting them during enumeration. It backs them with a kmalloc'=
d=20
resource and guards paths that do not apply to empty decoders.

> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 0c80b76a5f9b4..3b3cfa0f25074 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
[ ... ]
> @@ -545,7 +572,7 @@ int cxl_dpa_free(struct cxl_endpoint_decoder *cxled)
>  	struct device *dev =3D &cxled->cxld.dev;
> =20
>  	guard(rwsem_write)(&cxl_rwsem.dpa);
> -	if (!cxled->dpa_res)
> +	if (cxled_empty(cxled))
>  		return 0;

[Severity: High]
Does returning 0 here for 0-sized decoders bypass the release action?

If devm_cxl_dpa_release() is not called for 0-sized decoders, it appears
this would leak the reservation and prevent port->hdm_end from being=20
decremented, permanently breaking the DPA LIFO state machine for future=20
teardowns.

[ ... ]
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 45536909cc17d..e6929feb63786 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
[ ... ]
> @@ -2969,24 +2969,31 @@ static int poison_by_decoder(struct device *dev, =
void *arg)
>  	if (!cxled->dpa_res)
>  		return 0;
> =20
> -	cxlmd =3D cxled_to_memdev(cxled);
> -	cxlds =3D cxlmd->cxlds;
> -	mode =3D cxlds->part[cxled->part].mode;
> +	/*
> +	 * Handle the degenerate case of a device with only empty decoders. An
> +	 * empty decoder can still map a non-zero skip range, so advance the
> +	 * walk to commit_end either way.
> +	 */
> +	if (cxled->part >=3D 0) {

[Severity: Medium]
Will this condition skip checking poison in a valid skip range?=20

If firmware commits an empty decoder at an out-of-bounds base, cxled->part=
=20
would be -1. However, if its skip range is large enough to intersect a=20
valid partition, skipping this block seems to prevent querying the skipped=
=20
DPA space for hardware poison.

> +		cxlmd =3D cxled_to_memdev(cxled);
> +		cxlds =3D cxlmd->cxlds;
> +		mode =3D cxlds->part[cxled->part].mode;
> =20
> -	if (cxled->skip) {
> -		offset =3D cxled->dpa_res->start - cxled->skip;
> -		length =3D cxled->skip;
> -		rc =3D cxl_mem_get_poison(cxlmd, offset, length, NULL);
> +		if (cxled->skip) {
> +			offset =3D cxled->dpa_res->start - cxled->skip;
> +			length =3D cxled->skip;
> +			rc =3D cxl_mem_get_poison(cxlmd, offset, length, NULL);
> +			if (rc && !poison_efault_forgiven(rc, mode))
> +				return rc;
> +		}
[ ... ]
> @@ -3008,9 +3015,17 @@ int cxl_get_poison_by_endpoint(struct cxl_port *po=
rt)
>  	};
> =20
>  	rc =3D device_for_each_child(&port->dev, &ctx, poison_by_decoder);
> -	if (rc =3D=3D 1)
> +	if (rc =3D=3D 1) {
> +		/*
> +		 * No decoder with a sized DPA reservation was walked
> +		 * (every committed decoder is zero-size): scan all
> +		 * partitions in full.
> +		 */
> +		if (ctx.part < 0)
> +			ctx.part =3D 0;
>  		rc =3D cxl_get_poison_unmapped(to_cxl_memdev(port->uport_dev),
>  					     &ctx);

[Severity: High]
This is a pre-existing issue, but does cxl_get_poison_unmapped() abort=20
prematurely during its scan?

If a preceding partition is fully mapped, length evaluates to 0. In=20
cxl_get_poison_unmapped():

	for (int i =3D ctx->part; i < cxlds->nr_partitions; i++) {
		...
		length =3D res->end - offset + 1;
		if (!length)
			break;

Using break instead of continue here aborts the loop entirely. Does this=20
incorrectly skip hardware error queries for any subsequent unmapped=20
partitions (like PMEM)?

> +	}
> =20
>  	return rc;
>  }

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