Message ID | alpine.DEB.2.20.1705232307330.2409@nanos |
---|---|
State | New |
Headers | show |
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 -- 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
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 -- 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
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.
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 -- 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
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.
--- 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");
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(-) -- 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