diff mbox

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

Message ID 1371141522-29255-3-git-send-email-dianders@chromium.org (mailing list archive)
State New, archived
Headers show

Commit Message

Doug Anderson June 13, 2013, 4:38 p.m. UTC
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(+)

Comments

Tomasz Figa June 13, 2013, 4:44 p.m. UTC | #1
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);
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij June 13, 2013, 6:20 p.m. UTC | #2
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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij June 17, 2013, 4:56 p.m. UTC | #3
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
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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);