[PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Thomas Petazzoni]

Thomas, Jason, Marc, Rob,

In commit d17cab4451df ("irqchip: Kill off set_irq_flags usage"), Rob
Herring modified the irqchip drivers to not use the ARM-specific
set_irq_flags() and instead rely on various functions provided by the
core irq subsystem.

While his commit was supposed to have no functional effect, it in fact
does have one effect: the IRQ_NOAUTOEN flag used to be *cleared* for
all interrupts and it is now *set* by default.

Thanks to this flag being *cleared* by default, the irq-armada-370-xp
was able to properly re-enable per-CPU interrupts at resume time. Now
that this flag is *set*, the irqd_irq_disabled() function no longer
indicates that such per-CPU interrupts are enabled (and in fact a
CPU-global flag to tell whether a per-CPU is enabled or not is
silly). Due to this, our local timer per-CPU interrupt is no longer
re-enabled at resume time on Armada XP, on the boot CPU, which causes
a hang at resume time.

This is a regression between 4.2 (where suspend/resume works fine) and
4.3-rc (where suspend/resume is broken). Reverting d17cab4451df1 on
top of 4.3-rc makes the problem go away (of course you also need to
revert eb811129ed9ea so that set_irq_flags is re-introduced).

The minimal fix would be to clear the IRQ_NOAUTOEN flag so that we get
back to the original situation. However, this does not really seem
like the right fix.

Instead, this patch series proposes to add an is_enabled_percpu_irq()
function to the core irq subsystem, which is then used by the
irq-armada-370-xp to find out if such or such per-CPU interrupt should
be re-enabled at resume time on the boot CPU.

The organization of the patch series is as follows:

 - PATCH 1 introduces the is_enabled_percpu_irq() function.

 - PATCH 2 does a minor refactoring of armada_xp_mpic_secondary_init()
   to prepare the following patch.

 - PATCH 3 changes the irq-armada-370-xp driver to use the
   is_enabled_percpu_irq() to re-enable the per-CPU interrupts on the
   boot CPU at resume time, and also modifies the secondary CPU
   notifier to re-enable per-CPU interrupts if needed.

 - PATCH 4 and 5 are further cleanups/improvements to the
   irq-armada-370-xp, which are not needed to fix the problem.

Since this is fixing a regression introduced between 4.2 and 4.3-rc,
it would be great if patches 1 to 3 could be merged in 4.3. The last
two patches are only cosmetic, so merging them for 4.4 is of course
the way to go.

Thanks,

Thomas

Thomas Petazzoni (5):
  kernel: irq: implement is_enabled_percpu_irq()
  irqchip: armada-370-xp: prepare additions to
    armada_xp_mpic_secondary_init()
  irqchip: armada-370-xp: re-enable per-CPU interrupts at resume time
  irqchip: armada-370-xp: re-order register definitions
  irqchip: armada-370-xp: document the overall driver logic

 drivers/irqchip/irq-armada-370-xp.c | 151 +++++++++++++++++++++++++++++++-----
 include/linux/interrupt.h           |   1 +
 kernel/irq/chip.c                   |   5 ++
 kernel/irq/internals.h              |   1 +
 kernel/irq/manage.c                 |  19 +++++
 5 files changed, 158 insertions(+), 19 deletions(-)

-- 
2.6.2

[PATCH 1/5] kernel: irq: implement is_enabled_percpu_irq()

[Thomas Petazzoni]

Certain interrupt controller drivers have a register set that does not
make it easy to save/restore the mask of enabled/disabled interrupts
at suspend/resume time. At resume time, such drivers rely on the core
kernel irq subsystem to tell whether such or such interrupt is enabled
or not, in order to restore the proper state in the interrupt
controller register.

While the irqd_irq_disabled() provides the relevant information for
global interrupts, there is no similar function to query the
enabled/disabled state of a per-CPU interrupt.

Therefore, this commit complements the
enable_percpu_irq/disable_percpu_irq API with an
is_enabled_percpu_irq() function.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 include/linux/interrupt.h |  1 +
 kernel/irq/chip.c         |  5 +++++
 kernel/irq/internals.h    |  1 +
 kernel/irq/manage.c       | 19 +++++++++++++++++++
 4 files changed, 26 insertions(+)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index be7e75c..5ea9be5 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -193,6 +193,7 @@ extern void disable_irq(unsigned int irq);
 extern void disable_percpu_irq(unsigned int irq);
 extern void enable_irq(unsigned int irq);
 extern void enable_percpu_irq(unsigned int irq, unsigned int type);
+extern bool is_enabled_percpu_irq(unsigned int irq);
 extern void irq_wake_thread(unsigned int irq, void *dev_id);
 
 /* The following three functions are for the core kernel use only. */
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index e28169d..ebc00fc 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -246,6 +246,11 @@ void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu)
 	cpumask_set_cpu(cpu, desc->percpu_enabled);
 }
 
+bool irq_percpu_is_enabled(struct irq_desc *desc, unsigned int cpu)
+{
+	return cpumask_test_cpu(cpu, desc->percpu_enabled);
+}
+
 void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu)
 {
 	if (desc->irq_data.chip->irq_disable)
diff --git a/kernel/irq/internals.h b/kernel/irq/internals.h
index 5ef0c2d..37de1c5 100644
--- a/kernel/irq/internals.h
+++ b/kernel/irq/internals.h
@@ -68,6 +68,7 @@ extern void irq_shutdown(struct irq_desc *desc);
 extern void irq_enable(struct irq_desc *desc);
 extern void irq_disable(struct irq_desc *desc);
 extern void irq_percpu_enable(struct irq_desc *desc, unsigned int cpu);
+extern bool irq_percpu_is_enabled(struct irq_desc *desc, unsigned int cpu);
 extern void irq_percpu_disable(struct irq_desc *desc, unsigned int cpu);
 extern void mask_irq(struct irq_desc *desc);
 extern void unmask_irq(struct irq_desc *desc);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f9a59f6..d4afe65 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1666,6 +1666,25 @@ out:
 }
 EXPORT_SYMBOL_GPL(enable_percpu_irq);
 
+bool is_enabled_percpu_irq(unsigned int irq)
+{
+	unsigned int cpu = smp_processor_id();
+	unsigned long flags;
+	struct irq_desc *desc =
+		irq_get_desc_lock(irq, &flags, IRQ_GET_DESC_CHECK_PERCPU);
+	bool is_enabled;
+
+	if (!desc)
+		return false;
+
+	is_enabled = irq_percpu_is_enabled(desc, cpu);
+
+	irq_put_desc_unlock(desc, flags);
+
+	return is_enabled;
+}
+EXPORT_SYMBOL_GPL(is_enabled_percpu_irq);
+
 void disable_percpu_irq(unsigned int irq)
 {
 	unsigned int cpu = smp_processor_id();
-- 
2.6.2

[tip:irq/core] genirq: Implement irq_percpu_is_enabled()

[tip-bot for Thomas Petazzoni]

Commit-ID:  f0cb32207307e9d7b3ee8117078b7a37f8d0166e
Gitweb:     http://git.kernel.org/tip/f0cb32207307e9d7b3ee8117078b7a37f8d0166e
Author:     Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
AuthorDate: Tue, 20 Oct 2015 15:23:51 +0200
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Tue, 8 Dec 2015 12:53:29 +0100

genirq: Implement irq_percpu_is_enabled()

Certain interrupt controller drivers have a register set that does not
make it easy to save/restore the mask of enabled/disabled interrupts
at suspend/resume time. At resume time, such drivers rely on the core
kernel irq subsystem to tell whether such or such interrupt is enabled
or not, in order to restore the proper state in the interrupt
controller register.

While the irqd_irq_disabled() provides the relevant information for
global interrupts, there is no similar function to query the
enabled/disabled state of a per-CPU interrupt.

Therefore, this commit complements the percpu_irq API with an
irq_percpu_is_enabled() function.

[ tglx: Simplified the implementation and added kerneldoc ]

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: Tawfik Bayouk <tawfik@marvell.com>
Cc: Nadav Haklai <nadavh@marvell.com>
Cc: Lior Amsalem <alior@marvell.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Marc Zyngier <marc.zyngier@arm.com>
Link: http://lkml.kernel.org/r/1445347435-2333-2-git-send-email-thomas.petazzoni@free-electrons.com
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 include/linux/interrupt.h |  1 +
 kernel/irq/manage.c       | 25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index ad16809..cb30edb 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -195,6 +195,7 @@ extern void disable_irq(unsigned int irq);
 extern void disable_percpu_irq(unsigned int irq);
 extern void enable_irq(unsigned int irq);
 extern void enable_percpu_irq(unsigned int irq, unsigned int type);
+extern bool irq_percpu_is_enabled(unsigned int irq);
 extern void irq_wake_thread(unsigned int irq, void *dev_id);
 
 /* The following three functions are for the core kernel use only. */
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 0eebaee..c84670c 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1743,6 +1743,31 @@ out:
 }
 EXPORT_SYMBOL_GPL(enable_percpu_irq);
 
+/**
+ * irq_percpu_is_enabled - Check whether the per cpu irq is enabled
+ * @irq:	Linux irq number to check for
+ *
+ * Must be called from a non migratable context. Returns the enable
+ * state of a per cpu interrupt on the current cpu.
+ */
+bool irq_percpu_is_enabled(unsigned int irq)
+{
+	unsigned int cpu = smp_processor_id();
+	struct irq_desc *desc;
+	unsigned long flags;
+	bool is_enabled;
+
+	desc = irq_get_desc_lock(irq, &flags, IRQ_GET_DESC_CHECK_PERCPU);
+	if (!desc)
+		return false;
+
+	is_enabled = cpumask_test_cpu(cpu, desc->percpu_enabled);
+	irq_put_desc_unlock(desc, flags);
+
+	return is_enabled;
+}
+EXPORT_SYMBOL_GPL(irq_percpu_is_enabled);
+
 void disable_percpu_irq(unsigned int irq)
 {
 	unsigned int cpu = smp_processor_id();

[PATCH 2/5] irqchip: armada-370-xp: prepare additions to armada_xp_mpic_secondary_init()

[Thomas Petazzoni]

As a preparation to adding more code in
armada_xp_mpic_secondary_init(), this commit slightly refactors the
current function to bail out early if we're not handling the actions
of interest.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/irqchip/irq-armada-370-xp.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 655cb96..f5afe81 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -377,10 +377,11 @@ static void armada_mpic_send_doorbell(const struct cpumask *mask,
 static int armada_xp_mpic_secondary_init(struct notifier_block *nfb,
 					 unsigned long action, void *hcpu)
 {
-	if (action == CPU_STARTING || action == CPU_STARTING_FROZEN) {
-		armada_xp_mpic_perf_init();
-		armada_xp_mpic_smp_cpu_init();
-	}
+	if (action != CPU_STARTING && action != CPU_STARTING_FROZEN)
+		return NOTIFY_OK;
+
+	armada_xp_mpic_perf_init();
+	armada_xp_mpic_smp_cpu_init();
 
 	return NOTIFY_OK;
 }
-- 
2.6.2

Re: [PATCH 2/5] irqchip: armada-370-xp: prepare additions to armada_xp_mpic_secondary_init()

[Gregory CLEMENT]

Hi Thomas,
 
 On mar., oct. 20 2015, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> As a preparation to adding more code in
> armada_xp_mpic_secondary_init(), this commit slightly refactors the
> current function to bail out early if we're not handling the actions
> of interest.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory

> ---
>  drivers/irqchip/irq-armada-370-xp.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
> index 655cb96..f5afe81 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -377,10 +377,11 @@ static void armada_mpic_send_doorbell(const struct cpumask *mask,
>  static int armada_xp_mpic_secondary_init(struct notifier_block *nfb,
>  					 unsigned long action, void *hcpu)
>  {
> -	if (action == CPU_STARTING || action == CPU_STARTING_FROZEN) {
> -		armada_xp_mpic_perf_init();
> -		armada_xp_mpic_smp_cpu_init();
> -	}
> +	if (action != CPU_STARTING && action != CPU_STARTING_FROZEN)
> +		return NOTIFY_OK;
> +
> +	armada_xp_mpic_perf_init();
> +	armada_xp_mpic_smp_cpu_init();
>  
>  	return NOTIFY_OK;
>  }
> -- 
> 2.6.2
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 3/5] irqchip: armada-370-xp: re-enable per-CPU interrupts at resume time

[Thomas Petazzoni]

Commit d17cab4451df1 ("irqchip: Kill off set_irq_flags usage") changed
the code of armada_370_xp_mpic_irq_map() from using set_irq_flags() to
irq_set_probe().

While the commit log seems to imply that there are no functional
changes, there are indeed functional changes introduced by this
commit: the IRQ_NOAUTOEN flag is no longer cleared. This functional
change causes a regression on Armada XP, which no longer works
properly after suspend/resume because per-CPU interrupts remain
disabled.

Due to how the hardware registers work, the irq-armada-370-xp cannot
simply save/restore a bunch of registers at suspend/resume to make
sure that the interrupts remain in the same state after
resuming. Therefore, it relies on the kernel to say whether the
interrupt is disabled or not, using the irqd_irq_disabled()
function. This was all working fine while the IRQ_NOAUTOEN flag was
cleared.

With the change introduced by Rob Herring in d17cab4451df1, the
IRQ_NOAUTOEN flag is now set for all interrupts. irqd_irq_disabled()
returns false for per-CPU interrupts, and therefore our per-CPU
interrupts are no longer re-enabled after resume.

This commit fixes that by using irqd_irq_disabled() only for global
interrupts, and using the newly introduced is_enabled_percpu_irq() for
per-CPU interrupts.

Also, it fixes a related problems that per-CPU interrupts were only
re-enabled on the boot CPU and not other CPUs. Until now this wasn't a
problem since on this platform, only the local timers are using
per-CPU interrupts and the local timers of secondary CPUs are turned
off/on during CPU hotplug before suspend, after after resume. However,
in Linux 4.4, we will also be using per-CPU interrupts for the network
controller, so we need to properly restore the per-CPU interrupts on
secondary CPUs as well.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/irqchip/irq-armada-370-xp.c | 45 ++++++++++++++++++++++++++++++++-----
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index f5afe81..106ac4c 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -311,7 +311,6 @@ static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
 		irq_set_percpu_devid(virq);
 		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
 					handle_percpu_devid_irq);
-
 	} else {
 		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
 					handle_level_irq);
@@ -377,12 +376,35 @@ static void armada_mpic_send_doorbell(const struct cpumask *mask,
 static int armada_xp_mpic_secondary_init(struct notifier_block *nfb,
 					 unsigned long action, void *hcpu)
 {
+	unsigned int nirqs, irq;
+
 	if (action != CPU_STARTING && action != CPU_STARTING_FROZEN)
 		return NOTIFY_OK;
 
 	armada_xp_mpic_perf_init();
 	armada_xp_mpic_smp_cpu_init();
 
+	/* Re-enable per-CPU interrupts that were enabled before suspend */
+	nirqs = (readl(main_int_base + ARMADA_370_XP_INT_CONTROL) >> 2) & 0x3ff;
+	for (irq = 0; irq < nirqs; irq++) {
+		struct irq_data *data;
+		int virq;
+
+		virq = irq_linear_revmap(armada_370_xp_mpic_domain, irq);
+		if (virq == 0)
+			continue;
+
+		data = irq_get_irq_data(virq);
+
+		if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
+			continue;
+
+		if (!is_enabled_percpu_irq(virq))
+			continue;
+
+		armada_370_xp_irq_unmask(data);
+	}
+
 	return NOTIFY_OK;
 }
 
@@ -550,16 +572,27 @@ static void armada_370_xp_mpic_resume(void)
 		if (virq == 0)
 			continue;
 
-		if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
+		data = irq_get_irq_data(virq);
+
+		if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) {
+			/* Non per-CPU interrupts */
 			writel(irq, per_cpu_int_base +
 			       ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
-		else
+			if (!irqd_irq_disabled(data))
+				armada_370_xp_irq_unmask(data);
+		} else {
+			/* Per-CPU interrupts */
 			writel(irq, main_int_base +
 			       ARMADA_370_XP_INT_SET_ENABLE_OFFS);
 
-		data = irq_get_irq_data(virq);
-		if (!irqd_irq_disabled(data))
-			armada_370_xp_irq_unmask(data);
+			/*
+			 * Re-enable on the current CPU,
+			 * armada_xp_mpic_secondary_init() will take
+			 * care of secondary CPUs when they come up.
+			 */
+			if (is_enabled_percpu_irq(virq))
+				armada_370_xp_irq_unmask(data);
+		}
 	}
 
 	/* Reconfigure doorbells for IPIs and MSIs */
-- 
2.6.2

Re: [PATCH 3/5] irqchip: armada-370-xp: re-enable per-CPU interrupts at resume time

[Gregory CLEMENT]

Hi Thomas,
 
 On mar., oct. 20 2015, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> Commit d17cab4451df1 ("irqchip: Kill off set_irq_flags usage") changed
> the code of armada_370_xp_mpic_irq_map() from using set_irq_flags() to
> irq_set_probe().
>
> While the commit log seems to imply that there are no functional
> changes, there are indeed functional changes introduced by this
> commit: the IRQ_NOAUTOEN flag is no longer cleared. This functional
> change causes a regression on Armada XP, which no longer works
> properly after suspend/resume because per-CPU interrupts remain
> disabled.
>
> Due to how the hardware registers work, the irq-armada-370-xp cannot
> simply save/restore a bunch of registers at suspend/resume to make
> sure that the interrupts remain in the same state after
> resuming. Therefore, it relies on the kernel to say whether the
> interrupt is disabled or not, using the irqd_irq_disabled()
> function. This was all working fine while the IRQ_NOAUTOEN flag was
> cleared.
>
> With the change introduced by Rob Herring in d17cab4451df1, the
> IRQ_NOAUTOEN flag is now set for all interrupts. irqd_irq_disabled()
> returns false for per-CPU interrupts, and therefore our per-CPU
> interrupts are no longer re-enabled after resume.
>
> This commit fixes that by using irqd_irq_disabled() only for global
> interrupts, and using the newly introduced is_enabled_percpu_irq() for
> per-CPU interrupts.
>
> Also, it fixes a related problems that per-CPU interrupts were only
> re-enabled on the boot CPU and not other CPUs. Until now this wasn't a
> problem since on this platform, only the local timers are using
> per-CPU interrupts and the local timers of secondary CPUs are turned
> off/on during CPU hotplug before suspend, after after resume. However,
> in Linux 4.4, we will also be using per-CPU interrupts for the network
> controller, so we need to properly restore the per-CPU interrupts on
> secondary CPUs as well.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/irqchip/irq-armada-370-xp.c | 45 ++++++++++++++++++++++++++++++++-----
>  1 file changed, 39 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
> index f5afe81..106ac4c 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -311,7 +311,6 @@ static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
>  		irq_set_percpu_devid(virq);
>  		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
>  					handle_percpu_devid_irq);
> -
>  	} else {
>  		irq_set_chip_and_handler(virq, &armada_370_xp_irq_chip,
>  					handle_level_irq);
> @@ -377,12 +376,35 @@ static void armada_mpic_send_doorbell(const struct cpumask *mask,
>  static int armada_xp_mpic_secondary_init(struct notifier_block *nfb,
>  					 unsigned long action, void *hcpu)
>  {
> +	unsigned int nirqs, irq;
> +
>  	if (action != CPU_STARTING && action != CPU_STARTING_FROZEN)
>  		return NOTIFY_OK;
>  
>  	armada_xp_mpic_perf_init();
>  	armada_xp_mpic_smp_cpu_init();
>  
> +	/* Re-enable per-CPU interrupts that were enabled before suspend */
> +	nirqs = (readl(main_int_base + ARMADA_370_XP_INT_CONTROL) >> 2) & 0x3ff;
> +	for (irq = 0; irq < nirqs; irq++) {

Actually we could reduce this loop by using
ARMADA_370_XP_MAX_PER_CPU_IRQS, as we know that we can't have more per
cpu irq. 


> +		struct irq_data *data;
> +		int virq;
> +
> +		virq = irq_linear_revmap(armada_370_xp_mpic_domain, irq);
> +		if (virq == 0)
> +			continue;
> +
> +		data = irq_get_irq_data(virq);
> +
> +		if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
> +			continue;

So eventually you only manage the timer IRQs?

If it is intentional you could it differently, but I wonder why you don't
enable again the other percpu IRQ.
> +
> +		if (!is_enabled_percpu_irq(virq))
> +			continue;
> +
> +		armada_370_xp_irq_unmask(data);
> +	}
> +
>  	return NOTIFY_OK;
>  }
>  

The following chunk will conflict with "irqchip: armada-370-xp: Rework
per-cpu interrupts handling" which is in Linux next. But as this patch
is for 4.3, you can't do anything...

> @@ -550,16 +572,27 @@ static void armada_370_xp_mpic_resume(void)
>  		if (virq == 0)
>  			continue;
>  
> -		if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
> +		data = irq_get_irq_data(virq);
> +
> +		if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) {
> +			/* Non per-CPU interrupts */
>  			writel(irq, per_cpu_int_base +
>  			       ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
> -		else
> +			if (!irqd_irq_disabled(data))
> +				armada_370_xp_irq_unmask(data);
> +		} else {
> +			/* Per-CPU interrupts */
>  			writel(irq, main_int_base +
>  			       ARMADA_370_XP_INT_SET_ENABLE_OFFS);
>  
> -		data = irq_get_irq_data(virq);
> -		if (!irqd_irq_disabled(data))
> -			armada_370_xp_irq_unmask(data);
> +			/*
> +			 * Re-enable on the current CPU,
> +			 * armada_xp_mpic_secondary_init() will take
> +			 * care of secondary CPUs when they come up.
> +			 */
> +			if (is_enabled_percpu_irq(virq))
> +				armada_370_xp_irq_unmask(data);
> +		}
>  	}
>  
>  	/* Reconfigure doorbells for IPIs and MSIs */
> -- 
> 2.6.2
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 3/5] irqchip: armada-370-xp: re-enable per-CPU interrupts at resume time

[Thomas Petazzoni]

Hello,

On Tue, 20 Oct 2015 15:46:00 +0200, Gregory CLEMENT wrote:

> > +	/* Re-enable per-CPU interrupts that were enabled before suspend */
> > +	nirqs = (readl(main_int_base + ARMADA_370_XP_INT_CONTROL) >> 2) & 0x3ff;
> > +	for (irq = 0; irq < nirqs; irq++) {
> 
> Actually we could reduce this loop by using
> ARMADA_370_XP_MAX_PER_CPU_IRQS, as we know that we can't have more per
> cpu irq. 

Indeed. I can fix that up in the next version.

> > +		struct irq_data *data;
> > +		int virq;
> > +
> > +		virq = irq_linear_revmap(armada_370_xp_mpic_domain, irq);
> > +		if (virq == 0)
> > +			continue;
> > +
> > +		data = irq_get_irq_data(virq);
> > +
> > +		if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
> > +			continue;
> 
> So eventually you only manage the timer IRQs?
> 
> If it is intentional you could it differently, but I wonder why you don't
> enable again the other percpu IRQ.

The idea is to have the same condition at the one used in the
->resume() hook.

In the end, it should be using the is_percpu_irq() function as is done
in linux-next in the ->resume() function. But since this patch is
(hopefully) aimed at 4.3, I've for now kept the same logic as the
current ->resume() function.

> The following chunk will conflict with "irqchip: armada-370-xp: Rework
> per-cpu interrupts handling" which is in Linux next. But as this patch
> is for 4.3, you can't do anything...

Indeed. I intentionally based this series on 4.3-rc, because it's
fixing a regression introduced between 4.2 and 4.3-rc, and therefore as
such should be fixed before 4.3 is released if possible (though I
understand it's already -rc6 time so maybe a bit late).

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 3/5] irqchip: armada-370-xp: re-enable per-CPU interrupts at resume time

[Marcin Wojtas]

Hi Thomas,


>
> @@ -550,16 +572,27 @@ static void armada_370_xp_mpic_resume(void)
>                 if (virq == 0)
>                         continue;
>
> -               if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
> +               data = irq_get_irq_data(virq);
> +
> +               if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) {
> +                       /* Non per-CPU interrupts */
>                         writel(irq, per_cpu_int_base +

For "Non per-CPU interrupts" per_cpu_int_base is used - is it
intentional? In armada_370_xp_irq_mask/unmask the condition looks
exactly opposite...

>                                ARMADA_370_XP_INT_CLEAR_MASK_OFFS);
> -               else
> +                       if (!irqd_irq_disabled(data))
> +                               armada_370_xp_irq_unmask(data);
> +               } else {
> +                       /* Per-CPU interrupts */
>                         writel(irq, main_int_base +
>                                ARMADA_370_XP_INT_SET_ENABLE_OFFS);
>

Best regards,
Marcin

Re: [PATCH 3/5] irqchip: armada-370-xp: re-enable per-CPU interrupts at resume time

[Thomas Petazzoni]

Marcin,

On Sun, 25 Oct 2015 22:22:37 +0100, Marcin Wojtas wrote:

> > @@ -550,16 +572,27 @@ static void armada_370_xp_mpic_resume(void)
> >                 if (virq == 0)
> >                         continue;
> >
> > -               if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
> > +               data = irq_get_irq_data(virq);
> > +
> > +               if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) {
> > +                       /* Non per-CPU interrupts */
> >                         writel(irq, per_cpu_int_base +
> 
> For "Non per-CPU interrupts" per_cpu_int_base is used - is it
> intentional? In armada_370_xp_irq_mask/unmask the condition looks
> exactly opposite...

Yes, this is normal. Carefully read PATCH 5/5, which adds a big
comment, which explains the logic of the HW and how the
irq-armada-370-xp driver copes with it.

Each interrupt can be masked at two levels. One level is enabled when
the interrupted is mapped, the other upon ->mask()/->unmask(). So
when we're resuming, we need to re-enable the interrupt at the level it
was enabled in ->map(), and have ->mask()/->unmask() continue to
mask/unmask the interrupt at the other level.

For per-CPU interrupts, ->map() and ->resume() enable the interrupt at
the global level, and leave ->mask()/->unmask() enable/disable at the
per-CPU level.

For global interrupts, ->map() and ->resume() enable the interrupt at
the per-CPU level, and leave ->mask()/->unmask() enable/disable at the
global level.

Again, see PATCH 5/5, and let me know if there are still some unclear
aspects.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 3/5] irqchip: armada-370-xp: re-enable per-CPU interrupts at resume time

[Marcin Wojtas]

Thomas,

2015-10-26 1:10 GMT+01:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Marcin,
>
> On Sun, 25 Oct 2015 22:22:37 +0100, Marcin Wojtas wrote:
>
>> > @@ -550,16 +572,27 @@ static void armada_370_xp_mpic_resume(void)
>> >                 if (virq == 0)
>> >                         continue;
>> >
>> > -               if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ)
>> > +               data = irq_get_irq_data(virq);
>> > +
>> > +               if (irq != ARMADA_370_XP_TIMER0_PER_CPU_IRQ) {
>> > +                       /* Non per-CPU interrupts */
>> >                         writel(irq, per_cpu_int_base +
>>
>> For "Non per-CPU interrupts" per_cpu_int_base is used - is it
>> intentional? In armada_370_xp_irq_mask/unmask the condition looks
>> exactly opposite...
>
> Yes, this is normal. Carefully read PATCH 5/5, which adds a big
> comment, which explains the logic of the HW and how the
> irq-armada-370-xp driver copes with it.
>
> Each interrupt can be masked at two levels. One level is enabled when
> the interrupted is mapped, the other upon ->mask()/->unmask(). So
> when we're resuming, we need to re-enable the interrupt at the level it
> was enabled in ->map(), and have ->mask()/->unmask() continue to
> mask/unmask the interrupt at the other level.
>
> For per-CPU interrupts, ->map() and ->resume() enable the interrupt at
> the global level, and leave ->mask()/->unmask() enable/disable at the
> per-CPU level.
>
> For global interrupts, ->map() and ->resume() enable the interrupt at
> the per-CPU level, and leave ->mask()/->unmask() enable/disable at the
> global level.
>
> Again, see PATCH 5/5, and let me know if there are still some unclear
> aspects.
>

Thanks for the explanation - now it's clear.

Btw, I checked the patches with mvneta in both 'standby' and 'mem'
modes on A38x (with not-yet-submitted support for PM in mvneta and
pinctrl) and everything works properly. Hence:

Tested-by: Marcin Wojtas <mw@semihalf.com>

Best regards,
Marcin

Re: [PATCH 3/5] irqchip: armada-370-xp: re-enable per-CPU interrupts at resume time

[Thomas Petazzoni]

Marcin,

On Mon, 26 Oct 2015 05:35:46 +0100, Marcin Wojtas wrote:

> Thanks for the explanation - now it's clear.

Good :-) Hopefully the explanation in PATCH 5/5 is also clear enough.

> Btw, I checked the patches with mvneta in both 'standby' and 'mem'
> modes on A38x (with not-yet-submitted support for PM in mvneta and
> pinctrl) and everything works properly. Hence:

Thanks for the testing. However, I wonder why you think those changes
are need to get mvneta to work fine with the 'standby' mode ? While I
do agree that they are need for the 'mem' mode, they shouldn't be
needed for the 'standby' mode. For now, the standby mode only puts the
CPU into deep-idle, and that's all: all devices remain powered on, and
they don't lose their state.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 3/5] irqchip: armada-370-xp: re-enable per-CPU interrupts at resume time

[Marcin Wojtas]

Thomas,

2015-10-26 6:09 GMT+01:00 Thomas Petazzoni
<thomas.petazzoni@free-electrons.com>:
> Marcin,
>
> On Mon, 26 Oct 2015 05:35:46 +0100, Marcin Wojtas wrote:
>
>> Thanks for the explanation - now it's clear.
>
> Good :-) Hopefully the explanation in PATCH 5/5 is also clear enough.

The Ascii-art is beutiful, indeed:)

>
>> Btw, I checked the patches with mvneta in both 'standby' and 'mem'
>> modes on A38x (with not-yet-submitted support for PM in mvneta and
>> pinctrl) and everything works properly. Hence:
>
> Thanks for the testing. However, I wonder why you think those changes
> are need to get mvneta to work fine with the 'standby' mode ? While I
> do agree that they are need for the 'mem' mode, they shouldn't be
> needed for the 'standby' mode. For now, the standby mode only puts the
> CPU into deep-idle, and that's all: all devices remain powered on, and
> they don't lose their state.
>

Yes, you are right - without any pm_ops the driver works well after
suspend/resume in standby. However in the linux mem and standby is
treated exactly the same as pm sleep, so the same routines are
executed in both modes. Hence the s2ram support cannot spoil standby.

Best regards,
Marcin

Re: [PATCH 3/5] irqchip: armada-370-xp: re-enable per-CPU interrupts at resume time

[Thomas Petazzoni]

Dear Marcin Wojtas,

On Mon, 26 Oct 2015 08:06:18 +0100, Marcin Wojtas wrote:

> > Good :-) Hopefully the explanation in PATCH 5/5 is also clear enough.
> 
> The Ascii-art is beutiful, indeed:)

Glad to hear that my artistic skills are appreciated :)

> Yes, you are right - without any pm_ops the driver works well after
> suspend/resume in standby. However in the linux mem and standby is
> treated exactly the same as pm sleep, so the same routines are
> executed in both modes. Hence the s2ram support cannot spoil standby.

Absolutely. It would be nicer if we knew in the ->suspend() and
->resume() hooks if we're doing a standby suspend, or a mem suspend,
but IIRC, we don't have this information available.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[PATCH 4/5] irqchip: armada-370-xp: re-order register definitions

[Thomas Petazzoni]

In order to clarify to which register base the various register
definitions apply, this commit re-orders them, and adds a comment that
clearly indicate which registers are relative to "main_int_base" and
which registers are relative to "per_cpu_int_base".

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/irqchip/irq-armada-370-xp.c | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 106ac4c..888add6 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -34,25 +34,24 @@
 #include <asm/smp_plat.h>
 #include <asm/mach/irq.h>
 
-/* Interrupt Controller Registers Map */
-#define ARMADA_370_XP_INT_SET_MASK_OFFS		(0x48)
-#define ARMADA_370_XP_INT_CLEAR_MASK_OFFS	(0x4C)
-#define ARMADA_370_XP_INT_FABRIC_MASK_OFFS	(0x54)
-#define ARMADA_370_XP_INT_CAUSE_PERF(cpu)	(1 << cpu)
-
+/* Registers relative to main_int_base */
 #define ARMADA_370_XP_INT_CONTROL		(0x00)
+#define ARMADA_370_XP_SW_TRIG_INT_OFFS		(0x04)
 #define ARMADA_370_XP_INT_SET_ENABLE_OFFS	(0x30)
 #define ARMADA_370_XP_INT_CLEAR_ENABLE_OFFS	(0x34)
 #define ARMADA_370_XP_INT_SOURCE_CTL(irq)	(0x100 + irq*4)
 #define ARMADA_370_XP_INT_SOURCE_CPU_MASK	0xF
 #define ARMADA_370_XP_INT_IRQ_FIQ_MASK(cpuid)	((BIT(0) | BIT(8)) << cpuid)
 
-#define ARMADA_370_XP_CPU_INTACK_OFFS		(0x44)
+/* Registers relative to per_cpu_int_base */
+#define ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS	(0x08)
+#define ARMADA_370_XP_IN_DRBEL_MSK_OFFS		(0x0c)
 #define ARMADA_375_PPI_CAUSE			(0x10)
-
-#define ARMADA_370_XP_SW_TRIG_INT_OFFS           (0x4)
-#define ARMADA_370_XP_IN_DRBEL_MSK_OFFS          (0xc)
-#define ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS        (0x8)
+#define ARMADA_370_XP_CPU_INTACK_OFFS		(0x44)
+#define ARMADA_370_XP_INT_SET_MASK_OFFS		(0x48)
+#define ARMADA_370_XP_INT_CLEAR_MASK_OFFS	(0x4C)
+#define ARMADA_370_XP_INT_FABRIC_MASK_OFFS	(0x54)
+#define ARMADA_370_XP_INT_CAUSE_PERF(cpu)	(1 << cpu)
 
 #define ARMADA_370_XP_MAX_PER_CPU_IRQS		(28)
 
-- 
2.6.2

Re: [PATCH 4/5] irqchip: armada-370-xp: re-order register definitions

[Gregory CLEMENT]

Hi Thomas,
 
 On mar., oct. 20 2015, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> In order to clarify to which register base the various register
> definitions apply, this commit re-orders them, and adds a comment that
> clearly indicate which registers are relative to "main_int_base" and
> which registers are relative to "per_cpu_int_base".
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory

> ---
>  drivers/irqchip/irq-armada-370-xp.c | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
> index 106ac4c..888add6 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -34,25 +34,24 @@
>  #include <asm/smp_plat.h>
>  #include <asm/mach/irq.h>
>  
> -/* Interrupt Controller Registers Map */
> -#define ARMADA_370_XP_INT_SET_MASK_OFFS		(0x48)
> -#define ARMADA_370_XP_INT_CLEAR_MASK_OFFS	(0x4C)
> -#define ARMADA_370_XP_INT_FABRIC_MASK_OFFS	(0x54)
> -#define ARMADA_370_XP_INT_CAUSE_PERF(cpu)	(1 << cpu)
> -
> +/* Registers relative to main_int_base */
>  #define ARMADA_370_XP_INT_CONTROL		(0x00)
> +#define ARMADA_370_XP_SW_TRIG_INT_OFFS		(0x04)
>  #define ARMADA_370_XP_INT_SET_ENABLE_OFFS	(0x30)
>  #define ARMADA_370_XP_INT_CLEAR_ENABLE_OFFS	(0x34)
>  #define ARMADA_370_XP_INT_SOURCE_CTL(irq)	(0x100 + irq*4)
>  #define ARMADA_370_XP_INT_SOURCE_CPU_MASK	0xF
>  #define ARMADA_370_XP_INT_IRQ_FIQ_MASK(cpuid)	((BIT(0) | BIT(8)) << cpuid)
>  
> -#define ARMADA_370_XP_CPU_INTACK_OFFS		(0x44)
> +/* Registers relative to per_cpu_int_base */
> +#define ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS	(0x08)
> +#define ARMADA_370_XP_IN_DRBEL_MSK_OFFS		(0x0c)
>  #define ARMADA_375_PPI_CAUSE			(0x10)
> -
> -#define ARMADA_370_XP_SW_TRIG_INT_OFFS           (0x4)
> -#define ARMADA_370_XP_IN_DRBEL_MSK_OFFS          (0xc)
> -#define ARMADA_370_XP_IN_DRBEL_CAUSE_OFFS        (0x8)
> +#define ARMADA_370_XP_CPU_INTACK_OFFS		(0x44)
> +#define ARMADA_370_XP_INT_SET_MASK_OFFS		(0x48)
> +#define ARMADA_370_XP_INT_CLEAR_MASK_OFFS	(0x4C)
> +#define ARMADA_370_XP_INT_FABRIC_MASK_OFFS	(0x54)
> +#define ARMADA_370_XP_INT_CAUSE_PERF(cpu)	(1 << cpu)
>  
>  #define ARMADA_370_XP_MAX_PER_CPU_IRQS		(28)
>  
> -- 
> 2.6.2
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH 5/5] irqchip: armada-370-xp: document the overall driver logic

[Thomas Petazzoni]

Since the overall logic of the driver to handle the global and per-CPU
masking of the interrupts is far from trivial, this commit adds a long
comment detailing how the hardware operates and what strategy the
driver implements on top of that.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/irqchip/irq-armada-370-xp.c | 80 +++++++++++++++++++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 888add6..f14cc2d 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -34,6 +34,86 @@
 #include <asm/smp_plat.h>
 #include <asm/mach/irq.h>
 
+/*
+ * Overall diagram of the Armada XP interrupt controller:
+ *
+ *    To CPU 0                 To CPU 1
+ *
+ *       /\                       /\
+ *       ||                       ||
+ * +---------------+        +---------------+
+ * |               |	 |               |
+ * |    per-CPU    |	 |    per-CPU    |
+ * |  mask/unmask  |	 |  mask/unmask  |
+ * |     CPU0      |	 |     CPU1      |
+ * |               |	 |               |
+ * +---------------+	 +---------------+
+ *        /\                       /\
+ *        ||                       ||
+ *        \\_______________________//
+ *                     ||
+ *            +-------------------+
+ *            |                   |
+ *            | Global interrupt  |
+ *            |    mask/unmask    |
+ *            |                   |
+ *            +-------------------+
+ *                     /\
+ *                     ||
+ *               interrupt from
+ *                   device
+ *
+ * The "global interrupt mask/unmask" is modified using the
+ * ARMADA_370_XP_INT_SET_ENABLE_OFFS and
+ * ARMADA_370_XP_INT_CLEAR_ENABLE_OFFS registers, which are relative
+ * to "main_int_base".
+ *
+ * The "per-CPU mask/unmask" is modified using the
+ * ARMADA_370_XP_INT_SET_MASK_OFFS and
+ * ARMADA_370_XP_INT_CLEAR_MASK_OFFS registers, which are relative to
+ * "per_cpu_int_base". This base address points to a special address,
+ * which automatically accesses the registers of the current CPU.
+ *
+ * The per-CPU mask/unmask can also be adjusted using the global
+ * per-interrupt ARMADA_370_XP_INT_SOURCE_CTL register, which we use
+ * to configure interrupt affinity.
+ *
+ * Due to this model, all interrupts need to be mask/unmasked at two
+ * different levels: at the global level and at the per-CPU level.
+ *
+ * This driver takes the following approach to deal with this:
+ *
+ *  - For global interrupts:
+ *
+ *    At ->map() time, a global interrupt is unmasked at the per-CPU
+ *    mask/unmask level. It is therefore unmasked at this level for
+ *    the current CPU, running the ->map() code. This allows to have
+ *    the interrupt unmasked at this level in non-SMP
+ *    configurations. In SMP configurations, the ->set_affinity()
+ *    callback is called, which using the
+ *    ARMADA_370_XP_INT_SOURCE_CTL() readjusts the per-CPU mask/unmask
+ *    for the interrupt.
+ *
+ *    The ->mask() and ->unmask() operations only mask/unmask the
+ *    interrupt at the "global" level.
+ *
+ *    So, a global interrupt is enabled at the per-CPU level as soon
+ *    as it is mapped. At run time, the masking/unmasking takes place
+ *    at the global level.
+ *
+ *  - For per-CPU interrupts
+ *
+ *    At ->map() time, a per-CPU interrupt is unmasked at the global
+ *    mask/unmask level.
+ *
+ *    The ->mask() and ->unmask() operations mask/unmask the interrupt
+ *    at the per-CPU level.
+ *
+ *    So, a per-CPU interrupt is enabled at the global level as soon
+ *    as it is mapped. At run time, the masking/unmasking takes place
+ *    at the per-CPU level.
+ */
+
 /* Registers relative to main_int_base */
 #define ARMADA_370_XP_INT_CONTROL		(0x00)
 #define ARMADA_370_XP_SW_TRIG_INT_OFFS		(0x04)
-- 
2.6.2

Re: [PATCH 5/5] irqchip: armada-370-xp: document the overall driver logic

[Gregory CLEMENT]

Hi Thomas,
 
 On mar., oct. 20 2015, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote:

> Since the overall logic of the driver to handle the global and per-CPU
> masking of the interrupts is far from trivial, this commit adds a long
> comment detailing how the hardware operates and what strategy the
> driver implements on top of that.

It was really needed!

>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Acked-by: Gregory CLEMENT <gregory.clement@free-electrons.com>

Thanks,

Gregory

> ---
>  drivers/irqchip/irq-armada-370-xp.c | 80 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 80 insertions(+)
>
> diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
> index 888add6..f14cc2d 100644
> --- a/drivers/irqchip/irq-armada-370-xp.c
> +++ b/drivers/irqchip/irq-armada-370-xp.c
> @@ -34,6 +34,86 @@
>  #include <asm/smp_plat.h>
>  #include <asm/mach/irq.h>
>  
> +/*
> + * Overall diagram of the Armada XP interrupt controller:
> + *
> + *    To CPU 0                 To CPU 1
> + *
> + *       /\                       /\
> + *       ||                       ||
> + * +---------------+        +---------------+
> + * |               |	 |               |
> + * |    per-CPU    |	 |    per-CPU    |
> + * |  mask/unmask  |	 |  mask/unmask  |
> + * |     CPU0      |	 |     CPU1      |
> + * |               |	 |               |
> + * +---------------+	 +---------------+
> + *        /\                       /\
> + *        ||                       ||
> + *        \\_______________________//
> + *                     ||
> + *            +-------------------+
> + *            |                   |
> + *            | Global interrupt  |
> + *            |    mask/unmask    |
> + *            |                   |
> + *            +-------------------+
> + *                     /\
> + *                     ||
> + *               interrupt from
> + *                   device
> + *
> + * The "global interrupt mask/unmask" is modified using the
> + * ARMADA_370_XP_INT_SET_ENABLE_OFFS and
> + * ARMADA_370_XP_INT_CLEAR_ENABLE_OFFS registers, which are relative
> + * to "main_int_base".
> + *
> + * The "per-CPU mask/unmask" is modified using the
> + * ARMADA_370_XP_INT_SET_MASK_OFFS and
> + * ARMADA_370_XP_INT_CLEAR_MASK_OFFS registers, which are relative to
> + * "per_cpu_int_base". This base address points to a special address,
> + * which automatically accesses the registers of the current CPU.
> + *
> + * The per-CPU mask/unmask can also be adjusted using the global
> + * per-interrupt ARMADA_370_XP_INT_SOURCE_CTL register, which we use
> + * to configure interrupt affinity.
> + *
> + * Due to this model, all interrupts need to be mask/unmasked at two
> + * different levels: at the global level and at the per-CPU level.
> + *
> + * This driver takes the following approach to deal with this:
> + *
> + *  - For global interrupts:
> + *
> + *    At ->map() time, a global interrupt is unmasked at the per-CPU
> + *    mask/unmask level. It is therefore unmasked at this level for
> + *    the current CPU, running the ->map() code. This allows to have
> + *    the interrupt unmasked at this level in non-SMP
> + *    configurations. In SMP configurations, the ->set_affinity()
> + *    callback is called, which using the
> + *    ARMADA_370_XP_INT_SOURCE_CTL() readjusts the per-CPU mask/unmask
> + *    for the interrupt.
> + *
> + *    The ->mask() and ->unmask() operations only mask/unmask the
> + *    interrupt at the "global" level.
> + *
> + *    So, a global interrupt is enabled at the per-CPU level as soon
> + *    as it is mapped. At run time, the masking/unmasking takes place
> + *    at the global level.
> + *
> + *  - For per-CPU interrupts
> + *
> + *    At ->map() time, a per-CPU interrupt is unmasked at the global
> + *    mask/unmask level.
> + *
> + *    The ->mask() and ->unmask() operations mask/unmask the interrupt
> + *    at the per-CPU level.
> + *
> + *    So, a per-CPU interrupt is enabled at the global level as soon
> + *    as it is mapped. At run time, the masking/unmasking takes place
> + *    at the per-CPU level.
> + */
> +
>  /* Registers relative to main_int_base */
>  #define ARMADA_370_XP_INT_CONTROL		(0x00)
>  #define ARMADA_370_XP_SW_TRIG_INT_OFFS		(0x04)
> -- 
> 2.6.2
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [PATCH 5/5] irqchip: armada-370-xp: document the overall driver logic

[Thomas Petazzoni]

Hello,

On Tue, 20 Oct 2015 15:23:55 +0200, Thomas Petazzoni wrote:

> +/*
> + * Overall diagram of the Armada XP interrupt controller:
> + *
> + *    To CPU 0                 To CPU 1
> + *
> + *       /\                       /\
> + *       ||                       ||
> + * +---------------+        +---------------+
> + * |               |	 |               |

Things are a bit off here.

> + * |    per-CPU    |	 |    per-CPU    |
> + * |  mask/unmask  |	 |  mask/unmask  |
> + * |     CPU0      |	 |     CPU1      |
> + * |               |	 |               |
> + * +---------------+	 +---------------+
> + *        /\                       /\

and here. So I can fix that up in the next version.

If we're doing ASCII art, let's do it properly.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 5/5] irqchip: armada-370-xp: document the overall driver logic

[Jason Cooper]

On Tue, Oct 20, 2015 at 04:00:21PM +0200, Thomas Petazzoni wrote:
> Hello,
> 
> On Tue, 20 Oct 2015 15:23:55 +0200, Thomas Petazzoni wrote:
> 
> > +/*
> > + * Overall diagram of the Armada XP interrupt controller:
> > + *
> > + *    To CPU 0                 To CPU 1
> > + *
> > + *       /\                       /\
> > + *       ||                       ||
> > + * +---------------+        +---------------+
> > + * |               |	 |               |
> 
> Things are a bit off here.
> 
> > + * |    per-CPU    |	 |    per-CPU    |
> > + * |  mask/unmask  |	 |  mask/unmask  |
> > + * |     CPU0      |	 |     CPU1      |
> > + * |               |	 |               |
> > + * +---------------+	 +---------------+
> > + *        /\                       /\
> 
> and here. So I can fix that up in the next version.
> 
> If we're doing ASCII art, let's do it properly.

Agreed.  It's not pressing.  You can resubmit these last two separately
and we'll cue them up for 4.4.

thx,

Jason.

Re: [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Jason Cooper]

Thomas (tglx also),

On Tue, Oct 20, 2015 at 03:23:50PM +0200, Thomas Petazzoni wrote:
> Thomas, Jason, Marc, Rob,
> 
> In commit d17cab4451df ("irqchip: Kill off set_irq_flags usage"), Rob
> Herring modified the irqchip drivers to not use the ARM-specific
> set_irq_flags() and instead rely on various functions provided by the
> core irq subsystem.
> 
> While his commit was supposed to have no functional effect, it in fact
> does have one effect: the IRQ_NOAUTOEN flag used to be *cleared* for
> all interrupts and it is now *set* by default.
> 
> Thanks to this flag being *cleared* by default, the irq-armada-370-xp
> was able to properly re-enable per-CPU interrupts at resume time. Now
> that this flag is *set*, the irqd_irq_disabled() function no longer
> indicates that such per-CPU interrupts are enabled (and in fact a
> CPU-global flag to tell whether a per-CPU is enabled or not is
> silly). Due to this, our local timer per-CPU interrupt is no longer
> re-enabled at resume time on Armada XP, on the boot CPU, which causes
> a hang at resume time.
> 
> This is a regression between 4.2 (where suspend/resume works fine) and
> 4.3-rc (where suspend/resume is broken). Reverting d17cab4451df1 on
> top of 4.3-rc makes the problem go away (of course you also need to
> revert eb811129ed9ea so that set_irq_flags is re-introduced).
> 
> The minimal fix would be to clear the IRQ_NOAUTOEN flag so that we get
> back to the original situation. However, this does not really seem
> like the right fix.
> 
> Instead, this patch series proposes to add an is_enabled_percpu_irq()
> function to the core irq subsystem, which is then used by the
> irq-armada-370-xp to find out if such or such per-CPU interrupt should
> be re-enabled at resume time on the boot CPU.
> 
> The organization of the patch series is as follows:
> 
>  - PATCH 1 introduces the is_enabled_percpu_irq() function.
> 
>  - PATCH 2 does a minor refactoring of armada_xp_mpic_secondary_init()
>    to prepare the following patch.
> 
>  - PATCH 3 changes the irq-armada-370-xp driver to use the
>    is_enabled_percpu_irq() to re-enable the per-CPU interrupts on the
>    boot CPU at resume time, and also modifies the secondary CPU
>    notifier to re-enable per-CPU interrupts if needed.
> 
>  - PATCH 4 and 5 are further cleanups/improvements to the
>    irq-armada-370-xp, which are not needed to fix the problem.
> 
> Since this is fixing a regression introduced between 4.2 and 4.3-rc,
> it would be great if patches 1 to 3 could be merged in 4.3. The last
> two patches are only cosmetic, so merging them for 4.4 is of course
> the way to go.
> 
> Thanks,
> 
> Thomas
> 
> Thomas Petazzoni (5):
>   kernel: irq: implement is_enabled_percpu_irq()
>   irqchip: armada-370-xp: prepare additions to
>     armada_xp_mpic_secondary_init()
>   irqchip: armada-370-xp: re-enable per-CPU interrupts at resume time
>   irqchip: armada-370-xp: re-order register definitions
>   irqchip: armada-370-xp: document the overall driver logic
> 
>  drivers/irqchip/irq-armada-370-xp.c | 151 +++++++++++++++++++++++++++++++-----
>  include/linux/interrupt.h           |   1 +
>  kernel/irq/chip.c                   |   5 ++
>  kernel/irq/internals.h              |   1 +
>  kernel/irq/manage.c                 |  19 +++++
>  5 files changed, 158 insertions(+), 19 deletions(-)

Whole series,

Reviewed by: Jason Cooper <jason@lakedaemon.net>


Thomas (tglx), if you're happy with the core changes, let me know and
I'll cue the series up.

We know it's a bit late in the -rc cycle, but the alternative (reverting
Rob's patch) would likely be more destabilizing at this point.

thx,

Jason.

Re: [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Thomas Petazzoni]

Jason,

On Tue, 20 Oct 2015 14:04:27 +0000, Jason Cooper wrote:

> Whole series,
> 
> Reviewed by: Jason Cooper <jason@lakedaemon.net>

Thanks.

> Thomas (tglx), if you're happy with the core changes, let me know and
> I'll cue the series up.
> 
> We know it's a bit late in the -rc cycle, but the alternative (reverting
> Rob's patch) would likely be more destabilizing at this point.

As discussed on IRC, another simpler (code line wise) solution is to
simply clear the IRQ_NOAUTOEN flag in the irq-armada-370-xp, which
brings us back to what set_irq_flags() was doing, without actually
reverting Rob's patch.

However, relying on IRQ_NOAUTOEN being cleared doesn't seem like the
right long term solution, which is why I implemented what I believe is
a (hopefully) better long term solution.

But if you/tglx want something simple/stupid for 4.3 (one-line patch)
and then the real stuff for 4.4, I'd be happy to provide a series
implementing this idea.

Let me know what you and tglx think.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Russell King - ARM Linux]

On Tue, Oct 20, 2015 at 04:08:28PM +0200, Thomas Petazzoni wrote:
> As discussed on IRC, another simpler (code line wise) solution is to
> simply clear the IRQ_NOAUTOEN flag in the irq-armada-370-xp, which
> brings us back to what set_irq_flags() was doing, without actually
> reverting Rob's patch.
> 
> However, relying on IRQ_NOAUTOEN being cleared doesn't seem like the
> right long term solution, which is why I implemented what I believe is
> a (hopefully) better long term solution.

However, this is rather worrying.  NOAUTOEN is supposed to avoid enabling
the interrupt when the interrupt is claimed.

If, as a result of Rob's patch, we now have a load of IRQs which are
marked with NOAUTOEN which weren't, that's quite a large regression -
possibly one which hasn't been properly found (not everyone tests -rc
kernels) and we may be better to revert Rob's patch to avoid lots of
breakge being reported when 4.3 is released.

I think Rob's patches need another review in light of this, to determine
how much breakage there is here, and a decision how to proceed made on
that basis.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Thomas Petazzoni]

Russell,

On Tue, 20 Oct 2015 15:17:36 +0100, Russell King - ARM Linux wrote:

> However, this is rather worrying.  NOAUTOEN is supposed to avoid enabling
> the interrupt when the interrupt is claimed.
> 
> If, as a result of Rob's patch, we now have a load of IRQs which are
> marked with NOAUTOEN which weren't, that's quite a large regression -
> possibly one which hasn't been properly found (not everyone tests -rc
> kernels) and we may be better to revert Rob's patch to avoid lots of
> breakge being reported when 4.3 is released.

I believe the problem is only for per-CPU interrupts. We have
IRQ_NOAUTOEN set for per-CPU interrupts because:

static inline void irq_set_percpu_devid_flags(unsigned int irq)
{
        irq_set_status_flags(irq,
                             IRQ_NOAUTOEN | IRQ_PER_CPU | IRQ_NOTHREAD |
                             IRQ_NOPROBE | IRQ_PER_CPU_DEVID);
}

Calling set_irq_flags() used to have the effect of *clearing* the
IRQ_NOAUTOEN flag:

-void set_irq_flags(unsigned int irq, unsigned int iflags)
-{
-       unsigned long clr = 0, set = IRQ_NOREQUEST | IRQ_NOPROBE | IRQ_NOAUTOEN;
-
-       if (irq >= nr_irqs) {
-               pr_err("Trying to set irq flags for IRQ%d\n", irq);
-               return;
-       }
-
-       if (iflags & IRQF_VALID)
-               clr |= IRQ_NOREQUEST;
-       if (iflags & IRQF_PROBE)
-               clr |= IRQ_NOPROBE;
-       if (!(iflags & IRQF_NOAUTOEN))
-               clr |= IRQ_NOAUTOEN;
-       /* Order is clear bits in "clr" then set bits in "set" */
-       irq_modify_status(irq, clr, set & ~clr);
-}

I.e, unless you were passing IRQF_NOAUTOEN in your set_irq_flags()
invocation, set_irq_flags() was automatically clearing the IRQ_NOAUTOEN
bit.

But this is really only a per-CPU interrupt problem, which probably
limits the potential regressions caused by Rob's change.

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Thomas Gleixner]

On Tue, 20 Oct 2015, Russell King - ARM Linux wrote:

> On Tue, Oct 20, 2015 at 04:08:28PM +0200, Thomas Petazzoni wrote:
> > As discussed on IRC, another simpler (code line wise) solution is to
> > simply clear the IRQ_NOAUTOEN flag in the irq-armada-370-xp, which
> > brings us back to what set_irq_flags() was doing, without actually
> > reverting Rob's patch.
> > 
> > However, relying on IRQ_NOAUTOEN being cleared doesn't seem like the
> > right long term solution, which is why I implemented what I believe is
> > a (hopefully) better long term solution.
> 
> However, this is rather worrying.  NOAUTOEN is supposed to avoid enabling
> the interrupt when the interrupt is claimed.
> 
> If, as a result of Rob's patch, we now have a load of IRQs which are
> marked with NOAUTOEN which weren't, that's quite a large regression -
> possibly one which hasn't been properly found (not everyone tests -rc
> kernels) and we may be better to revert Rob's patch to avoid lots of
> breakge being reported when 4.3 is released.
> 
> I think Rob's patches need another review in light of this, to determine
> how much breakage there is here, and a decision how to proceed made on
> that basis.

I'll go over them tomorrow again and decide then what to do.

Thanks,

	tglx

Re: [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Thomas Gleixner]

On Tue, 20 Oct 2015, Thomas Gleixner wrote:
> On Tue, 20 Oct 2015, Russell King - ARM Linux wrote:
> > I think Rob's patches need another review in light of this, to determine
> > how much breakage there is here, and a decision how to proceed made on
> > that basis.
> 
> I'll go over them tomorrow again and decide then what to do.

I went over them once more and the only affected wreck I found is the
armada one.

Thanks,

	tglx

Re: [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Thomas Gleixner]

On Tue, 20 Oct 2015, Thomas Petazzoni wrote:
> As discussed on IRC, another simpler (code line wise) solution is to
> simply clear the IRQ_NOAUTOEN flag in the irq-armada-370-xp, which
> brings us back to what set_irq_flags() was doing, without actually
> reverting Rob's patch.

I prefer that for 4.3.
 
> However, relying on IRQ_NOAUTOEN being cleared doesn't seem like the
> right long term solution, which is why I implemented what I believe is
> a (hopefully) better long term solution.

Agreed.

I'll go over the proposed solution tomorrow afternoon (I'm
traveling/conferencing...)

Thanks,

	tglx

[PATCH] irqchip: irq-armada-370-xp: fix regression by clearing IRQ_NOAUTOEN

[Thomas Petazzoni]

Commit d17cab4451df1 ("irqchip: Kill off set_irq_flags usage") changed
the code of armada_370_xp_mpic_irq_map() from using set_irq_flags() to
irq_set_probe().

While the commit log seems to imply that there are no functional
changes, there are indeed functional changes introduced by this
commit: the IRQ_NOAUTOEN flag is no longer cleared. This functional
change causes a regression on Armada XP, which no longer works
properly after suspend/resume because per-CPU interrupts remain
disabled.

Due to how the hardware registers work, the irq-armada-370-xp cannot
simply save/restore a bunch of registers at suspend/resume to make
sure that the interrupts remain in the same state after
resuming. Therefore, it relies on the kernel to say whether the
interrupt is disabled or not, using the irqd_irq_disabled()
function. This was all working fine while the IRQ_NOAUTOEN flag was
cleared.

With the change introduced by Rob Herring in d17cab4451df1, the
IRQ_NOAUTOEN flag is now set for all interrupts. irqd_irq_disabled()
returns false for per-CPU interrupts, and therefore our per-CPU
interrupts are no longer re-enabled after resume.

This commit works around this problem by clearing again the
IRQ_NOAUTOEN flags, so that we are back to the situation we had before
commit d17cab4451df1. This work around is proposed as a minimal fix
for the problem, while a better long-term solution is being worked on.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
Here is the one-line minimal change that tglx said would be more
acceptable to have in 4.3.
---
 drivers/irqchip/irq-armada-370-xp.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/irqchip/irq-armada-370-xp.c b/drivers/irqchip/irq-armada-370-xp.c
index 655cb96..389318a 100644
--- a/drivers/irqchip/irq-armada-370-xp.c
+++ b/drivers/irqchip/irq-armada-370-xp.c
@@ -317,6 +317,7 @@ static int armada_370_xp_mpic_irq_map(struct irq_domain *h,
 					handle_level_irq);
 	}
 	irq_set_probe(virq);
+	irq_clear_status_flags(virq, IRQ_NOAUTOEN);
 
 	return 0;
 }
-- 
2.6.2

Re: [PATCH] irqchip: irq-armada-370-xp: fix regression by clearing IRQ_NOAUTOEN

[Jason Cooper]

Hey Thomas-i, Thomases, :)

On Wed, Oct 21, 2015 at 03:48:15PM +0200, Thomas Petazzoni wrote:
> Commit d17cab4451df1 ("irqchip: Kill off set_irq_flags usage") changed
> the code of armada_370_xp_mpic_irq_map() from using set_irq_flags() to
> irq_set_probe().
> 
> While the commit log seems to imply that there are no functional
> changes, there are indeed functional changes introduced by this
> commit: the IRQ_NOAUTOEN flag is no longer cleared. This functional
> change causes a regression on Armada XP, which no longer works
> properly after suspend/resume because per-CPU interrupts remain
> disabled.
> 
> Due to how the hardware registers work, the irq-armada-370-xp cannot
> simply save/restore a bunch of registers at suspend/resume to make
> sure that the interrupts remain in the same state after
> resuming. Therefore, it relies on the kernel to say whether the
> interrupt is disabled or not, using the irqd_irq_disabled()
> function. This was all working fine while the IRQ_NOAUTOEN flag was
> cleared.
> 
> With the change introduced by Rob Herring in d17cab4451df1, the
> IRQ_NOAUTOEN flag is now set for all interrupts. irqd_irq_disabled()
> returns false for per-CPU interrupts, and therefore our per-CPU
> interrupts are no longer re-enabled after resume.
> 
> This commit works around this problem by clearing again the
> IRQ_NOAUTOEN flags, so that we are back to the situation we had before
> commit d17cab4451df1. This work around is proposed as a minimal fix
> for the problem, while a better long-term solution is being worked on.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
> Here is the one-line minimal change that tglx said would be more
> acceptable to have in 4.3.
> ---
>  drivers/irqchip/irq-armada-370-xp.c | 1 +
>  1 file changed, 1 insertion(+)

Applied to irqchip/urgent with minor tweaking of the subject line.  I
trust at this point it's ok to pull in -rc2.

thx,

Jason.

Re: [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Thomas Petazzoni]

Thomas,

On Tue, 20 Oct 2015 21:23:29 +0200 (CEST), Thomas Gleixner wrote:
> On Tue, 20 Oct 2015, Thomas Petazzoni wrote:
> > As discussed on IRC, another simpler (code line wise) solution is to
> > simply clear the IRQ_NOAUTOEN flag in the irq-armada-370-xp, which
> > brings us back to what set_irq_flags() was doing, without actually
> > reverting Rob's patch.
> 
> I prefer that for 4.3.

Done, I've proposed a patch as a reply to this e-mail. It's a one-liner.

> > However, relying on IRQ_NOAUTOEN being cleared doesn't seem like the
> > right long term solution, which is why I implemented what I believe is
> > a (hopefully) better long term solution.
> 
> Agreed.
> 
> I'll go over the proposed solution tomorrow afternoon (I'm
> traveling/conferencing...)

Sure, no problem.

I also noticed that MSI interrupts are not properly re-enabled after
resume, but it's not a regression from 4.2, as I can also reproduce the
issue on 4.2, so I'll handle that separately.

Thanks!

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Thomas Petazzoni]

Thomas,

On Tue, 20 Oct 2015 21:23:29 +0200 (CEST), Thomas Gleixner wrote:

> > However, relying on IRQ_NOAUTOEN being cleared doesn't seem like the
> > right long term solution, which is why I implemented what I believe is
> > a (hopefully) better long term solution.
> 
> Agreed.
> 
> I'll go over the proposed solution tomorrow afternoon (I'm
> traveling/conferencing...)

Have you had the time to consider the proposed solution? For 4.3 we
implemented the quick work-around that consisted in clearing
IRQ_NOAUTOEN, but it's probably not a very good long-term solution.

Don't hesitate to let me know if you'd like to see some modifications
to the proposed approach, or if you have a totally different approach
in mind.

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Thomas Gleixner]

Thomas,

On Wed, 11 Nov 2015, Thomas Petazzoni wrote:
> Have you had the time to consider the proposed solution? For 4.3 we
> implemented the quick work-around that consisted in clearing
> IRQ_NOAUTOEN, but it's probably not a very good long-term solution.
> 
> Don't hesitate to let me know if you'd like to see some modifications
> to the proposed approach, or if you have a totally different approach
> in mind.

I'm not sure if we really need all that muck if we can just rely on
that flag. I don't see the extra value, but you might have something
in mind which does not jump into my face right now.

Thanks,

	tglx

Re: [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Thomas Petazzoni]

Thomas,

On Fri, 13 Nov 2015 15:11:16 -0500 (EST), Thomas Gleixner wrote:

> On Wed, 11 Nov 2015, Thomas Petazzoni wrote:
> > Have you had the time to consider the proposed solution? For 4.3 we
> > implemented the quick work-around that consisted in clearing
> > IRQ_NOAUTOEN, but it's probably not a very good long-term solution.
> > 
> > Don't hesitate to let me know if you'd like to see some modifications
> > to the proposed approach, or if you have a totally different approach
> > in mind.
> 
> I'm not sure if we really need all that muck if we can just rely on
> that flag. I don't see the extra value, but you might have something
> in mind which does not jump into my face right now.

Well, the problem is that IRQ_NOAUTOEN is a global flag, which is OK
for global interrupts, but not good for per-CPU interrupts, since you
don't have the information on a per-CPU basis of which interrupt was
enabled before suspend, and therefore should be re-enabled after resume.

Until now, we don't have the problem since the only per-CPU interrupt
we were using was the local timer interrupt, and the local timers on
secondary CPUs are switched off during suspend and re-enabled during
resume. So re-enabling the interrupt on the boot CPU on resume is
sufficient.

However, our network driver recently switched to using per-CPU
interrupts as well, and in this case, it is really important to be able
to re-enable the per-CPU interrupts and the appropriate CPUs at resume
time. Since our HW registers are made so that it is not possible to
read out at suspend time which interrupts are enabled, we have to ask
the Linux kernel at resume time which interrupts should be re-enabled
at the HW level. Which is what my more complicated series was doing.

Do you have other suggestions to allow us to know which per-CPU
interrupts should be re-enabled on the different CPUs at resume time ?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Thomas Gleixner]

Thomas,

On Fri, 4 Dec 2015, Thomas Petazzoni wrote:
> Well, the problem is that IRQ_NOAUTOEN is a global flag, which is OK
> for global interrupts, but not good for per-CPU interrupts, since you
> don't have the information on a per-CPU basis of which interrupt was
> enabled before suspend, and therefore should be re-enabled after resume.
> 
> Until now, we don't have the problem since the only per-CPU interrupt
> we were using was the local timer interrupt, and the local timers on
> secondary CPUs are switched off during suspend and re-enabled during
> resume. So re-enabling the interrupt on the boot CPU on resume is
> sufficient.
> 
> However, our network driver recently switched to using per-CPU
> interrupts as well, and in this case, it is really important to be able
> to re-enable the per-CPU interrupts and the appropriate CPUs at resume
> time. Since our HW registers are made so that it is not possible to
> read out at suspend time which interrupts are enabled, we have to ask
> the Linux kernel at resume time which interrupts should be re-enabled
> at the HW level. Which is what my more complicated series was doing.
> 
> Do you have other suggestions to allow us to know which per-CPU
> interrupts should be re-enabled on the different CPUs at resume time ?

Ok. That makes sense. So I'm going to pick up the core change.

Thanks,

	tglx

Re: [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Thomas Gleixner]

Thomas,

On Sat, 5 Dec 2015, Thomas Gleixner wrote:
> On Fri, 4 Dec 2015, Thomas Petazzoni wrote:
> > Well, the problem is that IRQ_NOAUTOEN is a global flag, which is OK
> > for global interrupts, but not good for per-CPU interrupts, since you
> > don't have the information on a per-CPU basis of which interrupt was
> > enabled before suspend, and therefore should be re-enabled after resume.
> > 
> > Until now, we don't have the problem since the only per-CPU interrupt
> > we were using was the local timer interrupt, and the local timers on
> > secondary CPUs are switched off during suspend and re-enabled during
> > resume. So re-enabling the interrupt on the boot CPU on resume is
> > sufficient.
> > 
> > However, our network driver recently switched to using per-CPU
> > interrupts as well, and in this case, it is really important to be able
> > to re-enable the per-CPU interrupts and the appropriate CPUs at resume
> > time. Since our HW registers are made so that it is not possible to
> > read out at suspend time which interrupts are enabled, we have to ask
> > the Linux kernel at resume time which interrupts should be re-enabled
> > at the HW level. Which is what my more complicated series was doing.
> > 
> > Do you have other suggestions to allow us to know which per-CPU
> > interrupts should be re-enabled on the different CPUs at resume time ?
> 
> Ok. That makes sense. So I'm going to pick up the core change.

Second thoughts. That network driver example does not make sense.

You have a suspend/resume mechanism and a cpu hotplug machinery in
that driver, right? So that should be responsible for
disabling/enabling the per cpu interrupts. I don't think it's the
proper way to do that in the irq chip driver at some random point
during resume as you'd reenable interrupts on cpus which are not
online yet.

Thanks,

	tglx

Re: [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Thomas Petazzoni]

Hello Thomas,

On Sun, 6 Dec 2015 10:28:15 +0100 (CET), Thomas Gleixner wrote:

> Second thoughts. That network driver example does not make sense.
> 
> You have a suspend/resume mechanism and a cpu hotplug machinery in
> that driver, right? So that should be responsible for
> disabling/enabling the per cpu interrupts. I don't think it's the
> proper way to do that in the irq chip driver at some random point
> during resume as you'd reenable interrupts on cpus which are not
> online yet.

The irqchip driver would re-enable the per-CPU interrupts in a CPU
notifier, so only when the secondary CPUs come online again after
resume.

When a device driver uses a normal (non per-CPU) interrupt, then it
doesn't have to take care of disabling the interrupt on suspend and
re-enabling the interrupt on resume at the interrupt controller level.
This is all transparently handled by the irqchip driver.

Why should the handling of per-CPU interrupts be different and require
explicit handling from each device driver rather than being
transparently handled by the irqchip driver ?

Best regards,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Thomas Gleixner]

On Tue, 8 Dec 2015, Thomas Petazzoni wrote:
> When a device driver uses a normal (non per-CPU) interrupt, then it
> doesn't have to take care of disabling the interrupt on suspend and
> re-enabling the interrupt on resume at the interrupt controller level.
> This is all transparently handled by the irqchip driver.
> 
> Why should the handling of per-CPU interrupts be different and require
> explicit handling from each device driver rather than being
> transparently handled by the irqchip driver ?

Fair enough. Did not think about the boot cpu part.

Thanks,

	tglx

Re: [PATCH 0/5] Fix regression introduced by set_irq_flags() removal

[Thomas Petazzoni]

Thomas,

On Tue, 8 Dec 2015 11:54:11 +0100 (CET), Thomas Gleixner wrote:
> On Tue, 8 Dec 2015, Thomas Petazzoni wrote:
> > When a device driver uses a normal (non per-CPU) interrupt, then it
> > doesn't have to take care of disabling the interrupt on suspend and
> > re-enabling the interrupt on resume at the interrupt controller level.
> > This is all transparently handled by the irqchip driver.
> > 
> > Why should the handling of per-CPU interrupts be different and require
> > explicit handling from each device driver rather than being
> > transparently handled by the irqchip driver ?  
> 
> Fair enough. Did not think about the boot cpu part.

I am reviving this *very* old thread, as I wanted to push the remaining
patches. But it seems like the issue no longer exists.

Summary of the story:

 - Between 4.2 and 4.3-rc, commit d17cab4451df1 broke suspend/resume on
   Armada XP. The issue was that the IRQ_NOAUTOEN was no longer
   cleared, and therefore irqd_irq_disabled() no longer indicated that
   our per-CPU interrupts were enabled, and consequently our local
   timer per-CPU interrupt was not re-enabled upon resume.

 - After some discussion, we merged a very simple workaround for 4.3,
   which consists in clearing IRQ_NOAUTOEN:

    irq_clear_status_flags(virq, IRQ_NOAUTOEN);

   But the idea was to do a better implementation later on, which my
   patch series at the same time was trying.

 - Now, fast forward to 4.10, if I remove the:

    irq_clear_status_flags(virq, IRQ_NOAUTOEN);

   then I can no longer reproduce the original issue from 4.3-rc, which
   clearing IRQ_NOAUTOEN was fixing. My system resumes fine.

Has there been some changes in this area that could explain that
irqd_irq_disable() now indicates that my per-CPU interrupt is enabled,
while it didn't say so back in 4.3-rc ?

Thanks,

Thomas
-- 
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com