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

[sashiko-bot@kernel.org]

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 return, causing a resource leak and permanently breaking the DPA LIFO state machine.
- [Medium] The poison scan for the skip range of an out-of-bounds empty decoder is bypassed, missing hardware error queries.

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

--- Patch [2]: [PATCH v9 2/3] cxl/hdm: Allow zero sized HDM decoders ---
commit 129bd5c21519972c027d6ca212a8591b5cea7517
Author: Richard Cheng <icheng@nvidia.com>

    cxl/hdm: Allow zero sized HDM decoders

This commit allows the kernel to accept and process zero-sized HDM decoders 
instead of rejecting them during enumeration. It backs them with a kmalloc'd 
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 = &cxled->cxld.dev;
>  
>  	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 
decremented, permanently breaking the DPA LIFO state machine for future 
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;
>  
> -	cxlmd = cxled_to_memdev(cxled);
> -	cxlds = cxlmd->cxlds;
> -	mode = 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 >= 0) {

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

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

> +		cxlmd = cxled_to_memdev(cxled);
> +		cxlds = cxlmd->cxlds;
> +		mode = cxlds->part[cxled->part].mode;
>  
> -	if (cxled->skip) {
> -		offset = cxled->dpa_res->start - cxled->skip;
> -		length = cxled->skip;
> -		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
> +		if (cxled->skip) {
> +			offset = cxled->dpa_res->start - cxled->skip;
> +			length = cxled->skip;
> +			rc = 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 *port)
>  	};
>  
>  	rc = device_for_each_child(&port->dev, &ctx, poison_by_decoder);
> -	if (rc == 1)
> +	if (rc == 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 = 0;
>  		rc = 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 
prematurely during its scan?

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

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

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

> +	}
>  
>  	return rc;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260805055524.22311-1-icheng@nvidia.com?part=2