All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2] clocksource/arm_arch_timer: Force per-CPU interrupt to be level-triggered
@ 2016-05-09 14:51 ` Marc Zyngier
  0 siblings, 0 replies; 4+ messages in thread
From: Marc Zyngier @ 2016-05-09 14:51 UTC (permalink / raw)
  To: Thomas Gleixner, Daniel Lezcano
  Cc: linux-arm-kernel, linux-kernel, Jon Hunter

The ARM architected timer produces level-triggered interrupts (this
is mandated by the architecture). Unfortunately, most device-trees
get this wrong, and expose an edge-triggered interrupt.

Until now, this wasn't too much an issue, as the programming of the
trigger would fail (the corresponding PPI cannot be reconfigured),
and the kernel would be happy with this. But we're about to change
this, and trust DT a lot if the driver doesn't provide its own
trigger information. In that context, the timer breaks badly.

While we do need to fix the DTs, there is also some userspace out
there (kvmtool) that generates the same kind of broken DT on the
fly, and that will completely break with newer kernels.

As a safety measure, and to keep buggy software alive as well as
buying us some time to fix DTs all over the place, let's check
what trigger configuration has been given us by the firmware.
If this is not a level configuration, then we know that the
DT/ACPI configuration is bust, and we pick some defaults which
won't be worse than the existing setup.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
- from v1: realised that there is no sane default, as we already have
  diverging configurations. Instead, test for a level interrupt, and
  force it to level low if current setting was edge. Also scream at the
  unfortunate user.

 drivers/clocksource/arm_arch_timer.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 5152b38..be8cc6d 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -8,6 +8,9 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+
+#define pr_fmt(fmt)	"arm_arch_timer: " fmt
+
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/device.h>
@@ -362,14 +365,32 @@ static bool arch_timer_has_nonsecure_ppi(void)
 		arch_timer_ppi[PHYS_NONSECURE_PPI]);
 }
 
+static u32 check_ppi_trigger(int irq)
+{
+	u32 flags = irq_get_trigger_type(irq);
+
+	if (flags != IRQF_TRIGGER_HIGH && flags != IRQF_TRIGGER_LOW) {
+		pr_warn("WARNING: Invalid trigger for IRQ%d, assuming level low\n", irq);
+		pr_warn("WARNING: Please fix your firmware\n");
+		flags = IRQF_TRIGGER_LOW;
+	}
+
+	return flags;
+}
+
 static int arch_timer_setup(struct clock_event_device *clk)
 {
+	u32 flags;
+
 	__arch_timer_setup(ARCH_CP15_TIMER, clk);
 
-	enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], 0);
+	flags = check_ppi_trigger(arch_timer_ppi[arch_timer_uses_ppi]);
+	enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], flags);
 
-	if (arch_timer_has_nonsecure_ppi())
-		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
+	if (arch_timer_has_nonsecure_ppi()) {
+		flags = check_ppi_trigger(arch_timer_ppi[PHYS_NONSECURE_PPI]);
+		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], flags);
+	}
 
 	arch_counter_set_user_access();
 	if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM))
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* [PATCH v2] clocksource/arm_arch_timer: Force per-CPU interrupt to be level-triggered
@ 2016-05-09 14:51 ` Marc Zyngier
  0 siblings, 0 replies; 4+ messages in thread
From: Marc Zyngier @ 2016-05-09 14:51 UTC (permalink / raw)
  To: linux-arm-kernel

The ARM architected timer produces level-triggered interrupts (this
is mandated by the architecture). Unfortunately, most device-trees
get this wrong, and expose an edge-triggered interrupt.

Until now, this wasn't too much an issue, as the programming of the
trigger would fail (the corresponding PPI cannot be reconfigured),
and the kernel would be happy with this. But we're about to change
this, and trust DT a lot if the driver doesn't provide its own
trigger information. In that context, the timer breaks badly.

While we do need to fix the DTs, there is also some userspace out
there (kvmtool) that generates the same kind of broken DT on the
fly, and that will completely break with newer kernels.

As a safety measure, and to keep buggy software alive as well as
buying us some time to fix DTs all over the place, let's check
what trigger configuration has been given us by the firmware.
If this is not a level configuration, then we know that the
DT/ACPI configuration is bust, and we pick some defaults which
won't be worse than the existing setup.

Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
- from v1: realised that there is no sane default, as we already have
  diverging configurations. Instead, test for a level interrupt, and
  force it to level low if current setting was edge. Also scream at the
  unfortunate user.

 drivers/clocksource/arm_arch_timer.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
index 5152b38..be8cc6d 100644
--- a/drivers/clocksource/arm_arch_timer.c
+++ b/drivers/clocksource/arm_arch_timer.c
@@ -8,6 +8,9 @@
  * it under the terms of the GNU General Public License version 2 as
  * published by the Free Software Foundation.
  */
+
+#define pr_fmt(fmt)	"arm_arch_timer: " fmt
+
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/device.h>
@@ -362,14 +365,32 @@ static bool arch_timer_has_nonsecure_ppi(void)
 		arch_timer_ppi[PHYS_NONSECURE_PPI]);
 }
 
+static u32 check_ppi_trigger(int irq)
+{
+	u32 flags = irq_get_trigger_type(irq);
+
+	if (flags != IRQF_TRIGGER_HIGH && flags != IRQF_TRIGGER_LOW) {
+		pr_warn("WARNING: Invalid trigger for IRQ%d, assuming level low\n", irq);
+		pr_warn("WARNING: Please fix your firmware\n");
+		flags = IRQF_TRIGGER_LOW;
+	}
+
+	return flags;
+}
+
 static int arch_timer_setup(struct clock_event_device *clk)
 {
+	u32 flags;
+
 	__arch_timer_setup(ARCH_CP15_TIMER, clk);
 
-	enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], 0);
+	flags = check_ppi_trigger(arch_timer_ppi[arch_timer_uses_ppi]);
+	enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], flags);
 
-	if (arch_timer_has_nonsecure_ppi())
-		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
+	if (arch_timer_has_nonsecure_ppi()) {
+		flags = check_ppi_trigger(arch_timer_ppi[PHYS_NONSECURE_PPI]);
+		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], flags);
+	}
 
 	arch_counter_set_user_access();
 	if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM))
-- 
2.1.4

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH v2] clocksource/arm_arch_timer: Force per-CPU interrupt to be level-triggered
  2016-05-09 14:51 ` Marc Zyngier
@ 2016-05-09 16:06   ` Matthias Brugger
  -1 siblings, 0 replies; 4+ messages in thread
From: Matthias Brugger @ 2016-05-09 16:06 UTC (permalink / raw)
  To: Marc Zyngier, Thomas Gleixner, Daniel Lezcano
  Cc: linux-kernel, linux-arm-kernel, Jon Hunter



On 09/05/16 16:51, Marc Zyngier wrote:
> The ARM architected timer produces level-triggered interrupts (this
> is mandated by the architecture). Unfortunately, most device-trees
> get this wrong, and expose an edge-triggered interrupt.
>
> Until now, this wasn't too much an issue, as the programming of the
> trigger would fail (the corresponding PPI cannot be reconfigured),
> and the kernel would be happy with this. But we're about to change
> this, and trust DT a lot if the driver doesn't provide its own
> trigger information. In that context, the timer breaks badly.
>
> While we do need to fix the DTs, there is also some userspace out
> there (kvmtool) that generates the same kind of broken DT on the
> fly, and that will completely break with newer kernels.
>
> As a safety measure, and to keep buggy software alive as well as
> buying us some time to fix DTs all over the place, let's check
> what trigger configuration has been given us by the firmware.
> If this is not a level configuration, then we know that the
> DT/ACPI configuration is bust, and we pick some defaults which
> won't be worse than the existing setup.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Reviewed-by: Matthias Brugger <mbrugger@suse.com>

> ---
> - from v1: realised that there is no sane default, as we already have
>    diverging configurations. Instead, test for a level interrupt, and
>    force it to level low if current setting was edge. Also scream at the
>    unfortunate user.
>
>   drivers/clocksource/arm_arch_timer.c | 27 ++++++++++++++++++++++++---
>   1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 5152b38..be8cc6d 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -8,6 +8,9 @@
>    * it under the terms of the GNU General Public License version 2 as
>    * published by the Free Software Foundation.
>    */
> +
> +#define pr_fmt(fmt)	"arm_arch_timer: " fmt
> +
>   #include <linux/init.h>
>   #include <linux/kernel.h>
>   #include <linux/device.h>
> @@ -362,14 +365,32 @@ static bool arch_timer_has_nonsecure_ppi(void)
>   		arch_timer_ppi[PHYS_NONSECURE_PPI]);
>   }
>
> +static u32 check_ppi_trigger(int irq)
> +{
> +	u32 flags = irq_get_trigger_type(irq);
> +
> +	if (flags != IRQF_TRIGGER_HIGH && flags != IRQF_TRIGGER_LOW) {
> +		pr_warn("WARNING: Invalid trigger for IRQ%d, assuming level low\n", irq);
> +		pr_warn("WARNING: Please fix your firmware\n");
> +		flags = IRQF_TRIGGER_LOW;
> +	}
> +
> +	return flags;
> +}
> +
>   static int arch_timer_setup(struct clock_event_device *clk)
>   {
> +	u32 flags;
> +
>   	__arch_timer_setup(ARCH_CP15_TIMER, clk);
>
> -	enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], 0);
> +	flags = check_ppi_trigger(arch_timer_ppi[arch_timer_uses_ppi]);
> +	enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], flags);
>
> -	if (arch_timer_has_nonsecure_ppi())
> -		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
> +	if (arch_timer_has_nonsecure_ppi()) {
> +		flags = check_ppi_trigger(arch_timer_ppi[PHYS_NONSECURE_PPI]);
> +		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], flags);
> +	}
>
>   	arch_counter_set_user_access();
>   	if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM))
>

^ permalink raw reply	[flat|nested] 4+ messages in thread
* [PATCH v2] clocksource/arm_arch_timer: Force per-CPU interrupt to be level-triggered
@ 2016-05-09 16:06   ` Matthias Brugger
  0 siblings, 0 replies; 4+ messages in thread
From: Matthias Brugger @ 2016-05-09 16:06 UTC (permalink / raw)
  To: linux-arm-kernel



On 09/05/16 16:51, Marc Zyngier wrote:
> The ARM architected timer produces level-triggered interrupts (this
> is mandated by the architecture). Unfortunately, most device-trees
> get this wrong, and expose an edge-triggered interrupt.
>
> Until now, this wasn't too much an issue, as the programming of the
> trigger would fail (the corresponding PPI cannot be reconfigured),
> and the kernel would be happy with this. But we're about to change
> this, and trust DT a lot if the driver doesn't provide its own
> trigger information. In that context, the timer breaks badly.
>
> While we do need to fix the DTs, there is also some userspace out
> there (kvmtool) that generates the same kind of broken DT on the
> fly, and that will completely break with newer kernels.
>
> As a safety measure, and to keep buggy software alive as well as
> buying us some time to fix DTs all over the place, let's check
> what trigger configuration has been given us by the firmware.
> If this is not a level configuration, then we know that the
> DT/ACPI configuration is bust, and we pick some defaults which
> won't be worse than the existing setup.
>
> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>

Reviewed-by: Matthias Brugger <mbrugger@suse.com>

> ---
> - from v1: realised that there is no sane default, as we already have
>    diverging configurations. Instead, test for a level interrupt, and
>    force it to level low if current setting was edge. Also scream at the
>    unfortunate user.
>
>   drivers/clocksource/arm_arch_timer.c | 27 ++++++++++++++++++++++++---
>   1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c
> index 5152b38..be8cc6d 100644
> --- a/drivers/clocksource/arm_arch_timer.c
> +++ b/drivers/clocksource/arm_arch_timer.c
> @@ -8,6 +8,9 @@
>    * it under the terms of the GNU General Public License version 2 as
>    * published by the Free Software Foundation.
>    */
> +
> +#define pr_fmt(fmt)	"arm_arch_timer: " fmt
> +
>   #include <linux/init.h>
>   #include <linux/kernel.h>
>   #include <linux/device.h>
> @@ -362,14 +365,32 @@ static bool arch_timer_has_nonsecure_ppi(void)
>   		arch_timer_ppi[PHYS_NONSECURE_PPI]);
>   }
>
> +static u32 check_ppi_trigger(int irq)
> +{
> +	u32 flags = irq_get_trigger_type(irq);
> +
> +	if (flags != IRQF_TRIGGER_HIGH && flags != IRQF_TRIGGER_LOW) {
> +		pr_warn("WARNING: Invalid trigger for IRQ%d, assuming level low\n", irq);
> +		pr_warn("WARNING: Please fix your firmware\n");
> +		flags = IRQF_TRIGGER_LOW;
> +	}
> +
> +	return flags;
> +}
> +
>   static int arch_timer_setup(struct clock_event_device *clk)
>   {
> +	u32 flags;
> +
>   	__arch_timer_setup(ARCH_CP15_TIMER, clk);
>
> -	enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], 0);
> +	flags = check_ppi_trigger(arch_timer_ppi[arch_timer_uses_ppi]);
> +	enable_percpu_irq(arch_timer_ppi[arch_timer_uses_ppi], flags);
>
> -	if (arch_timer_has_nonsecure_ppi())
> -		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], 0);
> +	if (arch_timer_has_nonsecure_ppi()) {
> +		flags = check_ppi_trigger(arch_timer_ppi[PHYS_NONSECURE_PPI]);
> +		enable_percpu_irq(arch_timer_ppi[PHYS_NONSECURE_PPI], flags);
> +	}
>
>   	arch_counter_set_user_access();
>   	if (IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM))
>

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-05-09 16:06 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-05-09 14:51 [PATCH v2] clocksource/arm_arch_timer: Force per-CPU interrupt to be level-triggered Marc Zyngier
2016-05-09 14:51 ` Marc Zyngier
2016-05-09 16:06 ` Matthias Brugger
2016-05-09 16:06   ` Matthias Brugger

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.