[PATCH 1/3] pinctrl: exynos: Add spinlocks to irq_mask and irq_unmask

[PATCH 1/3] pinctrl: exynos: Add spinlocks to irq_mask and irq_unmask

[Doug Anderson]

The patch:
  1984695 pinctrl: samsung: Protect bank registers with a spinlock

...added spinlocks to protect many accesses.  However, the irq_mask
and irq_unmask functions still do an unprotected read/modify/write.
Add the spinlock there.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/pinctrl/pinctrl-exynos.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c
index 2d76f66..c29a28e 100644
--- a/drivers/pinctrl/pinctrl-exynos.c
+++ b/drivers/pinctrl/pinctrl-exynos.c
@@ -56,10 +56,15 @@ static void exynos_gpio_irq_unmask(struct irq_data *irqd)
 	struct samsung_pinctrl_drv_data *d = bank->drvdata;
 	unsigned long reg_mask = d->ctrl->geint_mask + bank->eint_offset;
 	unsigned long mask;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->slock, flags);
 
 	mask = readl(d->virt_base + reg_mask);
 	mask &= ~(1 << irqd->hwirq);
 	writel(mask, d->virt_base + reg_mask);
+
+	spin_unlock_irqrestore(&bank->slock, flags);
 }
 
 static void exynos_gpio_irq_mask(struct irq_data *irqd)
@@ -68,10 +73,15 @@ static void exynos_gpio_irq_mask(struct irq_data *irqd)
 	struct samsung_pinctrl_drv_data *d = bank->drvdata;
 	unsigned long reg_mask = d->ctrl->geint_mask + bank->eint_offset;
 	unsigned long mask;
+	unsigned long flags;
+
+	spin_lock_irqsave(&bank->slock, flags);
 
 	mask = readl(d->virt_base + reg_mask);
 	mask |= 1 << irqd->hwirq;
 	writel(mask, d->virt_base + reg_mask);
+
+	spin_unlock_irqrestore(&bank->slock, flags);
 }
 
 static void exynos_gpio_irq_ack(struct irq_data *irqd)
@@ -264,10 +274,15 @@ static void exynos_wkup_irq_unmask(struct irq_data *irqd)
 	struct samsung_pinctrl_drv_data *d = b->drvdata;
 	unsigned long reg_mask = d->ctrl->weint_mask + b->eint_offset;
 	unsigned long mask;
+	unsigned long flags;
+
+	spin_lock_irqsave(&b->slock, flags);
 
 	mask = readl(d->virt_base + reg_mask);
 	mask &= ~(1 << irqd->hwirq);
 	writel(mask, d->virt_base + reg_mask);
+
+	spin_unlock_irqrestore(&b->slock, flags);
 }
 
 static void exynos_wkup_irq_mask(struct irq_data *irqd)
@@ -276,10 +291,15 @@ static void exynos_wkup_irq_mask(struct irq_data *irqd)
 	struct samsung_pinctrl_drv_data *d = b->drvdata;
 	unsigned long reg_mask = d->ctrl->weint_mask + b->eint_offset;
 	unsigned long mask;
+	unsigned long flags;
+
+	spin_lock_irqsave(&b->slock, flags);
 
 	mask = readl(d->virt_base + reg_mask);
 	mask |= 1 << irqd->hwirq;
 	writel(mask, d->virt_base + reg_mask);
+
+	spin_unlock_irqrestore(&b->slock, flags);
 }
 
 static void exynos_wkup_irq_ack(struct irq_data *irqd)
-- 
1.8.3

[PATCH 2/3] pinctrl: exynos: reorder xyz_irq_unmask() so future patch can ack

[Doug Anderson]

This patch does nothing but reorder the functions to improve the
readability of a future patch.

Signed-off-by: Doug Anderson <dianders@chromium.org>
---
 drivers/pinctrl/pinctrl-exynos.c | 52 ++++++++++++++++++++--------------------
 1 file changed, 26 insertions(+), 26 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c
index c29a28e..c0729a3 100644
--- a/drivers/pinctrl/pinctrl-exynos.c
+++ b/drivers/pinctrl/pinctrl-exynos.c
@@ -50,7 +50,7 @@ static const struct of_device_id exynos_wkup_irq_ids[] = {
 	{ }
 };
 
-static void exynos_gpio_irq_unmask(struct irq_data *irqd)
+static void exynos_gpio_irq_mask(struct irq_data *irqd)
 {
 	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
 	struct samsung_pinctrl_drv_data *d = bank->drvdata;
@@ -61,13 +61,22 @@ static void exynos_gpio_irq_unmask(struct irq_data *irqd)
 	spin_lock_irqsave(&bank->slock, flags);
 
 	mask = readl(d->virt_base + reg_mask);
-	mask &= ~(1 << irqd->hwirq);
+	mask |= 1 << irqd->hwirq;
 	writel(mask, d->virt_base + reg_mask);
 
 	spin_unlock_irqrestore(&bank->slock, flags);
 }
 
-static void exynos_gpio_irq_mask(struct irq_data *irqd)
+static void exynos_gpio_irq_ack(struct irq_data *irqd)
+{
+	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
+	struct samsung_pinctrl_drv_data *d = bank->drvdata;
+	unsigned long reg_pend = d->ctrl->geint_pend + bank->eint_offset;
+
+	writel(1 << irqd->hwirq, d->virt_base + reg_pend);
+}
+
+static void exynos_gpio_irq_unmask(struct irq_data *irqd)
 {
 	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
 	struct samsung_pinctrl_drv_data *d = bank->drvdata;
@@ -78,21 +87,12 @@ static void exynos_gpio_irq_mask(struct irq_data *irqd)
 	spin_lock_irqsave(&bank->slock, flags);
 
 	mask = readl(d->virt_base + reg_mask);
-	mask |= 1 << irqd->hwirq;
+	mask &= ~(1 << irqd->hwirq);
 	writel(mask, d->virt_base + reg_mask);
 
 	spin_unlock_irqrestore(&bank->slock, flags);
 }
 
-static void exynos_gpio_irq_ack(struct irq_data *irqd)
-{
-	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
-	struct samsung_pinctrl_drv_data *d = bank->drvdata;
-	unsigned long reg_pend = d->ctrl->geint_pend + bank->eint_offset;
-
-	writel(1 << irqd->hwirq, d->virt_base + reg_pend);
-}
-
 static int exynos_gpio_irq_set_type(struct irq_data *irqd, unsigned int type)
 {
 	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
@@ -268,7 +268,7 @@ err_domains:
 	return ret;
 }
 
-static void exynos_wkup_irq_unmask(struct irq_data *irqd)
+static void exynos_wkup_irq_mask(struct irq_data *irqd)
 {
 	struct samsung_pin_bank *b = irq_data_get_irq_chip_data(irqd);
 	struct samsung_pinctrl_drv_data *d = b->drvdata;
@@ -279,13 +279,22 @@ static void exynos_wkup_irq_unmask(struct irq_data *irqd)
 	spin_lock_irqsave(&b->slock, flags);
 
 	mask = readl(d->virt_base + reg_mask);
-	mask &= ~(1 << irqd->hwirq);
+	mask |= 1 << irqd->hwirq;
 	writel(mask, d->virt_base + reg_mask);
 
 	spin_unlock_irqrestore(&b->slock, flags);
 }
 
-static void exynos_wkup_irq_mask(struct irq_data *irqd)
+static void exynos_wkup_irq_ack(struct irq_data *irqd)
+{
+	struct samsung_pin_bank *b = irq_data_get_irq_chip_data(irqd);
+	struct samsung_pinctrl_drv_data *d = b->drvdata;
+	unsigned long pend = d->ctrl->weint_pend + b->eint_offset;
+
+	writel(1 << irqd->hwirq, d->virt_base + pend);
+}
+
+static void exynos_wkup_irq_unmask(struct irq_data *irqd)
 {
 	struct samsung_pin_bank *b = irq_data_get_irq_chip_data(irqd);
 	struct samsung_pinctrl_drv_data *d = b->drvdata;
@@ -296,21 +305,12 @@ static void exynos_wkup_irq_mask(struct irq_data *irqd)
 	spin_lock_irqsave(&b->slock, flags);
 
 	mask = readl(d->virt_base + reg_mask);
-	mask |= 1 << irqd->hwirq;
+	mask &= ~(1 << irqd->hwirq);
 	writel(mask, d->virt_base + reg_mask);
 
 	spin_unlock_irqrestore(&b->slock, flags);
 }
 
-static void exynos_wkup_irq_ack(struct irq_data *irqd)
-{
-	struct samsung_pin_bank *b = irq_data_get_irq_chip_data(irqd);
-	struct samsung_pinctrl_drv_data *d = b->drvdata;
-	unsigned long pend = d->ctrl->weint_pend + b->eint_offset;
-
-	writel(1 << irqd->hwirq, d->virt_base + pend);
-}
-
 static int exynos_wkup_irq_set_type(struct irq_data *irqd, unsigned int type)
 {
 	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
-- 
1.8.3

Re: [PATCH 2/3] pinctrl: exynos: reorder xyz_irq_unmask() so future patch can ack

[Tomasz Figa]

On Wednesday 12 of June 2013 10:33:18 Doug Anderson wrote:
> This patch does nothing but reorder the functions to improve the
> readability of a future patch.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/pinctrl/pinctrl-exynos.c | 52
> ++++++++++++++++++++-------------------- 1 file changed, 26
> insertions(+), 26 deletions(-)

IMHO just moving _ack() above _mask() (or _unmask()) would be enough, 
without touching _mask() and _unmask() in this patch, but have my

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

Best regards,
Tomasz

P.S. I think you might have missed linux-arm-kernel and linux-samsung-soc 
in recipients list of this series.

> diff --git a/drivers/pinctrl/pinctrl-exynos.c
> b/drivers/pinctrl/pinctrl-exynos.c index c29a28e..c0729a3 100644
> --- a/drivers/pinctrl/pinctrl-exynos.c
> +++ b/drivers/pinctrl/pinctrl-exynos.c
> @@ -50,7 +50,7 @@ static const struct of_device_id exynos_wkup_irq_ids[]
> = { { }
>  };
> 
> -static void exynos_gpio_irq_unmask(struct irq_data *irqd)
> +static void exynos_gpio_irq_mask(struct irq_data *irqd)
>  {
>  	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
>  	struct samsung_pinctrl_drv_data *d = bank->drvdata;
> @@ -61,13 +61,22 @@ static void exynos_gpio_irq_unmask(struct irq_data
> *irqd) spin_lock_irqsave(&bank->slock, flags);
> 
>  	mask = readl(d->virt_base + reg_mask);
> -	mask &= ~(1 << irqd->hwirq);
> +	mask |= 1 << irqd->hwirq;
>  	writel(mask, d->virt_base + reg_mask);
> 
>  	spin_unlock_irqrestore(&bank->slock, flags);
>  }
> 
> -static void exynos_gpio_irq_mask(struct irq_data *irqd)
> +static void exynos_gpio_irq_ack(struct irq_data *irqd)
> +{
> +	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
> +	struct samsung_pinctrl_drv_data *d = bank->drvdata;
> +	unsigned long reg_pend = d->ctrl->geint_pend + bank->eint_offset;
> +
> +	writel(1 << irqd->hwirq, d->virt_base + reg_pend);
> +}
> +
> +static void exynos_gpio_irq_unmask(struct irq_data *irqd)
>  {
>  	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
>  	struct samsung_pinctrl_drv_data *d = bank->drvdata;
> @@ -78,21 +87,12 @@ static void exynos_gpio_irq_mask(struct irq_data
> *irqd) spin_lock_irqsave(&bank->slock, flags);
> 
>  	mask = readl(d->virt_base + reg_mask);
> -	mask |= 1 << irqd->hwirq;
> +	mask &= ~(1 << irqd->hwirq);
>  	writel(mask, d->virt_base + reg_mask);
> 
>  	spin_unlock_irqrestore(&bank->slock, flags);
>  }
> 
> -static void exynos_gpio_irq_ack(struct irq_data *irqd)
> -{
> -	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
> -	struct samsung_pinctrl_drv_data *d = bank->drvdata;
> -	unsigned long reg_pend = d->ctrl->geint_pend + bank->eint_offset;
> -
> -	writel(1 << irqd->hwirq, d->virt_base + reg_pend);
> -}
> -
>  static int exynos_gpio_irq_set_type(struct irq_data *irqd, unsigned int
> type) {
>  	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
> @@ -268,7 +268,7 @@ err_domains:
>  	return ret;
>  }
> 
> -static void exynos_wkup_irq_unmask(struct irq_data *irqd)
> +static void exynos_wkup_irq_mask(struct irq_data *irqd)
>  {
>  	struct samsung_pin_bank *b = irq_data_get_irq_chip_data(irqd);
>  	struct samsung_pinctrl_drv_data *d = b->drvdata;
> @@ -279,13 +279,22 @@ static void exynos_wkup_irq_unmask(struct irq_data
> *irqd) spin_lock_irqsave(&b->slock, flags);
> 
>  	mask = readl(d->virt_base + reg_mask);
> -	mask &= ~(1 << irqd->hwirq);
> +	mask |= 1 << irqd->hwirq;
>  	writel(mask, d->virt_base + reg_mask);
> 
>  	spin_unlock_irqrestore(&b->slock, flags);
>  }
> 
> -static void exynos_wkup_irq_mask(struct irq_data *irqd)
> +static void exynos_wkup_irq_ack(struct irq_data *irqd)
> +{
> +	struct samsung_pin_bank *b = irq_data_get_irq_chip_data(irqd);
> +	struct samsung_pinctrl_drv_data *d = b->drvdata;
> +	unsigned long pend = d->ctrl->weint_pend + b->eint_offset;
> +
> +	writel(1 << irqd->hwirq, d->virt_base + pend);
> +}
> +
> +static void exynos_wkup_irq_unmask(struct irq_data *irqd)
>  {
>  	struct samsung_pin_bank *b = irq_data_get_irq_chip_data(irqd);
>  	struct samsung_pinctrl_drv_data *d = b->drvdata;
> @@ -296,21 +305,12 @@ static void exynos_wkup_irq_mask(struct irq_data
> *irqd) spin_lock_irqsave(&b->slock, flags);
> 
>  	mask = readl(d->virt_base + reg_mask);
> -	mask |= 1 << irqd->hwirq;
> +	mask &= ~(1 << irqd->hwirq);
>  	writel(mask, d->virt_base + reg_mask);
> 
>  	spin_unlock_irqrestore(&b->slock, flags);
>  }
> 
> -static void exynos_wkup_irq_ack(struct irq_data *irqd)
> -{
> -	struct samsung_pin_bank *b = irq_data_get_irq_chip_data(irqd);
> -	struct samsung_pinctrl_drv_data *d = b->drvdata;
> -	unsigned long pend = d->ctrl->weint_pend + b->eint_offset;
> -
> -	writel(1 << irqd->hwirq, d->virt_base + pend);
> -}
> -
>  static int exynos_wkup_irq_set_type(struct irq_data *irqd, unsigned int
> type) {
>  	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);

RE: [PATCH 2/3] pinctrl: exynos: reorder xyz_irq_unmask() so future patch can ack

[Kukjin Kim]

Doug Anderson wrote:
> 
> This patch does nothing but reorder the functions to improve the
> readability of a future patch.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>

Acked-by: Kukjin Kim <kgene.kim@samsung.com>

Thanks,
- Kukjin

Re: [PATCH 2/3] pinctrl: exynos: reorder xyz_irq_unmask() so future patch can ack

[Linus Walleij]

On Wed, Jun 12, 2013 at 7:33 PM, Doug Anderson <dianders@chromium.org> wrote:

> This patch does nothing but reorder the functions to improve the
> readability of a future patch.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>

Patch applied with Tomasz and Kukjin's ACKs,
I had to rebase it though, so please check the result
on my "devel" branch when I push it out shortly
(will be sent to linux-next as well).

Yours,
Linus Walleij

Re: [PATCH 2/3] pinctrl: exynos: reorder xyz_irq_unmask() so future patch can ack

[Doug Anderson]

Linus,

On Thu, Jun 13, 2013 at 5:32 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> Patch applied with Tomasz and Kukjin's ACKs,
> I had to rebase it though, so please check the result
> on my "devel" branch when I push it out shortly
> (will be sent to linux-next as well).

It looks like you haven't pushed yet so I went ahead and sent a respin
of part 3 against the same base as before.  Hopefully it should apply
cleanly or at least be simple to rebase.

I'm happy to check the patches once I notice that they're live.

Thank you for applying!

-Doug

[PATCH 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>
---
 drivers/pinctrl/pinctrl-exynos.c | 42 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/pinctrl/pinctrl-exynos.c b/drivers/pinctrl/pinctrl-exynos.c
index c0729a3..67b7a27 100644
--- a/drivers/pinctrl/pinctrl-exynos.c
+++ b/drivers/pinctrl/pinctrl-exynos.c
@@ -81,11 +81,32 @@ static void exynos_gpio_irq_unmask(struct irq_data *irqd)
 	struct samsung_pin_bank *bank = irq_data_get_irq_chip_data(irqd);
 	struct samsung_pinctrl_drv_data *d = bank->drvdata;
 	unsigned long reg_mask = d->ctrl->geint_mask + bank->eint_offset;
+	unsigned long reg_con = d->ctrl->geint_con + bank->eint_offset;
+	unsigned int pin = irqd->hwirq;
+	unsigned int shift = EXYNOS_EINT_CON_LEN * pin;
+	unsigned int con, trig_type;
 	unsigned long mask;
 	unsigned long flags;
 
 	spin_lock_irqsave(&bank->slock, 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.
+	 */
+	con = readl(d->virt_base + reg_con);
+	trig_type = (con >> shift) & EXYNOS_EINT_CON_MASK;
+	switch (trig_type) {
+	case EXYNOS_EINT_LEVEL_HIGH:
+	case EXYNOS_EINT_LEVEL_LOW:
+		exynos_gpio_irq_ack(irqd);
+		break;
+	}
+
 	mask = readl(d->virt_base + reg_mask);
 	mask &= ~(1 << irqd->hwirq);
 	writel(mask, d->virt_base + reg_mask);
@@ -299,11 +320,32 @@ static void exynos_wkup_irq_unmask(struct irq_data *irqd)
 	struct samsung_pin_bank *b = irq_data_get_irq_chip_data(irqd);
 	struct samsung_pinctrl_drv_data *d = b->drvdata;
 	unsigned long reg_mask = d->ctrl->weint_mask + b->eint_offset;
+	unsigned long reg_con = d->ctrl->weint_con + b->eint_offset;
+	unsigned int pin = irqd->hwirq;
+	unsigned long shift = EXYNOS_EINT_CON_LEN * pin;
+	unsigned long con, trig_type;
 	unsigned long mask;
 	unsigned long flags;
 
 	spin_lock_irqsave(&b->slock, 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.
+	 */
+	con = readl(d->virt_base + reg_con);
+	trig_type = (con >> shift) & EXYNOS_EINT_CON_MASK;
+	switch (trig_type) {
+	case EXYNOS_EINT_LEVEL_HIGH:
+	case EXYNOS_EINT_LEVEL_LOW:
+		exynos_wkup_irq_ack(irqd);
+		break;
+	}
+
 	mask = readl(d->virt_base + reg_mask);
 	mask &= ~(1 << irqd->hwirq);
 	writel(mask, d->virt_base + reg_mask);
-- 
1.8.3

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

[Tomasz Figa]

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?

Also one comment inline.

> Signed-off-by: Luigi Semenzato <semenzato@chromium.org>
> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/pinctrl/pinctrl-exynos.c | 42
> ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42
> insertions(+)
> 
> diff --git a/drivers/pinctrl/pinctrl-exynos.c
> b/drivers/pinctrl/pinctrl-exynos.c index c0729a3..67b7a27 100644
> --- a/drivers/pinctrl/pinctrl-exynos.c
> +++ b/drivers/pinctrl/pinctrl-exynos.c
> @@ -81,11 +81,32 @@ static void exynos_gpio_irq_unmask(struct irq_data
> *irqd) struct samsung_pin_bank *bank =
> irq_data_get_irq_chip_data(irqd); struct samsung_pinctrl_drv_data *d =
> bank->drvdata;
>  	unsigned long reg_mask = d->ctrl->geint_mask + bank->eint_offset;
> +	unsigned long reg_con = d->ctrl->geint_con + bank->eint_offset;
> +	unsigned int pin = irqd->hwirq;
> +	unsigned int shift = EXYNOS_EINT_CON_LEN * pin;
> +	unsigned int con, trig_type;
>  	unsigned long mask;
>  	unsigned long flags;
> 
>  	spin_lock_irqsave(&bank->slock, 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.
> +	 */
> +	con = readl(d->virt_base + reg_con);
> +	trig_type = (con >> shift) & EXYNOS_EINT_CON_MASK;
> +	switch (trig_type) {
> +	case EXYNOS_EINT_LEVEL_HIGH:
> +	case EXYNOS_EINT_LEVEL_LOW:
> +		exynos_gpio_irq_ack(irqd);
> +		break;
> +	}

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

Best regards,
Tomasz

> +
>  	mask = readl(d->virt_base + reg_mask);
>  	mask &= ~(1 << irqd->hwirq);
>  	writel(mask, d->virt_base + reg_mask);
> @@ -299,11 +320,32 @@ static void exynos_wkup_irq_unmask(struct irq_data
> *irqd) struct samsung_pin_bank *b = irq_data_get_irq_chip_data(irqd);
> struct samsung_pinctrl_drv_data *d = b->drvdata;
>  	unsigned long reg_mask = d->ctrl->weint_mask + b->eint_offset;
> +	unsigned long reg_con = d->ctrl->weint_con + b->eint_offset;
> +	unsigned int pin = irqd->hwirq;
> +	unsigned long shift = EXYNOS_EINT_CON_LEN * pin;
> +	unsigned long con, trig_type;
>  	unsigned long mask;
>  	unsigned long flags;
> 
>  	spin_lock_irqsave(&b->slock, 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.
> +	 */
> +	con = readl(d->virt_base + reg_con);
> +	trig_type = (con >> shift) & EXYNOS_EINT_CON_MASK;
> +	switch (trig_type) {
> +	case EXYNOS_EINT_LEVEL_HIGH:
> +	case EXYNOS_EINT_LEVEL_LOW:
> +		exynos_wkup_irq_ack(irqd);
> +		break;
> +	}
> +
>  	mask = readl(d->virt_base + reg_mask);
>  	mask &= ~(1 << irqd->hwirq);
>  	writel(mask, d->virt_base + reg_mask);

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

[Kukjin Kim]

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...

Anyway,
Acked-by: Kukjin Kim <kgene.kim@samsung.com>

Thanks,
- Kukjin

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

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

[Linus Walleij]

On Wed, Jun 12, 2013 at 7:33 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>

I'm holding this off until Tomasz' comments are addressed.

Yours,
Linus Walleij

Re: [PATCH 1/3] pinctrl: exynos: Add spinlocks to irq_mask and irq_unmask

[Tomasz Figa]

Hi Doug,

On Wednesday 12 of June 2013 10:33:17 Doug Anderson wrote:
> The patch:
>   1984695 pinctrl: samsung: Protect bank registers with a spinlock
> 
> ...added spinlocks to protect many accesses.  However, the irq_mask
> and irq_unmask functions still do an unprotected read/modify/write.
> Add the spinlock there.

Right, that's correct.

I always thought that the IRQ core already does some irqchip level 
locking, but it seems like I got confused by irq_desc spinlock. Thanks for 
spotting this.

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

Best regards,
Tomasz

> Signed-off-by: Doug Anderson <dianders@chromium.org>
> ---
>  drivers/pinctrl/pinctrl-exynos.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/pinctrl/pinctrl-exynos.c
> b/drivers/pinctrl/pinctrl-exynos.c index 2d76f66..c29a28e 100644
> --- a/drivers/pinctrl/pinctrl-exynos.c
> +++ b/drivers/pinctrl/pinctrl-exynos.c
> @@ -56,10 +56,15 @@ static void exynos_gpio_irq_unmask(struct irq_data
> *irqd) struct samsung_pinctrl_drv_data *d = bank->drvdata;
>  	unsigned long reg_mask = d->ctrl->geint_mask + bank->eint_offset;
>  	unsigned long mask;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bank->slock, flags);
> 
>  	mask = readl(d->virt_base + reg_mask);
>  	mask &= ~(1 << irqd->hwirq);
>  	writel(mask, d->virt_base + reg_mask);
> +
> +	spin_unlock_irqrestore(&bank->slock, flags);
>  }
> 
>  static void exynos_gpio_irq_mask(struct irq_data *irqd)
> @@ -68,10 +73,15 @@ static void exynos_gpio_irq_mask(struct irq_data
> *irqd) struct samsung_pinctrl_drv_data *d = bank->drvdata;
>  	unsigned long reg_mask = d->ctrl->geint_mask + bank->eint_offset;
>  	unsigned long mask;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&bank->slock, flags);
> 
>  	mask = readl(d->virt_base + reg_mask);
>  	mask |= 1 << irqd->hwirq;
>  	writel(mask, d->virt_base + reg_mask);
> +
> +	spin_unlock_irqrestore(&bank->slock, flags);
>  }
> 
>  static void exynos_gpio_irq_ack(struct irq_data *irqd)
> @@ -264,10 +274,15 @@ static void exynos_wkup_irq_unmask(struct irq_data
> *irqd) struct samsung_pinctrl_drv_data *d = b->drvdata;
>  	unsigned long reg_mask = d->ctrl->weint_mask + b->eint_offset;
>  	unsigned long mask;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&b->slock, flags);
> 
>  	mask = readl(d->virt_base + reg_mask);
>  	mask &= ~(1 << irqd->hwirq);
>  	writel(mask, d->virt_base + reg_mask);
> +
> +	spin_unlock_irqrestore(&b->slock, flags);
>  }
> 
>  static void exynos_wkup_irq_mask(struct irq_data *irqd)
> @@ -276,10 +291,15 @@ static void exynos_wkup_irq_mask(struct irq_data
> *irqd) struct samsung_pinctrl_drv_data *d = b->drvdata;
>  	unsigned long reg_mask = d->ctrl->weint_mask + b->eint_offset;
>  	unsigned long mask;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&b->slock, flags);
> 
>  	mask = readl(d->virt_base + reg_mask);
>  	mask |= 1 << irqd->hwirq;
>  	writel(mask, d->virt_base + reg_mask);
> +
> +	spin_unlock_irqrestore(&b->slock, flags);
>  }
> 
>  static void exynos_wkup_irq_ack(struct irq_data *irqd)

RE: [PATCH 1/3] pinctrl: exynos: Add spinlocks to irq_mask and irq_unmask

[Kukjin Kim]

Doug Anderson wrote:
> 
> The patch:
>   1984695 pinctrl: samsung: Protect bank registers with a spinlock
> 
> ...added spinlocks to protect many accesses.  However, the irq_mask
> and irq_unmask functions still do an unprotected read/modify/write.
> Add the spinlock there.
> 
> Signed-off-by: Doug Anderson <dianders@chromium.org>

Thanks :-)

Acked-by: Kukjin Kim <kgene.kim@samsung.com>

- Kukjin

Re: [PATCH 1/3] pinctrl: exynos: Add spinlocks to irq_mask and irq_unmask

[Linus Walleij]

On Wed, Jun 12, 2013 at 7:33 PM, Doug Anderson <dianders@chromium.org> wrote:

> The patch:
>   1984695 pinctrl: samsung: Protect bank registers with a spinlock
>
> ...added spinlocks to protect many accesses.  However, the irq_mask
> and irq_unmask functions still do an unprotected read/modify/write.
> Add the spinlock there.
>
> Signed-off-by: Doug Anderson <dianders@chromium.org>

Patch applied with Tomasz and Kukjin's ACKs.

Thanks!
Linus Walleij

[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