diff mbox series

[v3,09/12] media: staging: rkisp1: stats: protect write to 'is_streaming' in start_streaming cb

Message ID 20200922113402.12442-10-dafna.hirschfeld@collabora.com
State Accepted
Commit acb00111e906be800cf4b26625e1e0e42d123289
Headers show
Series [v3,01/12] media: staging: rkisp1: params: upon stream stop, iterate a local list to return the buffers | expand

Commit Message

Dafna Hirschfeld Sept. 22, 2020, 11:33 a.m. UTC
The field stats->is_streaming is written in 'start_streaming' callback
without the stats->lock protection.
The isr might run together with the callback so 'spin_lock_irq'
should be used.

Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Acked-by: Helen Koike <helen.koike@collabora.com>
---
 drivers/staging/media/rkisp1/rkisp1-stats.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Tomasz Figa Sept. 25, 2020, 8:47 p.m. UTC | #1
Hi Dafna,

On Tue, Sep 22, 2020 at 01:33:59PM +0200, Dafna Hirschfeld wrote:
> The field stats->is_streaming is written in 'start_streaming' callback
> without the stats->lock protection.
> The isr might run together with the callback so 'spin_lock_irq'
> should be used.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> Acked-by: Helen Koike <helen.koike@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-stats.c | 2 ++
>  1 file changed, 2 insertions(+)
> 

Thank you for the patch. Please see my comments inline.

> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index 6aa18d970f2b..51c64f75fe29 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -157,7 +157,9 @@ rkisp1_stats_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
>  {
>  	struct rkisp1_stats *stats = queue->drv_priv;
>  
> +	spin_lock_irq(&stats->lock);
>  	stats->is_streaming = true;

Do we need this separate stream? This is a similar note to the
parameters patch - it should be possible to handle this based on the
presence of the buffers in the list, without a custom flag.

Best regards,
Tomasz
diff mbox series

Patch

diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
index 6aa18d970f2b..51c64f75fe29 100644
--- a/drivers/staging/media/rkisp1/rkisp1-stats.c
+++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
@@ -157,7 +157,9 @@  rkisp1_stats_vb2_start_streaming(struct vb2_queue *queue, unsigned int count)
 {
 	struct rkisp1_stats *stats = queue->drv_priv;
 
+	spin_lock_irq(&stats->lock);
 	stats->is_streaming = true;
+	spin_unlock_irq(&stats->lock);
 
 	return 0;
 }