[PATCH 0/3] clocksource: at91: Remove irq handler when clock event is unused

[PATCH 0/3] clocksource: at91: Remove irq handler when clock event is unused

[Alexandre Belloni]

The IRQ handlers for the clockevent on at91 (pit or system timer) are shared.
The following patch in linux-rt was removing those when they are unused:
http://git.kernel.org/cgit/linux/kernel/git/rt/linux-rt-devel.git/commit/?id=227cd21851456fb08e87f5a35e0e51280a9fd439

Unfortunately, they bitrotted over time and actually break the at91rm9200 (the
irq gets removed but never requested again) and gives warning on all the other
at91 platforms.

The following patches replace the mentionned patch and are based on
clockevents/4.3 to be applied after the switch to the 'set-state' interface.

Alexandre Belloni (3):
  clocksource: atmel-st: Remove irq handler when clock event is unused
  clocksource: atmel-pit: drop at91sam926x_pit_common_init
  clocksource: atmel-pit: Remove irq handler when clock event is unused

 drivers/clocksource/timer-atmel-pit.c | 134 +++++++++++++++++-----------------
 drivers/clocksource/timer-atmel-st.c  |  43 +++++++++--
 2 files changed, 102 insertions(+), 75 deletions(-)

-- 
2.1.4

[PATCH 1/3] clocksource: atmel-st: Remove irq handler when clock event is unused

[Alexandre Belloni]

Setup and remove the interrupt handler in clock event mode selection.
This avoids calling the (shared) interrupt handler when the device is not
used.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/clocksource/timer-atmel-st.c | 43 +++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 8 deletions(-)

diff --git a/drivers/clocksource/timer-atmel-st.c b/drivers/clocksource/timer-atmel-st.c
index 41b7b6dc1d0d..a498b7446006 100644
--- a/drivers/clocksource/timer-atmel-st.c
+++ b/drivers/clocksource/timer-atmel-st.c
@@ -31,6 +31,7 @@
 
 static unsigned long last_crtr;
 static u32 irqmask;
+static int irq;
 static struct clock_event_device clkevt;
 static struct regmap *regmap_st;
 
@@ -121,17 +122,45 @@ static int clkevt32k_shutdown(struct clock_event_device *evt)
 	clkdev32k_disable_and_flush_irq();
 	irqmask = 0;
 	regmap_write(regmap_st, AT91_ST_IER, irqmask);
+
+	if (clockevent_state_periodic(evt) || clockevent_state_oneshot(evt))
+		free_irq(irq, regmap_st);
+
+	return 0;
+}
+
+static int atmel_st_request_irq(struct clock_event_device *dev)
+{
+	int ret;
+
+	if (clockevent_state_periodic(dev) || clockevent_state_oneshot(dev))
+		return 0;
+
+	ret = request_irq(irq, at91rm9200_timer_interrupt,
+			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
+			  "at91_tick", regmap_st);
+	if (ret) {
+		pr_alert("Unable to setup IRQ\n");
+		return ret;
+	}
+
 	return 0;
 }
 
 static int clkevt32k_set_oneshot(struct clock_event_device *dev)
 {
+	int ret;
+
 	clkdev32k_disable_and_flush_irq();
 
 	/*
 	 * ALM for oneshot irqs, set by next_event()
 	 * before 32 seconds have passed.
 	 */
+	ret = atmel_st_request_irq(dev);
+	if (ret)
+		return ret;
+
 	irqmask = AT91_ST_ALMS;
 	regmap_write(regmap_st, AT91_ST_RTAR, last_crtr);
 	regmap_write(regmap_st, AT91_ST_IER, irqmask);
@@ -140,9 +169,15 @@ static int clkevt32k_set_oneshot(struct clock_event_device *dev)
 
 static int clkevt32k_set_periodic(struct clock_event_device *dev)
 {
+	int ret;
+
 	clkdev32k_disable_and_flush_irq();
 
 	/* PIT for periodic irqs; fixed rate of 1/HZ */
+	ret = atmel_st_request_irq(dev);
+	if (ret)
+		return ret;
+
 	irqmask = AT91_ST_PITS;
 	regmap_write(regmap_st, AT91_ST_PIMR, RM9200_TIMER_LATCH);
 	regmap_write(regmap_st, AT91_ST_IER, irqmask);
@@ -198,7 +233,6 @@ static struct clock_event_device clkevt = {
 static void __init atmel_st_timer_init(struct device_node *node)
 {
 	unsigned int val;
-	int irq, ret;
 
 	regmap_st = syscon_node_to_regmap(node);
 	if (IS_ERR(regmap_st))
@@ -214,13 +248,6 @@ static void __init atmel_st_timer_init(struct device_node *node)
 	if (!irq)
 		panic(pr_fmt("Unable to get IRQ from DT\n"));
 
-	/* Make IRQs happen for the system timer */
-	ret = request_irq(irq, at91rm9200_timer_interrupt,
-			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
-			  "at91_tick", regmap_st);
-	if (ret)
-		panic(pr_fmt("Unable to setup IRQ\n"));
-
 	/* The 32KiHz "Slow Clock" (tick every 30517.58 nanoseconds) is used
 	 * directly for the clocksource and all clockevents, after adjusting
 	 * its prescaler from the 1 Hz default.
-- 
2.1.4

[PATCH 1/3] clocksource: atmel-st: Remove irq handler when clock event is unused

[Thomas Gleixner]

On Fri, 17 Jul 2015, Alexandre Belloni wrote:
> +static int atmel_st_request_irq(struct clock_event_device *dev)
> +{
> +	int ret;
> +
> +	if (clockevent_state_periodic(dev) || clockevent_state_oneshot(dev))
> +		return 0;
> +
> +	ret = request_irq(irq, at91rm9200_timer_interrupt,
> +			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
> +			  "at91_tick", regmap_st);
> +	if (ret) {
> +		pr_alert("Unable to setup IRQ\n");
> +		return ret;
> +	}
> +
>  	return 0;
>  }
>  
>  static int clkevt32k_set_oneshot(struct clock_event_device *dev)
>  {
> +	int ret;
> +
>  	clkdev32k_disable_and_flush_irq();
>  
>  	/*
>  	 * ALM for oneshot irqs, set by next_event()
>  	 * before 32 seconds have passed.
>  	 */
> +	ret = atmel_st_request_irq(dev);

You cannot call request_irq() from interrupt disabled context. It
works during early boot because might_sleep() is not active then, but
if that happens during normal runtime it wont work.

Thanks,

	tglx

[PATCH 1/3] clocksource: atmel-st: Remove irq handler when clock event is unused

[Alexandre Belloni]

On 18/07/2015 at 10:12:03 +0200, Thomas Gleixner wrote :
> On Fri, 17 Jul 2015, Alexandre Belloni wrote:
> > +static int atmel_st_request_irq(struct clock_event_device *dev)
> > +{
> > +	int ret;
> > +
> > +	if (clockevent_state_periodic(dev) || clockevent_state_oneshot(dev))
> > +		return 0;
> > +
> > +	ret = request_irq(irq, at91rm9200_timer_interrupt,
> > +			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
> > +			  "at91_tick", regmap_st);
> > +	if (ret) {
> > +		pr_alert("Unable to setup IRQ\n");
> > +		return ret;
> > +	}
> > +
> >  	return 0;
> >  }
> >  
> >  static int clkevt32k_set_oneshot(struct clock_event_device *dev)
> >  {
> > +	int ret;
> > +
> >  	clkdev32k_disable_and_flush_irq();
> >  
> >  	/*
> >  	 * ALM for oneshot irqs, set by next_event()
> >  	 * before 32 seconds have passed.
> >  	 */
> > +	ret = atmel_st_request_irq(dev);
> 
> You cannot call request_irq() from interrupt disabled context. It
> works during early boot because might_sleep() is not active then, but
> if that happens during normal runtime it wont work.
> 

Indeed, clockevents_switch_state() has to be called with interrupts
disabled. So I'm not sure anymore how we should go about this change
(obviously, the same applies to the pit change). Do you have an idea?


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

[PATCH 1/3] clocksource: atmel-st: Remove irq handler when clock event is unused

[Thomas Gleixner]

On Sun, 19 Jul 2015, Alexandre Belloni wrote:
> On 18/07/2015 at 10:12:03 +0200, Thomas Gleixner wrote :
> > On Fri, 17 Jul 2015, Alexandre Belloni wrote:
> > > +static int atmel_st_request_irq(struct clock_event_device *dev)
> > > +{
> > > +	int ret;
> > > +
> > > +	if (clockevent_state_periodic(dev) || clockevent_state_oneshot(dev))
> > > +		return 0;
> > > +
> > > +	ret = request_irq(irq, at91rm9200_timer_interrupt,
> > > +			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
> > > +			  "at91_tick", regmap_st);
> > > +	if (ret) {
> > > +		pr_alert("Unable to setup IRQ\n");
> > > +		return ret;
> > > +	}
> > > +
> > >  	return 0;
> > >  }
> > >  
> > >  static int clkevt32k_set_oneshot(struct clock_event_device *dev)
> > >  {
> > > +	int ret;
> > > +
> > >  	clkdev32k_disable_and_flush_irq();
> > >  
> > >  	/*
> > >  	 * ALM for oneshot irqs, set by next_event()
> > >  	 * before 32 seconds have passed.
> > >  	 */
> > > +	ret = atmel_st_request_irq(dev);
> > 
> > You cannot call request_irq() from interrupt disabled context. It
> > works during early boot because might_sleep() is not active then, but
> > if that happens during normal runtime it wont work.
> > 
> 
> Indeed, clockevents_switch_state() has to be called with interrupts
> disabled. So I'm not sure anymore how we should go about this change
> (obviously, the same applies to the pit change). Do you have an idea?

That's why I used setup/remove_irq() in the stale RT changes.

Thanks,

	tglx

[PATCH 1/3] clocksource: atmel-st: Remove irq handler when clock event is unused

[Alexandre Belloni]

On 20/07/2015 at 11:04:30 +0200, Thomas Gleixner wrote :
> That's why I used setup/remove_irq() in the stale RT changes.
> 

Hum, actually, you hit the same thing with setup/remove_irq (and I'm not sure
to follow why):

BUG: sleeping function called from invalid context at mm/slab.c:2863
in_atomic(): 1, irqs_disabled(): 128, pid: 596, name: sh
CPU: 0 PID: 596 Comm: sh Tainted: G        W       4.1.0-rc4+ #122
Hardware name: Atmel AT91RM9200
[<c000ebd4>] (unwind_backtrace) from [<c000ca64>] (show_stack+0x10/0x14)
[<c000ca64>] (show_stack) from [<c008a0c8>] (__kmalloc+0x38/0x104)
[<c008a0c8>] (__kmalloc) from [<c00d8770>] (__proc_create+0x11c/0x194)
[<c00d8770>] (__proc_create) from [<c00d8b44>] (proc_mkdir_data+0x30/0x9c)
[<c00d8b44>] (proc_mkdir_data) from [<c0042a00>] (register_handler_proc+0xfc/0x10c)
[<c0042a00>] (register_handler_proc) from [<c003f0b0>] (__setup_irq+0x438/0x518)
[<c003f0b0>] (__setup_irq) from [<c003f1e8>] (setup_irq+0x58/0x84)
[<c003f1e8>] (setup_irq) from [<c02de298>] (atmel_st_request_irq+0x4c/0x84)
[<c02de298>] (atmel_st_request_irq) from [<c02de338>] (clkevt32k_set_periodic+0x14/0x54)
[<c02de338>] (clkevt32k_set_periodic) from [<c00509a0>] (clockevents_switch_state+0x1c/0x58)
[<c00509a0>] (clockevents_switch_state) from [<c0050584>] (__clockevents_unbind+0xb8/0xfc)
[<c0050584>] (__clockevents_unbind) from [<c0054954>] (smp_call_function_single+0x34/0x44)
[<c0054954>] (smp_call_function_single) from [<c00506b0>] (sysfs_unbind_tick_dev+0xe8/0x114)
[<c00506b0>] (sysfs_unbind_tick_dev) from [<c00e1748>] (kernfs_fop_write+0x118/0x174)
[<c00e1748>] (kernfs_fop_write) from [<c008df00>] (__vfs_write+0x24/0xdc)
[<c008df00>] (__vfs_write) from [<c008e5f8>] (vfs_write+0xa4/0x128)
[<c008e5f8>] (vfs_write) from [<c008ecf4>] (SyS_write+0x40/0x78)
[<c008ecf4>] (SyS_write) from [<c000a240>] (ret_fast_syscall+0x0/0x30)


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

[PATCH 1/3] clocksource: atmel-st: Remove irq handler when clock event is unused

[Thomas Gleixner]

On Mon, 20 Jul 2015, Alexandre Belloni wrote:

> On 20/07/2015 at 11:04:30 +0200, Thomas Gleixner wrote :
> > That's why I used setup/remove_irq() in the stale RT changes.
> > 
> 
> Hum, actually, you hit the same thing with setup/remove_irq (and I'm not sure
> to follow why):

It's the kmalloc of the proc interface. So back to the drawing
board. I'll think about it tomorrow with brain awake.

Thanks,

	tglx

[PATCH 1/3] clocksource: atmel-st: Remove irq handler when clock event is unused

[Thomas Gleixner]

On Mon, 20 Jul 2015, Thomas Gleixner wrote:

> On Mon, 20 Jul 2015, Alexandre Belloni wrote:
> 
> > On 20/07/2015 at 11:04:30 +0200, Thomas Gleixner wrote :
> > > That's why I used setup/remove_irq() in the stale RT changes.
> > > 
> > 
> > Hum, actually, you hit the same thing with setup/remove_irq (and I'm not sure
> > to follow why):
> 
> It's the kmalloc of the proc interface. So back to the drawing
> board. I'll think about it tomorrow with brain awake.

So the only idea I could come up with is the old approach of creating
a dummy irq chip for the interrupts which share that timer/uart/rtc
whatever line.

I think Boris Brezillon had implemented it at some point, but it was
shot down for reasons I can't remember. That would allow us to simply
have disable_irq_nosync/enable_irq in the relevant code pathes and be
done with it. I guess I need to find the references and give it a try
again if nobody beats me to it.

Thanks,

	tglx

[PATCH 1/3] clocksource: atmel-st: Remove irq handler when clock event is unused

[Peter Zijlstra]

On Sun, Aug 02, 2015 at 11:10:21AM +0200, Thomas Gleixner wrote:
> I think Boris Brezillon had implemented it at some point, but it was
> shot down for reasons I can't remember. 

You weren't around at the time.. DT people didn't like it, said they
didn't like having to make up fake hardware in their DT crap.

> That would allow us to simply
> have disable_irq_nosync/enable_irq in the relevant code pathes and be
> done with it. I guess I need to find the references and give it a try
> again if nobody beats me to it.

lkml.kernel.org/r/1422527620-8308-3-git-send-email-boris.brezillon at free-electrons.com

[PATCH 1/3] clocksource: atmel-st: Remove irq handler when clock event is unused

[Boris Brezillon]

Peter, Thomas,

On Sun, 2 Aug 2015 11:40:28 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Sun, Aug 02, 2015 at 11:10:21AM +0200, Thomas Gleixner wrote:
> > I think Boris Brezillon had implemented it at some point, but it was
> > shot down for reasons I can't remember. 
> 
> You weren't around at the time.. DT people didn't like it, said they
> didn't like having to make up fake hardware in their DT crap.

I don't know who was right, but the fact is they won't be inclined to
take such an approach unless the virtual demuxer is not exposed in the
DT, which is almost impossible since irq users are identifying their
irq lines with a phandle to the interrupt controller and an interrupt
id (usually extracted from the datasheet).

Anyway, below is a solution providing a way to disable specific
handlers without reworking the way we are modeling shared interrupts.
Thomas, I know you were not in favor of the proposed approach, but,
AFAICT, it does not add any conditional path to the interrupt handling
path (which, IIRC, was one of your requirements), and is simple enough
to be used by people really needing it.

There's probably other drawback I haven't noticed, so please don't
hesitate to share your thoughts.

Thanks,

Boris

--- >8 ---

>From 3add0dceff714d94748106eba9867ea9aa3cd6a8 Mon Sep 17 00:00:00 2001
From: Boris Brezillon <boris.brezillon@free-electrons.com>
Date: Mon, 3 Aug 2015 15:18:08 +0200
Subject: [PATCH] irq: add disable_irq_handler()/enable_irq_handler() functions

Sometime we need to disable a specific handler on a shared irq line.
Currently, the only way this can be achieved is by freeing the irq handler
when we want to disable the irq and creating a new one when we want to
enable it.
This is not only adding some overhead to the disable/enable operations, but
the request_irq function cannot be called in atomic context, which means
it prevents disabling the interrupt in such situation.

This patch introduces three new functions: disable_irq_handler(),
disable_irq_handler_nosync() and enable_irq_handler() to allow disabling
a specific handler on a shared irq line.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 include/linux/irqdesc.h |  1 +
 kernel/irq/manage.c     | 82 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 83 insertions(+)

diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index fcea4e4..c8bd055 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -52,6 +52,7 @@ struct irq_desc {
 	irq_preflow_handler_t	preflow_handler;
 #endif
 	struct irqaction	*action;	/* IRQ action list */
+	struct irqaction	*disabled_actions;	/* IRQ disabled action list */
 	unsigned int		status_use_accessors;
 	unsigned int		core_internal_state__do_not_mess_with_it;
 	unsigned int		depth;		/* nested irq disables */
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f974485..4690ab4 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -458,6 +458,47 @@ void disable_irq_nosync(unsigned int irq)
 }
 EXPORT_SYMBOL(disable_irq_nosync);
 
+static int __disable_irq_handler_nosync(unsigned int irq, void *dev_id)
+{
+	unsigned long flags;
+	struct irqaction *action, **prev;
+	struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
+	int ret = 0;
+
+	if (!desc)
+		return -EINVAL;
+
+	for (action = desc->action, prev = &desc->action; action; action = action->next) {
+		if (action->dev_id == dev_id)
+			break;
+
+		prev = &action->next;
+	}
+
+	if (!action)
+		goto out;
+
+	*prev = action->next;
+
+	action->next = desc->disabled_actions;
+	desc->disabled_actions = action;
+	if (!desc->action) {
+		__disable_irq(desc, irq);
+		ret = 1;
+	}
+
+out:
+	irq_put_desc_busunlock(desc, flags);
+	return ret;
+}
+
+static void disable_irq_handler_nosync(unsigned int irq, void *dev_id)
+{
+	__disable_irq_handler_nosync(irq, dev_id);
+}
+EXPORT_SYMBOL(disable_irq_handler_nosync);
+
+
 /**
  *	disable_irq - disable an irq and wait for completion
  *	@irq: Interrupt to disable
@@ -477,6 +518,13 @@ void disable_irq(unsigned int irq)
 }
 EXPORT_SYMBOL(disable_irq);
 
+void disable_irq_handler(unsigned int irq, )
+{
+	if (__disable_irq_handler_nosync(irq) > 0)
+		synchronize_irq(irq);
+}
+EXPORT_SYMBOL(disable_irq);
+
 /**
  *	disable_hardirq - disables an irq and waits for hardirq completion
  *	@irq: Interrupt to disable
@@ -552,6 +600,40 @@ out:
 }
 EXPORT_SYMBOL(enable_irq);
 
+void enable_irq_handler(unsigned int irq, void *dev_id)
+{
+	unsigned long flags;
+	struct irqaction *action, **prev;
+	struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
+
+	if (!desc)
+		return -EINVAL;
+
+	for (action = desc->disabled_actions, prev = &desc->action; action;
+	     action = action->next) {
+		if (action->dev_id == dev_id)
+			break;
+
+		prev = &action->next;
+	}
+
+	if (!action)
+		goto out;
+
+	*prev = action->next;
+
+	action->next = desc->action;
+	desc->action = action;
+
+	if (!action->next)
+		__enable_irq(desc, irq);
+
+out:
+	irq_put_desc_busunlock(desc, flags);
+
+}
+EXPORT_SYMBOL(enable_irq_handler);
+
 static int set_irq_wake_real(unsigned int irq, unsigned int on)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
-- 
1.9.1

[PATCH 1/3] clocksource: atmel-st: Remove irq handler when clock event is unused

[Thomas Gleixner]

On Mon, 3 Aug 2015, Boris Brezillon wrote:
> On Sun, 2 Aug 2015 11:40:28 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Sun, Aug 02, 2015 at 11:10:21AM +0200, Thomas Gleixner wrote:
> > > I think Boris Brezillon had implemented it at some point, but it was
> > > shot down for reasons I can't remember. 
> > 
> > You weren't around at the time.. DT people didn't like it, said they
> > didn't like having to make up fake hardware in their DT crap.
> 
> I don't know who was right, but the fact is they won't be inclined to
> take such an approach unless the virtual demuxer is not exposed in the
> DT, which is almost impossible since irq users are identifying their
> irq lines with a phandle to the interrupt controller and an interrupt
> id (usually extracted from the datasheet).

I really have no idea why DT folks think that virtual devices are not
suitable for DT entries. Marks working assumption from the old thread:

 > This sounds like a DT-workaround for a Linux implementation problem,
 > and I don't think this the right way to solve your problem.

is simply wrong. This has nothing to do with a Linux implementation
problem. It's a sensible workaround for braindamaged hardware at the
proper abstraction level.

> Anyway, below is a solution providing a way to disable specific
> handlers without reworking the way we are modeling shared interrupts.
> Thomas, I know you were not in favor of the proposed approach, but,
> AFAICT, it does not add any conditional path to the interrupt handling
> path (which, IIRC, was one of your requirements), and is simple enough
> to be used by people really needing it.
> 
> There's probably other drawback I haven't noticed, so please don't
> hesitate to share your thoughts.

Yes, aside of the bloat, it needs special handling in free_irq() as
well.

Thanks,

	tglx

[PATCH 1/3] clocksource: atmel-st: Remove irq handler when clock event is unused

[Boris Brezillon]

On Mon, 3 Aug 2015 16:36:29 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Mon, 3 Aug 2015, Boris Brezillon wrote:
> > On Sun, 2 Aug 2015 11:40:28 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
> > 
> > > On Sun, Aug 02, 2015 at 11:10:21AM +0200, Thomas Gleixner wrote:
> > > > I think Boris Brezillon had implemented it at some point, but it was
> > > > shot down for reasons I can't remember. 
> > > 
> > > You weren't around at the time.. DT people didn't like it, said they
> > > didn't like having to make up fake hardware in their DT crap.
> > 
> > I don't know who was right, but the fact is they won't be inclined to
> > take such an approach unless the virtual demuxer is not exposed in the
> > DT, which is almost impossible since irq users are identifying their
> > irq lines with a phandle to the interrupt controller and an interrupt
> > id (usually extracted from the datasheet).
> 
> I really have no idea why DT folks think that virtual devices are not
> suitable for DT entries. Marks working assumption from the old thread:
> 
>  > This sounds like a DT-workaround for a Linux implementation problem,
>  > and I don't think this the right way to solve your problem.
> 
> is simply wrong. This has nothing to do with a Linux implementation
> problem. It's a sensible workaround for braindamaged hardware at the
> proper abstraction level.
> 
> > Anyway, below is a solution providing a way to disable specific
> > handlers without reworking the way we are modeling shared interrupts.
> > Thomas, I know you were not in favor of the proposed approach, but,
> > AFAICT, it does not add any conditional path to the interrupt handling
> > path (which, IIRC, was one of your requirements), and is simple enough
> > to be used by people really needing it.
> > 
> > There's probably other drawback I haven't noticed, so please don't
> > hesitate to share your thoughts.
> 
> Yes, aside of the bloat, it needs special handling in free_irq() as
> well.

Right. Below is a patch fixing the __setup_irq()/__free_irq() functions.

Anyway, I won't insist on this approach unless you want me to go
further, and I don't plan to spend more time on convincing DT folks that
the virtual demux is what we need (I already spent more time than I
wanted arguing on this feature :-/).

Best Regards,

Boris

--- >8 ---

>From 96448d56b202f2140e7008a3ca291b133696b4c8 Mon Sep 17 00:00:00 2001
From: Boris Brezillon <boris.brezillon@free-electrons.com>
Date: Mon, 3 Aug 2015 21:59:29 +0200
Subject: [PATCH] irq: add disable_irq_handler()/enable_irq_handler() functions

Sometime we need to disable a specific handler on a shared irq line.
Currently, the only way this can be achieved is by freeing the irq handler
when we want to disable the irq and creating a new one when we want to
enable.
This is not only adding some overhead to the disable/enable operations, but
the request_irq function cannot be called in atomic context, which means
it prevents disabling the interrupt in such situation.

This patch introduces three new functions: disable_irq_handler(),
disable_irq_handler_nosync() and enable_irq_handler() to allow disabling
a specific handler on a shared irq line.

Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 include/linux/irqdesc.h |   1 +
 kernel/irq/manage.c     | 143 ++++++++++++++++++++++++++++++++++++++++++------
 2 files changed, 126 insertions(+), 18 deletions(-)

diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index fcea4e4..c8bd055 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -52,6 +52,7 @@ struct irq_desc {
 	irq_preflow_handler_t	preflow_handler;
 #endif
 	struct irqaction	*action;	/* IRQ action list */
+	struct irqaction	*disabled_actions;	/* IRQ disabled action list */
 	unsigned int		status_use_accessors;
 	unsigned int		core_internal_state__do_not_mess_with_it;
 	unsigned int		depth;		/* nested irq disables */
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f974485..0e7432b 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -458,6 +458,47 @@ void disable_irq_nosync(unsigned int irq)
 }
 EXPORT_SYMBOL(disable_irq_nosync);
 
+static int __disable_irq_handler_nosync(unsigned int irq, void *dev_id)
+{
+	unsigned long flags;
+	struct irqaction *action, **prev;
+	struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
+	int ret = 0;
+
+	if (!desc)
+		return -EINVAL;
+
+	for (action = desc->action, prev = &desc->action; action; action = action->next) {
+		if (action->dev_id == dev_id)
+			break;
+
+		prev = &action->next;
+	}
+
+	if (!action)
+		goto out;
+
+	*prev = action->next;
+
+	action->next = desc->disabled_actions;
+	desc->disabled_actions = action;
+	if (!desc->action) {
+		__disable_irq(desc, irq);
+		ret = 1;
+	}
+
+out:
+	irq_put_desc_busunlock(desc, flags);
+	return ret;
+}
+
+static void disable_irq_handler_nosync(unsigned int irq, void *dev_id)
+{
+	__disable_irq_handler_nosync(irq, dev_id);
+}
+EXPORT_SYMBOL(disable_irq_handler_nosync);
+
+
 /**
  *	disable_irq - disable an irq and wait for completion
  *	@irq: Interrupt to disable
@@ -477,6 +518,13 @@ void disable_irq(unsigned int irq)
 }
 EXPORT_SYMBOL(disable_irq);
 
+void disable_irq_handler(unsigned int irq, )
+{
+	if (__disable_irq_handler_nosync(irq) > 0)
+		synchronize_irq(irq);
+}
+EXPORT_SYMBOL(disable_irq);
+
 /**
  *	disable_hardirq - disables an irq and waits for hardirq completion
  *	@irq: Interrupt to disable
@@ -552,6 +600,40 @@ out:
 }
 EXPORT_SYMBOL(enable_irq);
 
+void enable_irq_handler(unsigned int irq, void *dev_id)
+{
+	unsigned long flags;
+	struct irqaction *action, **prev;
+	struct irq_desc *desc = irq_get_desc_buslock(irq, &flags, IRQ_GET_DESC_CHECK_GLOBAL);
+
+	if (!desc)
+		return -EINVAL;
+
+	for (action = desc->disabled_actions, prev = &desc->action; action;
+	     action = action->next) {
+		if (action->dev_id == dev_id)
+			break;
+
+		prev = &action->next;
+	}
+
+	if (!action)
+		goto out;
+
+	*prev = action->next;
+
+	action->next = desc->action;
+	desc->action = action;
+
+	if (!action->next)
+		__enable_irq(desc, irq);
+
+out:
+	irq_put_desc_busunlock(desc, flags);
+
+}
+EXPORT_SYMBOL(enable_irq_handler);
+
 static int set_irq_wake_real(unsigned int irq, unsigned int on)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
@@ -1118,6 +1200,12 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 	raw_spin_lock_irqsave(&desc->lock, flags);
 	old_ptr = &desc->action;
 	old = *old_ptr;
+	if (!old) {
+		old_ptr = &desc->disabled_actions;
+		old = *old_ptr;
+	}
+
+	old = desc->action ? : desc->disabled_actions;
 	if (old) {
 		/*
 		 * Can't share interrupts unless both agree to and are
@@ -1136,20 +1224,27 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 		    (new->flags & IRQF_PERCPU))
 			goto mismatch;
 
-		/* add new interrupt at end of irq queue */
-		do {
-			/*
-			 * Or all existing action->thread_mask bits,
-			 * so we can find the next zero bit for this
-			 * new action.
-			 */
-			thread_mask |= old->thread_mask;
-			old_ptr = &old->next;
-			old = *old_ptr;
-		} while (old);
 		shared = 1;
 	}
 
+	if (irq_settings_can_autoenable(desc))
+		old_ptr = &desc->action;
+	else
+		old_ptr = &desc->disabled_actions;
+	old = *old_ptr;
+
+	/* add new interrupt at end of irq queue */
+	while (old) {
+		/*
+		 * Or all existing action->thread_mask bits,
+		 * so we can find the next zero bit for this
+		 * new action.
+		 */
+		thread_mask |= old->thread_mask;
+		old_ptr = &old->next;
+		old = *old_ptr;
+	}
+
 	/*
 	 * Setup the thread mask for this irqaction for ONESHOT. For
 	 * !ONESHOT irqs the thread mask is 0 so we can avoid a
@@ -1373,25 +1468,37 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 	for (;;) {
 		action = *action_ptr;
 
-		if (!action) {
-			WARN(1, "Trying to free already-free IRQ %d\n", irq);
-			raw_spin_unlock_irqrestore(&desc->lock, flags);
-
-			return NULL;
-		}
 
 		if (action->dev_id == dev_id)
 			break;
 		action_ptr = &action->next;
 	}
 
+	if (!action) {
+		action_ptr = &desc->disabled_actions;
+		for (;;) {
+			action = *action_ptr;
+
+			if (action->dev_id == dev_id)
+				break;
+			action_ptr = &action->next;
+		}
+	}
+
+	if (!action) {
+		WARN(1, "Trying to free already-free IRQ %d\n", irq);
+		raw_spin_unlock_irqrestore(&desc->lock, flags);
+
+		return NULL;
+	}
+
 	/* Found it - now remove it from the list of entries: */
 	*action_ptr = action->next;
 
 	irq_pm_remove_action(desc, action);
 
 	/* If this was the last handler, shut down the IRQ line: */
-	if (!desc->action) {
+	if (!desc->action && !desc->disabled_actions) {
 		irq_shutdown(desc);
 		irq_release_resources(desc);
 	}
-- 
1.9.1

[PATCH 2/3] clocksource: atmel-pit: drop at91sam926x_pit_common_init

[Alexandre Belloni]

Merge at91sam926x_pit_common_init in at91sam926x_pit_dt_init as there is no
other initialization (e.g. platform data) anymore.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/clocksource/timer-atmel-pit.c | 52 ++++++++++++++++-------------------
 1 file changed, 23 insertions(+), 29 deletions(-)

diff --git a/drivers/clocksource/timer-atmel-pit.c b/drivers/clocksource/timer-atmel-pit.c
index 58753223585b..e64af62a186c 100644
--- a/drivers/clocksource/timer-atmel-pit.c
+++ b/drivers/clocksource/timer-atmel-pit.c
@@ -177,12 +177,34 @@ static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
 /*
  * Set up both clocksource and clockevent support.
  */
-static void __init at91sam926x_pit_common_init(struct pit_data *data)
+static void __init at91sam926x_pit_dt_init(struct device_node *node)
 {
+	struct pit_data *data;
 	unsigned long	pit_rate;
 	unsigned	bits;
 	int		ret;
 
+	data = kzalloc(sizeof(*data), GFP_KERNEL);
+	if (!data)
+		panic(pr_fmt("Unable to allocate memory\n"));
+
+	data->base = of_iomap(node, 0);
+	if (!data->base)
+		panic(pr_fmt("Could not map PIT address\n"));
+
+	data->mck = of_clk_get(node, 0);
+	if (IS_ERR(data->mck))
+		/* Fallback on clkdev for !CCF-based boards */
+		data->mck = clk_get(NULL, "mck");
+
+	if (IS_ERR(data->mck))
+		panic(pr_fmt("Unable to get mck clk\n"));
+
+	/* Get the interrupts property */
+	data->irq = irq_of_parse_and_map(node, 0);
+	if (!data->irq)
+		panic(pr_fmt("Unable to get IRQ from DT\n"));
+
 	/*
 	 * Use our actual MCK to figure out how many MCK/16 ticks per
 	 * 1/HZ period (instead of a compile-time constant LATCH).
@@ -227,33 +249,5 @@ static void __init at91sam926x_pit_common_init(struct pit_data *data)
 	data->clkevt.suspend = at91sam926x_pit_suspend;
 	clockevents_register_device(&data->clkevt);
 }
-
-static void __init at91sam926x_pit_dt_init(struct device_node *node)
-{
-	struct pit_data *data;
-
-	data = kzalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		panic(pr_fmt("Unable to allocate memory\n"));
-
-	data->base = of_iomap(node, 0);
-	if (!data->base)
-		panic(pr_fmt("Could not map PIT address\n"));
-
-	data->mck = of_clk_get(node, 0);
-	if (IS_ERR(data->mck))
-		/* Fallback on clkdev for !CCF-based boards */
-		data->mck = clk_get(NULL, "mck");
-
-	if (IS_ERR(data->mck))
-		panic(pr_fmt("Unable to get mck clk\n"));
-
-	/* Get the interrupts property */
-	data->irq = irq_of_parse_and_map(node, 0);
-	if (!data->irq)
-		panic(pr_fmt("Unable to get IRQ from DT\n"));
-
-	at91sam926x_pit_common_init(data);
-}
 CLOCKSOURCE_OF_DECLARE(at91sam926x_pit, "atmel,at91sam9260-pit",
 		       at91sam926x_pit_dt_init);
-- 
2.1.4

[PATCH 3/3] clocksource: atmel-pit: Remove irq handler when clock event is unused

[Alexandre Belloni]

Setup and remove the interrupt handler in clock event mode selection.
This avoids calling the (shared) interrupt handler when the device is not
used.

Signed-off-by: Alexandre Belloni <alexandre.belloni@free-electrons.com>
---
 drivers/clocksource/timer-atmel-pit.c | 86 +++++++++++++++++++----------------
 1 file changed, 46 insertions(+), 40 deletions(-)

diff --git a/drivers/clocksource/timer-atmel-pit.c b/drivers/clocksource/timer-atmel-pit.c
index e64af62a186c..c77aafa7846e 100644
--- a/drivers/clocksource/timer-atmel-pit.c
+++ b/drivers/clocksource/timer-atmel-pit.c
@@ -96,15 +96,61 @@ static int pit_clkevt_shutdown(struct clock_event_device *dev)
 
 	/* disable irq, leaving the clocksource active */
 	pit_write(data->base, AT91_PIT_MR, (data->cycle - 1) | AT91_PIT_PITEN);
+
+	if (clockevent_state_periodic(dev))
+		free_irq(data->irq, data);
+
 	return 0;
 }
 
 /*
+ * IRQ handler for the timer.
+ */
+static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
+{
+	struct pit_data *data = dev_id;
+
+	/*
+	 * irqs should be disabled here, but as the irq is shared they are only
+	 * guaranteed to be off if the timer irq is registered first.
+	 */
+	WARN_ON_ONCE(!irqs_disabled());
+
+	/* The PIT interrupt may be disabled, and is shared */
+	if (clockevent_state_periodic(&data->clkevt) &&
+	    (pit_read(data->base, AT91_PIT_SR) & AT91_PIT_PITS)) {
+		unsigned nr_ticks;
+
+		/* Get number of ticks performed before irq, and ack it */
+		nr_ticks = PIT_PICNT(pit_read(data->base, AT91_PIT_PIVR));
+		do {
+			data->cnt += data->cycle;
+			data->clkevt.event_handler(&data->clkevt);
+			nr_ticks--;
+		} while (nr_ticks);
+
+		return IRQ_HANDLED;
+	}
+
+	return IRQ_NONE;
+}
+
+/*
  * Clockevent device:  interrupts every 1/HZ (== pit_cycles * MCK/16)
  */
 static int pit_clkevt_set_periodic(struct clock_event_device *dev)
 {
 	struct pit_data *data = clkevt_to_pit_data(dev);
+	int		ret;
+
+	/* Set up irq handler */
+	ret = request_irq(data->irq, at91sam926x_pit_interrupt,
+			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
+			  "at91_tick", data);
+	if (ret) {
+		pr_alert("Unable to setup IRQ\n");
+		return ret;
+	}
 
 	/* update clocksource counter */
 	data->cnt += data->cycle * PIT_PICNT(pit_read(data->base, AT91_PIT_PIVR));
@@ -143,38 +189,6 @@ static void at91sam926x_pit_resume(struct clock_event_device *cedev)
 }
 
 /*
- * IRQ handler for the timer.
- */
-static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
-{
-	struct pit_data *data = dev_id;
-
-	/*
-	 * irqs should be disabled here, but as the irq is shared they are only
-	 * guaranteed to be off if the timer irq is registered first.
-	 */
-	WARN_ON_ONCE(!irqs_disabled());
-
-	/* The PIT interrupt may be disabled, and is shared */
-	if (clockevent_state_periodic(&data->clkevt) &&
-	    (pit_read(data->base, AT91_PIT_SR) & AT91_PIT_PITS)) {
-		unsigned nr_ticks;
-
-		/* Get number of ticks performed before irq, and ack it */
-		nr_ticks = PIT_PICNT(pit_read(data->base, AT91_PIT_PIVR));
-		do {
-			data->cnt += data->cycle;
-			data->clkevt.event_handler(&data->clkevt);
-			nr_ticks--;
-		} while (nr_ticks);
-
-		return IRQ_HANDLED;
-	}
-
-	return IRQ_NONE;
-}
-
-/*
  * Set up both clocksource and clockevent support.
  */
 static void __init at91sam926x_pit_dt_init(struct device_node *node)
@@ -182,7 +196,6 @@ static void __init at91sam926x_pit_dt_init(struct device_node *node)
 	struct pit_data *data;
 	unsigned long	pit_rate;
 	unsigned	bits;
-	int		ret;
 
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
@@ -228,13 +241,6 @@ static void __init at91sam926x_pit_dt_init(struct device_node *node)
 	data->clksrc.flags = CLOCK_SOURCE_IS_CONTINUOUS,
 	clocksource_register_hz(&data->clksrc, pit_rate);
 
-	/* Set up irq handler */
-	ret = request_irq(data->irq, at91sam926x_pit_interrupt,
-			  IRQF_SHARED | IRQF_TIMER | IRQF_IRQPOLL,
-			  "at91_tick", data);
-	if (ret)
-		panic(pr_fmt("Unable to setup IRQ\n"));
-
 	/* Set up and register clockevents */
 	data->clkevt.name = "pit";
 	data->clkevt.features = CLOCK_EVT_FEAT_PERIODIC;
-- 
2.1.4

[PATCH 3/3] clocksource: atmel-pit: Remove irq handler when clock event is unused

[Thomas Gleixner]

On Fri, 17 Jul 2015, Alexandre Belloni wrote:
>  /*
> + * IRQ handler for the timer.
> + */
> +static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
> +{
> +	struct pit_data *data = dev_id;
> +
> +	/*
> +	 * irqs should be disabled here, but as the irq is shared they are only
> +	 * guaranteed to be off if the timer irq is registered first.

That's wrong. We run all handlers with interrupts disabled for about 5
years now.

> +	 */
> +	WARN_ON_ONCE(!irqs_disabled());


> +	/* The PIT interrupt may be disabled, and is shared */
> +	if (clockevent_state_periodic(&data->clkevt) &&
> +	    (pit_read(data->base, AT91_PIT_SR) & AT91_PIT_PITS)) {
> +		unsigned nr_ticks;
> +
> +		/* Get number of ticks performed before irq, and ack it */
> +		nr_ticks = PIT_PICNT(pit_read(data->base, AT91_PIT_PIVR));
> +		do {
> +			data->cnt += data->cycle;
> +			data->clkevt.event_handler(&data->clkevt);
> +			nr_ticks--;
> +		} while (nr_ticks);

I don't think you need this loop. You have a proper clocksource
registered so the timekeeping code will handle the lost ticks nicely.


Thanks,

	tglx

[PATCH 3/3] clocksource: atmel-pit: Remove irq handler when clock event is unused

[Alexandre Belloni]

On 18/07/2015 at 10:20:53 +0200, Thomas Gleixner wrote :
> On Fri, 17 Jul 2015, Alexandre Belloni wrote:
> >  /*
> > + * IRQ handler for the timer.
> > + */
> > +static irqreturn_t at91sam926x_pit_interrupt(int irq, void *dev_id)
> > +{
> > +	struct pit_data *data = dev_id;
> > +
> > +	/*
> > +	 * irqs should be disabled here, but as the irq is shared they are only
> > +	 * guaranteed to be off if the timer irq is registered first.
> 
> That's wrong. We run all handlers with interrupts disabled for about 5
> years now.
> 
> > +	 */
> > +	WARN_ON_ONCE(!irqs_disabled());
> 

Yeah, I was skeptical when I read that...

> 
> > +	/* The PIT interrupt may be disabled, and is shared */
> > +	if (clockevent_state_periodic(&data->clkevt) &&
> > +	    (pit_read(data->base, AT91_PIT_SR) & AT91_PIT_PITS)) {
> > +		unsigned nr_ticks;
> > +
> > +		/* Get number of ticks performed before irq, and ack it */
> > +		nr_ticks = PIT_PICNT(pit_read(data->base, AT91_PIT_PIVR));
> > +		do {
> > +			data->cnt += data->cycle;
> > +			data->clkevt.event_handler(&data->clkevt);
> > +			nr_ticks--;
> > +		} while (nr_ticks);
> 
> I don't think you need this loop. You have a proper clocksource
> registered so the timekeeping code will handle the lost ticks nicely.
> 

... for both comments, this is just code I'm moving from one place to
another. I'll have a look a clean that up in a preliminary patch.
Thanks!


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