Re: [PATCH v2 3/3] cxl/hdm: Restore commit_end when decoder enumeration fails

On Thu, Jul 16, 2026 at 04:41:30PM +0800, Richard Cheng wrote:
> On Wed, Jul 15, 2026 at 09:57:06PM +0800, [email protected] wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> > - [Medium] Mixing goto-based error handling with scope-based cleanup helpers (`scoped_guard()`) in the same function.
> > --
> > 
> > --- Patch [3]: [PATCH v2 3/3] cxl/hdm: Restore commit_end when decoder enumeration fails ---
> > commit fee00b887a5dc8f3c4c335d03db77d2ecce10947
> > Author: Alison Schofield <[email protected]>
> > 
> > cxl/hdm: Restore commit_end when decoder enumeration fails
> > 
> > This commit restores commit_end to the none-committed baseline on the
> > enumeration failure path so a subsequent probe can enumerate cleanly. It
> > also protects the commit_end advance against concurrent readers in the
> > DVSEC emulation path.
> > 
> > > diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
> > > --- a/drivers/cxl/core/hdm.c
> > > +++ b/drivers/cxl/core/hdm.c
> > 
> > [ ... ]
> > 
> > > @@ -1198,7 +1199,8 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> > >  				dev_warn(&port->dev,
> > >  					 "Failed to allocate decoder%d.%d\n",
> > >  					 port->id, i);
> > > -				return PTR_ERR(cxled);
> > > +				rc = PTR_ERR(cxled);
> > > +				goto err;
> > 
> > [ ... ]
> > 
> > > @@ -1220,17 +1223,23 @@ static int devm_cxl_enumerate_decoders(struct cxl_hdm *cxlhdm,
> > >  				 "Failed to initialize decoder%d.%d\n",
> > >  				 port->id, i);
> > >  			put_device(&cxld->dev);
> > > -			return rc;
> > > +			goto err;
> > >  		}
> > >  		rc = add_hdm_decoder(port, cxld);
> > >  		if (rc) {
> > >  			dev_warn(&port->dev,
> > >  				 "Failed to add decoder%d.%d\n", port->id, i);
> > > -			return rc;
> > > +			goto err;
> > >  		}
> > >  	}
> > >  
> > >  	return 0;
> > > +
> > > +err:
> > > +	/* Reset to the none committed baseline for a fresh enumeration */
> > > +	scoped_guard(rwsem_write, &cxl_rwsem.region)
> > > +		port->commit_end = -1;
> > > +	return rc;
> > >  }
> > 
> > [Severity: Medium]
> > Does mixing goto-based error handling with scope-based cleanup helpers
> > (scoped_guard) in the same function violate the cleanup subsystem guidelines?
> > 
> > The documentation in include/linux/cleanup.h notes that usage of "goto"
> > and cleanup helpers should never be mixed in the same function, because it
> > creates confusing ownership semantics and can lead to bugs under future
> > modifications.
> > 
> > Should devm_cxl_enumerate_decoders() either be converted entirely to
> > scope-based cleanup, or avoid using scoped_guard() in the error path?
> >
> 
> Hi Alison,
> 
> I think this finding is real, at this point, decoder added by earlier loop
> iterations are still registered until the caller's devres unwind runs.
> 
> So resetting commit_end here will create a window where those enable decoder
> objects are still visible, but cxl_num_decoders_committed() tell us it's 0 there.
> 
> I think the commit_end reset should be ordered after the registered decoder are removed.

Thanks for reviewing! 
Agreed, that's a real under-count window. Resetting commit_end on the failure
path leaves it at -1 while previously registered decoders are still visible.

v3 follows your suggestion and moves the reset into a devres action that's
registered before any decoders are added. Since decoder teardown is registered
later, devres unwinds them first and resets commit_end last, after all decoders
have been removed.

To verify, I instrumented the cxl_test mock enumerate path (which mirrors this
fix) to fail after decoder0 commits and confirmed the ordering. The failure path
leaves commit_end unchanged and the reset doesn't happen until teardown:

endpoint11: DEMO: failing decoder11.1, commit_end=0 (not reset here)
endpoint11: DEMO: devres reset commit_end=-1

-- Alison

> 
> --Richard
>  
> > -- 
> > Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
> >