ARM: gic_arch_extn (Was: [PATCH v3] irqchip: mmp: add dt support for wakeup)

ARM: gic_arch_extn (Was: [PATCH v3] irqchip: mmp: add dt support for wakeup)

[Thomas Gleixner]

@all who feel responsible for gic_arch_extn

On Wed, 4 Dec 2013, Thomas Gleixner wrote:
> I'm going to reply in a separate mail on this, because you have
> brought this to my attention, but you are not responsible in the first
> place for this brainfart.

Who came up with that gic_arch_extn concept in the first place?

It forces all GIC hotpath users to do:

   hotpath_function(x)
   {
   	do_hotpath_work();

	if (random_arch_wants_crap())
	   random_arch_crap(x);
   }

Brilliant design that. Even more so that we have only a few lonely
lusers of this brainfart. Lets look at these ordered by the output of

$ git grep -l gic_arch_extn

arch/arm/mach-imx/gpc.c
arch/arm/mach-omap2/omap-wakeupgen.c
arch/arm/mach-shmobile/intc-sh73a0.c
arch/arm/mach-shmobile/setup-r8a7779.c
arch/arm/mach-tegra/irq.c
arch/arm/mach-ux500/cpu.c

So looking at the first instance makes me go berserk already 

arch/arm/mach-imx/gpc.c

  This has the following repeating pattern:

  imx_gpc_irq_XXX(struct irq_data *d)
  {
	if (d->irq < 32)
	   return;  

  So the person who comitted that crime did notice, that the upper
  layer calls this for all interrupts even those < 32, but he could
  not be arsed to sit down and avoid that.

  Even worse this resulted in the following totaly misleading comment
  above the irq number < 32 check:

  	/* Sanity check for SPI irq */
	if (d->irq < 32)

  This has nothing to do with sanity.

       A sanity check is applied in case that something is expected to
       be always correct, but where we want to catch the corner case
       which we did not imagine yet.

  So what is this (d->irq < 32) check about?

      It's a proof of incompetence because the only lame excuse of
      implementing this nonsense is:

	 /*
	  * We are lazy and do that check on all irqs, but we could
	  * avoid that if we would register a different irq_chip for
	  * these irq lines.
	  */

And I really stop here, because all other places using that nonsense
are more or less equally braindamaged.

I leave that as a an exercise to those who are responsible for the
initial implementation of gic_arch_extn and those who blindly used it.

FYI, this made me even more alert of drivers/irqchip/ being used as a
dump ground for random nonsense. It's on my high prio watch list now
and you better get your gear together and clean up the mess before I
go berserk on you.

Non-Subtle-Hint: Get rid of gic_arch_extn

Thanks,

	tglx

ARM: gic_arch_extn (Was: [PATCH v3] irqchip: mmp: add dt support for wakeup)

[Russell King - ARM Linux]

On Thu, Dec 05, 2013 at 01:41:53AM +0100, Thomas Gleixner wrote:
> @all who feel responsible for gic_arch_extn
> 
> On Wed, 4 Dec 2013, Thomas Gleixner wrote:
> > I'm going to reply in a separate mail on this, because you have
> > brought this to my attention, but you are not responsible in the first
> > place for this brainfart.
> 
> Who came up with that gic_arch_extn concept in the first place?

If you'd spend more time reviewing IRQ patches then maybe you'd catch
this at review time.  So please stop your rediculous whinging when
most of the problem is your own lack of time.

If you must know, it was introduced by TI to work around the power
management shortcomings of the architecture mandated GIC.  No it doesn't
get called for IPIs, but it damned well needs to be called for normal
IRQs.

At the point it was created, it wasn't clear whether this also applied
to local IRQs.  Since I *no* *longer* have visibility of what SoC stuff
is doing with it, of course it's not going to get fixed when a common
pattern emerges.

So... congratulations, you've found something which can be improved,
which has come to light as the code has evolved and a better
understanding of what is required has been discovered.

ARM: gic_arch_extn (Was: [PATCH v3] irqchip: mmp: add dt support for wakeup)

[Thomas Gleixner]

Russell,

On Thu, 5 Dec 2013, Russell King - ARM Linux wrote:
> On Thu, Dec 05, 2013 at 01:41:53AM +0100, Thomas Gleixner wrote:
> > @all who feel responsible for gic_arch_extn
> > 
> > On Wed, 4 Dec 2013, Thomas Gleixner wrote:
> > > I'm going to reply in a separate mail on this, because you have
> > > brought this to my attention, but you are not responsible in the first
> > > place for this brainfart.
> > 
> > Who came up with that gic_arch_extn concept in the first place?
> 
> If you'd spend more time reviewing IRQ patches then maybe you'd catch
> this at review time.  So please stop your rediculous whinging when
> most of the problem is your own lack of time.

I'm not a native english speaker, so I want to make sure in the first
place that you meant:

    "ridiculous whingeing" 

Assumed that you meant that, let me ridicule you a bit.

The gic_arch_extn concept got merged with:

    commit d7ed36a4ea84e3a850f9932e2058ceef987d1acd
    Author: Santosh Shilimkar <santosh.shilimkar@ti.com>
    Date:   Wed Mar 2 08:03:22 2011 +0100

    ARM: 6777/1: gic: Add hooks for architecture specific extensions

    <SNIP>

    Cc: Russell King <rmk+kernel@arm.linux.org.uk>
    Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
    Acked-by: Colin Cross <ccross@android.com>
    Tested-by: Colin Cross <ccross@android.com>
    Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

    ---
    arch/arm/common/gic.c		|   47 ++++++++++++....
    arch/arm/include/asm/hardware/gic.h |    1

The patch in question was never cc'ed to me and you merged it on your
own.

So now you have the chuzpe to blame me for that, just because this
code moved to drivers/irqchip with

     commit 81243e444c6e9d1625073e4a3d3bc244c8a545f0
     Author: Rob Herring <rob.herring@calxeda.com>
     Date:   Tue Nov 20 21:21:40 2012 -0600

     irqchip: Move ARM GIC to drivers/irqchip
    
almost two years later?

The code move neither exempts you from the responsibility of merging
it nor does it imply a retroactive responsibility for me to review all
patches which went into that code prior to the move.

Thanks,

	tglx

ARM: gic_arch_extn (Was: [PATCH v3] irqchip: mmp: add dt support for wakeup)

[Russell King - ARM Linux]

On Thu, Dec 05, 2013 at 03:12:55AM +0100, Thomas Gleixner wrote:
> Russell,
> 
> On Thu, 5 Dec 2013, Russell King - ARM Linux wrote:
> > On Thu, Dec 05, 2013 at 01:41:53AM +0100, Thomas Gleixner wrote:
> > > @all who feel responsible for gic_arch_extn
> > > 
> > > On Wed, 4 Dec 2013, Thomas Gleixner wrote:
> > > > I'm going to reply in a separate mail on this, because you have
> > > > brought this to my attention, but you are not responsible in the first
> > > > place for this brainfart.
> > > 
> > > Who came up with that gic_arch_extn concept in the first place?
> > 
> > If you'd spend more time reviewing IRQ patches then maybe you'd catch
> > this at review time.  So please stop your rediculous whinging when
> > most of the problem is your own lack of time.
> 
> I'm not a native english speaker, so I want to make sure in the first
> place that you meant:
> 
>     "ridiculous whingeing" 
> 
> Assumed that you meant that, let me ridicule you a bit.
> 
> The gic_arch_extn concept got merged with:
> 
>     commit d7ed36a4ea84e3a850f9932e2058ceef987d1acd
>     Author: Santosh Shilimkar <santosh.shilimkar@ti.com>
>     Date:   Wed Mar 2 08:03:22 2011 +0100
> 
>     ARM: 6777/1: gic: Add hooks for architecture specific extensions
> 
>     <SNIP>
> 
>     Cc: Russell King <rmk+kernel@arm.linux.org.uk>
>     Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>     Acked-by: Colin Cross <ccross@android.com>
>     Tested-by: Colin Cross <ccross@android.com>
>     Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> 
>     ---
>     arch/arm/common/gic.c		|   47 ++++++++++++....
>     arch/arm/include/asm/hardware/gic.h |    1
> 
> The patch in question was never cc'ed to me and you merged it on your
> own.
> 
> So now you have the chuzpe to blame me for that, just because this
> code moved to drivers/irqchip with
> 
>      commit 81243e444c6e9d1625073e4a3d3bc244c8a545f0
>      Author: Rob Herring <rob.herring@calxeda.com>
>      Date:   Tue Nov 20 21:21:40 2012 -0600
> 
>      irqchip: Move ARM GIC to drivers/irqchip
>     
> almost two years later?
> 
> The code move neither exempts you from the responsibility of merging
> it nor does it imply a retroactive responsibility for me to review all
> patches which went into that code prior to the move.

And neither does it give you permission to send such an idiotic and
rediculous email.

I'm not going to do anything about it because "Thomas Glexiner" has
suddenly decided he doesn't like it.

As for your definition of "hotpath", you're really screwed on that
because you don't seem to understand what is or isn't the hotpath in
this code.

So there's not much point discussing this with you until you:

(a) calm down
(b) analyse it properly and work out the frequency under which each
class of IRQ (those >= 32 and those < 32) call into these functions.

To put it bluntly, you're wrong.

ARM: gic_arch_extn (Was: [PATCH v3] irqchip: mmp: add dt support for wakeup)

[Thomas Gleixner]

On Thu, 5 Dec 2013, Russell King - ARM Linux wrote:
> So there's not much point discussing this with you until you:
> 
> (a) calm down

Done so :)

> (b) analyse it properly and work out the frequency under which each
> class of IRQ (those >= 32 and those < 32) call into these functions.

Here you go:

The frequency of invoking the gic_arch_extn callbacks is exactly equal
to the frequency of interrupts in the system which go through the GIC
at least for mask/unmask/eoi. The frequency of calls per interrupt
depends on the interrupt type, but@minimum it is one.

So basically it does for any interrupt independent of >= 32 or < 32:

irq_fn(d)
{
	do_something_common(d);
	if (gic_arch_extn.fn)
	   gic_arch_extn.fn(d);
	do_something_common(d);
}

which then does:

extn_fn(d)
{
	if (this_irq_is_affected(d))
	   do_some_more(d);
}      

So when this_irq_is_affected(d) boils down to

   if (d->irq [<>] n)

then I agree, that it's debatable, whether the conditonal function
call and the simple this_irq_is_affected(d) check is worth to worry
about. Though there are people who care about two pointless
conditonals for various reasons.

But at the point when this_irq_is_affected(d) is doing a loop lookup,
then this really starts to smell badly.

Sure you might argue, that it's the problem of that particular patch
author to put a loop lookup into this_irq_is_affected().

Fair enough, though you have to admit that the design of the
gic_arch_extn actually enforces that, if you can't do a simple "if irq
[<>] n" check for whatever reason.

The alternative approach to that is to use different irq chip
implementations for interrupts affected by gic_arch_extn and those
which are not affected as we do in lot of other places do deal with
the subtle differences of particular interrupt lines. That definitely
would avoid that people try to stick more complex decision functions
than "irq [<>] n" into this_irq_is_affected().

Now looking at the locking szenario of GIC, it might eventually create
quite some duplicated code, which is undesirable as well. OTOH, the
locking requirements especially if I look at gic_[un]mask_irq and
gic_eoi_irq are not entirely clear to me from the code.

gic_[un]mask_irq(d)
{
	mask = 1 << SFT(gic_irq(d));

	lock(controller_lock);
	if (gic_arch_extn.irq_[un]mask)
	   gic_arch_extn.irq_[un]mask(d);
	writel(mask, GIC_ENABLE_[CLEAR|SET]);
	unlock(controller_lock);
}

while

gic_eoi_irq(d)
{
	if (gic_arch_extn.irq_eoi) {
	   lock(controller_lock);
	   gic_arch_extn.irq_eoi(d);
	   unlock(controller_lock);
	}
	writel(gic_irq(d), GIC_EOI);
}

So is there a requirement to serialize the mask/unmask operations for
a particular interrupt line against mask/unmask operations on a
different core on some other interrupt line?

  The operations for a particular interrupt line are already
  serialized at the core code level.

  The CLEAR/SET registers are designed to avoid locking in contrary to
  the classic ENABLE reg, where you have to maintain consistency of
  the full set of interrupts affected by that register.

  So for the case where gic_arch_extn is not used, the locking is
  completely pointless, right?

Or is this locking required to maintain consistency between the
gic_arch_extn.[un]mask and the GIC.[un]mask itself?

And even if the locking is required for this, then having two separate
chips with two different callbacks makes sense.

gic_mask_irq()
{
	writel(mask, GIC_ENABLE_CLEAR);
}

gic_mask_extn_irq(d)
{
	lock(controller_lock);
	gic_arch_extn.irq_mask(d);
	gic_mask_irq(d);
	unlock(controller_lock);
}

And then have the gic_chip and gic_extn_chip set for the various
interrupt lines.

That avoids several things:

1) The locking for non gic_arch_extn interrupts

2) Two conditionals for gic_arch_extn interrupts

3) The enforcement of adding complex decision functions into the
   gic_extn functions if there is no simple x < N check possible.

I might have missed something as always, but@least I did a proper
analysis of the code as it is understandable to a mere mortal.

Thoughts?

Thanks,

	tglx

ARM: gic_arch_extn (Was: [PATCH v3] irqchip: mmp: add dt support for wakeup)

[Russell King - ARM Linux]

On Fri, Dec 06, 2013 at 10:25:49PM +0100, Thomas Gleixner wrote:
> The frequency of invoking the gic_arch_extn callbacks is exactly equal
> to the frequency of interrupts in the system which go through the GIC
> at least for mask/unmask/eoi. The frequency of calls per interrupt
> depends on the interrupt type, but at minimum it is one.
> 
> So basically it does for any interrupt independent of >= 32 or < 32:
> 
> irq_fn(d)
> {
> 	do_something_common(d);
> 	if (gic_arch_extn.fn)
> 	   gic_arch_extn.fn(d);
> 	do_something_common(d);
> }
> 
> which then does:
> 
> extn_fn(d)
> {
> 	if (this_irq_is_affected(d))
> 	   do_some_more(d);
> }      
> 
> So when this_irq_is_affected(d) boils down to
> 
>    if (d->irq [<>] n)
> 
> then I agree, that it's debatable, whether the conditonal function
> call and the simple this_irq_is_affected(d) check is worth to worry
> about. Though there are people who care about two pointless
> conditonals for various reasons.


Right - and as you correctly identify, it has become clear through code
evolution that there is a difference between the extended handling of
IRQs above and below the 32 breakpoint.

Now, what you previously termed the hotpath - IRQs less than 32 - isn't
the path we want to optimise for, unless it is our intention to optimise
for an idle system.  Why?

IRQs 0-15 won't be seen in this path - they're the IPIs which are handled
completely outside of genirq.  So that leaves IRQs 16 to 31.  Of course,
only one or two are normally used (the watchdog driver has been removed,
so we're really down to just the local timer interrupt.)  In a system
doing work, you're going to have normal IRQs (IRQs >= 32) occuring,
probably at a much faster rate than the local timer interrupt.

Hence, there are two cases to optimise for: the case without the
extension hooks, and the case with the extension hook for IRQs >= 32.
Optimising the case for IRQ < 32 makes no sense because we're
effectively optimising for one IRQ vs all the rest.

Moving the test for IRQs >= 32 to the GIC code puts extra code and
overhead into that path, perturbing the case without extension - and
that's the use case we want to encourage.  Hence, having that test in
the code where the extension is needed makes total sense.

> But at the point when this_irq_is_affected(d) is doing a loop lookup,
> then this really starts to smell badly.

And why would that be a loop?  I can't speak for how it's been used
since it was introduced for OMAP - and it's well known why - remember,
we have arm-soc, and arm-soc deals with the SoC specific code, and as
such I no longer know what SoCs are doing with this stuff.

> The alternative approach to that is to use different irq chip
> implementations for interrupts affected by gic_arch_extn and those
> which are not affected as we do in lot of other places do deal with
> the subtle differences of particular interrupt lines. That definitely
> would avoid that people try to stick more complex decision functions
> than "irq [<>] n" into this_irq_is_affected().

... which results in more complexity.  Do we need complex code?  Do
we have implementations which loop?  Why make the thing complex in this
way if it's not actually required.  What you seem to be asking is for
overdesign to happen.  That's completely contary to the Linux philosophy.

Now, it may be that _since_ this code was originally merged, it does
need this complexity, but I'm not aware of it (see above for _why_), and
no one else has spotted this.  So to rant about it as if it's the worst
thing on the planet is a total over-reaction.

You may also like to note that I'm not Cc'd on GIC patches anymore.

> Now looking at the locking szenario of GIC, it might eventually create
> quite some duplicated code, which is undesirable as well. OTOH, the
> locking requirements especially if I look at gic_[un]mask_irq and
> gic_eoi_irq are not entirely clear to me from the code.
> 
> gic_[un]mask_irq(d)
> {
> 	mask = 1 << SFT(gic_irq(d));
> 
> 	lock(controller_lock);
> 	if (gic_arch_extn.irq_[un]mask)
> 	   gic_arch_extn.irq_[un]mask(d);
> 	writel(mask, GIC_ENABLE_[CLEAR|SET]);
> 	unlock(controller_lock);
> }

Do you really want to know what's absolutely hillarious here?  You're
now complaining about the locking in here...

commit c4bfa28aec58c588de55babe99f4c172ec534704
Author: Thomas Gleixner <tglx@linutronix.de>
Date:   Sat Jul 1 22:32:14 2006 +0100

    [ARM] 3686/1: ARM: arm/common: convert irq handling
    
    Patch from Thomas Gleixner
    
    From: Thomas Gleixner <tglx@linutronix.de>
    
    Convert the files in arch/arm/common to use the generic
    irq handling functions.
    
    Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
    Signed-off-by: Ingo Molnar <mingo@elte.hu>
    Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>

diff --git a/arch/arm/common/gic.c b/arch/arm/common/gic.c
index c02dc8116a18..f3c1ebfdd0aa 100644
--- a/arch/arm/common/gic.c
+++ b/arch/arm/common/gic.c
@@ -33,6 +33,7 @@
 
 static void __iomem *gic_dist_base;
 static void __iomem *gic_cpu_base;
+static DEFINE_SPINLOCK(irq_controller_lock);
 
 /*
  * Routines to acknowledge, disable and enable interrupts
@@ -52,32 +53,45 @@ static void __iomem *gic_cpu_base;
 static void gic_ack_irq(unsigned int irq)
 {
        u32 mask = 1 << (irq % 32);
+
+       spin_lock(&irq_controller_lock);
        writel(mask, gic_dist_base + GIC_DIST_ENABLE_CLEAR + (irq / 32) * 4);
        writel(irq, gic_cpu_base + GIC_CPU_EOI);
+       spin_unlock(&irq_controller_lock);
 }
 
 static void gic_mask_irq(unsigned int irq)
 {
        u32 mask = 1 << (irq % 32);
+
+       spin_lock(&irq_controller_lock);
        writel(mask, gic_dist_base + GIC_DIST_ENABLE_CLEAR + (irq / 32) * 4);
+       spin_unlock(&irq_controller_lock);
 }
 
 static void gic_unmask_irq(unsigned int irq)
 {
        u32 mask = 1 << (irq % 32);
+
+       spin_lock(&irq_controller_lock);
        writel(mask, gic_dist_base + GIC_DIST_ENABLE_SET + (irq / 32) * 4);
+       spin_unlock(&irq_controller_lock);
 }

... which is something you added in the first place, when you converted
the GIC to your genirq stuff. :)

> And even if the locking is required for this, then having two separate
> chips with two different callbacks makes sense.
> 
> gic_mask_irq()
> {
> 	writel(mask, GIC_ENABLE_CLEAR);
> }
> 
> gic_mask_extn_irq(d)
> {
> 	lock(controller_lock);
> 	gic_arch_extn.irq_mask(d);
> 	gic_mask_irq(d);
> 	unlock(controller_lock);
> }
> 
> And then have the gic_chip and gic_extn_chip set for the various
> interrupt lines.

Since knowledge of what platforms do with this extension has been long
lost (well, no one person ever had it because responsibility for this
stuff has been devolved...) I doubt we can safely get rid of that lock
without someone auditing this stuff.

Since I'm apparantly no longer responsible for the GIC - and I'm rarely
copied with patches for it anymore, I've basically washed my hands of
it - it seems everyone else maintains it now.