From patchwork Mon Jul 24 16:40:31 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Marc Gonzalez X-Patchwork-Id: 9859801 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id A6501601A1 for ; Mon, 24 Jul 2017 16:41:16 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 9734928573 for ; Mon, 24 Jul 2017 16:41:16 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 8BD4E28587; Mon, 24 Jul 2017 16:41:16 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-0.2 required=2.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID, RCVD_IN_DNSWL_NONE, URIBL_BLACK autolearn=no version=3.3.1 Received: from bombadil.infradead.org (bombadil.infradead.org [65.50.211.133]) (using TLSv1.2 with cipher AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 047A328573 for ; Mon, 24 Jul 2017 16:41:16 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:Date: Message-ID:From:References:To:Subject:Reply-To:Content-ID:Content-Description :Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=RPoaSRl4w6maazO4hv+J+eQhjO7JKFmHi1ype5yiSJI=; b=m9O22kcKqC/vKY zARrl/A7X5uvGVzBs4IaZ/aBFlAgm0pl2buUaBk2ToGNKrYS4DQKuFemPSaIVa/U7ZBASpaIN5whv 1bC3qDI8b+N1O/1fdrleaAp9eSY2O27PQ4iWn7g0I3G97okY1KrHvmnt+zPkSqR4/wsd/xnL5tQBO 6M2LBiEaDahZ4t6dEF/5O8f0e+huy+MoEVGacK54gESO0tQYY11Mf3cfifNP5vXcOuC/raUivNXVr zTVfxQLWGp/MD/Zh9tGvAdmnZ97wIbDOkk0a7/Od0k2sPHhXR3OEzCPEusNG3vBqXUWYwJ98FcKek R+M1ERFv5BhXc8vZPIsw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.87 #1 (Red Hat Linux)) id 1dZgPx-0002Xh-EG; Mon, 24 Jul 2017 16:41:13 +0000 Received: from us-smtp-delivery-107.mimecast.com ([216.205.24.107]) by bombadil.infradead.org with esmtps (Exim 4.87 #1 (Red Hat Linux)) id 1dZgPo-0002Ky-Te for linux-arm-kernel@lists.infradead.org; Mon, 24 Jul 2017 16:41:10 +0000 Received: from CPH-EX1.SDESIGNS.COM (195-215-56-170-static.dk.customer.tdc.net [195.215.56.170]) (Using TLS) by us-smtp-1.mimecast.com with ESMTP id us-mta-151-qj23MUA9Mx2c5HYHitRPjg-1; Mon, 24 Jul 2017 12:40:39 -0400 Received: from [172.27.0.114] (172.27.0.114) by CPH-EX1.sdesigns.com (192.168.10.36) with Microsoft SMTP Server (TLS) id 14.3.294.0; Mon, 24 Jul 2017 18:40:35 +0200 Subject: Re: [PATCH v2 2/6] irqchip/tango: Use irq_gc_mask_disable_and_ack_set To: Doug Berger , Thomas Gleixner , Florian Fainelli , Marc Zyngier References: <20170719190734.18566-1-opendmb@gmail.com> <20170719190734.18566-3-opendmb@gmail.com> From: Marc Gonzalez Message-ID: <7a51555f-8191-9ebd-1f30-7c20f6db9d3f@sigmadesigns.com> Date: Mon, 24 Jul 2017 18:40:31 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Firefox/52.0 SeaMonkey/2.49.1 MIME-Version: 1.0 In-Reply-To: <20170719190734.18566-3-opendmb@gmail.com> X-Originating-IP: [172.27.0.114] X-MC-Unique: qj23MUA9Mx2c5HYHitRPjg-1 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20170724_094105_197092_3B8E0C00 X-CRM114-Status: GOOD ( 14.43 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Boris Brezillon , Jason Cooper , Mason , Bartosz Golaszewski , Kevin Cernekee , LKML , Gregory Fong , Brian Norris , Linux ARM Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org X-Virus-Scanned: ClamAV using ClamSMTP [ Trimming CC list ] On 19/07/2017 21:07, Doug Berger wrote: > From: Florian Fainelli > > The only usage of the irq_gc_mask_disable_reg_and_ack() function > is by the Tango irqchip driver. This usage is replaced by the > irq_gc_mask_disable_and_ack_set() function since it provides the > intended functionality. > > Fixes: 4bba66899ac6 ("irqchip/tango: Add support for Sigma Designs SMP86xx/SMP87xx interrupt controller") > Signed-off-by: Florian Fainelli > Acked-by: Mans Rullgard > Signed-off-by: Doug Berger > --- > drivers/irqchip/irq-tango.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c > index bdbb5c0ff7fe..0c085303a583 100644 > --- a/drivers/irqchip/irq-tango.c > +++ b/drivers/irqchip/irq-tango.c > @@ -141,7 +141,7 @@ static void __init tangox_irq_init_chip(struct irq_chip_generic *gc, > for (i = 0; i < 2; i++) { > ct[i].chip.irq_ack = irq_gc_ack_set_bit; > ct[i].chip.irq_mask = irq_gc_mask_disable_reg; > - ct[i].chip.irq_mask_ack = irq_gc_mask_disable_reg_and_ack; > + ct[i].chip.irq_mask_ack = irq_gc_mask_disable_and_ack_set; > ct[i].chip.irq_unmask = irq_gc_unmask_enable_reg; > ct[i].chip.irq_set_type = tangox_irq_set_type; > ct[i].chip.name = gc->domain->name; > (I had a look only at patches 1 and 2) irq_gc_mask_disable_reg() = irq_reg_writel(gc, mask, ct->regs.disable); *ct->mask_cache &= ~mask; irq_gc_ack_set_bit() = irq_reg_writel(gc, mask, ct->regs.ack); irq_gc_mask_disable_reg_and_ack() = irq_reg_writel(gc, mask, ct->regs.mask); // regs.mask not defined for tango driver irq_reg_writel(gc, mask, ct->regs.ack); It will try to write at offset 0, which is a read-only register so no disaster, but interrupt is not disabled. I wonder why everything doesn't blow up... irq_gc_mask_disable_and_ack_set() = irq_reg_writel(gc, mask, ct->regs.disable); *ct->mask_cache &= ~mask; irq_reg_writel(gc, mask, ct->regs.ack); Your change makes sense. But note that mask_ack_irq() is defined as: static inline void mask_ack_irq(struct irq_desc *desc) { if (desc->irq_data.chip->irq_mask_ack) { desc->irq_data.chip->irq_mask_ack(&desc->irq_data); irq_state_set_masked(desc); } else { mask_irq(desc); if (desc->irq_data.chip->irq_ack) desc->irq_data.chip->irq_ack(&desc->irq_data); } } So IIUC, if we don't define a irq_mask_ack() callback, it will just call irq_mask() and irq_ack() which will do the right thing. So an alternative is simply to delete the assignment of ct[i].chip.irq_mask_ack. What do you think? One thing I'm not sure about is why we need to ack for level interrupts... We should be able to get away with defining ack() only for edge interrupts... I'm confused. If I try to do that, I get: [ 1.430547] irq 20: nobody cared (try booting with the "irqpoll" option) [ 1.437283] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.13.0-rc1 #148 [ 1.443754] Hardware name: Sigma Tango DT [ 1.447812] [] (unwind_backtrace) from [] (show_stack+0x10/0x14) [ 1.455598] [] (show_stack) from [] (dump_stack+0x84/0x98) [ 1.462864] [] (dump_stack) from [] (__report_bad_irq+0x28/0xcc) [ 1.470650] [] (__report_bad_irq) from [] (note_interrupt+0x28c/0x2dc) [ 1.478959] [] (note_interrupt) from [] (handle_irq_event_percpu+0x4c/0x58) [ 1.487702] [] (handle_irq_event_percpu) from [] (handle_irq_event+0x38/0x5c) [ 1.496621] [] (handle_irq_event) from [] (handle_level_irq+0xa8/0x124) [ 1.505017] [] (handle_level_irq) from [] (generic_handle_irq+0x24/0x34) [ 1.513501] [] (generic_handle_irq) from [] (tangox_dispatch_irqs+0x4c/0x58) [ 1.522333] [] (tangox_dispatch_irqs) from [] (tangox_irq_handler+0x4c/0xb0) [ 1.531163] [] (tangox_irq_handler) from [] (generic_handle_irq+0x24/0x34) [ 1.539819] [] (generic_handle_irq) from [] (__handle_domain_irq+0x5c/0xb4) [ 1.548562] [] (__handle_domain_irq) from [] (gic_handle_irq+0x48/0x8c) [ 1.556956] [] (gic_handle_irq) from [] (__irq_svc+0x6c/0xa8) Regards. diff --git a/drivers/irqchip/irq-tango.c b/drivers/irqchip/irq-tango.c index bdbb5c0ff7fe..825085cdab99 100644 --- a/drivers/irqchip/irq-tango.c +++ b/drivers/irqchip/irq-tango.c @@ -141,7 +141,6 @@ static void __init tangox_irq_init_chip(struct irq_chip_generic *gc, for (i = 0; i < 2; i++) { ct[i].chip.irq_ack = irq_gc_ack_set_bit; ct[i].chip.irq_mask = irq_gc_mask_disable_reg; - ct[i].chip.irq_mask_ack = irq_gc_mask_disable_reg_and_ack; ct[i].chip.irq_unmask = irq_gc_unmask_enable_reg; ct[i].chip.irq_set_type = tangox_irq_set_type; ct[i].chip.name = gc->domain->name;