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 <t...@linutronix.de>
>Cc: Gavin Shan <sha...@linux.vnet.ibm.com>
>Cc: Benjamin Herrenschmidt <b...@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);
>- }
> }
>
> /**
>
>
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
