diff mbox series

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

Message ID 20220110015018.26359-3-kabel@kernel.org (mailing list archive)
State New, archived
Headers show
Series PCI: aardvark controller fixes BATCH 4 | expand

Commit Message

Marek Behún Jan. 10, 2022, 1:49 a.m. UTC 
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(-)

Comments

Lorenzo Pieralisi Feb. 4, 2022, 5:24 p.m. UTC | #1 
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.

Certainly replacing the MSI payload to the HW irq number makes sense to
me (and this patch makes sense too, I think mainline code may miss some
MSI IRQs if I understand this patch correctly).

Lorenzo

> -		generic_handle_irq(msi_data);
> +		if (generic_handle_domain_irq(pcie->msi_inner_domain, msi_idx) == -EINVAL)
> +			dev_err_ratelimited(&pcie->pdev->dev, "unexpected MSI 0x%02x\n", msi_idx);
>  	}
>  
>  	advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING,
> -- 
> 2.34.1
>
Marc Zyngier Feb. 5, 2022, 10:53 a.m. UTC | #2 
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.
diff mbox series

Patch

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;
-		generic_handle_irq(msi_data);
+		if (generic_handle_domain_irq(pcie->msi_inner_domain, msi_idx) == -EINVAL)
+			dev_err_ratelimited(&pcie->pdev->dev, "unexpected MSI 0x%02x\n", msi_idx);
 	}
 
 	advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING,