Re: [PATCH 3/5] media: staging: rkisp1: stats: use spin_lock_irqsave for irq_lock

[Helen Koike <helen.koike@collabora.com>]

Hi Dafna,

On 5/12/20 9:05 AM, Dafna Hirschfeld wrote:
> Currently 'spin_lock' is used in order to lock the 'irq_lock'.
> This should be replaced with 'spin_lock_irqsave' since it is
> used in the irq handler.
> 
> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
> ---
>  drivers/staging/media/rkisp1/rkisp1-stats.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/rkisp1/rkisp1-stats.c b/drivers/staging/media/rkisp1/rkisp1-stats.c
> index 12998db955e6..5578fdeb8a18 100644
> --- a/drivers/staging/media/rkisp1/rkisp1-stats.c
> +++ b/drivers/staging/media/rkisp1/rkisp1-stats.c
> @@ -403,9 +403,10 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
>  	struct rkisp1_device *rkisp1 = stats->rkisp1;
>  	struct rkisp1_isp_readout_work *work;
>  	unsigned int isp_mis_tmp = 0;
> +	unsigned long flags;
>  	u32 val;
>  
> -	spin_lock(&stats->irq_lock);
> +	spin_lock_irqsave(&stats->irq_lock, flags);

Since you are moving this function to a threaded irq handler, you won't be in interrupt context.

The spin_lock_irqsave() function disable interrupts for the critical section, are you sure this is
required?

Regards,
Helen

>  
>  	val = RKISP1_STATS_MEAS_MASK;
>  	rkisp1_write(rkisp1, val, RKISP1_CIF_ISP_ICR);
> @@ -435,7 +436,7 @@ void rkisp1_stats_isr(struct rkisp1_stats *stats, u32 isp_ris)
>  	}
>  
>  unlock:
> -	spin_unlock(&stats->irq_lock);
> +	spin_unlock_irqrestore(&stats->irq_lock, flags);
>  }
>  
>  static void rkisp1_init_stats(struct rkisp1_stats *stats)
>