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

Boris Brezillon
2017-06-08T11:26:45Z

On Thu, 8 Jun 2017 19:41:39 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
>
>
> 2017-06-08 16:12 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > Le Thu, 8 Jun 2017 15:10:18 +0900,
> > Masahiro Yamada <yamada.masahiro@socionext.com> a écrit :
> >
> >> 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;
> >> }
> >>
> >>
> >
> > My bad, I didn't notice you were releasing the lock after calling
> > reinit_completion(). I still find this solution more complex than my
> > proposal, but I don't care that much.
>
>
> At first, I implemented exactly like you suggested;
> denali->irq_mask = irq_mask;
> reinit_completion(&denali->complete)
> in denali_reset_irq().
>
>
> IIRC, things were like this.
>
> Some time later, you memtioned to use ->cmd_ctrl
> instead of ->cmdfunc.
>
> Then I had a problem when I needed to implement
> denali_check_irq() in
> http://patchwork.ozlabs.org/patch/772395/
>
> denali_wait_for_irq() is blocked until interested IRQ happens.
> but ->dev_ready() hook should not be blocked.
> It should return if R/B# transition has happened or not.

Nope, it should return whether the NAND is ready or not, not whether a
busy -> ready transition occurred or not. It's typically done by
reading the NAND STATUS register or by checking the R/B pin status.

> So, I accumulate IRQ events in denali->irq_status
> that have happened since denali_reset_irq().

Yep, I see that.

>
>
>
> >>
> >>
> >>
> >> > 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;
> >
> > Well, it was just a precaution, in case some interrupts weren't cleared
> > during the previous test (for example if they were masked before the
> > event actually happened, which can occur if you have a timeout, but
> > the event is detected afterward).
>
> Turning on/off IRQ mask is problematic.
> So I did not do that.

I don't see why this is a problem. That's how it usually done.

>
> I enable IRQ mask in driver probe.
> I think this approach is more robust when we consider race conditions
> like you mentioned.

I'd like to hear more about the reasons you think it's more robust
than

* at-probe-time: mask all IRQs and reset IRQ status

* when doing a specific operation:
1/ reset irq status
2/ unmask relevant irqs (based on the operation you're doing)
3/ launch the operation
4/ wait for interrupts
5/ mask irqs and check the wait_for_completion() return code + irq
status

This approach shouldn't be racy, because you're resetting+unmasking
irqs before starting the real operation (the one supposed to generate
such interrupts). By doing that you also get rid of the extra
->irq_status field, and you don't have to check irq_status before
calling wait_for_completion().