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

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.

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