Re: [PATCH v7 1/2 RESEND] cxl/hdm: Allow zero sized HDM decoders

[Jonathan Cameron <jic23@kernel.org>]

On Tue, 21 Jul 2026 16:57:14 +0800
Richard Cheng <icheng@nvidia.com> wrote:

Hi Richard,

> CXL r4.0 §8.2.4.20.12 ("Committing Decoder Programming") and §14.13.10
> ("CXL HDM Decoder Zero Size Commit") permit committing an HDM decoder
> with size 0. BIOS may commit and lock such decoders so the OS cannot
> program regions through them, this is a design choice rather than a spec
> requirement.

Trivial but consistency in commit message formatting is good. I'd always use a
blank line between paragraphs.

> The kernel rejected these with -ENXIO during port enumeration and aborted
> the whole port, so affected systems showed nothing under 'cxl list'.
> 
> Treat empty decoders as first class instead of special casing them, back
> them with a kmalloc'd resource, since the resource tree can't represent
> an empty range, and keep the skip and hdm_end accounting intact. Guard
> the paths an empty decoder can't serve, e.g. region attach, DPA free, and
> poison queries.
> 
> Suggested-by: Dan Williams <djbw@kernel.org>
> Signed-off-by: Vishal Aslot <vaslot@nvidia.com>
> Signed-off-by: Richard Cheng <icheng@nvidia.com>
> Reviewed-by: Dan Williams <djbw@kernel.org>

...

Main comment is of the 'whilst you are here' variety. I'm fine
if you want to ignore it.  I just found the existing code a little trickier
to read than the ideal and some of it gets shuffled round in here.

Either way
Reviewed-by: Jonathan Cameron <jonathan.cameron@oss.qualcomm.com>

> ---
>  drivers/cxl/core/hdm.c    | 52 ++++++++++++++++++++++++++-------------
>  drivers/cxl/core/mbox.c   |  3 +++
>  drivers/cxl/core/region.c | 49 +++++++++++++++++++++++-------------
>  drivers/cxl/cxl.h         | 10 ++++++++
>  drivers/cxl/port.c        |  3 +++
>  5 files changed, 83 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 0c80b76a5f9b..ccbab2e21f06 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c

...


>  
> +static struct resource *cxl_dpa_request_region(struct resource *parent,
> +					       resource_size_t start,
> +					       resource_size_t n,
> +					       const char *name)
> +{
> +	if (!n) {
> +		struct resource *res = kmalloc_obj(*res);
> +
> +		if (!res)
> +			return NULL;
I'd format this a tiny bit differently but not that important.
The advantage is to keep the allocation and error check closely coupled.

		struct resource *res;

		res = malloc_obj(*res);
		if (!res)
			return NULL;

		*res = DEFINE_RES_NAMED(start, 0, name, IORESOURCE_MEM);

		return res;

> +		*res = DEFINE_RES_NAMED(start, 0, name, IORESOURCE_MEM);
> +		return res;
> +	}
> +
> +	return __request_region(parent, start, n, name, 0);
> +}

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 1e211542b6b6..6c7d9a52707c 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2115,7 +2115,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		return -ENXIO;
>  	}
>  
> -	if (!cxled->dpa_res) {
> +	if (cxled_empty(cxled)) {
>  		dev_dbg(&cxlr->dev, "%s:%s: missing DPA allocation.\n",
>  			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev));
>  		return -ENXIO;
> @@ -2959,28 +2959,35 @@ static int poison_by_decoder(struct device *dev, void *arg)
>  	if (!cxled->dpa_res)
>  		return rc;

Whilst touching code can we just return 0 for this and the one above.

The return values from this function are unusual so nice if we can make
them as obvious as possible!

>  
> -	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) {
> +		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 (rc == -EFAULT && mode == CXL_PARTMODE_RAM)

Maybe similar to below. 

> +				rc = 0;
> +			if (rc)
> +				return rc;
> +		}
>  
> -	if (cxled->skip) {
> -		offset = cxled->dpa_res->start - cxled->skip;
> -		length = cxled->skip;
> -		rc = cxl_mem_get_poison(cxlmd, offset, length, NULL);
> +		offset = cxled->dpa_res->start;
> +		length = cxled->dpa_res->end - offset + 1;
> +		rc = cxl_mem_get_poison(cxlmd, offset, length, cxled->cxld.region);
>  		if (rc == -EFAULT && mode == CXL_PARTMODE_RAM)
>  			rc = 0;
>  		if (rc)
>  			return rc;

Maybe whilst we are here we can avoid the rc dance?

		if (rc && !(rc == -EFAULT && mode == CXL_PARTMODE_RAM))
			return rc;

>  	}
>  
> -	offset = cxled->dpa_res->start;
> -	length = cxled->dpa_res->end - offset + 1;
> -	rc = cxl_mem_get_poison(cxlmd, offset, length, cxled->cxld.region);
> -	if (rc == -EFAULT && mode == CXL_PARTMODE_RAM)
> -		rc = 0;
> -	if (rc)
> -		return rc;
> -
>  	/* Iterate until commit_end is reached */
>  	if (cxled->cxld.id == ctx->port->commit_end) {
>  		ctx->offset = cxled->dpa_res->end + 1;