[PATCH] media: renesas: vsp1: Fix _irqsave and _irq mix

[PATCH] media: renesas: vsp1: Fix _irqsave and _irq mix

[Laurent Pinchart]

The histogram support mixes _irqsave and _irq, causing the following
smatch warning:

     drivers/media/platform/renesas/vsp1/vsp1_histo.c:153 histo_stop_streaming()
     warn: mixing irqsave and irq

The histo_stop_streaming() calls spin_lock_irqsave() followed by
wait_event_lock_irq(). The former hints that interrupts may be disabled
by the caller, while the latter reenables interrupts unconditionally.
This doesn't cause any real bug, as the function is always called with
interrupts enabled, but the pattern is still in correct.

Fix the problem by using spin_lock_irq() instead of spin_lock_irqsave()
in histo_stop_streaming(). While at it, switch to spin_lock_irq() and
spin_lock() as appropriate elsewhere.

Fixes: 99362e32332b ("[media] v4l: vsp1: Add histogram support")
Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
Closes: https://lore.kernel.org/linux-renesas-soc/164d74ff-312c-468f-be64-afa7182cd2f4@moroto.mountain/
Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
---
 .../media/platform/renesas/vsp1/vsp1_histo.c  | 20 ++++++++-----------
 1 file changed, 8 insertions(+), 12 deletions(-)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1_histo.c b/drivers/media/platform/renesas/vsp1/vsp1_histo.c
index 71155282ca11..cd1c8778662e 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_histo.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_histo.c
@@ -36,9 +36,8 @@ struct vsp1_histogram_buffer *
 vsp1_histogram_buffer_get(struct vsp1_histogram *histo)
 {
 	struct vsp1_histogram_buffer *buf = NULL;
-	unsigned long flags;
 
-	spin_lock_irqsave(&histo->irqlock, flags);
+	spin_lock(&histo->irqlock);
 
 	if (list_empty(&histo->irqqueue))
 		goto done;
@@ -49,7 +48,7 @@ vsp1_histogram_buffer_get(struct vsp1_histogram *histo)
 	histo->readout = true;
 
 done:
-	spin_unlock_irqrestore(&histo->irqlock, flags);
+	spin_unlock(&histo->irqlock);
 	return buf;
 }
 
@@ -58,7 +57,6 @@ void vsp1_histogram_buffer_complete(struct vsp1_histogram *histo,
 				    size_t size)
 {
 	struct vsp1_pipeline *pipe = histo->entity.pipe;
-	unsigned long flags;
 
 	/*
 	 * The pipeline pointer is guaranteed to be valid as this function is
@@ -70,10 +68,10 @@ void vsp1_histogram_buffer_complete(struct vsp1_histogram *histo,
 	vb2_set_plane_payload(&buf->buf.vb2_buf, 0, size);
 	vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
 
-	spin_lock_irqsave(&histo->irqlock, flags);
+	spin_lock(&histo->irqlock);
 	histo->readout = false;
 	wake_up(&histo->wait_queue);
-	spin_unlock_irqrestore(&histo->irqlock, flags);
+	spin_unlock(&histo->irqlock);
 }
 
 /* -----------------------------------------------------------------------------
@@ -124,11 +122,10 @@ static void histo_buffer_queue(struct vb2_buffer *vb)
 	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
 	struct vsp1_histogram *histo = vb2_get_drv_priv(vb->vb2_queue);
 	struct vsp1_histogram_buffer *buf = to_vsp1_histogram_buffer(vbuf);
-	unsigned long flags;
 
-	spin_lock_irqsave(&histo->irqlock, flags);
+	spin_lock_irq(&histo->irqlock);
 	list_add_tail(&buf->queue, &histo->irqqueue);
-	spin_unlock_irqrestore(&histo->irqlock, flags);
+	spin_unlock_irq(&histo->irqlock);
 }
 
 static int histo_start_streaming(struct vb2_queue *vq, unsigned int count)
@@ -140,9 +137,8 @@ static void histo_stop_streaming(struct vb2_queue *vq)
 {
 	struct vsp1_histogram *histo = vb2_get_drv_priv(vq);
 	struct vsp1_histogram_buffer *buffer;
-	unsigned long flags;
 
-	spin_lock_irqsave(&histo->irqlock, flags);
+	spin_lock_irq(&histo->irqlock);
 
 	/* Remove all buffers from the IRQ queue. */
 	list_for_each_entry(buffer, &histo->irqqueue, queue)
@@ -152,7 +148,7 @@ static void histo_stop_streaming(struct vb2_queue *vq)
 	/* Wait for the buffer being read out (if any) to complete. */
 	wait_event_lock_irq(histo->wait_queue, !histo->readout, histo->irqlock);
 
-	spin_unlock_irqrestore(&histo->irqlock, flags);
+	spin_unlock_irq(&histo->irqlock);
 }
 
 static const struct vb2_ops histo_video_queue_qops = {

base-commit: e695668af8523b059127dfa8b261c76e7c9cde10
-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: renesas: vsp1: Fix _irqsave and _irq mix

[Sergei Shtylyov]

On 5/5/24 8:45 PM, Laurent Pinchart wrote:

> The histogram support mixes _irqsave and _irq, causing the following
> smatch warning:
> 
>      drivers/media/platform/renesas/vsp1/vsp1_histo.c:153 histo_stop_streaming()
>      warn: mixing irqsave and irq
> 
> The histo_stop_streaming() calls spin_lock_irqsave() followed by
> wait_event_lock_irq(). The former hints that interrupts may be disabled
> by the caller, while the latter reenables interrupts unconditionally.
> This doesn't cause any real bug, as the function is always called with
> interrupts enabled, but the pattern is still in correct.

   Incorrect? :-)

> Fix the problem by using spin_lock_irq() instead of spin_lock_irqsave()
> in histo_stop_streaming(). While at it, switch to spin_lock_irq() and
> spin_lock() as appropriate elsewhere.
> 
> Fixes: 99362e32332b ("[media] v4l: vsp1: Add histogram support")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/linux-renesas-soc/164d74ff-312c-468f-be64-afa7182cd2f4@moroto.mountain/
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  .../media/platform/renesas/vsp1/vsp1_histo.c  | 20 ++++++++-----------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_histo.c b/drivers/media/platform/renesas/vsp1/vsp1_histo.c
> index 71155282ca11..cd1c8778662e 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_histo.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_histo.c
[...]

MBR, Sergey

Re: [PATCH] media: renesas: vsp1: Fix _irqsave and _irq mix

[Kieran Bingham]

Quoting Laurent Pinchart (2024-05-05 18:45:44)
> The histogram support mixes _irqsave and _irq, causing the following
> smatch warning:
> 
>      drivers/media/platform/renesas/vsp1/vsp1_histo.c:153 histo_stop_streaming()
>      warn: mixing irqsave and irq
> 
> The histo_stop_streaming() calls spin_lock_irqsave() followed by
> wait_event_lock_irq(). The former hints that interrupts may be disabled
> by the caller, while the latter reenables interrupts unconditionally.
> This doesn't cause any real bug, as the function is always called with
> interrupts enabled, but the pattern is still in correct.
> 
> Fix the problem by using spin_lock_irq() instead of spin_lock_irqsave()
> in histo_stop_streaming(). While at it, switch to spin_lock_irq() and
> spin_lock() as appropriate elsewhere.


Reviewed-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>

> Fixes: 99362e32332b ("[media] v4l: vsp1: Add histogram support")
> Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> Closes: https://lore.kernel.org/linux-renesas-soc/164d74ff-312c-468f-be64-afa7182cd2f4@moroto.mountain/
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> ---
>  .../media/platform/renesas/vsp1/vsp1_histo.c  | 20 ++++++++-----------
>  1 file changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/media/platform/renesas/vsp1/vsp1_histo.c b/drivers/media/platform/renesas/vsp1/vsp1_histo.c
> index 71155282ca11..cd1c8778662e 100644
> --- a/drivers/media/platform/renesas/vsp1/vsp1_histo.c
> +++ b/drivers/media/platform/renesas/vsp1/vsp1_histo.c
> @@ -36,9 +36,8 @@ struct vsp1_histogram_buffer *
>  vsp1_histogram_buffer_get(struct vsp1_histogram *histo)
>  {
>         struct vsp1_histogram_buffer *buf = NULL;
> -       unsigned long flags;
>  
> -       spin_lock_irqsave(&histo->irqlock, flags);
> +       spin_lock(&histo->irqlock);
>  
>         if (list_empty(&histo->irqqueue))
>                 goto done;
> @@ -49,7 +48,7 @@ vsp1_histogram_buffer_get(struct vsp1_histogram *histo)
>         histo->readout = true;
>  
>  done:
> -       spin_unlock_irqrestore(&histo->irqlock, flags);
> +       spin_unlock(&histo->irqlock);
>         return buf;
>  }
>  
> @@ -58,7 +57,6 @@ void vsp1_histogram_buffer_complete(struct vsp1_histogram *histo,
>                                     size_t size)
>  {
>         struct vsp1_pipeline *pipe = histo->entity.pipe;
> -       unsigned long flags;
>  
>         /*
>          * The pipeline pointer is guaranteed to be valid as this function is
> @@ -70,10 +68,10 @@ void vsp1_histogram_buffer_complete(struct vsp1_histogram *histo,
>         vb2_set_plane_payload(&buf->buf.vb2_buf, 0, size);
>         vb2_buffer_done(&buf->buf.vb2_buf, VB2_BUF_STATE_DONE);
>  
> -       spin_lock_irqsave(&histo->irqlock, flags);
> +       spin_lock(&histo->irqlock);
>         histo->readout = false;
>         wake_up(&histo->wait_queue);
> -       spin_unlock_irqrestore(&histo->irqlock, flags);
> +       spin_unlock(&histo->irqlock);
>  }
>  
>  /* -----------------------------------------------------------------------------
> @@ -124,11 +122,10 @@ static void histo_buffer_queue(struct vb2_buffer *vb)
>         struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>         struct vsp1_histogram *histo = vb2_get_drv_priv(vb->vb2_queue);
>         struct vsp1_histogram_buffer *buf = to_vsp1_histogram_buffer(vbuf);
> -       unsigned long flags;
>  
> -       spin_lock_irqsave(&histo->irqlock, flags);
> +       spin_lock_irq(&histo->irqlock);
>         list_add_tail(&buf->queue, &histo->irqqueue);
> -       spin_unlock_irqrestore(&histo->irqlock, flags);
> +       spin_unlock_irq(&histo->irqlock);
>  }
>  
>  static int histo_start_streaming(struct vb2_queue *vq, unsigned int count)
> @@ -140,9 +137,8 @@ static void histo_stop_streaming(struct vb2_queue *vq)
>  {
>         struct vsp1_histogram *histo = vb2_get_drv_priv(vq);
>         struct vsp1_histogram_buffer *buffer;
> -       unsigned long flags;
>  
> -       spin_lock_irqsave(&histo->irqlock, flags);
> +       spin_lock_irq(&histo->irqlock);
>  
>         /* Remove all buffers from the IRQ queue. */
>         list_for_each_entry(buffer, &histo->irqqueue, queue)
> @@ -152,7 +148,7 @@ static void histo_stop_streaming(struct vb2_queue *vq)
>         /* Wait for the buffer being read out (if any) to complete. */
>         wait_event_lock_irq(histo->wait_queue, !histo->readout, histo->irqlock);
>  
> -       spin_unlock_irqrestore(&histo->irqlock, flags);
> +       spin_unlock_irq(&histo->irqlock);
>  }
>  
>  static const struct vb2_ops histo_video_queue_qops = {
> 
> base-commit: e695668af8523b059127dfa8b261c76e7c9cde10
> -- 
> Regards,
> 
> Laurent Pinchart
>

Re: [PATCH] media: renesas: vsp1: Fix _irqsave and _irq mix

[Laurent Pinchart]

On Tue, May 07, 2024 at 11:11:25AM +0300, Sergei Shtylyov wrote:
> On 5/5/24 8:45 PM, Laurent Pinchart wrote:
> 
> > The histogram support mixes _irqsave and _irq, causing the following
> > smatch warning:
> > 
> >      drivers/media/platform/renesas/vsp1/vsp1_histo.c:153 histo_stop_streaming()
> >      warn: mixing irqsave and irq
> > 
> > The histo_stop_streaming() calls spin_lock_irqsave() followed by
> > wait_event_lock_irq(). The former hints that interrupts may be disabled
> > by the caller, while the latter reenables interrupts unconditionally.
> > This doesn't cause any real bug, as the function is always called with
> > interrupts enabled, but the pattern is still in correct.
> 
>    Incorrect? :-)

Oops :-) I'll fix it, but will likely not send a new version if that's
the only issue.

> > Fix the problem by using spin_lock_irq() instead of spin_lock_irqsave()
> > in histo_stop_streaming(). While at it, switch to spin_lock_irq() and
> > spin_lock() as appropriate elsewhere.
> > 
> > Fixes: 99362e32332b ("[media] v4l: vsp1: Add histogram support")
> > Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> > Closes: https://lore.kernel.org/linux-renesas-soc/164d74ff-312c-468f-be64-afa7182cd2f4@moroto.mountain/
> > Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> > ---
> >  .../media/platform/renesas/vsp1/vsp1_histo.c  | 20 ++++++++-----------
> >  1 file changed, 8 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/media/platform/renesas/vsp1/vsp1_histo.c b/drivers/media/platform/renesas/vsp1/vsp1_histo.c
> > index 71155282ca11..cd1c8778662e 100644
> > --- a/drivers/media/platform/renesas/vsp1/vsp1_histo.c
> > +++ b/drivers/media/platform/renesas/vsp1/vsp1_histo.c
> [...]
> 
> MBR, Sergey

-- 
Regards,

Laurent Pinchart