From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Walleij Subject: Re: [PATCH v4 5/7] pinctrl: aramda-37xx: Add irqchip support Date: Wed, 26 Apr 2017 14:03:15 +0200 Message-ID: References: <70ffe3343c13d01737bf74e5de4898d0c0be07a0.1491405475.git-series.gregory.clement@free-electrons.com> <87zif38qu2.fsf@free-electrons.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: <87zif38qu2.fsf-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Gregory CLEMENT Cc: "linux-gpio-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , Thomas Petazzoni , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , Rob Herring , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Nadav Haklai , Victor Gu , Marcin Wojtas , Wilson Ding , Hua Jing , Neta Zur Hershkovits List-Id: linux-gpio@vger.kernel.org On Wed, Apr 26, 2017 at 11:23 AM, Gregory CLEMENT wrote: > On lun., avril 24 2017, Linus Walleij wrote: >>> + spin_lock_irqsave(&info->irq_lock, flags); >>> + status = readl_relaxed(info->base + IRQ_STATUS + 4 * i); >>> + /* Manage only the interrupt that was enabled */ >>> + status &= readl_relaxed(info->base + IRQ_EN + 4 * i); >>> + spin_unlock_irqrestore(&info->irq_lock, flags); >>> + while (status) { >>> + u32 hwirq = ffs(status) - 1; >>> + u32 virq = irq_find_mapping(d, hwirq + >>> + i * GPIO_PER_REG); >>> + >>> + generic_handle_irq(virq); >>> + status &= ~BIT(hwirq); >>> + } >> >> You hae a problem here is a new IRQ appears while you are inside >> of this loop. You need to re-read the status register for each iteration >> (and &= with the IRQ_EN I guess). > > If a new IRQ appears during the loop, then the irq handler will be > called again because the cause of this new IRQ won't have been acked > yet. So I think we're fine here. That *might* be true. It is true if the CPU gets a level IRQ from the GPIO controller. But hardware dealing with edge IRQs can be very quirky here, and just send a pulse on the line to the CPU if the CPU-bound IRQ is also just edge triggered. And then that pulse would potentially be missed while dealing with the current IRQ in this handler. (And exactly this happened to us on other hardware.) But anyway: why let the irq handler be called again if you can avoid it? You would avoid a double context switch by just checking it again in the loop before exiting the handler. And that can be really nice for latency-sensitive stuff. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2999552AbdDZMDa (ORCPT ); Wed, 26 Apr 2017 08:03:30 -0400 Received: from mail-io0-f181.google.com ([209.85.223.181]:36596 "EHLO mail-io0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1954510AbdDZMDR (ORCPT ); Wed, 26 Apr 2017 08:03:17 -0400 MIME-Version: 1.0 In-Reply-To: <87zif38qu2.fsf@free-electrons.com> References: <70ffe3343c13d01737bf74e5de4898d0c0be07a0.1491405475.git-series.gregory.clement@free-electrons.com> <87zif38qu2.fsf@free-electrons.com> From: Linus Walleij Date: Wed, 26 Apr 2017 14:03:15 +0200 Message-ID: Subject: Re: [PATCH v4 5/7] pinctrl: aramda-37xx: Add irqchip support To: Gregory CLEMENT Cc: "linux-gpio@vger.kernel.org" , Jason Cooper , Andrew Lunn , Sebastian Hesselbarth , Thomas Petazzoni , "linux-arm-kernel@lists.infradead.org" , Rob Herring , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Nadav Haklai , Victor Gu , Marcin Wojtas , Wilson Ding , Hua Jing , Neta Zur Hershkovits Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Apr 26, 2017 at 11:23 AM, Gregory CLEMENT wrote: > On lun., avril 24 2017, Linus Walleij wrote: >>> + spin_lock_irqsave(&info->irq_lock, flags); >>> + status = readl_relaxed(info->base + IRQ_STATUS + 4 * i); >>> + /* Manage only the interrupt that was enabled */ >>> + status &= readl_relaxed(info->base + IRQ_EN + 4 * i); >>> + spin_unlock_irqrestore(&info->irq_lock, flags); >>> + while (status) { >>> + u32 hwirq = ffs(status) - 1; >>> + u32 virq = irq_find_mapping(d, hwirq + >>> + i * GPIO_PER_REG); >>> + >>> + generic_handle_irq(virq); >>> + status &= ~BIT(hwirq); >>> + } >> >> You hae a problem here is a new IRQ appears while you are inside >> of this loop. You need to re-read the status register for each iteration >> (and &= with the IRQ_EN I guess). > > If a new IRQ appears during the loop, then the irq handler will be > called again because the cause of this new IRQ won't have been acked > yet. So I think we're fine here. That *might* be true. It is true if the CPU gets a level IRQ from the GPIO controller. But hardware dealing with edge IRQs can be very quirky here, and just send a pulse on the line to the CPU if the CPU-bound IRQ is also just edge triggered. And then that pulse would potentially be missed while dealing with the current IRQ in this handler. (And exactly this happened to us on other hardware.) But anyway: why let the irq handler be called again if you can avoid it? You would avoid a double context switch by just checking it again in the loop before exiting the handler. And that can be really nice for latency-sensitive stuff. Yours, Linus Walleij From mboxrd@z Thu Jan 1 00:00:00 1970 From: linus.walleij@linaro.org (Linus Walleij) Date: Wed, 26 Apr 2017 14:03:15 +0200 Subject: [PATCH v4 5/7] pinctrl: aramda-37xx: Add irqchip support In-Reply-To: <87zif38qu2.fsf@free-electrons.com> References: <70ffe3343c13d01737bf74e5de4898d0c0be07a0.1491405475.git-series.gregory.clement@free-electrons.com> <87zif38qu2.fsf@free-electrons.com> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Apr 26, 2017 at 11:23 AM, Gregory CLEMENT wrote: > On lun., avril 24 2017, Linus Walleij wrote: >>> + spin_lock_irqsave(&info->irq_lock, flags); >>> + status = readl_relaxed(info->base + IRQ_STATUS + 4 * i); >>> + /* Manage only the interrupt that was enabled */ >>> + status &= readl_relaxed(info->base + IRQ_EN + 4 * i); >>> + spin_unlock_irqrestore(&info->irq_lock, flags); >>> + while (status) { >>> + u32 hwirq = ffs(status) - 1; >>> + u32 virq = irq_find_mapping(d, hwirq + >>> + i * GPIO_PER_REG); >>> + >>> + generic_handle_irq(virq); >>> + status &= ~BIT(hwirq); >>> + } >> >> You hae a problem here is a new IRQ appears while you are inside >> of this loop. You need to re-read the status register for each iteration >> (and &= with the IRQ_EN I guess). > > If a new IRQ appears during the loop, then the irq handler will be > called again because the cause of this new IRQ won't have been acked > yet. So I think we're fine here. That *might* be true. It is true if the CPU gets a level IRQ from the GPIO controller. But hardware dealing with edge IRQs can be very quirky here, and just send a pulse on the line to the CPU if the CPU-bound IRQ is also just edge triggered. And then that pulse would potentially be missed while dealing with the current IRQ in this handler. (And exactly this happened to us on other hardware.) But anyway: why let the irq handler be called again if you can avoid it? You would avoid a double context switch by just checking it again in the loop before exiting the handler. And that can be really nice for latency-sensitive stuff. Yours, Linus Walleij