Re: gpio-pch: BUG - driver does not honour IRQF_ONESHOTn

[Thomas Gleixner]

On Wed, 25 Apr 2012, Jean-Francois Dagenais wrote:

> Isn't anyone concerned with this? I am replying to myself to provoke a response.

Here you go.
 
> Configuring a gpio pin with the gpio-pch driver with
> "IRQF_TRIGGER_LOW | IRQF_ONESHOT" generates an interrupt
> storm for threaded ISR until the ISR thread actually gets to physically clear
> the interrupt on the triggering chip!! The immediate observable symptom
> is the high CPU usage for my ISR thread task and the interrupt count in
> /proc/interrupts incrementing radically.
> See below for more details.
> 

> > Unfortunately, since all of the handling of the adp5588 is done in
> > a thread function, the interrupt stays low between the moment the
> > hard handler is run and the 5588 function is run. Since
> > pch_gpio_handler clears the interrupt status right away, I get an
> > interrupt storm for the pch_gpio_handler function.

> > the line that does "iowrite32(BIT(i), &chip->reg->iclr);" right
> > before calling generic_handle_irq should be executed only after
> > the corresponding nested ISR has run it's thread function.

Sigh, that driver is broken in several ways.

Does the following untested patch solve your problem?

It's not complete and I neither have access to that hardware nor did I
bother to look for docs.

Fact is, that using handle_simple_irq is preventing the usage of
oneshot threaded handlers for the demultiplexed interrupts, because
the simple handler does not deal with masking/unmasking. So yes you
run into an irq storm.

The "ack" of the sub interrupt before calling the handler is crap as
well. Though it kinda works as long as you are not using threaded
interrupts... 

The other nonsense is that the set_type function unconditionally
enables the interrupt. It's supposed to set the type and nothing
else. The unmasking is done by the core code. If that hardware
requires to do the type changes in masked state, then it should
indicate it to the core by adding the IRQCHIP_SET_TYPE_MASKED flag to
the irq chip. But I can't see it masking it. I just see the
unconditional unmask and enable. Looks like one of those, it works but
we don't know why thingies....

That said, the patch is just done from my core code POV and the
limited information I was able to pull out of the driver code.

Thanks,

	tglx

diff --git a/drivers/gpio/gpio-pch.c b/drivers/gpio/gpio-pch.c
index e8729cc..8b92fec 100644
--- a/drivers/gpio/gpio-pch.c
+++ b/drivers/gpio/gpio-pch.c
@@ -230,16 +230,12 @@ static void pch_gpio_setup(struct pch_gpio *chip)
 
 static int pch_irq_type(struct irq_data *d, unsigned int type)
 {
-	u32 im;
-	u32 __iomem *im_reg;
-	u32 ien;
-	u32 im_pos;
-	int ch;
-	unsigned long flags;
-	u32 val;
-	int irq = d->irq;
 	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
 	struct pch_gpio *chip = gc->private;
+	u32 im, im_pos, val;
+	u32 __iomem *im_reg;
+	unsigned long flags;
+	int ch, irq = d->irq;
 
 	ch = irq - chip->irq_base;
 	if (irq <= chip->irq_base + 7) {
@@ -270,30 +266,22 @@ static int pch_irq_type(struct irq_data *d, unsigned int type)
 	case IRQ_TYPE_LEVEL_LOW:
 		val = PCH_LEVEL_L;
 		break;
-	case IRQ_TYPE_PROBE:
-		goto end;
 	default:
-		dev_warn(chip->dev, "%s: unknown type(%dd)",
-			__func__, type);
-		goto end;
+		goto unlock;
 	}
 
 	/* Set interrupt mode */
 	im = ioread32(im_reg) & ~(PCH_IM_MASK << (im_pos * 4));
 	iowrite32(im | (val << (im_pos * 4)), im_reg);
 
-	/* iclr */
-	iowrite32(BIT(ch), &chip->reg->iclr);
+	/* And the handler */
+	if (type & (IRQ_TYPE_LEVEL_LOW | IRQ_TYPE_LEVEL_HIGH))
+		__irq_set_handler_locked(d->irq, handle_level_irq);
+	else if (type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
+		__irq_set_handler_locked(d->irq, handle_edge_irq);
 
-	/* IMASKCLR */
-	iowrite32(BIT(ch), &chip->reg->imaskclr);
-
-	/* Enable interrupt */
-	ien = ioread32(&chip->reg->ien);
-	iowrite32(ien | BIT(ch), &chip->reg->ien);
-end:
+unlock:
 	spin_unlock_irqrestore(&chip->spinlock, flags);
-
 	return 0;
 }
 
@@ -313,18 +301,24 @@ static void pch_irq_mask(struct irq_data *d)
 	iowrite32(1 << (d->irq - chip->irq_base), &chip->reg->imask);
 }
 
+static void pch_irq_ack(struct irq_data *d)
+{
+	struct irq_chip_generic *gc = irq_data_get_irq_chip_data(d);
+	struct pch_gpio *chip = gc->private;
+
+	iowrite32(1 << (d->irq - chip->irq_base), &chip->reg->iclr);
+}
+
 static irqreturn_t pch_gpio_handler(int irq, void *dev_id)
 {
 	struct pch_gpio *chip = dev_id;
 	u32 reg_val = ioread32(&chip->reg->istatus);
-	int i;
-	int ret = IRQ_NONE;
+	int i, ret = IRQ_NONE;
 
 	for (i = 0; i < gpio_pins[chip->ioh]; i++) {
 		if (reg_val & BIT(i)) {
 			dev_dbg(chip->dev, "%s:[%d]:irq=%d  status=0x%x\n",
 				__func__, i, irq, reg_val);
-			iowrite32(BIT(i), &chip->reg->iclr);
 			generic_handle_irq(chip->irq_base + i);
 			ret = IRQ_HANDLED;
 		}
@@ -343,6 +337,7 @@ static __devinit void pch_gpio_alloc_generic_chip(struct pch_gpio *chip,
 	gc->private = chip;
 	ct = gc->chip_types;
 
+	ct->chip.irq_ack = pch_irq_ack;
 	ct->chip.irq_mask = pch_irq_mask;
 	ct->chip.irq_unmask = pch_irq_unmask;
 	ct->chip.irq_set_type = pch_irq_type;
@@ -357,6 +352,7 @@ static int __devinit pch_gpio_probe(struct pci_dev *pdev,
 	s32 ret;
 	struct pch_gpio *chip;
 	int irq_base;
+	u32 msk;
 
 	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
 	if (chip == NULL)
@@ -418,8 +414,10 @@ static int __devinit pch_gpio_probe(struct pci_dev *pdev,
 
 	pch_gpio_alloc_generic_chip(chip, irq_base, gpio_pins[chip->ioh]);
 
-	/* Initialize interrupt ien register */
-	iowrite32(0, &chip->reg->ien);
+	/* Mask all interrupts, but enable them */
+	msk = (1 << gpio_pins[chip->ioh]) - 1;
+	iowrite32(msk, &chip->reg->imask);
+	iowrite32(msk, &chip->reg->ien);
 end:
 	return 0;
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/