Re: [PATCH 1/2] media: ipu6: Fix ipu7 firmware context leak on stream start

[Sakari Ailus]

Hi David,

Thank you for the set.

On Sun, Sep 27, 2026 at 09:16:30PM +0100, David Carlier wrote:
> ipu7_fw_isys_init() runs on every first stream start, but allocates the
> firmware context and queue configs with devm and never frees them, so
> each STREAMON/STREAMOFF cycle leaks them. Use kzalloc and free them in
> ipu7_fw_isys_cleanup().
> 
> Fixes: 9ab793dbc176 ("media: ipu6: Add ipu7 fw isys ops")
> Signed-off-by: David Carlier <devnexen@xxxxxxxxx>
> ---
>  drivers/media/pci/intel/ipu6/ipu7-fw-isys.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/pci/intel/ipu6/ipu7-fw-isys.c b/drivers/media/pci/intel/ipu6/ipu7-fw-isys.c
> index aee9227fd66c..19e063b22a15 100644
> --- a/drivers/media/pci/intel/ipu6/ipu7-fw-isys.c
> +++ b/drivers/media/pci/intel/ipu6/ipu7-fw-isys.c
> @@ -34,6 +34,8 @@ static void ipu7_fw_isys_cleanup(struct ipu6_isys *isys)
>  	}
>  
>  	isys->fwctx = NULL;
> +	kfree(fwctx->queue_configs);
> +	kfree(fwctx);
>  }
>  
>  static int ipu7_fw_isys_open(struct ipu6_isys *isys)
> @@ -67,8 +69,7 @@ static int ipu7_fw_isys_init(struct ipu6_isys *isys, unsigned int num_streams)
>  	int ret;
>  
>  	/* Allocate and init firmware context. */
> -	fwctx = devm_kzalloc(dev, sizeof(struct ipu7_fw_com_context),
> -			     GFP_KERNEL);
> +	fwctx = kzalloc_obj(*fwctx);
>  	if (!fwctx)
>  		return -ENOMEM;
>  
> @@ -76,10 +77,9 @@ static int ipu7_fw_isys_init(struct ipu6_isys *isys, unsigned int num_streams)
>  	fwctx->num_output_queues = IPU7_INSYS_MAX_OUTPUT_QUEUES;
>  	num_queues = fwctx->num_input_queues + fwctx->num_output_queues;
>  
> -	queue_configs = devm_kcalloc(dev, num_queues, sizeof(*queue_configs),
> -				     GFP_KERNEL);
> +	queue_configs = kzalloc_objs(*queue_configs, num_queues);
>  	if (!queue_configs) {
> -		ipu7_fw_isys_cleanup(isys);
> +		kfree(fwctx);
>  		return -ENOMEM;
>  	}
>  	fwctx->fw_entry = adev->fw_entry;
> @@ -111,7 +111,8 @@ static int ipu7_fw_isys_init(struct ipu6_isys *isys, unsigned int num_streams)
>  				   &fw_config_dma_addr, GFP_KERNEL, 0);
>  	if (!fw_config) {
>  		dev_err(dev, "Failed to allocate isys subsys config.\n");
> -		ipu7_fw_isys_cleanup(isys);
> +		kfree(queue_configs);
> +		kfree(fwctx);

Instead of releasing the memory here, could you add a few labels for error
handling?

Doesn't ipu6_isys_fwcom_cfg_init() suffer from the same?

I think it could also make sense to keep these allocations around until the
device is unbound, but it's best to fix the issues first in any case.

>  		return -ENOMEM;
>  	}
>  	fwctx->fw_config = fw_config;

-- 
Regards,

Sakari Ailus