[PATCH 0/2] Error handling fixes in stm32-dcmi driver

[PATCH 0/2] Error handling fixes in stm32-dcmi driver

[Alain Volmat]

This serie implements fixes in error path of the stm32-dcmi driver.
As part of it, it also fixes the way video device is handled in
order to ensure that it is only created when the attached sensor
is well initialized. (current implementation leads to having a
video device always created and but never deleted upon removal of
the module if sensor initialization is failing)

Alain Volmat (2):
  media: stm32-dcmi: create video dev within notifier bound
  media: stm32-dcmi: fix probe error path & module remove

 drivers/media/platform/stm32/stm32-dcmi.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

[PATCH 1/2] media: stm32-dcmi: create video dev within notifier bound

[Alain Volmat]

In case of an error during the initialization of the sensor,
the video device is still available since created at the
probe of the dcmi driver. Moreover the device wouldn't
be released even when removing the module since the release
is performed as part of the notifier unbind callback
(not called if no sensor is properly initialized).

This patch move the video device creation with the v4l2 notifier
bound handler in order to avoid having a video device created when
an error happen during the pipe (dcmi - sensor) initialization.

This also makes the video device creation symmetric with the
release which is already done within the notifier unbind handler.

Fixes: 37404f91ef8b ("[media] stm32-dcmi: STM32 DCMI camera interface driver")
Signed-off-by: Alain Volmat <alain.volmat@st.com>
---
 drivers/media/platform/stm32/stm32-dcmi.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
index b8931490b83b..5e60d4c6eeeb 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -1747,6 +1747,15 @@ static int dcmi_graph_notify_bound(struct v4l2_async_notifier *notifier,
 
 	dev_dbg(dcmi->dev, "Subdev \"%s\" bound\n", subdev->name);
 
+	ret = video_register_device(dcmi->vdev, VFL_TYPE_VIDEO, -1);
+	if (ret) {
+		dev_err(dcmi->dev, "Failed to register video device\n");
+		return ret;
+	}
+
+	dev_dbg(dcmi->dev, "Device registered as %s\n",
+		video_device_node_name(dcmi->vdev));
+
 	/*
 	 * Link this sub-device to DCMI, it could be
 	 * a parallel camera sensor or a bridge
@@ -1759,10 +1768,11 @@ static int dcmi_graph_notify_bound(struct v4l2_async_notifier *notifier,
 				    &dcmi->vdev->entity, 0,
 				    MEDIA_LNK_FL_IMMUTABLE |
 				    MEDIA_LNK_FL_ENABLED);
-	if (ret)
+	if (ret) {
 		dev_err(dcmi->dev, "Failed to create media pad link with subdev \"%s\"\n",
 			subdev->name);
-	else
+		video_unregister_device(dcmi->vdev);
+	} else
 		dev_dbg(dcmi->dev, "DCMI is now linked to \"%s\"\n",
 			subdev->name);
 
@@ -1974,15 +1984,6 @@ static int dcmi_probe(struct platform_device *pdev)
 	}
 	dcmi->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT;
 
-	ret = video_register_device(dcmi->vdev, VFL_TYPE_VIDEO, -1);
-	if (ret) {
-		dev_err(dcmi->dev, "Failed to register video device\n");
-		goto err_media_entity_cleanup;
-	}
-
-	dev_dbg(dcmi->dev, "Device registered as %s\n",
-		video_device_node_name(dcmi->vdev));
-
 	/* Buffer queue */
 	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
 	q->io_modes = VB2_MMAP | VB2_READ | VB2_DMABUF;
-- 
2.7.4

Re: [PATCH 1/2] media: stm32-dcmi: create video dev within notifier bound

[Hugues FRUCHET]

Reviewed-by: Hugues Fruchet <hugues.fruchet@st.com>

On 7/28/20 8:37 AM, Alain Volmat wrote:
> In case of an error during the initialization of the sensor,
> the video device is still available since created at the
> probe of the dcmi driver. Moreover the device wouldn't
> be released even when removing the module since the release
> is performed as part of the notifier unbind callback
> (not called if no sensor is properly initialized).
> 
> This patch move the video device creation with the v4l2 notifier
> bound handler in order to avoid having a video device created when
> an error happen during the pipe (dcmi - sensor) initialization.
> 
> This also makes the video device creation symmetric with the
> release which is already done within the notifier unbind handler.
> 
> Fixes: 37404f91ef8b ("[media] stm32-dcmi: STM32 DCMI camera interface driver")
> Signed-off-by: Alain Volmat <alain.volmat@st.com>
> ---
>   drivers/media/platform/stm32/stm32-dcmi.c | 23 ++++++++++++-----------
>   1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index b8931490b83b..5e60d4c6eeeb 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -1747,6 +1747,15 @@ static int dcmi_graph_notify_bound(struct v4l2_async_notifier *notifier,
>   
>   	dev_dbg(dcmi->dev, "Subdev \"%s\" bound\n", subdev->name);
>   
> +	ret = video_register_device(dcmi->vdev, VFL_TYPE_VIDEO, -1);
> +	if (ret) {
> +		dev_err(dcmi->dev, "Failed to register video device\n");
> +		return ret;
> +	}
> +
> +	dev_dbg(dcmi->dev, "Device registered as %s\n",
> +		video_device_node_name(dcmi->vdev));
> +
>   	/*
>   	 * Link this sub-device to DCMI, it could be
>   	 * a parallel camera sensor or a bridge
> @@ -1759,10 +1768,11 @@ static int dcmi_graph_notify_bound(struct v4l2_async_notifier *notifier,
>   				    &dcmi->vdev->entity, 0,
>   				    MEDIA_LNK_FL_IMMUTABLE |
>   				    MEDIA_LNK_FL_ENABLED);
> -	if (ret)
> +	if (ret) {
>   		dev_err(dcmi->dev, "Failed to create media pad link with subdev \"%s\"\n",
>   			subdev->name);
> -	else
> +		video_unregister_device(dcmi->vdev);
> +	} else
>   		dev_dbg(dcmi->dev, "DCMI is now linked to \"%s\"\n",
>   			subdev->name);
>   
> @@ -1974,15 +1984,6 @@ static int dcmi_probe(struct platform_device *pdev)
>   	}
>   	dcmi->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT;
>   
> -	ret = video_register_device(dcmi->vdev, VFL_TYPE_VIDEO, -1);
> -	if (ret) {
> -		dev_err(dcmi->dev, "Failed to register video device\n");
> -		goto err_media_entity_cleanup;
> -	}
> -
> -	dev_dbg(dcmi->dev, "Device registered as %s\n",
> -		video_device_node_name(dcmi->vdev));
> -
>   	/* Buffer queue */
>   	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>   	q->io_modes = VB2_MMAP | VB2_READ | VB2_DMABUF;
> 

Re: [PATCH 1/2] media: stm32-dcmi: create video dev within notifier bound

[Hans Verkuil]

On 28/07/2020 08:37, Alain Volmat wrote:
> In case of an error during the initialization of the sensor,
> the video device is still available since created at the
> probe of the dcmi driver. Moreover the device wouldn't
> be released even when removing the module since the release
> is performed as part of the notifier unbind callback
> (not called if no sensor is properly initialized).
> 
> This patch move the video device creation with the v4l2 notifier
> bound handler in order to avoid having a video device created when
> an error happen during the pipe (dcmi - sensor) initialization.
> 
> This also makes the video device creation symmetric with the
> release which is already done within the notifier unbind handler.
> 
> Fixes: 37404f91ef8b ("[media] stm32-dcmi: STM32 DCMI camera interface driver")
> Signed-off-by: Alain Volmat <alain.volmat@st.com>
> ---
>  drivers/media/platform/stm32/stm32-dcmi.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index b8931490b83b..5e60d4c6eeeb 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -1747,6 +1747,15 @@ static int dcmi_graph_notify_bound(struct v4l2_async_notifier *notifier,
>  
>  	dev_dbg(dcmi->dev, "Subdev \"%s\" bound\n", subdev->name);
>  
> +	ret = video_register_device(dcmi->vdev, VFL_TYPE_VIDEO, -1);
> +	if (ret) {
> +		dev_err(dcmi->dev, "Failed to register video device\n");
> +		return ret;
> +	}

Why in the bound callback? The video device is typically created in the complete
callback, since that's the point where everything is ready.

You should not create a video device unless it is ready for use, and that's only
valid at the end of the complete callback.

Regards,

	Hans

> +
> +	dev_dbg(dcmi->dev, "Device registered as %s\n",
> +		video_device_node_name(dcmi->vdev));
> +
>  	/*
>  	 * Link this sub-device to DCMI, it could be
>  	 * a parallel camera sensor or a bridge
> @@ -1759,10 +1768,11 @@ static int dcmi_graph_notify_bound(struct v4l2_async_notifier *notifier,
>  				    &dcmi->vdev->entity, 0,
>  				    MEDIA_LNK_FL_IMMUTABLE |
>  				    MEDIA_LNK_FL_ENABLED);
> -	if (ret)
> +	if (ret) {
>  		dev_err(dcmi->dev, "Failed to create media pad link with subdev \"%s\"\n",
>  			subdev->name);
> -	else
> +		video_unregister_device(dcmi->vdev);
> +	} else
>  		dev_dbg(dcmi->dev, "DCMI is now linked to \"%s\"\n",
>  			subdev->name);
>  
> @@ -1974,15 +1984,6 @@ static int dcmi_probe(struct platform_device *pdev)
>  	}
>  	dcmi->vdev->entity.flags |= MEDIA_ENT_FL_DEFAULT;
>  
> -	ret = video_register_device(dcmi->vdev, VFL_TYPE_VIDEO, -1);
> -	if (ret) {
> -		dev_err(dcmi->dev, "Failed to register video device\n");
> -		goto err_media_entity_cleanup;
> -	}
> -
> -	dev_dbg(dcmi->dev, "Device registered as %s\n",
> -		video_device_node_name(dcmi->vdev));
> -
>  	/* Buffer queue */
>  	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
>  	q->io_modes = VB2_MMAP | VB2_READ | VB2_DMABUF;
> 

[PATCH 2/2] media: stm32-dcmi: fix probe error path & module remove

[Alain Volmat]

This commit add missing vb2_queue_release calls with the
probe error path and module remove.
Missing v4l2_async_notifier_unregister is also added within
the probe error path

Fixes: 37404f91ef8b ("[media] stm32-dcmi: STM32 DCMI camera interface driver")
Signed-off-by: Alain Volmat <alain.volmat@st.com>
---
 drivers/media/platform/stm32/stm32-dcmi.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
index 5e60d4c6eeeb..57830ee691be 100644
--- a/drivers/media/platform/stm32/stm32-dcmi.c
+++ b/drivers/media/platform/stm32/stm32-dcmi.c
@@ -2004,7 +2004,7 @@ static int dcmi_probe(struct platform_device *pdev)
 
 	ret = dcmi_graph_init(dcmi);
 	if (ret < 0)
-		goto err_media_entity_cleanup;
+		goto err_vb2_queue_release;
 
 	/* Reset device */
 	ret = reset_control_assert(dcmi->rstc);
@@ -2030,7 +2030,10 @@ static int dcmi_probe(struct platform_device *pdev)
 	return 0;
 
 err_cleanup:
+	v4l2_async_notifier_unregister(&dcmi->notifier);
 	v4l2_async_notifier_cleanup(&dcmi->notifier);
+err_vb2_queue_release:
+	vb2_queue_release(q);
 err_media_entity_cleanup:
 	media_entity_cleanup(&dcmi->vdev->entity);
 err_device_release:
@@ -2052,6 +2055,7 @@ static int dcmi_remove(struct platform_device *pdev)
 
 	v4l2_async_notifier_unregister(&dcmi->notifier);
 	v4l2_async_notifier_cleanup(&dcmi->notifier);
+	vb2_queue_release(&dcmi->queue);
 	media_entity_cleanup(&dcmi->vdev->entity);
 	v4l2_device_unregister(&dcmi->v4l2_dev);
 	media_device_cleanup(&dcmi->mdev);
-- 
2.7.4

Re: [PATCH 2/2] media: stm32-dcmi: fix probe error path & module remove

[Hugues FRUCHET]

Reviewed-by: Hugues Fruchet <hugues.fruchet@st.com>

On 7/28/20 8:37 AM, Alain Volmat wrote:
> This commit add missing vb2_queue_release calls with the
> probe error path and module remove.
> Missing v4l2_async_notifier_unregister is also added within
> the probe error path
> 
> Fixes: 37404f91ef8b ("[media] stm32-dcmi: STM32 DCMI camera interface driver")
> Signed-off-by: Alain Volmat <alain.volmat@st.com>
> ---
>   drivers/media/platform/stm32/stm32-dcmi.c | 6 +++++-
>   1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index 5e60d4c6eeeb..57830ee691be 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -2004,7 +2004,7 @@ static int dcmi_probe(struct platform_device *pdev)
>   
>   	ret = dcmi_graph_init(dcmi);
>   	if (ret < 0)
> -		goto err_media_entity_cleanup;
> +		goto err_vb2_queue_release;
>   
>   	/* Reset device */
>   	ret = reset_control_assert(dcmi->rstc);
> @@ -2030,7 +2030,10 @@ static int dcmi_probe(struct platform_device *pdev)
>   	return 0;
>   
>   err_cleanup:
> +	v4l2_async_notifier_unregister(&dcmi->notifier);
>   	v4l2_async_notifier_cleanup(&dcmi->notifier);
> +err_vb2_queue_release:
> +	vb2_queue_release(q);
>   err_media_entity_cleanup:
>   	media_entity_cleanup(&dcmi->vdev->entity);
>   err_device_release:
> @@ -2052,6 +2055,7 @@ static int dcmi_remove(struct platform_device *pdev)
>   
>   	v4l2_async_notifier_unregister(&dcmi->notifier);
>   	v4l2_async_notifier_cleanup(&dcmi->notifier);
> +	vb2_queue_release(&dcmi->queue);
>   	media_entity_cleanup(&dcmi->vdev->entity);
>   	v4l2_device_unregister(&dcmi->v4l2_dev);
>   	media_device_cleanup(&dcmi->mdev);
> 

Re: [PATCH 2/2] media: stm32-dcmi: fix probe error path & module remove

[Hans Verkuil]

On 28/07/2020 08:37, Alain Volmat wrote:
> This commit add missing vb2_queue_release calls with the
> probe error path and module remove.

No, vb2_queue_release() should not be called.

See this series for more information:

https://patchwork.linuxtv.org/project/linux-media/cover/20200713113048.1150542-1-hverkuil-cisco@xs4all.nl/

I'm made a PR for that patch series, so hopefully it will be merged soon.

From what I can tell you don't need this patch at all for this driver.

Regards,

	Hans

> Missing v4l2_async_notifier_unregister is also added within
> the probe error path
> 
> Fixes: 37404f91ef8b ("[media] stm32-dcmi: STM32 DCMI camera interface driver")
> Signed-off-by: Alain Volmat <alain.volmat@st.com>
> ---
>  drivers/media/platform/stm32/stm32-dcmi.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/stm32/stm32-dcmi.c b/drivers/media/platform/stm32/stm32-dcmi.c
> index 5e60d4c6eeeb..57830ee691be 100644
> --- a/drivers/media/platform/stm32/stm32-dcmi.c
> +++ b/drivers/media/platform/stm32/stm32-dcmi.c
> @@ -2004,7 +2004,7 @@ static int dcmi_probe(struct platform_device *pdev)
>  
>  	ret = dcmi_graph_init(dcmi);
>  	if (ret < 0)
> -		goto err_media_entity_cleanup;
> +		goto err_vb2_queue_release;
>  
>  	/* Reset device */
>  	ret = reset_control_assert(dcmi->rstc);
> @@ -2030,7 +2030,10 @@ static int dcmi_probe(struct platform_device *pdev)
>  	return 0;
>  
>  err_cleanup:
> +	v4l2_async_notifier_unregister(&dcmi->notifier);
>  	v4l2_async_notifier_cleanup(&dcmi->notifier);
> +err_vb2_queue_release:
> +	vb2_queue_release(q);
>  err_media_entity_cleanup:
>  	media_entity_cleanup(&dcmi->vdev->entity);
>  err_device_release:
> @@ -2052,6 +2055,7 @@ static int dcmi_remove(struct platform_device *pdev)
>  
>  	v4l2_async_notifier_unregister(&dcmi->notifier);
>  	v4l2_async_notifier_cleanup(&dcmi->notifier);
> +	vb2_queue_release(&dcmi->queue);
>  	media_entity_cleanup(&dcmi->vdev->entity);
>  	v4l2_device_unregister(&dcmi->v4l2_dev);
>  	media_device_cleanup(&dcmi->mdev);
>