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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux