[PATCH for next v1 0/2] gpio: few clean up patches to replace spin_lock_irqsave with spin_lock

[PATCH for next v1 0/2] gpio: few clean up patches to replace spin_lock_irqsave with spin_lock

[Luo Jiaxing]

There is no need to use API with _irqsave in hard IRQ handler, So replace
those with spin_lock.

Luo Jiaxing (2):
  gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in
    omap_gpio_irq_handler()
  gpio: grgpio: Replace spin_lock_irqsave with spin_lock in
    grgpio_irq_handler()

 drivers/gpio/gpio-grgpio.c |  5 ++---
 drivers/gpio/gpio-omap.c   | 15 ++++++---------
 2 files changed, 8 insertions(+), 12 deletions(-)

-- 
2.7.4

[PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Luo Jiaxing]

There is no need to use API with _irqsave in omap_gpio_irq_handler(),
because it already be in a irq-disabled context.

Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
---
 drivers/gpio/gpio-omap.c | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index 41952bb..dc8bbf4 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -560,8 +560,6 @@ static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
 	u32 enabled, isr, edge;
 	unsigned int bit;
 	struct gpio_bank *bank = gpiobank;
-	unsigned long wa_lock_flags;
-	unsigned long lock_flags;
 
 	isr_reg = bank->base + bank->regs->irqstatus;
 	if (WARN_ON(!isr_reg))
@@ -572,7 +570,7 @@ static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
 		return IRQ_NONE;
 
 	while (1) {
-		raw_spin_lock_irqsave(&bank->lock, lock_flags);
+		raw_spin_lock(&bank->lock);
 
 		enabled = omap_get_gpio_irqbank_mask(bank);
 		isr = readl_relaxed(isr_reg) & enabled;
@@ -586,7 +584,7 @@ static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
 		if (edge)
 			omap_clear_gpio_irqbank(bank, edge);
 
-		raw_spin_unlock_irqrestore(&bank->lock, lock_flags);
+		raw_spin_unlock(&bank->lock);
 
 		if (!isr)
 			break;
@@ -595,7 +593,7 @@ static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
 			bit = __ffs(isr);
 			isr &= ~(BIT(bit));
 
-			raw_spin_lock_irqsave(&bank->lock, lock_flags);
+			raw_spin_lock(&bank->lock);
 			/*
 			 * Some chips can't respond to both rising and falling
 			 * at the same time.  If this irq was requested with
@@ -606,15 +604,14 @@ static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
 			if (bank->toggle_mask & (BIT(bit)))
 				omap_toggle_gpio_edge_triggering(bank, bit);
 
-			raw_spin_unlock_irqrestore(&bank->lock, lock_flags);
+			raw_spin_unlock(&bank->lock);
 
-			raw_spin_lock_irqsave(&bank->wa_lock, wa_lock_flags);
+			raw_spin_lock(&bank->wa_lock);
 
 			generic_handle_irq(irq_find_mapping(bank->chip.irq.domain,
 							    bit));
 
-			raw_spin_unlock_irqrestore(&bank->wa_lock,
-						   wa_lock_flags);
+			raw_spin_unlock(&bank->wa_lock);
 		}
 	}
 exit:
-- 
2.7.4

Re: [Linuxarm] [PATCH for next v1 0/2] gpio: few clean up patches to replace spin_lock_irqsave with spin_lock

[luojiaxing]

Sorry, my operation error causes a patch missing from this patch set. I 
re-send the patch set. Please check the new one.

On 2021/2/8 16:56, Luo Jiaxing wrote:
> There is no need to use API with _irqsave in hard IRQ handler, So replace
> those with spin_lock.
>
> Luo Jiaxing (2):
>    gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in
>      omap_gpio_irq_handler()
>    gpio: grgpio: Replace spin_lock_irqsave with spin_lock in
>      grgpio_irq_handler()
>
>   drivers/gpio/gpio-grgpio.c |  5 ++---
>   drivers/gpio/gpio-omap.c   | 15 ++++++---------
>   2 files changed, 8 insertions(+), 12 deletions(-)
>

Re: [Linuxarm] [PATCH for next v1 0/2] gpio: few clean up patches to replace spin_lock_irqsave with spin_lock

[Andy Shevchenko]

On Mon, Feb 8, 2021 at 11:11 AM luojiaxing <luojiaxing@huawei.com> wrote:
>
> Sorry, my operation error causes a patch missing from this patch set. I
> re-send the patch set. Please check the new one.

What is the new one?! You have to give proper versioning and change
log for your series.

> On 2021/2/8 16:56, Luo Jiaxing wrote:
> > There is no need to use API with _irqsave in hard IRQ handler, So replace
> > those with spin_lock.

How do you know that another CPU in the system can't serve the
following interrupt from the hardware at the same time?

-- 
With Best Regards,
Andy Shevchenko

Re: [Linuxarm] [PATCH for next v1 0/2] gpio: few clean up patches to replace spin_lock_irqsave with spin_lock

[luojiaxing]

On 2021/2/8 21:28, Andy Shevchenko wrote:
> On Mon, Feb 8, 2021 at 11:11 AM luojiaxing <luojiaxing@huawei.com> wrote:
>> Sorry, my operation error causes a patch missing from this patch set. I
>> re-send the patch set. Please check the new one.
> What is the new one?! You have to give proper versioning and change
> log for your series.


sure, I will send a new one later, but let me answer your question first.


>
>> On 2021/2/8 16:56, Luo Jiaxing wrote:
>>> There is no need to use API with _irqsave in hard IRQ handler, So replace
>>> those with spin_lock.
> How do you know that another CPU in the system can't serve the
> following interrupt from the hardware at the same time?


Yes, I have some question before.


There are some similar discussion here,  please take a look, Song baohua 
explained it more professionally.

https://lore.kernel.org/lkml/e949a474a9284ac6951813bfc8b34945@hisilicon.com/


Here are some excerpts from the discussion:


I think the code disabling irq in hardIRQ is simply wrong.
Since this commit
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc
genirq: Run irq handlers with interrupts disabled

interrupt handlers are definitely running in a irq-disabled context
unless irq handlers enable them explicitly in the handler to permit
other interrupts.


Thanks

Jiaxing


>

Re: [Linuxarm] [PATCH for next v1 0/2] gpio: few clean up patches to replace spin_lock_irqsave with spin_lock

[Andy Shevchenko]

On Tue, Feb 9, 2021 at 11:24 AM luojiaxing <luojiaxing@huawei.com> wrote:
> On 2021/2/8 21:28, Andy Shevchenko wrote:
> > On Mon, Feb 8, 2021 at 11:11 AM luojiaxing <luojiaxing@huawei.com> wrote:
> >> Sorry, my operation error causes a patch missing from this patch set. I
> >> re-send the patch set. Please check the new one.
> > What is the new one?! You have to give proper versioning and change
> > log for your series.
>
> sure, I will send a new one later, but let me answer your question first.
>
> >> On 2021/2/8 16:56, Luo Jiaxing wrote:
> >>> There is no need to use API with _irqsave in hard IRQ handler, So replace
> >>> those with spin_lock.
> > How do you know that another CPU in the system can't serve the

The keyword here is: *another*.

> > following interrupt from the hardware at the same time?
>
> Yes, I have some question before.
>
> There are some similar discussion here,  please take a look, Song baohua
> explained it more professionally.
>
> https://lore.kernel.org/lkml/e949a474a9284ac6951813bfc8b34945@hisilicon.com/
>
> Here are some excerpts from the discussion:
>
> I think the code disabling irq in hardIRQ is simply wrong.

Why?

> Since this commit
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc
> genirq: Run irq handlers with interrupts disabled
>
> interrupt handlers are definitely running in a irq-disabled context
> unless irq handlers enable them explicitly in the handler to permit
> other interrupts.

This doesn't explain any changes in the behaviour on SMP.
IRQ line can be disabled on a few stages:
 a) on the source (IP that generates an event)
 b) on IRQ router / controller
 c) on CPU side

The commit above is discussing (rightfully!) the problem when all
interrupts are being served by a *single* core. Nobody prevents them
from being served by *different* cores simultaneously. Also, see [1].

[1]: https://www.kernel.org/doc/htmldocs/kernel-locking/cheatsheet.html

-- 
With Best Regards,
Andy Shevchenko

Re: [Linuxarm] [PATCH for next v1 0/2] gpio: few clean up patches to replace spin_lock_irqsave with spin_lock

[luojiaxing]

On 2021/2/9 17:42, Andy Shevchenko wrote:
> On Tue, Feb 9, 2021 at 11:24 AM luojiaxing <luojiaxing@huawei.com> wrote:
>> On 2021/2/8 21:28, Andy Shevchenko wrote:
>>> On Mon, Feb 8, 2021 at 11:11 AM luojiaxing <luojiaxing@huawei.com> wrote:
>>>> Sorry, my operation error causes a patch missing from this patch set. I
>>>> re-send the patch set. Please check the new one.
>>> What is the new one?! You have to give proper versioning and change
>>> log for your series.
>> sure, I will send a new one later, but let me answer your question first.
>>
>>>> On 2021/2/8 16:56, Luo Jiaxing wrote:
>>>>> There is no need to use API with _irqsave in hard IRQ handler, So replace
>>>>> those with spin_lock.
>>> How do you know that another CPU in the system can't serve the
> The keyword here is: *another*.


ooh, sorry, now I got your point.


As to me, I don't think another CPU can serve the IRQ when one CPU 
runing hard IRQ handler,

except it's a per CPU interrupts.


The following is a simple call logic when IRQ come.

elx_irq -> handle_arch_irq -> __handle_domain_irq -> desc->handle_irq -> 
handle_irq_event


Assume that two CPUs receive the same IRQ and enter the preceding 
process. Both of them will go to desc->handle_irq().

In handle_irq(), raw_spin_lock(&desc->lock) always be called first. 
Therefore, even if two CPUs are running handle_irq(),

only one can get the spin lock. Assume that CPU A obtains the spin lock. 
Then CPU A will sets the status of irq_data to

IRQD_IRQ_INPROGRESS in handle_irq_event() and releases the spin lock. 
Even though CPU B gets the spin lock later and

continue to run handle_irq(), but the check of irq_may_run(desc) causes 
it to exit.


so, I think we don't own the situation that two CPU server the hard IRQ 
handler at the same time.


>
>>> following interrupt from the hardware at the same time?
>> Yes, I have some question before.
>>
>> There are some similar discussion here,  please take a look, Song baohua
>> explained it more professionally.
>>
>> https://lore.kernel.org/lkml/e949a474a9284ac6951813bfc8b34945@hisilicon.com/
>>
>> Here are some excerpts from the discussion:
>>
>> I think the code disabling irq in hardIRQ is simply wrong.
> Why?


I mention the following call before.

elx_irq -> handle_arch_irq -> __handle_domain_irq -> desc->handle_irq -> 
handle_irq_event


__handle_domain_irq() will call irq_enter(), it ensures that the IRQ 
processing of the current CPU can not be preempted.

So I think this is the reason why Song baohua said it's not need to 
disable IRQ in hardIRQ handler.


>
>> Since this commit
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc
>> genirq: Run irq handlers with interrupts disabled
>>
>> interrupt handlers are definitely running in a irq-disabled context
>> unless irq handlers enable them explicitly in the handler to permit
>> other interrupts.
> This doesn't explain any changes in the behaviour on SMP.
> IRQ line can be disabled on a few stages:
>   a) on the source (IP that generates an event)
>   b) on IRQ router / controller
>   c) on CPU side


yes, you are right.


>
> The commit above is discussing (rightfully!) the problem when all
> interrupts are being served by a *single* core. Nobody prevents them
> from being served by *different* cores simultaneously. Also, see [1].
>
> [1]: https://www.kernel.org/doc/htmldocs/kernel-locking/cheatsheet.html


I check [1], quite useful description about locking, thanks. But you can

see Table of locking Requirements


Between IRQ handler A and IRQ handle A, it's no need for a SLIS.


Thanks

Jiaxing


>

Re: [Linuxarm] [PATCH for next v1 0/2] gpio: few clean up patches to replace spin_lock_irqsave with spin_lock

[Andy Shevchenko]

On Wed, Feb 10, 2021 at 5:43 AM luojiaxing <luojiaxing@huawei.com> wrote:
> On 2021/2/9 17:42, Andy Shevchenko wrote:
> > On Tue, Feb 9, 2021 at 11:24 AM luojiaxing <luojiaxing@huawei.com> wrote:
> >> On 2021/2/8 21:28, Andy Shevchenko wrote:
> >>> On Mon, Feb 8, 2021 at 11:11 AM luojiaxing <luojiaxing@huawei.com> wrote:
> >>>> On 2021/2/8 16:56, Luo Jiaxing wrote:
> >>>>> There is no need to use API with _irqsave in hard IRQ handler, So replace
> >>>>> those with spin_lock.
> >>> How do you know that another CPU in the system can't serve the
> > The keyword here is: *another*.
>
> ooh, sorry, now I got your point.
>
> As to me, I don't think another CPU can serve the IRQ when one CPU
> runing hard IRQ handler,

Why is it so?
Each CPU can serve IRQs separately.

> except it's a per CPU interrupts.

I didn't get how it is related.

> The following is a simple call logic when IRQ come.
>
> elx_irq -> handle_arch_irq -> __handle_domain_irq -> desc->handle_irq ->
> handle_irq_event

What is `elx_irq()`? I haven't found any mention of this in the kernel
source tree.
But okay, it shouldn't prevent our discussion.

> Assume that two CPUs receive the same IRQ and enter the preceding
> process. Both of them will go to desc->handle_irq().

Ah, I'm talking about the same IRQ by number (like Linux IRQ number,
means from the same source), but with different sequence number (means
two consequent events).

> In handle_irq(), raw_spin_lock(&desc->lock) always be called first.
> Therefore, even if two CPUs are running handle_irq(),
>
> only one can get the spin lock. Assume that CPU A obtains the spin lock.
> Then CPU A will sets the status of irq_data to
>
> IRQD_IRQ_INPROGRESS in handle_irq_event() and releases the spin lock.
> Even though CPU B gets the spin lock later and
>
> continue to run handle_irq(), but the check of irq_may_run(desc) causes
> it to exit.
>
>
> so, I think we don't own the situation that two CPU server the hard IRQ
> handler at the same time.

Okay. Assuming your analysis is correct, have you considered the case
when all IRQ handlers are threaded? (There is a kernel command line
option to force this)

> >>> following interrupt from the hardware at the same time?
> >> Yes, I have some question before.
> >>
> >> There are some similar discussion here,  please take a look, Song baohua
> >> explained it more professionally.
> >>
> >> https://lore.kernel.org/lkml/e949a474a9284ac6951813bfc8b34945@hisilicon.com/
> >>
> >> Here are some excerpts from the discussion:
> >>
> >> I think the code disabling irq in hardIRQ is simply wrong.
> > Why?
>
>
> I mention the following call before.
>
> elx_irq -> handle_arch_irq -> __handle_domain_irq -> desc->handle_irq ->
> handle_irq_event
>
>
> __handle_domain_irq() will call irq_enter(), it ensures that the IRQ
> processing of the current CPU can not be preempted.
>
> So I think this is the reason why Song baohua said it's not need to
> disable IRQ in hardIRQ handler.
>
> >> Since this commit
> >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=e58aa3d2d0cc
> >> genirq: Run irq handlers with interrupts disabled
> >>
> >> interrupt handlers are definitely running in a irq-disabled context
> >> unless irq handlers enable them explicitly in the handler to permit
> >> other interrupts.
> > This doesn't explain any changes in the behaviour on SMP.
> > IRQ line can be disabled on a few stages:
> >   a) on the source (IP that generates an event)
> >   b) on IRQ router / controller
> >   c) on CPU side
>
> yes, you are right.
>
> > The commit above is discussing (rightfully!) the problem when all
> > interrupts are being served by a *single* core. Nobody prevents them
> > from being served by *different* cores simultaneously. Also, see [1].
> >
> > [1]: https://www.kernel.org/doc/htmldocs/kernel-locking/cheatsheet.html
>
> I check [1], quite useful description about locking, thanks. But you can
> see Table of locking Requirements
>
> Between IRQ handler A and IRQ handle A, it's no need for a SLIS.

Right, but it's not the case in the patches you provided.

--
With Best Regards,
Andy Shevchenko

RE: [Linuxarm] Re: [PATCH for next v1 0/2] gpio: few clean up patches to replace spin_lock_irqsave with spin_lock

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Wednesday, February 10, 2021 11:51 PM
> To: luojiaxing <luojiaxing@huawei.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Grygorii Strashko
> <grygorii.strashko@ti.com>; Santosh Shilimkar <ssantosh@kernel.org>; Kevin
> Hilman <khilman@kernel.org>; open list:GPIO SUBSYSTEM
> <linux-gpio@vger.kernel.org>; Linux Kernel Mailing List
> <linux-kernel@vger.kernel.org>; linuxarm@openeuler.org
> Subject: [Linuxarm] Re: [PATCH for next v1 0/2] gpio: few clean up patches to
> replace spin_lock_irqsave with spin_lock
> 
> On Wed, Feb 10, 2021 at 5:43 AM luojiaxing <luojiaxing@huawei.com> wrote:
> > On 2021/2/9 17:42, Andy Shevchenko wrote:
> > > On Tue, Feb 9, 2021 at 11:24 AM luojiaxing <luojiaxing@huawei.com> wrote:
> > >> On 2021/2/8 21:28, Andy Shevchenko wrote:
> > >>> On Mon, Feb 8, 2021 at 11:11 AM luojiaxing <luojiaxing@huawei.com> wrote:
> > >>>> On 2021/2/8 16:56, Luo Jiaxing wrote:
> > >>>>> There is no need to use API with _irqsave in hard IRQ handler, So replace
> > >>>>> those with spin_lock.
> > >>> How do you know that another CPU in the system can't serve the
> > > The keyword here is: *another*.
> >
> > ooh, sorry, now I got your point.
> >
> > As to me, I don't think another CPU can serve the IRQ when one CPU
> > runing hard IRQ handler,
> 
> Why is it so?
> Each CPU can serve IRQs separately.
> 
> > except it's a per CPU interrupts.
> 
> I didn't get how it is related.
> 
> > The following is a simple call logic when IRQ come.
> >
> > elx_irq -> handle_arch_irq -> __handle_domain_irq -> desc->handle_irq ->
> > handle_irq_event
> 
> What is `elx_irq()`? I haven't found any mention of this in the kernel
> source tree.
> But okay, it shouldn't prevent our discussion.
> 
> > Assume that two CPUs receive the same IRQ and enter the preceding
> > process. Both of them will go to desc->handle_irq().
> 
> Ah, I'm talking about the same IRQ by number (like Linux IRQ number,
> means from the same source), but with different sequence number (means
> two consequent events).
> 
> > In handle_irq(), raw_spin_lock(&desc->lock) always be called first.
> > Therefore, even if two CPUs are running handle_irq(),
> >
> > only one can get the spin lock. Assume that CPU A obtains the spin lock.
> > Then CPU A will sets the status of irq_data to
> >
> > IRQD_IRQ_INPROGRESS in handle_irq_event() and releases the spin lock.
> > Even though CPU B gets the spin lock later and
> >
> > continue to run handle_irq(), but the check of irq_may_run(desc) causes
> > it to exit.
> >
> >
> > so, I think we don't own the situation that two CPU server the hard IRQ
> > handler at the same time.
> 
> Okay. Assuming your analysis is correct, have you considered the case
> when all IRQ handlers are threaded? (There is a kernel command line
> option to force this)
> 
> > >>> following interrupt from the hardware at the same time?
> > >> Yes, I have some question before.
> > >>
> > >> There are some similar discussion here,  please take a look, Song baohua
> > >> explained it more professionally.
> > >>
> > >>
> https://lore.kernel.org/lkml/e949a474a9284ac6951813bfc8b34945@hisilicon.co
> m/
> > >>
> > >> Here are some excerpts from the discussion:
> > >>
> > >> I think the code disabling irq in hardIRQ is simply wrong.
> > > Why?
> >
> >
> > I mention the following call before.
> >
> > elx_irq -> handle_arch_irq -> __handle_domain_irq -> desc->handle_irq ->
> > handle_irq_event
> >
> >
> > __handle_domain_irq() will call irq_enter(), it ensures that the IRQ
> > processing of the current CPU can not be preempted.
> >
> > So I think this is the reason why Song baohua said it's not need to
> > disable IRQ in hardIRQ handler.
> >
> > >> Since this commit
> > >>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/
> ?id=e58aa3d2d0cc
> > >> genirq: Run irq handlers with interrupts disabled
> > >>
> > >> interrupt handlers are definitely running in a irq-disabled context
> > >> unless irq handlers enable them explicitly in the handler to permit
> > >> other interrupts.
> > > This doesn't explain any changes in the behaviour on SMP.
> > > IRQ line can be disabled on a few stages:
> > >   a) on the source (IP that generates an event)
> > >   b) on IRQ router / controller
> > >   c) on CPU side
> >
> > yes, you are right.
> >
> > > The commit above is discussing (rightfully!) the problem when all
> > > interrupts are being served by a *single* core. Nobody prevents them
> > > from being served by *different* cores simultaneously. Also, see [1].
> > >
> > > [1]: https://www.kernel.org/doc/htmldocs/kernel-locking/cheatsheet.html
> >
> > I check [1], quite useful description about locking, thanks. But you can
> > see Table of locking Requirements
> >
> > Between IRQ handler A and IRQ handle A, it's no need for a SLIS.
> 
> Right, but it's not the case in the patches you provided.

The code still holds spin_lock. So if two cpus call same IRQ handler,
spin_lock makes them spin; and if interrupts are threaded, spin_lock
makes two threads run the same handler one by one.

> 
> --
> With Best Regards,
> Andy Shevchenko

Thanks
Barry

Re: [Linuxarm] Re: [PATCH for next v1 0/2] gpio: few clean up patches to replace spin_lock_irqsave with spin_lock

[Andy Shevchenko]

On Wed, Feb 10, 2021 at 11:50:45AM +0000, Song Bao Hua (Barry Song) wrote:
> > -----Original Message-----
> > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> > Sent: Wednesday, February 10, 2021 11:51 PM
> > On Wed, Feb 10, 2021 at 5:43 AM luojiaxing <luojiaxing@huawei.com> wrote:
> > > On 2021/2/9 17:42, Andy Shevchenko wrote:

...

> > > Between IRQ handler A and IRQ handle A, it's no need for a SLIS.
> > 
> > Right, but it's not the case in the patches you provided.
> 
> The code still holds spin_lock. So if two cpus call same IRQ handler,
> spin_lock makes them spin; and if interrupts are threaded, spin_lock
> makes two threads run the same handler one by one.

If you run on an SMP system and it happens that spin_lock_irqsave() just
immediately after spin_unlock(), you will get into the troubles. Am I mistaken?

I think this entire activity is a carefully crafted mine field for the future
syzcaller and fuzzers alike. I don't believe there are no side effects in a long
term on all possible systems and configurations (including forced threaded IRQ
handlers).

I would love to see a better explanation in the commit message of such patches
which makes it clear that there are *no* side effects.

For time being, NAK to the all patches of this kind.

-- 
With Best Regards,
Andy Shevchenko

RE: [Linuxarm] Re: [PATCH for next v1 0/2] gpio: few clean up patches to replace spin_lock_irqsave with spin_lock

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Thursday, February 11, 2021 3:57 AM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: luojiaxing <luojiaxing@huawei.com>; Linus Walleij
> <linus.walleij@linaro.org>; Grygorii Strashko <grygorii.strashko@ti.com>;
> Santosh Shilimkar <ssantosh@kernel.org>; Kevin Hilman <khilman@kernel.org>;
> open list:GPIO SUBSYSTEM <linux-gpio@vger.kernel.org>; Linux Kernel Mailing
> List <linux-kernel@vger.kernel.org>; linuxarm@openeuler.org
> Subject: Re: [Linuxarm] Re: [PATCH for next v1 0/2] gpio: few clean up patches
> to replace spin_lock_irqsave with spin_lock
> 
> On Wed, Feb 10, 2021 at 11:50:45AM +0000, Song Bao Hua (Barry Song) wrote:
> > > -----Original Message-----
> > > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> > > Sent: Wednesday, February 10, 2021 11:51 PM
> > > On Wed, Feb 10, 2021 at 5:43 AM luojiaxing <luojiaxing@huawei.com> wrote:
> > > > On 2021/2/9 17:42, Andy Shevchenko wrote:
> 
> ...
> 
> > > > Between IRQ handler A and IRQ handle A, it's no need for a SLIS.
> > >
> > > Right, but it's not the case in the patches you provided.
> >
> > The code still holds spin_lock. So if two cpus call same IRQ handler,
> > spin_lock makes them spin; and if interrupts are threaded, spin_lock
> > makes two threads run the same handler one by one.
> 
> If you run on an SMP system and it happens that spin_lock_irqsave() just
> immediately after spin_unlock(), you will get into the troubles. Am I mistaken?

Hi Andy,
Thanks for your reply.

But I don't agree spin_lock_irqsave() just immediately after spin_unlock()
could a problem on SMP.
When the 1st cpu releases spinlock by spin_unlock, it has completed its section
of accessing the critical data, then 2nd cpu gets the spin_lock. These two CPUs
won't have overlap on accessing the same data.

> 
> I think this entire activity is a carefully crafted mine field for the future
> syzcaller and fuzzers alike. I don't believe there are no side effects in a
> long
> term on all possible systems and configurations (including forced threaded IRQ
> handlers).

Also I don't understand why forced threaded IRQ could be a problem. Since IRQ has
been a thread, this actually makes the situation much simpler than non-threaded
IRQ. Since all threads including those IRQ threads want to hold spin_lock,
they won't access the same critical data at the same time either.

> 
> I would love to see a better explanation in the commit message of such patches
> which makes it clear that there are *no* side effects.
> 

People had the same questions before, But I guess the discussion in this commit
has led to a better commit log:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4eb7d0cd59

> For time being, NAK to the all patches of this kind.

Fair enough, if you expect better explanation, I agree the commit log is too
short.

> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Thanks
Barry

Re: [Linuxarm] Re: [PATCH for next v1 0/2] gpio: few clean up patches to replace spin_lock_irqsave with spin_lock

[Andy Shevchenko]

On Wed, Feb 10, 2021 at 10:42 PM Song Bao Hua (Barry Song)
<song.bao.hua@hisilicon.com> wrote:
> > -----Original Message-----
> > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> > Sent: Thursday, February 11, 2021 3:57 AM
> > On Wed, Feb 10, 2021 at 11:50:45AM +0000, Song Bao Hua (Barry Song) wrote:
> > > > -----Original Message-----
> > > > From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> > > > Sent: Wednesday, February 10, 2021 11:51 PM
> > > > On Wed, Feb 10, 2021 at 5:43 AM luojiaxing <luojiaxing@huawei.com> wrote:
> > > > > On 2021/2/9 17:42, Andy Shevchenko wrote:
> >
> > ...
> >
> > > > > Between IRQ handler A and IRQ handle A, it's no need for a SLIS.
> > > >
> > > > Right, but it's not the case in the patches you provided.
> > >
> > > The code still holds spin_lock. So if two cpus call same IRQ handler,
> > > spin_lock makes them spin; and if interrupts are threaded, spin_lock
> > > makes two threads run the same handler one by one.
> >
> > If you run on an SMP system and it happens that spin_lock_irqsave() just
> > immediately after spin_unlock(), you will get into the troubles. Am I mistaken?
>
> Hi Andy,
> Thanks for your reply.
>
> But I don't agree spin_lock_irqsave() just immediately after spin_unlock()
> could a problem on SMP.
> When the 1st cpu releases spinlock by spin_unlock, it has completed its section
> of accessing the critical data, then 2nd cpu gets the spin_lock. These two CPUs
> won't have overlap on accessing the same data.
>
> >
> > I think this entire activity is a carefully crafted mine field for the future
> > syzcaller and fuzzers alike. I don't believe there are no side effects in a
> > long
> > term on all possible systems and configurations (including forced threaded IRQ
> > handlers).
>
> Also I don't understand why forced threaded IRQ could be a problem. Since IRQ has
> been a thread, this actually makes the situation much simpler than non-threaded
> IRQ. Since all threads including those IRQ threads want to hold spin_lock,
> they won't access the same critical data at the same time either.
>
> >
> > I would love to see a better explanation in the commit message of such patches
> > which makes it clear that there are *no* side effects.
> >
>
> People had the same questions before, But I guess the discussion in this commit
> has led to a better commit log:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4eb7d0cd59
>
> > For time being, NAK to the all patches of this kind.
>
> Fair enough, if you expect better explanation, I agree the commit log is too
> short.

Yes, my main concern that the commit message style as "I feel it's
wrong" is inappropriate to this kind of patch. The one you pointed out
above is better, you may give it even more thrust by explaining why it
was in the first place and what happened between the driver gained
this type of spinlock and your patch.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Grygorii Strashko]

On 08/02/2021 10:56, Luo Jiaxing wrote:
> There is no need to use API with _irqsave in omap_gpio_irq_handler(),
> because it already be in a irq-disabled context.
> 
> Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
> ---
>   drivers/gpio/gpio-omap.c | 15 ++++++---------
>   1 file changed, 6 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
> index 41952bb..dc8bbf4 100644
> --- a/drivers/gpio/gpio-omap.c
> +++ b/drivers/gpio/gpio-omap.c
> @@ -560,8 +560,6 @@ static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
>   	u32 enabled, isr, edge;
>   	unsigned int bit;
>   	struct gpio_bank *bank = gpiobank;
> -	unsigned long wa_lock_flags;
> -	unsigned long lock_flags;
>   
>   	isr_reg = bank->base + bank->regs->irqstatus;
>   	if (WARN_ON(!isr_reg))
> @@ -572,7 +570,7 @@ static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
>   		return IRQ_NONE;
>   
>   	while (1) {
> -		raw_spin_lock_irqsave(&bank->lock, lock_flags);
> +		raw_spin_lock(&bank->lock);
>   
>   		enabled = omap_get_gpio_irqbank_mask(bank);
>   		isr = readl_relaxed(isr_reg) & enabled;
> @@ -586,7 +584,7 @@ static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
>   		if (edge)
>   			omap_clear_gpio_irqbank(bank, edge);
>   
> -		raw_spin_unlock_irqrestore(&bank->lock, lock_flags);
> +		raw_spin_unlock(&bank->lock);
>   
>   		if (!isr)
>   			break;
> @@ -595,7 +593,7 @@ static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
>   			bit = __ffs(isr);
>   			isr &= ~(BIT(bit));
>   
> -			raw_spin_lock_irqsave(&bank->lock, lock_flags);
> +			raw_spin_lock(&bank->lock);
>   			/*
>   			 * Some chips can't respond to both rising and falling
>   			 * at the same time.  If this irq was requested with
> @@ -606,15 +604,14 @@ static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
>   			if (bank->toggle_mask & (BIT(bit)))
>   				omap_toggle_gpio_edge_triggering(bank, bit);
>   
> -			raw_spin_unlock_irqrestore(&bank->lock, lock_flags);
> +			raw_spin_unlock(&bank->lock);
>   
> -			raw_spin_lock_irqsave(&bank->wa_lock, wa_lock_flags);
> +			raw_spin_lock(&bank->wa_lock);
>   
>   			generic_handle_irq(irq_find_mapping(bank->chip.irq.domain,
>   							    bit));
>   
> -			raw_spin_unlock_irqrestore(&bank->wa_lock,
> -						   wa_lock_flags);
> +			raw_spin_unlock(&bank->wa_lock);
>   		}
>   	}
>   exit:
> 

NACK.
Who said that this is always hard IRQ handler?
What about RT-kernel or boot with "threadirqs"?

-- 
Best regards,
grygorii

Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Arnd Bergmann]

On Thu, Feb 11, 2021 at 7:25 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> On 08/02/2021 10:56, Luo Jiaxing wrote:
> > There is no need to use API with _irqsave in omap_gpio_irq_handler(),
> > because it already be in a irq-disabled context.
>
> NACK.
> Who said that this is always hard IRQ handler?
> What about RT-kernel or boot with "threadirqs"?

In those cases, the interrupt handler is just a normal thread, so the
preempt_disable() that is implied by raw_spin_lock() is sufficient.

Disabling interrupts inside of an interrupt handler is always incorrect,
the patch looks like a useful cleanup to me, if only for readability.

       Arnd

Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Grygorii Strashko]

On 11/02/2021 21:39, Arnd Bergmann wrote:
> On Thu, Feb 11, 2021 at 7:25 PM Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>> On 08/02/2021 10:56, Luo Jiaxing wrote:
>>> There is no need to use API with _irqsave in omap_gpio_irq_handler(),
>>> because it already be in a irq-disabled context.
>>
>> NACK.
>> Who said that this is always hard IRQ handler?
>> What about RT-kernel or boot with "threadirqs"?
> 
> In those cases, the interrupt handler is just a normal thread, so the
> preempt_disable() that is implied by raw_spin_lock() is sufficient.
> 
> Disabling interrupts inside of an interrupt handler is always incorrect,
> the patch looks like a useful cleanup to me, if only for readability.

Note. there is also generic_handle_irq() call inside.

-- 
Best regards,
grygorii

RE: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Grygorii Strashko [mailto:grygorii.strashko@ti.com]
> Sent: Friday, February 12, 2021 9:17 AM
> To: Arnd Bergmann <arnd@kernel.org>
> Cc: luojiaxing <luojiaxing@huawei.com>; Linus Walleij
> <linus.walleij@linaro.org>; Andy Shevchenko <andy.shevchenko@gmail.com>; Andy
> Shevchenko <andriy.shevchenko@linux.intel.com>; Santosh Shilimkar
> <ssantosh@kernel.org>; Kevin Hilman <khilman@kernel.org>; open list:GPIO
> SUBSYSTEM <linux-gpio@vger.kernel.org>, linux-kernel@vger.kernel.org
> <linux-kernel@vger.kernel.org>; linuxarm@openeuler.org
> Subject: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace
> raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()
> 
> 
> 
> On 11/02/2021 21:39, Arnd Bergmann wrote:
> > On Thu, Feb 11, 2021 at 7:25 PM Grygorii Strashko
> > <grygorii.strashko@ti.com> wrote:
> >> On 08/02/2021 10:56, Luo Jiaxing wrote:
> >>> There is no need to use API with _irqsave in omap_gpio_irq_handler(),
> >>> because it already be in a irq-disabled context.
> >>
> >> NACK.
> >> Who said that this is always hard IRQ handler?
> >> What about RT-kernel or boot with "threadirqs"?
> >
> > In those cases, the interrupt handler is just a normal thread, so the
> > preempt_disable() that is implied by raw_spin_lock() is sufficient.
> >
> > Disabling interrupts inside of an interrupt handler is always incorrect,
> > the patch looks like a useful cleanup to me, if only for readability.
> 
> Note. there is also generic_handle_irq() call inside.

So generic_handle_irq() is not safe to run in thread thus requires
an interrupt-disabled environment to run? If so, I'd rather this
irqsave moved into generic_handle_irq() rather than asking everyone
calling it to do irqsave.

On the other hand, the author changed a couple of spin_lock_irqsave
to spin_lock, if only this one is incorrect, it seems it is worth a
new version to fix this.

> 
> --
> Best regards,
> grygorii

Thanks
Barry

Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Arnd Bergmann]

On Fri, Feb 12, 2021 at 6:05 AM Song Bao Hua (Barry Song)
<song.bao.hua@hisilicon.com> wrote:
> > -----Original Message-----

> >
> > Note. there is also generic_handle_irq() call inside.
>
> So generic_handle_irq() is not safe to run in thread thus requires
> an interrupt-disabled environment to run? If so, I'd rather this
> irqsave moved into generic_handle_irq() rather than asking everyone
> calling it to do irqsave.

In a preempt-rt kernel, interrupts are run in task context, so they clearly
should not be called with interrupts disabled, that would defeat the
purpose of making them preemptible.

generic_handle_irq() does need to run with in_irq()==true though,
but this should be set by the caller of the gpiochip's handler, and
it is not set by raw_spin_lock_irqsave().

       Arnd

RE: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@kernel.org]
> Sent: Friday, February 12, 2021 10:45 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>; luojiaxing
> <luojiaxing@huawei.com>; Linus Walleij <linus.walleij@linaro.org>; Andy
> Shevchenko <andy.shevchenko@gmail.com>; Andy Shevchenko
> <andriy.shevchenko@linux.intel.com>; Santosh Shilimkar <ssantosh@kernel.org>;
> Kevin Hilman <khilman@kernel.org>; open list:GPIO SUBSYSTEM
> <linux-gpio@vger.kernel.org>, linux-kernel@vger.kernel.org
> <linux-kernel@vger.kernel.org>; linuxarm@openeuler.org
> Subject: Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace
> raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()
> 
> On Fri, Feb 12, 2021 at 6:05 AM Song Bao Hua (Barry Song)
> <song.bao.hua@hisilicon.com> wrote:
> > > -----Original Message-----
> 
> > >
> > > Note. there is also generic_handle_irq() call inside.
> >
> > So generic_handle_irq() is not safe to run in thread thus requires
> > an interrupt-disabled environment to run? If so, I'd rather this
> > irqsave moved into generic_handle_irq() rather than asking everyone
> > calling it to do irqsave.
> 
> In a preempt-rt kernel, interrupts are run in task context, so they clearly
> should not be called with interrupts disabled, that would defeat the
> purpose of making them preemptible.

Yes. Sounds sensible. Irqsave in generic_handle_irq() will defeat
the purpose of RT.

> 
> generic_handle_irq() does need to run with in_irq()==true though,
> but this should be set by the caller of the gpiochip's handler, and
> it is not set by raw_spin_lock_irqsave().
> 

So sounds like this issue of in_irq()=true is still irrelevant with
this patch.

I guess this should have been set by the caller of the gpiochip's
handler somewhere, otherwise, gpiochip's irq handler won't be able
to be threaded. Has it been set somewhere?

>        Arnd

Thanks
Barry

Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Grygorii Strashko]

Hi Arnd,

On 12/02/2021 11:45, Arnd Bergmann wrote:
> On Fri, Feb 12, 2021 at 6:05 AM Song Bao Hua (Barry Song)
> <song.bao.hua@hisilicon.com> wrote:
>>> -----Original Message-----
> 
>>>
>>> Note. there is also generic_handle_irq() call inside.
>>
>> So generic_handle_irq() is not safe to run in thread thus requires
>> an interrupt-disabled environment to run? If so, I'd rather this
>> irqsave moved into generic_handle_irq() rather than asking everyone
>> calling it to do irqsave.
> 
> In a preempt-rt kernel, interrupts are run in task context, so they clearly
> should not be called with interrupts disabled, that would defeat the
> purpose of making them preemptible.
> 
> generic_handle_irq() does need to run with in_irq()==true though,
> but this should be set by the caller of the gpiochip's handler, and
> it is not set by raw_spin_lock_irqsave().

It will produce warning from __handle_irq_event_percpu(), as this is IRQ dispatcher
and generic_handle_irq() will call one of handle_level_irq or handle_edge_irq.

The history behind this is commit 450fa54cfd66 ("gpio: omap: convert to use generic irq handler").

The resent related discussion:
https://lkml.org/lkml/2020/12/5/208



-- 
Best regards,
grygorii

RE: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Grygorii Strashko [mailto:grygorii.strashko@ti.com]
> Sent: Friday, February 12, 2021 11:28 PM
> To: Arnd Bergmann <arnd@kernel.org>; Song Bao Hua (Barry Song)
> <song.bao.hua@hisilicon.com>
> Cc: luojiaxing <luojiaxing@huawei.com>; Linus Walleij
> <linus.walleij@linaro.org>; Andy Shevchenko <andy.shevchenko@gmail.com>; Andy
> Shevchenko <andriy.shevchenko@linux.intel.com>; Santosh Shilimkar
> <ssantosh@kernel.org>; Kevin Hilman <khilman@kernel.org>; open list:GPIO
> SUBSYSTEM <linux-gpio@vger.kernel.org>; linux-kernel@vger.kernel.org;
> linuxarm@openeuler.org
> Subject: Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace
> raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()
> 
> Hi Arnd,
> 
> On 12/02/2021 11:45, Arnd Bergmann wrote:
> > On Fri, Feb 12, 2021 at 6:05 AM Song Bao Hua (Barry Song)
> > <song.bao.hua@hisilicon.com> wrote:
> >>> -----Original Message-----
> >
> >>>
> >>> Note. there is also generic_handle_irq() call inside.
> >>
> >> So generic_handle_irq() is not safe to run in thread thus requires
> >> an interrupt-disabled environment to run? If so, I'd rather this
> >> irqsave moved into generic_handle_irq() rather than asking everyone
> >> calling it to do irqsave.
> >
> > In a preempt-rt kernel, interrupts are run in task context, so they clearly
> > should not be called with interrupts disabled, that would defeat the
> > purpose of making them preemptible.
> >
> > generic_handle_irq() does need to run with in_irq()==true though,
> > but this should be set by the caller of the gpiochip's handler, and
> > it is not set by raw_spin_lock_irqsave().
> 
> It will produce warning from __handle_irq_event_percpu(), as this is IRQ
> dispatcher
> and generic_handle_irq() will call one of handle_level_irq or handle_edge_irq.
> 
> The history behind this is commit 450fa54cfd66 ("gpio: omap: convert to use
> generic irq handler").
> 
> The resent related discussion:
> https://lkml.org/lkml/2020/12/5/208

Ok, second thought. irqsave before generic_handle_irq() won't defeat
the purpose of preemption too much as the dispatched irq handlers by
gpiochip will run in their own threads but not in the thread of
gpiochip's handler.

so looks like this patch can improve by:
* move other raw_spin_lock_irqsave to raw_spin_lock;
* keep the raw_spin_lock_irqsave before generic_handle_irq() to mute
the warning in genirq.

> 
> 
> 
> --
> Best regards,
> Grygorii

Thanks
Barry

Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Andy Shevchenko]

On Fri, Feb 12, 2021 at 10:42:19AM +0000, Song Bao Hua (Barry Song) wrote:
> > From: Grygorii Strashko [mailto:grygorii.strashko@ti.com]
> > Sent: Friday, February 12, 2021 11:28 PM
> > On 12/02/2021 11:45, Arnd Bergmann wrote:
> > > On Fri, Feb 12, 2021 at 6:05 AM Song Bao Hua (Barry Song)
> > > <song.bao.hua@hisilicon.com> wrote:

> > >>> Note. there is also generic_handle_irq() call inside.
> > >>
> > >> So generic_handle_irq() is not safe to run in thread thus requires
> > >> an interrupt-disabled environment to run? If so, I'd rather this
> > >> irqsave moved into generic_handle_irq() rather than asking everyone
> > >> calling it to do irqsave.
> > >
> > > In a preempt-rt kernel, interrupts are run in task context, so they clearly
> > > should not be called with interrupts disabled, that would defeat the
> > > purpose of making them preemptible.
> > >
> > > generic_handle_irq() does need to run with in_irq()==true though,
> > > but this should be set by the caller of the gpiochip's handler, and
> > > it is not set by raw_spin_lock_irqsave().
> > 
> > It will produce warning from __handle_irq_event_percpu(), as this is IRQ
> > dispatcher
> > and generic_handle_irq() will call one of handle_level_irq or handle_edge_irq.
> > 
> > The history behind this is commit 450fa54cfd66 ("gpio: omap: convert to use
> > generic irq handler").
> > 
> > The resent related discussion:
> > https://lkml.org/lkml/2020/12/5/208
> 
> Ok, second thought. irqsave before generic_handle_irq() won't defeat
> the purpose of preemption too much as the dispatched irq handlers by
> gpiochip will run in their own threads but not in the thread of
> gpiochip's handler.
> 
> so looks like this patch can improve by:
> * move other raw_spin_lock_irqsave to raw_spin_lock;
> * keep the raw_spin_lock_irqsave before generic_handle_irq() to mute
> the warning in genirq.

Isn't the idea of irqsave is to prevent dead lock from the process context when
we get interrupt on the *same* CPU?

-- 
With Best Regards,
Andy Shevchenko

Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Arnd Bergmann]

On Fri, Feb 12, 2021 at 11:42 AM Song Bao Hua (Barry Song)
<song.bao.hua@hisilicon.com> wrote:
>
> Ok, second thought. irqsave before generic_handle_irq() won't defeat
> the purpose of preemption too much as the dispatched irq handlers by
> gpiochip will run in their own threads but not in the thread of
> gpiochip's handler.
>
> so looks like this patch can improve by:
> * move other raw_spin_lock_irqsave to raw_spin_lock;
> * keep the raw_spin_lock_irqsave before generic_handle_irq() to mute
> the warning in genirq.

It seems that the other drivers just call handle_nested_irq() instead
of generic_handle_irq().

       Arnd

RE: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> Sent: Friday, February 12, 2021 11:57 PM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> Cc: Grygorii Strashko <grygorii.strashko@ti.com>; Arnd Bergmann
> <arnd@kernel.org>; luojiaxing <luojiaxing@huawei.com>; Linus Walleij
> <linus.walleij@linaro.org>; Santosh Shilimkar <ssantosh@kernel.org>; Kevin
> Hilman <khilman@kernel.org>; open list:GPIO SUBSYSTEM
> <linux-gpio@vger.kernel.org>; linux-kernel@vger.kernel.org;
> linuxarm@openeuler.org
> Subject: Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace
> raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()
> 
> On Fri, Feb 12, 2021 at 10:42:19AM +0000, Song Bao Hua (Barry Song) wrote:
> > > From: Grygorii Strashko [mailto:grygorii.strashko@ti.com]
> > > Sent: Friday, February 12, 2021 11:28 PM
> > > On 12/02/2021 11:45, Arnd Bergmann wrote:
> > > > On Fri, Feb 12, 2021 at 6:05 AM Song Bao Hua (Barry Song)
> > > > <song.bao.hua@hisilicon.com> wrote:
> 
> > > >>> Note. there is also generic_handle_irq() call inside.
> > > >>
> > > >> So generic_handle_irq() is not safe to run in thread thus requires
> > > >> an interrupt-disabled environment to run? If so, I'd rather this
> > > >> irqsave moved into generic_handle_irq() rather than asking everyone
> > > >> calling it to do irqsave.
> > > >
> > > > In a preempt-rt kernel, interrupts are run in task context, so they clearly
> > > > should not be called with interrupts disabled, that would defeat the
> > > > purpose of making them preemptible.
> > > >
> > > > generic_handle_irq() does need to run with in_irq()==true though,
> > > > but this should be set by the caller of the gpiochip's handler, and
> > > > it is not set by raw_spin_lock_irqsave().
> > >
> > > It will produce warning from __handle_irq_event_percpu(), as this is IRQ
> > > dispatcher
> > > and generic_handle_irq() will call one of handle_level_irq or
> handle_edge_irq.
> > >
> > > The history behind this is commit 450fa54cfd66 ("gpio: omap: convert to
> use
> > > generic irq handler").
> > >
> > > The resent related discussion:
> > > https://lkml.org/lkml/2020/12/5/208
> >
> > Ok, second thought. irqsave before generic_handle_irq() won't defeat
> > the purpose of preemption too much as the dispatched irq handlers by
> > gpiochip will run in their own threads but not in the thread of
> > gpiochip's handler.
> >
> > so looks like this patch can improve by:
> > * move other raw_spin_lock_irqsave to raw_spin_lock;
> > * keep the raw_spin_lock_irqsave before generic_handle_irq() to mute
> > the warning in genirq.
> 
> Isn't the idea of irqsave is to prevent dead lock from the process context when
> we get interrupt on the *same* CPU?

Anyway, gpiochip is more tricky as it is also a irq dispatcher. Moving
spin_lock_irq to spin_lock in the irq handler of non-irq dispatcher
driver is almost always correct.

But for gpiochip, would the below be true though it is almost alway true
for non-irq dispatcher?

1. While gpiochip's handler runs in hardIRQ, interrupts are disabled, so no more
interrupt on the same cpu -> No deadleak.

2. While gpiochip's handler runs in threads
* other non-threaded interrupts such as timer tick might come on same cpu,
but they are an irrelevant driver and thus they are not going to get the
lock gpiochip's handler has held. -> no deadlock.
* other devices attached to this gpiochip might get interrupts, since 
gpiochip's handler is running in threads, raw_spin_lock can help avoid
messing up the critical data by two threads -> still no deadlock.

> 
> --
> With Best Regards,
> Andy Shevchenko
> 

Thanks
Barry

Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Andy Shevchenko]

On Fri, Feb 12, 2021 at 11:59:28AM +0100, Arnd Bergmann wrote:
> On Fri, Feb 12, 2021 at 11:42 AM Song Bao Hua (Barry Song)
> <song.bao.hua@hisilicon.com> wrote:
> >
> > Ok, second thought. irqsave before generic_handle_irq() won't defeat
> > the purpose of preemption too much as the dispatched irq handlers by
> > gpiochip will run in their own threads but not in the thread of
> > gpiochip's handler.
> >
> > so looks like this patch can improve by:
> > * move other raw_spin_lock_irqsave to raw_spin_lock;
> > * keep the raw_spin_lock_irqsave before generic_handle_irq() to mute
> > the warning in genirq.
> 
> It seems that the other drivers just call handle_nested_irq() instead
> of generic_handle_irq().

And IIRC all of them request threaded IRQ explicitly.

-- 
With Best Regards,
Andy Shevchenko

Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Grygorii Strashko]

On 12/02/2021 13:29, Song Bao Hua (Barry Song) wrote:
> 
> 
>> -----Original Message-----
>> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
>> Sent: Friday, February 12, 2021 11:57 PM
>> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
>> Cc: Grygorii Strashko <grygorii.strashko@ti.com>; Arnd Bergmann
>> <arnd@kernel.org>; luojiaxing <luojiaxing@huawei.com>; Linus Walleij
>> <linus.walleij@linaro.org>; Santosh Shilimkar <ssantosh@kernel.org>; Kevin
>> Hilman <khilman@kernel.org>; open list:GPIO SUBSYSTEM
>> <linux-gpio@vger.kernel.org>; linux-kernel@vger.kernel.org;
>> linuxarm@openeuler.org
>> Subject: Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace
>> raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()
>>
>> On Fri, Feb 12, 2021 at 10:42:19AM +0000, Song Bao Hua (Barry Song) wrote:
>>>> From: Grygorii Strashko [mailto:grygorii.strashko@ti.com]
>>>> Sent: Friday, February 12, 2021 11:28 PM
>>>> On 12/02/2021 11:45, Arnd Bergmann wrote:
>>>>> On Fri, Feb 12, 2021 at 6:05 AM Song Bao Hua (Barry Song)
>>>>> <song.bao.hua@hisilicon.com> wrote:
>>
>>>>>>> Note. there is also generic_handle_irq() call inside.
>>>>>>
>>>>>> So generic_handle_irq() is not safe to run in thread thus requires
>>>>>> an interrupt-disabled environment to run? If so, I'd rather this
>>>>>> irqsave moved into generic_handle_irq() rather than asking everyone
>>>>>> calling it to do irqsave.
>>>>>
>>>>> In a preempt-rt kernel, interrupts are run in task context, so they clearly
>>>>> should not be called with interrupts disabled, that would defeat the
>>>>> purpose of making them preemptible.
>>>>>
>>>>> generic_handle_irq() does need to run with in_irq()==true though,
>>>>> but this should be set by the caller of the gpiochip's handler, and
>>>>> it is not set by raw_spin_lock_irqsave().
>>>>
>>>> It will produce warning from __handle_irq_event_percpu(), as this is IRQ
>>>> dispatcher
>>>> and generic_handle_irq() will call one of handle_level_irq or
>> handle_edge_irq.
>>>>
>>>> The history behind this is commit 450fa54cfd66 ("gpio: omap: convert to
>> use
>>>> generic irq handler").
>>>>
>>>> The resent related discussion:
>>>> https://lkml.org/lkml/2020/12/5/208
>>>
>>> Ok, second thought. irqsave before generic_handle_irq() won't defeat
>>> the purpose of preemption too much as the dispatched irq handlers by
>>> gpiochip will run in their own threads but not in the thread of
>>> gpiochip's handler.
>>>
>>> so looks like this patch can improve by:
>>> * move other raw_spin_lock_irqsave to raw_spin_lock;
>>> * keep the raw_spin_lock_irqsave before generic_handle_irq() to mute
>>> the warning in genirq.
>>
>> Isn't the idea of irqsave is to prevent dead lock from the process context when
>> we get interrupt on the *same* CPU?
> 
> Anyway, gpiochip is more tricky as it is also a irq dispatcher. Moving
> spin_lock_irq to spin_lock in the irq handler of non-irq dispatcher
> driver is almost always correct.
> 
> But for gpiochip, would the below be true though it is almost alway true
> for non-irq dispatcher?
> 
> 1. While gpiochip's handler runs in hardIRQ, interrupts are disabled, so no more
> interrupt on the same cpu -> No deadleak.
> 
> 2. While gpiochip's handler runs in threads
> * other non-threaded interrupts such as timer tick might come on same cpu,
> but they are an irrelevant driver and thus they are not going to get the
> lock gpiochip's handler has held. -> no deadlock.
> * other devices attached to this gpiochip might get interrupts, since
> gpiochip's handler is running in threads, raw_spin_lock can help avoid
> messing up the critical data by two threads -> still no deadlock.

The worst RT case I can imagine is when gpio API is still called from hard IRQ context by some
other device driver - some toggling for example.
Note. RT or "threadirqs" does not mean gpiochip become sleepable.

In this case:
  threaded handler
    raw_spin_lock
	IRQ from other device
           hard_irq handler
             gpiod_x()
		raw_spin_lock_irqsave() -- oops

But in general, what are the benefit of such changes at all, except better marking call context annotation,
so we are spending so much time on it?


-- 
Best regards,
grygorii

RE: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Grygorii Strashko [mailto:grygorii.strashko@ti.com]
> Sent: Saturday, February 13, 2021 12:53 AM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Andy Shevchenko
> <andy.shevchenko@gmail.com>
> Cc: Arnd Bergmann <arnd@kernel.org>; luojiaxing <luojiaxing@huawei.com>; Linus
> Walleij <linus.walleij@linaro.org>; Santosh Shilimkar <ssantosh@kernel.org>;
> Kevin Hilman <khilman@kernel.org>; open list:GPIO SUBSYSTEM
> <linux-gpio@vger.kernel.org>; linux-kernel@vger.kernel.org;
> linuxarm@openeuler.org
> Subject: Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace
> raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()
> 
> 
> 
> On 12/02/2021 13:29, Song Bao Hua (Barry Song) wrote:
> >
> >
> >> -----Original Message-----
> >> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> >> Sent: Friday, February 12, 2021 11:57 PM
> >> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> >> Cc: Grygorii Strashko <grygorii.strashko@ti.com>; Arnd Bergmann
> >> <arnd@kernel.org>; luojiaxing <luojiaxing@huawei.com>; Linus Walleij
> >> <linus.walleij@linaro.org>; Santosh Shilimkar <ssantosh@kernel.org>; Kevin
> >> Hilman <khilman@kernel.org>; open list:GPIO SUBSYSTEM
> >> <linux-gpio@vger.kernel.org>; linux-kernel@vger.kernel.org;
> >> linuxarm@openeuler.org
> >> Subject: Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace
> >> raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()
> >>
> >> On Fri, Feb 12, 2021 at 10:42:19AM +0000, Song Bao Hua (Barry Song) wrote:
> >>>> From: Grygorii Strashko [mailto:grygorii.strashko@ti.com]
> >>>> Sent: Friday, February 12, 2021 11:28 PM
> >>>> On 12/02/2021 11:45, Arnd Bergmann wrote:
> >>>>> On Fri, Feb 12, 2021 at 6:05 AM Song Bao Hua (Barry Song)
> >>>>> <song.bao.hua@hisilicon.com> wrote:
> >>
> >>>>>>> Note. there is also generic_handle_irq() call inside.
> >>>>>>
> >>>>>> So generic_handle_irq() is not safe to run in thread thus requires
> >>>>>> an interrupt-disabled environment to run? If so, I'd rather this
> >>>>>> irqsave moved into generic_handle_irq() rather than asking everyone
> >>>>>> calling it to do irqsave.
> >>>>>
> >>>>> In a preempt-rt kernel, interrupts are run in task context, so they clearly
> >>>>> should not be called with interrupts disabled, that would defeat the
> >>>>> purpose of making them preemptible.
> >>>>>
> >>>>> generic_handle_irq() does need to run with in_irq()==true though,
> >>>>> but this should be set by the caller of the gpiochip's handler, and
> >>>>> it is not set by raw_spin_lock_irqsave().
> >>>>
> >>>> It will produce warning from __handle_irq_event_percpu(), as this is IRQ
> >>>> dispatcher
> >>>> and generic_handle_irq() will call one of handle_level_irq or
> >> handle_edge_irq.
> >>>>
> >>>> The history behind this is commit 450fa54cfd66 ("gpio: omap: convert to
> >> use
> >>>> generic irq handler").
> >>>>
> >>>> The resent related discussion:
> >>>> https://lkml.org/lkml/2020/12/5/208
> >>>
> >>> Ok, second thought. irqsave before generic_handle_irq() won't defeat
> >>> the purpose of preemption too much as the dispatched irq handlers by
> >>> gpiochip will run in their own threads but not in the thread of
> >>> gpiochip's handler.
> >>>
> >>> so looks like this patch can improve by:
> >>> * move other raw_spin_lock_irqsave to raw_spin_lock;
> >>> * keep the raw_spin_lock_irqsave before generic_handle_irq() to mute
> >>> the warning in genirq.
> >>
> >> Isn't the idea of irqsave is to prevent dead lock from the process context
> when
> >> we get interrupt on the *same* CPU?
> >
> > Anyway, gpiochip is more tricky as it is also a irq dispatcher. Moving
> > spin_lock_irq to spin_lock in the irq handler of non-irq dispatcher
> > driver is almost always correct.
> >
> > But for gpiochip, would the below be true though it is almost alway true
> > for non-irq dispatcher?
> >
> > 1. While gpiochip's handler runs in hardIRQ, interrupts are disabled, so no
> more
> > interrupt on the same cpu -> No deadleak.
> >
> > 2. While gpiochip's handler runs in threads
> > * other non-threaded interrupts such as timer tick might come on same cpu,
> > but they are an irrelevant driver and thus they are not going to get the
> > lock gpiochip's handler has held. -> no deadlock.
> > * other devices attached to this gpiochip might get interrupts, since
> > gpiochip's handler is running in threads, raw_spin_lock can help avoid
> > messing up the critical data by two threads -> still no deadlock.
> 
> The worst RT case I can imagine is when gpio API is still called from hard IRQ
> context by some
> other device driver - some toggling for example.
> Note. RT or "threadirqs" does not mean gpiochip become sleepable.
> 
> In this case:
>   threaded handler
>     raw_spin_lock
> 	IRQ from other device
>            hard_irq handler
>              gpiod_x()
> 		raw_spin_lock_irqsave() -- oops

Actually no oops here. other drivers don't hold the same
spinlock of this driver.

> 
> But in general, what are the benefit of such changes at all, except better marking
> call context annotation,
> so we are spending so much time on it?

TBH, the benefit is really tiny except code cleanup. just curious how things could
be different while it happens in an irq dispatcher's handler.

> 
> 
> --
> Best regards,
> Grygorii

Thanks
Barry

Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Grygorii Strashko]

On 12/02/2021 15:12, Song Bao Hua (Barry Song) wrote:
> 
> 
>> -----Original Message-----
>> From: Grygorii Strashko [mailto:grygorii.strashko@ti.com]
>> Sent: Saturday, February 13, 2021 12:53 AM
>> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Andy Shevchenko
>> <andy.shevchenko@gmail.com>
>> Cc: Arnd Bergmann <arnd@kernel.org>; luojiaxing <luojiaxing@huawei.com>; Linus
>> Walleij <linus.walleij@linaro.org>; Santosh Shilimkar <ssantosh@kernel.org>;
>> Kevin Hilman <khilman@kernel.org>; open list:GPIO SUBSYSTEM
>> <linux-gpio@vger.kernel.org>; linux-kernel@vger.kernel.org;
>> linuxarm@openeuler.org
>> Subject: Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace
>> raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()
>>
>>
>>
>> On 12/02/2021 13:29, Song Bao Hua (Barry Song) wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
>>>> Sent: Friday, February 12, 2021 11:57 PM
>>>> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
>>>> Cc: Grygorii Strashko <grygorii.strashko@ti.com>; Arnd Bergmann
>>>> <arnd@kernel.org>; luojiaxing <luojiaxing@huawei.com>; Linus Walleij
>>>> <linus.walleij@linaro.org>; Santosh Shilimkar <ssantosh@kernel.org>; Kevin
>>>> Hilman <khilman@kernel.org>; open list:GPIO SUBSYSTEM
>>>> <linux-gpio@vger.kernel.org>; linux-kernel@vger.kernel.org;
>>>> linuxarm@openeuler.org
>>>> Subject: Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace
>>>> raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()
>>>>
>>>> On Fri, Feb 12, 2021 at 10:42:19AM +0000, Song Bao Hua (Barry Song) wrote:
>>>>>> From: Grygorii Strashko [mailto:grygorii.strashko@ti.com]
>>>>>> Sent: Friday, February 12, 2021 11:28 PM
>>>>>> On 12/02/2021 11:45, Arnd Bergmann wrote:
>>>>>>> On Fri, Feb 12, 2021 at 6:05 AM Song Bao Hua (Barry Song)
>>>>>>> <song.bao.hua@hisilicon.com> wrote:
>>>>
>>>>>>>>> Note. there is also generic_handle_irq() call inside.
>>>>>>>>
>>>>>>>> So generic_handle_irq() is not safe to run in thread thus requires
>>>>>>>> an interrupt-disabled environment to run? If so, I'd rather this
>>>>>>>> irqsave moved into generic_handle_irq() rather than asking everyone
>>>>>>>> calling it to do irqsave.
>>>>>>>
>>>>>>> In a preempt-rt kernel, interrupts are run in task context, so they clearly
>>>>>>> should not be called with interrupts disabled, that would defeat the
>>>>>>> purpose of making them preemptible.
>>>>>>>
>>>>>>> generic_handle_irq() does need to run with in_irq()==true though,
>>>>>>> but this should be set by the caller of the gpiochip's handler, and
>>>>>>> it is not set by raw_spin_lock_irqsave().
>>>>>>
>>>>>> It will produce warning from __handle_irq_event_percpu(), as this is IRQ
>>>>>> dispatcher
>>>>>> and generic_handle_irq() will call one of handle_level_irq or
>>>> handle_edge_irq.
>>>>>>
>>>>>> The history behind this is commit 450fa54cfd66 ("gpio: omap: convert to
>>>> use
>>>>>> generic irq handler").
>>>>>>
>>>>>> The resent related discussion:
>>>>>> https://lkml.org/lkml/2020/12/5/208
>>>>>
>>>>> Ok, second thought. irqsave before generic_handle_irq() won't defeat
>>>>> the purpose of preemption too much as the dispatched irq handlers by
>>>>> gpiochip will run in their own threads but not in the thread of
>>>>> gpiochip's handler.
>>>>>
>>>>> so looks like this patch can improve by:
>>>>> * move other raw_spin_lock_irqsave to raw_spin_lock;
>>>>> * keep the raw_spin_lock_irqsave before generic_handle_irq() to mute
>>>>> the warning in genirq.
>>>>
>>>> Isn't the idea of irqsave is to prevent dead lock from the process context
>> when
>>>> we get interrupt on the *same* CPU?
>>>
>>> Anyway, gpiochip is more tricky as it is also a irq dispatcher. Moving
>>> spin_lock_irq to spin_lock in the irq handler of non-irq dispatcher
>>> driver is almost always correct.
>>>
>>> But for gpiochip, would the below be true though it is almost alway true
>>> for non-irq dispatcher?
>>>
>>> 1. While gpiochip's handler runs in hardIRQ, interrupts are disabled, so no
>> more
>>> interrupt on the same cpu -> No deadleak.
>>>
>>> 2. While gpiochip's handler runs in threads
>>> * other non-threaded interrupts such as timer tick might come on same cpu,
>>> but they are an irrelevant driver and thus they are not going to get the
>>> lock gpiochip's handler has held. -> no deadlock.
>>> * other devices attached to this gpiochip might get interrupts, since
>>> gpiochip's handler is running in threads, raw_spin_lock can help avoid
>>> messing up the critical data by two threads -> still no deadlock.
>>
>> The worst RT case I can imagine is when gpio API is still called from hard IRQ
>> context by some
>> other device driver - some toggling for example.
>> Note. RT or "threadirqs" does not mean gpiochip become sleepable.
>>
>> In this case:
>>    threaded handler
>>      raw_spin_lock
>> 	IRQ from other device
>>             hard_irq handler
>>               gpiod_x()
>> 		raw_spin_lock_irqsave() -- oops
> 
> Actually no oops here. other drivers don't hold the same
> spinlock of this driver.

huh.
driver/module A requests gpio and uses it in its hard_irq handler by calling GPIO API
(Like gpiod_set_value()), those will go to this driver and end up in omap_gpio_set().

> 
>>
>> But in general, what are the benefit of such changes at all, except better marking
>> call context annotation,
>> so we are spending so much time on it?
> 
> TBH, the benefit is really tiny except code cleanup. just curious how things could
> be different while it happens in an irq dispatcher's handler.


-- 
Best regards,
grygorii

RE: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Grygorii Strashko [mailto:grygorii.strashko@ti.com]
> Sent: Saturday, February 13, 2021 3:09 AM
> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Andy Shevchenko
> <andy.shevchenko@gmail.com>
> Cc: Arnd Bergmann <arnd@kernel.org>; luojiaxing <luojiaxing@huawei.com>; Linus
> Walleij <linus.walleij@linaro.org>; Santosh Shilimkar <ssantosh@kernel.org>;
> Kevin Hilman <khilman@kernel.org>; open list:GPIO SUBSYSTEM
> <linux-gpio@vger.kernel.org>; linux-kernel@vger.kernel.org;
> linuxarm@openeuler.org
> Subject: Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace
> raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()
> 
> 
> 
> On 12/02/2021 15:12, Song Bao Hua (Barry Song) wrote:
> >
> >
> >> -----Original Message-----
> >> From: Grygorii Strashko [mailto:grygorii.strashko@ti.com]
> >> Sent: Saturday, February 13, 2021 12:53 AM
> >> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Andy Shevchenko
> >> <andy.shevchenko@gmail.com>
> >> Cc: Arnd Bergmann <arnd@kernel.org>; luojiaxing <luojiaxing@huawei.com>;
> Linus
> >> Walleij <linus.walleij@linaro.org>; Santosh Shilimkar
> <ssantosh@kernel.org>;
> >> Kevin Hilman <khilman@kernel.org>; open list:GPIO SUBSYSTEM
> >> <linux-gpio@vger.kernel.org>; linux-kernel@vger.kernel.org;
> >> linuxarm@openeuler.org
> >> Subject: Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace
> >> raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()
> >>
> >>
> >>
> >> On 12/02/2021 13:29, Song Bao Hua (Barry Song) wrote:
> >>>
> >>>
> >>>> -----Original Message-----
> >>>> From: Andy Shevchenko [mailto:andy.shevchenko@gmail.com]
> >>>> Sent: Friday, February 12, 2021 11:57 PM
> >>>> To: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>
> >>>> Cc: Grygorii Strashko <grygorii.strashko@ti.com>; Arnd Bergmann
> >>>> <arnd@kernel.org>; luojiaxing <luojiaxing@huawei.com>; Linus Walleij
> >>>> <linus.walleij@linaro.org>; Santosh Shilimkar <ssantosh@kernel.org>;
> Kevin
> >>>> Hilman <khilman@kernel.org>; open list:GPIO SUBSYSTEM
> >>>> <linux-gpio@vger.kernel.org>; linux-kernel@vger.kernel.org;
> >>>> linuxarm@openeuler.org
> >>>> Subject: Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace
> >>>> raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()
> >>>>
> >>>> On Fri, Feb 12, 2021 at 10:42:19AM +0000, Song Bao Hua (Barry Song) wrote:
> >>>>>> From: Grygorii Strashko [mailto:grygorii.strashko@ti.com]
> >>>>>> Sent: Friday, February 12, 2021 11:28 PM
> >>>>>> On 12/02/2021 11:45, Arnd Bergmann wrote:
> >>>>>>> On Fri, Feb 12, 2021 at 6:05 AM Song Bao Hua (Barry Song)
> >>>>>>> <song.bao.hua@hisilicon.com> wrote:
> >>>>
> >>>>>>>>> Note. there is also generic_handle_irq() call inside.
> >>>>>>>>
> >>>>>>>> So generic_handle_irq() is not safe to run in thread thus requires
> >>>>>>>> an interrupt-disabled environment to run? If so, I'd rather this
> >>>>>>>> irqsave moved into generic_handle_irq() rather than asking everyone
> >>>>>>>> calling it to do irqsave.
> >>>>>>>
> >>>>>>> In a preempt-rt kernel, interrupts are run in task context, so they
> clearly
> >>>>>>> should not be called with interrupts disabled, that would defeat the
> >>>>>>> purpose of making them preemptible.
> >>>>>>>
> >>>>>>> generic_handle_irq() does need to run with in_irq()==true though,
> >>>>>>> but this should be set by the caller of the gpiochip's handler, and
> >>>>>>> it is not set by raw_spin_lock_irqsave().
> >>>>>>
> >>>>>> It will produce warning from __handle_irq_event_percpu(), as this is
> IRQ
> >>>>>> dispatcher
> >>>>>> and generic_handle_irq() will call one of handle_level_irq or
> >>>> handle_edge_irq.
> >>>>>>
> >>>>>> The history behind this is commit 450fa54cfd66 ("gpio: omap: convert
> to
> >>>> use
> >>>>>> generic irq handler").
> >>>>>>
> >>>>>> The resent related discussion:
> >>>>>> https://lkml.org/lkml/2020/12/5/208
> >>>>>
> >>>>> Ok, second thought. irqsave before generic_handle_irq() won't defeat
> >>>>> the purpose of preemption too much as the dispatched irq handlers by
> >>>>> gpiochip will run in their own threads but not in the thread of
> >>>>> gpiochip's handler.
> >>>>>
> >>>>> so looks like this patch can improve by:
> >>>>> * move other raw_spin_lock_irqsave to raw_spin_lock;
> >>>>> * keep the raw_spin_lock_irqsave before generic_handle_irq() to mute
> >>>>> the warning in genirq.
> >>>>
> >>>> Isn't the idea of irqsave is to prevent dead lock from the process context
> >> when
> >>>> we get interrupt on the *same* CPU?
> >>>
> >>> Anyway, gpiochip is more tricky as it is also a irq dispatcher. Moving
> >>> spin_lock_irq to spin_lock in the irq handler of non-irq dispatcher
> >>> driver is almost always correct.
> >>>
> >>> But for gpiochip, would the below be true though it is almost alway true
> >>> for non-irq dispatcher?
> >>>
> >>> 1. While gpiochip's handler runs in hardIRQ, interrupts are disabled, so
> no
> >> more
> >>> interrupt on the same cpu -> No deadleak.
> >>>
> >>> 2. While gpiochip's handler runs in threads
> >>> * other non-threaded interrupts such as timer tick might come on same cpu,
> >>> but they are an irrelevant driver and thus they are not going to get the
> >>> lock gpiochip's handler has held. -> no deadlock.
> >>> * other devices attached to this gpiochip might get interrupts, since
> >>> gpiochip's handler is running in threads, raw_spin_lock can help avoid
> >>> messing up the critical data by two threads -> still no deadlock.
> >>
> >> The worst RT case I can imagine is when gpio API is still called from hard
> IRQ
> >> context by some
> >> other device driver - some toggling for example.
> >> Note. RT or "threadirqs" does not mean gpiochip become sleepable.
> >>
> >> In this case:
> >>    threaded handler
> >>      raw_spin_lock
> >> 	IRQ from other device
> >>             hard_irq handler
> >>               gpiod_x()
> >> 		raw_spin_lock_irqsave() -- oops
> >
> > Actually no oops here. other drivers don't hold the same
> > spinlock of this driver.
> 
> huh.
> driver/module A requests gpio and uses it in its hard_irq handler by calling
> GPIO API
> (Like gpiod_set_value()), those will go to this driver and end up in
> omap_gpio_set().

Yes, this could be a corner though it doesn't make any sense
to use IRQF_NO_THREAD for this kind of driver/module A on rt
as this will defeat the purpose of preemption by adding a long
irqsoff section.

Since it cannot completely avoid this lockdep issue, I think
that it is pointless to continue struggling with this patch
which is changing an irq dispatcher driver any more. 

> 
> >
> >>
> >> But in general, what are the benefit of such changes at all, except better
> marking
> >> call context annotation,
> >> so we are spending so much time on it?
> >
> > TBH, the benefit is really tiny except code cleanup. just curious how things
> could
> > be different while it happens in an irq dispatcher's handler.
> 
> 
> --
> Best regards,
> Grygorii

Thanks
Barry

Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Arnd Bergmann]

On Fri, Feb 12, 2021 at 12:53 PM Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
>
> The worst RT case I can imagine is when gpio API is still called from hard IRQ context by some
> other device driver - some toggling for example.
> Note. RT or "threadirqs" does not mean gpiochip become sleepable.
>
> In this case:
>   threaded handler
>     raw_spin_lock
>         IRQ from other device
>            hard_irq handler
>              gpiod_x()
>                 raw_spin_lock_irqsave() -- oops
>

Good point, I had missed the fact that drivers can call gpio functions from
hardirq context when I replied earlier, gpio is clearly special here.

          Arnd

RE: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()

[Song Bao Hua (Barry Song)]

> -----Original Message-----
> From: Arnd Bergmann [mailto:arnd@kernel.org]
> Sent: Saturday, February 13, 2021 9:23 AM
> To: Grygorii Strashko <grygorii.strashko@ti.com>
> Cc: Song Bao Hua (Barry Song) <song.bao.hua@hisilicon.com>; Andy Shevchenko
> <andy.shevchenko@gmail.com>; luojiaxing <luojiaxing@huawei.com>; Linus
> Walleij <linus.walleij@linaro.org>; Santosh Shilimkar <ssantosh@kernel.org>;
> Kevin Hilman <khilman@kernel.org>; open list:GPIO SUBSYSTEM
> <linux-gpio@vger.kernel.org>; linux-kernel@vger.kernel.org;
> linuxarm@openeuler.org
> Subject: Re: [Linuxarm] Re: [PATCH for next v1 1/2] gpio: omap: Replace
> raw_spin_lock_irqsave with raw_spin_lock in omap_gpio_irq_handler()
> 
> On Fri, Feb 12, 2021 at 12:53 PM Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
> >
> > The worst RT case I can imagine is when gpio API is still called from hard
> IRQ context by some
> > other device driver - some toggling for example.
> > Note. RT or "threadirqs" does not mean gpiochip become sleepable.
> >
> > In this case:
> >   threaded handler
> >     raw_spin_lock
> >         IRQ from other device
> >            hard_irq handler
> >              gpiod_x()
> >                 raw_spin_lock_irqsave() -- oops
> >
> 
> Good point, I had missed the fact that drivers can call gpio functions from
> hardirq context when I replied earlier, gpio is clearly special here.


Yes. Gpio provides APIs, thus, other drivers can go directly into the
territory of gpio driver.

Another one which is even more special might be m68k, which I cc-ed you
yesterday:
https://lore.kernel.org/lkml/c46ddb954cfe45d9849c911271d7ec23@hisilicon.com/

> 
>           Arnd

Thanks
Barry

[PATCH for next v1 0/2] gpio: few clean up patches to replace spin_lock_irqsave with spin_lock

[Luo Jiaxing]

There is no need to use API with _irqsave in hard IRQ handler, So replace
those with spin_lock.

Luo Jiaxing (2):
  gpio: omap: Replace raw_spin_lock_irqsave with raw_spin_lock in
    omap_gpio_irq_handler()
  gpio: grgpio: Replace spin_lock_irqsave with spin_lock in
    grgpio_irq_handler()

 drivers/gpio/gpio-grgpio.c |  5 ++---
 drivers/gpio/gpio-omap.c   | 15 ++++++---------
 2 files changed, 8 insertions(+), 12 deletions(-)

-- 
2.7.4