Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1311100 > unrolled thread

[PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-01-17 03:30 +0100
    Re: [PATCH 4.4-rt2] fix  arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-01-18 18:30 +0100
      Re: [PATCH 4.4-rt2] fix  arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-01-18 19:50 +0100
        Re: [PATCH 4.4-rt2] fix  arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch Sebastian Andrzej Siewior <bigeasy@linutronix.de> - 2016-01-18 21:30 +0100
          Re: [PATCH 4.4-rt2] fix  arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch Alexandre Belloni <alexandre.belloni@free-electrons.com> - 2016-01-19 02:30 +0100
        Re: [PATCH 4.4-rt2] fix  arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch Thomas Gleixner <tglx@linutronix.de> - 2016-01-20 12:10 +0100

#1311100 — [PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch breaks the
build, fix that.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/clocksource/timer-atmel-pit.c | 68 +++++++++++++++++------------------
 drivers/clocksource/timer-atmel-st.c  |  8 ++---
 2 files changed, 38 insertions(+), 38 deletions(-)

diff --git a/drivers/clocksource/timer-atmel-pit.c b/drivers/clocksource/timer-atmel-pit.c
index 80d74c4adcbe..43b50634d640 100644
--- a/drivers/clocksource/timer-atmel-pit.c
+++ b/drivers/clocksource/timer-atmel-pit.c
@@ -96,11 +96,44 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
 
 	/* disable irq, leaving the clocksource active */
 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
-	free_irq(atmel_pit_irq, data);
+	if (!clockevent_state_detached(dev))
+		free_irq(data->irq, data);
 	return 0;
 }
 
 /*
+ * IRQ handler for the timer.
+ */
+static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
+{
+	struct pit_data *data = dev_id;
+
+	/*
+	 * irqs should be disabled here, but as the irq is shared they are only
+	 * guaranteed to be off if the timer irq is registered first.
+	 */
+	WARN_ON_ONCE(!irqs_disabled());
+
+	/* The PIT interrupt may be disabled, and is shared */
+	if (clockevent_state_periodic(&data->clkevt) &&
+	    (pit_read(data->base, AT91_PIT_SR) & AT91_PIT_PITS)) {
+		unsigned nr_ticks;
+
+		/* Get number of ticks performed before irq, and ack it */
+		nr_ticks = PIT_PICNT(pit_read(data->base, AT91_PIT_PIVR));
+		do {
+			data->cnt += data->cycle;
+			data->clkevt.event_handler(&data->clkevt);
+			nr_ticks--;
+		} while (nr_ticks);
+
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+/*
  * Clockevent device:  interrupts every 1/HZ (== pit_cycles * MCK/16)
  */
 static int pit_clkevt_set_periodic(struct clock_event_device *dev)
@@ -151,45 +184,12 @@ static void at91sam926x_pit_resume(struct clock_event_device *cedev)
 }
 
 /*
- * IRQ handler for the timer.
- */
-static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
-{
-	struct pit_data *data = dev_id;
-
-	/*
-	 * irqs should be disabled here, but as the irq is shared they are only
-	 * guaranteed to be off if the timer irq is registered first.
-	 */
-	WARN_ON_ONCE(!irqs_disabled());
-
-	/* The PIT interrupt may be disabled, and is shared */
-	if (clockevent_state_periodic(&data->clkevt) &&
-	    (pit_read(data->base, AT91_PIT_SR) & AT91_PIT_PITS)) {
-		unsigned nr_ticks;
-
-		/* Get number of ticks performed before irq, and ack it */
-		nr_ticks = PIT_PICNT(pit_read(data->base, AT91_PIT_PIVR));
-		do {
-			data->cnt += data->cycle;
-			data->clkevt.event_handler(&data->clkevt);
-			nr_ticks--;
-		} while (nr_ticks);
-
-		return IRQ_HANDLED;
-	}
-
-	return IRQ_NONE;
-}
-
-/*
  * Set up both clocksource and clockevent support.
  */
 static void __init at91sam926x_pit_common_init(struct pit_data *data)
 {
 	unsigned long	pit_rate;
 	unsigned	bits;
-	int		ret;
 
 	/*
 	 * Use our actual MCK to figure out how many MCK/16 ticks per
diff --git a/drivers/clocksource/timer-atmel-st.c b/drivers/clocksource/timer-atmel-st.c
index ea37afc26e1b..11ce404d0791 100644
--- a/drivers/clocksource/timer-atmel-st.c
+++ b/drivers/clocksource/timer-atmel-st.c
@@ -150,7 +150,7 @@ static int clkevt32k_set_oneshot(struct clock_event_device *dev)
 
 static int clkevt32k_set_periodic(struct clock_event_device *dev)
 {
-	int irq;
+	int ret;
 
 	clkdev32k_disable_and_flush_irq();
 
@@ -229,15 +229,15 @@ static void __init atmel_st_timer_init(struct device_node *node)
 	regmap_read(regmap_st, AT91_ST_SR, &val);
 
 	/* Get the interrupts property */
-	irq  = irq_of_parse_and_map(node, 0);
-	if (!irq)
+	atmel_st_irq  = irq_of_parse_and_map(node, 0);
+	if (!atmel_st_irq)
 		panic(pr_fmt("Unable to get IRQ from DT\n"));
 
 	sclk = of_clk_get(node, 0);
 	if (IS_ERR(sclk))
 		panic(pr_fmt("Unable to get slow clock\n"));
 
-	clk_prepare_enable(sclk);
+	ret = clk_prepare_enable(sclk);
 	if (ret)
 		panic(pr_fmt("Could not enable slow clock\n"));
 
-- 
2.5.0

[toc] | [next] | [standalone]

#1311704 — Re: [PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

* Alexandre Belloni | 2016-01-17 03:23:14 [+0100]:

>index 80d74c4adcbe..43b50634d640 100644
>--- a/drivers/clocksource/timer-atmel-pit.c
>+++ b/drivers/clocksource/timer-atmel-pit.c
>@@ -96,11 +96,44 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
> 
> 	/* disable irq, leaving the clocksource active */
> 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
>-	free_irq(atmel_pit_irq, data);
>+	if (!clockevent_state_detached(dev))
>+		free_irq(data->irq, data);

I did it in the meantime without clockevent_state_detached(). From what
it looks, it first sets the state and then invokes
pit_clkevt_shutdown(). Any particular reason for this?

> 	return 0;
> }
> 
> /*
>+ * IRQ handler for the timer.
>+ */
>+static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)

this is just here to avoid to forward declaration.
…
>diff --git a/drivers/clocksource/timer-atmel-st.c b/drivers/clocksource/timer-atmel-st.c
>index ea37afc26e1b..11ce404d0791 100644
>--- a/drivers/clocksource/timer-atmel-st.c
>+++ b/drivers/clocksource/timer-atmel-st.c
>@@ -229,15 +229,15 @@ static void __init atmel_st_timer_init(struct device_node *node)
> 	regmap_read(regmap_st, AT91_ST_SR, &val);
> 
> 	/* Get the interrupts property */
>-	irq  = irq_of_parse_and_map(node, 0);
>-	if (!irq)
>+	atmel_st_irq  = irq_of_parse_and_map(node, 0);
>+	if (!atmel_st_irq)
> 		panic(pr_fmt("Unable to get IRQ from DT\n"));
> 
> 	sclk = of_clk_get(node, 0);
> 	if (IS_ERR(sclk))
> 		panic(pr_fmt("Unable to get slow clock\n"));
> 
>-	clk_prepare_enable(sclk);
>+	ret = clk_prepare_enable(sclk);
this piece applies to upstream v4.4.

> 	if (ret)
> 		panic(pr_fmt("Could not enable slow clock\n"));
> 

Sebastian

[toc] | [prev] | [next] | [standalone]

#1311745 — Re: [PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

On 18/01/2016 at 18:25:22 +0100, Sebastian Andrzej Siewior wrote :
> * Alexandre Belloni | 2016-01-17 03:23:14 [+0100]:
> 
> >index 80d74c4adcbe..43b50634d640 100644
> >--- a/drivers/clocksource/timer-atmel-pit.c
> >+++ b/drivers/clocksource/timer-atmel-pit.c
> >@@ -96,11 +96,44 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
> > 
> > 	/* disable irq, leaving the clocksource active */
> > 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
> >-	free_irq(atmel_pit_irq, data);
> >+	if (!clockevent_state_detached(dev))
> >+		free_irq(data->irq, data);
> 
> I did it in the meantime without clockevent_state_detached(). From what
> it looks, it first sets the state and then invokes
> pit_clkevt_shutdown(). Any particular reason for this?
> 

Yeah, I forgot to mention that. Freeing the irq unconditionally
results in:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 0 at kernel/irq/manage.c:1541
__free_irq+0xb4/0x2c8()
Trying to free already-free IRQ 16
Modules linked in:
CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.4.0-rt2+ #31
Hardware name: Atmel SAMA5
[<c0016344>] (unwind_backtrace) from [<c0012d7c>] (show_stack+0x10/0x14)
[<c0012d7c>] (show_stack) from [<c021639c>] (dump_stack+0x80/0x94)
[<c021639c>] (dump_stack) from [<c001f528>] (warn_slowpath_common+0x80/0xb0)
[<c001f528>] (warn_slowpath_common) from [<c001f588>] (warn_slowpath_fmt+0x30/0x40)
[<c001f588>] (warn_slowpath_fmt) from [<c00615e0>] (__free_irq+0xb4/0x2c8)
[<c00615e0>] (__free_irq) from [<c0061878>] (free_irq+0x3c/0x70)
[<c0061878>] (free_irq) from [<c0391ba8>] (pit_clkevt_shutdown+0x24/0x2c)
[<c0391ba8>] (pit_clkevt_shutdown) from [<c007d9a0>] (clockevents_switch_state+0x60/0x130)
[<c007d9a0>] (clockevents_switch_state) from [<c007dda4>] (clockevents_exchange_device+0x78/0x8c)
[<c007dda4>] (clockevents_exchange_device) from [<c007e628>] (tick_check_new_device+0x90/0xd0)
[<c007e628>] (tick_check_new_device) from [<c007d488>] (clockevents_register_device+0x54/0x10c)
[<c007d488>] (clockevents_register_device) from [<c07073bc>] (clocksource_probe+0x4c/0x90)
[<c07073bc>] (clocksource_probe) from [<c06eab58>] (start_kernel+0x278/0x3a4)
[<c06eab58>] (start_kernel) from [<2000807c>] (0x2000807c)
---[ end trace 0000000000000001 ]---


My understanding is that clockevents_exchange_device() changes the state
from detached to shutdown and so at that point the IRQ has never been
requested.


> > 	return 0;
> > }
> > 
> > /*
> >+ * IRQ handler for the timer.
> >+ */
> >+static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
> 
> this is just here to avoid to forward declaration.
> …

Indeed.

> >diff --git a/drivers/clocksource/timer-atmel-st.c b/drivers/clocksource/timer-atmel-st.c
> >index ea37afc26e1b..11ce404d0791 100644
> >--- a/drivers/clocksource/timer-atmel-st.c
> >+++ b/drivers/clocksource/timer-atmel-st.c
> >@@ -229,15 +229,15 @@ static void __init atmel_st_timer_init(struct device_node *node)
> > 	regmap_read(regmap_st, AT91_ST_SR, &val);
> > 
> > 	/* Get the interrupts property */
> >-	irq  = irq_of_parse_and_map(node, 0);
> >-	if (!irq)
> >+	atmel_st_irq  = irq_of_parse_and_map(node, 0);
> >+	if (!atmel_st_irq)
> > 		panic(pr_fmt("Unable to get IRQ from DT\n"));
> > 
> > 	sclk = of_clk_get(node, 0);
> > 	if (IS_ERR(sclk))
> > 		panic(pr_fmt("Unable to get slow clock\n"));
> > 
> >-	clk_prepare_enable(sclk);
> >+	ret = clk_prepare_enable(sclk);
> this piece applies to upstream v4.4.
> 

Yeah, I'll submit it.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1311791 — Re: [PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

On 01/18/2016 07:42 PM, Alexandre Belloni wrote:
> On 18/01/2016 at 18:25:22 +0100, Sebastian Andrzej Siewior wrote :
>> * Alexandre Belloni | 2016-01-17 03:23:14 [+0100]:
>>
>>> index 80d74c4adcbe..43b50634d640 100644
>>> --- a/drivers/clocksource/timer-atmel-pit.c
>>> +++ b/drivers/clocksource/timer-atmel-pit.c
>>> @@ -96,11 +96,44 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
>>>
>>> 	/* disable irq, leaving the clocksource active */
>>> 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
>>> -	free_irq(atmel_pit_irq, data);
>>> +	if (!clockevent_state_detached(dev))
>>> +		free_irq(data->irq, data);
>>
>> I did it in the meantime without clockevent_state_detached(). From what
>> it looks, it first sets the state and then invokes
>> pit_clkevt_shutdown(). Any particular reason for this?
>>
> 
> Yeah, I forgot to mention that. Freeing the irq unconditionally
> results in:
…

> 
> 
> My understanding is that clockevents_exchange_device() changes the state
> from detached to shutdown and so at that point the IRQ has never been
> requested.

I see. So we get shutdown called twice while set_periodic was only
called once. In that case I would suggest to have internal bookkeeping
instead of relying on current core's behavior when it is time free the
irq.

> 

Sebastian

[toc] | [prev] | [next] | [standalone]

#1311882 — Re: [PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

On 18/01/2016 at 21:24:28 +0100, Sebastian Andrzej Siewior wrote :
> > 
> > My understanding is that clockevents_exchange_device() changes the state
> > from detached to shutdown and so at that point the IRQ has never been
> > requested.
> 
> I see. So we get shutdown called twice while set_periodic was only
> called once. In that case I would suggest to have internal bookkeeping
> instead of relying on current core's behavior when it is time free the
> irq.
> 

Ok, I can do that. What should I base my patch on?

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[toc] | [prev] | [next] | [standalone]

#1313124 — Re: [PATCH 4.4-rt2] fix arm-at91-pit-remove-irq-handler-when-clock-is-unused.patch

On Mon, 18 Jan 2016, Alexandre Belloni wrote:
> On 18/01/2016 at 18:25:22 +0100, Sebastian Andrzej Siewior wrote :
> > * Alexandre Belloni | 2016-01-17 03:23:14 [+0100]:
> > 
> > >index 80d74c4adcbe..43b50634d640 100644
> > >--- a/drivers/clocksource/timer-atmel-pit.c
> > >+++ b/drivers/clocksource/timer-atmel-pit.c
> > >@@ -96,11 +96,44 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
> > > 
> > > 	/* disable irq, leaving the clocksource active */
> > > 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
> > >-	free_irq(atmel_pit_irq, data);
> > >+	if (!clockevent_state_detached(dev))
> > >+		free_irq(data->irq, data);
> > 
> > I did it in the meantime without clockevent_state_detached(). From what
> > it looks, it first sets the state and then invokes
> > pit_clkevt_shutdown(). Any particular reason for this?
> > 
> 
> Yeah, I forgot to mention that. Freeing the irq unconditionally
> results in:

Well freeing the irq from that context in RT only works because its called
before SYSTEM_STATE=RUNNING. So no, this was wrong forever.

The issue we are dealing with is that the timer interrupt is shared with the
uart. So the timer has IRQ_NO_THREAD set and the uart interrupt gets force
threaded. So that results in a failure to request the interrupt for the
UART. That's not RT specific, that already happens in mainline if you add
'threadirqs' to the command line.

So until the DT folks come to senses and we get that dummy demux chip done, I
came up with the following - completely untested - solution.

The downside of this is, that the timer will be delayed until the uart thread
returns, but with the replacement clockevent in place on RT that's a non
issue. For mainline it's obviously better than what we have now.

Thanks,

	tglx

8<---------------

--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -61,6 +61,10 @@
  *                interrupt handler after suspending interrupts. For system
  *                wakeup devices users need to implement wakeup detection in
  *                their interrupt handlers.
+ * IRQF_COND_ONESHOT - If the IRQ is shared between a NO_THREAD user and a
+ *		regular interrupt, force the ONESHOT flag on the NO_THREAD user
+ *		when threaded irqs are enforced. Workaround for silly ATMEL
+ *		SoCs which share the timer and the UART interrupt
  * IRQF_NO_SOFTIRQ_CALL - Do not process softirqs in the irq thread context (RT)
  */
 #define IRQF_SHARED		0x00000080
@@ -75,7 +79,8 @@
 #define IRQF_NO_THREAD		0x00010000
 #define IRQF_EARLY_RESUME	0x00020000
 #define IRQF_COND_SUSPEND	0x00040000
-#define IRQF_NO_SOFTIRQ_CALL	0x00080000
+#define IRQF_COND_ONESHOT	0x00080000
+#define IRQF_NO_SOFTIRQ_CALL	0x00100000
 
 #define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND | IRQF_NO_THREAD)
 
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1208,6 +1208,14 @@ static int
 	new->irq = irq;
 
 	/*
+	 * Workaround for silly ATMEL SoCs with shared timer and uart
+	 * interrupt.
+	 */
+	if (force_irqthreads && (new->flags & IRQF_COND_ONESHOT) &&
+	    (new->flags & IRQF_NO_THREAD))
+		new->flags |= IRQF_ONESHOT;
+
+	/*
 	 * Check whether the interrupt nests into another interrupt
 	 * thread.
 	 */
--- a/drivers/clocksource/timer-atmel-pit.c
+++ b/drivers/clocksource/timer-atmel-pit.c
@@ -208,7 +208,7 @@ static void __init at91sam926x_pit_commo
 
 	/* Set up irq handler */
 	ret = request_irq(data->irq, at91sam926x_pit_interrupt,
-			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
+			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL| IRQF_COND_ONESHOT,
 			  "at91_tick", data);
 	if (ret)
 		panic(pr_fmt("Unable to setup IRQ\n"));
--- a/drivers/clocksource/timer-atmel-st.c
+++ b/drivers/clocksource/timer-atmel-st.c
@@ -216,7 +216,7 @@ static void __init atmel_st_timer_init(s
 
 	/* Make IRQs happen for the system timer */
 	ret = request_irq(irq, at91rm9200_timer_interrupt,
-			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
+			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL | IRQF_COND_ONESHOT,
 			  "at91_tick", regmap_st);
 	if (ret)
 		panic(pr_fmt("Unable to setup IRQ\n"));

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web