Re: [PATCH 7/7] cxl: Fix double unregistration of CXL regions for type2 devices

["Dan Williams (nvidia)" <djbw@kernel.org>]

Dave Jiang wrote:
> For a type2 device cxl_memdev_attach_region() is provided as the callback
> to devm_cxl_add_memdev() call. In cxl_memdev_attach_region() a devm
> action cxl_endpoint_region_autoremove() is registered to remove the
> region when the device is removed. And this action calls
> unregister_region() on trigger.
> 
> When the endpoint port driver is probed, discover_region() is called
> to register a region. The call tree becomes discover_region() ->
> cxl_add_to_region() -> construct_region() -> __create_region() ->
> devm_cxl_add_region(). In devm_cxl_add_region(), unregister_region()
> is also added as a devm action.
> 
> When removing the cxl_test module, the platform devices are removed
> and that triggers the endpoint driver removal and calling of
> unregister_region(). And then when the mock type2 device driver is being
> removed, the devm action of unregister_region() is called again, which
> causes a kernel warning about double unregistration of the same region.
> Add a check to see if the devm action is already removed before calling
> unregister_region() for cxl_endpoint_region_autoremove().
> 
> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> 
> ---
> - Need to add a fixes tag when the patch commit that introduces
>   cxl_endpoint_region_autoremove() is stable.
> ---
>  drivers/cxl/core/region.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
> index 61a3efdbf3c9..1a7c0895bb88 100644
> --- a/drivers/cxl/core/region.c
> +++ b/drivers/cxl/core/region.c
> @@ -4296,7 +4296,8 @@ static void cxl_endpoint_region_autoremove(void *_cxlr)
>  	struct cxl_root_decoder *cxlrd = cxlr->cxlrd;
>  	struct cxl_port *port = cxlrd_to_port(cxlrd);
>  
> -	devm_release_action(port->uport_dev, unregister_region, cxlr);
> +	if (!devm_remove_action_nowarn(port->uport_dev, unregister_region, cxlr))
> +		unregister_region(cxlr);

Good find, but no, it's always a mistake to use
devm_remove_action_nowarn() outside of Rust.

I am drafting a replacement fix for the issue Sungwoo reported as the
approach there would also apply here. The main observation is that we
need to separate the remove action lifetime from the region lifetime.