Re: [RFC PATCH] cxl: Fix premature commit_end increment on decoder commit failure

[Alison Schofield <alison.schofield@intel.com>]

On Wed, Jan 28, 2026 at 03:00:14PM +0800, Yuxiong Wang wrote:
> Resending with corrected CC list - apologies to those who received the
> message twice.
> 
> Thanks for comments!
> 
> On Tue, Jan 27, 2026 at 01:15:04PM -0800, Alison Schofield wrote:
> > On Tue, Jan 27, 2026 at 04:48:55PM +0800, Yuxiong Wang wrote:
> > > In cxl_decoder_commit(), commit_end is incremented before verifying whether the
> > > commit succeeded, and the CXL_DECODER_F_ENABLE bit in cxld->flags is only set
> > > after a successful commit. As a result, if the commit fails, commit_end has been
> > > incremented and cxld->reset() has no effect since the flag is not set, so commit_end
> > > remains incorrectly incremented. The inconsistency between commit_end and
> > > CXL_DECODER_F_ENABLE causes failure during subsequent either commit or reset
> > > operations.
> > > 
> > > Fix this by incrementing commit_end only after confirming the commit succeeded.
> > > Since cxld_await_commit() clears the decoder commit bit on failure, no additional
> > > reset is required. Remove the ineffective cxld->reset() call in this case.
> > 
> > 
> > Why are the writel()'s that cxld->reset() intended to do not really
> > needed? Is only the twiddle of the COMMIT bit needed for a reset or
> > do we need to 0 the size and base offsets we previously wrote?
> 
> In this case, cxld->reset() does not work because CXL_DECODER_F_ENABLE bit
> in cxld->flag is not set. 
> 
> The size and base registers shall have no effects if the commit bit is cleared.
> And except the reset and commit functions, there are no other functions that
> touch these registers after initialization. So I think the key step is to clear
> the commit bit, which has been done in cxld_await_commit(), and the size and
> base will be overwritten in the next commit.
> 
> Considering the potential hardware problems, perhaps it's more a secure
> solution to clear all these offsets. I can add a function like
> 'clear_hw_decoder()' to do this.

Thanks for the explanation. Looking in the spec now ;) I don't see
clearing the offsets as required. 8.2.4.20.12.

Reviewed-by: Alison Schofield <alison.schofield@intel.com>

> 
> > 
> > How did you find this?  Maybe we can add test case to cxl unit tests.
> 
> I think this is a very rare situation, since the software has done very
> strict checks before commits. We found this when testing the cxl module
> of kernel and firmware. We committed the region in OS, and faked a host
> bridge hdm decoder commit failure in firmware. During the second commit,
> we triggered this problem with 'out of order commit'.

Thanks for sharing that.


> 
> > 
> > Thanks,
> > Alison
> 
> Best Regards,
> Yuxiong
> 
> > 
> > 
> > > 
> > > Fixes: 176baef ("cxl/hdm: Commit decoder state to hardware")
> > > Signed-off-by: Yuxiong Wang <yuxiong.wang@linux.alibaba.com>
> > > Acked-by: Huang Ying <ying.huang@linux.alibaba.com>
> > > ---
> > >  drivers/cxl/core/hdm.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > > index eb5a3a7640c6..912f648a6b7a 100644
> > > --- a/drivers/cxl/core/hdm.c
> > > +++ b/drivers/cxl/core/hdm.c
> > > @@ -844,14 +844,13 @@ static int cxl_decoder_commit(struct cxl_decoder *cxld)
> > >  	scoped_guard(rwsem_read, &cxl_rwsem.dpa)
> > >  		setup_hw_decoder(cxld, hdm);
> > >  
> > > -	port->commit_end++;
> > >  	rc = cxld_await_commit(hdm, cxld->id);
> > >  	if (rc) {
> > >  		dev_dbg(&port->dev, "%s: error %d committing decoder\n",
> > >  			dev_name(&cxld->dev), rc);
> > > -		cxld->reset(cxld);
> > >  		return rc;
> > >  	}
> > > +	port->commit_end++;
> > >  	cxld->flags |= CXL_DECODER_F_ENABLE;
> > >  
> > >  	return 0;
> > > -- 
> > > 2.50.1 (Apple Git-155)
> > >