| Message ID | 1496836352-8016-11-git-send-email-yamada.masahiro@socionext.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mtd: nand: denali: Denali NAND IP patch bomb | expand |
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. You should also clear existing interrupts before launching your operation, otherwise you might wakeup on previous events. > + spin_unlock_irqrestore(&denali->irq_lock, flags); > + > + time_left = wait_for_completion_timeout(&denali->complete, > + msecs_to_jiffies(1000)); > + if (!time_left) { > + dev_err(denali->dev, "timeout while waiting for irq 0x%x\n", > + denali->irq_mask); > + return 0; > + } > + > + return denali->irq_status; > } >
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
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
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().
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
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
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
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
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
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
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c index 8ad1e96f6d03..62798e6d7009 100644 --- a/drivers/mtd/nand/denali.c +++ b/drivers/mtd/nand/denali.c @@ -31,21 +31,6 @@ MODULE_LICENSE("GPL"); #define DENALI_NAND_NAME "denali-nand" /* - * We define a macro here that combines all interrupts this driver uses into - * a single constant value, for convenience. - */ -#define DENALI_IRQ_ALL (INTR__DMA_CMD_COMP | \ - INTR__ECC_TRANSACTION_DONE | \ - INTR__ECC_ERR | \ - INTR__PROGRAM_FAIL | \ - INTR__LOAD_COMP | \ - INTR__PROGRAM_COMP | \ - INTR__TIME_OUT | \ - INTR__ERASE_FAIL | \ - INTR__RST_COMP | \ - INTR__ERASE_COMP) - -/* * indicates whether or not the internal value for the flash bank is * valid or not */ @@ -71,20 +56,14 @@ static inline struct denali_nand_info *mtd_to_denali(struct mtd_info *mtd) #define DENALI_READ 0 #define DENALI_WRITE 0x100 +#define DENALI_NR_BANKS 4 + /* * this is a helper macro that allows us to * format the bank into the proper bits for the controller */ #define BANK(x) ((x) << 24) -/* forward declarations */ -static void clear_interrupts(struct denali_nand_info *denali); -static uint32_t wait_for_irq(struct denali_nand_info *denali, - uint32_t irq_mask); -static void denali_irq_enable(struct denali_nand_info *denali, - uint32_t int_mask); -static uint32_t read_interrupt_status(struct denali_nand_info *denali); - /* * The bus interface clock, clk_x, is phase aligned with the core clock. The * clk_x is an integral multiple N of the core clk. The value N is configured @@ -143,22 +122,6 @@ static void read_status(struct denali_nand_info *denali) write_byte_to_buf(denali, 0); } -/* resets a specific device connected to the core */ -static void reset_bank(struct denali_nand_info *denali) -{ - uint32_t irq_status; - uint32_t irq_mask = INTR__RST_COMP | INTR__TIME_OUT; - - clear_interrupts(denali); - - iowrite32(1 << denali->flash_bank, denali->flash_reg + DEVICE_RESET); - - irq_status = wait_for_irq(denali, irq_mask); - - if (irq_status & INTR__TIME_OUT) - dev_err(denali->dev, "reset bank failed.\n"); -} - /* Reset the flash controller */ static uint16_t denali_nand_reset(struct denali_nand_info *denali) { @@ -201,169 +164,123 @@ static void detect_max_banks(struct denali_nand_info *denali) denali->max_banks <<= 1; } -static void denali_set_intr_modes(struct denali_nand_info *denali, - uint16_t INT_ENABLE) +static void denali_enable_irq(struct denali_nand_info *denali) { - if (INT_ENABLE) - iowrite32(1, denali->flash_reg + GLOBAL_INT_ENABLE); - else - iowrite32(0, denali->flash_reg + GLOBAL_INT_ENABLE); -} + int i; -/* - * validation function to verify that the controlling software is making - * a valid request - */ -static inline bool is_flash_bank_valid(int flash_bank) -{ - return flash_bank >= 0 && flash_bank < 4; + for (i = 0; i < DENALI_NR_BANKS; i++) + iowrite32(U32_MAX, denali->flash_reg + INTR_EN(i)); + iowrite32(GLOBAL_INT_EN_FLAG, denali->flash_reg + GLOBAL_INT_ENABLE); } -static void denali_irq_init(struct denali_nand_info *denali) +static void denali_disable_irq(struct denali_nand_info *denali) { - uint32_t int_mask; int i; - /* Disable global interrupts */ - denali_set_intr_modes(denali, false); - - int_mask = DENALI_IRQ_ALL; - - /* Clear all status bits */ - for (i = 0; i < denali->max_banks; ++i) - iowrite32(0xFFFF, denali->flash_reg + INTR_STATUS(i)); - - denali_irq_enable(denali, int_mask); + for (i = 0; i < DENALI_NR_BANKS; i++) + iowrite32(0, denali->flash_reg + INTR_EN(i)); + iowrite32(0, denali->flash_reg + GLOBAL_INT_ENABLE); } -static void denali_irq_cleanup(int irqnum, struct denali_nand_info *denali) +static void denali_clear_irq(struct denali_nand_info *denali, + int bank, uint32_t irq_status) { - denali_set_intr_modes(denali, false); + /* write one to clear bits */ + iowrite32(irq_status, denali->flash_reg + INTR_STATUS(bank)); } -static void denali_irq_enable(struct denali_nand_info *denali, - uint32_t int_mask) +static void denali_clear_irq_all(struct denali_nand_info *denali) { int i; - for (i = 0; i < denali->max_banks; ++i) - iowrite32(int_mask, denali->flash_reg + INTR_EN(i)); + for (i = 0; i < DENALI_NR_BANKS; i++) + denali_clear_irq(denali, i, U32_MAX); } -/* - * This function only returns when an interrupt that this driver cares about - * occurs. This is to reduce the overhead of servicing interrupts - */ -static inline uint32_t denali_irq_detected(struct denali_nand_info *denali) +static irqreturn_t denali_isr(int irq, void *dev_id) { - return read_interrupt_status(denali) & DENALI_IRQ_ALL; -} + struct denali_nand_info *denali = dev_id; + irqreturn_t ret = IRQ_NONE; + uint32_t irq_status; + int i; -/* Interrupts are cleared by writing a 1 to the appropriate status bit */ -static inline void clear_interrupt(struct denali_nand_info *denali, - uint32_t irq_mask) -{ - uint32_t intr_status_reg; + spin_lock(&denali->irq_lock); - intr_status_reg = INTR_STATUS(denali->flash_bank); + for (i = 0; i < DENALI_NR_BANKS; i++) { + irq_status = ioread32(denali->flash_reg + INTR_STATUS(i)); + if (irq_status) + ret = IRQ_HANDLED; - iowrite32(irq_mask, denali->flash_reg + intr_status_reg); -} + denali_clear_irq(denali, i, irq_status); -static void clear_interrupts(struct denali_nand_info *denali) -{ - uint32_t status; + if (i != denali->flash_bank) + continue; + + denali->irq_status |= irq_status; - spin_lock_irq(&denali->irq_lock); + if (denali->irq_status & denali->irq_mask) + complete(&denali->complete); + } - status = read_interrupt_status(denali); - clear_interrupt(denali, status); + spin_unlock(&denali->irq_lock); - denali->irq_status = 0x0; - spin_unlock_irq(&denali->irq_lock); + return ret; } -static uint32_t read_interrupt_status(struct denali_nand_info *denali) +static void denali_reset_irq(struct denali_nand_info *denali) { - uint32_t intr_status_reg; - - intr_status_reg = INTR_STATUS(denali->flash_bank); + unsigned long flags; - return ioread32(denali->flash_reg + intr_status_reg); + spin_lock_irqsave(&denali->irq_lock, flags); + denali->irq_status = 0; + denali->irq_mask = 0; + spin_unlock_irqrestore(&denali->irq_lock, flags); } -/* - * 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); + spin_unlock_irqrestore(&denali->irq_lock, flags); + + time_left = wait_for_completion_timeout(&denali->complete, + msecs_to_jiffies(1000)); + if (!time_left) { + dev_err(denali->dev, "timeout while waiting for irq 0x%x\n", + denali->irq_mask); + return 0; + } + + return denali->irq_status; } -static uint32_t wait_for_irq(struct denali_nand_info *denali, uint32_t irq_mask) +/* resets a specific device connected to the core */ +static void reset_bank(struct denali_nand_info *denali) { - unsigned long comp_res; - uint32_t intr_status; - unsigned long timeout = msecs_to_jiffies(1000); + uint32_t irq_status; - do { - comp_res = - wait_for_completion_timeout(&denali->complete, timeout); - spin_lock_irq(&denali->irq_lock); - intr_status = denali->irq_status; - - if (intr_status & irq_mask) { - denali->irq_status &= ~irq_mask; - spin_unlock_irq(&denali->irq_lock); - /* our interrupt was detected */ - break; - } + denali_reset_irq(denali); - /* - * these are not the interrupts you are looking for - - * need to wait again - */ - spin_unlock_irq(&denali->irq_lock); - } while (comp_res != 0); + iowrite32(1 << denali->flash_bank, denali->flash_reg + DEVICE_RESET); - if (comp_res == 0) { - /* timeout */ - pr_err("timeout occurred, status = 0x%x, mask = 0x%x\n", - intr_status, irq_mask); + irq_status = denali_wait_for_irq(denali, + INTR__RST_COMP | INTR__TIME_OUT); - intr_status = 0; - } - return intr_status; + if (!(irq_status & INTR__RST_COMP)) + dev_err(denali->dev, "reset bank failed.\n"); } /* @@ -397,7 +314,7 @@ static int denali_send_pipeline_cmd(struct denali_nand_info *denali, setup_ecc_for_xfer(denali, ecc_en, transfer_spare); - clear_interrupts(denali); + denali_reset_irq(denali); addr = BANK(denali->flash_bank) | denali->page; @@ -479,9 +396,9 @@ static int write_oob_data(struct mtd_info *mtd, uint8_t *buf, int page) write_data_to_flash_mem(denali, buf, mtd->oobsize); /* wait for operation to complete */ - irq_status = wait_for_irq(denali, irq_mask); + irq_status = denali_wait_for_irq(denali, irq_mask); - if (irq_status == 0) { + if (!(irq_status & INTR__PROGRAM_COMP)) { dev_err(denali->dev, "OOB write failed\n"); status = -EIO; } @@ -510,9 +427,9 @@ static void read_oob_data(struct mtd_info *mtd, uint8_t *buf, int page) * can always use status0 bit as the * mask is identical for each bank. */ - irq_status = wait_for_irq(denali, irq_mask); + irq_status = denali_wait_for_irq(denali, irq_mask); - if (irq_status == 0) + if (!(irq_status & INTR__LOAD_COMP)) dev_err(denali->dev, "page on OOB timeout %d\n", denali->page); @@ -620,9 +537,9 @@ static int denali_sw_ecc_fixup(struct mtd_info *mtd, unsigned int err_byte, err_sector, err_device; uint8_t err_cor_value; unsigned int prev_sector = 0; + uint32_t irq_status; - /* read the ECC errors. we'll ignore them for now */ - denali_set_intr_modes(denali, false); + denali_reset_irq(denali); do { err_addr = ioread32(denali->flash_reg + ECC_ERROR_ADDRESS); @@ -674,10 +591,9 @@ static int denali_sw_ecc_fixup(struct mtd_info *mtd, * ECC_TRANSACTION_DONE interrupt, so here just wait for * a while for this interrupt */ - while (!(read_interrupt_status(denali) & INTR__ECC_TRANSACTION_DONE)) - cpu_relax(); - clear_interrupts(denali); - denali_set_intr_modes(denali, true); + irq_status = denali_wait_for_irq(denali, INTR__ECC_TRANSACTION_DONE); + if (!(irq_status & INTR__ECC_TRANSACTION_DONE)) + return -EIO; return max_bitflips; } @@ -778,15 +694,14 @@ static int write_page(struct mtd_info *mtd, struct nand_chip *chip, dma_sync_single_for_device(denali->dev, addr, size, DMA_TO_DEVICE); - clear_interrupts(denali); + denali_reset_irq(denali); denali_enable_dma(denali, true); denali_setup_dma(denali, DENALI_WRITE); /* wait for operation to complete */ - irq_status = wait_for_irq(denali, irq_mask); - - if (irq_status == 0) { + irq_status = denali_wait_for_irq(denali, irq_mask); + if (!(irq_status & INTR__DMA_CMD_COMP)) { dev_err(denali->dev, "timeout on write_page (type = %d)\n", raw_xfer); ret = -EIO; @@ -865,11 +780,11 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip, denali_enable_dma(denali, true); dma_sync_single_for_device(denali->dev, addr, size, DMA_FROM_DEVICE); - clear_interrupts(denali); + denali_reset_irq(denali); denali_setup_dma(denali, DENALI_READ); /* wait for operation to complete */ - irq_status = wait_for_irq(denali, irq_mask); + irq_status = denali_wait_for_irq(denali, irq_mask); dma_sync_single_for_cpu(denali->dev, addr, size, DMA_FROM_DEVICE); @@ -901,6 +816,7 @@ static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, dma_addr_t addr = denali->buf.dma_buf; size_t size = mtd->writesize + mtd->oobsize; uint32_t irq_mask = INTR__DMA_CMD_COMP; + uint32_t irq_status; denali->page = page; @@ -909,11 +825,13 @@ static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip, dma_sync_single_for_device(denali->dev, addr, size, DMA_FROM_DEVICE); - clear_interrupts(denali); + denali_reset_irq(denali); denali_setup_dma(denali, DENALI_READ); /* wait for operation to complete */ - wait_for_irq(denali, irq_mask); + irq_status = denali_wait_for_irq(denali, irq_mask); + if (irq_status & INTR__DMA_CMD_COMP) + return -ETIMEDOUT; dma_sync_single_for_cpu(denali->dev, addr, size, DMA_FROM_DEVICE); @@ -940,9 +858,7 @@ static void denali_select_chip(struct mtd_info *mtd, int chip) { struct denali_nand_info *denali = mtd_to_denali(mtd); - spin_lock_irq(&denali->irq_lock); denali->flash_bank = chip; - spin_unlock_irq(&denali->irq_lock); } static int denali_waitfunc(struct mtd_info *mtd, struct nand_chip *chip) @@ -953,19 +869,19 @@ static int denali_waitfunc(struct mtd_info *mtd, struct nand_chip *chip) static int denali_erase(struct mtd_info *mtd, int page) { struct denali_nand_info *denali = mtd_to_denali(mtd); - uint32_t cmd, irq_status; - clear_interrupts(denali); + denali_reset_irq(denali); /* setup page read request for access type */ cmd = MODE_10 | BANK(denali->flash_bank) | page; index_addr(denali, cmd, 0x1); /* wait for erase to complete or failure to occur */ - irq_status = wait_for_irq(denali, INTR__ERASE_COMP | INTR__ERASE_FAIL); + irq_status = denali_wait_for_irq(denali, + INTR__ERASE_COMP | INTR__ERASE_FAIL); - return irq_status & INTR__ERASE_FAIL ? NAND_STATUS_FAIL : PASS; + return irq_status & INTR__ERASE_COMP ? 0 : NAND_STATUS_FAIL; } static void denali_cmdfunc(struct mtd_info *mtd, unsigned int cmd, int col, @@ -1153,7 +1069,6 @@ static void denali_hw_init(struct denali_nand_info *denali) /* Should set value for these registers when init */ iowrite32(0, denali->flash_reg + TWO_ROW_ADDR_CYCLES); iowrite32(1, denali->flash_reg + ECC_ENABLE); - denali_irq_init(denali); } int denali_calc_ecc_bytes(int step_size, int strength) @@ -1265,9 +1180,6 @@ static void denali_drv_init(struct denali_nand_info *denali) /* indicate that MTD has not selected a valid bank yet */ denali->flash_bank = CHIP_SELECT_INVALID; - - /* initialize our irq_status variable to indicate no interrupts */ - denali->irq_status = 0; } static int denali_multidev_fixup(struct denali_nand_info *denali) @@ -1337,6 +1249,8 @@ int denali_init(struct denali_nand_info *denali) denali_hw_init(denali); denali_drv_init(denali); + denali_clear_irq_all(denali); + /* Request IRQ after all the hardware initialization is finished */ ret = devm_request_irq(denali->dev, denali->irq, denali_isr, IRQF_SHARED, DENALI_NAND_NAME, denali); @@ -1345,8 +1259,8 @@ int denali_init(struct denali_nand_info *denali) return ret; } - /* now that our ISR is registered, we can enable interrupts */ - denali_set_intr_modes(denali, true); + denali_enable_irq(denali); + nand_set_flash_node(chip, denali->dev->of_node); /* Fallback to the default name if DT did not give "label" property */ if (!mtd->name) @@ -1368,7 +1282,7 @@ int denali_init(struct denali_nand_info *denali) */ ret = nand_scan_ident(mtd, denali->max_banks, NULL); if (ret) - goto failed_req_irq; + goto disable_irq; /* allocate the right size buffer now */ devm_kfree(denali->dev, denali->buf.buf); @@ -1377,7 +1291,7 @@ int denali_init(struct denali_nand_info *denali) GFP_KERNEL); if (!denali->buf.buf) { ret = -ENOMEM; - goto failed_req_irq; + goto disable_irq; } ret = dma_set_mask(denali->dev, @@ -1385,7 +1299,7 @@ int denali_init(struct denali_nand_info *denali) 64 : 32)); if (ret) { dev_err(denali->dev, "No usable DMA configuration\n"); - goto failed_req_irq; + goto disable_irq; } denali->buf.dma_buf = dma_map_single(denali->dev, denali->buf.buf, @@ -1394,7 +1308,7 @@ int denali_init(struct denali_nand_info *denali) if (dma_mapping_error(denali->dev, denali->buf.dma_buf)) { dev_err(denali->dev, "Failed to map DMA buffer\n"); ret = -EIO; - goto failed_req_irq; + goto disable_irq; } /* @@ -1418,7 +1332,7 @@ int denali_init(struct denali_nand_info *denali) ret = denali_ecc_setup(mtd, chip, denali); if (ret) { dev_err(denali->dev, "Failed to setup ECC settings.\n"); - goto failed_req_irq; + goto disable_irq; } dev_dbg(denali->dev, @@ -1452,21 +1366,21 @@ int denali_init(struct denali_nand_info *denali) ret = denali_multidev_fixup(denali); if (ret) - goto failed_req_irq; + goto disable_irq; ret = nand_scan_tail(mtd); if (ret) - goto failed_req_irq; + goto disable_irq; ret = mtd_device_register(mtd, NULL, 0); if (ret) { dev_err(denali->dev, "Failed to register MTD: %d\n", ret); - goto failed_req_irq; + goto disable_irq; } return 0; -failed_req_irq: - denali_irq_cleanup(denali->irq, denali); +disable_irq: + denali_disable_irq(denali); return ret; } @@ -1484,7 +1398,7 @@ void denali_remove(struct denali_nand_info *denali) int bufsize = mtd->writesize + mtd->oobsize; nand_release(mtd); - denali_irq_cleanup(denali->irq, denali); + denali_disable_irq(denali); dma_unmap_single(denali->dev, denali->buf.dma_buf, bufsize, DMA_BIDIRECTIONAL); } diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h index fb473895a79d..a0ac0f84f8b5 100644 --- a/drivers/mtd/nand/denali.h +++ b/drivers/mtd/nand/denali.h @@ -325,6 +325,7 @@ struct denali_nand_info { /* elements used by ISR */ struct completion complete; spinlock_t irq_lock; + uint32_t irq_mask; uint32_t irq_status; int irq;
Simplify the interrupt handling and fix issues: - The register field view of INTR_EN / INTR_STATUS is different among IP versions. The global macro DENALI_IRQ_ALL is hard-coded for Intel platforms. The interrupt mask should be determined at run-time depending on the running platform. - wait_for_irq() loops do {} while() until interested flags are asserted. The logic can be simplified. - The spin_lock() guard seems too complex (and suspicious in a race condition if wait_for_completion_timeout() bails out by timeout). - denali->complete is reused again and again, but reinit_completion() is missing. Add it. Re-work the code to make it more robust and easier to handle. While we are here, also rename the jump label "failed_req_irq" to more appropriate "disable_irq". Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com> --- Changes in v5: None Changes in v4: None Changes in v3: None Changes in v2: - Newly added drivers/mtd/nand/denali.c | 316 +++++++++++++++++----------------------------- drivers/mtd/nand/denali.h | 1 + 2 files changed, 116 insertions(+), 201 deletions(-) -- 2.7.4