Re: [PATCH] cxl/hdm: fix a warning in devm_remove_action()

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

On Sun, Mar 08, 2026 at 08:08:10PM -0400, Sungwoo Kim wrote:
> In the following race scenario, devm_remove_action() can be called
> before devm_add_action(), triggering a warning because there is no
> action to remove.
> 
> To fix this, extend a critical section to embrace both
> __cxl_dpa_reserve() and devm_add_action().

The fix LGTM. I suggest tightening up the code comment. Now it
talks through the race in detail and also cross-ref another location.
Limit the comment to describing the ordering requirement we need to
preserve. The commit message already captures the full race and
rationale.

Comment update suggestion below-

snip

> +++ b/drivers/cxl/core/hdm.c
> @@ -509,13 +509,20 @@ int devm_cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>  	struct cxl_port *port = cxled_to_port(cxled);
>  	int rc;
>  
> -	scoped_guard(rwsem_write, &cxl_rwsem.dpa)
> -		rc = __cxl_dpa_reserve(cxled, base, len, skipped);
> +	guard(rwsem_write)(&cxl_rwsem.dpa);
> +	rc = __cxl_dpa_reserve(cxled, base, len, skipped);
>  
>  	if (rc)
>  		return rc;
>  
> -	return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
> +	/* See comments in cxl_dpa_alloc()*/

Something like this:
/* Keep reservation and devres registration ordered under dpa lock */


snip

> @@ -613,7 +620,7 @@ static int __cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, u64 size)
>  	struct resource *p, *last;
>  	int part;
>  
> -	guard(rwsem_write)(&cxl_rwsem.dpa);
> +	lockdep_assert_held_write(&cxl_rwsem.dpa);
>  	if (cxled->cxld.region) {
>  		dev_dbg(dev, "decoder attached to %s\n",
>  			dev_name(&cxled->cxld.region->dev));
> @@ -679,11 +686,28 @@ int cxl_dpa_alloc(struct cxl_endpoint_decoder *cxled, u64 size)
>  	struct cxl_port *port = cxled_to_port(cxled);
>  	int rc;
>  
> +	guard(rwsem_write)(&cxl_rwsem.dpa);
>  	rc = __cxl_dpa_alloc(cxled, size);
>  	if (rc)
>  		return rc;
>  
> -	return devm_add_action_or_reset(&port->dev, cxl_dpa_release, cxled);
> +	/*
> +	 * Add the devres action while still holding cxl_rwsem.dpa to prevent
> +	 * a race with cxl_dpa_free(). Without this, a concurrent cxl_dpa_free()
> +	 * can observe dpa_res set (by __cxl_dpa_reserve()) and attempt
> +	 * devm_remove_action() before devm_add_action() has been called,
> +	 * triggering a WARN_ON in devm_remove_action().
> +	 * Also, devm_add_action_or_reset() cannot be used here because
> +	 * cxl_dpa_release() tries to hold the dpa lock that is already held,
> +	 * causing a self deadlock.
> +	 */

and like this:

/*
 * Hold cxl_rwsem.dpa across allocation and devres registration
 * so cxl_dpa_free() cannot observe dpa_res without a matching
 * devres action.
 */

snip