[RESEND PATCH] gpio: omap: Fix lost edge interrupts

[RESEND PATCH] gpio: omap: Fix lost edge interrupts

[Grygorii Strashko]

Now acking of edge irqs happens the following way:
- omap_gpio_irq_handler
  - "isr" = read irq status
  - omap_clear_gpio_irqbank(bank, isr_saved & ~level_mask); 
	^ clear edge status, so irq can be accepted
  - loop while "isr"
	generic_handle_irq()
	 - handle_edge_irq()
	    - desc->irq_data.chip->irq_ack(&desc->irq_data); 
		- omap_gpio_ack_irq()
it might be that at this moment edge IRQ was triggered again and it will be
cleared and IRQ will be lost.

Use handle_simple_irq and clear edge interrupts early without disabling them in
omap_gpio_irq_handler to avoid loosing interrupts.

[1] https://marc.info/?l=linux-omap&m=149004465313534&w=2
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
---
Resend with proper cc list.

 drivers/gpio/gpio-omap.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-omap.c b/drivers/gpio/gpio-omap.c
index dbf869f..0c80052 100644
--- a/drivers/gpio/gpio-omap.c
+++ b/drivers/gpio/gpio-omap.c
@@ -518,7 +518,13 @@ static int omap_gpio_irq_type(struct irq_data *d, unsigned type)
 	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
 		irq_set_handler_locked(d, handle_level_irq);
 	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
-		irq_set_handler_locked(d, handle_edge_irq);
+		/*
+		 * Edge IRQs are already cleared/acked in irq_handler and
+		 * not need to be masked, as result handle_edge_irq()
+		 * logic is excessed here and may cause lose of interrupts.
+		 * So just use handle_simple_irq.
+		 */
+		irq_set_handler_locked(d, handle_simple_irq);
 
 	return 0;
 
@@ -678,7 +684,7 @@ static void omap_gpio_free(struct gpio_chip *chip, unsigned offset)
 static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
 {
 	void __iomem *isr_reg = NULL;
-	u32 isr;
+	u32 enabled, isr, level_mask;
 	unsigned int bit;
 	struct gpio_bank *bank = gpiobank;
 	unsigned long wa_lock_flags;
@@ -691,23 +697,21 @@ static irqreturn_t omap_gpio_irq_handler(int irq, void *gpiobank)
 	pm_runtime_get_sync(bank->chip.parent);
 
 	while (1) {
-		u32 isr_saved, level_mask = 0;
-		u32 enabled;
-
 		raw_spin_lock_irqsave(&bank->lock, lock_flags);
 
 		enabled = omap_get_gpio_irqbank_mask(bank);
-		isr_saved = isr = readl_relaxed(isr_reg) & enabled;
+		isr = readl_relaxed(isr_reg) & enabled;
 
 		if (bank->level_mask)
 			level_mask = bank->level_mask & enabled;
+		else
+			level_mask = 0;
 
 		/* clear edge sensitive interrupts before handler(s) are
 		called so that we don't miss any interrupt occurred while
 		executing them */
-		omap_disable_gpio_irqbank(bank, isr_saved & ~level_mask);
-		omap_clear_gpio_irqbank(bank, isr_saved & ~level_mask);
-		omap_enable_gpio_irqbank(bank, isr_saved & ~level_mask);
+		if (isr & ~level_mask)
+			omap_clear_gpio_irqbank(bank, isr & ~level_mask);
 
 		raw_spin_unlock_irqrestore(&bank->lock, lock_flags);
 
-- 
2.10.1

Re: [RESEND PATCH] gpio: omap: Fix lost edge interrupts

[Santosh Shilimkar]

On 10/3/2017 9:17 AM, Grygorii Strashko wrote:
> Now acking of edge irqs happens the following way:
> - omap_gpio_irq_handler
>    - "isr" = read irq status
>    - omap_clear_gpio_irqbank(bank, isr_saved & ~level_mask);
> 	^ clear edge status, so irq can be accepted
>    - loop while "isr"
> 	generic_handle_irq()
> 	 - handle_edge_irq()
> 	    - desc->irq_data.chip->irq_ack(&desc->irq_data);
> 		- omap_gpio_ack_irq()
> it might be that at this moment edge IRQ was triggered again and it will be
> cleared and IRQ will be lost.
> 
> Use handle_simple_irq and clear edge interrupts early without disabling them in
> omap_gpio_irq_handler to avoid loosing interrupts.
> 
> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Domap-26m-3D149004465313534-26w-3D2&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=XBn1JQGPwR8CsE7xpP3wPlG6DQU7qw8ym65xieNZ4hY&m=-JZAaXlsRBFYNqtZ-2KOemoupa4pL7ka9D3wKn6hX9o&s=c-1XuQUl3_1uYedoNhmY70xCO3fAftWB7cmFxgyC3j4&e=
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> ---
> Resend with proper cc list.
> 
This was one of the concern I was thinking when GPIO IRQ conversion
was done. Grygorii since you did that conversion, can you please
check since I see now that the irq code is becoming increasingly
complex.

Regards,
Santosh

Re: [RESEND PATCH] gpio: omap: Fix lost edge interrupts

[Grygorii Strashko]

Hi Santosh,

On 10/03/2017 11:41 AM, Santosh Shilimkar wrote:
> 
> 
> On 10/3/2017 9:17 AM, Grygorii Strashko wrote:
>> Now acking of edge irqs happens the following way:
>> - omap_gpio_irq_handler
>>    - "isr" = read irq status
>>    - omap_clear_gpio_irqbank(bank, isr_saved & ~level_mask);
>>     ^ clear edge status, so irq can be accepted
>>    - loop while "isr"
>>     generic_handle_irq()
>>      - handle_edge_irq()
>>         - desc->irq_data.chip->irq_ack(&desc->irq_data);
>>         - omap_gpio_ack_irq()
>> it might be that at this moment edge IRQ was triggered again and it 
>> will be
>> cleared and IRQ will be lost.
>>
>> Use handle_simple_irq and clear edge interrupts early without 
>> disabling them in
>> omap_gpio_irq_handler to avoid loosing interrupts.
>>
>> [1] 
>> https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Domap-26m-3D149004465313534-26w-3D2&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=XBn1JQGPwR8CsE7xpP3wPlG6DQU7qw8ym65xieNZ4hY&m=-JZAaXlsRBFYNqtZ-2KOemoupa4pL7ka9D3wKn6hX9o&s=c-1XuQUl3_1uYedoNhmY70xCO3fAftWB7cmFxgyC3j4&e= 
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
>> ---
>> Resend with proper cc list.
>>
> This was one of the concern I was thinking when GPIO IRQ conversion
> was done. Grygorii since you did that conversion, can you please
> check since I see now that the irq code is becoming increasingly
> complex.

This patch was developed in coop with Ladislav and he intensively tested it and
this patch actually simplifies IRQ handling. Also, OMAP driver parts which
this patch touches are ancient. 

 

-- 
regards,
-grygorii

Re: [RESEND PATCH] gpio: omap: Fix lost edge interrupts

[Ladislav Michl]

On Tue, Oct 03, 2017 at 11:52:01AM -0500, Grygorii Strashko wrote:
> Hi Santosh,
> 
> On 10/03/2017 11:41 AM, Santosh Shilimkar wrote:
> > 
> > 
> > On 10/3/2017 9:17 AM, Grygorii Strashko wrote:
> >> Now acking of edge irqs happens the following way:
> >> - omap_gpio_irq_handler
> >>    - "isr" = read irq status
> >>    - omap_clear_gpio_irqbank(bank, isr_saved & ~level_mask);
> >>     ^ clear edge status, so irq can be accepted
> >>    - loop while "isr"
> >>     generic_handle_irq()
> >>      - handle_edge_irq()
> >>         - desc->irq_data.chip->irq_ack(&desc->irq_data);
> >>         - omap_gpio_ack_irq()
> >> it might be that at this moment edge IRQ was triggered again and it 
> >> will be
> >> cleared and IRQ will be lost.
> >>
> >> Use handle_simple_irq and clear edge interrupts early without 
> >> disabling them in
> >> omap_gpio_irq_handler to avoid loosing interrupts.
> >>
> >> [1] 
> >> https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Domap-26m-3D149004465313534-26w-3D2&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=XBn1JQGPwR8CsE7xpP3wPlG6DQU7qw8ym65xieNZ4hY&m=-JZAaXlsRBFYNqtZ-2KOemoupa4pL7ka9D3wKn6hX9o&s=c-1XuQUl3_1uYedoNhmY70xCO3fAftWB7cmFxgyC3j4&e= 
> >>
> >> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> >> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> >> ---
> >> Resend with proper cc list.
> >>
> > This was one of the concern I was thinking when GPIO IRQ conversion
> > was done. Grygorii since you did that conversion, can you please
> > check since I see now that the irq code is becoming increasingly
> > complex.
> 
> This patch was developed in coop with Ladislav and he intensively tested it and
> this patch actually simplifies IRQ handling. Also, OMAP driver parts which
> this patch touches are ancient.

Just for a reference, testing is described here:
https://marc.info/?l=linux-omap&m=149022183102208&w=2
But there's another missing piece to make edge irqs work on OMAPs:
https://marc.info/?l=linux-omap&m=149037823114641&w=2
Tony, what is the status of your patch? Perhaps we should spin-off
separate thread for it...

Best regards,
	ladis

Re: [RESEND PATCH] gpio: omap: Fix lost edge interrupts

[Santosh Shilimkar]

On 10/3/2017 9:52 AM, Grygorii Strashko wrote:
> Hi Santosh,
> 
> On 10/03/2017 11:41 AM, Santosh Shilimkar wrote:
>>

[...]

>> This was one of the concern I was thinking when GPIO IRQ conversion
>> was done. Grygorii since you did that conversion, can you please
>> check since I see now that the irq code is becoming increasingly
>> complex.
> 
> This patch was developed in coop with Ladislav and he intensively tested it and
> this patch actually simplifies IRQ handling. Also, OMAP driver parts which
> this patch touches are ancient.
> 
Good to know that its being tested and thanksfor the background.

regards,
Santosh

Re: [RESEND PATCH] gpio: omap: Fix lost edge interrupts

[Tony Lindgren]

* Ladislav Michl <ladis@linux-mips.org> [171003 10:23]:
> But there's another missing piece to make edge irqs work on OMAPs:
> https://marc.info/?l=linux-omap&m=149037823114641&w=2
> Tony, what is the status of your patch? Perhaps we should spin-off
> separate thread for it...

Oh so did that fix things for you? It never went anywhere as I was waiting
for you to test it.. Yeah let's start a separate thread on that one.

Regards,

Tony

Re: [RESEND PATCH] gpio: omap: Fix lost edge interrupts

[Linus Walleij]

On Tue, Oct 3, 2017 at 6:17 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:

> Now acking of edge irqs happens the following way:
> - omap_gpio_irq_handler
>   - "isr" = read irq status
>   - omap_clear_gpio_irqbank(bank, isr_saved & ~level_mask);
>         ^ clear edge status, so irq can be accepted
>   - loop while "isr"
>         generic_handle_irq()
>          - handle_edge_irq()
>             - desc->irq_data.chip->irq_ack(&desc->irq_data);
>                 - omap_gpio_ack_irq()
> it might be that at this moment edge IRQ was triggered again and it will be
> cleared and IRQ will be lost.
>
> Use handle_simple_irq and clear edge interrupts early without disabling them in
> omap_gpio_irq_handler to avoid loosing interrupts.
>
> [1] https://marc.info/?l=linux-omap&m=149004465313534&w=2
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>

Patch applied for fixes.

I guess it is the right thing to do?

Other maintainers need to tell me if I should hold it back.

Yours,
Linus Walleij

Re: [RESEND PATCH] gpio: omap: Fix lost edge interrupts

[santosh.shilimkar]

On 10/7/17 4:18 AM, Linus Walleij wrote:
> On Tue, Oct 3, 2017 at 6:17 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
> 
>> Now acking of edge irqs happens the following way:
>> - omap_gpio_irq_handler
>>    - "isr" = read irq status
>>    - omap_clear_gpio_irqbank(bank, isr_saved & ~level_mask);
>>          ^ clear edge status, so irq can be accepted
>>    - loop while "isr"
>>          generic_handle_irq()
>>           - handle_edge_irq()
>>              - desc->irq_data.chip->irq_ack(&desc->irq_data);
>>                  - omap_gpio_ack_irq()
>> it might be that at this moment edge IRQ was triggered again and it will be
>> cleared and IRQ will be lost.
>>
>> Use handle_simple_irq and clear edge interrupts early without disabling them in
>> omap_gpio_irq_handler to avoid loosing interrupts.
>>
>> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__marc.info_-3Fl-3Dlinux-2Domap-26m-3D149004465313534-26w-3D2&d=DwIBaQ&c=RoP1YumCXCgaWHvlZYR8PQcxBKCX5YTpkKY057SbK10&r=XBn1JQGPwR8CsE7xpP3wPlG6DQU7qw8ym65xieNZ4hY&m=GC9UD4RPcq1Ss-JanXn4japAFhT1w4RZlq9WqDLSw_o&s=smHFYjIN3O5THK4qP413bHMnr8PaWUv4xyUbbTwXHk4&e=

This link doesn't need to be part of the changelog.

>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> 
> Patch applied for fixes.
> 
> I guess it is the right thing to do?
> 
> Other maintainers need to tell me if I should hold it back.
> 
Am fine with it after Grygorii's reasoning.

Regards,
Santosh