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

[Gavin Shan <shangw@linux.vnet.ibm.com>]

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);
>-	}
> }
>
> /**
>
>