Re: [PATCH v2 02/23] PCI: aardvark: Fix reading MSI interrupt number

From: Marc Zyngier <maz@kernel.org>
To: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Cc: "Marek Behún" <kabel@kernel.org>,
	"Bjorn Helgaas" <helgaas@kernel.org>,
	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
Date: Sat, 05 Feb 2022 10:53:48 +0000	[thread overview]
Message-ID: <87czk137pf.wl-maz@kernel.org> (raw)
In-Reply-To: <20220204172400.GA4867@lpieralisi>

Hi Lorenzo,

On Fri, 04 Feb 2022 17:24:00 +0000,
Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> wrote:
> 
> On Mon, Jan 10, 2022 at 02:49:57AM +0100, Marek Behún wrote:
> > From: Pali Rohár <pali@kernel.org>
> > 
> > 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.
> > 
> > Experiments show that this is not so, and instead PCIE_MSI_PAYLOAD_REG
> > always contains the number of the last received MSI, overall.
> > 
> > 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.
> > 
> > Fixes: 8c39d710363c ("PCI: aardvark: Add Aardvark PCI host controller driver")
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Signed-off-by: Marek Behún <kabel@kernel.org>
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/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 advk_pcie *pcie)
> >  static void advk_pcie_handle_msi(struct advk_pcie *pcie)
> >  {
> >  	u32 msi_val, msi_mask, msi_status, msi_idx;
> > -	u16 msi_data;
> >  
> >  	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
> >  	msi_val = advk_readl(pcie, PCIE_MSI_STATUS_REG);
> > @@ -1403,13 +1402,9 @@ static void advk_pcie_handle_msi(struct advk_pcie *pcie)
> >  		if (!(BIT(msi_idx) & msi_status))
> >  			continue;
> >  
> > -		/*
> > -		 * 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 = advk_readl(pcie, PCIE_MSI_PAYLOAD_REG) & PCIE_MSI_DATA_MASK;
> 
> Ok, it took me a while to understand how aardvark handles MSIs.
> 
> 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.
> 
> 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.

-- 
Without deviation from the norm, progress is not possible.