Re: [PATCH] media: nxp: imx8-isi: Cleanup video device in error path

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Dan,

Thank you for the patch.

On Thu, May 22, 2025 at 03:30:08PM +0100, Daniel Scally wrote:
> mxc_isi_v4l2_init() registers video devices and v4l2 subdevices but
> the video devices are not cleaned up in that function's error path
> which means they're left hanging if it fails. Update the function
> to clean them up properly.
> 
> Signed-off-by: Daniel Scally <dan.scally@ideasonboard.com>
> ---
>  drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> index 1e79b1211b603a71f0427e82a997787110f7e4ac..4a74b988217f504a03dfe330a7bac76cdecc933c 100644
> --- a/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> +++ b/drivers/media/platform/nxp/imx8-isi/imx8-isi-core.c
> @@ -154,7 +154,7 @@ static int mxc_isi_v4l2_init(struct mxc_isi_dev *isi)
>  		if (ret < 0) {
>  			dev_err(isi->dev, "Failed to register pipe%u: %d\n", i,
>  				ret);
> -			goto err_v4l2;
> +			goto err_cleanup_pipe;
>  		}
>  
>  		ret = media_create_pad_link(&isi->crossbar.sd.entity,
> @@ -164,14 +164,14 @@ static int mxc_isi_v4l2_init(struct mxc_isi_dev *isi)
>  					    MEDIA_LNK_FL_IMMUTABLE |
>  					    MEDIA_LNK_FL_ENABLED);
>  		if (ret < 0)
> -			goto err_v4l2;
> +			goto err_cleanup_pipe;
>  	}
>  
>  	/* Register the M2M device. */
>  	ret = mxc_isi_m2m_register(isi, v4l2_dev);
>  	if (ret < 0) {
>  		dev_err(isi->dev, "Failed to register M2M device: %d\n", ret);
> -		goto err_v4l2;
> +		goto err_cleanup_pipe;
>  	}
>  
>  	/* Initialize, fill and register the async notifier. */
> @@ -212,6 +212,9 @@ static int mxc_isi_v4l2_init(struct mxc_isi_dev *isi)
>  err_m2m:
>  	mxc_isi_m2m_unregister(isi);
>  	v4l2_async_nf_cleanup(&isi->notifier);
> +err_cleanup_pipe:
> +	for (i = 0; i < isi->pdata->num_channels; ++i)
> +		mxc_isi_pipe_unregister(&isi->pipes[i]);

mxc_isi_pipe_unregister() calls mxc_isi_video_unregister(), which locks
the video->lock mutex. The mutex is initialized in
mxc_isi_video_register(), so you will have an issue here when reaching
this label from within the pipe registration loop for any non-registered
pipe.

One option to address this would be to return from
mxc_isi_video_unregister() if video->pipe is NULL.

>  err_v4l2:
>  	v4l2_device_unregister(v4l2_dev);
>  err_media:
> 
> ---
> base-commit: b64b134942c8cf4801ea288b3fd38b509aedec21
> change-id: 20250522-djrscally-imx8-isi-fixes-fed2d7cffb1c

-- 
Regards,

Laurent Pinchart