[PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Uwe Kleine-König]

These functions are called with irqs already off.

at91rm2000 had a WARN_ON_ONCE if irqs were enabled since Nov 2008 with
noone reporting having hit it.

It should be safe to remove now.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Alessandro Rubini <rubini@unipv.it>
Cc: Andrea Gallo <andrea.gallo@stericsson.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Eric Miao <eric.miao@marvell.com>
Cc: Hugh Dickins <hugh@veritas.com>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Kristoffer Ericson <Kristoffer.Ericson@gmail.com>
Cc: Magnus Damm <damm@igel.co.jp>
Cc: Nicolas Pitre <nico@marvell.com>
Cc: Remy Bohmer <linux@bohmer.net>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Rusty Russell <rusty@rustcorp.com.au>
---
Hello,

should I split the patch?  Would you prefer to add a WARN_ON_ONCE for
some time (as at91rm9200 had it)?

Best regards
Uwe

 arch/arm/mach-at91/at91rm9200_time.c  |   14 --------------
 arch/arm/mach-at91/at91sam926x_time.c |    6 +-----
 arch/arm/mach-nomadik/timer.c         |    9 +--------
 arch/arm/mach-pxa/time.c              |   10 +---------
 arch/arm/mach-sa1100/time.c           |    8 +-------
 5 files changed, 4 insertions(+), 43 deletions(-)

diff --git a/arch/arm/mach-at91/at91rm9200_time.c b/arch/arm/mach-at91/at91rm9200_time.c
index 309f351..f27ccf6 100644
--- a/arch/arm/mach-at91/at91rm9200_time.c
+++ b/arch/arm/mach-at91/at91rm9200_time.c
@@ -132,24 +132,11 @@ clkevt32k_mode(enum clock_event_mode mode, struct clock_event_device *dev)
 static int
 clkevt32k_next_event(unsigned long delta, struct clock_event_device *dev)
 {
-	unsigned long	flags;
 	u32		alm;
 	int		status = 0;
 
 	BUG_ON(delta < 2);
 
-	/* Use "raw" primitives so we behave correctly on RT kernels. */
-	raw_local_irq_save(flags);
-
-	/*
-	 * According to Thomas Gleixner irqs are already disabled here.  Simply
-	 * removing raw_local_irq_save above (and the matching
-	 * raw_local_irq_restore) was not accepted.  See
-	 * http://thread.gmane.org/gmane.linux.ports.arm.kernel/41174
-	 * So for now (2008-11-20) just warn once if irqs were not disabled ...
-	 */
-	WARN_ON_ONCE(!raw_irqs_disabled_flags(flags));
-
 	/* The alarm IRQ uses absolute time (now+delta), not the relative
 	 * time (delta) in our calling convention.  Like all clockevents
 	 * using such "match" hardware, we have a race to defend against.
@@ -169,7 +156,6 @@ clkevt32k_next_event(unsigned long delta, struct clock_event_device *dev)
 	alm += delta;
 	at91_sys_write(AT91_ST_RTAR, alm);
 
-	raw_local_irq_restore(flags);
 	return status;
 }
 
diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
index 4bd56ae..c26cd6e 100644
--- a/arch/arm/mach-at91/at91sam926x_time.c
+++ b/arch/arm/mach-at91/at91sam926x_time.c
@@ -62,16 +62,12 @@ static struct clocksource pit_clk = {
 static void
 pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev)
 {
-	unsigned long	flags;
-
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
-		/* update clocksource counter, then enable the IRQ */
-		raw_local_irq_save(flags);
+		/* update clocksource counter */
 		pit_cnt += pit_cycle * PIT_PICNT(at91_sys_read(AT91_PIT_PIVR));
 		at91_sys_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN
 				| AT91_PIT_PITIEN);
-		raw_local_irq_restore(flags);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
 		BUG();
diff --git a/arch/arm/mach-nomadik/timer.c b/arch/arm/mach-nomadik/timer.c
index d1738e7..e361be6 100644
--- a/arch/arm/mach-nomadik/timer.c
+++ b/arch/arm/mach-nomadik/timer.c
@@ -54,24 +54,17 @@ static struct clocksource nmdk_clksrc = {
 static void nmdk_clkevt_mode(enum clock_event_mode mode,
 			     struct clock_event_device *dev)
 {
-	unsigned long flags;
-
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
-		/* enable interrupts -- and count current value? */
-		raw_local_irq_save(flags);
+		/* count current value? */
 		writel(readl(mtu_base + MTU_IMSC) | 1, mtu_base + MTU_IMSC);
-		raw_local_irq_restore(flags);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
 		BUG(); /* Not supported, yet */
 		/* FALLTHROUGH */
 	case CLOCK_EVT_MODE_SHUTDOWN:
 	case CLOCK_EVT_MODE_UNUSED:
-		/* disable irq */
-		raw_local_irq_save(flags);
 		writel(readl(mtu_base + MTU_IMSC) & ~1, mtu_base + MTU_IMSC);
-		raw_local_irq_restore(flags);
 		break;
 	case CLOCK_EVT_MODE_RESUME:
 		break;
diff --git a/arch/arm/mach-pxa/time.c b/arch/arm/mach-pxa/time.c
index 750c448..293e40a 100644
--- a/arch/arm/mach-pxa/time.c
+++ b/arch/arm/mach-pxa/time.c
@@ -76,14 +76,12 @@ pxa_ost0_interrupt(int irq, void *dev_id)
 static int
 pxa_osmr0_set_next_event(unsigned long delta, struct clock_event_device *dev)
 {
-	unsigned long flags, next, oscr;
+	unsigned long next, oscr;
 
-	raw_local_irq_save(flags);
 	OIER |= OIER_E0;
 	next = OSCR + delta;
 	OSMR0 = next;
 	oscr = OSCR;
-	raw_local_irq_restore(flags);
 
 	return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0;
 }
@@ -91,23 +89,17 @@ pxa_osmr0_set_next_event(unsigned long delta, struct clock_event_device *dev)
 static void
 pxa_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *dev)
 {
-	unsigned long irqflags;
-
 	switch (mode) {
 	case CLOCK_EVT_MODE_ONESHOT:
-		raw_local_irq_save(irqflags);
 		OIER &= ~OIER_E0;
 		OSSR = OSSR_M0;
-		raw_local_irq_restore(irqflags);
 		break;
 
 	case CLOCK_EVT_MODE_UNUSED:
 	case CLOCK_EVT_MODE_SHUTDOWN:
 		/* initializing, released, or preparing for suspend */
-		raw_local_irq_save(irqflags);
 		OIER &= ~OIER_E0;
 		OSSR = OSSR_M0;
-		raw_local_irq_restore(irqflags);
 		break;
 
 	case CLOCK_EVT_MODE_RESUME:
diff --git a/arch/arm/mach-sa1100/time.c b/arch/arm/mach-sa1100/time.c
index 95d92e8..a7a6091 100644
--- a/arch/arm/mach-sa1100/time.c
+++ b/arch/arm/mach-sa1100/time.c
@@ -35,14 +35,12 @@ static irqreturn_t sa1100_ost0_interrupt(int irq, void *dev_id)
 static int
 sa1100_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c)
 {
-	unsigned long flags, next, oscr;
+	unsigned long next, oscr;
 
-	raw_local_irq_save(flags);
 	OIER |= OIER_E0;
 	next = OSCR + delta;
 	OSMR0 = next;
 	oscr = OSCR;
-	raw_local_irq_restore(flags);
 
 	return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0;
 }
@@ -50,16 +48,12 @@ sa1100_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c)
 static void
 sa1100_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c)
 {
-	unsigned long flags;
-
 	switch (mode) {
 	case CLOCK_EVT_MODE_ONESHOT:
 	case CLOCK_EVT_MODE_UNUSED:
 	case CLOCK_EVT_MODE_SHUTDOWN:
-		raw_local_irq_save(flags);
 		OIER &= ~OIER_E0;
 		OSSR = OSSR_M0;
-		raw_local_irq_restore(flags);
 		break;
 
 	case CLOCK_EVT_MODE_RESUME:
-- 
1.6.4.3

[PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Russell King - ARM Linux]

On Mon, Sep 21, 2009 at 09:39:22AM +0200, Uwe Kleine-K?nig wrote:
> These functions are called with irqs already off.
> 
> at91rm2000 had a WARN_ON_ONCE if irqs were enabled since Nov 2008 with
> noone reporting having hit it.

It might be useful to document these clockevent interfaces.  There's
at least a few ARM clockevent implementations which don't set the
periodic interval when set_next_event() is called - probably because
it wasn't realised that it was required.

[PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Uwe Kleine-König]

On Mon, Sep 21, 2009 at 10:04:00AM +0100, Russell King - ARM Linux wrote:
> On Mon, Sep 21, 2009 at 09:39:22AM +0200, Uwe Kleine-K?nig wrote:
> > These functions are called with irqs already off.
> > 
> > at91rm2000 had a WARN_ON_ONCE if irqs were enabled since Nov 2008 with
> > noone reporting having hit it.
> 
> It might be useful to document these clockevent interfaces.  There's
> at least a few ARM clockevent implementations which don't set the
> periodic interval when set_next_event() is called - probably because
> it wasn't realised that it was required.
I'm a bit confused now as AFAIK set_next_event isn't called in periodic
mode.

You mean they don't start the timer if mode==CLOCK_EVT_MODE_PERIODIC in
.set_mode?
It looks as if pxa is such an implementation, it does nothing in this
case.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Russell King - ARM Linux]

On Mon, Sep 21, 2009 at 11:16:14AM +0200, Uwe Kleine-K?nig wrote:
> On Mon, Sep 21, 2009 at 10:04:00AM +0100, Russell King - ARM Linux wrote:
> > On Mon, Sep 21, 2009 at 09:39:22AM +0200, Uwe Kleine-K?nig wrote:
> > > These functions are called with irqs already off.
> > > 
> > > at91rm2000 had a WARN_ON_ONCE if irqs were enabled since Nov 2008 with
> > > noone reporting having hit it.
> > 
> > It might be useful to document these clockevent interfaces.  There's
> > at least a few ARM clockevent implementations which don't set the
> > periodic interval when set_next_event() is called - probably because
> > it wasn't realised that it was required.
> I'm a bit confused now as AFAIK set_next_event isn't called in periodic
> mode.
> 
> You mean they don't start the timer if mode==CLOCK_EVT_MODE_PERIODIC in
> .set_mode?
> It looks as if pxa is such an implementation, it does nothing in this
> case.

PXA doesn't support periodic mode - the hardware is incapable of providing
periodic interrupts without software support, so we only support it in
one-shot mode (that's actually what the hardware provides.)

I'm talking about this kind of thing:

        case CLOCK_EVT_MODE_PERIODIC:
                /* timer load already set up */
                ctrl = TWD_TIMER_CONTROL_ENABLE | TWD_TIMER_CONTROL_IT_ENABLE
                        | TWD_TIMER_CONTROL_PERIODIC;
                break;
        case CLOCK_EVT_MODE_ONESHOT:
                /* period set, and timer enabled in 'next_event' hook */
                ctrl = TWD_TIMER_CONTROL_IT_ENABLE | TWD_TIMER_CONTROL_ONESHOT;
                break;

The first comment is not true if we ever switch to oneshot mode - the
reload register is overwritten.  If we then switch back to periodic
mode, we'll interrupt at whatever periodic rate which was programmed
by the oneshot code.  OMAP mpu_timer1 looks to be the same.

Nomadik only works because it doesn't support periodic mode.

[PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Alessandro Rubini]

> Nomadik only works because it doesn't support periodic mode.

s/periodic/one-shot/

Uwe, I can't test it as my board is currently unavailable, but I have
no problems in removing the irqsave stuff if not needed. Actually,
I use the atmel code as a reference, so I got the irqsave from there.

Feel free to add my acked-by if you think it's worth.

/alessandro

[PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Kristoffer Ericson]

Dont see the harm (will do a formal test later today), so feel
free to add acked-by.

/Kristoffer

On Mon, 21 Sep 2009 09:39:22 +0200
Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> wrote:

> These functions are called with irqs already off.
> 
> at91rm2000 had a WARN_ON_ONCE if irqs were enabled since Nov 2008 with
> noone reporting having hit it.
> 
> It should be safe to remove now.
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Alessandro Rubini <rubini@unipv.it>
> Cc: Andrea Gallo <andrea.gallo@stericsson.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Eric Miao <eric.miao@marvell.com>
> Cc: Hugh Dickins <hugh@veritas.com>
> Cc: John Stultz <johnstul@us.ibm.com>
> Cc: Kristoffer Ericson <Kristoffer.Ericson@gmail.com>
> Cc: Magnus Damm <damm@igel.co.jp>
> Cc: Nicolas Pitre <nico@marvell.com>
> Cc: Remy Bohmer <linux@bohmer.net>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> ---
> Hello,
> 
> should I split the patch?  Would you prefer to add a WARN_ON_ONCE for
> some time (as at91rm9200 had it)?
> 
> Best regards
> Uwe
> 
>  arch/arm/mach-at91/at91rm9200_time.c  |   14 --------------
>  arch/arm/mach-at91/at91sam926x_time.c |    6 +-----
>  arch/arm/mach-nomadik/timer.c         |    9 +--------
>  arch/arm/mach-pxa/time.c              |   10 +---------
>  arch/arm/mach-sa1100/time.c           |    8 +-------
>  5 files changed, 4 insertions(+), 43 deletions(-)
> 
> diff --git a/arch/arm/mach-at91/at91rm9200_time.c b/arch/arm/mach-at91/at91rm9200_time.c
> index 309f351..f27ccf6 100644
> --- a/arch/arm/mach-at91/at91rm9200_time.c
> +++ b/arch/arm/mach-at91/at91rm9200_time.c
> @@ -132,24 +132,11 @@ clkevt32k_mode(enum clock_event_mode mode, struct clock_event_device *dev)
>  static int
>  clkevt32k_next_event(unsigned long delta, struct clock_event_device *dev)
>  {
> -	unsigned long	flags;
>  	u32		alm;
>  	int		status = 0;
>  
>  	BUG_ON(delta < 2);
>  
> -	/* Use "raw" primitives so we behave correctly on RT kernels. */
> -	raw_local_irq_save(flags);
> -
> -	/*
> -	 * According to Thomas Gleixner irqs are already disabled here.  Simply
> -	 * removing raw_local_irq_save above (and the matching
> -	 * raw_local_irq_restore) was not accepted.  See
> -	 * http://thread.gmane.org/gmane.linux.ports.arm.kernel/41174
> -	 * So for now (2008-11-20) just warn once if irqs were not disabled ...
> -	 */
> -	WARN_ON_ONCE(!raw_irqs_disabled_flags(flags));
> -
>  	/* The alarm IRQ uses absolute time (now+delta), not the relative
>  	 * time (delta) in our calling convention.  Like all clockevents
>  	 * using such "match" hardware, we have a race to defend against.
> @@ -169,7 +156,6 @@ clkevt32k_next_event(unsigned long delta, struct clock_event_device *dev)
>  	alm += delta;
>  	at91_sys_write(AT91_ST_RTAR, alm);
>  
> -	raw_local_irq_restore(flags);
>  	return status;
>  }
>  
> diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
> index 4bd56ae..c26cd6e 100644
> --- a/arch/arm/mach-at91/at91sam926x_time.c
> +++ b/arch/arm/mach-at91/at91sam926x_time.c
> @@ -62,16 +62,12 @@ static struct clocksource pit_clk = {
>  static void
>  pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev)
>  {
> -	unsigned long	flags;
> -
>  	switch (mode) {
>  	case CLOCK_EVT_MODE_PERIODIC:
> -		/* update clocksource counter, then enable the IRQ */
> -		raw_local_irq_save(flags);
> +		/* update clocksource counter */
>  		pit_cnt += pit_cycle * PIT_PICNT(at91_sys_read(AT91_PIT_PIVR));
>  		at91_sys_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN
>  				| AT91_PIT_PITIEN);
> -		raw_local_irq_restore(flags);
>  		break;
>  	case CLOCK_EVT_MODE_ONESHOT:
>  		BUG();
> diff --git a/arch/arm/mach-nomadik/timer.c b/arch/arm/mach-nomadik/timer.c
> index d1738e7..e361be6 100644
> --- a/arch/arm/mach-nomadik/timer.c
> +++ b/arch/arm/mach-nomadik/timer.c
> @@ -54,24 +54,17 @@ static struct clocksource nmdk_clksrc = {
>  static void nmdk_clkevt_mode(enum clock_event_mode mode,
>  			     struct clock_event_device *dev)
>  {
> -	unsigned long flags;
> -
>  	switch (mode) {
>  	case CLOCK_EVT_MODE_PERIODIC:
> -		/* enable interrupts -- and count current value? */
> -		raw_local_irq_save(flags);
> +		/* count current value? */
>  		writel(readl(mtu_base + MTU_IMSC) | 1, mtu_base + MTU_IMSC);
> -		raw_local_irq_restore(flags);
>  		break;
>  	case CLOCK_EVT_MODE_ONESHOT:
>  		BUG(); /* Not supported, yet */
>  		/* FALLTHROUGH */
>  	case CLOCK_EVT_MODE_SHUTDOWN:
>  	case CLOCK_EVT_MODE_UNUSED:
> -		/* disable irq */
> -		raw_local_irq_save(flags);
>  		writel(readl(mtu_base + MTU_IMSC) & ~1, mtu_base + MTU_IMSC);
> -		raw_local_irq_restore(flags);
>  		break;
>  	case CLOCK_EVT_MODE_RESUME:
>  		break;
> diff --git a/arch/arm/mach-pxa/time.c b/arch/arm/mach-pxa/time.c
> index 750c448..293e40a 100644
> --- a/arch/arm/mach-pxa/time.c
> +++ b/arch/arm/mach-pxa/time.c
> @@ -76,14 +76,12 @@ pxa_ost0_interrupt(int irq, void *dev_id)
>  static int
>  pxa_osmr0_set_next_event(unsigned long delta, struct clock_event_device *dev)
>  {
> -	unsigned long flags, next, oscr;
> +	unsigned long next, oscr;
>  
> -	raw_local_irq_save(flags);
>  	OIER |= OIER_E0;
>  	next = OSCR + delta;
>  	OSMR0 = next;
>  	oscr = OSCR;
> -	raw_local_irq_restore(flags);
>  
>  	return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0;
>  }
> @@ -91,23 +89,17 @@ pxa_osmr0_set_next_event(unsigned long delta, struct clock_event_device *dev)
>  static void
>  pxa_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *dev)
>  {
> -	unsigned long irqflags;
> -
>  	switch (mode) {
>  	case CLOCK_EVT_MODE_ONESHOT:
> -		raw_local_irq_save(irqflags);
>  		OIER &= ~OIER_E0;
>  		OSSR = OSSR_M0;
> -		raw_local_irq_restore(irqflags);
>  		break;
>  
>  	case CLOCK_EVT_MODE_UNUSED:
>  	case CLOCK_EVT_MODE_SHUTDOWN:
>  		/* initializing, released, or preparing for suspend */
> -		raw_local_irq_save(irqflags);
>  		OIER &= ~OIER_E0;
>  		OSSR = OSSR_M0;
> -		raw_local_irq_restore(irqflags);
>  		break;
>  
>  	case CLOCK_EVT_MODE_RESUME:
> diff --git a/arch/arm/mach-sa1100/time.c b/arch/arm/mach-sa1100/time.c
> index 95d92e8..a7a6091 100644
> --- a/arch/arm/mach-sa1100/time.c
> +++ b/arch/arm/mach-sa1100/time.c
> @@ -35,14 +35,12 @@ static irqreturn_t sa1100_ost0_interrupt(int irq, void *dev_id)
>  static int
>  sa1100_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c)
>  {
> -	unsigned long flags, next, oscr;
> +	unsigned long next, oscr;
>  
> -	raw_local_irq_save(flags);
>  	OIER |= OIER_E0;
>  	next = OSCR + delta;
>  	OSMR0 = next;
>  	oscr = OSCR;
> -	raw_local_irq_restore(flags);
>  
>  	return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0;
>  }
> @@ -50,16 +48,12 @@ sa1100_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c)
>  static void
>  sa1100_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c)
>  {
> -	unsigned long flags;
> -
>  	switch (mode) {
>  	case CLOCK_EVT_MODE_ONESHOT:
>  	case CLOCK_EVT_MODE_UNUSED:
>  	case CLOCK_EVT_MODE_SHUTDOWN:
> -		raw_local_irq_save(flags);
>  		OIER &= ~OIER_E0;
>  		OSSR = OSSR_M0;
> -		raw_local_irq_restore(flags);
>  		break;
>  
>  	case CLOCK_EVT_MODE_RESUME:
> -- 
> 1.6.4.3
> 


-- 
Kristoffer Ericson <kristoffer.ericson@gmail.com>

[PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Eric Miao]

Don't have time go through all the calling sequences here, and
not sure if there are corner cases that these will be called
without IRQ disabled.

2009/9/21 Kristoffer Ericson <kristoffer.ericson@gmail.com>:
> Dont see the harm (will do a formal test later today), so feel
> free to add acked-by.
>
> /Kristoffer
>
> On Mon, 21 Sep 2009 09:39:22 +0200
> Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de> wrote:
>
>> These functions are called with irqs already off.
>>
>> at91rm2000 had a WARN_ON_ONCE if irqs were enabled since Nov 2008 with
>> noone reporting having hit it.
>>
>> It should be safe to remove now.
>>
>> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Alessandro Rubini <rubini@unipv.it>
>> Cc: Andrea Gallo <andrea.gallo@stericsson.com>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Eric Miao <eric.miao@marvell.com>
>> Cc: Hugh Dickins <hugh@veritas.com>
>> Cc: John Stultz <johnstul@us.ibm.com>
>> Cc: Kristoffer Ericson <Kristoffer.Ericson@gmail.com>
>> Cc: Magnus Damm <damm@igel.co.jp>
>> Cc: Nicolas Pitre <nico@marvell.com>
>> Cc: Remy Bohmer <linux@bohmer.net>
>> Cc: Russell King <linux@arm.linux.org.uk>
>> Cc: Rusty Russell <rusty@rustcorp.com.au>
>> ---
>> Hello,
>>
>> should I split the patch? ?Would you prefer to add a WARN_ON_ONCE for
>> some time (as at91rm9200 had it)?
>>
>> Best regards
>> Uwe
>>
>> ?arch/arm/mach-at91/at91rm9200_time.c ?| ? 14 --------------
>> ?arch/arm/mach-at91/at91sam926x_time.c | ? ?6 +-----
>> ?arch/arm/mach-nomadik/timer.c ? ? ? ? | ? ?9 +--------
>> ?arch/arm/mach-pxa/time.c ? ? ? ? ? ? ?| ? 10 +---------
>> ?arch/arm/mach-sa1100/time.c ? ? ? ? ? | ? ?8 +-------
>> ?5 files changed, 4 insertions(+), 43 deletions(-)
>>
>> diff --git a/arch/arm/mach-at91/at91rm9200_time.c b/arch/arm/mach-at91/at91rm9200_time.c
>> index 309f351..f27ccf6 100644
>> --- a/arch/arm/mach-at91/at91rm9200_time.c
>> +++ b/arch/arm/mach-at91/at91rm9200_time.c
>> @@ -132,24 +132,11 @@ clkevt32k_mode(enum clock_event_mode mode, struct clock_event_device *dev)
>> ?static int
>> ?clkevt32k_next_event(unsigned long delta, struct clock_event_device *dev)
>> ?{
>> - ? ? unsigned long ? flags;
>> ? ? ? u32 ? ? ? ? ? ? alm;
>> ? ? ? int ? ? ? ? ? ? status = 0;
>>
>> ? ? ? BUG_ON(delta < 2);
>>
>> - ? ? /* Use "raw" primitives so we behave correctly on RT kernels. */
>> - ? ? raw_local_irq_save(flags);
>> -
>> - ? ? /*
>> - ? ? ?* According to Thomas Gleixner irqs are already disabled here. ?Simply
>> - ? ? ?* removing raw_local_irq_save above (and the matching
>> - ? ? ?* raw_local_irq_restore) was not accepted. ?See
>> - ? ? ?* http://thread.gmane.org/gmane.linux.ports.arm.kernel/41174
>> - ? ? ?* So for now (2008-11-20) just warn once if irqs were not disabled ...
>> - ? ? ?*/
>> - ? ? WARN_ON_ONCE(!raw_irqs_disabled_flags(flags));
>> -
>> ? ? ? /* The alarm IRQ uses absolute time (now+delta), not the relative
>> ? ? ? ?* time (delta) in our calling convention. ?Like all clockevents
>> ? ? ? ?* using such "match" hardware, we have a race to defend against.
>> @@ -169,7 +156,6 @@ clkevt32k_next_event(unsigned long delta, struct clock_event_device *dev)
>> ? ? ? alm += delta;
>> ? ? ? at91_sys_write(AT91_ST_RTAR, alm);
>>
>> - ? ? raw_local_irq_restore(flags);
>> ? ? ? return status;
>> ?}
>>
>> diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
>> index 4bd56ae..c26cd6e 100644
>> --- a/arch/arm/mach-at91/at91sam926x_time.c
>> +++ b/arch/arm/mach-at91/at91sam926x_time.c
>> @@ -62,16 +62,12 @@ static struct clocksource pit_clk = {
>> ?static void
>> ?pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev)
>> ?{
>> - ? ? unsigned long ? flags;
>> -
>> ? ? ? switch (mode) {
>> ? ? ? case CLOCK_EVT_MODE_PERIODIC:
>> - ? ? ? ? ? ? /* update clocksource counter, then enable the IRQ */
>> - ? ? ? ? ? ? raw_local_irq_save(flags);
>> + ? ? ? ? ? ? /* update clocksource counter */
>> ? ? ? ? ? ? ? pit_cnt += pit_cycle * PIT_PICNT(at91_sys_read(AT91_PIT_PIVR));
>> ? ? ? ? ? ? ? at91_sys_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? | AT91_PIT_PITIEN);
>> - ? ? ? ? ? ? raw_local_irq_restore(flags);
>> ? ? ? ? ? ? ? break;
>> ? ? ? case CLOCK_EVT_MODE_ONESHOT:
>> ? ? ? ? ? ? ? BUG();
>> diff --git a/arch/arm/mach-nomadik/timer.c b/arch/arm/mach-nomadik/timer.c
>> index d1738e7..e361be6 100644
>> --- a/arch/arm/mach-nomadik/timer.c
>> +++ b/arch/arm/mach-nomadik/timer.c
>> @@ -54,24 +54,17 @@ static struct clocksource nmdk_clksrc = {
>> ?static void nmdk_clkevt_mode(enum clock_event_mode mode,
>> ? ? ? ? ? ? ? ? ? ? ? ? ? ?struct clock_event_device *dev)
>> ?{
>> - ? ? unsigned long flags;
>> -
>> ? ? ? switch (mode) {
>> ? ? ? case CLOCK_EVT_MODE_PERIODIC:
>> - ? ? ? ? ? ? /* enable interrupts -- and count current value? */
>> - ? ? ? ? ? ? raw_local_irq_save(flags);
>> + ? ? ? ? ? ? /* count current value? */
>> ? ? ? ? ? ? ? writel(readl(mtu_base + MTU_IMSC) | 1, mtu_base + MTU_IMSC);
>> - ? ? ? ? ? ? raw_local_irq_restore(flags);
>> ? ? ? ? ? ? ? break;
>> ? ? ? case CLOCK_EVT_MODE_ONESHOT:
>> ? ? ? ? ? ? ? BUG(); /* Not supported, yet */
>> ? ? ? ? ? ? ? /* FALLTHROUGH */
>> ? ? ? case CLOCK_EVT_MODE_SHUTDOWN:
>> ? ? ? case CLOCK_EVT_MODE_UNUSED:
>> - ? ? ? ? ? ? /* disable irq */
>> - ? ? ? ? ? ? raw_local_irq_save(flags);
>> ? ? ? ? ? ? ? writel(readl(mtu_base + MTU_IMSC) & ~1, mtu_base + MTU_IMSC);
>> - ? ? ? ? ? ? raw_local_irq_restore(flags);
>> ? ? ? ? ? ? ? break;
>> ? ? ? case CLOCK_EVT_MODE_RESUME:
>> ? ? ? ? ? ? ? break;
>> diff --git a/arch/arm/mach-pxa/time.c b/arch/arm/mach-pxa/time.c
>> index 750c448..293e40a 100644
>> --- a/arch/arm/mach-pxa/time.c
>> +++ b/arch/arm/mach-pxa/time.c
>> @@ -76,14 +76,12 @@ pxa_ost0_interrupt(int irq, void *dev_id)
>> ?static int
>> ?pxa_osmr0_set_next_event(unsigned long delta, struct clock_event_device *dev)
>> ?{
>> - ? ? unsigned long flags, next, oscr;
>> + ? ? unsigned long next, oscr;
>>
>> - ? ? raw_local_irq_save(flags);
>> ? ? ? OIER |= OIER_E0;
>> ? ? ? next = OSCR + delta;
>> ? ? ? OSMR0 = next;
>> ? ? ? oscr = OSCR;
>> - ? ? raw_local_irq_restore(flags);
>>
>> ? ? ? return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0;
>> ?}
>> @@ -91,23 +89,17 @@ pxa_osmr0_set_next_event(unsigned long delta, struct clock_event_device *dev)
>> ?static void
>> ?pxa_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *dev)
>> ?{
>> - ? ? unsigned long irqflags;
>> -
>> ? ? ? switch (mode) {
>> ? ? ? case CLOCK_EVT_MODE_ONESHOT:
>> - ? ? ? ? ? ? raw_local_irq_save(irqflags);
>> ? ? ? ? ? ? ? OIER &= ~OIER_E0;
>> ? ? ? ? ? ? ? OSSR = OSSR_M0;
>> - ? ? ? ? ? ? raw_local_irq_restore(irqflags);
>> ? ? ? ? ? ? ? break;
>>
>> ? ? ? case CLOCK_EVT_MODE_UNUSED:
>> ? ? ? case CLOCK_EVT_MODE_SHUTDOWN:
>> ? ? ? ? ? ? ? /* initializing, released, or preparing for suspend */
>> - ? ? ? ? ? ? raw_local_irq_save(irqflags);
>> ? ? ? ? ? ? ? OIER &= ~OIER_E0;
>> ? ? ? ? ? ? ? OSSR = OSSR_M0;
>> - ? ? ? ? ? ? raw_local_irq_restore(irqflags);
>> ? ? ? ? ? ? ? break;
>>
>> ? ? ? case CLOCK_EVT_MODE_RESUME:
>> diff --git a/arch/arm/mach-sa1100/time.c b/arch/arm/mach-sa1100/time.c
>> index 95d92e8..a7a6091 100644
>> --- a/arch/arm/mach-sa1100/time.c
>> +++ b/arch/arm/mach-sa1100/time.c
>> @@ -35,14 +35,12 @@ static irqreturn_t sa1100_ost0_interrupt(int irq, void *dev_id)
>> ?static int
>> ?sa1100_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c)
>> ?{
>> - ? ? unsigned long flags, next, oscr;
>> + ? ? unsigned long next, oscr;
>>
>> - ? ? raw_local_irq_save(flags);
>> ? ? ? OIER |= OIER_E0;
>> ? ? ? next = OSCR + delta;
>> ? ? ? OSMR0 = next;
>> ? ? ? oscr = OSCR;
>> - ? ? raw_local_irq_restore(flags);
>>
>> ? ? ? return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0;
>> ?}
>> @@ -50,16 +48,12 @@ sa1100_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c)
>> ?static void
>> ?sa1100_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c)
>> ?{
>> - ? ? unsigned long flags;
>> -
>> ? ? ? switch (mode) {
>> ? ? ? case CLOCK_EVT_MODE_ONESHOT:
>> ? ? ? case CLOCK_EVT_MODE_UNUSED:
>> ? ? ? case CLOCK_EVT_MODE_SHUTDOWN:
>> - ? ? ? ? ? ? raw_local_irq_save(flags);
>> ? ? ? ? ? ? ? OIER &= ~OIER_E0;
>> ? ? ? ? ? ? ? OSSR = OSSR_M0;
>> - ? ? ? ? ? ? raw_local_irq_restore(flags);
>> ? ? ? ? ? ? ? break;
>>
>> ? ? ? case CLOCK_EVT_MODE_RESUME:
>> --
>> 1.6.4.3
>>
>
>
> --
> Kristoffer Ericson <kristoffer.ericson@gmail.com>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

[PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Remy Bohmer]

Hi,

2009/9/21 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> These functions are called with irqs already off.
>
> at91rm2000 had a WARN_ON_ONCE if irqs were enabled since Nov 2008 with
> noone reporting having hit it.

I guess this is not a processor widely used these days with recent
kernels, it has been deprecated by Atmel a couple of years ago.
Besides that, there is also a TC-library clocksource/event
implementation inside the kernel that runs at a much higher frequency
compared to the 32kHz the PIT is running on. It is likely that someone
who cares about accurate timestamps would use the TC-lib
implementation on this processor. So, it would have surprised me if
anybody would actually hit this WARN_ON_ONCE...

> It should be safe to remove now.

But anyway, if the problem we encountered back then would still be
here, many more boards/architectures would suffer from the same
problem. So, I guess it is safe to remove the irq_disable now.
But to be completely sure; I will test this patch on 2.6.31-rt11 with
the sam9261 processor once I have it running stable.(I expect Friday
and stress it during the weekend. Will inform you about the results on
Monday)

> should I split the patch?  Would you prefer to add a WARN_ON_ONCE for
> some time (as at91rm9200 had it)?

I expect this is not needed...


Kind Regards,

Remy

[RESENT PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Uwe Kleine-König]

These functions are called with irqs already off.

AT91RM2000 had a WARN_ON_ONCE if irqs were enabled since Nov 2008 with
noone reporting having hit it.

It should be safe to remove now.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Cc: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Alessandro Rubini <rubini@unipv.it>
Cc: Andrea Gallo <andrea.gallo@stericsson.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Eric Miao <eric.y.miao@gmail.com>
Cc: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Cc: John Stultz <johnstul@us.ibm.com>
Acked-by: Kristoffer Ericson <Kristoffer.Ericson@gmail.com>
Cc: Magnus Damm <damm@igel.co.jp>
Cc: Nicolas Pitre <nico@marvell.com>
Cc: Remy Bohmer <linux@bohmer.net>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Rusty Russell <rusty@rustcorp.com.au>
---
Hello,

@Eric: You wrote you were not sure about this patch.  Would you prefer adding a
WARN_ON in a first step?

@Remy: You planned to test this back in September.  Did you?

Best regards
Uwe

 arch/arm/mach-at91/at91rm9200_time.c  |   14 --------------
 arch/arm/mach-at91/at91sam926x_time.c |    6 +-----
 arch/arm/mach-nomadik/timer.c         |    9 +--------
 arch/arm/mach-pxa/time.c              |   10 +---------
 arch/arm/mach-sa1100/time.c           |    8 +-------
 5 files changed, 4 insertions(+), 43 deletions(-)

diff --git a/arch/arm/mach-at91/at91rm9200_time.c b/arch/arm/mach-at91/at91rm9200_time.c
index 309f351..f27ccf6 100644
--- a/arch/arm/mach-at91/at91rm9200_time.c
+++ b/arch/arm/mach-at91/at91rm9200_time.c
@@ -132,24 +132,11 @@ clkevt32k_mode(enum clock_event_mode mode, struct clock_event_device *dev)
 static int
 clkevt32k_next_event(unsigned long delta, struct clock_event_device *dev)
 {
-	unsigned long	flags;
 	u32		alm;
 	int		status = 0;
 
 	BUG_ON(delta < 2);
 
-	/* Use "raw" primitives so we behave correctly on RT kernels. */
-	raw_local_irq_save(flags);
-
-	/*
-	 * According to Thomas Gleixner irqs are already disabled here.  Simply
-	 * removing raw_local_irq_save above (and the matching
-	 * raw_local_irq_restore) was not accepted.  See
-	 * http://thread.gmane.org/gmane.linux.ports.arm.kernel/41174
-	 * So for now (2008-11-20) just warn once if irqs were not disabled ...
-	 */
-	WARN_ON_ONCE(!raw_irqs_disabled_flags(flags));
-
 	/* The alarm IRQ uses absolute time (now+delta), not the relative
 	 * time (delta) in our calling convention.  Like all clockevents
 	 * using such "match" hardware, we have a race to defend against.
@@ -169,7 +156,6 @@ clkevt32k_next_event(unsigned long delta, struct clock_event_device *dev)
 	alm += delta;
 	at91_sys_write(AT91_ST_RTAR, alm);
 
-	raw_local_irq_restore(flags);
 	return status;
 }
 
diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
index 4bd56ae..c26cd6e 100644
--- a/arch/arm/mach-at91/at91sam926x_time.c
+++ b/arch/arm/mach-at91/at91sam926x_time.c
@@ -62,16 +62,12 @@ static struct clocksource pit_clk = {
 static void
 pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev)
 {
-	unsigned long	flags;
-
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
-		/* update clocksource counter, then enable the IRQ */
-		raw_local_irq_save(flags);
+		/* update clocksource counter */
 		pit_cnt += pit_cycle * PIT_PICNT(at91_sys_read(AT91_PIT_PIVR));
 		at91_sys_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN
 				| AT91_PIT_PITIEN);
-		raw_local_irq_restore(flags);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
 		BUG();
diff --git a/arch/arm/mach-nomadik/timer.c b/arch/arm/mach-nomadik/timer.c
index d1738e7..e361be6 100644
--- a/arch/arm/mach-nomadik/timer.c
+++ b/arch/arm/mach-nomadik/timer.c
@@ -54,24 +54,17 @@ static struct clocksource nmdk_clksrc = {
 static void nmdk_clkevt_mode(enum clock_event_mode mode,
 			     struct clock_event_device *dev)
 {
-	unsigned long flags;
-
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
-		/* enable interrupts -- and count current value? */
-		raw_local_irq_save(flags);
+		/* count current value? */
 		writel(readl(mtu_base + MTU_IMSC) | 1, mtu_base + MTU_IMSC);
-		raw_local_irq_restore(flags);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
 		BUG(); /* Not supported, yet */
 		/* FALLTHROUGH */
 	case CLOCK_EVT_MODE_SHUTDOWN:
 	case CLOCK_EVT_MODE_UNUSED:
-		/* disable irq */
-		raw_local_irq_save(flags);
 		writel(readl(mtu_base + MTU_IMSC) & ~1, mtu_base + MTU_IMSC);
-		raw_local_irq_restore(flags);
 		break;
 	case CLOCK_EVT_MODE_RESUME:
 		break;
diff --git a/arch/arm/mach-pxa/time.c b/arch/arm/mach-pxa/time.c
index 750c448..293e40a 100644
--- a/arch/arm/mach-pxa/time.c
+++ b/arch/arm/mach-pxa/time.c
@@ -76,14 +76,12 @@ pxa_ost0_interrupt(int irq, void *dev_id)
 static int
 pxa_osmr0_set_next_event(unsigned long delta, struct clock_event_device *dev)
 {
-	unsigned long flags, next, oscr;
+	unsigned long next, oscr;
 
-	raw_local_irq_save(flags);
 	OIER |= OIER_E0;
 	next = OSCR + delta;
 	OSMR0 = next;
 	oscr = OSCR;
-	raw_local_irq_restore(flags);
 
 	return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0;
 }
@@ -91,23 +89,17 @@ pxa_osmr0_set_next_event(unsigned long delta, struct clock_event_device *dev)
 static void
 pxa_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *dev)
 {
-	unsigned long irqflags;
-
 	switch (mode) {
 	case CLOCK_EVT_MODE_ONESHOT:
-		raw_local_irq_save(irqflags);
 		OIER &= ~OIER_E0;
 		OSSR = OSSR_M0;
-		raw_local_irq_restore(irqflags);
 		break;
 
 	case CLOCK_EVT_MODE_UNUSED:
 	case CLOCK_EVT_MODE_SHUTDOWN:
 		/* initializing, released, or preparing for suspend */
-		raw_local_irq_save(irqflags);
 		OIER &= ~OIER_E0;
 		OSSR = OSSR_M0;
-		raw_local_irq_restore(irqflags);
 		break;
 
 	case CLOCK_EVT_MODE_RESUME:
diff --git a/arch/arm/mach-sa1100/time.c b/arch/arm/mach-sa1100/time.c
index b9cbb56..74b6e0e 100644
--- a/arch/arm/mach-sa1100/time.c
+++ b/arch/arm/mach-sa1100/time.c
@@ -35,14 +35,12 @@ static irqreturn_t sa1100_ost0_interrupt(int irq, void *dev_id)
 static int
 sa1100_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c)
 {
-	unsigned long flags, next, oscr;
+	unsigned long next, oscr;
 
-	raw_local_irq_save(flags);
 	OIER |= OIER_E0;
 	next = OSCR + delta;
 	OSMR0 = next;
 	oscr = OSCR;
-	raw_local_irq_restore(flags);
 
 	return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0;
 }
@@ -50,16 +48,12 @@ sa1100_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c)
 static void
 sa1100_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c)
 {
-	unsigned long flags;
-
 	switch (mode) {
 	case CLOCK_EVT_MODE_ONESHOT:
 	case CLOCK_EVT_MODE_UNUSED:
 	case CLOCK_EVT_MODE_SHUTDOWN:
-		raw_local_irq_save(flags);
 		OIER &= ~OIER_E0;
 		OSSR = OSSR_M0;
-		raw_local_irq_restore(flags);
 		break;
 
 	case CLOCK_EVT_MODE_RESUME:
-- 
1.6.5.2

[RESENT PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Russell King - ARM Linux]

On Thu, Nov 26, 2009 at 11:26:04AM +0100, Uwe Kleine-K?nig wrote:
> These functions are called with irqs already off.
> 
> AT91RM2000 had a WARN_ON_ONCE if irqs were enabled since Nov 2008 with
> noone reporting having hit it.

Can we please start to create some documentation for this, even if it
just starts off as "these callbacks are always called with irqs
disabled" or some such thing.

I find the generic time stuff extremely difficult to work with, and I
suspect I'm not the only one.  This is probably why people like to be
sure by having their own IRQ disabling.

[RESENT PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Russell King - ARM Linux]

On Thu, Nov 26, 2009 at 10:50:02AM +0000, Russell King - ARM Linux wrote:
> On Thu, Nov 26, 2009 at 11:26:04AM +0100, Uwe Kleine-K?nig wrote:
> > These functions are called with irqs already off.
> > 
> > AT91RM2000 had a WARN_ON_ONCE if irqs were enabled since Nov 2008 with
> > noone reporting having hit it.
> 
> Can we please start to create some documentation for this, even if it
> just starts off as "these callbacks are always called with irqs
> disabled" or some such thing.
> 
> I find the generic time stuff extremely difficult to work with, and I
> suspect I'm not the only one.  This is probably why people like to be
> sure by having their own IRQ disabling.

To prove the IRQ-ness of the set_next_event callback, I've traced through
all the time code and come up with all these possible call paths:

set_next_event
`-clockevents_program_event
  +-tick_handle_periodic_broadcast
  | `- dev->event_handler
  |
  +-tick_handle_periodic
  | `- dev->event_handler
  |
  +-tick_setup_periodic
  | +-tick_broadcast_start_periodic
  | | +-tick_check_broadcast_device
  | | | `-tick_check_new_device (irqsave)
  | | |
  | | +-tick_device_uses_broadcast (irqsave)
  | | |
  | | +-tick_do_broadcast_on_off (irqsave)
  | | |
  | | `-tick_resume_broadcast (irqsave)
  | |
  | +-tick_do_broadcast_on_off (irqsave)
  | |
  | +-tick_setup_device
  | | `-tick_check_new_device (irqsave)
  | |
  | `-tick_resume (irqsave)
  |
  `-tick_dev_program_event
    +-tick_broadcast_set_event
    | +-tick_handle_oneshot_broadcast
    | | `- dev->event_handler
    | |
    | +-tick_broadcast_oneshot_control (irqsave)
    | |
    | +-tick_broadcast_setup_oneshot
    |   +-tick_do_broadcast_on_off (irqsave)
    |   `-tick_broadcast_switch_to_oneshot (irqsave)
    |
    +-tick_program_event
    | +-tick_resume_oneshot
    | | `-tick_resume (irqsave)
    | |
    | +-tick_nohz_stop_sched_tick (irqsave)
    | |
    | +-tick_nohz_restart
    | | +-tick_nohz_restart_sched_tick (irqdisable)
    | | |
    | | `-tick_nohz_kick_tick (#if 0'd out)
    | |
    | +-tick_nohz_reprogram
    | | `-tick_nohz_handler
    | |   `- dev->event_handler
    | |
    | `-tick_nohz_switch_to_nohz (irqdisable)
    |
    `-tick_setup_oneshot
      `-tick_setup_device
        `-tick_check_new_device (irqsave)

All leaves end in one of four cases:
1. a call via dev->event_handler
2. a function which uses spin_lock_irqsave before calling the child
3. a function which uses local_irq_disable before calling the child
4. a call which is #if 0'd out

So, we can be certain that in cases 2, 3, 4, set_next_event will be
called with IRQs disabled.  That leaves case 1, which is called from
the implementations interrupt handling function, or:

tick_do_broadcast
+-tick_do_periodic_broadcast
| `-tick_handle_periodic_broadcast
|   `- dev->event_handler
`-tick_handle_oneshot_broadcast
   `- dev->event_handler

which basically leaves us with the implementations interrupt handling
function.  If that always calls the event handler with IRQs disabled,
then set_next_event will also be called with IRQs disabled.

Is the same true for set_mode?  Without doing a similar analysis, I
wouldn't know.  I'm sure the folk who created generic time would surely
know the answer off the tops of their heads.

[RESENT PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Uwe Kleine-König]

Hello,

On Thu, Nov 26, 2009 at 11:31:58AM +0000, Russell King - ARM Linux wrote:
> On Thu, Nov 26, 2009 at 10:50:02AM +0000, Russell King - ARM Linux wrote:
> > On Thu, Nov 26, 2009 at 11:26:04AM +0100, Uwe Kleine-K?nig wrote:
> > > These functions are called with irqs already off.
> > > 
> > > AT91RM2000 had a WARN_ON_ONCE if irqs were enabled since Nov 2008 with
> > > noone reporting having hit it.
> > 
> > Can we please start to create some documentation for this, even if it
> > just starts off as "these callbacks are always called with irqs
> > disabled" or some such thing.
> > 
> > I find the generic time stuff extremely difficult to work with, and I
> > suspect I'm not the only one.  This is probably why people like to be
> > sure by having their own IRQ disabling.
> 
> To prove the IRQ-ness of the set_next_event callback, I've traced through
> all the time code and come up with all these possible call paths:
> 
> [...]
> 
> All leaves end in one of four cases:
> 1. a call via dev->event_handler
> 2. a function which uses spin_lock_irqsave before calling the child
> 3. a function which uses local_irq_disable before calling the child
> 4. a call which is #if 0'd out
> 
> So, we can be certain that in cases 2, 3, 4, set_next_event will be
> called with IRQs disabled.  That leaves case 1, which is called from
> the implementations interrupt handling function, or:
> 
> tick_do_broadcast
> +-tick_do_periodic_broadcast
> | `-tick_handle_periodic_broadcast
> |   `- dev->event_handler
> `-tick_handle_oneshot_broadcast
>    `- dev->event_handler
I currently fail to trace where the irqs are disabled, but I have an
at91rm2000 machine and the warning doesn't trigger.
Where are irqs reenabled after exception entry?  Is it before or after
the handler is called?
 
For that machine event_handler is hrtimer_interrupt.  That has an
annotation that it's always called with irqs disabled.

> which basically leaves us with the implementations interrupt handling
> function.  If that always calls the event handler with IRQs disabled,
> then set_next_event will also be called with IRQs disabled.
Thomas, do you care to shed light on this?

If you don't care I suggest to add the same check as for at91rm2000 for
the other platforms and see what happens.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[RESENT PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Thomas Gleixner]

On Fri, 27 Nov 2009, Uwe Kleine-K?nig wrote:
> > which basically leaves us with the implementations interrupt handling
> > function.  If that always calls the event handler with IRQs disabled,
> > then set_next_event will also be called with IRQs disabled.
> Thomas, do you care to shed light on this?

I answered this before and still all CE functions are called with irqs
disabled. The timer interrupt is required to run with irqs disabled
and it was so before we switched to CE and the generic implementation.

> If you don't care I suggest to add the same check as for at91rm2000 for
> the other platforms and see what happens.

And as last time I suggest to add a comment to the ce struct
documentation, only this time I'll do it myself.

Thanks,

	tglx

[RESENT PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Russell King - ARM Linux]

On Fri, Nov 27, 2009 at 11:44:55AM +0100, Uwe Kleine-K?nig wrote:
> Hello,
> 
> On Thu, Nov 26, 2009 at 11:31:58AM +0000, Russell King - ARM Linux wrote:
> > tick_do_broadcast
> > +-tick_do_periodic_broadcast
> > | `-tick_handle_periodic_broadcast
> > |   `- dev->event_handler
> > `-tick_handle_oneshot_broadcast
> >    `- dev->event_handler
> I currently fail to trace where the irqs are disabled, but I have an
> at91rm2000 machine and the warning doesn't trigger.
> Where are irqs reenabled after exception entry?  Is it before or after
> the handler is called?

If interrupt handlers are requested with IRQF_DISABLED (as timer
interrupts should be), they will be called with IRQs disabled.

There is one exception: if an interrupt is requested with
IRQF_SHARED|IRQF_DISABLED and either another handler enables interrupts
itself and leaves them enabled, or was requested without IRQF_DISABLED.
In this case, such a handler can still be called with IRQs enabled.

[RESENT PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Uwe Kleine-König]

Hello Russell,

On Fri, Nov 27, 2009 at 07:58:57PM +0000, Russell King - ARM Linux wrote:
> On Fri, Nov 27, 2009 at 11:44:55AM +0100, Uwe Kleine-K?nig wrote:
> > Hello,
> > 
> > On Thu, Nov 26, 2009 at 11:31:58AM +0000, Russell King - ARM Linux wrote:
> > > tick_do_broadcast
> > > +-tick_do_periodic_broadcast
> > > | `-tick_handle_periodic_broadcast
> > > |   `- dev->event_handler
> > > `-tick_handle_oneshot_broadcast
> > >    `- dev->event_handler
> > I currently fail to trace where the irqs are disabled, but I have an
> > at91rm2000 machine and the warning doesn't trigger.
> > Where are irqs reenabled after exception entry?  Is it before or after
> > the handler is called?
> 
> If interrupt handlers are requested with IRQF_DISABLED (as timer
> interrupts should be), they will be called with IRQs disabled.
> 
> There is one exception: if an interrupt is requested with
> IRQF_SHARED|IRQF_DISABLED and either another handler enables interrupts
> itself and leaves them enabled, or was requested without IRQF_DISABLED.
> In this case, such a handler can still be called with IRQs enabled.
OK, so this should make it save for mach-nomadik/timer.c,
mach-pxa/time.c and mach-sa1100/time.c.

Only mach-at91/at91rm9200_time.c and mach-at91/at91sam926x_time.c
request with IRQF_SHARED | IRQF_DISABLED.
So for there the right solution is to let the local_irq_save in place
and add a comment that it is needed as the timer irq is shared.
This hopefully stopps people copying it unnecessarily.

@Eric: do you still prefer the warning or is it OK for you now to remove
the irq saving for nomadik, pxa and sa1100?

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[RESENT PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Russell King - ARM Linux]

On Fri, Nov 27, 2009 at 09:38:23PM +0100, Uwe Kleine-K?nig wrote:
> OK, so this should make it save for mach-nomadik/timer.c,
> mach-pxa/time.c and mach-sa1100/time.c.
> 
> Only mach-at91/at91rm9200_time.c and mach-at91/at91sam926x_time.c
> request with IRQF_SHARED | IRQF_DISABLED.
> So for there the right solution is to let the local_irq_save in place
> and add a comment that it is needed as the timer irq is shared.
> This hopefully stopps people copying it unnecessarily.

Given that the gentime code spits out warnings if its called with IRQs
enabled, I don't think keeping the local_irq_save() in place is worth
it - especially given Thomas' response.

We've seen such warnings on AT91 (though I don't think we got positively
to the bottom of it with the reporter.)  If you _do_ want to ensure that
AT91 is safe, wrapping the call to dev->event_handler(dev) in the AT91
interrupt handler with a local_irq_save/local_irq_restore would seem
sensible.

[RESENT PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Thomas Gleixner]

On Fri, 27 Nov 2009, Russell King - ARM Linux wrote:
> On Fri, Nov 27, 2009 at 09:38:23PM +0100, Uwe Kleine-K?nig wrote:
> > OK, so this should make it save for mach-nomadik/timer.c,
> > mach-pxa/time.c and mach-sa1100/time.c.
> > 
> > Only mach-at91/at91rm9200_time.c and mach-at91/at91sam926x_time.c
> > request with IRQF_SHARED | IRQF_DISABLED.
> > So for there the right solution is to let the local_irq_save in place
> > and add a comment that it is needed as the timer irq is shared.
> > This hopefully stopps people copying it unnecessarily.
> 
> Given that the gentime code spits out warnings if its called with IRQs
> enabled, I don't think keeping the local_irq_save() in place is worth
> it - especially given Thomas' response.
> 
> We've seen such warnings on AT91 (though I don't think we got positively
> to the bottom of it with the reporter.)  If you _do_ want to ensure that

Any pointer ?

> AT91 is safe, wrapping the call to dev->event_handler(dev) in the AT91
> interrupt handler with a local_irq_save/local_irq_restore would seem
> sensible.

The only way how that would happen is when the serial interrupt would
be set up _BEFORE_ the timer interrupt and when the serial handler
would enable interrupts at some point. Shared interrupt handlers are
called in order of setup.

Thanks,

	tglx

[RESENT PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Russell King - ARM Linux]

On Fri, Nov 27, 2009 at 10:31:15PM +0100, Thomas Gleixner wrote:
> On Fri, 27 Nov 2009, Russell King - ARM Linux wrote:
> > On Fri, Nov 27, 2009 at 09:38:23PM +0100, Uwe Kleine-K?nig wrote:
> > > OK, so this should make it save for mach-nomadik/timer.c,
> > > mach-pxa/time.c and mach-sa1100/time.c.
> > > 
> > > Only mach-at91/at91rm9200_time.c and mach-at91/at91sam926x_time.c
> > > request with IRQF_SHARED | IRQF_DISABLED.
> > > So for there the right solution is to let the local_irq_save in place
> > > and add a comment that it is needed as the timer irq is shared.
> > > This hopefully stopps people copying it unnecessarily.
> > 
> > Given that the gentime code spits out warnings if its called with IRQs
> > enabled, I don't think keeping the local_irq_save() in place is worth
> > it - especially given Thomas' response.
> > 
> > We've seen such warnings on AT91 (though I don't think we got positively
> > to the bottom of it with the reporter.)  If you _do_ want to ensure that
> 
> Any pointer ?

http://lists.arm.linux.org.uk/lurker/thread/20091030.165300.55a15962.en.html

[RESENT PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Thomas Gleixner]

On Fri, 27 Nov 2009, Russell King - ARM Linux wrote:
> On Fri, Nov 27, 2009 at 10:31:15PM +0100, Thomas Gleixner wrote:
> > On Fri, 27 Nov 2009, Russell King - ARM Linux wrote:
> > > On Fri, Nov 27, 2009 at 09:38:23PM +0100, Uwe Kleine-K?nig wrote:
> > > > OK, so this should make it save for mach-nomadik/timer.c,
> > > > mach-pxa/time.c and mach-sa1100/time.c.
> > > > 
> > > > Only mach-at91/at91rm9200_time.c and mach-at91/at91sam926x_time.c
> > > > request with IRQF_SHARED | IRQF_DISABLED.
> > > > So for there the right solution is to let the local_irq_save in place
> > > > and add a comment that it is needed as the timer irq is shared.
> > > > This hopefully stopps people copying it unnecessarily.
> > > 
> > > Given that the gentime code spits out warnings if its called with IRQs
> > > enabled, I don't think keeping the local_irq_save() in place is worth
> > > it - especially given Thomas' response.
> > > 
> > > We've seen such warnings on AT91 (though I don't think we got positively
> > > to the bottom of it with the reporter.)  If you _do_ want to ensure that
> > 
> > Any pointer ?
> 
> http://lists.arm.linux.org.uk/lurker/thread/20091030.165300.55a15962.en.html

That thread ends unfortunately at the point where it started to be
interesting.

I have AT91 platforms myself and we have not seen any problem at
all.

Thanks,

	tglx

[PATCH] warn about shared irqs requesting IRQF_DISABLED registered with setup_irq

[Uwe Kleine-König]

IRQF_DISABLED is not guaranteed on shared irqs.  There is already a
warning in place for irqs registered with request_irq (introduced in
470c66239ef03).  Move it to __setup_irq, this way it triggers for both
request_irq and setup_irq.

One irq that is now warned about is the timer tick on at91 (ARCH=arm).

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Nicolas Pitre <nico@marvell.com>
Cc: Eric Miao <eric.y.miao@gmail.com>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Remy Bohmer <linux@bohmer.net>,
Cc: Hugh Dickins <hugh.dickins@tiscali.co.uk>,
Cc: Andrea Gallo <andrea.gallo@stericsson.com>,
Cc: Thomas Gleixner <tglx@linutronix.de>,
Cc: linux-arm-kernel at lists.infradead.org
---
 kernel/irq/manage.c |   26 +++++++++++++-------------
 1 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index bde4c66..59f4b54 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -613,6 +613,19 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	int nested, shared = 0;
 	int ret;
 
+	/*
+	 * handle_IRQ_event() always ignores IRQF_DISABLED except for
+	 * the _first_ irqaction (sigh).  That can cause oopsing, but
+	 * the behavior is classified as "will not fix" so we need to
+	 * start nudging drivers away from using that idiom.
+	 */
+	if ((new->flags & (IRQF_SHARED|IRQF_DISABLED)) ==
+					(IRQF_SHARED|IRQF_DISABLED)) {
+		pr_warning(
+		  "IRQ %d/%s: IRQF_DISABLED is not guaranteed on shared IRQs\n",
+			irq, new->name);
+	}
+
 	if (!desc)
 		return -EINVAL;
 
@@ -1008,19 +1021,6 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 	struct irq_desc *desc;
 	int retval;
 
-	/*
-	 * handle_IRQ_event() always ignores IRQF_DISABLED except for
-	 * the _first_ irqaction (sigh).  That can cause oopsing, but
-	 * the behavior is classified as "will not fix" so we need to
-	 * start nudging drivers away from using that idiom.
-	 */
-	if ((irqflags & (IRQF_SHARED|IRQF_DISABLED)) ==
-					(IRQF_SHARED|IRQF_DISABLED)) {
-		pr_warning(
-		  "IRQ %d/%s: IRQF_DISABLED is not guaranteed on shared IRQs\n",
-			irq, devname);
-	}
-
 #ifdef CONFIG_LOCKDEP
 	/*
 	 * Lockdep wants atomic interrupt handlers:
-- 
1.6.5.2

[PATCH] warn about shared irqs requesting IRQF_DISABLED registered with setup_irq

[Thomas Gleixner]

On Fri, 27 Nov 2009, Uwe Kleine-K?nig wrote:

> IRQF_DISABLED is not guaranteed on shared irqs.  There is already a
> warning in place for irqs registered with request_irq (introduced in
> 470c66239ef03).  Move it to __setup_irq, this way it triggers for both
> request_irq and setup_irq.
> 
> One irq that is now warned about is the timer tick on at91 (ARCH=arm).

And how does that help ? The interrupt is shared between the timer and
the debug port. There is nothing you can do about that.

The interupt handlers are called in order of setup. The AT91 timer
irq is set up first and if that's not the case then it needs to be
fixed and the only way to catch it is in the affected interrupt
handler.

Applying your patch does not change the hardware and will just result
in useless, annoying and confusing dmesg warnings.

> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Remy Bohmer <linux@bohmer.net>,
> Cc: Hugh Dickins <hugh.dickins@tiscali.co.uk>,
> Cc: Andrea Gallo <andrea.gallo@stericsson.com>,

Nice that you added me (and others) to that long list, but ...

> Cc: Thomas Gleixner <tglx@linutronix.de>,
------------------------------------------^

Thanks,

	tglx

[PATCH] warn about shared irqs requesting IRQF_DISABLED registered with setup_irq

[Uwe Kleine-König]

On Fri, Nov 27, 2009 at 11:18:00PM +0100, Thomas Gleixner wrote:
> On Fri, 27 Nov 2009, Uwe Kleine-K?nig wrote:
> 
> > IRQF_DISABLED is not guaranteed on shared irqs.  There is already a
> > warning in place for irqs registered with request_irq (introduced in
> > 470c66239ef03).  Move it to __setup_irq, this way it triggers for both
> > request_irq and setup_irq.
> > 
> > One irq that is now warned about is the timer tick on at91 (ARCH=arm).
> 
> And how does that help ? The interrupt is shared between the timer and
> the debug port. There is nothing you can do about that.
> 
> The interupt handlers are called in order of setup. The AT91 timer
> irq is set up first and if that's not the case then it needs to be
> fixed and the only way to catch it is in the affected interrupt
> handler.
Russell already suggests to save (and restore) irqs in the handler
before (and after resp.) calling the clockevent functions.

Should it use the raw_ variants?
 
> Applying your patch does not change the hardware and will just result
> in useless, annoying and confusing dmesg warnings.
My patch wasn't mainly to help in the at91 case.  I just thought that a
warning that is triggered with request_irq and request_threaded_irq
shouldn't be skipped when using setup_irq.

Maybe the warning should only be printed if there's a mismatch between
different actions of the same irq regarding IRQF_DISABLED?!

I will prepare a patch for both (i.e. fixing the at91 ISRs and the
warning about IRQF_DISABLED | IRQF_SHARED) but probably only on Monday.
 
Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[PATCH] warn about shared irqs requesting IRQF_DISABLED registered with setup_irq

[Thomas Gleixner]

On Sat, 28 Nov 2009, Uwe Kleine-K?nig wrote:

> On Fri, Nov 27, 2009 at 11:18:00PM +0100, Thomas Gleixner wrote:
> > On Fri, 27 Nov 2009, Uwe Kleine-K?nig wrote:
> > 
> > > IRQF_DISABLED is not guaranteed on shared irqs.  There is already a
> > > warning in place for irqs registered with request_irq (introduced in
> > > 470c66239ef03).  Move it to __setup_irq, this way it triggers for both
> > > request_irq and setup_irq.
> > > 
> > > One irq that is now warned about is the timer tick on at91 (ARCH=arm).
> > 
> > And how does that help ? The interrupt is shared between the timer and
> > the debug port. There is nothing you can do about that.
> > 
> > The interupt handlers are called in order of setup. The AT91 timer
> > irq is set up first and if that's not the case then it needs to be
> > fixed and the only way to catch it is in the affected interrupt
> > handler.
>
> Russell already suggests to save (and restore) irqs in the handler
> before (and after resp.) calling the clockevent functions.

What about analysing the code and verifying that the setup order is
correct ?

Adding save/restore_irq just because you have no clue what the code
does is utter nonsense.

Thanks,

	tglx

[PATCH] warn about shared irqs requesting IRQF_DISABLED registered with setup_irq

[David Brownell]

On Saturday 28 November 2009, Thomas Gleixner wrote:
> What about analysing the code and verifying that the setup order is
> correct ?

Better to not care too much about setup order.  Sometimes it's
unavoidable -- X initializes before Y "or else..." -- but the
patch I saw was just reporting goofage better.


> Adding save/restore_irq just because you have no clue what the code
> does is utter nonsense.

Absolutely.  If that helps, find and fix the real bug.

[PATCH] warn about shared irqs requesting IRQF_DISABLED registered with setup_irq

[Jamie Lokier]

Thomas Gleixner wrote:
> What about analysing the code and verifying that the setup order is
> correct ?
> 
> Adding save/restore_irq just because you have no clue what the code
> does is utter nonsense.

Wouldn't it be quite a lot nicer if generic setup moved the
IRQF_DISABLED handler to be first in the list, if that actually works
in a useful way rather than simply being a quirk that irqs are
disabled for the first one?

-- Jamie

[PATCH] warn about shared irqs requesting IRQF_DISABLED registered with setup_irq

[Uwe Kleine-König]

Hello,

On Sun, Nov 29, 2009 at 02:31:18AM +0000, Jamie Lokier wrote:
> Thomas Gleixner wrote:
> > What about analysing the code and verifying that the setup order is
> > correct ?
> > 
> > Adding save/restore_irq just because you have no clue what the code
> > does is utter nonsense.
> 
> Wouldn't it be quite a lot nicer if generic setup moved the
> IRQF_DISABLED handler to be first in the list, if that actually works
> in a useful way rather than simply being a quirk that irqs are
> disabled for the first one?
Hmm, what happens if an ISR runs with irqs disabled even though it
doesn't expect it?  I wouldn't bet that nothing breaks.

IMHO the best is if a warning is printed or registering fails if shared
irq actions don't agree about wanting IRQF_DISABLED.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[PATCH] warn about shared irqs requesting IRQF_DISABLED registered with setup_irq

[Jamie Lokier]

Uwe Kleine-K?nig wrote:
> Hello,
> 
> On Sun, Nov 29, 2009 at 02:31:18AM +0000, Jamie Lokier wrote:
> > Thomas Gleixner wrote:
> > > What about analysing the code and verifying that the setup order is
> > > correct ?
> > > 
> > > Adding save/restore_irq just because you have no clue what the code
> > > does is utter nonsense.
> > 
> > Wouldn't it be quite a lot nicer if generic setup moved the
> > IRQF_DISABLED handler to be first in the list, if that actually works
> > in a useful way rather than simply being a quirk that irqs are
> > disabled for the first one?
> Hmm, what happens if an ISR runs with irqs disabled even though it
> doesn't expect it?  I wouldn't bet that nothing breaks.

Moving the IRQF_DISABLED handler to be first will run an ISR with
interrupts disabled which *does* expect it, so that's good.

According to this thread, at the moment when you have multiple
IRQF_DISABLED|IRQF_SHARED ISRs, only the first one is run with
interrupts disabled.

In fact I don't see why the kernel cannot put _all_ of the
IRQ_DISABLED handlers at the beginning of the list, traverse those
with interrupts disabled, then enable interrupts them for the
remaining handlers.

> IMHO the best is if a warning is printed or registering fails if shared
> irq actions don't agree about wanting IRQF_DISABLED.

On the hardware in question, the debug and timer interrupts share a
line, and I'm guessing only the timer interrupt should have IRQF_DISABLED.

Or we could do away with this silliness and just switch everything to
threaded interrupts with RT-priorities ;-)

-- Jamie

[PATCH] warn about shared irqs requesting IRQF_DISABLED registered with setup_irq

[Russell King - ARM Linux]

On Sun, Nov 29, 2009 at 03:18:40PM +0000, Jamie Lokier wrote:
> Or we could do away with this silliness and just switch everything to
> threaded interrupts with RT-priorities ;-)

... thereby needlessly increasing the latency of all interrupt handling
and probably breaking some devices.

Some devices (eg, SMC91x, serial ports, PIO audio - AACI, MMCI) are
_extremely_ sensitive to interrupt latency due to lack of FIFO depth.

MMCI already sees overruns on ARM platforms if you try and clock the
cards at anything over about 500kHz.  Adding any more latency means
reducing the maximum clock rate there, and therefore crippling
throughput even more than it is already.

[PATCH] warn about shared irqs requesting IRQF_DISABLED registered with setup_irq

[Jamie Lokier]

Russell King - ARM Linux wrote:
> On Sun, Nov 29, 2009 at 03:18:40PM +0000, Jamie Lokier wrote:
> > Or we could do away with this silliness and just switch everything to
> > threaded interrupts with RT-priorities ;-)
> 
> ... thereby needlessly increasing the latency of all interrupt handling
> and probably breaking some devices.

It's not that cut and dried.  RT gives you priorities where you might
not have had them before, so in some cases can reduce worst-case
latency for critical devices.  IRQF_DISABLED is a cheap two-level
priority scheme; RT threaded interrupts extend it.

The extra processing increases latency, yes, but that is in a sense a
scheduling fast-path problem; it's not _intrinsic_ to threaded
interrupts that they have to have higher latency (you don't have to
use the normal calculation or task switch to get equivalent
behaviour), but undoubtedly trying to optimise that leads to very
twisty code and state representations, and I don't see it happening in
Linux any time soon.

-- Jamie

[PATCH] warn about shared irqs requesting IRQF_DISABLED registered with setup_irq

[Uwe Kleine-König]

Hello Jamie,

On Sun, Nov 29, 2009 at 03:18:40PM +0000, Jamie Lokier wrote:
> According to this thread, at the moment when you have multiple
> IRQF_DISABLED|IRQF_SHARED ISRs, only the first one is run with
> interrupts disabled.
No.  When you have multiple IRQF_SHARED ISRs *all* are run with irqs
disabled if the first has IRQF_DISABLED.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[PATCH] warn about shared irqs requesting IRQF_DISABLED registered with setup_irq

[Thomas Gleixner]

On Sun, 29 Nov 2009, Jamie Lokier wrote:
> > > Wouldn't it be quite a lot nicer if generic setup moved the
> > > IRQF_DISABLED handler to be first in the list, if that actually works
> > > in a useful way rather than simply being a quirk that irqs are
> > > disabled for the first one?
> > Hmm, what happens if an ISR runs with irqs disabled even though it
> > doesn't expect it?  I wouldn't bet that nothing breaks.
> 
> Moving the IRQF_DISABLED handler to be first will run an ISR with
> interrupts disabled which *does* expect it, so that's good.
> 
> According to this thread, at the moment when you have multiple
> IRQF_DISABLED|IRQF_SHARED ISRs, only the first one is run with
> interrupts disabled.

Wrong. The flags of the first action are checked to decide whether
interrupts are enabled _before_ calling the action handler.

So for shared interrupts we have the following combinations:

     action1            action2
A    -	                -
B    IRQF_DISABLED      -
C    -                  IRQF_DISABLED
D    IRQF_DISABLED      IRQF_DISABLED

A) Correct behaviour for action1 and action2

B) Correct behaviour for action1, but action2 is called with
   interrupts disabled

C) Correct behaviour for action1, but action2 is called with
   interrupts enabled
 
D) Correct behaviour for action1 and action2 in theory

> In fact I don't see why the kernel cannot put _all_ of the
> IRQ_DISABLED handlers at the beginning of the list, traverse those
> with interrupts disabled, then enable interrupts them for the
> remaining handlers.

That does not work reliable, because IRQF_DISABLED is just describing
the calling convention of the interrupt.

It does not say, that the handler is not allowed to enable
interrupts. So for D) there is no guarantee that the action1 handler
keeps interrupts disabled. If it enables interrupts in the handler
then the assumptions of action2 are already violated.

> > IMHO the best is if a warning is printed or registering fails if shared
> > irq actions don't agree about wanting IRQF_DISABLED.

No, we print a warning when IRQF_SHARED is used with IRQF_DISABLED as
there is no point about agreeing on something which can not be
guaranteed at all.

Thanks,

	tglx

[PATCH] warn about shared irqs requesting IRQF_DISABLED registered with setup_irq

[David Brownell]

On Saturday 28 November 2009, Uwe Kleine-K?nig wrote:
> ?I just thought that a
> warning that is triggered with request_irq and request_threaded_irq
> shouldn't be skipped when using setup_irq.

Seems fair to me.

[PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED at the right place

[Uwe Kleine-König]

For shared irqs IRQF_DISABLED is only guaranteed for the first handler.
So only warn starting at the second registration.

The warning is moved to __setup_irq having the additional benefit of
catching actions registered using setup_irq not only register_irq.

This doesn't fix the cases where setup order is wrong but it should
report the broken cases more reliably.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Nicolas Pitre <nico@marvell.com>
Cc: Eric Miao <eric.y.miao@gmail.com>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Remy Bohmer <linux@bohmer.net>
Cc: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Cc: Andrea Gallo <andrea.gallo@stericsson.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jamie Lokier <jamie@shareable.org>
Cc: linux-arm-kernel at lists.infradead.org
---
 kernel/irq/manage.c |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index bde4c66..d8f7415 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -702,6 +702,16 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 			goto mismatch;
 #endif
 
+		/*
+		 * handle_IRQ_event() only honors IRQF_DISABLED for the _first_
+		 * irqaction.  As the first handler might reenable irqs all bets
+		 * are off for the later handlers even if the first one has
+		 * IRQF_DISABLED, too.
+		 */
+		if (new->flags & IRQF_DISABLED)
+			pr_warning("IRQ %d/%s: IRQF_DISABLED is not guaranteed "
+					"on shared IRQs\n", irq, new->name);
+
 		/* add new interrupt at end of irq queue */
 		do {
 			old_ptr = &old->next;
@@ -1008,19 +1018,6 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 	struct irq_desc *desc;
 	int retval;
 
-	/*
-	 * handle_IRQ_event() always ignores IRQF_DISABLED except for
-	 * the _first_ irqaction (sigh).  That can cause oopsing, but
-	 * the behavior is classified as "will not fix" so we need to
-	 * start nudging drivers away from using that idiom.
-	 */
-	if ((irqflags & (IRQF_SHARED|IRQF_DISABLED)) ==
-					(IRQF_SHARED|IRQF_DISABLED)) {
-		pr_warning(
-		  "IRQ %d/%s: IRQF_DISABLED is not guaranteed on shared IRQs\n",
-			irq, devname);
-	}
-
 #ifdef CONFIG_LOCKDEP
 	/*
 	 * Lockdep wants atomic interrupt handlers:
-- 
1.6.5.2

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Thomas Gleixner]

On Mon, 30 Nov 2009, Uwe Kleine-K?nig wrote:
> For shared irqs IRQF_DISABLED is only guaranteed for the first handler.
> So only warn starting at the second registration.
> 
> The warning is moved to __setup_irq having the additional benefit of
> catching actions registered using setup_irq not only register_irq.
> 
> This doesn't fix the cases where setup order is wrong but it should
> report the broken cases more reliably.

The whole IRQF_DISABLED trickery is questionable and I'm pretty
unhappy about the warning in general.

While it is true that there is no guarantee of IRQF_DISABLED on shared
interrupts (at least not for the secondary handlers) we really need to
think about the reason why we want to run interrupt handlers with
interrupts enabled at all.

The separation of interrupt handlers which run with interrupts
disabled/enabled goes all the way back to Linux 1.0, which had two
interrupt handling modes:

1) handlers installed with SA_INTERRUPT ran atomically with interrupts
   disabled.

2) handlers installed without SA_INTERRUPT ran with interrupts enabled
   as they did more complex stuff like signal handling in the kernel.

The interrupt which was always run with interrupts disabled was the
timer interrupt because some of the "slower" interrupt handlers were
relying on jiffies being updated, which is only possible when they run
with interrupts enabled and no such handler can interrupt the timer
interrupt.

In the 2.1.x timeframe the discussion about shared interrupt handlers
and the treatment of SA_INTERRUPT (today IRQF_DISABLED) was resolved
by changing the code to what we have right now. If you read back in
the archives you will find the same arguments as we have seen in this
thread and a boatload of different solutions to that.

The real question is why we want to run an interrupt handler with
interrupts enabled at all. There are two reaons AFAICT:

1) interrupt handler relies on jiffies being updated:

   I don't think that this is the case anymore and if we still have
   code which does it is probably historic crap which is unused for
   quite a time.

2) interrupt handler runs a long time:

   I'm sure we still have some of those especially in the
   archaelogical corners of drivers/* and in the creative space of the
   embedded "oh, I don't know why but it works" departement. That's
   code which needs to be fixed anyway.

The correct solution IMNSHO is to get rid of IRQF_DISABLED and run
interrupt handlers always with interrupts disabled and require them
not to reenable interrupts themself.

Thoughts ?

	 tglx

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Peter Zijlstra]

On Mon, 2009-11-30 at 14:54 +0100, Thomas Gleixner wrote:
> On Mon, 30 Nov 2009, Uwe Kleine-K?nig wrote:
> > For shared irqs IRQF_DISABLED is only guaranteed for the first handler.
> > So only warn starting at the second registration.
> > 
> > The warning is moved to __setup_irq having the additional benefit of
> > catching actions registered using setup_irq not only register_irq.
> > 
> > This doesn't fix the cases where setup order is wrong but it should
> > report the broken cases more reliably.
> 
> The whole IRQF_DISABLED trickery is questionable and I'm pretty
> unhappy about the warning in general.
> 
> While it is true that there is no guarantee of IRQF_DISABLED on shared
> interrupts (at least not for the secondary handlers) we really need to
> think about the reason why we want to run interrupt handlers with
> interrupts enabled at all.
> 
> The separation of interrupt handlers which run with interrupts
> disabled/enabled goes all the way back to Linux 1.0, which had two
> interrupt handling modes:
> 
> 1) handlers installed with SA_INTERRUPT ran atomically with interrupts
>    disabled.
> 
> 2) handlers installed without SA_INTERRUPT ran with interrupts enabled
>    as they did more complex stuff like signal handling in the kernel.
> 
> The interrupt which was always run with interrupts disabled was the
> timer interrupt because some of the "slower" interrupt handlers were
> relying on jiffies being updated, which is only possible when they run
> with interrupts enabled and no such handler can interrupt the timer
> interrupt.
> 
> In the 2.1.x timeframe the discussion about shared interrupt handlers
> and the treatment of SA_INTERRUPT (today IRQF_DISABLED) was resolved
> by changing the code to what we have right now. If you read back in
> the archives you will find the same arguments as we have seen in this
> thread and a boatload of different solutions to that.
> 
> The real question is why we want to run an interrupt handler with
> interrupts enabled at all. There are two reaons AFAICT:
> 
> 1) interrupt handler relies on jiffies being updated:
> 
>    I don't think that this is the case anymore and if we still have
>    code which does it is probably historic crap which is unused for
>    quite a time.
> 
> 2) interrupt handler runs a long time:
> 
>    I'm sure we still have some of those especially in the
>    archaelogical corners of drivers/* and in the creative space of the
>    embedded "oh, I don't know why but it works" departement. That's
>    code which needs to be fixed anyway.
> 
> The correct solution IMNSHO is to get rid of IRQF_DISABLED and run
> interrupt handlers always with interrupts disabled and require them
> not to reenable interrupts themself.
> 
> Thoughts ?

I'm all for removing that brain damage:

  http://lkml.org/lkml/2009/3/2/33

We should convert the broken hardware PIO and 3com interrupt things to
threaded interrupts, and simply mandate all IRQ handlers run short and
with IRQs disabled.

Except I guess that will upset some of the IRQ priority folks, like
power, where they (iirc) have a stack per irq prio level.

But its not like the core kernel knows about these nesting rules and can
actually track any of that muck.

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Thomas Gleixner]

On Mon, 30 Nov 2009, Peter Zijlstra wrote:
> On Mon, 2009-11-30 at 14:54 +0100, Thomas Gleixner wrote:
> > The correct solution IMNSHO is to get rid of IRQF_DISABLED and run
> > interrupt handlers always with interrupts disabled and require them
> > not to reenable interrupts themself.
> > 
> > Thoughts ?
> 
> I'm all for removing that brain damage:
> 
>   http://lkml.org/lkml/2009/3/2/33

Darn, I knew that we discussed that before, but my memory tricked me
into believing that it was years ago :)

> We should convert the broken hardware PIO and 3com interrupt things to
> threaded interrupts, and simply mandate all IRQ handlers run short and
> with IRQs disabled.

The ide stuff seems to be the only one which uses
local_irq_enable_in_hardirq() so that at least it tripped over lockdep
already.

What's the 3com wreckage about ?

> Except I guess that will upset some of the IRQ priority folks, like
> power, where they (iirc) have a stack per irq prio level.

Is that actually used ? Ben ?
 
> But its not like the core kernel knows about these nesting rules and can
> actually track any of that muck.

True.

	tglx
 

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Alan Cox]

On Mon, 30 Nov 2009 15:24:40 +0100 (CET)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Mon, 30 Nov 2009, Peter Zijlstra wrote:
> > On Mon, 2009-11-30 at 14:54 +0100, Thomas Gleixner wrote:
> > > The correct solution IMNSHO is to get rid of IRQF_DISABLED and run
> > > interrupt handlers always with interrupts disabled and require them
> > > not to reenable interrupts themself.
> > > 
> > > Thoughts ?
> > 
> > I'm all for removing that brain damage:
> > 
> >   http://lkml.org/lkml/2009/3/2/33
> 
> Darn, I knew that we discussed that before, but my memory tricked me
> into believing that it was years ago :)

Well the patch listed there is utterly bogus and will cause hangs at
startup. The problem case is IRQF_SHARED|IRQF_DISABLED. The patch messes
up all the unshared cases too - and lots of non sharable IRQ hardware just
jams the IRQ line high until you beat it into sense (8530's are notorious
for getting into that kind of state at init for example). You can't simply
remove the disabled from those drivers, you need to be able to allocate a
non-shared IRQ, and then enable it or do major driver restructuring of
obscure old driver code.

SHARED|DISABLED ought to WARN_ON() and if that doesn't motivate people
then return -EINVAL. And with any luck that'll prove 6 months later that
most of the offenders are not used and we can delete them wholesale.

DISABLE without SHARED is fine, and saves waking the dead.

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Russell King - ARM Linux]

On Mon, Nov 30, 2009 at 02:47:02PM +0000, Alan Cox wrote:
> SHARED|DISABLED ought to WARN_ON() and if that doesn't motivate people
> then return -EINVAL.

That is an impossibility.  There is hardware out there (AT91) where
the timer interrupt is shared with other peripherals, and you end
up with a mixture of irqs-disabled and irqs-enabled handlers sharing
the same interrupt.

Luckily, the timer interrupt is the first to claim, and so is the first
to be run.  However, there was a problem reported a while back of the
clock event code being called on AT91 with IRQs enabled - unfortunately
the original reporters stopped responding so it was never worked out
what was going on.

My point is that if we outlaw irqs-disabled shared interrupts, it puts
Atmel AT91 support into immediate difficulties.

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Alan Cox]

On Mon, 30 Nov 2009 15:01:00 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Mon, Nov 30, 2009 at 02:47:02PM +0000, Alan Cox wrote:
> > SHARED|DISABLED ought to WARN_ON() and if that doesn't motivate people
> > then return -EINVAL.
> 
> That is an impossibility.  There is hardware out there (AT91) where
> the timer interrupt is shared with other peripherals, and you end
> up with a mixture of irqs-disabled and irqs-enabled handlers sharing
> the same interrupt.

Well that will encourage people to fix it.

> My point is that if we outlaw irqs-disabled shared interrupts, it puts
> Atmel AT91 support into immediate difficulties.

If a driver disables the timer irq across a tick aren't you already in
trouble ?

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Russell King - ARM Linux]

On Mon, Nov 30, 2009 at 03:32:23PM +0000, Alan Cox wrote:
> On Mon, 30 Nov 2009 15:01:00 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > On Mon, Nov 30, 2009 at 02:47:02PM +0000, Alan Cox wrote:
> > > SHARED|DISABLED ought to WARN_ON() and if that doesn't motivate people
> > > then return -EINVAL.
> > 
> > That is an impossibility.  There is hardware out there (AT91) where
> > the timer interrupt is shared with other peripherals, and you end
> > up with a mixture of irqs-disabled and irqs-enabled handlers sharing
> > the same interrupt.
> 
> Well that will encourage people to fix it.
> 
> > My point is that if we outlaw irqs-disabled shared interrupts, it puts
> > Atmel AT91 support into immediate difficulties.
> 
> If a driver disables the timer irq across a tick aren't you already in
> trouble ?

Not with this clockevents code - you can miss timer interrupts and the core
time keeping code catches up automatically.

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Andrew Victor]

hi,

> There is hardware out there (AT91) where
> the timer interrupt is shared with other peripherals, and you end
> up with a mixture of irqs-disabled and irqs-enabled handlers sharing
> the same interrupt.

For the AT91 case I don't think this shouldn't matter.

The AT91's have a priority-level interrupt controller, so:
  1. a lower-priority interrupt won't interrupt a higher-priority
  2. shared interrupts cannot interrupt each other until irq_finish()
is called (a write to AIC_EOICR)

Since the Timer, DBGU serial port (and other system peripherals) are
on the same priority level they cannot interrupt each other.
(ie, basically as-if always irqs-disabled).

The case of irqs-enabled does means that a higher-priority interrupt
could interrupt [*], but it's not a shared-IRQ in that case.

([*] The system peripherals have the highest priority by default, so
the user would need to override the defaults for this to occur)


Regards,
 Andrew Victor

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[David Brownell]

On Monday 30 November 2009, Russell King - ARM Linux wrote:
> On Mon, Nov 30, 2009 at 02:47:02PM +0000, Alan Cox wrote:
> > SHARED|DISABLED ought to WARN_ON() and if that doesn't motivate people
> > then return -EINVAL.
> 
> That is an impossibility.  There is hardware out there (AT91) where
> the timer interrupt is shared with other peripherals, and you end
> up with a mixture of irqs-disabled and irqs-enabled handlers sharing
> the same interrupt.

For the record:  AT91 isn't restricted to the system timers hooked
up on irq 0 ... there's also drivers/clocksource/tcb_clksrc.c (not
at the same hardware priority).

But to concur, this is indeed messy.  Both the system timer and
the serial console generally share the same IRQ; both are very
timing-sensitive.  I've seen console character dropouts after
tweaking timer IRQ handling.  And I've never convinced myself
that Linux handles the hardware IRQ priority on those chips as
well as it could.  


> My point is that if we outlaw irqs-disabled shared interrupts, it puts
> Atmel AT91 support into immediate difficulties.

ISTR that those TCB modules don't share IRQs with other peripherals.

Also, that Linux doesn't use them for much else.  I've yet to see a
three-phase motor driver using the TCB's PWM capabilities, for example.

- Dave

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[David Brownell]

On Monday 30 November 2009, Alan Cox wrote:
> SHARED|DISABLED ought to WARN_ON() and if that doesn't motivate people
> then return -EINVAL. And with any luck that'll prove 6 months later that
> most of the offenders are not used and we can delete them wholesale.

So ... merge an updated version of the original patch, to
get full WARN coverage?

We've had that warning for a long time now.  The original
patch just covered non-request_irq() cases.  So by your
timetable we're ready for the "return -EINVAL" stage of
the migration... at least, for request_irq() callers.

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Andy Walls]

On Mon, 2009-11-30 at 12:38 -0800, David Brownell wrote:
> On Monday 30 November 2009, Alan Cox wrote:
> > SHARED|DISABLED ought to WARN_ON() and if that doesn't motivate people
> > then return -EINVAL. And with any luck that'll prove 6 months later that
> > most of the offenders are not used and we can delete them wholesale.
> 
> So ... merge an updated version of the original patch, to
> get full WARN coverage?
> 
> We've had that warning for a long time now.  The original
> patch just covered non-request_irq() cases.  So by your
> timetable we're ready for the "return -EINVAL" stage of
> the migration... at least, for request_irq() callers.

OK, I'm motivated.  I haven't followed the discussion closely though.

Can someone give me a clue as to the preferred way to correct this:

        /* Register IRQ */
        retval = request_irq(cx->pci_dev->irq, cx18_irq_handler,
                             IRQF_SHARED | IRQF_DISABLED,
                             cx->v4l2_dev.name, (void *)cx);


?
The top half handler performs as little work as it possibly can and
schedules the long duration activites on a workqueue already.

The device is always on a plug-in PCI card.

Regards,
Andy

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Benjamin Herrenschmidt]

On Mon, 2009-11-30 at 15:24 +0100, Thomas Gleixner wrote:
> 
> > Except I guess that will upset some of the IRQ priority folks, like
> > power, where they (iirc) have a stack per irq prio level.
> 
> Is that actually used ? Ben ?

Nah, we have 2 dedicated stacks.. one for external interrupts and one
for softirq. On embedded we also have a separate stack for the critical
interrupts and machine checks but both are special (critical interrupts
are aking to FIQ on ARM).

> > But its not like the core kernel knows about these nesting rules and
> can actually track any of that muck.
> 
> True.

Well, thing is, in cases where we have a sane PIC, the PIC itself is
perfectly good at keeping the source and anything of the same priority
or lower masked while we handle an irq.

So disabling local CPU IRQs will basically add an unnecessary blocking
of both timer interrupts and perfmon interrupts while doing so.

Yes, all driver interrupt handlers -should- be only running short amount
of code in their handlers but you know how it is. The drift introduced
on timer and perfmon events can be a problem, the later might even make
it difficult to figure out what an -interrupt- is taking more time than
it should.

I would suggest we timestamp the handlers in the core btw and warn if
they take too long so we get a chance to track down the bad guys.

Cheers,
Ben.

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Thomas Gleixner]

On Tue, 1 Dec 2009, Benjamin Herrenschmidt wrote:
> Well, thing is, in cases where we have a sane PIC, the PIC itself is
> perfectly good at keeping the source and anything of the same priority
> or lower masked while we handle an irq.

Unfortunately the majority of PICs does not fall into that category.
 
> So disabling local CPU IRQs will basically add an unnecessary blocking
> of both timer interrupts and perfmon interrupts while doing so.
> 
> Yes, all driver interrupt handlers -should- be only running short amount
> of code in their handlers but you know how it is. The drift introduced
> on timer and perfmon events can be a problem, the later might even make
> it difficult to figure out what an -interrupt- is taking more time than
> it should.

The timer problem only affects the old style tick/jiffies driven
hardware where you have no continous clock source for keeping track of
time. Even x86 managed to do something about that recently :)

Are the perf events on power generally coming through the standard irq
handler code path and/or sensitive to local_irq_disable() ?

> I would suggest we timestamp the handlers in the core btw and warn if
> they take too long so we get a chance to track down the bad guys.

The hassle is to find a time which we think is appropriate as a
threshold which is of course depending on the cpu power of a
system. Also I wonder whether we'd need to make such a warning thing
aware of irq nesting.

Thanks,

	tglx

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Benjamin Herrenschmidt]

On Mon, 2009-11-30 at 22:31 +0100, Thomas Gleixner wrote:

> Are the perf events on power generally coming through the standard irq
> handler code path and/or sensitive to local_irq_disable() ?

They are in HW yes. On ppc64, we do soft-disabling, which mean that we
can still get the perf events within a local_irq_disable() region
provided we don't get another interrupt within that region that forces
us to hard disable so it would make the problem less bad I suppose.

> > I would suggest we timestamp the handlers in the core btw and warn
> if
> > they take too long so we get a chance to track down the bad guys.
> 
> The hassle is to find a time which we think is appropriate as a
> threshold which is of course depending on the cpu power of a
> system. Also I wonder whether we'd need to make such a warning thing
> aware of irq nesting.

But if we always disable interrupts while running the handlers, we don't
nest right ?

Cheers,
Ben.

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Thomas Gleixner]

On Tue, 1 Dec 2009, Benjamin Herrenschmidt wrote:
> On Mon, 2009-11-30 at 22:31 +0100, Thomas Gleixner wrote:
> 
> > Are the perf events on power generally coming through the standard irq
> > handler code path and/or sensitive to local_irq_disable() ?
> 
> They are in HW yes. On ppc64, we do soft-disabling, which mean that we
> can still get the perf events within a local_irq_disable() region
> provided we don't get another interrupt within that region that forces
> us to hard disable so it would make the problem less bad I suppose.
> 
> > > I would suggest we timestamp the handlers in the core btw and warn
> > if
> > > they take too long so we get a chance to track down the bad guys.
> > 
> > The hassle is to find a time which we think is appropriate as a
> > threshold which is of course depending on the cpu power of a
> > system. Also I wonder whether we'd need to make such a warning thing
> > aware of irq nesting.
> 
> But if we always disable interrupts while running the handlers, we don't
> nest right ?

Right, in that case we do not and it's easy to instrument.

Thanks,

	tglx

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Russell King - ARM Linux]

On Mon, Nov 30, 2009 at 02:54:54PM +0100, Thomas Gleixner wrote:
> The correct solution IMNSHO is to get rid of IRQF_DISABLED and run
> interrupt handlers always with interrupts disabled and require them
> not to reenable interrupts themself.

I think Linus advocated at one point making the default be "irqs disabled"
and only only if a flag was passed would handlers run with IRQs enabled.
However, that might have been a step towards having all handlers running
with IRQs disabled.

I'm all in favour of it.  There are a large number of relatively simple
interrupt handlers out there which don't care about whether they're
called with IRQs disabled or not.

However, I think we still have a number of corner cases.  The SMC91x
driver comes to mind, with its stupidly small FIFOs, where the majority
of implementations have to have the packets loaded via PIO - and this
seems to generally happen from IRQ context.

The upshot of that is switching the SMC91x interrupt handler to run with
IRQs disabled will mean that serial can suffer with overruns, especially
if the serial port FIFO is also small.

The alternative is to push the "expensive" packet-loading parts of SMC91x
support into a tasklet, but that's probably going to impact performance
of the driver.

What I'm saying is that I think it's a good idea, but we should be
cautious about forcing a blanket change - to do so I believe risks creating
performance regressions.

Maybe a "safer" way forward is to go and find all those request_irq()
sites and add IRQF_DISABLED to them all, wait for regression reports and
selectively remove the IRQF_DISABLED flags?  We would then be able to
build up a picture of the problematical drivers that need to be reworked,
and whether the "run everything with irqs disabled" is even a practical
proposition.


Now, at the risk of covering old ground, how about we have two separate
irqaction lists, one for handlers to be called with irqs disabled and
one for handlers with irqs enabled.  We run the irqs-disabled list
first, naturally with irqs disabled.  If, at the end of that run (or
maybe after each handler), IRQs have ended being enabled, print some
diagnostics.  (We're going to need something like this to ensure that
drivers interrupt handlers don't enable IRQs themselves.)  Then enable
IRQs and run the irqs-enabled chain.

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Russell King - ARM Linux]

On Mon, Nov 30, 2009 at 02:37:03PM +0000, Russell King - ARM Linux wrote:
> Now, at the risk of covering old ground, how about we have two separate
> irqaction lists, one for handlers to be called with irqs disabled and
> one for handlers with irqs enabled.  We run the irqs-disabled list
> first, naturally with irqs disabled.  If, at the end of that run (or
> maybe after each handler), IRQs have ended being enabled, print some
> diagnostics.  (We're going to need something like this to ensure that
> drivers interrupt handlers don't enable IRQs themselves.)  Then enable
> IRQs and run the irqs-enabled chain.

Oh, and the other interesting thing to do may be to have a way of
measuring how much time irq handlers run for, so that handlers taking
an excessive time (more than 0.5ms or so - thinking about the 1000Hz
timer rate found on some arches) can be targetted.

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Thomas Gleixner]

On Mon, 30 Nov 2009, Russell King - ARM Linux wrote:

> On Mon, Nov 30, 2009 at 02:37:03PM +0000, Russell King - ARM Linux wrote:
> > Now, at the risk of covering old ground, how about we have two separate
> > irqaction lists, one for handlers to be called with irqs disabled and
> > one for handlers with irqs enabled.  We run the irqs-disabled list
> > first, naturally with irqs disabled.  If, at the end of that run (or
> > maybe after each handler), IRQs have ended being enabled, print some
> > diagnostics.  (We're going to need something like this to ensure that
> > drivers interrupt handlers don't enable IRQs themselves.)  Then enable
> > IRQs and run the irqs-enabled chain.
> 
> Oh, and the other interesting thing to do may be to have a way of
> measuring how much time irq handlers run for, so that handlers taking
> an excessive time (more than 0.5ms or so - thinking about the 1000Hz
> timer rate found on some arches) can be targetted.

.33 will have trace points for this, so the infrastructure is there or
do you think about something permanent which does not depend on the
tracer ?

Thanks,

	tglx

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Alan Cox]

> However, I think we still have a number of corner cases.  The SMC91x
> driver comes to mind, with its stupidly small FIFOs, where the majority
> of implementations have to have the packets loaded via PIO - and this
> seems to generally happen from IRQ context.

Everything 8390 based is in the same boat. It relies on being able to
use disable_irq_nosync/enable_irq and knows all about the joys of
interrupt bus asynchronicity internally. That does however allow it to
get sane results by using the irq controller to mask the potentially
shared IRQ at source.

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Thomas Gleixner]

On Mon, 30 Nov 2009, Alan Cox wrote:

> > However, I think we still have a number of corner cases.  The SMC91x
> > driver comes to mind, with its stupidly small FIFOs, where the majority
> > of implementations have to have the packets loaded via PIO - and this
> > seems to generally happen from IRQ context.
> 
> Everything 8390 based is in the same boat. It relies on being able to
> use disable_irq_nosync/enable_irq and knows all about the joys of
> interrupt bus asynchronicity internally. That does however allow it to
> get sane results by using the irq controller to mask the potentially
> shared IRQ at source.

So that would be a known candidate for IRQF_NEEDS_IRQS_ENABLED, right?

Either that or we decide to push such beasts into the threaded irq
space to keep them working until the last card hits the trashcan. I
know that this would still need to disable the interrupt on the PIC
level, but we have already mechanisms for that in the threaded code.

Thanks,

	tglx

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Alan Cox]

> Either that or we decide to push such beasts into the threaded irq
> space to keep them working until the last card hits the trashcan. I
> know that this would still need to disable the interrupt on the PIC
> level, but we have already mechanisms for that in the threaded code.

The 8390 is essentially a single thread device so treating interrupts as
events indicating work is to be done might make sense

Unfortunately you cannot check the interrupt flags on the chip without
switching to page 0, which will cause any parallel tx to crap itself and
potentially hang the box. It's a design from single CPU days and the
programming model is solely around 'stack the register window selected,
do stuff in irq, put it back', so any parallel execution ends in tears,
even peeking to see if the IRQ is ours.

These chips still keep popping up in old boxes although the rtl8139 seems
to have exterminated them at last in all the ultra-cheap devices.

Pushing them into threaded IRQ space with PIC masking seems to make
complete sense. The wonderously gothic IRQ magic becomes a mutex, the IRQ
handler may sleep blocking the IRQ during a transmit and the transmit
path may block during an IRQ thread execution. Reset works as a mutex and
all the crap and magic goes away.

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Nicolas Pitre]

On Mon, 30 Nov 2009, Russell King - ARM Linux wrote:

> However, I think we still have a number of corner cases.  The SMC91x
> driver comes to mind, with its stupidly small FIFOs, where the majority
> of implementations have to have the packets loaded via PIO - and this
> seems to generally happen from IRQ context.

That is indeed the case for the RX path in order to avoid packet drop as 
much as possible.  The TX path is always run outside IRQ context though.

> The upshot of that is switching the SMC91x interrupt handler to run with
> IRQs disabled will mean that serial can suffer with overruns, especially
> if the serial port FIFO is also small.

That can happen now already anyway, regardless of whether IRQ handlers 
are run with IRQs enabled or not.  Suffice to have serial and smc91x 
interrupts asserted more or less at the same time, i.e. before the 
pending interrupt sources are actually determined and interrupts enabled 
again, in which case the IRQ handlers are serialized usually according 
to their IRQ number (most target don't use sophisticated IRQ 
priorities).

And then, the serial interrupt isn't currently registered with 
IRQF_DISABLED, meaning that its handler can be interrupted by any other 
interrupt coming along, including the heavier smc91x RX code.  That 
isn't much different from having all interrupt handlers run with IRQs 
disabled and the serial interrupt having to wait after the smc91x one.

In other words, I don't think having all IRQ handlers run with IRQs 
disabled would change much wrt average IRQ latency in practice.  
Without real hardware based IRQ priority management (or thread based IRQ 
handlers with software managed priorities), The smc91x handler could 
adversely affect the serial handler in either cases.


Nicolas

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Thomas Gleixner]

On Mon, 30 Nov 2009, Russell King - ARM Linux wrote:
> On Mon, Nov 30, 2009 at 02:54:54PM +0100, Thomas Gleixner wrote:
> Maybe a "safer" way forward is to go and find all those request_irq()
> sites and add IRQF_DISABLED to them all, wait for regression reports and
> selectively remove the IRQF_DISABLED flags?  We would then be able to
> build up a picture of the problematical drivers that need to be reworked,
> and whether the "run everything with irqs disabled" is even a practical
> proposition.

Well, that's basically the same as removing IRQF_DISABLED, setting the
default to run disabled and provide a new flag IRQF_ENABLE_IRQS which
can be added to drivers when regressions show up. That way you just
have to grep for IRQF_ENABLE_IRQS instead of doing a search for
everything which has it removed. We can keep IRQF_DISABLED around by
setting it to 0 for the transition, so we don't have to touch the
world in the first place.
 
> Now, at the risk of covering old ground, how about we have two separate
> irqaction lists, one for handlers to be called with irqs disabled and
> one for handlers with irqs enabled.  We run the irqs-disabled list
> first, naturally with irqs disabled.  If, at the end of that run (or
> maybe after each handler), IRQs have ended being enabled, print some
> diagnostics.  (We're going to need something like this to ensure that
> drivers interrupt handlers don't enable IRQs themselves.)  Then enable
> IRQs and run the irqs-enabled chain.

Pretty old ground. :) That was discussed 10 years ago and never found
much love, but yes we could do that. Either two list or sorting the
entries. That might avoid the obvious pitfall, but I doubt it'd help
us to fix the drivers which think that they need to do the irqs
enabled dance.

Thanks,

	tglx

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Uwe Kleine-König]

Hello,

On Mon, Nov 30, 2009 at 02:54:54PM +0100, Thomas Gleixner wrote:
> On Mon, 30 Nov 2009, Uwe Kleine-K?nig wrote:
> > For shared irqs IRQF_DISABLED is only guaranteed for the first handler.
> > So only warn starting at the second registration.
> > 
> > The warning is moved to __setup_irq having the additional benefit of
> > catching actions registered using setup_irq not only register_irq.
> > 
> > This doesn't fix the cases where setup order is wrong but it should
> > report the broken cases more reliably.
> 
> The whole IRQF_DISABLED trickery is questionable and I'm pretty
> unhappy about the warning in general.
> 
> While it is true that there is no guarantee of IRQF_DISABLED on shared
> interrupts (at least not for the secondary handlers) we really need to
> think about the reason why we want to run interrupt handlers with
> interrupts enabled at all.
> 
> The separation of interrupt handlers which run with interrupts
> disabled/enabled goes all the way back to Linux 1.0, which had two
> interrupt handling modes:
> 
> 1) handlers installed with SA_INTERRUPT ran atomically with interrupts
>    disabled.
> 
> 2) handlers installed without SA_INTERRUPT ran with interrupts enabled
>    as they did more complex stuff like signal handling in the kernel.

> The interrupt which was always run with interrupts disabled was the
> timer interrupt because some of the "slower" interrupt handlers were
> relying on jiffies being updated, which is only possible when they run
> with interrupts enabled and no such handler can interrupt the timer
> interrupt.
> 
> In the 2.1.x timeframe the discussion about shared interrupt handlers
> and the treatment of SA_INTERRUPT (today IRQF_DISABLED) was resolved
> by changing the code to what we have right now. If you read back in
> the archives you will find the same arguments as we have seen in this
> thread and a boatload of different solutions to that.
> 
> The real question is why we want to run an interrupt handler with
> interrupts enabled at all. There are two reaons AFAICT:
> 
> 1) interrupt handler relies on jiffies being updated:
> 
>    I don't think that this is the case anymore and if we still have
>    code which does it is probably historic crap which is unused for
>    quite a time.
> 
> 2) interrupt handler runs a long time:
> 
>    I'm sure we still have some of those especially in the
>    archaelogical corners of drivers/* and in the creative space of the
>    embedded "oh, I don't know why but it works" departement. That's
>    code which needs to be fixed anyway.

I think there is

 3) you can only benefit from decent priority hardware if irqs are
    processed while irqs are enabled.

I think 

	git grep handle_fasteoi_irq

gives an overview here: some hits in arch/powerpc, arch/sparc and
arch/x86/kernel/apic/io_apic.c.  (There is handle_prio_irq in
arch/arm/mach-ns9xxx, but the priodecoder is crappy and actually it
should use handle_level_irq IIRC.)

Best regards
Uwe
 
-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

Get rid of IRQF_DISABLED - (was [PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED)

[Thomas Gleixner]

On Mon, 30 Nov 2009, Uwe Kleine-K?nig wrote:
> I think there is
> 
>  3) you can only benefit from decent priority hardware if irqs are
>     processed while irqs are enabled.
> 
> I think 
> 
> 	git grep handle_fasteoi_irq
> 
> gives an overview here: some hits in arch/powerpc, arch/sparc and
> arch/x86/kernel/apic/io_apic.c.  (There is handle_prio_irq in

No. That handler is not an indicator for prio hardware actively used
in the sense of allowing higher prio interrupts to interrupt a current
running lower priority one. It can be used when the irq controller
does not fire the interrupt again before the eoi acknowledge has been
done.

Thanks,

	tglx

[PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED at the right place

[David Brownell]

On Monday 30 November 2009, Uwe Kleine-K?nig wrote:
> +???????????????if (new->flags & IRQF_DISABLED)
> +???????????????????????pr_warning("IRQ %d/%s: IRQF_DISABLED is not guaranteed "
> +???????????????????????????????????????"on shared IRQs\n", irq, new->name);

This should have copied the original test ... this way,
it's dropping the SHARED constraint, and trying to morph
into a generic "IRQF_DISABLED is eeebil!" test.

If it just moved the original test, I'd have no problem
with the patch.


... although it'd still not address the general mess in
this area, it'd at least not introduce false warnings.

[PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED at the right place

[Uwe Kleine-König]

On Mon, Nov 30, 2009 at 12:21:30PM -0800, David Brownell wrote:
> On Monday 30 November 2009, Uwe Kleine-K?nig wrote:
> > +???????????????if (new->flags & IRQF_DISABLED)
> > +???????????????????????pr_warning("IRQ %d/%s: IRQF_DISABLED is not guaranteed "
> > +???????????????????????????????????????"on shared IRQs\n", irq, new->name);
> 
> This should have copied the original test ... this way,
> it's dropping the SHARED constraint, and trying to morph
> into a generic "IRQF_DISABLED is eeebil!" test.
No, it doesn't.  The inserted code is in an if block:

	old_ptr = &desc->action;
	old = *old_ptr;
	if (old) {
		/* ... */
		if (!((old->flags & new->flags) & IRQF_SHARED) ||
			((old->flags ^ new->flags) & IRQF_TRIGGER_MASK)) {
			old_name = old->name;
			goto mismatch;
		}

		...

+		if (new->flags & IRQF_DISABLED)
+			pr_warning("...");

and the mismatch label is further below.  So the warning is still only
hit if a shared irq is registered.

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[RESEND PATCH] genirq: warn about IRQF_SHARED|IRQF_DISABLED at the right place

[Uwe Kleine-König]

For shared irqs IRQF_DISABLED is only guaranteed for the first handler.
So only warn starting at the second registration.

The warning is moved to __setup_irq having the additional benefit of
catching actions registered using setup_irq not only register_irq.

This doesn't fix the cases where setup order is wrong but it should
report the broken cases more reliably.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Cc: David Brownell <dbrownell@users.sourceforge.net>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Nicolas Pitre <nico@marvell.com>
Cc: Eric Miao <eric.y.miao@gmail.com>
Cc: John Stultz <johnstul@us.ibm.com>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Remy Bohmer <linux@bohmer.net>
Cc: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Cc: Andrea Gallo <andrea.gallo@stericsson.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Jamie Lokier <jamie@shareable.org>
Cc: linux-arm-kernel at lists.infradead.org
---
Hello,

until Thomas or Peter get round to dump IRQF_DISABLED completely this
patch removes some false positive warnings and adds a valid warning in
some situations that weren't catched before.

Best regards
Uwe

 kernel/irq/manage.c |   23 ++++++++++-------------
 1 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index bde4c66..d8f7415 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -702,6 +702,16 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 			goto mismatch;
 #endif
 
+		/*
+		 * handle_IRQ_event() only honors IRQF_DISABLED for the _first_
+		 * irqaction.  As the first handler might reenable irqs all bets
+		 * are off for the later handlers even if the first one has
+		 * IRQF_DISABLED, too.
+		 */
+		if (new->flags & IRQF_DISABLED)
+			pr_warning("IRQ %d/%s: IRQF_DISABLED is not guaranteed "
+					"on shared IRQs\n", irq, new->name);
+
 		/* add new interrupt at end of irq queue */
 		do {
 			old_ptr = &old->next;
@@ -1008,19 +1018,6 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 	struct irq_desc *desc;
 	int retval;
 
-	/*
-	 * handle_IRQ_event() always ignores IRQF_DISABLED except for
-	 * the _first_ irqaction (sigh).  That can cause oopsing, but
-	 * the behavior is classified as "will not fix" so we need to
-	 * start nudging drivers away from using that idiom.
-	 */
-	if ((irqflags & (IRQF_SHARED|IRQF_DISABLED)) ==
-					(IRQF_SHARED|IRQF_DISABLED)) {
-		pr_warning(
-		  "IRQ %d/%s: IRQF_DISABLED is not guaranteed on shared IRQs\n",
-			irq, devname);
-	}
-
 #ifdef CONFIG_LOCKDEP
 	/*
 	 * Lockdep wants atomic interrupt handlers:
-- 
1.6.6

[RESENT PATCH] Don't disable irqs in set_next_event and set_mode callbacks

[Eric Miao]

> Hello,
>
> @Eric: You wrote you were not sure about this patch. ?Would you prefer adding a
> WARN_ON in a first step?
>

Yes, that will be a good idea.

> @Remy: You planned to test this back in September. ?Did you?
>
> Best regards
> Uwe
>
> ?arch/arm/mach-at91/at91rm9200_time.c ?| ? 14 --------------
> ?arch/arm/mach-at91/at91sam926x_time.c | ? ?6 +-----
> ?arch/arm/mach-nomadik/timer.c ? ? ? ? | ? ?9 +--------
> ?arch/arm/mach-pxa/time.c ? ? ? ? ? ? ?| ? 10 +---------
> ?arch/arm/mach-sa1100/time.c ? ? ? ? ? | ? ?8 +-------
> ?5 files changed, 4 insertions(+), 43 deletions(-)
>
> diff --git a/arch/arm/mach-at91/at91rm9200_time.c b/arch/arm/mach-at91/at91rm9200_time.c
> index 309f351..f27ccf6 100644
> --- a/arch/arm/mach-at91/at91rm9200_time.c
> +++ b/arch/arm/mach-at91/at91rm9200_time.c
> @@ -132,24 +132,11 @@ clkevt32k_mode(enum clock_event_mode mode, struct clock_event_device *dev)
> ?static int
> ?clkevt32k_next_event(unsigned long delta, struct clock_event_device *dev)
> ?{
> - ? ? ? unsigned long ? flags;
> ? ? ? ?u32 ? ? ? ? ? ? alm;
> ? ? ? ?int ? ? ? ? ? ? status = 0;
>
> ? ? ? ?BUG_ON(delta < 2);
>
> - ? ? ? /* Use "raw" primitives so we behave correctly on RT kernels. */
> - ? ? ? raw_local_irq_save(flags);
> -
> - ? ? ? /*
> - ? ? ? ?* According to Thomas Gleixner irqs are already disabled here. ?Simply
> - ? ? ? ?* removing raw_local_irq_save above (and the matching
> - ? ? ? ?* raw_local_irq_restore) was not accepted. ?See
> - ? ? ? ?* http://thread.gmane.org/gmane.linux.ports.arm.kernel/41174
> - ? ? ? ?* So for now (2008-11-20) just warn once if irqs were not disabled ...
> - ? ? ? ?*/
> - ? ? ? WARN_ON_ONCE(!raw_irqs_disabled_flags(flags));
> -
> ? ? ? ?/* The alarm IRQ uses absolute time (now+delta), not the relative
> ? ? ? ? * time (delta) in our calling convention. ?Like all clockevents
> ? ? ? ? * using such "match" hardware, we have a race to defend against.
> @@ -169,7 +156,6 @@ clkevt32k_next_event(unsigned long delta, struct clock_event_device *dev)
> ? ? ? ?alm += delta;
> ? ? ? ?at91_sys_write(AT91_ST_RTAR, alm);
>
> - ? ? ? raw_local_irq_restore(flags);
> ? ? ? ?return status;
> ?}
>
> diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
> index 4bd56ae..c26cd6e 100644
> --- a/arch/arm/mach-at91/at91sam926x_time.c
> +++ b/arch/arm/mach-at91/at91sam926x_time.c
> @@ -62,16 +62,12 @@ static struct clocksource pit_clk = {
> ?static void
> ?pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev)
> ?{
> - ? ? ? unsigned long ? flags;
> -
> ? ? ? ?switch (mode) {
> ? ? ? ?case CLOCK_EVT_MODE_PERIODIC:
> - ? ? ? ? ? ? ? /* update clocksource counter, then enable the IRQ */
> - ? ? ? ? ? ? ? raw_local_irq_save(flags);
> + ? ? ? ? ? ? ? /* update clocksource counter */
> ? ? ? ? ? ? ? ?pit_cnt += pit_cycle * PIT_PICNT(at91_sys_read(AT91_PIT_PIVR));
> ? ? ? ? ? ? ? ?at91_sys_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?| AT91_PIT_PITIEN);
> - ? ? ? ? ? ? ? raw_local_irq_restore(flags);
> ? ? ? ? ? ? ? ?break;
> ? ? ? ?case CLOCK_EVT_MODE_ONESHOT:
> ? ? ? ? ? ? ? ?BUG();
> diff --git a/arch/arm/mach-nomadik/timer.c b/arch/arm/mach-nomadik/timer.c
> index d1738e7..e361be6 100644
> --- a/arch/arm/mach-nomadik/timer.c
> +++ b/arch/arm/mach-nomadik/timer.c
> @@ -54,24 +54,17 @@ static struct clocksource nmdk_clksrc = {
> ?static void nmdk_clkevt_mode(enum clock_event_mode mode,
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? struct clock_event_device *dev)
> ?{
> - ? ? ? unsigned long flags;
> -
> ? ? ? ?switch (mode) {
> ? ? ? ?case CLOCK_EVT_MODE_PERIODIC:
> - ? ? ? ? ? ? ? /* enable interrupts -- and count current value? */
> - ? ? ? ? ? ? ? raw_local_irq_save(flags);
> + ? ? ? ? ? ? ? /* count current value? */
> ? ? ? ? ? ? ? ?writel(readl(mtu_base + MTU_IMSC) | 1, mtu_base + MTU_IMSC);
> - ? ? ? ? ? ? ? raw_local_irq_restore(flags);
> ? ? ? ? ? ? ? ?break;
> ? ? ? ?case CLOCK_EVT_MODE_ONESHOT:
> ? ? ? ? ? ? ? ?BUG(); /* Not supported, yet */
> ? ? ? ? ? ? ? ?/* FALLTHROUGH */
> ? ? ? ?case CLOCK_EVT_MODE_SHUTDOWN:
> ? ? ? ?case CLOCK_EVT_MODE_UNUSED:
> - ? ? ? ? ? ? ? /* disable irq */
> - ? ? ? ? ? ? ? raw_local_irq_save(flags);
> ? ? ? ? ? ? ? ?writel(readl(mtu_base + MTU_IMSC) & ~1, mtu_base + MTU_IMSC);
> - ? ? ? ? ? ? ? raw_local_irq_restore(flags);
> ? ? ? ? ? ? ? ?break;
> ? ? ? ?case CLOCK_EVT_MODE_RESUME:
> ? ? ? ? ? ? ? ?break;
> diff --git a/arch/arm/mach-pxa/time.c b/arch/arm/mach-pxa/time.c
> index 750c448..293e40a 100644
> --- a/arch/arm/mach-pxa/time.c
> +++ b/arch/arm/mach-pxa/time.c
> @@ -76,14 +76,12 @@ pxa_ost0_interrupt(int irq, void *dev_id)
> ?static int
> ?pxa_osmr0_set_next_event(unsigned long delta, struct clock_event_device *dev)
> ?{
> - ? ? ? unsigned long flags, next, oscr;
> + ? ? ? unsigned long next, oscr;
>
> - ? ? ? raw_local_irq_save(flags);
> ? ? ? ?OIER |= OIER_E0;
> ? ? ? ?next = OSCR + delta;
> ? ? ? ?OSMR0 = next;
> ? ? ? ?oscr = OSCR;
> - ? ? ? raw_local_irq_restore(flags);
>
> ? ? ? ?return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0;
> ?}
> @@ -91,23 +89,17 @@ pxa_osmr0_set_next_event(unsigned long delta, struct clock_event_device *dev)
> ?static void
> ?pxa_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *dev)
> ?{
> - ? ? ? unsigned long irqflags;
> -
> ? ? ? ?switch (mode) {
> ? ? ? ?case CLOCK_EVT_MODE_ONESHOT:
> - ? ? ? ? ? ? ? raw_local_irq_save(irqflags);
> ? ? ? ? ? ? ? ?OIER &= ~OIER_E0;
> ? ? ? ? ? ? ? ?OSSR = OSSR_M0;
> - ? ? ? ? ? ? ? raw_local_irq_restore(irqflags);
> ? ? ? ? ? ? ? ?break;
>
> ? ? ? ?case CLOCK_EVT_MODE_UNUSED:
> ? ? ? ?case CLOCK_EVT_MODE_SHUTDOWN:
> ? ? ? ? ? ? ? ?/* initializing, released, or preparing for suspend */
> - ? ? ? ? ? ? ? raw_local_irq_save(irqflags);
> ? ? ? ? ? ? ? ?OIER &= ~OIER_E0;
> ? ? ? ? ? ? ? ?OSSR = OSSR_M0;
> - ? ? ? ? ? ? ? raw_local_irq_restore(irqflags);
> ? ? ? ? ? ? ? ?break;
>
> ? ? ? ?case CLOCK_EVT_MODE_RESUME:
> diff --git a/arch/arm/mach-sa1100/time.c b/arch/arm/mach-sa1100/time.c
> index b9cbb56..74b6e0e 100644
> --- a/arch/arm/mach-sa1100/time.c
> +++ b/arch/arm/mach-sa1100/time.c
> @@ -35,14 +35,12 @@ static irqreturn_t sa1100_ost0_interrupt(int irq, void *dev_id)
> ?static int
> ?sa1100_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c)
> ?{
> - ? ? ? unsigned long flags, next, oscr;
> + ? ? ? unsigned long next, oscr;
>
> - ? ? ? raw_local_irq_save(flags);
> ? ? ? ?OIER |= OIER_E0;
> ? ? ? ?next = OSCR + delta;
> ? ? ? ?OSMR0 = next;
> ? ? ? ?oscr = OSCR;
> - ? ? ? raw_local_irq_restore(flags);
>
> ? ? ? ?return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0;
> ?}
> @@ -50,16 +48,12 @@ sa1100_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c)
> ?static void
> ?sa1100_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c)
> ?{
> - ? ? ? unsigned long flags;
> -
> ? ? ? ?switch (mode) {
> ? ? ? ?case CLOCK_EVT_MODE_ONESHOT:
> ? ? ? ?case CLOCK_EVT_MODE_UNUSED:
> ? ? ? ?case CLOCK_EVT_MODE_SHUTDOWN:
> - ? ? ? ? ? ? ? raw_local_irq_save(flags);
> ? ? ? ? ? ? ? ?OIER &= ~OIER_E0;
> ? ? ? ? ? ? ? ?OSSR = OSSR_M0;
> - ? ? ? ? ? ? ? raw_local_irq_restore(flags);
> ? ? ? ? ? ? ? ?break;
>
> ? ? ? ?case CLOCK_EVT_MODE_RESUME:
> --
> 1.6.5.2
>
>

[PATCH 1/2] arm/at91: Don't disable irqs in set_next_event and set_mode callbacks

[Uwe Kleine-König]

on AT91 the timer irq is shared, so the handler might be entered without
irqs being disabled.  Though this should not happen as the timer irq is
registered early, there have been some reports on the mailing list.

To make debugging that problem easier next time it pops up a WARN_ON is
added in the handler if irqs are not off.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Magnus Damm <damm@opensource.se>
Cc: Hugh Dickins <hugh@veritas.com>
Cc: John Stultz <johnstul@us.ibm.com>
---
 arch/arm/mach-at91/at91rm9200_time.c  |   20 ++++++--------------
 arch/arm/mach-at91/at91sam926x_time.c |   11 ++++++-----
 2 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-at91/at91rm9200_time.c b/arch/arm/mach-at91/at91rm9200_time.c
index 309f351..f494ce9 100644
--- a/arch/arm/mach-at91/at91rm9200_time.c
+++ b/arch/arm/mach-at91/at91rm9200_time.c
@@ -58,6 +58,12 @@ static irqreturn_t at91rm9200_timer_interrupt(int irq, void *dev_id)
 {
 	u32	sr = at91_sys_read(AT91_ST_SR) & irqmask;
 
+	/*
+	 * 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(!irqs_disabled());
+
 	/* simulate "oneshot" timer with alarm */
 	if (sr & AT91_ST_ALMS) {
 		clkevt.event_handler(&clkevt);
@@ -132,24 +138,11 @@ clkevt32k_mode(enum clock_event_mode mode, struct clock_event_device *dev)
 static int
 clkevt32k_next_event(unsigned long delta, struct clock_event_device *dev)
 {
-	unsigned long	flags;
 	u32		alm;
 	int		status = 0;
 
 	BUG_ON(delta < 2);
 
-	/* Use "raw" primitives so we behave correctly on RT kernels. */
-	raw_local_irq_save(flags);
-
-	/*
-	 * According to Thomas Gleixner irqs are already disabled here.  Simply
-	 * removing raw_local_irq_save above (and the matching
-	 * raw_local_irq_restore) was not accepted.  See
-	 * http://thread.gmane.org/gmane.linux.ports.arm.kernel/41174
-	 * So for now (2008-11-20) just warn once if irqs were not disabled ...
-	 */
-	WARN_ON_ONCE(!raw_irqs_disabled_flags(flags));
-
 	/* The alarm IRQ uses absolute time (now+delta), not the relative
 	 * time (delta) in our calling convention.  Like all clockevents
 	 * using such "match" hardware, we have a race to defend against.
@@ -169,7 +162,6 @@ clkevt32k_next_event(unsigned long delta, struct clock_event_device *dev)
 	alm += delta;
 	at91_sys_write(AT91_ST_RTAR, alm);
 
-	raw_local_irq_restore(flags);
 	return status;
 }
 
diff --git a/arch/arm/mach-at91/at91sam926x_time.c b/arch/arm/mach-at91/at91sam926x_time.c
index 4bd56ae..786f172 100644
--- a/arch/arm/mach-at91/at91sam926x_time.c
+++ b/arch/arm/mach-at91/at91sam926x_time.c
@@ -62,16 +62,12 @@ static struct clocksource pit_clk = {
 static void
 pit_clkevt_mode(enum clock_event_mode mode, struct clock_event_device *dev)
 {
-	unsigned long	flags;
-
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
-		/* update clocksource counter, then enable the IRQ */
-		raw_local_irq_save(flags);
+		/* update clocksource counter */
 		pit_cnt += pit_cycle * PIT_PICNT(at91_sys_read(AT91_PIT_PIVR));
 		at91_sys_write(AT91_PIT_MR, (pit_cycle - 1) | AT91_PIT_PITEN
 				| AT91_PIT_PITIEN);
-		raw_local_irq_restore(flags);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
 		BUG();
@@ -100,6 +96,11 @@ static struct clock_event_device pit_clkevt = {
  */
 static irqreturn_t at91sam926x_pit_interrupt(int irq, void *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(!irqs_disabled());
 
 	/* The PIT interrupt may be disabled, and is shared */
 	if ((pit_clkevt.mode == CLOCK_EVT_MODE_PERIODIC)
-- 
1.6.5.2

[PATCH 2/2] arm/{pxa, sa1100, nomadik}: Don't disable irqs in set_next_event and set_mode

[Uwe Kleine-König]

These functions are called with irqs already off.  This commit removes
the calls to raw_local_irq_save and raw_local_irq_restore on platforms
that don't have to use a shared interrupt for their timekeeping.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
Cc: Eric Miao <eric.y.miao@gmail.com>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Nicolas Pitre <nico@marvell.com>
Cc: Magnus Damm <damm@opensource.se>
Cc: Alessandro Rubini <rubini@unipv.it>
Cc: STEricsson <STEricsson_nomadik_linux@list.st.com>
Cc: srinidhi kasagar <srinidhi.kasagar@stericsson.com>
---
Hello,

@Eric, IIRC you stoped objecting to just remove the irq saving after
Russell's analysis, but I cannot find your mail.  Do I remember right?

Best regards
Uwe

 arch/arm/mach-pxa/time.c      |   10 +---------
 arch/arm/mach-sa1100/time.c   |    8 +-------
 arch/arm/plat-nomadik/timer.c |    9 +--------
 3 files changed, 3 insertions(+), 24 deletions(-)

diff --git a/arch/arm/mach-pxa/time.c b/arch/arm/mach-pxa/time.c
index 750c448..293e40a 100644
--- a/arch/arm/mach-pxa/time.c
+++ b/arch/arm/mach-pxa/time.c
@@ -76,14 +76,12 @@ pxa_ost0_interrupt(int irq, void *dev_id)
 static int
 pxa_osmr0_set_next_event(unsigned long delta, struct clock_event_device *dev)
 {
-	unsigned long flags, next, oscr;
+	unsigned long next, oscr;
 
-	raw_local_irq_save(flags);
 	OIER |= OIER_E0;
 	next = OSCR + delta;
 	OSMR0 = next;
 	oscr = OSCR;
-	raw_local_irq_restore(flags);
 
 	return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0;
 }
@@ -91,23 +89,17 @@ pxa_osmr0_set_next_event(unsigned long delta, struct clock_event_device *dev)
 static void
 pxa_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *dev)
 {
-	unsigned long irqflags;
-
 	switch (mode) {
 	case CLOCK_EVT_MODE_ONESHOT:
-		raw_local_irq_save(irqflags);
 		OIER &= ~OIER_E0;
 		OSSR = OSSR_M0;
-		raw_local_irq_restore(irqflags);
 		break;
 
 	case CLOCK_EVT_MODE_UNUSED:
 	case CLOCK_EVT_MODE_SHUTDOWN:
 		/* initializing, released, or preparing for suspend */
-		raw_local_irq_save(irqflags);
 		OIER &= ~OIER_E0;
 		OSSR = OSSR_M0;
-		raw_local_irq_restore(irqflags);
 		break;
 
 	case CLOCK_EVT_MODE_RESUME:
diff --git a/arch/arm/mach-sa1100/time.c b/arch/arm/mach-sa1100/time.c
index b9cbb56..74b6e0e 100644
--- a/arch/arm/mach-sa1100/time.c
+++ b/arch/arm/mach-sa1100/time.c
@@ -35,14 +35,12 @@ static irqreturn_t sa1100_ost0_interrupt(int irq, void *dev_id)
 static int
 sa1100_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c)
 {
-	unsigned long flags, next, oscr;
+	unsigned long next, oscr;
 
-	raw_local_irq_save(flags);
 	OIER |= OIER_E0;
 	next = OSCR + delta;
 	OSMR0 = next;
 	oscr = OSCR;
-	raw_local_irq_restore(flags);
 
 	return (signed)(next - oscr) <= MIN_OSCR_DELTA ? -ETIME : 0;
 }
@@ -50,16 +48,12 @@ sa1100_osmr0_set_next_event(unsigned long delta, struct clock_event_device *c)
 static void
 sa1100_osmr0_set_mode(enum clock_event_mode mode, struct clock_event_device *c)
 {
-	unsigned long flags;
-
 	switch (mode) {
 	case CLOCK_EVT_MODE_ONESHOT:
 	case CLOCK_EVT_MODE_UNUSED:
 	case CLOCK_EVT_MODE_SHUTDOWN:
-		raw_local_irq_save(flags);
 		OIER &= ~OIER_E0;
 		OSSR = OSSR_M0;
-		raw_local_irq_restore(flags);
 		break;
 
 	case CLOCK_EVT_MODE_RESUME:
diff --git a/arch/arm/plat-nomadik/timer.c b/arch/arm/plat-nomadik/timer.c
index 62f18ad..fa7cb3a 100644
--- a/arch/arm/plat-nomadik/timer.c
+++ b/arch/arm/plat-nomadik/timer.c
@@ -49,24 +49,17 @@ static struct clocksource nmdk_clksrc = {
 static void nmdk_clkevt_mode(enum clock_event_mode mode,
 			     struct clock_event_device *dev)
 {
-	unsigned long flags;
-
 	switch (mode) {
 	case CLOCK_EVT_MODE_PERIODIC:
-		/* enable interrupts -- and count current value? */
-		raw_local_irq_save(flags);
+		/* count current value? */
 		writel(readl(mtu_base + MTU_IMSC) | 1, mtu_base + MTU_IMSC);
-		raw_local_irq_restore(flags);
 		break;
 	case CLOCK_EVT_MODE_ONESHOT:
 		BUG(); /* Not supported, yet */
 		/* FALLTHROUGH */
 	case CLOCK_EVT_MODE_SHUTDOWN:
 	case CLOCK_EVT_MODE_UNUSED:
-		/* disable irq */
-		raw_local_irq_save(flags);
 		writel(readl(mtu_base + MTU_IMSC) & ~1, mtu_base + MTU_IMSC);
-		raw_local_irq_restore(flags);
 		break;
 	case CLOCK_EVT_MODE_RESUME:
 		break;
-- 
1.6.5.2

[PATCH 1/2] arm/at91: Don't disable irqs in set_next_event and set_mode callbacks

[Uwe Kleine-König]

Hello Russell,

I havn't got any more feedback but I think these are OK after the last
comments.  Do you take them?

You can pull them from

	git://git.pengutronix.de/git/ukl/linux-2.6.git arm/clock-event

Best regards
Uwe

-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[PATCH 1/2] arm/at91: Don't disable irqs in set_next_event and set_mode callbacks

[Russell King - ARM Linux]

On Fri, Jan 22, 2010 at 05:08:24PM +0100, Uwe Kleine-K?nig wrote:
> Hello Russell,
> 
> I havn't got any more feedback but I think these are OK after the last
> comments.  Do you take them?
> 
> You can pull them from
> 
> 	git://git.pengutronix.de/git/ukl/linux-2.6.git arm/clock-event

I really don't like pulling git urls which I don't know what I'm pulling.
Please include at least a diffstat and shortlog of the commits you're
expecting to be sent.

Something like the output from git request-pull would be ideal.

[PATCH 1/2] arm/at91: Don't disable irqs in set_next_event and set_mode callbacks

[Uwe Kleine-König]

Hello Russell,

On Fri, Jan 22, 2010 at 04:36:58PM +0000, Russell King - ARM Linux wrote:
> On Fri, Jan 22, 2010 at 05:08:24PM +0100, Uwe Kleine-K?nig wrote:
> > You can pull them from
> > 
> > 	git://git.pengutronix.de/git/ukl/linux-2.6.git arm/clock-event
> 
> I really don't like pulling git urls which I don't know what I'm pulling.
> Please include at least a diffstat and shortlog of the commits you're
> expecting to be sent.
> 
> Something like the output from git request-pull would be ideal.
OK, appended below.  Actually I expected you to make me sending them to
the patch system.  But as I prefer when you pull from me, I'm happy to
provide you with diffstat and shortlog.

Best regards
Uwe

The following changes since commit 066000dd856709b6980123eb39b957fe26993f7b:
  Ananth N Mavinakayanahalli (1):
        Revert "x86, apic: Use logical flat on intel with <= 8 logical cpus"

are available in the git repository at:

  git://git.pengutronix.de/git/ukl/linux-2.6.git arm/clock-event

Uwe Kleine-K?nig (2):
      arm/at91: Don't disable irqs in set_next_event and set_mode callbacks
      arm/{pxa,sa1100,nomadik}: Don't disable irqs in set_next_event and set_mode

 arch/arm/mach-at91/at91rm9200_time.c  |   20 ++++++--------------
 arch/arm/mach-at91/at91sam926x_time.c |   11 ++++++-----
 arch/arm/mach-pxa/time.c              |   10 +---------
 arch/arm/mach-sa1100/time.c           |    8 +-------
 arch/arm/plat-nomadik/timer.c         |    9 +--------
 5 files changed, 15 insertions(+), 43 deletions(-)


-- 
Pengutronix e.K.                              | Uwe Kleine-K?nig            |
Industrial Linux Solutions                    | http://www.pengutronix.de/  |

[PATCH 1/2] arm/at91: Don't disable irqs in set_next_event and set_mode callbacks

[Uwe Kleine-König]

Hello Russell,

On Fri, Jan 22, 2010 at 05:52:38PM +0100, Uwe Kleine-K?nig wrote:
> On Fri, Jan 22, 2010 at 04:36:58PM +0000, Russell King - ARM Linux wrote:
> > On Fri, Jan 22, 2010 at 05:08:24PM +0100, Uwe Kleine-K?nig wrote:
> > > You can pull them from
> > > 
> > > 	git://git.pengutronix.de/git/ukl/linux-2.6.git arm/clock-event
> > 
> > I really don't like pulling git urls which I don't know what I'm pulling.
> > Please include at least a diffstat and shortlog of the commits you're
> > expecting to be sent.
> > 
> > Something like the output from git request-pull would be ideal.
> OK, appended below.  Actually I expected you to make me sending them to
> the patch system.  But as I prefer when you pull from me, I'm happy to
> provide you with diffstat and shortlog.
You didn't pull yet.  Is this still on your list?


Best regards
Uwe

> The following changes since commit 066000dd856709b6980123eb39b957fe26993f7b:
>   Ananth N Mavinakayanahalli (1):
>         Revert "x86, apic: Use logical flat on intel with <= 8 logical cpus"
> 
> are available in the git repository at:
> 
>   git://git.pengutronix.de/git/ukl/linux-2.6.git arm/clock-event
> 
> Uwe Kleine-K?nig (2):
>       arm/at91: Don't disable irqs in set_next_event and set_mode callbacks
>       arm/{pxa,sa1100,nomadik}: Don't disable irqs in set_next_event and set_mode
> 
>  arch/arm/mach-at91/at91rm9200_time.c  |   20 ++++++--------------
>  arch/arm/mach-at91/at91sam926x_time.c |   11 ++++++-----
>  arch/arm/mach-pxa/time.c              |   10 +---------
>  arch/arm/mach-sa1100/time.c           |    8 +-------
>  arch/arm/plat-nomadik/timer.c         |    9 +--------
>  5 files changed, 15 insertions(+), 43 deletions(-)

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |