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 9CB17C433F5 for ; Sat, 5 Feb 2022 10:53:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233832AbiBEKxz (ORCPT ); Sat, 5 Feb 2022 05:53:55 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46452 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S237662AbiBEKxy (ORCPT ); Sat, 5 Feb 2022 05:53:54 -0500 Received: from ams.source.kernel.org (ams.source.kernel.org [IPv6:2604:1380:4601:e00::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 75527C061346 for ; Sat, 5 Feb 2022 02:53:53 -0800 (PST) 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 096ACB80689 for ; Sat, 5 Feb 2022 10:53:52 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id BA9EAC340E8; Sat, 5 Feb 2022 10:53:50 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1644058430; bh=PhlmbpJV6s/aLdgSou91yb+NkA4DduZk9byZMm4L80U=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=asQ1FSXKNNWx8tYGIuwbkI66d6AiDMVjb/Kdwg6p9vvz9S4SY0HNuiJKFOcF8xi4B 10Iq1Eddu13MJyq20C65yZVAXqEbRYEFtreegLeSB/ZPaU2SbUp3FX5MGYuAM+XQXd zLhu0/zFkSW5ehpWJyDxlsaEl0+t5jnvUPpRty6BK5Filycqi7re4wAx62FL6V995D lJaPO/DPkbYURLKtgqBi+ihnmdY2eCpOv+cnuR4BGaT+5j2E5Nl9HScLJDTk9MiNZw PEjQg77GvBIkWSFpdx+ySdCAOJwcyw2UOtb3rrjzzKdeZw3L40yCohRrY924ObmIUz QGYrOfN6FFX/g== Received: from sofa.misterjones.org ([185.219.108.64] helo=billy-the-mountain.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 1nGIhQ-005dQ0-Bw; Sat, 05 Feb 2022 10:53:48 +0000 Date: Sat, 05 Feb 2022 10:53:48 +0000 Message-ID: <87czk137pf.wl-maz@kernel.org> From: Marc Zyngier To: Lorenzo Pieralisi Cc: Marek =?UTF-8?B?QmVow7pu?= , Bjorn Helgaas , pali@kernel.org, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH v2 02/23] PCI: aardvark: Fix reading MSI interrupt number In-Reply-To: <20220204172400.GA4867@lpieralisi> References: <20220110015018.26359-1-kabel@kernel.org> <20220110015018.26359-3-kabel@kernel.org> <20220204172400.GA4867@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 (aarch64-unknown-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: quoted-printable X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: lorenzo.pieralisi@arm.com, kabel@kernel.org, helgaas@kernel.org, pali@kernel.org, linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.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 Hi Lorenzo, On Fri, 04 Feb 2022 17:24:00 +0000, Lorenzo Pieralisi wrote: >=20 > On Mon, Jan 10, 2022 at 02:49:57AM +0100, Marek Beh=C3=BAn wrote: > > From: Pali Roh=C3=A1r > >=20 > > In advk_pcie_handle_msi() the authors expect that when bit i in the W1C > > register PCIE_MSI_STATUS_REG is cleared, the PCIE_MSI_PAYLOAD_REG is > > updated to contain the MSI number corresponding to index i. > >=20 > > Experiments show that this is not so, and instead PCIE_MSI_PAYLOAD_REG > > always contains the number of the last received MSI, overall. > >=20 > > Do not read PCIE_MSI_PAYLOAD_REG register for determining MSI interrupt > > number. Since Aardvark already forbids more than 32 interrupts and uses > > own allocated hwirq numbers, the msi_idx already corresponds to the > > received MSI number. > >=20 > > Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller d= river") > > Signed-off-by: Pali Roh=C3=A1r > > Signed-off-by: Marek Beh=C3=BAn > > --- > > drivers/pci/controller/pci-aardvark.c | 9 ++------- > > 1 file changed, 2 insertions(+), 7 deletions(-) > >=20 > > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/contro= ller/pci-aardvark.c > > index 62baddd2ca95..fd95ad64c887 100644 > > --- a/drivers/pci/controller/pci-aardvark.c > > +++ b/drivers/pci/controller/pci-aardvark.c > > @@ -1393,7 +1393,6 @@ static void advk_pcie_remove_irq_domain(struct ad= vk_pcie *pcie) > > static void advk_pcie_handle_msi(struct advk_pcie *pcie) > > { > > u32 msi_val, msi_mask, msi_status, msi_idx; > > - u16 msi_data; > > =20 > > msi_mask =3D advk_readl(pcie, PCIE_MSI_MASK_REG); > > msi_val =3D advk_readl(pcie, PCIE_MSI_STATUS_REG); > > @@ -1403,13 +1402,9 @@ static void advk_pcie_handle_msi(struct advk_pci= e *pcie) > > if (!(BIT(msi_idx) & msi_status)) > > continue; > > =20 > > - /* > > - * msi_idx contains bits [4:0] of the msi_data and msi_data > > - * contains 16bit MSI interrupt number > > - */ > > advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG); > > - msi_data =3D advk_readl(pcie, PCIE_MSI_PAYLOAD_REG) & PCIE_MSI_DATA_= MASK; >=20 > Ok, it took me a while to understand how aardvark handles MSIs. >=20 > IIUC, msi_data contains the payload of the latest MSI write received. > > First off, I believe that using a Linux IRQ number for MSI data > (payload)(I guess you rely on its truncated bits [4:0] to trigger the > related MSI IRQs, which I believe is questionable - if not broken) is > not a good idea. >=20 > Is my understanding correct ? > > This patch and the following one are fixing this. Given that this is IRQ > domain code if Marc can cast a look into it that would help me, to make > sure I have not missed anything. I had a look at this series a long while ago, and nothing seem out of sorts on the MSI front. If anything, this is better than what is currently there. As for the rest of the series: Patch 18 is odd: the use of the handle_simple_irq() flow is likely papering over something (I'd expect RP interrupts to be edge triggered), and it is impossible to mask them. But hey, after being singled out by the author as the big bad bully who prevents people from fixing things, I can't say I care. I only objected to patch 23, which is a total no-go. Thanks, M. --=20 Without deviation from the norm, progress is not possible.