From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751512AbdFHM6K (ORCPT ); Thu, 8 Jun 2017 08:58:10 -0400 Received: from conssluserg-06.nifty.com ([210.131.2.91]:21105 "EHLO conssluserg-06.nifty.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750914AbdFHM6J (ORCPT ); Thu, 8 Jun 2017 08:58:09 -0400 DKIM-Filter: OpenDKIM Filter v2.10.3 conssluserg-06.nifty.com v58Cw1G2023398 X-Nifty-SrcIP: [209.85.213.177] MIME-Version: 1.0 In-Reply-To: <20170608132620.17fc7c96@bbrezillon> References: <1496836352-8016-1-git-send-email-yamada.masahiro@socionext.com> <1496836352-8016-11-git-send-email-yamada.masahiro@socionext.com> <20170607155701.4bc89ad8@bbrezillon> <20170608091239.0095511b@bbrezillon> <20170608132620.17fc7c96@bbrezillon> From: Masahiro Yamada Date: Thu, 8 Jun 2017 21:58:00 +0900 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling To: Boris Brezillon Cc: Richard Weinberger , Marek Vasut , Artem Bityutskiy , Chuanxiao Dong , Linux Kernel Mailing List , Dinh Nguyen , linux-mtd@lists.infradead.org, Masami Hiramatsu , Cyrille Pitchen , Jassi Brar , Brian Norris , Enrico Jorns , David Woodhouse Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from quoted-printable to 8bit by mail.home.local id v58CwDwV011491 Hi Boris, 2017-06-08 20:26 GMT+09:00 Boris Brezillon : > On Thu, 8 Jun 2017 19:41:39 +0900 > Masahiro Yamada wrote: > >> Hi Boris, >> >> >> 2017-06-08 16:12 GMT+09:00 Boris Brezillon : >> > Le Thu, 8 Jun 2017 15:10:18 +0900, >> > Masahiro Yamada a écrit : >> > >> >> Hi Boris, >> >> >> >> >> >> 2017-06-07 22:57 GMT+09:00 Boris Brezillon : >> >> > On Wed, 7 Jun 2017 20:52:19 +0900 >> >> > Masahiro Yamada 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