diff mbox series

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

Message ID 20210815103624.19528-3-pali@kernel.org
State New
Headers show
Series PCI: aardvark: MSI interrupt fixes | expand

Commit Message

Pali Rohár Aug. 15, 2021, 10:36 a.m. UTC 
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(-)

Comments

Marc Zyngier Aug. 15, 2021, 4:56 p.m. UTC | #1 
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.
Pali Rohár Aug. 15, 2021, 5:36 p.m. UTC | #2 
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.
Marc Zyngier Aug. 15, 2021, 9:55 p.m. UTC | #3 
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
Pali Rohár Aug. 15, 2021, 11:10 p.m. UTC | #4 
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
diff mbox series

Patch

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 |