[03/26] powerpc: eeh: Kill another abuse of irq_desc

commit 91150af3a (powerpc/eeh: Fix unbalanced enable for IRQ) is
another brilliant example of trainwreck engineering.

The patch "fixes" the issue of an unbalanced call to irq_enable()
which causes a prominent warning by checking the disabled state of the
interrupt line and call conditionally into the core code.

This is wrong in two aspects:

1) The warning is there to tell users, that they need to fix their
   asymetric enable/disable patterns by finding the root cause and
   solving it there.

   It's definitely not meant to work around it by conditionally
   calling into the core code depending on the random state of the irq
   line.

   Asymetric irq_disable/enable calls are a clear sign of wrong usage
   of the interfaces which have to be cured at the root and not by
   somehow hacking around it.

2) The abuse of core internal data structure instead of using the
   proper interfaces for retrieving the information for the 'hack
   around'

   irq_desc is core internal and it's clear enough stated.

Replace at least the irq_desc abuse with the proper functions and add
a big fat comment why this is absurd and completely wrong.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Gavin Shan <shangw@linux.vnet.ibm.com>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: ppc <linuxppc-dev@lists.ozlabs.org>
---
 arch/powerpc/kernel/eeh_driver.c |   26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

The problems iirc have to do with drivers doing stupid things, but I'll
let Gavin comment further and fix that up properly.

Cheers,
Ben.

On Sun, Feb 23, 2014 at 09:40:09PM -0000, Thomas Gleixner wrote:
>commit 91150af3a (powerpc/eeh: Fix unbalanced enable for IRQ) is
>another brilliant example of trainwreck engineering.
>
>The patch "fixes" the issue of an unbalanced call to irq_enable()
>which causes a prominent warning by checking the disabled state of the
>interrupt line and call conditionally into the core code.
>
>This is wrong in two aspects:
>
>1) The warning is there to tell users, that they need to fix their
>   asymetric enable/disable patterns by finding the root cause and
>   solving it there.
>
>   It's definitely not meant to work around it by conditionally
>   calling into the core code depending on the random state of the irq
>   line.
>
>   Asymetric irq_disable/enable calls are a clear sign of wrong usage
>   of the interfaces which have to be cured at the root and not by
>   somehow hacking around it.
>
>2) The abuse of core internal data structure instead of using the
>   proper interfaces for retrieving the information for the 'hack
>   around'
>
>   irq_desc is core internal and it's clear enough stated.
>
>Replace at least the irq_desc abuse with the proper functions and add
>a big fat comment why this is absurd and completely wrong.
>

Thanks for pointing it out. I think we might have this patch for now
and I'll look into individual drivers to fix the unbalanced function
calls later one by one.

Thanks,
Gavin

>Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
>Cc: Gavin Shan <shangw@linux.vnet.ibm.com>
>Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>Cc: ppc <linuxppc-dev@lists.ozlabs.org>
>---
> arch/powerpc/kernel/eeh_driver.c |   26 +++++++++++++++++++++-----
> 1 file changed, 21 insertions(+), 5 deletions(-)
>
>Index: tip/arch/powerpc/kernel/eeh_driver.c
>===================================================================
>--- tip.orig/arch/powerpc/kernel/eeh_driver.c
>+++ tip/arch/powerpc/kernel/eeh_driver.c
>@@ -143,15 +143,31 @@ static void eeh_disable_irq(struct pci_d
> static void eeh_enable_irq(struct pci_dev *dev)
> {
> 	struct eeh_dev *edev = pci_dev_to_eeh_dev(dev);
>-	struct irq_desc *desc;
>
> 	if ((edev->mode) & EEH_DEV_IRQ_DISABLED) {
> 		edev->mode &= ~EEH_DEV_IRQ_DISABLED;
>-
>-		desc = irq_to_desc(dev->irq);
>-		if (desc && desc->depth > 0)
>+		/*
>+		 * FIXME !!!!!
>+		 *
>+		 * This is just ass backwards. This maze has
>+		 * unbalanced irq_enable/disable calls. So instead of
>+		 * finding the root cause it works around the warning
>+		 * in the irq_enable code by conditionally calling
>+		 * into it.
>+		 *
>+		 * That's just wrong.The warning in the core code is
>+		 * there to tell people to fix their assymetries in
>+		 * their own code, not by abusing the core information
>+		 * to avoid it.
>+		 *
>+		 * I so wish that the assymetry would be the other way
>+		 * round and a few more irq_disable calls render that
>+		 * shit unusable forever.
>+		 *
>+		 *	tglx
>+		 */
>+		if (irqd_irq_disabled(irq_get_irq_data(dev->irq))
> 			enable_irq(dev->irq);
>-	}
> }
>
> /**
>
>

On Mon, 24 Feb 2014, Gavin Shan wrote:
> On Sun, Feb 23, 2014 at 09:40:09PM -0000, Thomas Gleixner wrote:
> >commit 91150af3a (powerpc/eeh: Fix unbalanced enable for IRQ) is
> >another brilliant example of trainwreck engineering.
> >
> >The patch "fixes" the issue of an unbalanced call to irq_enable()
> >which causes a prominent warning by checking the disabled state of the
> >interrupt line and call conditionally into the core code.
> >
> >This is wrong in two aspects:
> >
> >1) The warning is there to tell users, that they need to fix their
> >   asymetric enable/disable patterns by finding the root cause and
> >   solving it there.
> >
> >   It's definitely not meant to work around it by conditionally
> >   calling into the core code depending on the random state of the irq
> >   line.
> >
> >   Asymetric irq_disable/enable calls are a clear sign of wrong usage
> >   of the interfaces which have to be cured at the root and not by
> >   somehow hacking around it.
> >
> >2) The abuse of core internal data structure instead of using the
> >   proper interfaces for retrieving the information for the 'hack
> >   around'
> >
> >   irq_desc is core internal and it's clear enough stated.
> >
> >Replace at least the irq_desc abuse with the proper functions and add
> >a big fat comment why this is absurd and completely wrong.
> >
> 
> Thanks for pointing it out. I think we might have this patch for now
> and I'll look into individual drivers to fix the unbalanced function
> calls later one by one.

Fine with me. You wont escape my scan scripts :)

Thanks,

	tglx