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

[Richard Cheng]

On Fri, Sep 04, 2026 at 09:53:32AM +0800, Alison Schofield wrote:
> On Wed, Aug 05, 2026 at 01:55:23PM +0800, Richard Cheng wrote:
> > 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.
> > 
> > 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.
> 
> Hi Richard,
> 
> I applied this set to 7.3-rc1 and retested w the new test case in
> cxl-topology.sh - all good.
> 
> I do have some comments though - see below 
> 
> snip...
> I'm snipping past a bunch of Reviewed-by tags a bit sheepishly.
> Maybe you'll quickly dispute my feedback?
> 
> > 
> > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > index 0c80b76a5f9b..3b3cfa0f2507 100644
> > --- a/drivers/cxl/core/hdm.c
> > +++ b/drivers/cxl/core/hdm.c
> > @@ -240,6 +240,18 @@ static resource_size_t __adjust_skip(struct cxl_dev_state *cxlds,
> >  }
> >  #define release_skip(c, b, l) __adjust_skip((c), (b), (l), NULL)
> >  
> > +static void cxl_dpa_release_region(struct resource *parent,
> > +				   struct resource *res)
> > +{
> > +	/* zero sized decoders are not tracked in the resource tree */
> > +	if (resource_size(res) == 0) {
> > +	}
> > +
> > +	__release_region(parent, res->start, resource_size(res));
> > +}
> > +
> >  /*
> >   * Must be called in a context that synchronizes against this decoder's
> >   * port ->remove() callback (like an endpoint decoder sysfs attribute)
> > @@ -256,7 +268,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 +348,26 @@ 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;
> > +
> > +		res = kmalloc_obj(*res);
> > +		if (!res)
> > +			return NULL;
> > +
> > +		*res = DEFINE_RES_NAMED(start, 0, name, IORESOURCE_MEM);
> > +
> > +		return res;
> > +	}
> > +
> > +	return __request_region(parent, start, n, name, 0);
> > +}
> > +
> 
> 2 failures are now collapsing into one -EBUSY 
> 1) __request_region() returning NULL is a resource conflict.
> 2) kmalloc_obj() returning NULL is -ENOMEM
> 
> Can this preserve the -ENONMEM?  Maybe using ERR_PTR() or handle the
> zero size alloc in __cxl_dpa_reserve()?
>

Hi Alison,

I agree with this, I'll fix it in the next version.

 
> 
> snip
> >  
> > @@ -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;
> >  	if (cxled->cxld.region) {
> >  		dev_dbg(dev, "decoder assigned to: %s\n",
> 
> 
> Is cxled_empty() the right test above?
> 
> The old check only returns early when there is no DPA resource.
> cxled_empty() also returns true for a zero-sized decoder that does
> have a resource, so this can return success before reaching the
> CXL_DECODER_F_ENABLE check.
> 
> For a committed zero-sized decoder, that means echo 0 > dpa_size can
> report success even though the decoder can't actually be freed.
> 
> Can the original !cxled->dpa_res check should remain, or the empty
> case be handled after the existing state checks.
> 
> This also means I don't think "DPA free" belongs in the commit message's
> list of paths that need to be guarded.
> 
> -- Alison
> 

Hmm for this, from the first glance it would make the serie go back to
reject the zero-sized decoder, but I do agree we should seperate
the handling path when decoder state is different rather than return success
for all.

I'll look into it and see how it goes, they should be addressed in v10.
Thanks alot for the review.

Best regards,
Richard Cheng.

> snip