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

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

[Yuxiong Wang]

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.

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)

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

[Dave Jiang]

On 1/27/26 1:48 AM, 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.
> 
> 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>

Looks the correct fix to me.

Reviewed-by: Dave Jiang <dave.jiang@intel.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;

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

[Alison Schofield]

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?

How did you find this?  Maybe we can add test case to cxl unit tests.

Thanks,
Alison


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

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

[Yuxiong Wang]

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.

> 
> 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,
> 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)
> > 

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

[Alison Schofield]

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