[PATCH 0/3] PCI: aardvark: MSI interrupt fixes

[PATCH 0/3] PCI: aardvark: MSI interrupt fixes

[Pali Rohár]

This patch series fixes MSI and MSI-X interrupt support in
pci-aardvark.c driver and depends on patch series:
https://lore.kernel.org/linux-pci/20210625090319.10220-1-pali@kernel.org/

Pali Rohár (3):
  PCI: aardvark: Fix reading MSI interrupt number
  PCI: aardvark: Fix masking MSI interrupts
  PCI: aardvark: Enable MSI-X support

 drivers/pci/controller/pci-aardvark.c | 57 +++++++++++++++++++++------
 1 file changed, 44 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH 1/3] PCI: aardvark: Fix reading MSI interrupt number

[Pali Rohár]

Experiments showed that in register PCIE_MSI_PAYLOAD_REG is stored number
of the last received MSI interrupt and not number of MSI interrupt which
belongs to msi_idx bit. Therefore this implies that aardvark HW can cache
only bits [4:0] of received MSI interrupts with effectively means that it
supports only MSI interrupts with numbers 0-31.

Do not read PCIE_MSI_PAYLOAD_REG register for determining MSI interrupt
number. Instead ensure that pci-aardvark.c configures only MSI numbers in
range 0-31 and then msi_idx contains correct received MSI number.

Signed-off-by: Pali Rohár <pali@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/pci/controller/pci-aardvark.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 5e4febdbdd33..bacfccee44fe 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1199,7 +1199,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;
 	int virq;
 
 	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
@@ -1210,17 +1209,13 @@ 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 from MSI inner domain
-		 */
 		advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG);
-		msi_data = advk_readl(pcie, PCIE_MSI_PAYLOAD_REG) & PCIE_MSI_DATA_MASK;
-		virq = irq_find_mapping(pcie->msi_inner_domain, msi_data);
+
+		virq = irq_find_mapping(pcie->msi_inner_domain, msi_idx);
 		if (virq)
 			generic_handle_irq(virq);
 		else
-			dev_err_ratelimited(&pcie->pdev->dev, "unexpected MSI 0x%04hx\n", msi_data);
+			dev_err_ratelimited(&pcie->pdev->dev, "unexpected MSI 0x%02x\n", msi_idx);
 	}
 
 	advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING,
-- 
2.20.1

[PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts

[Pali Rohár]

Masking of individual MSI interrupts is done via PCIE_MSI_MASK_REG
register. At the driver probe time mask all MSI interrupts and then let
kernel IRQ chip code to unmask particular MSI interrupt when needed.

Signed-off-by: Pali Rohár <pali@kernel.org>
Cc: stable@vger.kernel.org # f21a8b1b6837 ("PCI: aardvark: Move to MSI handling using generic MSI support")
---
 drivers/pci/controller/pci-aardvark.c | 44 ++++++++++++++++++++++++---
 1 file changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index bacfccee44fe..96580e1e4539 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -480,12 +480,10 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_REG);
 	advk_writel(pcie, PCIE_IRQ_ALL_MASK, HOST_CTRL_INT_STATUS_REG);
 
-	/* Disable All ISR0/1 Sources */
+	/* Disable All ISR0/1 and MSI Sources */
 	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_MASK_REG);
 	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_MASK_REG);
-
-	/* Unmask all MSIs */
-	advk_writel(pcie, ~(u32)PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
+	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
 
 	/* Unmask summary MSI interrupt */
 	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
@@ -1026,6 +1024,40 @@ static int advk_msi_set_affinity(struct irq_data *irq_data,
 	return -EINVAL;
 }
 
+static void advk_msi_irq_mask(struct irq_data *d)
+{
+	struct advk_pcie *pcie = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	u32 mask;
+
+	mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
+	mask |= BIT(hwirq);
+	advk_writel(pcie, mask, PCIE_MSI_MASK_REG);
+}
+
+static void advk_msi_irq_unmask(struct irq_data *d)
+{
+	struct advk_pcie *pcie = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	u32 mask;
+
+	mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
+	mask &= ~BIT(hwirq);
+	advk_writel(pcie, mask, PCIE_MSI_MASK_REG);
+}
+
+static void advk_msi_top_irq_mask(struct irq_data *d)
+{
+	pci_msi_mask_irq(d);
+	irq_chip_mask_parent(d);
+}
+
+static void advk_msi_top_irq_unmask(struct irq_data *d)
+{
+	pci_msi_unmask_irq(d);
+	irq_chip_unmask_parent(d);
+}
+
 static int advk_msi_irq_domain_alloc(struct irq_domain *domain,
 				     unsigned int virq,
 				     unsigned int nr_irqs, void *args)
@@ -1119,9 +1151,13 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 	bottom_ic->name = "MSI";
 	bottom_ic->irq_compose_msi_msg = advk_msi_irq_compose_msi_msg;
 	bottom_ic->irq_set_affinity = advk_msi_set_affinity;
+	bottom_ic->irq_mask = advk_msi_irq_mask;
+	bottom_ic->irq_unmask = advk_msi_irq_unmask;
 
 	msi_ic = &pcie->msi_irq_chip;
 	msi_ic->name = "advk-MSI";
+	msi_ic->irq_mask = advk_msi_top_irq_mask;
+	msi_ic->irq_unmask = advk_msi_top_irq_unmask;
 
 	msi_di = &pcie->msi_domain_info;
 	msi_di->flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
-- 
2.20.1

Re: [PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts

[Marc Zyngier]

On Sun, 15 Aug 2021 11:36:23 +0100,
Pali Rohár <pali@kernel.org> wrote:
> 
> Masking of individual MSI interrupts is done via PCIE_MSI_MASK_REG
> register. At the driver probe time mask all MSI interrupts and then let
> kernel IRQ chip code to unmask particular MSI interrupt when needed.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Cc: stable@vger.kernel.org # f21a8b1b6837 ("PCI: aardvark: Move to MSI handling using generic MSI support")
> ---
>  drivers/pci/controller/pci-aardvark.c | 44 ++++++++++++++++++++++++---
>  1 file changed, 40 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index bacfccee44fe..96580e1e4539 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -480,12 +480,10 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_REG);
>  	advk_writel(pcie, PCIE_IRQ_ALL_MASK, HOST_CTRL_INT_STATUS_REG);
>  
> -	/* Disable All ISR0/1 Sources */
> +	/* Disable All ISR0/1 and MSI Sources */
>  	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_MASK_REG);
>  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_MASK_REG);
> -
> -	/* Unmask all MSIs */
> -	advk_writel(pcie, ~(u32)PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
> +	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
>  
>  	/* Unmask summary MSI interrupt */
>  	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
> @@ -1026,6 +1024,40 @@ static int advk_msi_set_affinity(struct irq_data *irq_data,
>  	return -EINVAL;
>  }
>  
> +static void advk_msi_irq_mask(struct irq_data *d)
> +{
> +	struct advk_pcie *pcie = d->domain->host_data;
> +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
> +	u32 mask;
> +
> +	mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
> +	mask |= BIT(hwirq);
> +	advk_writel(pcie, mask, PCIE_MSI_MASK_REG);

This isn't atomic, and will results in corruption when two MSIs are
masked/unmasked concurrently.

	M.

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

Re: [PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts

[Pali Rohár]

On Sunday 15 August 2021 17:56:04 Marc Zyngier wrote:
> On Sun, 15 Aug 2021 11:36:23 +0100,
> Pali Rohár <pali@kernel.org> wrote:
> > 
> > Masking of individual MSI interrupts is done via PCIE_MSI_MASK_REG
> > register. At the driver probe time mask all MSI interrupts and then let
> > kernel IRQ chip code to unmask particular MSI interrupt when needed.
> > 
> > Signed-off-by: Pali Rohár <pali@kernel.org>
> > Cc: stable@vger.kernel.org # f21a8b1b6837 ("PCI: aardvark: Move to MSI handling using generic MSI support")
> > ---
> >  drivers/pci/controller/pci-aardvark.c | 44 ++++++++++++++++++++++++---
> >  1 file changed, 40 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> > index bacfccee44fe..96580e1e4539 100644
> > --- a/drivers/pci/controller/pci-aardvark.c
> > +++ b/drivers/pci/controller/pci-aardvark.c
> > @@ -480,12 +480,10 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> >  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_REG);
> >  	advk_writel(pcie, PCIE_IRQ_ALL_MASK, HOST_CTRL_INT_STATUS_REG);
> >  
> > -	/* Disable All ISR0/1 Sources */
> > +	/* Disable All ISR0/1 and MSI Sources */
> >  	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_MASK_REG);
> >  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_MASK_REG);
> > -
> > -	/* Unmask all MSIs */
> > -	advk_writel(pcie, ~(u32)PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
> > +	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
> >  
> >  	/* Unmask summary MSI interrupt */
> >  	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
> > @@ -1026,6 +1024,40 @@ static int advk_msi_set_affinity(struct irq_data *irq_data,
> >  	return -EINVAL;
> >  }
> >  
> > +static void advk_msi_irq_mask(struct irq_data *d)
> > +{
> > +	struct advk_pcie *pcie = d->domain->host_data;
> > +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
> > +	u32 mask;
> > +
> > +	mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
> > +	mask |= BIT(hwirq);
> > +	advk_writel(pcie, mask, PCIE_MSI_MASK_REG);
> 
> This isn't atomic, and will results in corruption when two MSIs are
> masked/unmasked concurrently.

Does it mean that also current implementation of masking legacy
interrupt is incorrect?

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pci-aardvark.c?h=v5.13#n874

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

Re: [PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts

[Marc Zyngier]

On 15 August 2021 18:36:59 BST, "Pali Rohár" <pali@kernel.org> wrote:
>On Sunday 15 August 2021 17:56:04 Marc Zyngier wrote:
>> On Sun, 15 Aug 2021 11:36:23 +0100,
>> Pali Rohár <pali@kernel.org> wrote:
>> > 
>> > Masking of individual MSI interrupts is done via PCIE_MSI_MASK_REG
>> > register. At the driver probe time mask all MSI interrupts and then let
>> > kernel IRQ chip code to unmask particular MSI interrupt when needed.
>> > 
>> > Signed-off-by: Pali Rohár <pali@kernel.org>
>> > Cc: stable@vger.kernel.org # f21a8b1b6837 ("PCI: aardvark: Move to MSI handling using generic MSI support")
>> > ---
>> >  drivers/pci/controller/pci-aardvark.c | 44 ++++++++++++++++++++++++---
>> >  1 file changed, 40 insertions(+), 4 deletions(-)
>> > 
>> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
>> > index bacfccee44fe..96580e1e4539 100644
>> > --- a/drivers/pci/controller/pci-aardvark.c
>> > +++ b/drivers/pci/controller/pci-aardvark.c
>> > @@ -480,12 +480,10 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
>> >  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_REG);
>> >  	advk_writel(pcie, PCIE_IRQ_ALL_MASK, HOST_CTRL_INT_STATUS_REG);
>> >  
>> > -	/* Disable All ISR0/1 Sources */
>> > +	/* Disable All ISR0/1 and MSI Sources */
>> >  	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_MASK_REG);
>> >  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_MASK_REG);
>> > -
>> > -	/* Unmask all MSIs */
>> > -	advk_writel(pcie, ~(u32)PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
>> > +	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
>> >  
>> >  	/* Unmask summary MSI interrupt */
>> >  	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
>> > @@ -1026,6 +1024,40 @@ static int advk_msi_set_affinity(struct irq_data *irq_data,
>> >  	return -EINVAL;
>> >  }
>> >  
>> > +static void advk_msi_irq_mask(struct irq_data *d)
>> > +{
>> > +	struct advk_pcie *pcie = d->domain->host_data;
>> > +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
>> > +	u32 mask;
>> > +
>> > +	mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
>> > +	mask |= BIT(hwirq);
>> > +	advk_writel(pcie, mask, PCIE_MSI_MASK_REG);
>> 
>> This isn't atomic, and will results in corruption when two MSIs are
>> masked/unmasked concurrently.
>
>Does it mean that also current implementation of masking legacy
>interrupt is incorrect?
>
>https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pci-aardvark.c?h=v5.13#n874

Yes, that's completely busted. If you have configuration registers that are shared between interrupts and that the HW doesn't provide set/clear accessors so that it can cope with such races, you need mutual exclusion.

You'd think people would have worked that one out.... 60 years ago?

        M.

Jazz is not dead, it just smells funny

Re: [PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts

[Pali Rohár]

On Sunday 15 August 2021 22:55:01 Marc Zyngier wrote:
> On 15 August 2021 18:36:59 BST, "Pali Rohár" <pali@kernel.org> wrote:
> >On Sunday 15 August 2021 17:56:04 Marc Zyngier wrote:
> >> On Sun, 15 Aug 2021 11:36:23 +0100,
> >> Pali Rohár <pali@kernel.org> wrote:
> >> > 
> >> > Masking of individual MSI interrupts is done via PCIE_MSI_MASK_REG
> >> > register. At the driver probe time mask all MSI interrupts and then let
> >> > kernel IRQ chip code to unmask particular MSI interrupt when needed.
> >> > 
> >> > Signed-off-by: Pali Rohár <pali@kernel.org>
> >> > Cc: stable@vger.kernel.org # f21a8b1b6837 ("PCI: aardvark: Move to MSI handling using generic MSI support")
> >> > ---
> >> >  drivers/pci/controller/pci-aardvark.c | 44 ++++++++++++++++++++++++---
> >> >  1 file changed, 40 insertions(+), 4 deletions(-)
> >> > 
> >> > diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> >> > index bacfccee44fe..96580e1e4539 100644
> >> > --- a/drivers/pci/controller/pci-aardvark.c
> >> > +++ b/drivers/pci/controller/pci-aardvark.c
> >> > @@ -480,12 +480,10 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
> >> >  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_REG);
> >> >  	advk_writel(pcie, PCIE_IRQ_ALL_MASK, HOST_CTRL_INT_STATUS_REG);
> >> >  
> >> > -	/* Disable All ISR0/1 Sources */
> >> > +	/* Disable All ISR0/1 and MSI Sources */
> >> >  	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_MASK_REG);
> >> >  	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_MASK_REG);
> >> > -
> >> > -	/* Unmask all MSIs */
> >> > -	advk_writel(pcie, ~(u32)PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
> >> > +	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
> >> >  
> >> >  	/* Unmask summary MSI interrupt */
> >> >  	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
> >> > @@ -1026,6 +1024,40 @@ static int advk_msi_set_affinity(struct irq_data *irq_data,
> >> >  	return -EINVAL;
> >> >  }
> >> >  
> >> > +static void advk_msi_irq_mask(struct irq_data *d)
> >> > +{
> >> > +	struct advk_pcie *pcie = d->domain->host_data;
> >> > +	irq_hw_number_t hwirq = irqd_to_hwirq(d);
> >> > +	u32 mask;
> >> > +
> >> > +	mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
> >> > +	mask |= BIT(hwirq);
> >> > +	advk_writel(pcie, mask, PCIE_MSI_MASK_REG);
> >> 
> >> This isn't atomic, and will results in corruption when two MSIs are
> >> masked/unmasked concurrently.
> >
> >Does it mean that also current implementation of masking legacy
> >interrupt is incorrect?
> >
> >https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pci-aardvark.c?h=v5.13#n874
> 
> Yes, that's completely busted. If you have configuration registers that are shared between interrupts and that the HW doesn't provide set/clear accessors so that it can cope with such races, you need mutual exclusion.

I see... I did not think about it in this patch as I used same pattern
as was already used for legacy interrupts...

I will this issue for both MSI and legacy interrupts.

Anyway, it looks like that exactly same issue is in pcie-uniphier.c and
pci-ftpci100.c drivers:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/dwc/pcie-uniphier.c?h=v5.13#n171
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pci/controller/pci-ftpci100.c?h=v5.13#n270

> You'd think people would have worked that one out.... 60 years ago?
> 
>         M.
> 
> Jazz is not dead, it just smells funny

[PATCH 3/3] PCI: aardvark: Enable MSI-X support

[Pali Rohár]

According to PCI 3.0 specification, sending both MSI and MSI-X interrupts
is done by DWORD memory write operation to doorbell message address. The
write operation for MSI has zero upper 16 bits and the MSI interrupt number
in the lower 16 bits. The write operation for MSI-X contains a 32-bit value
from MSI-X table.

As driver supports and assigns only interrupt numbers from range 0..31,
enable also MSI-X support.

Testing proved that kernel can correctly receive MSI-X interrupts from PCIe
cards which supports both MSI and MSI-X interrupts.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/pci/controller/pci-aardvark.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 96580e1e4539..279b2884c545 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1161,7 +1161,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 
 	msi_di = &pcie->msi_domain_info;
 	msi_di->flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
-		MSI_FLAG_MULTI_PCI_MSI;
+			MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
 	msi_di->chip = msi_ic;
 
 	pcie->msi_inner_domain =
-- 
2.20.1

[PATCH v2 0/3] PCI: aardvark: MSI interrupt fixes

[Pali Rohár]

This patch series fixes MSI and MSI-X interrupt support in
pci-aardvark.c driver and depends on patch series:
https://lore.kernel.org/linux-pci/20210625090319.10220-1-pali@kernel.org/

Pali Rohár (3):
  PCI: aardvark: Fix reading MSI interrupt number
  PCI: aardvark: Fix masking MSI interrupts
  PCI: aardvark: Enable MSI-X support

 drivers/pci/controller/pci-aardvark.c | 65 +++++++++++++++++++++------
 1 file changed, 52 insertions(+), 13 deletions(-)

-- 
2.20.1

[PATCH v2 1/3] PCI: aardvark: Fix reading MSI interrupt number

[Pali Rohár]

Experiments showed that in register PCIE_MSI_PAYLOAD_REG is stored number
of the last received MSI interrupt and not number of MSI interrupt which
belongs to msi_idx bit. Therefore this implies that aardvark HW can cache
only bits [4:0] of received MSI interrupts with effectively means that it
supports only MSI interrupts with numbers 0-31.

Do not read PCIE_MSI_PAYLOAD_REG register for determining MSI interrupt
number. Instead ensure that pci-aardvark.c configures only MSI numbers in
range 0-31 and then msi_idx contains correct received MSI number.

Signed-off-by: Pali Rohár <pali@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/pci/controller/pci-aardvark.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 48fbfa7eb24c..81c4a9ff91a3 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1232,7 +1232,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;
 	int virq;
 
 	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
@@ -1243,17 +1242,13 @@ 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 from MSI inner domain
-		 */
 		advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG);
-		msi_data = advk_readl(pcie, PCIE_MSI_PAYLOAD_REG) & PCIE_MSI_DATA_MASK;
-		virq = irq_find_mapping(pcie->msi_inner_domain, msi_data);
+
+		virq = irq_find_mapping(pcie->msi_inner_domain, msi_idx);
 		if (virq)
 			generic_handle_irq(virq);
 		else
-			dev_err_ratelimited(&pcie->pdev->dev, "unexpected MSI 0x%04hx\n", msi_data);
+			dev_err_ratelimited(&pcie->pdev->dev, "unexpected MSI 0x%02x\n", msi_idx);
 	}
 
 	advk_writel(pcie, PCIE_ISR0_MSI_INT_PENDING,
-- 
2.20.1

Re: [PATCH v2 1/3] PCI: aardvark: Fix reading MSI interrupt number

[Marc Zyngier]

On Mon, 23 Aug 2021 17:40:31 +0100,
Pali Rohár <pali@kernel.org> wrote:
> 
> Experiments showed that in register PCIE_MSI_PAYLOAD_REG is stored number
> of the last received MSI interrupt and not number of MSI interrupt which
> belongs to msi_idx bit. Therefore this implies that aardvark HW can cache
> only bits [4:0] of received MSI interrupts with effectively means that it
> supports only MSI interrupts with numbers 0-31.
> 
> Do not read PCIE_MSI_PAYLOAD_REG register for determining MSI interrupt
> number. Instead ensure that pci-aardvark.c configures only MSI numbers in
> range 0-31 and then msi_idx contains correct received MSI number.
> 
> Signed-off-by: Pali Rohár <pali@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/pci/controller/pci-aardvark.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
> index 48fbfa7eb24c..81c4a9ff91a3 100644
> --- a/drivers/pci/controller/pci-aardvark.c
> +++ b/drivers/pci/controller/pci-aardvark.c
> @@ -1232,7 +1232,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;
>  	int virq;
>  
>  	msi_mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
> @@ -1243,17 +1242,13 @@ 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 from MSI inner domain
> -		 */
>  		advk_writel(pcie, BIT(msi_idx), PCIE_MSI_STATUS_REG);
> -		msi_data = advk_readl(pcie, PCIE_MSI_PAYLOAD_REG) & PCIE_MSI_DATA_MASK;
> -		virq = irq_find_mapping(pcie->msi_inner_domain, msi_data);
> +
> +		virq = irq_find_mapping(pcie->msi_inner_domain, msi_idx);
>  		if (virq)
>  			generic_handle_irq(virq);

NAK.

As I have explained before, I want this API dead, and I don't feel
like doing another pass at the whole of the kernel tree to remove
these patterns.

A patch targeting stable is not a mandate for using deprecated
APIs. You can always send another patch for stable versions before
5.14.

	M.

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

[PATCH v2 2/3] PCI: aardvark: Fix masking MSI interrupts

[Pali Rohár]

Masking of individual MSI interrupts is done via PCIE_MSI_MASK_REG
register. At the driver probe time mask all MSI interrupts and then let
kernel IRQ chip code to unmask particular MSI interrupt when needed.

Signed-off-by: Pali Rohár <pali@kernel.org>
Cc: stable@vger.kernel.org # f21a8b1b6837 ("PCI: aardvark: Move to MSI handling using generic MSI support")

---
Changes in v2:
* Guard register updates by raw spin lock
---
 drivers/pci/controller/pci-aardvark.c | 52 ++++++++++++++++++++++++---
 1 file changed, 48 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 81c4a9ff91a3..0e81d7f37465 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -241,6 +241,7 @@ struct advk_pcie {
 	struct irq_domain *msi_inner_domain;
 	struct irq_chip msi_bottom_irq_chip;
 	struct irq_chip msi_irq_chip;
+	raw_spinlock_t msi_irq_lock;
 	struct msi_domain_info msi_domain_info;
 	DECLARE_BITMAP(msi_used, MSI_IRQ_NUM);
 	struct mutex msi_used_lock;
@@ -481,12 +482,10 @@ static void advk_pcie_setup_hw(struct advk_pcie *pcie)
 	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_REG);
 	advk_writel(pcie, PCIE_IRQ_ALL_MASK, HOST_CTRL_INT_STATUS_REG);
 
-	/* Disable All ISR0/1 Sources */
+	/* Disable All ISR0/1 and MSI Sources */
 	advk_writel(pcie, PCIE_ISR0_ALL_MASK, PCIE_ISR0_MASK_REG);
 	advk_writel(pcie, PCIE_ISR1_ALL_MASK, PCIE_ISR1_MASK_REG);
-
-	/* Unmask all MSIs */
-	advk_writel(pcie, ~(u32)PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
+	advk_writel(pcie, PCIE_MSI_ALL_MASK, PCIE_MSI_MASK_REG);
 
 	/* Unmask summary MSI interrupt */
 	reg = advk_readl(pcie, PCIE_ISR0_MASK_REG);
@@ -1051,6 +1050,46 @@ static int advk_msi_set_affinity(struct irq_data *irq_data,
 	return -EINVAL;
 }
 
+static void advk_msi_irq_mask(struct irq_data *d)
+{
+	struct advk_pcie *pcie = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 mask;
+
+	raw_spin_lock_irqsave(&pcie->msi_irq_lock, flags);
+	mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
+	mask |= BIT(hwirq);
+	advk_writel(pcie, mask, PCIE_MSI_MASK_REG);
+	raw_spin_unlock_irqrestore(&pcie->msi_irq_lock, flags);
+}
+
+static void advk_msi_irq_unmask(struct irq_data *d)
+{
+	struct advk_pcie *pcie = d->domain->host_data;
+	irq_hw_number_t hwirq = irqd_to_hwirq(d);
+	unsigned long flags;
+	u32 mask;
+
+	raw_spin_lock_irqsave(&pcie->msi_irq_lock, flags);
+	mask = advk_readl(pcie, PCIE_MSI_MASK_REG);
+	mask &= ~BIT(hwirq);
+	advk_writel(pcie, mask, PCIE_MSI_MASK_REG);
+	raw_spin_unlock_irqrestore(&pcie->msi_irq_lock, flags);
+}
+
+static void advk_msi_top_irq_mask(struct irq_data *d)
+{
+	pci_msi_mask_irq(d);
+	irq_chip_mask_parent(d);
+}
+
+static void advk_msi_top_irq_unmask(struct irq_data *d)
+{
+	pci_msi_unmask_irq(d);
+	irq_chip_unmask_parent(d);
+}
+
 static int advk_msi_irq_domain_alloc(struct irq_domain *domain,
 				     unsigned int virq,
 				     unsigned int nr_irqs, void *args)
@@ -1143,6 +1182,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 	struct irq_chip *bottom_ic, *msi_ic;
 	struct msi_domain_info *msi_di;
 
+	raw_spin_lock_init(&pcie->msi_irq_lock);
 	mutex_init(&pcie->msi_used_lock);
 
 	bottom_ic = &pcie->msi_bottom_irq_chip;
@@ -1150,9 +1190,13 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 	bottom_ic->name = "MSI";
 	bottom_ic->irq_compose_msi_msg = advk_msi_irq_compose_msi_msg;
 	bottom_ic->irq_set_affinity = advk_msi_set_affinity;
+	bottom_ic->irq_mask = advk_msi_irq_mask;
+	bottom_ic->irq_unmask = advk_msi_irq_unmask;
 
 	msi_ic = &pcie->msi_irq_chip;
 	msi_ic->name = "advk-MSI";
+	msi_ic->irq_mask = advk_msi_top_irq_mask;
+	msi_ic->irq_unmask = advk_msi_top_irq_unmask;
 
 	msi_di = &pcie->msi_domain_info;
 	msi_di->flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
-- 
2.20.1

[PATCH v2 3/3] PCI: aardvark: Enable MSI-X support

[Pali Rohár]

According to PCI 3.0 specification, sending both MSI and MSI-X interrupts
is done by DWORD memory write operation to doorbell message address. The
write operation for MSI has zero upper 16 bits and the MSI interrupt number
in the lower 16 bits. The write operation for MSI-X contains a 32-bit value
from MSI-X table.

As driver supports and assigns only interrupt numbers from range 0..31,
enable also MSI-X support.

Testing proved that kernel can correctly receive MSI-X interrupts from PCIe
cards which supports both MSI and MSI-X interrupts.

Signed-off-by: Pali Rohár <pali@kernel.org>
Reviewed-by: Marek Behún <kabel@kernel.org>
Cc: stable@vger.kernel.org
---
 drivers/pci/controller/pci-aardvark.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pci/controller/pci-aardvark.c b/drivers/pci/controller/pci-aardvark.c
index 0e81d7f37465..2c944a04fba8 100644
--- a/drivers/pci/controller/pci-aardvark.c
+++ b/drivers/pci/controller/pci-aardvark.c
@@ -1200,7 +1200,7 @@ static int advk_pcie_init_msi_irq_domain(struct advk_pcie *pcie)
 
 	msi_di = &pcie->msi_domain_info;
 	msi_di->flags = MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS |
-		MSI_FLAG_MULTI_PCI_MSI;
+			MSI_FLAG_MULTI_PCI_MSI | MSI_FLAG_PCI_MSIX;
 	msi_di->chip = msi_ic;
 
 	pcie->msi_inner_domain =
-- 
2.20.1