linux-media.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Dafna Hirschfeld <dafna.hirschfeld@collabora.com>
Cc: Helen Koike <helen.koike@collabora.com>,
	linux-media@vger.kernel.org, ezequiel@collabora.com,
	hverkuil@xs4all.nl, kernel@collabora.com, dafna3@gmail.com,
	sakari.ailus@linux.intel.com, linux-rockchip@lists.infradead.org,
	mchehab@kernel.org
Subject: Re: [PATCH 3/5] media: staging: rkisp1: stats: use spin_lock_irqsave for irq_lock
Date: Thu, 21 May 2020 02:40:23 +0300	[thread overview]
Message-ID: <20200520234023.GC25474@pendragon.ideasonboard.com> (raw)
In-Reply-To: <5c76ffe9-db3e-20a0-f280-75ded6c72e2c@collabora.com>

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@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?
>
> 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

  reply	other threads:[~2020-05-20 23:40 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-05-12 12:05 [PATCH 0/5] media: staging: rkisp1: change workqueue to threaded irq in stats Dafna Hirschfeld
2020-05-12 12:05 ` [PATCH 1/5] media: staging: rkisp1: return IRQ_NONE in isr when irq isn't for ISP Dafna Hirschfeld
2020-05-12 15:42   ` kbuild test robot
2020-05-20 10:58   ` Helen Koike
2020-05-20 20:58     ` Laurent Pinchart
2020-05-12 12:05 ` [PATCH 2/5] media: staging: rkisp1: use a macro for the statistics flags mask Dafna Hirschfeld
2020-05-20 11:03   ` Helen Koike
2020-05-20 23:27   ` Laurent Pinchart
2020-05-12 12:05 ` [PATCH 3/5] media: staging: rkisp1: stats: use spin_lock_irqsave for irq_lock Dafna Hirschfeld
2020-05-20 11:11   ` Helen Koike
2020-05-20 19:22     ` Dafna Hirschfeld
2020-05-20 23:40       ` Laurent Pinchart [this message]
2020-05-12 12:05 ` [PATCH 4/5] media: staging: rkisp1: stats: replace locks wq_lock, irq_lock with one lock Dafna Hirschfeld
2020-05-20 23:48   ` Laurent Pinchart
2020-05-12 12:05 ` [PATCH 5/5] media: staging: rkisp1: replace workqueue with threaded irq for reading statistics registers Dafna Hirschfeld
2020-05-12 16:41   ` kbuild test robot
2020-05-21  0:09   ` Laurent Pinchart
2020-05-21 10:38     ` Tomasz Figa
2020-05-28 19:19       ` Dafna Hirschfeld
2020-05-28 19:35         ` Tomasz Figa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200520234023.GC25474@pendragon.ideasonboard.com \
    --to=laurent.pinchart@ideasonboard.com \
    --cc=dafna.hirschfeld@collabora.com \
    --cc=dafna3@gmail.com \
    --cc=ezequiel@collabora.com \
    --cc=helen.koike@collabora.com \
    --cc=hverkuil@xs4all.nl \
    --cc=kernel@collabora.com \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-rockchip@lists.infradead.org \
    --cc=mchehab@kernel.org \
    --cc=sakari.ailus@linux.intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).