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

On Fri, Jul 24, 2026 at 02:50:27PM +0800, Richard Cheng wrote:
> 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

I'm wondering about these 'while you're at it' changes.

While (of course ;)) I'm for the greater good of the code, I'm curious
to see how they will be added to this patchset. Perhaps they will appear
as prep patches, refactors, or cleanups?

I did review and test this today, but will hold my tag til next
revision.

-- Alison

> 
> Best regards,
> Richard Cheng.