[PATCH] pinctrl/amd: Use regular interrupt instead of chained

[PATCH] pinctrl/amd: Use regular interrupt instead of chained

[Thomas Gleixner]

The AMD pinctrl driver uses a chained interrupt to demultiplex the GPIO
interrupts. Kevin Vandeventer reported, that his new AMD Ryzen locks up
hard on boot when the AMD pinctrl driver is initialized. The reason is an
interrupt storm. It's not clear whether that's caused by hardware or
firmware or both.

Using chained interrupts on X86 is a dangerous endavour. If a system is
misconfigured or the hardware buggy there is no safety net to catch an
interrupt storm.

Convert the driver to use a regular interrupt for the demultiplex
handler. This allows the interrupt storm detector to catch the malfunction
and lets the system boot up.

This should be backported to stable because it's likely that more users run
into this problem as the AMD Ryzen machines are spreading.

Reported-by: Kevin Vandeventer
Link: https://bugzilla.suse.com/show_bug.cgi?id=1034261
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/pinctrl/pinctrl-amd.c |   91 ++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 50 deletions(-)

--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -495,64 +495,54 @@ static struct irq_chip amd_gpio_irqchip
 	.flags        = IRQCHIP_SKIP_SET_WAKE,
 };
 
-static void amd_gpio_irq_handler(struct irq_desc *desc)
+#define PIN_IRQ_PENDING	(BIT(INTERRUPT_STS_OFF) | BIT(WAKE_STS_OFF))
+
+static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
 {
-	u32 i;
-	u32 off;
-	u32 reg;
-	u32 pin_reg;
-	u64 reg64;
-	int handled = 0;
-	unsigned int irq;
+	struct amd_gpio *gpio_dev = dev_id;
+	struct gpio_chip *gc = &gpio_dev->gc;
+	irqreturn_t ret = IRQ_NONE;
+	unsigned int i, irqnr;
 	unsigned long flags;
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
-	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
+	u32 *regs, regval;
+	u64 status, mask;
 
-	chained_irq_enter(chip, desc);
-	/*enable GPIO interrupt again*/
+	/* Read the wake status */
 	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
-	reg = readl(gpio_dev->base + WAKE_INT_STATUS_REG1);
-	reg64 = reg;
-	reg64 = reg64 << 32;
-
-	reg = readl(gpio_dev->base + WAKE_INT_STATUS_REG0);
-	reg64 |= reg;
+	status = readl(gpio_dev->base + WAKE_INT_STATUS_REG1);
+	status <<= 32;
+	status |= readl(gpio_dev->base + WAKE_INT_STATUS_REG0);
 	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 
-	/*
-	 * first 46 bits indicates interrupt status.
-	 * one bit represents four interrupt sources.
-	*/
-	for (off = 0; off < 46 ; off++) {
-		if (reg64 & BIT(off)) {
-			for (i = 0; i < 4; i++) {
-				pin_reg = readl(gpio_dev->base +
-						(off * 4 + i) * 4);
-				if ((pin_reg & BIT(INTERRUPT_STS_OFF)) ||
-					(pin_reg & BIT(WAKE_STS_OFF))) {
-					irq = irq_find_mapping(gc->irqdomain,
-								off * 4 + i);
-					generic_handle_irq(irq);
-					writel(pin_reg,
-						gpio_dev->base
-						+ (off * 4 + i) * 4);
-					handled++;
-				}
-			}
+	/* Bit 0-45 contain the relevant status bits */
+	status &= (1ULL << 46) - 1;
+	regs = gpio_dev->base;
+	for (mask = 1, irqnr = 0; status; mask <<= 1, regs += 4, irqnr += 4) {
+		if (!(status & mask))
+			continue;
+		status &= ~mask;
+
+		/* Each status bit covers four pins */
+		for (i = 0; i < 4; i++) {
+			regval = readl(regs + i);
+			if (!(regval & PIN_IRQ_PENDING))
+				continue;
+			irq = irq_find_mapping(gc->irqdomain, irqnr + i);
+			generic_handle_irq(irq);
+			/* Clear interrupt */
+			writel(regval, regs + i);
+			ret = IRQ_HANDLED;
 		}
 	}
 
-	if (handled == 0)
-		handle_bad_irq(desc);
-
+	/* Signal EOI to the GPIO unit */
 	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
-	reg = readl(gpio_dev->base + WAKE_INT_MASTER_REG);
-	reg |= EOI_MASK;
-	writel(reg, gpio_dev->base + WAKE_INT_MASTER_REG);
+	regval = readl(gpio_dev->base + WAKE_INT_MASTER_REG);
+	regval |= EOI_MASK;
+	writel(regval, gpio_dev->base + WAKE_INT_MASTER_REG);
 	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 
-	chained_irq_exit(chip, desc);
+	return ret;
 }
 
 static int amd_get_groups_count(struct pinctrl_dev *pctldev)
@@ -821,10 +811,11 @@ static int amd_gpio_probe(struct platfor
 		goto out2;
 	}
 
-	gpiochip_set_chained_irqchip(&gpio_dev->gc,
-				 &amd_gpio_irqchip,
-				 irq_base,
-				 amd_gpio_irq_handler);
+	ret = devm_request_irq(&pdev->dev, irq_base, amd_gpio_irq_handler, 0,
+			       KBUILD_MODNAME, gpio_dev);
+	if (ret)
+		goto out2;
+
 	platform_set_drvdata(pdev, gpio_dev);
 
 	dev_dbg(&pdev->dev, "amd gpio driver loaded\n");

RE: [PATCH] pinctrl/amd: Use regular interrupt instead of chained

[Shah, Nehal-bakulchandra]

Hi Thomas,

Thanks for the patch. However, we have received this issue from multiple people and different disro but it occurs only on Gigabyte hardware. With reference AM4 ryzen board we are not facing this issue.
We are in discussion with gigabyte to check the BIOS part. Once we have clarity on that, we can consider driver part. Also, this code is running on multiple platform of different customers so changing directly at this point of time may be risky in my point of view. Requesting you to hold this patch till we get clarity on bios end.

Thanks for your understanding.

Regards

Nehal

-----Original Message-----
From: linux-gpio-owner@vger.kernel.org [mailto:linux-gpio-owner@vger.kernel.org] On Behalf Of Thomas Gleixner
Sent: Wednesday, May 24, 2017 2:54 AM
To: LKML <linux-kernel@vger.kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>; linux-gpio@vger.kernel.org; Borislav Petkov <bp@alien8.de>; Xue, Ken <Ken.Xue@amd.com>
Subject: [PATCH] pinctrl/amd: Use regular interrupt instead of chained

The AMD pinctrl driver uses a chained interrupt to demultiplex the GPIO interrupts. Kevin Vandeventer reported, that his new AMD Ryzen locks up hard on boot when the AMD pinctrl driver is initialized. The reason is an interrupt storm. It's not clear whether that's caused by hardware or firmware or both.

Using chained interrupts on X86 is a dangerous endavour. If a system is misconfigured or the hardware buggy there is no safety net to catch an interrupt storm.

Convert the driver to use a regular interrupt for the demultiplex handler. This allows the interrupt storm detector to catch the malfunction and lets the system boot up.

This should be backported to stable because it's likely that more users run into this problem as the AMD Ryzen machines are spreading.

Reported-by: Kevin Vandeventer
Link: https://bugzilla.suse.com/show_bug.cgi?id=1034261
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 drivers/pinctrl/pinctrl-amd.c |   91 ++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 50 deletions(-)

--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -495,64 +495,54 @@ static struct irq_chip amd_gpio_irqchip
 	.flags        = IRQCHIP_SKIP_SET_WAKE,
 };
 
-static void amd_gpio_irq_handler(struct irq_desc *desc)
+#define PIN_IRQ_PENDING	(BIT(INTERRUPT_STS_OFF) | BIT(WAKE_STS_OFF))
+
+static irqreturn_t amd_gpio_irq_handler(int irq, void *dev_id)
 {
-	u32 i;
-	u32 off;
-	u32 reg;
-	u32 pin_reg;
-	u64 reg64;
-	int handled = 0;
-	unsigned int irq;
+	struct amd_gpio *gpio_dev = dev_id;
+	struct gpio_chip *gc = &gpio_dev->gc;
+	irqreturn_t ret = IRQ_NONE;
+	unsigned int i, irqnr;
 	unsigned long flags;
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
-	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
+	u32 *regs, regval;
+	u64 status, mask;
 
-	chained_irq_enter(chip, desc);
-	/*enable GPIO interrupt again*/
+	/* Read the wake status */
 	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
-	reg = readl(gpio_dev->base + WAKE_INT_STATUS_REG1);
-	reg64 = reg;
-	reg64 = reg64 << 32;
-
-	reg = readl(gpio_dev->base + WAKE_INT_STATUS_REG0);
-	reg64 |= reg;
+	status = readl(gpio_dev->base + WAKE_INT_STATUS_REG1);
+	status <<= 32;
+	status |= readl(gpio_dev->base + WAKE_INT_STATUS_REG0);
 	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 
-	/*
-	 * first 46 bits indicates interrupt status.
-	 * one bit represents four interrupt sources.
-	*/
-	for (off = 0; off < 46 ; off++) {
-		if (reg64 & BIT(off)) {
-			for (i = 0; i < 4; i++) {
-				pin_reg = readl(gpio_dev->base +
-						(off * 4 + i) * 4);
-				if ((pin_reg & BIT(INTERRUPT_STS_OFF)) ||
-					(pin_reg & BIT(WAKE_STS_OFF))) {
-					irq = irq_find_mapping(gc->irqdomain,
-								off * 4 + i);
-					generic_handle_irq(irq);
-					writel(pin_reg,
-						gpio_dev->base
-						+ (off * 4 + i) * 4);
-					handled++;
-				}
-			}
+	/* Bit 0-45 contain the relevant status bits */
+	status &= (1ULL << 46) - 1;
+	regs = gpio_dev->base;
+	for (mask = 1, irqnr = 0; status; mask <<= 1, regs += 4, irqnr += 4) {
+		if (!(status & mask))
+			continue;
+		status &= ~mask;
+
+		/* Each status bit covers four pins */
+		for (i = 0; i < 4; i++) {
+			regval = readl(regs + i);
+			if (!(regval & PIN_IRQ_PENDING))
+				continue;
+			irq = irq_find_mapping(gc->irqdomain, irqnr + i);
+			generic_handle_irq(irq);
+			/* Clear interrupt */
+			writel(regval, regs + i);
+			ret = IRQ_HANDLED;
 		}
 	}
 
-	if (handled == 0)
-		handle_bad_irq(desc);
-
+	/* Signal EOI to the GPIO unit */
 	raw_spin_lock_irqsave(&gpio_dev->lock, flags);
-	reg = readl(gpio_dev->base + WAKE_INT_MASTER_REG);
-	reg |= EOI_MASK;
-	writel(reg, gpio_dev->base + WAKE_INT_MASTER_REG);
+	regval = readl(gpio_dev->base + WAKE_INT_MASTER_REG);
+	regval |= EOI_MASK;
+	writel(regval, gpio_dev->base + WAKE_INT_MASTER_REG);
 	raw_spin_unlock_irqrestore(&gpio_dev->lock, flags);
 
-	chained_irq_exit(chip, desc);
+	return ret;
 }
 
 static int amd_get_groups_count(struct pinctrl_dev *pctldev) @@ -821,10 +811,11 @@ static int amd_gpio_probe(struct platfor
 		goto out2;
 	}
 
-	gpiochip_set_chained_irqchip(&gpio_dev->gc,
-				 &amd_gpio_irqchip,
-				 irq_base,
-				 amd_gpio_irq_handler);
+	ret = devm_request_irq(&pdev->dev, irq_base, amd_gpio_irq_handler, 0,
+			       KBUILD_MODNAME, gpio_dev);
+	if (ret)
+		goto out2;
+
 	platform_set_drvdata(pdev, gpio_dev);
 
 	dev_dbg(&pdev->dev, "amd gpio driver loaded\n");
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] pinctrl/amd: Use regular interrupt instead of chained

[Thomas Gleixner]

Nehal,

On Fri, 26 May 2017, Shah, Nehal-bakulchandra wrote:
> Thanks for the patch. However, we have received this issue from multiple
> people and different disro but it occurs only on Gigabyte hardware. With
> reference AM4 ryzen board we are not facing this issue.  We are in
> discussion with gigabyte to check the BIOS part. Once we have clarity on
> that, we can consider driver part. Also, this code is running on multiple
> platform of different customers so changing directly at this point of
> time may be risky in my point of view. Requesting you to hold this patch
> till we get clarity on bios end.

It does not matter at all whether this is a problem only on GB
hardware. Fact is, that this happened and it will happen again.

The patch does not change any functionality of the driver, it merily makes
it more robust and spares users the bloody annoying experience of a non
booting machine and the tedious task of figuring out why.

The main objective of the kernel is robustness and not pleasing the ego of
silicon vendors. We can't prevent the stupidity of BIOS people, we merily
can deal with it.

That patch should go into mainline ASAP and backported to stable in order
to help those people who bought wreckaged hardware.

Thanks,

	tglx

RE: [PATCH] pinctrl/amd: Use regular interrupt instead of chained

[Shah, Nehal-bakulchandra]

Hi Thomas,

Thanks  for the prompt reply. Agree on points.

we will validate at our end and shall provide the update.


Nehal
-----Original Message-----
From: Thomas Gleixner [mailto:tglx@linutronix.de] 
Sent: Friday, May 26, 2017 12:19 PM
To: Shah, Nehal-bakulchandra <Nehal-bakulchandra.Shah@amd.com>
Cc: LKML <linux-kernel@vger.kernel.org>; Linus Walleij <linus.walleij@linaro.org>; linux-gpio@vger.kernel.org; Borislav Petkov <bp@alien8.de>; Xue, Ken <Ken.Xue@amd.com>; S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>; stable@vger.kernel.org
Subject: RE: [PATCH] pinctrl/amd: Use regular interrupt instead of chained

Nehal,

On Fri, 26 May 2017, Shah, Nehal-bakulchandra wrote:
> Thanks for the patch. However, we have received this issue from 
> multiple people and different disro but it occurs only on Gigabyte 
> hardware. With reference AM4 ryzen board we are not facing this issue.  
> We are in discussion with gigabyte to check the BIOS part. Once we 
> have clarity on that, we can consider driver part. Also, this code is 
> running on multiple platform of different customers so changing 
> directly at this point of time may be risky in my point of view. 
> Requesting you to hold this patch till we get clarity on bios end.

It does not matter at all whether this is a problem only on GB hardware. Fact is, that this happened and it will happen again.

The patch does not change any functionality of the driver, it merily makes it more robust and spares users the bloody annoying experience of a non booting machine and the tedious task of figuring out why.

The main objective of the kernel is robustness and not pleasing the ego of silicon vendors. We can't prevent the stupidity of BIOS people, we merily can deal with it.

That patch should go into mainline ASAP and backported to stable in order to help those people who bought wreckaged hardware.

Thanks,

	tglx

Re: [PATCH] pinctrl/amd: Use regular interrupt instead of chained

[Borislav Petkov]

On Fri, May 26, 2017 at 09:33:10AM +0000, Shah, Nehal-bakulchandra wrote:
> Hi Thomas,
> 
> Thanks  for the prompt reply. Agree on points.
> 
> we will validate at our end and shall provide the update.

First of all, please do not top-post.

What update are you talking about? The patch needs to go in regardless.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] pinctrl/amd: Use regular interrupt instead of chained

[Borislav Petkov]

On Fri, May 26, 2017 at 09:33:10AM +0000, Shah, Nehal-bakulchandra wrote:
> Hi Thomas,
> 
> Thanks  for the prompt reply. Agree on points.
> 
> we will validate at our end and shall provide the update.

Any news on the issue? Resolution?

I still have a user reporting "irq 7: nobody cared (try booting with the
"irqpoll" option)" pointing to amd_gpio_irq_handler(), even with Thomas'
patch.

See https://bugzilla.suse.com/show_bug.cgi?id=1034261

So something's still not completely fine yet.

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] pinctrl/amd: Use regular interrupt instead of chained

[Thomas Gleixner]

On Mon, 19 Jun 2017, Borislav Petkov wrote:

> On Fri, May 26, 2017 at 09:33:10AM +0000, Shah, Nehal-bakulchandra wrote:
> > Hi Thomas,
> > 
> > Thanks  for the prompt reply. Agree on points.
> > 
> > we will validate at our end and shall provide the update.
> 
> Any news on the issue? Resolution?
> 
> I still have a user reporting "irq 7: nobody cared (try booting with the
> "irqpoll" option)" pointing to amd_gpio_irq_handler(), even with Thomas'
> patch.
> 
> See https://bugzilla.suse.com/show_bug.cgi?id=1034261
> 
> So something's still not completely fine yet.

Well, that's kinda expected.

If the interrupt _IS_ screaming because the hardware is buggered, then the
nobody cared thing will detect it and switch it off. That's all what we can
do, aside of not loading the driver at all.

But that's way better than silently locking up the box forever.

Thanks

	tglx

Re: [PATCH] pinctrl/amd: Use regular interrupt instead of chained

[Borislav Petkov]

On Tue, Jun 20, 2017 at 11:22:05AM +0200, Thomas Gleixner wrote:
> If the interrupt _IS_ screaming because the hardware is buggered, then the
> nobody cared thing will detect it and switch it off. That's all what we can
> do, aside of not loading the driver at all.
> 
> But that's way better than silently locking up the box forever.

Sounds to me we should route this fix to stable.

Looking at Linus' branch:

https://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-pinctrl.git/commit/?h=fixes&id=ba714a9c1dea85e0bf2899d02dfeb9c70040427c

patch isn't tagged for stable.

Should it be?

It certainly is an improvement of the situation.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] pinctrl/amd: Use regular interrupt instead of chained

[Linus Walleij]

On Tue, May 23, 2017 at 11:23 PM, Thomas Gleixner <tglx@linutronix.de> wrote:

> The AMD pinctrl driver uses a chained interrupt to demultiplex the GPIO
> interrupts. Kevin Vandeventer reported, that his new AMD Ryzen locks up
> hard on boot when the AMD pinctrl driver is initialized. The reason is an
> interrupt storm. It's not clear whether that's caused by hardware or
> firmware or both.
>
> Using chained interrupts on X86 is a dangerous endavour. If a system is
> misconfigured or the hardware buggy there is no safety net to catch an
> interrupt storm.
>
> Convert the driver to use a regular interrupt for the demultiplex
> handler. This allows the interrupt storm detector to catch the malfunction
> and lets the system boot up.
>
> This should be backported to stable because it's likely that more users run
> into this problem as the AMD Ryzen machines are spreading.
>
> Reported-by: Kevin Vandeventer
> Link: https://bugzilla.suse.com/show_bug.cgi?id=1034261
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>

Patch applied for fixes.

Hm, I wonder if there is a bunch of other x86 drivers that should just
request the IRQ?

Yours,
Linus Walleij

Re: [PATCH] pinctrl/amd: Use regular interrupt instead of chained

[Thomas Gleixner]

On Mon, 29 May 2017, Linus Walleij wrote:

> On Tue, May 23, 2017 at 11:23 PM, Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > The AMD pinctrl driver uses a chained interrupt to demultiplex the GPIO
> > interrupts. Kevin Vandeventer reported, that his new AMD Ryzen locks up
> > hard on boot when the AMD pinctrl driver is initialized. The reason is an
> > interrupt storm. It's not clear whether that's caused by hardware or
> > firmware or both.
> >
> > Using chained interrupts on X86 is a dangerous endavour. If a system is
> > misconfigured or the hardware buggy there is no safety net to catch an
> > interrupt storm.
> >
> > Convert the driver to use a regular interrupt for the demultiplex
> > handler. This allows the interrupt storm detector to catch the malfunction
> > and lets the system boot up.
> >
> > This should be backported to stable because it's likely that more users run
> > into this problem as the AMD Ryzen machines are spreading.
> >
> > Reported-by: Kevin Vandeventer
> > Link: https://bugzilla.suse.com/show_bug.cgi?id=1034261
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> 
> Patch applied for fixes.
> 
> Hm, I wonder if there is a bunch of other x86 drivers that should just
> request the IRQ?

For sanity reasons I think so. chained interrupts are fine if you have
bootloader, device tree and kernel under control. Once BIOS/UEFI comes into
play the user is helpless against this kind of wreckage. We'll get that
same joy with ARM64 sooner than later.

Thanks,

	tglx

Re: [PATCH] pinctrl/amd: Use regular interrupt instead of chained

[Borislav Petkov]

On Mon, May 29, 2017 at 01:55:57PM +0200, Linus Walleij wrote:
> Patch applied for fixes.

Btw, any chance you can add the CC:stable tag to it or would you prefer
for me to backport it once it hits mainline?

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.

Re: [PATCH] pinctrl/amd: Use regular interrupt instead of chained

[Linus Walleij]

On Tue, Jun 20, 2017 at 2:28 PM, Borislav Petkov <bp@alien8.de> wrote:
> On Mon, May 29, 2017 at 01:55:57PM +0200, Linus Walleij wrote:
>> Patch applied for fixes.
>
> Btw, any chance you can add the CC:stable tag to it or would you prefer
> for me to backport it once it hits mainline?

This is sent to Torvalds, but you can simply suggest to Greg to pick
this for stable according to the process in:
Documentation/process/stable-kernel-rules.rst

Yours,
Linus Walleij

Re: [PATCH] pinctrl/amd: Use regular interrupt instead of chained

[Borislav Petkov]

On Wed, Jun 21, 2017 at 06:37:09PM +0200, Linus Walleij wrote:
> This is sent to Torvalds, but you can simply suggest to Greg to pick
> this for stable according to the process in:
> Documentation/process/stable-kernel-rules.rst

Yeah, I saw your pull request after hitting send. I'll send it to Greg.

Thanks.

-- 
Regards/Gruss,
    Boris.

Good mailing practices for 400: avoid top-posting and trim the reply.