Hi Dafna, On Wed, May 20, 2020 at 09:22:29PM +0200, Dafna Hirschfeld wrote: > On 20.05.20 13:11, Helen Koike wrote: > > 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@xxxxxxxxxxxxx> > >> --- > >> 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? > > Hi, > The lock is also used in the hard irq handler in the patch that moves the statistics to threaded interrupt. > The code in the hard irq iterates the buffers queue to find the next buffer available and set the flags of > the ready statistics on it. The rules about spinlocks are as follows. If the lock is never used in an interrupt handler context (or similar context, such as a timer handler for instance), then you can use spin_lock(). The reason is that, in such cases, a section covered by spin_lock()/spin_unlock() will not be preempted on the same CPU by a section that could try to take the same lock. This also applies to locks that are only used in interrupt contexts where interrupts are guaranteed to be disabled. To put it differently, if there's no risk that a spinlock-covered section will be preempted by code that will try to take the same lock, spin_lock() is enough. Otherwise, you should use spin_lock() in code that is guaranteed to run with interrupts disabled (such as hard IRQ handlers, or code that called after another spin_lock_irq() or spin_lock_irqsave() on another lock), spin_lock_irq() in code that is guaranteed to run with interrupts enabled (such as code paths from userspace where you can guarantee interrupts haven't been disabled by, for instance, a spin_lock_irq() in the call stack), and spin_lock_irqsave() when you're not sure. There's a tendency to always use spin_lock_irqsave() just to make sure, and it can indeed avoid potential issues when code is later refactored and assumptions that lead to the selection of the propery spin_lock*() variant change. That's a bit idea in paths where latency is critical, but should otherwise not be too much of a problem. In this patch, as rkisp1_stats_isr() is run in a hard IRQ context with interrupts disabled, there's no need to use spin_lock_irqsave(), spin_lock() is totally fine. > >> 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) -- Regards, Laurent Pinchart
