Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1660780 > unrolled thread

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

Started byMasahiro Yamada <yamada.masahiro@socionext.com>
First post2017-06-08 08:20 +0200
Last post2017-06-13 06:50 +0200
Articles 10 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-08 08:20 +0200
    Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-08 09:20 +0200
      Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-08 12:50 +0200
        Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-08 13:30 +0200
          Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-08 15:00 +0200
            Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-08 17:50 +0200
              Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-08 19:30 +0200
                Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-08 19:40 +0200
                Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-09 10:00 +0200
                  Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 06:50 +0200

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

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2017-06-08 08:20 +0200
SubjectRe: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling
Message-ID<tPYXv-yI-7@gated-at.bofh.it>
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

[toc] | [next] | [standalone]

#1660815

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2017-06-08 09:20 +0200
Message-ID<tPZTA-1ak-7@gated-at.bofh.it>
In reply to#1660780
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.

> 
> 
> 
> > 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.

[toc] | [prev] | [next] | [standalone]

#1661052

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2017-06-08 12:50 +0200
Message-ID<tQ3aN-3bB-5@gated-at.bofh.it>
In reply to#1660815
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.
So, I accumulate IRQ events in denali->irq_status
that have happened since denali_reset_irq().



>>
>>
>>
>> > 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 enable IRQ mask in driver probe.
I think this approach is more robust when we consider race conditions
like you mentioned.

>>
>>
>> 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.

Yeah.

I came up with this solution after my long thought and efforts,
so I'd like to go with this.


-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1661068

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2017-06-08 13:30 +0200
Message-ID<tQ3Nv-3Dr-9@gated-at.bofh.it>
In reply to#1661052
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().

[toc] | [prev] | [next] | [standalone]

#1661155

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2017-06-08 15:00 +0200
Message-ID<tQ5cC-4oc-15@gated-at.bofh.it>
In reply to#1661068
Hi Boris,

2017-06-08 20:26 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> 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.

Checking the R/B pin is probably impossible unless
the pin is changed into a GPIO port.

I also considered NAND_CMD_STATUS, but
I can not recall why I chose the current approach.
Perhaps I thought returning detected IRQ
is faster than accessing the chip for NAND_CMD_STATUS.

I can try NAND_CMD_STATUS approach if you like.





>> 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().


IIRC, I was thinking like this:

One IRQ line may be shared among multiple hardware including Denali.
denali_pci may do this.

The Denali IRQ handler need to check irq status
because it should return IRQ_HANDLED if the event comes from Denali controller.
Otherwise, the event comes from different hardware, so
Denali IRQ handler should return IRQ_NONE.

wait_for_completion_timeout() may bail out with timeout error,
then proceed to denali_reset_irq() for the next operation.
Afterwards, the event actually may happen, and invoke IRQ handler.

denali_reset_irq() and denali_isr() compete to grab the spin lock.

If denali_reset_irq() wins, it clears INTR_STATUS register
(if implemented like you suggested first) or changes IRQ mask for the
next event.
After that, denali_isr enters the critical section and checks IRQ bit
but at this moment, the IRQ bit has gone.  So, it assumes this event
is not for Denali, so returns IRQ_NONE.  Nobody returns IRQ_HANDLED.

Then, kernel will complain "irq *: nobody cared"


In my opinion, IRQ should be checked and cleared in one place
(in IRQ handler).

Enabling/disabling IRQ mask is not problem unless it masks out
already-asserted IRQ status bits.


-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1661412

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2017-06-08 17:50 +0200
Message-ID<tQ7R8-66J-11@gated-at.bofh.it>
In reply to#1661155
On Thu, 8 Jun 2017 21:58:00 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris,
> 
> 2017-06-08 20:26 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > 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.  
> 
> Checking the R/B pin is probably impossible unless
> the pin is changed into a GPIO port.
> 
> I also considered NAND_CMD_STATUS, but
> I can not recall why I chose the current approach.
> Perhaps I thought returning detected IRQ
> is faster than accessing the chip for NAND_CMD_STATUS.
> 
> I can try NAND_CMD_STATUS approach if you like.

Depends what you're trying to do. IIUC, you use denali_wait_for_irq()
inside your ->reset()/->read/write_{page,oob}[_raw]() methods, which is
perfectly fine (assuming CUSTOM_PAGE_ACCESS is set) since these hooks
are expected to wait for chip readiness before returning.

You could also implement ->waitfunc() using denali_wait_for_irq() if
you're able to detect R/B transitions, but I'm not sure it's worth it,
because you overload almost all the methods using this hook (the only
one remaining is ->onfi_set_features(), and using STATUS polling should
not be an issue in this case).

Implementing ->dev_ready() is not necessary. When not provided, the
core falls back to STATUS polling and you seem to support
NAND_CMD_STATUS in denali_cmdfunc(). Note that even if it's not fully
reliable in the current driver, you're switching to ->cmd_ctrl() at the
end of the series anyway, so we should be good after that.

> 
> 
> 
> 
> 
> >> 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().  
> 
> 
> IIRC, I was thinking like this:
> 
> One IRQ line may be shared among multiple hardware including Denali.
> denali_pci may do this.
> 
> The Denali IRQ handler need to check irq status
> because it should return IRQ_HANDLED if the event comes from Denali controller.
> Otherwise, the event comes from different hardware, so
> Denali IRQ handler should return IRQ_NONE.

Correct.

> 
> wait_for_completion_timeout() may bail out with timeout error,
> then proceed to denali_reset_irq() for the next operation.

Before calling denali_reset_irq() you should re-mask the irqs you
unmasked in #1. Actually, calling denali_reset_irq() after
wait_for_completion_timeout() is not even needed here because you'll
clear pending irqs before launching the next NAND command.

> Afterwards, the event actually may happen, and invoke IRQ handler.

Not if you masked IRQs after wait_for_completion_timeout() returned.

> 
> denali_reset_irq() and denali_isr() compete to grab the spin lock.
> 
> If denali_reset_irq() wins, it clears INTR_STATUS register
> (if implemented like you suggested first) or changes IRQ mask for the
> next event.
> After that, denali_isr enters the critical section and checks IRQ bit
> but at this moment, the IRQ bit has gone.  So, it assumes this event
> is not for Denali, so returns IRQ_NONE.  Nobody returns IRQ_HANDLED.

Not if you have masked the interrupts.

> 
> Then, kernel will complain "irq *: nobody cared"
> 
> 
> In my opinion, IRQ should be checked and cleared in one place
> (in IRQ handler).
> 
> Enabling/disabling IRQ mask is not problem unless it masks out
> already-asserted IRQ status bits.

Here is a patch to show you what I had in mind [1] (it applies on top
of this patch). AFAICT, there's no races, no interrupt loss, and you
get rid of the ->irq_mask/status/lock fields.

[1]http://code.bulix.org/fufia6-145571

[toc] | [prev] | [next] | [standalone]

#1661509

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2017-06-08 19:30 +0200
Message-ID<tQ9pT-79G-1@gated-at.bofh.it>
In reply to#1661412
Hi Boris

2017-06-09 0:43 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Thu, 8 Jun 2017 21:58:00 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> Hi Boris,
>>
>> 2017-06-08 20:26 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> > 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.
>>
>> Checking the R/B pin is probably impossible unless
>> the pin is changed into a GPIO port.
>>
>> I also considered NAND_CMD_STATUS, but
>> I can not recall why I chose the current approach.
>> Perhaps I thought returning detected IRQ
>> is faster than accessing the chip for NAND_CMD_STATUS.
>>
>> I can try NAND_CMD_STATUS approach if you like.
>
> Depends what you're trying to do. IIUC, you use denali_wait_for_irq()
> inside your ->reset()/->read/write_{page,oob}[_raw]() methods, which is
> perfectly fine (assuming CUSTOM_PAGE_ACCESS is set) since these hooks
> are expected to wait for chip readiness before returning.
>
> You could also implement ->waitfunc() using denali_wait_for_irq() if
> you're able to detect R/B transitions,

R/B transition will set INTR__INT_ACT interrupt.

I think it is easy in my implementation of denali_wait_for_irq(),
like

   denali_wait_for_irq(denali, INTR__INT_ACT);



But, you are suggesting me to change it.
In your way, you give IRQ masks to denali_reset_irq(), like
denali_reset_irq(denali, INTR__ERASE_COMP | INTR__ERASE_FAIL);

Then, we have no room of IRQ bit in denali_wait_for_irq().

How will you implement it?







> but I'm not sure it's worth it,
> because you overload almost all the methods using this hook (the only
> one remaining is ->onfi_set_features(), and using STATUS polling should
> not be an issue in this case).
>
> Implementing ->dev_ready() is not necessary. When not provided, the
> core falls back to STATUS polling and you seem to support
> NAND_CMD_STATUS in denali_cmdfunc(). Note that even if it's not fully
> reliable in the current driver, you're switching to ->cmd_ctrl() at the
> end of the series anyway, so we should be good after that.

->dev_ready() is optional, but we may end up with waiting more than needed.

        case NAND_CMD_RESET:
                if (chip->dev_ready)
                         break;
                udelay(chip->chip_delay);


chip->chip_delay is probably set large enough, so this is not optimal.




If I add something more, the following two bugs were found by
denali_dev_ready().

commit 3158fa0e739615769cc047d2428f30f4c3b6640e
commit c5d664aa5a4c4b257a54eb35045031630d105f49


If NAND core is fine, denali_dev_ready() works fine too.

If not, it is a sign of bug of nand_command(_lp).
This is contributing to the core improvement.





>>
>> IIRC, I was thinking like this:
>>
>> One IRQ line may be shared among multiple hardware including Denali.
>> denali_pci may do this.
>>
>> The Denali IRQ handler need to check irq status
>> because it should return IRQ_HANDLED if the event comes from Denali controller.
>> Otherwise, the event comes from different hardware, so
>> Denali IRQ handler should return IRQ_NONE.
>
> Correct.
>
>>
>> wait_for_completion_timeout() may bail out with timeout error,
>> then proceed to denali_reset_irq() for the next operation.
>
> Before calling denali_reset_irq() you should re-mask the irqs you
> unmasked in #1. Actually, calling denali_reset_irq() after
> wait_for_completion_timeout() is not even needed here because you'll
> clear pending irqs before launching the next NAND command.
>
>> Afterwards, the event actually may happen, and invoke IRQ handler.
>
> Not if you masked IRQs after wait_for_completion_timeout() returned.


        wait_for_completion_timeout(&denali->complete, msecs_to_jiffies(1000));
                <<< WHAT IF IRQ EVENT HAPPENS HERE ? >>>
        iowrite32(0, denali->flash_reg + INTR_EN(denali->flash_bank));




Also, you ignore the return value of wait_for_completion_timeout(),
then drop my precious error message()

  dev_err(denali->dev, "timeout while waiting for irq 0x%x\n",
          denali->irq_mask)



> Here is a patch to show you what I had in mind [1] (it applies on top
> of this patch). AFAICT, there's no races, no interrupt loss, and you
> get rid of the ->irq_mask/status/lock fields.
>
> [1]http://code.bulix.org/fufia6-145571
>


Problem Scenario A
 [1] wait_for_completion_timeout() exits with timeout.
 [2] IRQ happens and denali_isr() is invoked
 [3] iowrite32(0, denali->flash_reg + INTR_EN(denali->flash_bank));
 [4] status = ioread32(denali->flash_reg + INTR_STATUS(bank)) &
              ioread32(denali->flash_reg + INTR_EN(bank));
      (status is set to 0 because INTR_EN(bank) is now 0)
 [5] return IRQ_NONE;
 [6] kernel complains  "irq *: nobody cared"



Problem Scenario B  (unlikely to happen, though)
 [1] wait_for_completion_timeout() exits with timeout.
 [2] IRQ happens and denali_isr() is invoked
 [3] iowrite32(0, denali->flash_reg + INTR_EN(denali->flash_bank));
 [4] chip->select_chip(mtd, -1)
 [5] denali->flash_bank = -1
 [6] status = ioread32(denali->flash_reg + INTR_STATUS(bank)) &
              ioread32(denali->flash_reg + INTR_EN(bank));
       ( access to non-existing INTR_STATUS(-1) )





-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1661524

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2017-06-08 19:40 +0200
Message-ID<tQ9zA-7f5-27@gated-at.bofh.it>
In reply to#1661509
2017-06-09 2:26 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:

> ->dev_ready() is optional, but we may end up with waiting more than needed.
>
>         case NAND_CMD_RESET:
>                 if (chip->dev_ready)
>                          break;
>                 udelay(chip->chip_delay);
>
>
> chip->chip_delay is probably set large enough, so this is not optimal.

I misunderstood the code.

The following line will be the most of the part of delay.

     nand_wait_status_ready(mtd, 250);


-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [next] | [standalone]

#1662059

FromBoris Brezillon <boris.brezillon@free-electrons.com>
Date2017-06-09 10:00 +0200
Message-ID<tQmZP-7c4-9@gated-at.bofh.it>
In reply to#1661509
Hi Masahiro,

On Fri, 9 Jun 2017 02:26:34 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> Hi Boris
> 
> 2017-06-09 0:43 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Thu, 8 Jun 2017 21:58:00 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >  
> >> Hi Boris,
> >>
> >> 2017-06-08 20:26 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:  
> >> > 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.  
> >>
> >> Checking the R/B pin is probably impossible unless
> >> the pin is changed into a GPIO port.
> >>
> >> I also considered NAND_CMD_STATUS, but
> >> I can not recall why I chose the current approach.
> >> Perhaps I thought returning detected IRQ
> >> is faster than accessing the chip for NAND_CMD_STATUS.
> >>
> >> I can try NAND_CMD_STATUS approach if you like.  
> >
> > Depends what you're trying to do. IIUC, you use denali_wait_for_irq()
> > inside your ->reset()/->read/write_{page,oob}[_raw]() methods, which is
> > perfectly fine (assuming CUSTOM_PAGE_ACCESS is set) since these hooks
> > are expected to wait for chip readiness before returning.
> >
> > You could also implement ->waitfunc() using denali_wait_for_irq() if
> > you're able to detect R/B transitions,  
> 
> R/B transition will set INTR__INT_ACT interrupt.
> 
> I think it is easy in my implementation of denali_wait_for_irq(),
> like
> 
>    denali_wait_for_irq(denali, INTR__INT_ACT);
> 
> 
> 
> But, you are suggesting me to change it.

This is clearly not a hard requirement, I was just curious and wanted
to understand why you had such a convoluted interrupt handling design. I
think I now understand why (see below).

> In your way, you give IRQ masks to denali_reset_irq(), like
> denali_reset_irq(denali, INTR__ERASE_COMP | INTR__ERASE_FAIL);
> 
> Then, we have no room of IRQ bit in denali_wait_for_irq().
> 
> How will you implement it?

It should be pretty easy: just make sure you reset the INTR__INT_ACT
status flag before sending a command (->cmd_ctrl()), and then unmask the
INTR__INT_ACT in denali_waitfunc() just before calling
denali_wait_for_irqs(). This should guarantee that you don't loose any
events, while keeping the logic rather simple.

> 
> 
> > but I'm not sure it's worth it,
> > because you overload almost all the methods using this hook (the only
> > one remaining is ->onfi_set_features(), and using STATUS polling should
> > not be an issue in this case).
> >
> > Implementing ->dev_ready() is not necessary. When not provided, the
> > core falls back to STATUS polling and you seem to support
> > NAND_CMD_STATUS in denali_cmdfunc(). Note that even if it's not fully
> > reliable in the current driver, you're switching to ->cmd_ctrl() at the
> > end of the series anyway, so we should be good after that.  
> 
> ->dev_ready() is optional, but we may end up with waiting more than needed.  
> 
>         case NAND_CMD_RESET:
>                 if (chip->dev_ready)
>                          break;
>                 udelay(chip->chip_delay);
> 
> 
> chip->chip_delay is probably set large enough, so this is not optimal.

That's true, this udelay should not be needed in your case.
 
> 
> 
> If I add something more, the following two bugs were found by
> denali_dev_ready().
> 
> commit 3158fa0e739615769cc047d2428f30f4c3b6640e
> commit c5d664aa5a4c4b257a54eb35045031630d105f49
> 
> 
> If NAND core is fine, denali_dev_ready() works fine too.
> 
> If not, it is a sign of bug of nand_command(_lp).
> This is contributing to the core improvement.
> 

Had a second look at denali_dev_ready() and it seems to do the right
thing, so let's keep it like that.
 
> 
> >>
> >> IIRC, I was thinking like this:
> >>
> >> One IRQ line may be shared among multiple hardware including Denali.
> >> denali_pci may do this.
> >>
> >> The Denali IRQ handler need to check irq status
> >> because it should return IRQ_HANDLED if the event comes from Denali controller.
> >> Otherwise, the event comes from different hardware, so
> >> Denali IRQ handler should return IRQ_NONE.  
> >
> > Correct.
> >  
> >>
> >> wait_for_completion_timeout() may bail out with timeout error,
> >> then proceed to denali_reset_irq() for the next operation.  
> >
> > Before calling denali_reset_irq() you should re-mask the irqs you
> > unmasked in #1. Actually, calling denali_reset_irq() after
> > wait_for_completion_timeout() is not even needed here because you'll
> > clear pending irqs before launching the next NAND command.
> >  
> >> Afterwards, the event actually may happen, and invoke IRQ handler.  
> >
> > Not if you masked IRQs after wait_for_completion_timeout() returned.  
> 
> 
>         wait_for_completion_timeout(&denali->complete, msecs_to_jiffies(1000));
>                 <<< WHAT IF IRQ EVENT HAPPENS HERE ? >>>
>         iowrite32(0, denali->flash_reg + INTR_EN(denali->flash_bank));

You're right, the write to INTR_EN() should be protected by a
spin_lock_irqsave to prevent concurrency between the irq handler and
the thread executing this function (and we should also take the lock
from the irq handler when doing status & mask). I didn't consider the
SMP case when coding this approach (one CPU can handle the interrupt
while the other one continues executing this function after the
timeout).

> 
> 
> 
> 
> Also, you ignore the return value of wait_for_completion_timeout(),
> then drop my precious error message()
> 
>   dev_err(denali->dev, "timeout while waiting for irq 0x%x\n",
>           denali->irq_mask)

Timeout can be detected by testing the status: if none of the flags we
were waiting for are set this is a timeout. Maybe I forgot to add this
message back though.

> 
> 
> 
> > Here is a patch to show you what I had in mind [1] (it applies on top
> > of this patch). AFAICT, there's no races, no interrupt loss, and you
> > get rid of the ->irq_mask/status/lock fields.
> >
> > [1]http://code.bulix.org/fufia6-145571
> >  
> 
> 
> Problem Scenario A
>  [1] wait_for_completion_timeout() exits with timeout.
>  [2] IRQ happens and denali_isr() is invoked
>  [3] iowrite32(0, denali->flash_reg + INTR_EN(denali->flash_bank));
>  [4] status = ioread32(denali->flash_reg + INTR_STATUS(bank)) &
>               ioread32(denali->flash_reg + INTR_EN(bank));
>       (status is set to 0 because INTR_EN(bank) is now 0)
>  [5] return IRQ_NONE;
>  [6] kernel complains  "irq *: nobody cared"

Okay, this is the part I initially misunderstood. Your goal is to never
ever return IRQ_NONE, while I was accepting to rarely return IRQ_NONE
in the unlikely interrupt-just-after-timeout case. Note that the kernel
irq infrastructure accepts rare occurrences or IRQ_NONE [1].

> 
> 
> 
> Problem Scenario B  (unlikely to happen, though)
>  [1] wait_for_completion_timeout() exits with timeout.
>  [2] IRQ happens and denali_isr() is invoked
>  [3] iowrite32(0, denali->flash_reg + INTR_EN(denali->flash_bank));
>  [4] chip->select_chip(mtd, -1)
>  [5] denali->flash_bank = -1
>  [6] status = ioread32(denali->flash_reg + INTR_STATUS(bank)) &
>               ioread32(denali->flash_reg + INTR_EN(bank));
>        ( access to non-existing INTR_STATUS(-1) )

Wrapping the write INTR_EN() into a
spin_lock_irqsave/unlock_irqrestore() section and doing the same in the
interrupt handler (without irqsave/restore) should solve the problem.

This being said, I'm not asking you to change the code, I just wanted
to understand why you were doing it like that.

Thanks,

Boris

[1]http://elixir.free-electrons.com/linux/latest/source/kernel/irq/spurious.c#L407

[toc] | [prev] | [next] | [standalone]

#1664413

FromMasahiro Yamada <yamada.masahiro@socionext.com>
Date2017-06-13 06:50 +0200
Message-ID<tRLW9-3nB-3@gated-at.bofh.it>
In reply to#1662059
Hi Boris,


2017-06-09 16:58 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> Hi Masahiro,
>
> On Fri, 9 Jun 2017 02:26:34 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> Hi Boris
>>
>> 2017-06-09 0:43 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> > On Thu, 8 Jun 2017 21:58:00 +0900
>> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >
>> >> Hi Boris,
>> >>
>> >> 2017-06-08 20:26 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> >> > 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.
>> >>
>> >> Checking the R/B pin is probably impossible unless
>> >> the pin is changed into a GPIO port.
>> >>
>> >> I also considered NAND_CMD_STATUS, but
>> >> I can not recall why I chose the current approach.
>> >> Perhaps I thought returning detected IRQ
>> >> is faster than accessing the chip for NAND_CMD_STATUS.
>> >>
>> >> I can try NAND_CMD_STATUS approach if you like.
>> >
>> > Depends what you're trying to do. IIUC, you use denali_wait_for_irq()
>> > inside your ->reset()/->read/write_{page,oob}[_raw]() methods, which is
>> > perfectly fine (assuming CUSTOM_PAGE_ACCESS is set) since these hooks
>> > are expected to wait for chip readiness before returning.
>> >
>> > You could also implement ->waitfunc() using denali_wait_for_irq() if
>> > you're able to detect R/B transitions,
>>
>> R/B transition will set INTR__INT_ACT interrupt.
>>
>> I think it is easy in my implementation of denali_wait_for_irq(),
>> like
>>
>>    denali_wait_for_irq(denali, INTR__INT_ACT);
>>
>>
>>
>> But, you are suggesting me to change it.
>
> This is clearly not a hard requirement, I was just curious and wanted
> to understand why you had such a convoluted interrupt handling design. I
> think I now understand why (see below).
>
>> In your way, you give IRQ masks to denali_reset_irq(), like
>> denali_reset_irq(denali, INTR__ERASE_COMP | INTR__ERASE_FAIL);
>>
>> Then, we have no room of IRQ bit in denali_wait_for_irq().
>>
>> How will you implement it?
>
> It should be pretty easy: just make sure you reset the INTR__INT_ACT
> status flag before sending a command (->cmd_ctrl()), and then unmask the
> INTR__INT_ACT in denali_waitfunc() just before calling
> denali_wait_for_irqs(). This should guarantee that you don't loose any
> events, while keeping the logic rather simple.

Right.  This way will be possible.

One compromise I see is that
it sets INTR__INT_ACT (= wait for R/B# IRQ event) for all commands.
Some commands actually trigger R/B# transition, but some do not.

We can make it precise like nand_command_lp(),
but I do not want to write such a switch statement in my driver.
(this must be maintained for possible new command addition in the future)


Anyway, I will send v6 in my current approach.




>>
>>
>>
>> > Here is a patch to show you what I had in mind [1] (it applies on top
>> > of this patch). AFAICT, there's no races, no interrupt loss, and you
>> > get rid of the ->irq_mask/status/lock fields.
>> >
>> > [1]http://code.bulix.org/fufia6-145571
>> >
>>
>>
>> Problem Scenario A
>>  [1] wait_for_completion_timeout() exits with timeout.
>>  [2] IRQ happens and denali_isr() is invoked
>>  [3] iowrite32(0, denali->flash_reg + INTR_EN(denali->flash_bank));
>>  [4] status = ioread32(denali->flash_reg + INTR_STATUS(bank)) &
>>               ioread32(denali->flash_reg + INTR_EN(bank));
>>       (status is set to 0 because INTR_EN(bank) is now 0)
>>  [5] return IRQ_NONE;
>>  [6] kernel complains  "irq *: nobody cared"
>
> Okay, this is the part I initially misunderstood. Your goal is to never
> ever return IRQ_NONE, while I was accepting to rarely return IRQ_NONE
> in the unlikely interrupt-just-after-timeout case. Note that the kernel
> irq infrastructure accepts rare occurrences or IRQ_NONE [1].

I wanted to be strict here.

But, I did not know the kernel is tolerant with rare IRQ_NONE.
Thanks for the pointer!




-- 
Best Regards
Masahiro Yamada

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web