Re: [PATCH] gpio: rcar: Fix level interrupt handling

[Valentine <valentine.barshak@cogentembedded.com>]

On 11/29/2013 07:00 PM, Laurent Pinchart wrote:
> Hi Valentine,

Hi Laurent,

>
> Thank you for the patch.
>
> 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.

>
>> Signed-off-by: Valentine Barshak <valentine.barshak@cogentembedded.com>
>> ---
>>   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++;
>>   	}

Thanks,
Val.