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 vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id DB611C433F5 for ; Fri, 28 Jan 2022 08:57:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230350AbiA1I5W (ORCPT ); Fri, 28 Jan 2022 03:57:22 -0500 Received: from dfw.source.kernel.org ([139.178.84.217]:41558 "EHLO dfw.source.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229847AbiA1I5U (ORCPT ); Fri, 28 Jan 2022 03:57:20 -0500 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 440B961DB4; Fri, 28 Jan 2022 08:57:20 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A508EC340E0; Fri, 28 Jan 2022 08:57:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1643360239; bh=eA0GRKE5jNFxledJDISs9E2n145awy0LOLWI3Kj0OCw=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=hfnsg/b/fnzfO7W3HdljYTzrlb2uSKZmWuNmXxQxBjBzTF4/87QiwUfhBOrUqcQ9o VqEpBVJ/09By38KslCaSZifpWNtzGRrqT45RbhqLwii/m0Ve4CMGsEk1YGj62kQ88y Te0x0ARTckdiSvT1Be6JX4lYnidxJvX3ceXUi/Csn+xeebovSgHtaiqQF0+pzCyf+5 zT4AgmPr6dH0hYgSdc8kPg6pYsU7P877Gkc1fdKVjATVVBfFesDB/1L9PQ480J5MrB xfL5EjeMNo1drRYV1uYTG0Iqr8bmMqj7aljAsGvNS/gNE0I9zjBy0Us89b2NHJGt69 9Y5CvGzyJFhgg== 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 1nDN4H-003lDg-8T; Fri, 28 Jan 2022 08:57:17 +0000 Date: Fri, 28 Jan 2022 08:57:16 +0000 Message-ID: <87r18s5jbn.wl-maz@kernel.org> From: Marc Zyngier To: Bjorn Helgaas Cc: "qizhong.cheng" , Ryder Lee , Jianjun Wang , Lorenzo Pieralisi , Krzysztof =?UTF-8?B?V2lsY3p5xYRz?= =?UTF-8?B?a2k=?= , Bjorn Helgaas , linux-pci@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, chuanjia.liu@mediatek.com, Srikanth Thokala , Pratyush Anand , Thomas Petazzoni , Pali =?UTF-8?B?Um9ow6Fy?= Subject: Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence In-Reply-To: <20220127212100.GA102267@bhelgaas> References: <20220127212100.GA102267@bhelgaas> 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=US-ASCII X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: helgaas@kernel.org, qizhong.cheng@mediatek.com, ryder.lee@mediatek.com, jianjun.wang@mediatek.com, lorenzo.pieralisi@arm.com, kw@linux.com, bhelgaas@google.com, linux-pci@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, chuanjia.liu@mediatek.com, srikanth.thokala@intel.com, pratyush.anand@gmail.com, thomas.petazzoni@bootlin.com, pali@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-kernel@vger.kernel.org On Thu, 27 Jan 2022 21:21:00 +0000, Bjorn Helgaas wrote: > > [+cc Srikanth, Pratyush, Thomas, Pali, Ryder, Jianjun] > > On Wed, Jan 26, 2022 at 11:37:58AM +0800, qizhong.cheng wrote: > > On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote: > > > On 2022-01-25 16:57, Bjorn Helgaas wrote: > > > > On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng wrote: > > > > > As an edge-triggered interrupts, its interrupt status should > > > > > be cleared before dispatch to the handler of device. > > > > > > > > I'm not an IRQ expert, but the reasoning that "we should clear > > > > the MSI interrupt status before dispatching the handler because > > > > MSI is an edge-triggered interrupt" doesn't seem completely > > > > convincing because your code will now look like this: > > > > > > > > /* Clear the INTx */ > > > > writel(1 << bit, port->base + PCIE_INT_STATUS); > > > > generic_handle_domain_irq(port->irq_domain, bit - INTX_SHIFT); > > > > ... > > > > > > > > /* Clear MSI interrupt status */ > > > > writel(MSI_STATUS, port->base + PCIE_INT_STATUS); > > > > generic_handle_domain_irq(port->inner_domain, bit); > > > > > > > > You clear interrupt status before dispatching the handler for > > > > *both* level-triggered INTx interrupts and edge-triggered MSI > > > > interrupts. > > > > > > > > So it doesn't seem that simply being edge-triggered is the > > > > critical factor here. > > > > > > This is the usual problem with these half-baked implementations. > > > The signalling to the primary interrupt controller is level, as > > > they take a multitude of input and (crucially) latch the MSI > > > edges. Effectively, this is an edge-to-level converter, with all > > > the problems that this creates. > > > > > > By clearing the status *after* the handling, you lose edges that > > > have been received and coalesced after the read of the status > > > register. By clearing it *before*, you are acknowledging the > > > interrupts early, and allowing them to be coalesced independently > > > of the ones that have been received earlier. > > > > > > This is however mostly an educated guess. Someone with access to > > > the TRM should verify this. > > > > Yes, as Maz said, we save the edge-interrupt status so that it > > becomes a level-interrupt. This is similar to an edge-to-level > > converter, so we need to clear it *before*. We found this problem > > through a lot of experiments and tested this patch. > > I thought there might be other host controllers with similar design, > so I looked at all the other drivers and tried to figure out whether > any others had similar problems. > > The ones below look suspicious to me because they all clear some sort > of status register *after* handling an MSI. Can you guys take a look > and make sure they are working correctly? > > keembay_pcie_msi_irq_handler > status = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS) > if (status & MSI_CTRL_INT) > dw_handle_msi_irq > generic_handle_domain_irq > writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS) > > spear13xx_pcie_irq_handler > status = readl(&app_reg->int_sts) > if (status & MSI_CTRL_INT) > dw_handle_msi_irq > generic_handle_domain_irq > writel(status, &app_reg->int_clr) I think these two are fine. The top level interrupt is only a level signal that the is something to process. The only thing that is unclear is what the effect of writing to that status register if MSIs are pending at that point. A sane implementation would just ignore the write. The actual processing is done in dw_handle_msi_irq(), reading the PCIE_MSI_INTR0_STATUS register. This same register is then used to Ack the interrupt, one bit at a time, as interrupts are handled (see dw_pci_bottom_ack). Ack taking place before the handling, it makes it safe for edge delivery. > > advk_pcie_handle_int > isr0_status = advk_readl(pcie, PCIE_ISR0_REG) > if (isr0_status & PCIE_ISR0_MSI_INT_PENDING) > advk_pcie_handle_msi > advk_readl(pcie, PCIE_MSI_STATUS_REG) > advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG) > generic_handle_irq > advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING, PCIE_ISR0_REG) Same thing, I guess. It is just that the Ack has been open-coded. > > mtk_pcie_irq_handler > status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG) > for_each_set_bit_from(irq_bit, &status, ...) > mtk_pcie_msi_handler > generic_handle_domain_irq > writel_relaxed(BIT(irq_bit), pcie->base + PCIE_INT_STATUS_REG) Similar thing. The PCIE_MSI_SET_STATUS register is read first, and then written back in the ack callback. M. -- Without deviation from the norm, progress is not possible. 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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3CD7CC433F5 for ; Fri, 28 Jan 2022 08:57:58 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=k5Lanhwfi9R0bb8AUmSOAcn2+K6OWAUUXJfN3JNAwv8=; b=J77znyHUeFqc47 LarpVwPvGeNgbRwdnIgPy0Al6NjU/Y69JX0XCtBFThnhUzuEeDuYbpl5cfdvSO9r2azcujPvbSN/V 2OvWJS4OjkYcnJE0mOSEosDzBoRiILJaOXNZnG8Q+baDSU58UEm1N2pNQEWXHhTEVKj27WKnuwGK1 IYMxgUXy9/HRyU6EEQyXVE+keEZydVuDw7h5ch3AyAaWt+xMb3r0Q29NT0qvwtMQHK54ukJ2wotou QtCBoHn+ACWsbn0HHVTw9iV3kf7OFCZJLNT0xph12SnlCRY6zZLVI9//UjTyKIOiXoF0qs4m4J7lp CmybidnSbidaOviSGAMQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nDN4p-00166X-TC; Fri, 28 Jan 2022 08:57:51 +0000 Received: from ams.source.kernel.org ([145.40.68.75]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nDN4M-0015xh-4D; Fri, 28 Jan 2022 08:57:24 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id D0E96B824CF; Fri, 28 Jan 2022 08:57:20 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A508EC340E0; Fri, 28 Jan 2022 08:57:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1643360239; bh=eA0GRKE5jNFxledJDISs9E2n145awy0LOLWI3Kj0OCw=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=hfnsg/b/fnzfO7W3HdljYTzrlb2uSKZmWuNmXxQxBjBzTF4/87QiwUfhBOrUqcQ9o VqEpBVJ/09By38KslCaSZifpWNtzGRrqT45RbhqLwii/m0Ve4CMGsEk1YGj62kQ88y Te0x0ARTckdiSvT1Be6JX4lYnidxJvX3ceXUi/Csn+xeebovSgHtaiqQF0+pzCyf+5 zT4AgmPr6dH0hYgSdc8kPg6pYsU7P877Gkc1fdKVjATVVBfFesDB/1L9PQ480J5MrB xfL5EjeMNo1drRYV1uYTG0Iqr8bmMqj7aljAsGvNS/gNE0I9zjBy0Us89b2NHJGt69 9Y5CvGzyJFhgg== 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 1nDN4H-003lDg-8T; Fri, 28 Jan 2022 08:57:17 +0000 Date: Fri, 28 Jan 2022 08:57:16 +0000 Message-ID: <87r18s5jbn.wl-maz@kernel.org> From: Marc Zyngier To: Bjorn Helgaas Cc: "qizhong.cheng" , Ryder Lee , Jianjun Wang , Lorenzo Pieralisi , Krzysztof =?UTF-8?B?V2lsY3p5xYRz?= =?UTF-8?B?a2k=?= , Bjorn Helgaas , linux-pci@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, chuanjia.liu@mediatek.com, Srikanth Thokala , Pratyush Anand , Thomas Petazzoni , Pali =?UTF-8?B?Um9ow6Fy?= Subject: Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence In-Reply-To: <20220127212100.GA102267@bhelgaas> References: <20220127212100.GA102267@bhelgaas> 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") X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: helgaas@kernel.org, qizhong.cheng@mediatek.com, ryder.lee@mediatek.com, jianjun.wang@mediatek.com, lorenzo.pieralisi@arm.com, kw@linux.com, bhelgaas@google.com, linux-pci@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, chuanjia.liu@mediatek.com, srikanth.thokala@intel.com, pratyush.anand@gmail.com, thomas.petazzoni@bootlin.com, pali@kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220128_005722_498737_942DE11F X-CRM114-Status: GOOD ( 42.09 ) X-BeenThere: linux-mediatek@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "Linux-mediatek" Errors-To: linux-mediatek-bounces+linux-mediatek=archiver.kernel.org@lists.infradead.org On Thu, 27 Jan 2022 21:21:00 +0000, Bjorn Helgaas wrote: > > [+cc Srikanth, Pratyush, Thomas, Pali, Ryder, Jianjun] > > On Wed, Jan 26, 2022 at 11:37:58AM +0800, qizhong.cheng wrote: > > On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote: > > > On 2022-01-25 16:57, Bjorn Helgaas wrote: > > > > On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng wrote: > > > > > As an edge-triggered interrupts, its interrupt status should > > > > > be cleared before dispatch to the handler of device. > > > > > > > > I'm not an IRQ expert, but the reasoning that "we should clear > > > > the MSI interrupt status before dispatching the handler because > > > > MSI is an edge-triggered interrupt" doesn't seem completely > > > > convincing because your code will now look like this: > > > > > > > > /* Clear the INTx */ > > > > writel(1 << bit, port->base + PCIE_INT_STATUS); > > > > generic_handle_domain_irq(port->irq_domain, bit - INTX_SHIFT); > > > > ... > > > > > > > > /* Clear MSI interrupt status */ > > > > writel(MSI_STATUS, port->base + PCIE_INT_STATUS); > > > > generic_handle_domain_irq(port->inner_domain, bit); > > > > > > > > You clear interrupt status before dispatching the handler for > > > > *both* level-triggered INTx interrupts and edge-triggered MSI > > > > interrupts. > > > > > > > > So it doesn't seem that simply being edge-triggered is the > > > > critical factor here. > > > > > > This is the usual problem with these half-baked implementations. > > > The signalling to the primary interrupt controller is level, as > > > they take a multitude of input and (crucially) latch the MSI > > > edges. Effectively, this is an edge-to-level converter, with all > > > the problems that this creates. > > > > > > By clearing the status *after* the handling, you lose edges that > > > have been received and coalesced after the read of the status > > > register. By clearing it *before*, you are acknowledging the > > > interrupts early, and allowing them to be coalesced independently > > > of the ones that have been received earlier. > > > > > > This is however mostly an educated guess. Someone with access to > > > the TRM should verify this. > > > > Yes, as Maz said, we save the edge-interrupt status so that it > > becomes a level-interrupt. This is similar to an edge-to-level > > converter, so we need to clear it *before*. We found this problem > > through a lot of experiments and tested this patch. > > I thought there might be other host controllers with similar design, > so I looked at all the other drivers and tried to figure out whether > any others had similar problems. > > The ones below look suspicious to me because they all clear some sort > of status register *after* handling an MSI. Can you guys take a look > and make sure they are working correctly? > > keembay_pcie_msi_irq_handler > status = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS) > if (status & MSI_CTRL_INT) > dw_handle_msi_irq > generic_handle_domain_irq > writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS) > > spear13xx_pcie_irq_handler > status = readl(&app_reg->int_sts) > if (status & MSI_CTRL_INT) > dw_handle_msi_irq > generic_handle_domain_irq > writel(status, &app_reg->int_clr) I think these two are fine. The top level interrupt is only a level signal that the is something to process. The only thing that is unclear is what the effect of writing to that status register if MSIs are pending at that point. A sane implementation would just ignore the write. The actual processing is done in dw_handle_msi_irq(), reading the PCIE_MSI_INTR0_STATUS register. This same register is then used to Ack the interrupt, one bit at a time, as interrupts are handled (see dw_pci_bottom_ack). Ack taking place before the handling, it makes it safe for edge delivery. > > advk_pcie_handle_int > isr0_status = advk_readl(pcie, PCIE_ISR0_REG) > if (isr0_status & PCIE_ISR0_MSI_INT_PENDING) > advk_pcie_handle_msi > advk_readl(pcie, PCIE_MSI_STATUS_REG) > advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG) > generic_handle_irq > advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING, PCIE_ISR0_REG) Same thing, I guess. It is just that the Ack has been open-coded. > > mtk_pcie_irq_handler > status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG) > for_each_set_bit_from(irq_bit, &status, ...) > mtk_pcie_msi_handler > generic_handle_domain_irq > writel_relaxed(BIT(irq_bit), pcie->base + PCIE_INT_STATUS_REG) Similar thing. The PCIE_MSI_SET_STATUS register is read first, and then written back in the ack callback. M. -- Without deviation from the norm, progress is not possible. _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek 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 bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 5F99DC433F5 for ; Fri, 28 Jan 2022 08:58:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Subject:Cc:To:From:Message-ID:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=qrl6lL9B31dvFW1E3+6EB/e1WRpRwUzF0wzF0Ch+7kA=; b=QhDzW6hVJUMgFt qANApAwwkSl/00N4zHY3ALiNVs03P14PRmOn0ijFb0IWSyh0TNSAsjU+9k/M0dpw5d7t/wxRKOzWJ Nmilw9GdCvbhmu+s5YoYiA9N/RE4hBDwa1gr2zkkdZoBY6CuoJlKZslsmh9LlQUYDO4PkMd154h8U YRQ90Npyhko1P5/IruteXP18pD1KLb9WPEvoEPng332hepDoIhUiu6k79l9bMfTInvIE/pHgYgJwV irW0G5CLtbsGV2vp+QNmRQCbfIIav++erp5mHjQzeA+fIxKvmjm16IKA/r/7EpDK7QcWtMgl6wggi NcjhinSyskP11AMxq/jA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1nDN4T-001607-1j; Fri, 28 Jan 2022 08:57:29 +0000 Received: from ams.source.kernel.org ([145.40.68.75]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1nDN4M-0015xh-4D; Fri, 28 Jan 2022 08:57:24 +0000 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id D0E96B824CF; Fri, 28 Jan 2022 08:57:20 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id A508EC340E0; Fri, 28 Jan 2022 08:57:19 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1643360239; bh=eA0GRKE5jNFxledJDISs9E2n145awy0LOLWI3Kj0OCw=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=hfnsg/b/fnzfO7W3HdljYTzrlb2uSKZmWuNmXxQxBjBzTF4/87QiwUfhBOrUqcQ9o VqEpBVJ/09By38KslCaSZifpWNtzGRrqT45RbhqLwii/m0Ve4CMGsEk1YGj62kQ88y Te0x0ARTckdiSvT1Be6JX4lYnidxJvX3ceXUi/Csn+xeebovSgHtaiqQF0+pzCyf+5 zT4AgmPr6dH0hYgSdc8kPg6pYsU7P877Gkc1fdKVjATVVBfFesDB/1L9PQ480J5MrB xfL5EjeMNo1drRYV1uYTG0Iqr8bmMqj7aljAsGvNS/gNE0I9zjBy0Us89b2NHJGt69 9Y5CvGzyJFhgg== 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 1nDN4H-003lDg-8T; Fri, 28 Jan 2022 08:57:17 +0000 Date: Fri, 28 Jan 2022 08:57:16 +0000 Message-ID: <87r18s5jbn.wl-maz@kernel.org> From: Marc Zyngier To: Bjorn Helgaas Cc: "qizhong.cheng" , Ryder Lee , Jianjun Wang , Lorenzo Pieralisi , Krzysztof =?UTF-8?B?V2lsY3p5xYRz?= =?UTF-8?B?a2k=?= , Bjorn Helgaas , linux-pci@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, chuanjia.liu@mediatek.com, Srikanth Thokala , Pratyush Anand , Thomas Petazzoni , Pali =?UTF-8?B?Um9ow6Fy?= Subject: Re: [PATCH] PCI: mediatek: Change MSI interrupt processing sequence In-Reply-To: <20220127212100.GA102267@bhelgaas> References: <20220127212100.GA102267@bhelgaas> 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") X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: helgaas@kernel.org, qizhong.cheng@mediatek.com, ryder.lee@mediatek.com, jianjun.wang@mediatek.com, lorenzo.pieralisi@arm.com, kw@linux.com, bhelgaas@google.com, linux-pci@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, chuanjia.liu@mediatek.com, srikanth.thokala@intel.com, pratyush.anand@gmail.com, thomas.petazzoni@bootlin.com, pali@kernel.org X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220128_005722_498737_942DE11F X-CRM114-Status: GOOD ( 42.09 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Thu, 27 Jan 2022 21:21:00 +0000, Bjorn Helgaas wrote: > > [+cc Srikanth, Pratyush, Thomas, Pali, Ryder, Jianjun] > > On Wed, Jan 26, 2022 at 11:37:58AM +0800, qizhong.cheng wrote: > > On Tue, 2022-01-25 at 17:21 +0000, Marc Zyngier wrote: > > > On 2022-01-25 16:57, Bjorn Helgaas wrote: > > > > On Sun, Jan 23, 2022 at 11:33:06AM +0800, qizhong cheng wrote: > > > > > As an edge-triggered interrupts, its interrupt status should > > > > > be cleared before dispatch to the handler of device. > > > > > > > > I'm not an IRQ expert, but the reasoning that "we should clear > > > > the MSI interrupt status before dispatching the handler because > > > > MSI is an edge-triggered interrupt" doesn't seem completely > > > > convincing because your code will now look like this: > > > > > > > > /* Clear the INTx */ > > > > writel(1 << bit, port->base + PCIE_INT_STATUS); > > > > generic_handle_domain_irq(port->irq_domain, bit - INTX_SHIFT); > > > > ... > > > > > > > > /* Clear MSI interrupt status */ > > > > writel(MSI_STATUS, port->base + PCIE_INT_STATUS); > > > > generic_handle_domain_irq(port->inner_domain, bit); > > > > > > > > You clear interrupt status before dispatching the handler for > > > > *both* level-triggered INTx interrupts and edge-triggered MSI > > > > interrupts. > > > > > > > > So it doesn't seem that simply being edge-triggered is the > > > > critical factor here. > > > > > > This is the usual problem with these half-baked implementations. > > > The signalling to the primary interrupt controller is level, as > > > they take a multitude of input and (crucially) latch the MSI > > > edges. Effectively, this is an edge-to-level converter, with all > > > the problems that this creates. > > > > > > By clearing the status *after* the handling, you lose edges that > > > have been received and coalesced after the read of the status > > > register. By clearing it *before*, you are acknowledging the > > > interrupts early, and allowing them to be coalesced independently > > > of the ones that have been received earlier. > > > > > > This is however mostly an educated guess. Someone with access to > > > the TRM should verify this. > > > > Yes, as Maz said, we save the edge-interrupt status so that it > > becomes a level-interrupt. This is similar to an edge-to-level > > converter, so we need to clear it *before*. We found this problem > > through a lot of experiments and tested this patch. > > I thought there might be other host controllers with similar design, > so I looked at all the other drivers and tried to figure out whether > any others had similar problems. > > The ones below look suspicious to me because they all clear some sort > of status register *after* handling an MSI. Can you guys take a look > and make sure they are working correctly? > > keembay_pcie_msi_irq_handler > status = readl(pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS) > if (status & MSI_CTRL_INT) > dw_handle_msi_irq > generic_handle_domain_irq > writel(status, pcie->apb_base + PCIE_REGS_INTERRUPT_STATUS) > > spear13xx_pcie_irq_handler > status = readl(&app_reg->int_sts) > if (status & MSI_CTRL_INT) > dw_handle_msi_irq > generic_handle_domain_irq > writel(status, &app_reg->int_clr) I think these two are fine. The top level interrupt is only a level signal that the is something to process. The only thing that is unclear is what the effect of writing to that status register if MSIs are pending at that point. A sane implementation would just ignore the write. The actual processing is done in dw_handle_msi_irq(), reading the PCIE_MSI_INTR0_STATUS register. This same register is then used to Ack the interrupt, one bit at a time, as interrupts are handled (see dw_pci_bottom_ack). Ack taking place before the handling, it makes it safe for edge delivery. > > advk_pcie_handle_int > isr0_status = advk_readl(pcie, PCIE_ISR0_REG) > if (isr0_status & PCIE_ISR0_MSI_INT_PENDING) > advk_pcie_handle_msi > advk_readl(pcie, PCIE_MSI_STATUS_REG) > advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG) > generic_handle_irq > advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING, PCIE_ISR0_REG) Same thing, I guess. It is just that the Ack has been open-coded. > > mtk_pcie_irq_handler > status = readl_relaxed(pcie->base + PCIE_INT_STATUS_REG) > for_each_set_bit_from(irq_bit, &status, ...) > mtk_pcie_msi_handler > generic_handle_domain_irq > writel_relaxed(BIT(irq_bit), pcie->base + PCIE_INT_STATUS_REG) Similar thing. The PCIE_MSI_SET_STATUS register is read first, and then written back in the ack callback. M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel