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

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?

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