Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling

Masahiro Yamada
2017-06-08T06:11:11Z

Hi Boris,

2017-06-07 22:57 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Wed, 7 Jun 2017 20:52:19 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>
>> -/*
>> - * This is the interrupt service routine. It handles all interrupts
>> - * sent to this device. Note that on CE4100, this is a shared interrupt.
>> - */
>> -static irqreturn_t denali_isr(int irq, void *dev_id)
>> +static uint32_t denali_wait_for_irq(struct denali_nand_info *denali,
>> + uint32_t irq_mask)
>> {
>> - struct denali_nand_info *denali = dev_id;
>> + unsigned long time_left, flags;
>> uint32_t irq_status;
>> - irqreturn_t result = IRQ_NONE;
>>
>> - spin_lock(&denali->irq_lock);
>> + spin_lock_irqsave(&denali->irq_lock, flags);
>>
>> - /* check to see if a valid NAND chip has been selected. */
>> - if (is_flash_bank_valid(denali->flash_bank)) {
>> - /*
>> - * check to see if controller generated the interrupt,
>> - * since this is a shared interrupt
>> - */
>> - irq_status = denali_irq_detected(denali);
>> - if (irq_status != 0) {
>> - /* handle interrupt */
>> - /* first acknowledge it */
>> - clear_interrupt(denali, irq_status);
>> - /*
>> - * store the status in the device context for someone
>> - * to read
>> - */
>> - denali->irq_status |= irq_status;
>> - /* notify anyone who cares that it happened */
>> - complete(&denali->complete);
>> - /* tell the OS that we've handled this */
>> - result = IRQ_HANDLED;
>> - }
>> + irq_status = denali->irq_status;
>> +
>> + if (irq_mask & irq_status) {
>> + spin_unlock_irqrestore(&denali->irq_lock, flags);
>> + return irq_status;
>> }
>> - spin_unlock(&denali->irq_lock);
>> - return result;
>> +
>> + denali->irq_mask = irq_mask;
>> + reinit_completion(&denali->complete);
>
> These 2 instructions should be done before calling
> denali_wait_for_irq() (for example in denali_reset_irq()), otherwise
> you might loose events if they happen between your irq_status read and
> the reinit_completion() call.

No.

denali->irq_lock avoids a race between denali_isr() and
denali_wait_for_irq().

The line
denali->irq_status |= irq_status;
in denali_isr() accumulates all events that have happened
since denali_reset_irq().

If the interested IRQs have already happened
before denali_wait_for_irq(), it just return immediately
without using completion.

I do not mind adding a comment like below
if you think my intention is unclear, though.

/* Return immediately if interested IRQs have already happend. */
if (irq_mask & irq_status) {
spin_unlock_irqrestore(&denali->irq_lock, flags);
return irq_status;
}

> You should also clear existing interrupts
> before launching your operation, otherwise you might wakeup on previous
> events.

I do not see a point in your suggestion.

denali_isr() reads out IRQ_STATUS(i) and immediately clears IRQ bits.

IRQ events triggered by previous events are accumulated in denali->irq_status.

denali_reset_irq() clears it.

denali->irq_status = 0;

Again, denali->irq_lock avoids a race between denali_reset_irq() and
denali_irq(),
so this works correctly.

--
Best Regards
Masahiro Yamada