On 4/9/19 1:07 PM, Eugen.Hristev@xxxxxxxxxxxxx wrote:
> From: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> ---
> 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;
> }
>
>
