Re: [PATCH v2] cxl/memdev: automate cleanup with __free()

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Mon, 23 Jun 2025 14:08:41 +0530
Pranav Tyagi <pranav.tyagi03@gmail.com> wrote:

> Use the scope based resource management (defined in linux/cleanup.h) to
> automate the lifetime control of struct cxl_mbox_transfer_fw. This
> eliminates explicit kfree() calls and makes the code more robust and
> maintainable in presence of early returns.
> 
> Signed-off-by: Pranav Tyagi <pranav.tyagi03@gmail.com>

Resend as I accidentally only sent 1st reply to Pranav.

> ---
>  drivers/cxl/core/memdev.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/cxl/core/memdev.c b/drivers/cxl/core/memdev.c
> index f88a13adf7fa..38f4449f9740 100644
> --- a/drivers/cxl/core/memdev.c
> +++ b/drivers/cxl/core/memdev.c
> @@ -7,6 +7,7 @@
>  #include <linux/slab.h>
>  #include <linux/idr.h>
>  #include <linux/pci.h>
> +#include <linux/cleanup.h>
>  #include <cxlmem.h>
>  #include "trace.h"
>  #include "core.h"
> @@ -802,11 +803,10 @@ static int cxl_mem_activate_fw(struct cxl_memdev_state *mds, int slot)
>  static int cxl_mem_abort_fw_xfer(struct cxl_memdev_state *mds)
>  {
>  	struct cxl_mailbox *cxl_mbox = &mds->cxlds.cxl_mbox;
> -	struct cxl_mbox_transfer_fw *transfer;
>  	struct cxl_mbox_cmd mbox_cmd;
> -	int rc;
> -
> -	transfer = kzalloc(struct_size(transfer, data, 0), GFP_KERNEL);
> +	
> +	struct cxl_mbox_transfer_fw *transfer __free(kfree) =
> +		kzalloc(struct_size(transfer, data, 0), GFP_KERNEL);

This one is fine.

>  	if (!transfer)
>  		return -ENOMEM;
>  
> @@ -821,9 +821,7 @@ static int cxl_mem_abort_fw_xfer(struct cxl_memdev_state *mds)
>  
>  	transfer->action = CXL_FW_TRANSFER_ACTION_ABORT;
>  
> -	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
> -	kfree(transfer);
> -	return rc;
> +	return cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
>  }
>  
>  static void cxl_fw_cleanup(struct fw_upload *fwl)
> @@ -880,7 +878,7 @@ static enum fw_upload_err cxl_fw_write(struct fw_upload *fwl, const u8 *data,
>  	struct cxl_dev_state *cxlds = &mds->cxlds;
>  	struct cxl_mailbox *cxl_mbox = &cxlds->cxl_mbox;
>  	struct cxl_memdev *cxlmd = cxlds->cxlmd;
> -	struct cxl_mbox_transfer_fw *transfer;
> +	struct cxl_mbox_transfer_fw *transfer __free(kfree);

This one is not.

Look at the comments in cleanup.h and consider if this obeys the rules
on use of this feature that are laid out there.  As it stands you will have
kfree() of an uninitialized pointer with unpredictable results in some
of the error paths.

>  	struct cxl_mbox_cmd mbox_cmd;
>  	u32 cur_size, remaining;
>  	size_t size_in;
> @@ -949,7 +947,7 @@ static enum fw_upload_err cxl_fw_write(struct fw_upload *fwl, const u8 *data,
>  	rc = cxl_internal_send_cmd(cxl_mbox, &mbox_cmd);
>  	if (rc < 0) {
>  		rc = FW_UPLOAD_ERR_RW_ERROR;
> -		goto out_free;
> +		return rc;
>  	}
>  
>  	*written = cur_size;
> @@ -963,14 +961,11 @@ static enum fw_upload_err cxl_fw_write(struct fw_upload *fwl, const u8 *data,
>  			dev_err(&cxlmd->dev, "Error activating firmware: %d\n",
>  				rc);
>  			rc = FW_UPLOAD_ERR_HW_ERROR;
> -			goto out_free;
> +			return rc;
>  		}
>  	}
>  
>  	rc = FW_UPLOAD_ERR_NONE;

return FW_UPLOAD_ERR_NONE;

> -
> -out_free:
> -	kfree(transfer);
>  	return rc;
>  }
>