From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F1B6CC4332F for ; Mon, 4 Oct 2021 15:31:58 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DAB626139E for ; Mon, 4 Oct 2021 15:31:58 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235490AbhJDPdq convert rfc822-to-8bit (ORCPT ); Mon, 4 Oct 2021 11:33:46 -0400 Received: from mail.kernel.org ([198.145.29.99]:54072 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S234917AbhJDPdq (ORCPT ); Mon, 4 Oct 2021 11:33:46 -0400 Received: from disco-boy.misterjones.org (disco-boy.misterjones.org [51.254.78.96]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 87C2461244; Mon, 4 Oct 2021 15:31:57 +0000 (UTC) Received: from sofa.misterjones.org ([185.219.108.64] helo=why.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1mXPwZ-00Efau-BR; Mon, 04 Oct 2021 16:31:55 +0100 Date: Mon, 04 Oct 2021 16:31:54 +0100 Message-ID: <871r50st5h.wl-maz@kernel.org> From: Marc Zyngier To: Lorenzo Pieralisi Cc: Marek =?UTF-8?B?QmVow7pu?= , Thomas Petazzoni , Bjorn Helgaas , linux-pci@vger.kernel.org, pali@kernel.org, stable@vger.kernel.org Subject: Re: [PATCH 06/13] PCI: aardvark: Do not clear status bits of masked interrupts In-Reply-To: <20211004140653.GB24914@lpieralisi> References: <20211001195856.10081-1-kabel@kernel.org> <20211001195856.10081-7-kabel@kernel.org> <20211004140653.GB24914@lpieralisi> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: lorenzo.pieralisi@arm.com, kabel@kernel.org, thomas.petazzoni@bootlin.com, bhelgaas@google.com, linux-pci@vger.kernel.org, pali@kernel.org, stable@vger.kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false Precedence: bulk List-ID: X-Mailing-List: linux-pci@vger.kernel.org On Mon, 04 Oct 2021 15:06:53 +0100, Lorenzo Pieralisi wrote: > > [+Marc - always better to have his eyes on IRQ handling code] > > On Fri, Oct 01, 2021 at 09:58:49PM +0200, Marek Behún wrote: > > From: Pali Rohár > > > > It is incorrect to clear status bits of masked interrupts. > > > > The aardvark driver clears all status interrupt bits if no unmasked > > status bit is set. Masked bits should never be cleared. > > > > Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver") > > Signed-off-by: Pali Rohár > > Reviewed-by: Marek Behún > > Signed-off-by: Marek Behún > > Cc: stable@vger.kernel.org > > --- > > drivers/pci/controller/pci-aardvark.c | 5 +---- > > 1 file changed, 1 insertion(+), 4 deletions(-) > > > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c > > index d5d6f92e5143..e4986806a189 100644 > > --- a/drivers/pci/controller/pci-aardvark.c > > +++ b/drivers/pci/controller/pci-aardvark.c > > @@ -1295,11 +1295,8 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie) > > isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG); > > isr1_status = isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK); > > > > - if (!isr0_status && !isr1_status) { > > - advk_writel(pcie, isr0_val, PCIE_ISR0_REG); > > - advk_writel(pcie, isr1_val, PCIE_ISR1_REG); > > This looks fine - on the other hand if no interrupt is set in the status > registers (that are filtered with the masks) we are dealing with a > spurious IRQ right ? Just gauging how severe this is. > > Lorenzo > > > + if (!isr0_status && !isr1_status) > > return; The whole thing is a bit odd. What the commit message doesn't say is whether the status register shows the status of the line before masking, or after masking. The code seems to imply the former, but then the behaviour is awkward. How did we end-up here the first place? if that's only a spurious interrupt, then I'd probably simplify the code altogether, and drop all the early return code. Something like below, as usual completely untested. M. diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c index 596ebcfcc82d..1d8f257ecb63 100644 --- a/drivers/pci/controller/pci-aardvark.c +++ b/drivers/pci/controller/pci-aardvark.c @@ -1275,7 +1275,8 @@ static void advk_pcie_handle_msi(struct advk_pcie *pcie) static void advk_pcie_handle_int(struct advk_pcie *pcie) { u32 isr0_val, isr0_mask, isr0_status; - u32 isr1_val, isr1_mask, isr1_status; + u32 isr1_val, isr1_mask; + unsigned long isr1_status; int i; isr0_val = advk_readl(pcie, PCIE_ISR0_REG); @@ -1285,22 +1286,14 @@ static void advk_pcie_handle_int(struct advk_pcie *pcie) isr1_val = advk_readl(pcie, PCIE_ISR1_REG); isr1_mask = advk_readl(pcie, PCIE_ISR1_MASK_REG); isr1_status = isr1_val & ((~isr1_mask) & PCIE_ISR1_ALL_MASK); - - if (!isr0_status && !isr1_status) { - advk_writel(pcie, isr0_val, PCIE_ISR0_REG); - advk_writel(pcie, isr1_val, PCIE_ISR1_REG); - return; - } + isr1_status >> 8; /* Process MSI interrupts */ if (isr0_status & PCIE_ISR0_MSI_INT_PENDING) advk_pcie_handle_msi(pcie); /* Process legacy interrupts */ - for (i = 0; i < PCI_NUM_INTX; i++) { - if (!(isr1_status & PCIE_ISR1_INTX_ASSERT(i))) - continue; - + for_each_set_bit(i, &isr1_status, PCI_NUM_INTX) { advk_writel(pcie, PCIE_ISR1_INTX_ASSERT(i), PCIE_ISR1_REG); -- Without deviation from the norm, progress is not possible.