Le Thu, 8 Jun 2017 15:10:18 +0900,
Masahiro Yamada <yamada.masah...@socionext.com> a écrit :
> Hi Boris,
>
>
> 2017-06-07 22:57 GMT+09:00 Boris Brezillon
> <boris.brezil...@free-electrons.com>:
> > On Wed, 7 Jun 2017 20:52:19 +0900
> > Masahiro Yamada <yamada.masah...@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.
>
>
>
> > 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).
>
>
> Again, denali->irq_lock avoids a race between denali_reset_irq() and
> denali_irq(),
> so this works correctly.
>
>
Anyway, you seem confident that you're doing the right thing, so I'll
let you decide what is appropriate and redirect any bug report to you if
that happens :-P.
