LinuxLists.cc - [PATCH 1/3] pinctrl: exynos: Add spinlocks to irq_mask and irq

[permalink] [raw]

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

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 <[email protected]>
Signed-off-by: Doug Anderson <[email protected]>
---
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

2013-06-12 17:33:33

[permalink] [raw]

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

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

Signed-off-by: Doug Anderson <[email protected]>
---
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

2013-06-13 10:54:31

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Doug Anderson <[email protected]>
> ---
> 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);

2013-06-13 11:03:16

[permalink] [raw]

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

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 <[email protected]>

Best regards,
Tomasz

> Signed-off-by: Doug Anderson <[email protected]>
> ---
> 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)

2013-06-13 11:14:31

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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 <[email protected]>

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

2013-06-13 12:00:43

[permalink] [raw]

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

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 <[email protected]>

Thanks :-)

Acked-by: Kukjin Kim <[email protected]>

- Kukjin

2013-06-13 12:01:33

[permalink] [raw]

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

Doug Anderson wrote:
>
> This patch does nothing but reorder the functions to improve the
> readability of a future patch.
>
> Signed-off-by: Doug Anderson <[email protected]>

Acked-by: Kukjin Kim <[email protected]>

Thanks,
- Kukjin

2013-06-13 12:04:12

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Doug Anderson <[email protected]>

BTW, probably we need a similar fixing in the mach-exynos/common.c file
before pinct?? for distro...

Anyway,
Acked-by: Kukjin Kim <[email protected]>

Thanks,
- Kukjin

2013-06-13 12:29:47

[permalink] [raw]

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

On Wed, Jun 12, 2013 at 7:33 PM, Doug Anderson <[email protected]> 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 <[email protected]>

Patch applied with Tomasz and Kukjin's ACKs.

Thanks!
Linus Walleij

2013-06-13 12:32:43

[permalink] [raw]

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

On Wed, Jun 12, 2013 at 7:33 PM, Doug Anderson <[email protected]> wrote:

> This patch does nothing but reorder the functions to improve the
> readability of a future patch.
>
> Signed-off-by: Doug Anderson <[email protected]>

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

2013-06-13 12:34:47

[permalink] [raw]

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

On Wed, Jun 12, 2013 at 7:33 PM, Doug Anderson <[email protected]> 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 <[email protected]>
> Signed-off-by: Doug Anderson <[email protected]>

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

Yours,
Linus Walleij

2013-06-13 16:34:47

[permalink] [raw]

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

Tomasz,

On Thu, Jun 13, 2013 at 3:54 AM, Tomasz Figa <[email protected]> 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

2013-06-13 16:38:38

[permalink] [raw]

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

Kukjin,

On Thu, Jun 13, 2013 at 5:04 AM, Kukjin Kim <[email protected]> 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 <[email protected]>
>> Signed-off-by: Doug Anderson <[email protected]>
>
> 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

2013-06-13 16:40:30

[permalink] [raw]

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

On Thursday 13 of June 2013 09:34:43 Doug Anderson wrote:
> Tomasz,
>
> On Thu, Jun 13, 2013 at 3:54 AM, Tomasz Figa <[email protected]>
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

2013-06-13 16:42:21

[permalink] [raw]

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

On Thursday 13 of June 2013 09:38:33 Doug Anderson wrote:
> Kukjin,
>
> On Thu, Jun 13, 2013 at 5:04 AM, Kukjin Kim <[email protected]>
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 <[email protected]>
> >> Signed-off-by: Doug Anderson <[email protected]>
> >
> > 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

2013-06-13 16:43:49

[permalink] [raw]

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

Linus,

On Thu, Jun 13, 2013 at 5:32 AM, Linus Walleij <[email protected]> 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

2013-06-13 16:44:16

[permalink] [raw]

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

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 <[email protected]>
> Signed-off-by: Doug Anderson <[email protected]>
> ---
> 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 <[email protected]>

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

2013-06-13 16:48:18

[permalink] [raw]

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

2013-06-13 16:50:39

[permalink] [raw]

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

Tomasz,

On Thu, Jun 13, 2013 at 9:42 AM, Tomasz Figa <[email protected]> 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

2013-06-13 18:20:09

[permalink] [raw]

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

On Thu, Jun 13, 2013 at 6:38 PM, Doug Anderson <[email protected]> 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 <[email protected]>
> Signed-off-by: Doug Anderson <[email protected]>
> ---
> 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

2013-06-13 23:13:11

[permalink] [raw]

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

Doug Anderson wrote:
>
> Tomasz,
>
> On Thu, Jun 13, 2013 at 9:42 AM, Tomasz Figa <[email protected]> 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

2013-06-14 00:00:07

[permalink] [raw]

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

Kukjin

<take 2, not in HTML mode>

On Thu, Jun 13, 2013 at 4:13 PM, Kukjin Kim <[email protected]> wrote:
> Doug Anderson wrote:
>>
>> Tomasz,
>>
>> On Thu, Jun 13, 2013 at 9:42 AM, Tomasz Figa <[email protected]> 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.

2013-06-14 00:18:23

[permalink] [raw]

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

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

> On Thu, Jun 13, 2013 at 4:13 PM, Kukjin Kim <[email protected]> wrote:
> > Doug Anderson wrote:
> >>
> >> Tomasz,
> >>
> >> On Thu, Jun 13, 2013 at 9:42 AM, Tomasz Figa <[email protected]>
> 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

2013-06-17 16:56:39