diff mbox series

[1/7] media: atmel: atmel-isc: add safe checks and fixed wrong ISC state in error case

Message ID 1554807715-2353-2-git-send-email-eugen.hristev@microchip.com (mailing list archive)
State New, archived
Headers show
Series media: atmel: atmel-isc: new features | expand

Commit Message

Eugen Hristev April 9, 2019, 11:07 a.m. UTC 
From: Eugen Hristev <eugen.hristev@microchip.com>

This adds safety checks on some scenarios:
- start streaming but streaming is already started
- start streaming but no buffer in the dma queue
- spin lock is not released in error scenario
- no frame is configured but dma is requested to start
- configure ISC may have been called without need, before checking if buffer is
ok.

Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
---
 drivers/media/platform/atmel/atmel-isc.c | 32 +++++++++++++++++++++++++++-----
 1 file changed, 27 insertions(+), 5 deletions(-)

Comments

Hans Verkuil April 10, 2019, 2:19 p.m. UTC | #1 
On 4/9/19 1:07 PM, Eugen.Hristev@microchip.com wrote:
> From: Eugen Hristev <eugen.hristev@microchip.com>
> 
> This adds safety checks on some scenarios:
> - start streaming but streaming is already started

Can't happen. vb2 checks for that.

> - start streaming but no buffer in the dma queue

Can't happen since min_buffers_needed is > 0. So start_streaming will
never be called unless at least one buffer is queued.

> - spin lock is not released in error scenario
> - no frame is configured but dma is requested to start

Can this ever happen? It's set by start_streaming when you know there is
at least one buffer, so this seems overkill.

Regards,

	Hans

> - configure ISC may have been called without need, before checking if buffer is
> ok.
> 
> Signed-off-by: Eugen Hristev <eugen.hristev@microchip.com>
> ---
>  drivers/media/platform/atmel/atmel-isc.c | 32 +++++++++++++++++++++++++++-----
>  1 file changed, 27 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
> index a10db16..3c19761 100644
> --- a/drivers/media/platform/atmel/atmel-isc.c
> +++ b/drivers/media/platform/atmel/atmel-isc.c
> @@ -722,6 +722,11 @@ static void isc_start_dma(struct isc_device *isc)
>  	u32 dctrl_dview;
>  	dma_addr_t addr0;
>  
> +	if (!isc->cur_frm) {
> +		v4l2_err(&isc->v4l2_dev, "Video buffer not available\n");
> +		return;
> +	}
> +
>  	addr0 = vb2_dma_contig_plane_dma_addr(&isc->cur_frm->vb.vb2_buf, 0);
>  	regmap_write(regmap, ISC_DAD0, addr0);
>  
> @@ -886,6 +891,9 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	unsigned long flags;
>  	int ret;
>  
> +	if (vb2_is_streaming(&isc->vb2_vidq))
> +		return -EBUSY;
> +
>  	/* Enable stream on the sub device */
>  	ret = v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 1);
>  	if (ret && ret != -ENOIOCTLCMD) {
> @@ -896,6 +904,20 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  	pm_runtime_get_sync(isc->dev);
>  
> +	spin_lock_irqsave(&isc->dma_queue_lock, flags);
> +
> +	isc->sequence = 0;
> +	isc->stop = false;
> +	reinit_completion(&isc->comp);
> +
> +	if (list_empty(&isc->dma_queue)) {
> +		v4l2_err(&isc->v4l2_dev, "dma queue empty\n");
> +		ret = -EINVAL;
> +		goto err_configure_unlock;
> +	}
> +
> +	spin_unlock_irqrestore(&isc->dma_queue_lock, flags);
> +
>  	ret = isc_configure(isc);
>  	if (unlikely(ret))
>  		goto err_configure;
> @@ -905,10 +927,6 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  	spin_lock_irqsave(&isc->dma_queue_lock, flags);
>  
> -	isc->sequence = 0;
> -	isc->stop = false;
> -	reinit_completion(&isc->comp);
> -
>  	isc->cur_frm = list_first_entry(&isc->dma_queue,
>  					struct isc_buffer, list);
>  	list_del(&isc->cur_frm->list);
> @@ -919,8 +937,11 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  	return 0;
>  
> +err_configure_unlock:
> +	spin_unlock_irqrestore(&isc->dma_queue_lock, flags);
> +
>  err_configure:
> -	pm_runtime_put_sync(isc->dev);
> +	isc->stop = true;
>  
>  	v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 0);
>  
> @@ -931,6 +952,7 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	INIT_LIST_HEAD(&isc->dma_queue);
>  	spin_unlock_irqrestore(&isc->dma_queue_lock, flags);
>  
> +	pm_runtime_put_sync(isc->dev);
>  	return ret;
>  }
>  
>
diff mbox series

Patch

diff --git a/drivers/media/platform/atmel/atmel-isc.c b/drivers/media/platform/atmel/atmel-isc.c
index a10db16..3c19761 100644
--- a/drivers/media/platform/atmel/atmel-isc.c
+++ b/drivers/media/platform/atmel/atmel-isc.c
@@ -722,6 +722,11 @@  static void isc_start_dma(struct isc_device *isc)
 	u32 dctrl_dview;
 	dma_addr_t addr0;
 
+	if (!isc->cur_frm) {
+		v4l2_err(&isc->v4l2_dev, "Video buffer not available\n");
+		return;
+	}
+
 	addr0 = vb2_dma_contig_plane_dma_addr(&isc->cur_frm->vb.vb2_buf, 0);
 	regmap_write(regmap, ISC_DAD0, addr0);
 
@@ -886,6 +891,9 @@  static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 	unsigned long flags;
 	int ret;
 
+	if (vb2_is_streaming(&isc->vb2_vidq))
+		return -EBUSY;
+
 	/* Enable stream on the sub device */
 	ret = v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 1);
 	if (ret && ret != -ENOIOCTLCMD) {
@@ -896,6 +904,20 @@  static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	pm_runtime_get_sync(isc->dev);
 
+	spin_lock_irqsave(&isc->dma_queue_lock, flags);
+
+	isc->sequence = 0;
+	isc->stop = false;
+	reinit_completion(&isc->comp);
+
+	if (list_empty(&isc->dma_queue)) {
+		v4l2_err(&isc->v4l2_dev, "dma queue empty\n");
+		ret = -EINVAL;
+		goto err_configure_unlock;
+	}
+
+	spin_unlock_irqrestore(&isc->dma_queue_lock, flags);
+
 	ret = isc_configure(isc);
 	if (unlikely(ret))
 		goto err_configure;
@@ -905,10 +927,6 @@  static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	spin_lock_irqsave(&isc->dma_queue_lock, flags);
 
-	isc->sequence = 0;
-	isc->stop = false;
-	reinit_completion(&isc->comp);
-
 	isc->cur_frm = list_first_entry(&isc->dma_queue,
 					struct isc_buffer, list);
 	list_del(&isc->cur_frm->list);
@@ -919,8 +937,11 @@  static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	return 0;
 
+err_configure_unlock:
+	spin_unlock_irqrestore(&isc->dma_queue_lock, flags);
+
 err_configure:
-	pm_runtime_put_sync(isc->dev);
+	isc->stop = true;
 
 	v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 0);
 
@@ -931,6 +952,7 @@  static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 	INIT_LIST_HEAD(&isc->dma_queue);
 	spin_unlock_irqrestore(&isc->dma_queue_lock, flags);
 
+	pm_runtime_put_sync(isc->dev);
 	return ret;
 }