Re: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

Re: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

[Doug Anderson]

Tomasz,

On Thu, Jun 13, 2013 at 3:54 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> Hi Doug,
>
> On Wednesday 12 of June 2013 10:33:19 Doug Anderson wrote:
>> A level-triggered interrupt should be acked after the interrupt line
>> becomes inactive and before it is unmasked, or else another interrupt
>> will be immediately triggered.  Acking before or after calling the
>> handler is not enough.
>
> Nice catch.
>
> I guess that pinctrl-s3c64xx will need similar fix as well, won't it?

It needs this whole series of 3, probably.  The mask and unmask need
the lock and as well as the acking for level interrupts.

I don't have any way to test that code but it's a pretty simple change
to make.  Do you want to do it or do you have an idea of someone who
should?


> I think you can eliminate most of the code by doing this following way:
>
>         if (irqd_get_trigger_type(irqd) & IRQ_TYPE_LEVEL_MASK)
>                 exynos_gpio_irq_ack(irqd);

Duh, right.  OK, v2 coming shortly.  Thank you for pointing out the
right way to do this!  :)

-Doug

Re: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

[Tomasz Figa]

On Thursday 13 of June 2013 09:34:43 Doug Anderson wrote:
> Tomasz,
> 
> On Thu, Jun 13, 2013 at 3:54 AM, Tomasz Figa <tomasz.figa@gmail.com> 
wrote:
> > Hi Doug,
> > 
> > On Wednesday 12 of June 2013 10:33:19 Doug Anderson wrote:
> >> A level-triggered interrupt should be acked after the interrupt line
> >> becomes inactive and before it is unmasked, or else another interrupt
> >> will be immediately triggered.  Acking before or after calling the
> >> handler is not enough.
> > 
> > Nice catch.
> > 
> > I guess that pinctrl-s3c64xx will need similar fix as well, won't it?
> 
> It needs this whole series of 3, probably.  The mask and unmask need
> the lock and as well as the acking for level interrupts.
> 
> I don't have any way to test that code but it's a pretty simple change
> to make.  Do you want to do it or do you have an idea of someone who
> should?

I'll take care of s3c64xx, probably as a part of my patches finally adding 
DT support for it, as without them the pinctrl-s3c64xx driver is just 
sitting there unused.

> > I think you can eliminate most of the code by doing this following 
way:
> >         if (irqd_get_trigger_type(irqd) & IRQ_TYPE_LEVEL_MASK)
> >         
> >                 exynos_gpio_irq_ack(irqd);
> 
> Duh, right.  OK, v2 coming shortly.

Good!

> Thank you for pointing out the
> right way to do this!  :)

You're welcome.

Best regards,
Tomasz

Re: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

[Doug Anderson]

Kukjin,

On Thu, Jun 13, 2013 at 5:04 AM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> Doug Anderson wrote:
>>
>> A level-triggered interrupt should be acked after the interrupt line
>> becomes inactive and before it is unmasked, or else another interrupt
>> will be immediately triggered.  Acking before or after calling the
>> handler is not enough.
>>
>> Signed-off-by: Luigi Semenzato <semenzato@chromium.org>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>
> BTW, probably we need a similar fixing in the mach-exynos/common.c file
> before pinct기 for distro...

Is anyone using the functions in mach-exynos/common.c file anymore?  I
thought that non-dt exynos support was going away and then we could
just delete a whole lot of code from that file.

-Doug

Re: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

[Tomasz Figa]

On Thursday 13 of June 2013 09:38:33 Doug Anderson wrote:
> Kukjin,
> 
> On Thu, Jun 13, 2013 at 5:04 AM, Kukjin Kim <kgene.kim@samsung.com> 
wrote:
> > Doug Anderson wrote:
> >> A level-triggered interrupt should be acked after the interrupt line
> >> becomes inactive and before it is unmasked, or else another interrupt
> >> will be immediately triggered.  Acking before or after calling the
> >> handler is not enough.
> >> 
> >> Signed-off-by: Luigi Semenzato <semenzato@chromium.org>
> >> Signed-off-by: Doug Anderson <dianders@chromium.org>
> > 
> > BTW, probably we need a similar fixing in the mach-exynos/common.c
> > file
> > before pinct기 for distro...
> 
> Is anyone using the functions in mach-exynos/common.c file anymore?  I
> thought that non-dt exynos support was going away and then we could
> just delete a whole lot of code from that file.

I think Kukjin meant stable kernels that support Exynos boards using board 
files and without pinctrl. Would make sense to have them fixed as well, I 
guess.

Best regards,
Tomasz

> 
> -Doug

Re: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

[Doug Anderson]

Tomasz,

On Thu, Jun 13, 2013 at 9:42 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> > BTW, probably we need a similar fixing in the mach-exynos/common.c
>> > file
>> > before pinct기 for distro...
>>
>> Is anyone using the functions in mach-exynos/common.c file anymore?  I
>> thought that non-dt exynos support was going away and then we could
>> just delete a whole lot of code from that file.
>
> I think Kukjin meant stable kernels that support Exynos boards using board
> files and without pinctrl. Would make sense to have them fixed as well, I
> guess.

Ah, makes sense.  Kukjin: do you know of someone who needs this
(someone who is picking up linux-stable updates for exynos)?  I don't
think it's important for ChromeOS for this particular patch.  If
there's someone who needs this to officially land on linux-stable I'd
be happy to review their backport of this patch.

-Doug

RE: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

[Kukjin Kim]

Doug Anderson wrote:
> 
> Tomasz,
> 
> On Thu, Jun 13, 2013 at 9:42 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> >> > BTW, probably we need a similar fixing in the mach-exynos/common.c
> >> > file
> >> > before pinct기 for distro...
> >>
> >> Is anyone using the functions in mach-exynos/common.c file anymore?  I
> >> thought that non-dt exynos support was going away and then we could
> >> just delete a whole lot of code from that file.
> >
> > I think Kukjin meant stable kernels that support Exynos boards using
> board
> > files and without pinctrl. Would make sense to have them fixed as well,
> I
> > guess.
>
Yes, correct. Thanks, Tomasz.

> Ah, makes sense.  Kukjin: do you know of someone who needs this
> (someone who is picking up linux-stable updates for exynos)?  I don't
> think it's important for ChromeOS for this particular patch.  If
> there's someone who needs this to officially land on linux-stable I'd
> be happy to review their backport of this patch.
> 
As you know, developing something like Android, Tizen use the stable kernel (long-term? I'm not sure) and there was a problem about this issue. So I mean, would be fixed for the stable kernel.

Thanks,
- Kukjin

Re: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

[Doug Anderson]

Kukjin

<take 2, not in HTML mode>

On Thu, Jun 13, 2013 at 4:13 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> Doug Anderson wrote:
>>
>> Tomasz,
>>
>> On Thu, Jun 13, 2013 at 9:42 AM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
>> >> > BTW, probably we need a similar fixing in the mach-exynos/common.c
>> >> > file
>> >> > before pinct기 for distro...
>> >>
>> >> Is anyone using the functions in mach-exynos/common.c file anymore?  I
>> >> thought that non-dt exynos support was going away and then we could
>> >> just delete a whole lot of code from that file.
>> >
>> > I think Kukjin meant stable kernels that support Exynos boards using
>> board
>> > files and without pinctrl. Would make sense to have them fixed as well,
>> I
>> > guess.
>>
> Yes, correct. Thanks, Tomasz.
>
>> Ah, makes sense.  Kukjin: do you know of someone who needs this
>> (someone who is picking up linux-stable updates for exynos)?  I don't
>> think it's important for ChromeOS for this particular patch.  If
>> there's someone who needs this to officially land on linux-stable I'd
>> be happy to review their backport of this patch.
>>
> As you know, developing something like Android, Tizen use the stable kernel (long-term? I'm not sure) and there was a problem about this issue. So I mean, would be fixed for the stable kernel.

Sure, but do they actually pull in from linux-stable periodically?
I'd imagine that they have a private tree and that it would be their
job to backport any fixes onto their kernel.

RE: [PATCH 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

[Kukjin Kim]

Doug Anderson wrote:
> 
> Kukjin
> 
> <take 2, not in HTML mode>
> 
Oops, sorry.

> On Thu, Jun 13, 2013 at 4:13 PM, Kukjin Kim <kgene.kim@samsung.com> wrote:
> > Doug Anderson wrote:
> >>
> >> Tomasz,
> >>
> >> On Thu, Jun 13, 2013 at 9:42 AM, Tomasz Figa <tomasz.figa@gmail.com>
> wrote:
> >> >> > BTW, probably we need a similar fixing in the mach-exynos/common.c
> >> >> > file
> >> >> > before pinct기 for distro...
> >> >>
> >> >> Is anyone using the functions in mach-exynos/common.c file anymore?
> I
> >> >> thought that non-dt exynos support was going away and then we could
> >> >> just delete a whole lot of code from that file.
> >> >
> >> > I think Kukjin meant stable kernels that support Exynos boards using
> >> board
> >> > files and without pinctrl. Would make sense to have them fixed as
> well,
> >> I
> >> > guess.
> >>
> > Yes, correct. Thanks, Tomasz.
> >
> >> Ah, makes sense.  Kukjin: do you know of someone who needs this
> >> (someone who is picking up linux-stable updates for exynos)?  I don't
> >> think it's important for ChromeOS for this particular patch.  If
> >> there's someone who needs this to officially land on linux-stable I'd
> >> be happy to review their backport of this patch.
> >>
> > As you know, developing something like Android, Tizen use the stable
> kernel (long-term? I'm not sure) and there was a problem about this issue.
> So I mean, would be fixed for the stable kernel.
> 
> Sure, but do they actually pull in from linux-stable periodically?
> I'd imagine that they have a private tree and that it would be their
> job to backport any fixes onto their kernel.

Right, the projects usually pull the linux-stable kernel when it starts. But as far as I know, they pick up some fixes from linux-stable during developing. Or for next project, would be better. I'm not sure what version will be used next time but it's obvious it will not be latest mainline :-)

Thanks,
- Kukjin

[PATCH v2 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

[Doug Anderson]

A level-triggered interrupt should be acked after the interrupt line
becomes inactive and before it is unmasked, or else another interrupt
will be immediately triggered.  Acking before or after calling the
handler is not enough.

Signed-off-by: Luigi Semenzato <semenzato@chromium.org>
Signed-off-by: Doug Anderson <dianders@chromium.org>
---
Changes in v2:
- Greatly simplified using Tomasz's suggestion of irqd_get_trigger_type
- Moved acking out of the bank spinlock since since it's not needed.
- Linus W. has already applied parts 1 and 2, so not resending.

 drivers/pinctrl/pinctrl-exynos.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c
index c0729a3..ef75321 100644
--- a/drivers/pinctrl/pinctrl-exynos.c
+++ b/drivers/pinctrl/pinctrl-exynos.c
@@ -84,6 +84,17 @@ static void exynos_gpio_irq_unmask(struct irq_data *irqd)
 	unsigned long mask;
 	unsigned long flags;
 
+	/*
+	 * Ack level interrupts right before unmask
+	 *
+	 * If we don't do this we'll get a double-interrupt.  Level triggered
+	 * interrupts must not fire an interrupt if the level is not
+	 * _currently_ active, even if it was active while the interrupt was
+	 * masked.
+	 */
+	if (irqd_get_trigger_type(irqd) & IRQ_TYPE_LEVEL_MASK)
+		exynos_gpio_irq_ack(irqd);
+
 	spin_lock_irqsave(&bank->slock, flags);
 
 	mask = readl(d->virt_base + reg_mask);
@@ -302,6 +313,17 @@ static void exynos_wkup_irq_unmask(struct irq_data *irqd)
 	unsigned long mask;
 	unsigned long flags;
 
+	/*
+	 * Ack level interrupts right before unmask
+	 *
+	 * If we don't do this we'll get a double-interrupt.  Level triggered
+	 * interrupts must not fire an interrupt if the level is not
+	 * _currently_ active, even if it was active while the interrupt was
+	 * masked.
+	 */
+	if (irqd_get_trigger_type(irqd) & IRQ_TYPE_LEVEL_MASK)
+		exynos_wkup_irq_ack(irqd);
+
 	spin_lock_irqsave(&b->slock, flags);
 
 	mask = readl(d->virt_base + reg_mask);
-- 
1.8.3

Re: [PATCH v2 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

[Tomasz Figa]

On Thursday 13 of June 2013 09:38:42 Doug Anderson wrote:
> A level-triggered interrupt should be acked after the interrupt line
> becomes inactive and before it is unmasked, or else another interrupt
> will be immediately triggered.  Acking before or after calling the
> handler is not enough.
> 
> Signed-off-by: Luigi Semenzato <semenzato@chromium.org>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Changes in v2:
> - Greatly simplified using Tomasz's suggestion of irqd_get_trigger_type
> - Moved acking out of the bank spinlock since since it's not needed.
> - Linus W. has already applied parts 1 and 2, so not resending.
> 
>  drivers/pinctrl/pinctrl-exynos.c | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)

Looks good. Thanks.

Acked-by: Tomasz Figa <t.figa@samsung.com>

Best regards,
Tomasz

> diff --git a/drivers/pinctrl/pinctrl-exynos.c
> b/drivers/pinctrl/pinctrl-exynos.c index c0729a3..ef75321 100644
> --- a/drivers/pinctrl/pinctrl-exynos.c
> +++ b/drivers/pinctrl/pinctrl-exynos.c
> @@ -84,6 +84,17 @@ static void exynos_gpio_irq_unmask(struct irq_data
> *irqd) unsigned long mask;
>  	unsigned long flags;
> 
> +	/*
> +	 * Ack level interrupts right before unmask
> +	 *
> +	 * If we don't do this we'll get a double-interrupt.  Level 
triggered
> +	 * interrupts must not fire an interrupt if the level is not
> +	 * _currently_ active, even if it was active while the interrupt 
was
> +	 * masked.
> +	 */
> +	if (irqd_get_trigger_type(irqd) & IRQ_TYPE_LEVEL_MASK)
> +		exynos_gpio_irq_ack(irqd);
> +
>  	spin_lock_irqsave(&bank->slock, flags);
> 
>  	mask = readl(d->virt_base + reg_mask);
> @@ -302,6 +313,17 @@ static void exynos_wkup_irq_unmask(struct irq_data
> *irqd) unsigned long mask;
>  	unsigned long flags;
> 
> +	/*
> +	 * Ack level interrupts right before unmask
> +	 *
> +	 * If we don't do this we'll get a double-interrupt.  Level 
triggered
> +	 * interrupts must not fire an interrupt if the level is not
> +	 * _currently_ active, even if it was active while the interrupt 
was
> +	 * masked.
> +	 */
> +	if (irqd_get_trigger_type(irqd) & IRQ_TYPE_LEVEL_MASK)
> +		exynos_wkup_irq_ack(irqd);
> +
>  	spin_lock_irqsave(&b->slock, flags);
> 
>  	mask = readl(d->virt_base + reg_mask);

Re: [PATCH v2 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

[Linus Walleij]

On Thu, Jun 13, 2013 at 6:38 PM, Doug Anderson <dianders@chromium.org> wrote:

> A level-triggered interrupt should be acked after the interrupt line
> becomes inactive and before it is unmasked, or else another interrupt
> will be immediately triggered.  Acking before or after calling the
> handler is not enough.
>
> Signed-off-by: Luigi Semenzato <semenzato@chromium.org>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
> Changes in v2:
> - Greatly simplified using Tomasz's suggestion of irqd_get_trigger_type
> - Moved acking out of the bank spinlock since since it's not needed.
> - Linus W. has already applied parts 1 and 2, so not resending.

Thanks, this v2 version applied with Tomasz ACK.

Yours,
Linus Walleij

Re: [PATCH v2 3/3] pinctrl: exynos: ack level-triggered interrupts before unmasking

[Linus Walleij]

On Thu, Jun 13, 2013 at 8:20 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Jun 13, 2013 at 6:38 PM, Doug Anderson <dianders@chromium.org> wrote:
>
>> A level-triggered interrupt should be acked after the interrupt line
>> becomes inactive and before it is unmasked, or else another interrupt
>> will be immediately triggered.  Acking before or after calling the
>> handler is not enough.
>>
>> Signed-off-by: Luigi Semenzato <semenzato@chromium.org>
>> Signed-off-by: Doug Anderson <dianders@chromium.org>
>> ---
>> Changes in v2:
>> - Greatly simplified using Tomasz's suggestion of irqd_get_trigger_type
>> - Moved acking out of the bank spinlock since since it's not needed.
>> - Linus W. has already applied parts 1 and 2, so not resending.
>
> Thanks, this v2 version applied with Tomasz ACK.

As noted this thing exploded due to patch fuzzing.

Could you respin this on top of my "devel" branch?

Yours,
Linus Walleij