Re: [RFC PATCH 2/3] cxl/region: Auto-create a region for memdev attach

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Medium] Mixing goto-based error handling with __free() or guard() cleanup in the same function.

Pre-existing issues:
- [High] Self-deadlock in find_committed_attach_region() on the devm_add_action_or_reset() error path due to function-scoped read lock on cxl_rwsem.region.
--

--- Patch [2]: [RFC PATCH 2/3] cxl/region: Auto-create a region for memdev attach ---
commit 5faea7eff07e5a585a3f3dc3abd38f41084338fd
Author: Richard Cheng <[email protected]>

cxl/region: Auto-create a region for memdev attach

devm_cxl_probe_mem() currently fails when FW has not committed a region,
even when a Type-2 accelerator has usable CXL.mem capacity. This patch
auto-creates a region when no mapped decoder exists by selecting a manual
DEVMEM decoder and a compatible root decoder.

> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 7fcaddc61180d..4ceabdfdd3b6a 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
[ ... ]
> +static struct cxl_region *
> +create_attach_region(struct cxl_endpoint_decoder *cxled,
> +		     struct cxl_root_decoder *cxlrd, resource_size_t size)
> +{
> +	struct cxl_region *cxlr;
> +	int rc;
> +
> +	guard(mutex)(&cxlrd->regions_lock);
> +	do {
[ ... ]
> +	rc = device_attach(&cxlr->dev);
> +	if (rc <= 0) {
> +		if (!rc)
> +			rc = -ENXIO;
> +		goto err_unregister;
> +	}
> +
> +	get_device(&cxlr->dev);
> +	return cxlr;
> +
> +err_unregister:
> +	unregister_region(cxlr, CXL_REGION_RESET_MANAGED_DETACH);
> +	return ERR_PTR(rc);
> +}

[Severity: Medium]
Is it acceptable to mix guard() based cleanup with goto-based error handling
in this function? According to the cleanup subsystem guidelines, functions
should ideally use scope-based cleanup exclusively or goto-based cleanup
exclusively to prevent complex ownership and LIFO ordering bugs.

[ ... ]
> +static int create_memdev_attach_region(struct cxl_memdev *cxlmd,
> +				       struct cxl_attach_region *attach)
> +{
> +	struct cxl_port *endpoint = cxlmd->endpoint;
> +	struct device *decoder_dev __free(put_device) = NULL;
> +	struct cxl_endpoint_decoder *cxled;
[ ... ]
> +	cxlrd = find_attach_root_decoder(cxled);
> +	if (IS_ERR(cxlrd)) {
> +		rc = PTR_ERR(cxlrd);
> +		dev_dbg(cxlmd->cxlds->dev,
> +			"no compatible Type-2 root decoder to auto-create a region for %s: %d\n",
> +			dev_name(&cxlmd->dev), rc);
> +		goto err_cleanup_dpa;
> +	}

[Severity: Medium]
Similar to create_attach_region() above, does mixing __free() and goto labels
in the same function conflict with the cleanup subsystem expectations?

[ ... ]
>  /*
> - * Runs in cxl_mem_probe context after successful endpoint probe, assumes the
> - * simple case of single mapped decoder per memdev.
> + * Attach to a firmware-precommitted region already mapped to the endpoint.
> + * Return 0 on success, -ENODEV when no region is present (the caller then
> + * auto-creates one), or a negative errno for a present-but-unusable region.
>   */
> -int cxl_memdev_attach_region(struct cxl_memdev *cxlmd)
> +static int find_committed_attach_region(struct cxl_memdev *cxlmd,
> +					struct cxl_attach_region *attach)
>  {
> -	struct cxl_attach_region *attach =
> -		container_of(cxlmd->attach, typeof(*attach), attach);
>  	struct cxl_port *endpoint = cxlmd->endpoint;
>  	struct cxl_endpoint_decoder *cxled;
>  	struct cxl_region *cxlr;
>  	int rc;
>  
> -	/* hold endpoint lock to setup autoremove of the region */
> -	guard(device)(&endpoint->dev);
> -	if (!endpoint->dev.driver)
> -		return -ENXIO;
>  	guard(rwsem_read)(&cxl_rwsem.region);
>  	guard(rwsem_read)(&cxl_rwsem.dpa);
[ ... ]
> -	/* Only teardown regions that pass validation, ignore the rest */
> +	/* Only teardown regions that pass validation. */
>  	get_device(&cxlr->dev);
> -	rc = devm_add_action_or_reset(&endpoint->dev,
> -				      endpoint_unregister_region, cxlr);
> +	rc = devm_add_action_or_reset(&endpoint->dev, endpoint_unregister_region,
> +				      cxlr);
>  	if (rc)
>  		return rc;

[Severity: High]
This is a pre-existing issue, but can this devm_add_action_or_reset() trigger
a self-deadlock on failure?

Because the function scope is guarded by a read lock on cxl_rwsem.region:

    guard(rwsem_read)(&cxl_rwsem.region);

If devm_add_action_or_reset() fails, it immediately executes its callback
endpoint_unregister_region() inline. This callback eventually calls
unregister_region() and detach_target(), which attempts to acquire a write
lock on the same semaphore:

    ACQUIRE(rwsem_write_kill, rwsem)(&cxl_rwsem.region)

Since the current thread already holds the function-scoped read lock, wouldn't
this result in an ABBA-style self-deadlock?

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