irq-type-flags.patch

irq-type-flags.patch

[Andrew Morton]

Is everyone OK with this?


From: Russell King <rmk+lkml@arm.linux.org.uk>

Some ARM platforms have the ability to program the interrupt controller to
detect various interrupt edges and/or levels.  For some platforms, this is
critical to setup correctly, particularly those which the setting is
dependent on the device.

Currently, ARM drivers do (eg) the following:

	err = request_irq(irq, ...);

	set_irq_type(irq, IRQT_RISING);

However, if the interrupt has previously been programmed to be level
sensitive (for whatever reason) then this will cause an interrupt storm.

Hence, if we combine set_irq_type() with request_irq(), we can then safely
set the type prior to unmasking the interrupt.  The unfortunate problem is
that in order to support this, these flags need to be visible outside of
the ARM architecture - drivers such as smc91x need these flags and they're
cross-architecture.

Finally, the SA_TRIGGER_* flag passed to request_irq() should reflect the
property that the device would like.  The IRQ controller code should do its
best to select the most appropriate supported mode.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Andrew Morton <akpm@osdl.org>
---

 arch/arm/kernel/irq.c  |   16 ++++++++++++++--
 include/linux/signal.h |    8 ++++++++
 2 files changed, 22 insertions(+), 2 deletions(-)

diff -puN arch/arm/kernel/irq.c~irq-type-flags arch/arm/kernel/irq.c
--- devel/arch/arm/kernel/irq.c~irq-type-flags	2005-11-06 00:55:46.000000000 -0800
+++ devel-akpm/arch/arm/kernel/irq.c	2005-11-06 00:55:46.000000000 -0800
@@ -681,10 +681,16 @@ int setup_irq(unsigned int irq, struct i
 	 */
 	desc = irq_desc + irq;
 	spin_lock_irqsave(&irq_controller_lock, flags);
+#define SA_TRIGGER	(SA_TRIGGER_HIGH|SA_TRIGGER_LOW|\
+			 SA_TRIGGER_RISING|SA_TRIGGER_FALLING)
 	p = &desc->action;
 	if ((old = *p) != NULL) {
-		/* Can't share interrupts unless both agree to */
-		if (!(old->flags & new->flags & SA_SHIRQ)) {
+		/*
+		 * Can't share interrupts unless both agree to and are
+		 * the same type.
+		 */
+		if (!(old->flags & new->flags & SA_SHIRQ) ||
+		    (~old->flags & new->flags) & SA_TRIGGER) {
 			spin_unlock_irqrestore(&irq_controller_lock, flags);
 			return -EBUSY;
 		}
@@ -704,6 +710,12 @@ int setup_irq(unsigned int irq, struct i
 		desc->running = 0;
 		desc->pending = 0;
 		desc->disable_depth = 1;
+
+		if (new->flags & SA_TRIGGER) {
+			unsigned int type = new->flags & SA_TRIGGER;
+			desc->chip->set_type(irq, type);
+		}
+
 		if (!desc->noautoenable) {
 			desc->disable_depth = 0;
 			desc->chip->unmask(irq);
diff -puN include/linux/signal.h~irq-type-flags include/linux/signal.h
--- devel/include/linux/signal.h~irq-type-flags	2005-11-06 00:55:46.000000000 -0800
+++ devel-akpm/include/linux/signal.h	2005-11-06 00:55:46.000000000 -0800
@@ -18,6 +18,14 @@
 #define SA_PROBE		SA_ONESHOT
 #define SA_SAMPLE_RANDOM	SA_RESTART
 #define SA_SHIRQ		0x04000000
+/*
+ * As above, these correspond to the __IRQT defines in asm-arm/irq.h
+ * to select the interrupt line behaviour.
+ */
+#define SA_TRIGGER_HIGH		0x00000008
+#define SA_TRIGGER_LOW		0x00000004
+#define SA_TRIGGER_RISING	0x00000002
+#define SA_TRIGGER_FALLING	0x00000001
 
 /*
  * Real Time signals may be queued.
_

Re: irq-type-flags.patch

[William Lee Irwin III]

On Tue, Nov 08, 2005 at 07:10:57PM -0800, Andrew Morton wrote:
> Is everyone OK with this?
> From: Russell King <rmk+lkml@arm.linux.org.uk>
> Some ARM platforms have the ability to program the interrupt controller to
> detect various interrupt edges and/or levels.  For some platforms, this is
> critical to setup correctly, particularly those which the setting is
> dependent on the device.
> Currently, ARM drivers do (eg) the following:
> 	err = request_irq(irq, ...);
> 	set_irq_type(irq, IRQT_RISING);
> However, if the interrupt has previously been programmed to be level
> sensitive (for whatever reason) then this will cause an interrupt storm.
> Hence, if we combine set_irq_type() with request_irq(), we can then safely
> set the type prior to unmasking the interrupt.  The unfortunate problem is
> that in order to support this, these flags need to be visible outside of
> the ARM architecture - drivers such as smc91x need these flags and they're
> cross-architecture.
> Finally, the SA_TRIGGER_* flag passed to request_irq() should reflect the
> property that the device would like.  The IRQ controller code should do its
> best to select the most appropriate supported mode.

This seems innocuous enough to me. I anticipate that this will have no
impact on sparc32.


-- wli

Re: irq-type-flags.patch

[David Howells]

Andrew Morton <akpm@osdl.org> wrote:

> +			desc->chip->set_type(irq, type);

How's the error handled if the PIC can't handle the requested type?

It seems like it ought to be wrong that the driver is setting this. It ought
to be a property of the arch or the bus.

David

Re: irq-type-flags.patch

[Russell King]

On Wed, Nov 09, 2005 at 01:20:00PM +0000, David Howells wrote:
> Andrew Morton <akpm@osdl.org> wrote:
> 
> > +			desc->chip->set_type(irq, type);
> 
> How's the error handled if the PIC can't handle the requested type?
> 
> It seems like it ought to be wrong that the driver is setting this. It ought
> to be a property of the arch or the bus.

No it should not.  Neither the arch not the bus dictates what the
device manufacturer decides to do with their interrupt output.

It most definitely is a property of the device/driver and not a
higher level.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: irq-type-flags.patch

[Paul Mundt]

[-- Attachment #1: Type: text/plain, Size: 2055 bytes --]

On Tue, Nov 08, 2005 at 07:10:57PM -0800, Andrew Morton wrote:
> Is everyone OK with this?
> 
Looks fine to me, especially if it gets that set_irq_type() stuff out of
some of the drivers.

> diff -puN arch/arm/kernel/irq.c~irq-type-flags arch/arm/kernel/irq.c
> --- devel/arch/arm/kernel/irq.c~irq-type-flags	2005-11-06 00:55:46.000000000 -0800
> +++ devel-akpm/arch/arm/kernel/irq.c	2005-11-06 00:55:46.000000000 -0800
> @@ -681,10 +681,16 @@ int setup_irq(unsigned int irq, struct i
>  	 */
>  	desc = irq_desc + irq;
>  	spin_lock_irqsave(&irq_controller_lock, flags);
> +#define SA_TRIGGER	(SA_TRIGGER_HIGH|SA_TRIGGER_LOW|\
> +			 SA_TRIGGER_RISING|SA_TRIGGER_FALLING)
>  	p = &desc->action;
>  	if ((old = *p) != NULL) {
> -		/* Can't share interrupts unless both agree to */
> -		if (!(old->flags & new->flags & SA_SHIRQ)) {
> +		/*
> +		 * Can't share interrupts unless both agree to and are
> +		 * the same type.
> +		 */
> +		if (!(old->flags & new->flags & SA_SHIRQ) ||
> +		    (~old->flags & new->flags) & SA_TRIGGER) {
>  			spin_unlock_irqrestore(&irq_controller_lock, flags);
>  			return -EBUSY;
>  		}
> diff -puN include/linux/signal.h~irq-type-flags include/linux/signal.h
> --- devel/include/linux/signal.h~irq-type-flags	2005-11-06 00:55:46.000000000 -0800
> +++ devel-akpm/include/linux/signal.h	2005-11-06 00:55:46.000000000 -0800
> @@ -18,6 +18,14 @@
>  #define SA_PROBE		SA_ONESHOT
>  #define SA_SAMPLE_RANDOM	SA_RESTART
>  #define SA_SHIRQ		0x04000000
> +/*
> + * As above, these correspond to the __IRQT defines in asm-arm/irq.h
> + * to select the interrupt line behaviour.
> + */
> +#define SA_TRIGGER_HIGH		0x00000008
> +#define SA_TRIGGER_LOW		0x00000004
> +#define SA_TRIGGER_RISING	0x00000002
> +#define SA_TRIGGER_FALLING	0x00000001
>  
>  /*
>   * Real Time signals may be queued.
> _
It probably makes sense to move SA_TRIGGER here as well, as it's going to
be a pretty common mask. Otherwise everyone that plans to make use of
this will likely end up duplicating it.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: irq-type-flags.patch

[Matthew Wilcox]

On Wed, Nov 09, 2005 at 03:30:23PM +0200, Paul Mundt wrote:
> > +#define SA_TRIGGER_LOW		0x00000004
> > +#define SA_TRIGGER_RISING	0x00000002
> > +#define SA_TRIGGER_FALLING	0x00000001
> It probably makes sense to move SA_TRIGGER here as well, as it's going to
> be a pretty common mask. Otherwise everyone that plans to make use of
> this will likely end up duplicating it.

Are there any other architectures that plan to make use of this?

Re: irq-type-flags.patch

[Paul Mundt]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

On Wed, Nov 09, 2005 at 06:49:32AM -0700, Matthew Wilcox wrote:
> On Wed, Nov 09, 2005 at 03:30:23PM +0200, Paul Mundt wrote:
> > > +#define SA_TRIGGER_LOW		0x00000004
> > > +#define SA_TRIGGER_RISING	0x00000002
> > > +#define SA_TRIGGER_FALLING	0x00000001
> > It probably makes sense to move SA_TRIGGER here as well, as it's going to
> > be a pretty common mask. Otherwise everyone that plans to make use of
> > this will likely end up duplicating it.
> 
> Are there any other architectures that plan to make use of this?

SH will at least to some extent. There's some older SH-3's that could
benefit from this (we currently have some ugly hacks in place to handle
the edge cases for some of these legacy devices on these systems, but
it's pretty broken at the moment). The newer cores don't have a lot of
use for this though.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: irq-type-flags.patch

[Russell King]

On Wed, Nov 09, 2005 at 06:49:32AM -0700, Matthew Wilcox wrote:
> On Wed, Nov 09, 2005 at 03:30:23PM +0200, Paul Mundt wrote:
> > > +#define SA_TRIGGER_LOW		0x00000004
> > > +#define SA_TRIGGER_RISING	0x00000002
> > > +#define SA_TRIGGER_FALLING	0x00000001
> > It probably makes sense to move SA_TRIGGER here as well, as it's going to
> > be a pretty common mask. Otherwise everyone that plans to make use of
> > this will likely end up duplicating it.
> 
> Are there any other architectures that plan to make use of this?

Just to be clear - the primary reason I'm putting these in a shared
file is that we don't want to add #ifdefs to drivers (which is what
would be necessary if it was placed in asm-arm.)

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: irq-type-flags.patch

[David Howells]

Russell King <rmk@arm.linux.org.uk> wrote:

> Neither the arch not the bus dictates what the
> device manufacturer decides to do with their interrupt output.

On the other hand, the device manufacturer does not decree what the arch and
supporting chipset will *accept*. There's no guarantee that all trigger types
will be supported. For instance on my ASB2305 board, I've got an on-chip PIC
that will accept any of the four trigger types, but the PCI bus interrupt line
attached to one of the external interrupt pins will *only* accept low-level
triggered. In such a case, the bus *must* override anything set by one of the
devices.

David

Re: irq-type-flags.patch

[Matthew Wilcox]

On Wed, Nov 09, 2005 at 02:47:58PM +0000, Russell King wrote:
> On Wed, Nov 09, 2005 at 06:49:32AM -0700, Matthew Wilcox wrote:
> > On Wed, Nov 09, 2005 at 03:30:23PM +0200, Paul Mundt wrote:
> > > > +#define SA_TRIGGER_LOW		0x00000004
> > > > +#define SA_TRIGGER_RISING	0x00000002
> > > > +#define SA_TRIGGER_FALLING	0x00000001
> > > It probably makes sense to move SA_TRIGGER here as well, as it's going to
> > > be a pretty common mask. Otherwise everyone that plans to make use of
> > > this will likely end up duplicating it.
> > 
> > Are there any other architectures that plan to make use of this?
> 
> Just to be clear - the primary reason I'm putting these in a shared
> file is that we don't want to add #ifdefs to drivers (which is what
> would be necessary if it was placed in asm-arm.)

Certainly.  The question is whether it makes sense to put the SA_TRIGGER
define (being the | of all these) in arch files or in the common files.
And it sounds like since SH also needs to use this, that should also go
in adjacent to the SA_TRIGGER_* defines.

Re: irq-type-flags.patch

[Russell King]

On Wed, Nov 09, 2005 at 03:13:47PM +0000, David Howells wrote:
> Russell King <rmk@arm.linux.org.uk> wrote:
> 
> > Neither the arch not the bus dictates what the
> > device manufacturer decides to do with their interrupt output.
> 
> On the other hand, the device manufacturer does not decree what the arch and
> supporting chipset will *accept*.

David, you have completely the wrong idea about this.  The driver does
not decree anything.  The driver is telling the IRQ subsystem what type
of interrupt the chip produces.

If an _architecture_ wants to indirect that through something, all well
and good - it can, but that's up to the architectures request_irq()
internals.

> There's no guarantee that all trigger types
> will be supported. For instance on my ASB2305 board, I've got an on-chip PIC
> that will accept any of the four trigger types, but the PCI bus interrupt line
> attached to one of the external interrupt pins will *only* accept low-level
> triggered. In such a case, the bus *must* override anything set by one of the
> devices.

We have this scenario on ARM today - for years - and it isn't a problem
there.  So why is it such a massive problem here?

I think the root of this issue is that you're thinking that the driver
uses this to dictate to the interrupt controller what it wants the
controller to program itself for.  It's more a case that the driver is
saying "my device produces this kind of interrupt, please configure
the input appropriately."

Maybe the real issue here is that we should have dev_request_irq which
takes a struct device?  That's another, separate, orthogonal interface
extension which those who require it can work on.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: irq-type-flags.patch

[Russell King]

On Wed, Nov 09, 2005 at 08:26:59AM -0700, Matthew Wilcox wrote:
> On Wed, Nov 09, 2005 at 02:47:58PM +0000, Russell King wrote:
> > On Wed, Nov 09, 2005 at 06:49:32AM -0700, Matthew Wilcox wrote:
> > > On Wed, Nov 09, 2005 at 03:30:23PM +0200, Paul Mundt wrote:
> > > > > +#define SA_TRIGGER_LOW		0x00000004
> > > > > +#define SA_TRIGGER_RISING	0x00000002
> > > > > +#define SA_TRIGGER_FALLING	0x00000001
> > > > It probably makes sense to move SA_TRIGGER here as well, as it's going to
> > > > be a pretty common mask. Otherwise everyone that plans to make use of
> > > > this will likely end up duplicating it.
> > > 
> > > Are there any other architectures that plan to make use of this?
> > 
> > Just to be clear - the primary reason I'm putting these in a shared
> > file is that we don't want to add #ifdefs to drivers (which is what
> > would be necessary if it was placed in asm-arm.)
> 
> Certainly.  The question is whether it makes sense to put the SA_TRIGGER
> define (being the | of all these) in arch files or in the common files.
> And it sounds like since SH also needs to use this, that should also go
> in adjacent to the SA_TRIGGER_* defines.

I'm fine with moving it there.  Shall I do a new patch?

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: irq-type-flags.patch

[David Howells]

Russell King <rmk@arm.linux.org.uk> wrote:

> I think the root of this issue is that you're thinking that the driver
> uses this to dictate to the interrupt controller what it wants the
> controller to program itself for.  It's more a case that the driver is
> saying "my device produces this kind of interrupt, please configure
> the input appropriately."

Okay.

Still, I'd like to see some handling for a mismatch - the case in which the
driver says "my device can produce X" but X isn't available with that PIC/bus
combination. Or maybe provide a way (if there isn't one already) for the
driver to query the available trigger types. Some devices I've come across
have configurable interrupt generators.

David

Re: irq-type-flags.patch

[Andi Kleen]

On Wednesday 09 November 2005 04:10, Andrew Morton wrote:
> Is everyone OK with this?
>

These flags will be nops on x86 and many other architectures. Could
be confusing to driver writers. Can this please be made clear in comments?

-Andi

>