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

On Wed, Jul 22, 2026 at 01:38:22AM +0800, Jonathan Cameron wrote:
> On Tue, 21 Jul 2026 16:57:14 +0800
> Richard Cheng <[email protected]> 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.
>

Sure.
 
> > 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 <[email protected]>
> > Signed-off-by: Vishal Aslot <[email protected]>
> > Signed-off-by: Richard Cheng <[email protected]>
> > Reviewed-by: Dan Williams <[email protected]>
> 
> ...
> 
> 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 <[email protected]>
> 
> > ---
> >  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!
> 

No problem.

> >  
> > -	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;

Thanks for the review, I think these suggestions are all reasonable, I'll make them
in v8.

1. Add missing blank line between paragraphs in commit message
2. for cxl_dpa_request_region(), I'll split the declaration from the allocation so
   the alloc and its error check sit together
3. for poison_by_decoder(), both early guards will return 0 explicitly rather than
   the zero rc, and I'll collapse the -EFAULT/RAM handling into the single condition
   you suggested, in both teh skip and mapped blocks

Best regards,
Richard Cheng.