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

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

[Daniel Scally]

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]);
 err_v4l2:
 	v4l2_device_unregister(v4l2_dev);
 err_media:

---
base-commit: b64b134942c8cf4801ea288b3fd38b509aedec21
change-id: 20250522-djrscally-imx8-isi-fixes-fed2d7cffb1c

Best regards,
-- 
Daniel Scally <dan.scally@ideasonboard.com>

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

[Fabio Estevam]

Hi Daniel,

On Thu, May 22, 2025 at 11:30 AM Daniel Scally
<dan.scally@ideasonboard.com> 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>

It looks like this deserves a Fixes: tag.

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

[Dan Scally]

Hi Fabio

On 22/05/2025 15:32, Fabio Estevam wrote:
> Hi Daniel,
>
> On Thu, May 22, 2025 at 11:30 AM Daniel Scally
> <dan.scally@ideasonboard.com> 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>
> It looks like this deserves a Fixes: tag.


Ah, yes indeed, that should be:


Fixes: cf21f328fcaf ("media: nxp: Add i.MX8 ISI driver")

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

[Laurent Pinchart]

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

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

[Dan Scally]

Hi Laurent

On 22/05/2025 16:26, Laurent Pinchart wrote:
> 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.


Ah - good spot, thanks

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