From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [PATCH] gpio: rcar: Fix level interrupt handling Date: Fri, 29 Nov 2013 20:22:45 +0100 Message-ID: <1632716.JFGkUWIjxn@avalon> References: <1385731940-25359-1-git-send-email-valentine.barshak@cogentembedded.com> <2631922.gOT6eudPom@avalon> <5298DC44.1020209@cogentembedded.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Return-path: Received: from perceval.ideasonboard.com ([95.142.166.194]:41964 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750994Ab3K2TWr (ORCPT ); Fri, 29 Nov 2013 14:22:47 -0500 In-Reply-To: <5298DC44.1020209@cogentembedded.com> Sender: linux-gpio-owner@vger.kernel.org List-Id: linux-gpio@vger.kernel.org To: Valentine Cc: linux-sh@vger.kernel.org, linux-gpio@vger.kernel.org, Simon Horman , Magnus Damm , Linus Walleij Hi Valentine, On Friday 29 November 2013 22:26:12 Valentine wrote: > On 11/29/2013 10:06 PM, Laurent Pinchart wrote: > > On Friday 29 November 2013 21:36:56 Valentine wrote: > >> On 11/29/2013 07:39 PM, Laurent Pinchart wrote: > >>> On Friday 29 November 2013 19:15:58 Valentine wrote: > >>>> On 11/29/2013 07:00 PM, Laurent Pinchart wrote: > >>>>> On Friday 29 November 2013 17:32:20 Valentine Barshak wrote: > >>>>>> According to the manual, if a port is set for level detection using > >>>>>> the corresponding bit in the edge/level select register and an > >>>>>> external level interrupt signal is asserted, the corresponding bit in > >>>>>> INTDT does not use the FF to hold the input. Thus, writing 1 to the > >>>>>> corresponding bits in INTCLR cannot clear the corresponding bits in > >>>>>> the INTDT register. Instead, when an external input signal is > >>>>>> stopped, the corresponding bit in INTDT is cleared automatically. > >>>>>> > >>>>>> Since the INTDT bit cannot be cleared for the level interrupts until > >>>>>> the interrupt signal is stopped, we end up with the infinite loop > >>>>>> when using deferred (threaded) IRQ handling. > >>>>>> > >>>>>> Workaround the issue by dropping level interrupts from the pending > >>>>>> mask once the interrupt is handled. If the IRQ is not cleared by the > >>>>>> handler, it will be invoked again when the interrupt is re-enabled. > >>>>> > >>>>> Isn't it an issue common to all IRQ chip drivers that should be > >>>>> handled by the IRQ core ? > >>>> > >>>> No, it isn't. > >>>> > >>>>> When using level-triggered interrupts with threaded IRQ handlers, > >>>>> the core should disable the interrupt and re-enable it after executing > >>>>> the threaded handler. > >>>> > >>>> The problem is not in disabling/re-enabling interrupts. > >>>> Even when the interrupt is disabled, the corresponding INTDT bit is not > >>>> cleared. Thus, the "while" loop never ends if a level interrupt happens > >>>> since its bit is always set in the "pending" mask. > >>>> In this case we never start deferred interrupt service routine, and > >>>> never de-assert it. > >>>> > >>>> The patch fixes this issue by dropping the IRQ bit from the "pending" > >>>> mask once the IRQ is handled at low-level. > >>> > >>> Right, I had misunderstood the purpose of your patch. I would rephrase > >>> the commit message to replace "Workaround the issue" by "Fix the issue", > >>> as this is a proper fix, not a workaround. > >> > >> OK > >> > >>> There's also another issue that, if I'm not mistaken, isn't fixed by > >>> this patch. Let's assume that a low-level level-triggered IRQ is enabled > >>> on GPIO 0 with a threaded IRQ handler and an edge-triggered IRQ is > >>> enabled GPIO 1. > >>> > >>> When GPIO 0 becomes low the gpio_rcar_irq_handler() is called and loops > >>> over the INTDT register. Only bit 0 is set, the mask is updated to mask > >>> the GPIO 0 IRQ and the corresponding IRQ handler is executed. As the IRQ > >>> is threaded the IRQ source won't be acknowledged right away, bit 0 in > >>> the INTDT register is thus not cleared. With this patch applied the loop > >>> finishes and the gpio_rcar_irq_handler() function returns. > >>> > >>> If GPIO 1 is then toggled before the thread IRQ handler for GPIO 0 is > >>> executed, the gpio_rcar_irq_handler() will be called again, and the loop > >>> will handle the GPIO 0 IRQ again as bit 0 in INTDT is still set. > >>> > >>> I'm not familiar enough with the IRQ core to know whether this problem > >>> is already handled in the core, that should be at least checked. > >> > >> The IRQ core (handle_level_irq in this case) should not start the actual > >> IRQ handler if the IRQ is disabled. > > > > OK, then there's no issue. > > > >>>>>> Signed-off-by: Valentine Barshak > >>>>>> > >>>>>> --- > >>>>>> > >>>>>> drivers/gpio/gpio-rcar.c | 10 ++++++++-- > >>>>>> 1 file changed, 8 insertions(+), 2 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c > >>>>>> index d3f15ae..918a1de 100644 > >>>>>> --- a/drivers/gpio/gpio-rcar.c > >>>>>> +++ b/drivers/gpio/gpio-rcar.c > >>>>>> @@ -166,12 +166,18 @@ static int gpio_rcar_irq_set_type(struct > >>>>>> irq_data *d, unsigned int type) > >>>>>> static irqreturn_t gpio_rcar_irq_handler(int irq, void *dev_id) > >>>>>> { > >>>>>> struct gpio_rcar_priv *p = dev_id; > >>>>>> - u32 pending; > >>>>>> + u32 pending, mask = 0; > >>>>>> unsigned int offset, irqs_handled = 0; > >>>>>> > >>>>>> - while ((pending = gpio_rcar_read(p, INTDT))) { > >>>>>> + /* > >>>>>> + * Level interrupts cannot be cleared in the INTDT, > >>>>>> + * so we just drop them from the pending mask when > >>>>>> + * the interrupt is handled. > >>>>>> + */ > >>>>>> + while ((pending = gpio_rcar_read(p, INTDT) & ~mask)) { > >>>>>> offset = __ffs(pending); > >>>>>> gpio_rcar_write(p, INTCLR, BIT(offset)); > >>>>>> + mask |= BIT(offset) & ~gpio_rcar_read(p, EDGLEVEL); > >>>>>> generic_handle_irq(irq_find_mapping(p->irq_domain, offset)); > >>>>>> irqs_handled++; > >>>>>> } > >>> > >>> What about something like this > >>> > >>> pending = gpio_rcar_read(p, INTDT) > >>> & gpio_rcar_read(p, INTMSK); > >> > >> Looks good to me. > >> I'd probably keep it inside the loop instead of caching though. > > > > I had thought about that and decided to move it before the loop to keep > > the code simple. In the rare case of IRQs triggered between the INTDT read > > and the end of the loop, those would be handled by a new call to > > gpio_rcar_irq_handler(). > > The problem is that we can't use INTMSK before the loop because the > interrupts are enabled unless the handler is called. So we need to check > the INTMSK after generic_handle_irq(). If you read both INTDT and INTMSK before the loop that shouldn't be an issue as the loop then wouldn't process the same interrupt twice anyway. > > A comment is needed to explain the logic. With the pending interrupts read > > before the loop, you could use something like > > > > /* > > * Read the pending interrupts. The INTDT bits corresponding to > > * level-triggered interrupts can't be cleared by writing to the INTCLR > > * register and will stay set until the interrupt source deassert the > > * IRQ signal. As this can be deferred when using threaded interrupt > > * handlers, we need to mask out the hardware masked interrupts to > > * avoid generating spurious interrupts. > > */ > > > > Thinking about this, masking seems to be optional as handle_level_irq() > > will ignore those interrupts if I'm not mistaken. We could then use > > I'd prefer not to step into the generic handler since, I think, increments > the IRQ counter, even when the interrupt is disabled. You're right. > > /* > > * Read the pending interrupts. Even though hardware masked > > * level-triggered interrupts will have their corresponding INTDT bit > > * set when active, there is no need to ignore them here as they will > > * be ignored by handle_level_irq(). > > */ > > pending = gpio_rcar_read(p, INTDT); > > > > while (pending) { > > offset = __ffs(pending); > > gpio_rcar_write(p, INTCLR, BIT(offset)); > > pending &= ~BIT(offset); > > generic_handle_irq(irq_find_mapping(p->irq_domain, offset)); > > irqs_handled++; > > > > } > > > >>> while (pending) { > >>> offset = __ffs(pending); > >>> gpio_rcar_write(p, INTCLR, BIT(offset)); > >>> pending &= ~BIT(offset); > >>> generic_handle_irq(irq_find_mapping(p->irq_domain, offset)); > >>> irqs_handled++; > >>> > >>> } -- Regards, Laurent Pinchart From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Date: Fri, 29 Nov 2013 19:22:45 +0000 Subject: Re: [PATCH] gpio: rcar: Fix level interrupt handling Message-Id: <1632716.JFGkUWIjxn@avalon> List-Id: References: <1385731940-25359-1-git-send-email-valentine.barshak@cogentembedded.com> <2631922.gOT6eudPom@avalon> <5298DC44.1020209@cogentembedded.com> In-Reply-To: <5298DC44.1020209@cogentembedded.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Valentine Cc: linux-sh@vger.kernel.org, linux-gpio@vger.kernel.org, Simon Horman , Magnus Damm , Linus Walleij Hi Valentine, On Friday 29 November 2013 22:26:12 Valentine wrote: > On 11/29/2013 10:06 PM, Laurent Pinchart wrote: > > On Friday 29 November 2013 21:36:56 Valentine wrote: > >> On 11/29/2013 07:39 PM, Laurent Pinchart wrote: > >>> On Friday 29 November 2013 19:15:58 Valentine wrote: > >>>> On 11/29/2013 07:00 PM, Laurent Pinchart wrote: > >>>>> On Friday 29 November 2013 17:32:20 Valentine Barshak wrote: > >>>>>> According to the manual, if a port is set for level detection using > >>>>>> the corresponding bit in the edge/level select register and an > >>>>>> external level interrupt signal is asserted, the corresponding bit in > >>>>>> INTDT does not use the FF to hold the input. Thus, writing 1 to the > >>>>>> corresponding bits in INTCLR cannot clear the corresponding bits in > >>>>>> the INTDT register. Instead, when an external input signal is > >>>>>> stopped, the corresponding bit in INTDT is cleared automatically. > >>>>>> > >>>>>> Since the INTDT bit cannot be cleared for the level interrupts until > >>>>>> the interrupt signal is stopped, we end up with the infinite loop > >>>>>> when using deferred (threaded) IRQ handling. > >>>>>> > >>>>>> Workaround the issue by dropping level interrupts from the pending > >>>>>> mask once the interrupt is handled. If the IRQ is not cleared by the > >>>>>> handler, it will be invoked again when the interrupt is re-enabled. > >>>>> > >>>>> Isn't it an issue common to all IRQ chip drivers that should be > >>>>> handled by the IRQ core ? > >>>> > >>>> No, it isn't. > >>>> > >>>>> When using level-triggered interrupts with threaded IRQ handlers, > >>>>> the core should disable the interrupt and re-enable it after executing > >>>>> the threaded handler. > >>>> > >>>> The problem is not in disabling/re-enabling interrupts. > >>>> Even when the interrupt is disabled, the corresponding INTDT bit is not > >>>> cleared. Thus, the "while" loop never ends if a level interrupt happens > >>>> since its bit is always set in the "pending" mask. > >>>> In this case we never start deferred interrupt service routine, and > >>>> never de-assert it. > >>>> > >>>> The patch fixes this issue by dropping the IRQ bit from the "pending" > >>>> mask once the IRQ is handled at low-level. > >>> > >>> Right, I had misunderstood the purpose of your patch. I would rephrase > >>> the commit message to replace "Workaround the issue" by "Fix the issue", > >>> as this is a proper fix, not a workaround. > >> > >> OK > >> > >>> There's also another issue that, if I'm not mistaken, isn't fixed by > >>> this patch. Let's assume that a low-level level-triggered IRQ is enabled > >>> on GPIO 0 with a threaded IRQ handler and an edge-triggered IRQ is > >>> enabled GPIO 1. > >>> > >>> When GPIO 0 becomes low the gpio_rcar_irq_handler() is called and loops > >>> over the INTDT register. Only bit 0 is set, the mask is updated to mask > >>> the GPIO 0 IRQ and the corresponding IRQ handler is executed. As the IRQ > >>> is threaded the IRQ source won't be acknowledged right away, bit 0 in > >>> the INTDT register is thus not cleared. With this patch applied the loop > >>> finishes and the gpio_rcar_irq_handler() function returns. > >>> > >>> If GPIO 1 is then toggled before the thread IRQ handler for GPIO 0 is > >>> executed, the gpio_rcar_irq_handler() will be called again, and the loop > >>> will handle the GPIO 0 IRQ again as bit 0 in INTDT is still set. > >>> > >>> I'm not familiar enough with the IRQ core to know whether this problem > >>> is already handled in the core, that should be at least checked. > >> > >> The IRQ core (handle_level_irq in this case) should not start the actual > >> IRQ handler if the IRQ is disabled. > > > > OK, then there's no issue. > > > >>>>>> Signed-off-by: Valentine Barshak > >>>>>> > >>>>>> --- > >>>>>> > >>>>>> drivers/gpio/gpio-rcar.c | 10 ++++++++-- > >>>>>> 1 file changed, 8 insertions(+), 2 deletions(-) > >>>>>> > >>>>>> diff --git a/drivers/gpio/gpio-rcar.c b/drivers/gpio/gpio-rcar.c > >>>>>> index d3f15ae..918a1de 100644 > >>>>>> --- a/drivers/gpio/gpio-rcar.c > >>>>>> +++ b/drivers/gpio/gpio-rcar.c > >>>>>> @@ -166,12 +166,18 @@ static int gpio_rcar_irq_set_type(struct > >>>>>> irq_data *d, unsigned int type) > >>>>>> static irqreturn_t gpio_rcar_irq_handler(int irq, void *dev_id) > >>>>>> { > >>>>>> struct gpio_rcar_priv *p = dev_id; > >>>>>> - u32 pending; > >>>>>> + u32 pending, mask = 0; > >>>>>> unsigned int offset, irqs_handled = 0; > >>>>>> > >>>>>> - while ((pending = gpio_rcar_read(p, INTDT))) { > >>>>>> + /* > >>>>>> + * Level interrupts cannot be cleared in the INTDT, > >>>>>> + * so we just drop them from the pending mask when > >>>>>> + * the interrupt is handled. > >>>>>> + */ > >>>>>> + while ((pending = gpio_rcar_read(p, INTDT) & ~mask)) { > >>>>>> offset = __ffs(pending); > >>>>>> gpio_rcar_write(p, INTCLR, BIT(offset)); > >>>>>> + mask |= BIT(offset) & ~gpio_rcar_read(p, EDGLEVEL); > >>>>>> generic_handle_irq(irq_find_mapping(p->irq_domain, offset)); > >>>>>> irqs_handled++; > >>>>>> } > >>> > >>> What about something like this > >>> > >>> pending = gpio_rcar_read(p, INTDT) > >>> & gpio_rcar_read(p, INTMSK); > >> > >> Looks good to me. > >> I'd probably keep it inside the loop instead of caching though. > > > > I had thought about that and decided to move it before the loop to keep > > the code simple. In the rare case of IRQs triggered between the INTDT read > > and the end of the loop, those would be handled by a new call to > > gpio_rcar_irq_handler(). > > The problem is that we can't use INTMSK before the loop because the > interrupts are enabled unless the handler is called. So we need to check > the INTMSK after generic_handle_irq(). If you read both INTDT and INTMSK before the loop that shouldn't be an issue as the loop then wouldn't process the same interrupt twice anyway. > > A comment is needed to explain the logic. With the pending interrupts read > > before the loop, you could use something like > > > > /* > > * Read the pending interrupts. The INTDT bits corresponding to > > * level-triggered interrupts can't be cleared by writing to the INTCLR > > * register and will stay set until the interrupt source deassert the > > * IRQ signal. As this can be deferred when using threaded interrupt > > * handlers, we need to mask out the hardware masked interrupts to > > * avoid generating spurious interrupts. > > */ > > > > Thinking about this, masking seems to be optional as handle_level_irq() > > will ignore those interrupts if I'm not mistaken. We could then use > > I'd prefer not to step into the generic handler since, I think, increments > the IRQ counter, even when the interrupt is disabled. You're right. > > /* > > * Read the pending interrupts. Even though hardware masked > > * level-triggered interrupts will have their corresponding INTDT bit > > * set when active, there is no need to ignore them here as they will > > * be ignored by handle_level_irq(). > > */ > > pending = gpio_rcar_read(p, INTDT); > > > > while (pending) { > > offset = __ffs(pending); > > gpio_rcar_write(p, INTCLR, BIT(offset)); > > pending &= ~BIT(offset); > > generic_handle_irq(irq_find_mapping(p->irq_domain, offset)); > > irqs_handled++; > > > > } > > > >>> while (pending) { > >>> offset = __ffs(pending); > >>> gpio_rcar_write(p, INTCLR, BIT(offset)); > >>> pending &= ~BIT(offset); > >>> generic_handle_irq(irq_find_mapping(p->irq_domain, offset)); > >>> irqs_handled++; > >>> > >>> } -- Regards, Laurent Pinchart