Re: [PATCH v4 0/2] Support zero-sized HDM decoders

[Richard Cheng]

On Tue, Jun 09, 2026 at 04:37:30PM +0800, Dan Williams (nvidia) wrote:
> Dan Williams (nvidia) wrote:
> [..]
> > I think the way to solve this is something like below (untested).
> 
> ...whoops and unset apparently.
> 
> > It keeps @hdm_end aligned with the available decoders, and tracks the
> > start of zero allocations relative to their skip. I believe it may
> > also address the Sashiko report.
> 
> -- >8 --
> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> index 0c80b76a5f9b..3f9d97c9a3b7 100644
> --- a/drivers/cxl/core/hdm.c
> +++ b/drivers/cxl/core/hdm.c
> @@ -209,6 +209,14 @@ void cxl_dpa_debug(struct seq_file *file, struct cxl_dev_state *cxlds)
>  }
>  EXPORT_SYMBOL_NS_GPL(cxl_dpa_debug, "CXL");
>  
> +static void cxl_dpa_release_region(struct resource *parent, struct resource *res)
> +{
> +	/* zero sized decoders are not tracked in tree */
> +	if (resource_size(res) == 0)
> +		kfree(res);
> +	__release_region(parent, res->start, resource_size(res));
> +}
> +
>  /* See request_skip() kernel-doc */
>  static resource_size_t __adjust_skip(struct cxl_dev_state *cxlds,
>  				     const resource_size_t skip_base,
> @@ -256,7 +264,7 @@ static void __cxl_dpa_release(struct cxl_endpoint_decoder *cxled)
>  
>  	/* save @skip_start, before @res is released */
>  	skip_start = res->start - cxled->skip;
> -	__release_region(&cxlds->dpa_res, res->start, resource_size(res));
> +	cxl_dpa_release_region(&cxlds->dpa_res, res);
>  	if (cxled->skip)
>  		release_skip(cxlds, skip_start, cxled->skip);
>  	cxled->skip = 0;
> @@ -336,6 +344,22 @@ static int request_skip(struct cxl_dev_state *cxlds,
>  	return -EBUSY;
>  }
>  
> +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;
> +		*res = DEFINE_RES_NAMED(start, n, name, 0);
> +		return res;
> +	}
> +	return __request_region(parent, start, n, name, 0);
> +}
> +
>  static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>  			     resource_size_t base, resource_size_t len,
>  			     resource_size_t skipped)
> @@ -349,12 +373,6 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>  
>  	lockdep_assert_held_write(&cxl_rwsem.dpa);
>  
> -	if (!len) {
> -		dev_warn(dev, "decoder%d.%d: empty reservation attempted\n",
> -			 port->id, cxled->cxld.id);
> -		return -EINVAL;
> -	}
> -
>  	if (cxled->dpa_res) {
>  		dev_dbg(dev, "decoder%d.%d: existing allocation %pr assigned\n",
>  			port->id, cxled->cxld.id, cxled->dpa_res);
> @@ -378,8 +396,8 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>  		if (rc)
>  			return rc;
>  	}
> -	res = __request_region(&cxlds->dpa_res, base, len,
> -			       dev_name(&cxled->cxld.dev), 0);
> +	res = cxl_dpa_request_region(&cxlds->dpa_res, base, len,
> +				     dev_name(&cxled->cxld.dev));
>  	if (!res) {
>  		dev_dbg(dev, "decoder%d.%d: failed to reserve allocation\n",
>  			port->id, cxled->cxld.id);
> @@ -545,7 +563,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->dpa_res || !resource_size(cxled->dpa_res))
>  		return 0;
>  	if (cxled->cxld.region) {
>  		dev_dbg(dev, "decoder assigned to: %s\n",
> diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c
> index 7c6c5b7450a5..6f4d634b2cfb 100644
> --- a/drivers/cxl/core/mbox.c
> +++ b/drivers/cxl/core/mbox.c
> @@ -1433,6 +1433,9 @@ int cxl_mem_get_poison(struct cxl_memdev *cxlmd, u64 offset, u64 len,
>  	int nr_records = 0;
>  	int rc;
>  
> +	if (!len)
> +		return 0;
> +
>  	ACQUIRE(mutex_intr, lock)(&mds->poison.mutex);
>  	if ((rc = ACQUIRE_ERR(mutex_intr, &lock)))
>  		return rc;
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index e50dc716d4e8..0d03e3bedb40 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -2090,7 +2090,7 @@ static int cxl_region_attach(struct cxl_region *cxlr,
>  		return -ENXIO;
>  	}
>  
> -	if (!cxled->dpa_res) {
> +	if (!cxled->dpa_res || !resource_size(cxled->dpa_res)) {
>  		dev_dbg(&cxlr->dev, "%s:%s: missing DPA allocation.\n",
>  			dev_name(&cxlmd->dev), dev_name(&cxled->cxld.dev));
>  		return -ENXIO;

Hi Dan,

Thanks for your review, much appreciated !

Agree with your suggestion that we should treat zero-sized
decoder as normal one, I'll adopt this in v5, we won't make
it special case.

v4 never reads a zero-size decoder's SKIP reg, so *dpa_base
goes stale for that layout.
3 fixups from wiring the sketch up:

1. cxl_dpa_release_region() needs a return after kfree(), otherwise it
   falls through to __release_region() on the freed, never-inserted
   resource.

2. The zero-size resource needs IORESOURCE_MEM flags, or
   resource_contains() fails its resource_type() check and cxled->part
   never resolves, leading to a part[-1] read in poison_by_decoder().

3. A fully-burned device (decoder0 zero-size at DPA 0) still hits the
   Sashiko report: .end wraps, no partition contains it, and
   cxl_get_poison_unmapped() bails on part == -1. v5 gates the poison
   queries on part >= 0 and falls back to scanning all partitions when
   nothing sized was walked.

Best regards,
Richard Cheng.