LKML Archive on lore.kernel.org help / color / mirror / Atom feed
* [PATCH 0/3] PCI: aardvark: MSI interrupt fixes @ 2021-08-15 10:36 Pali Rohár 2021-08-15 10:36 ` [PATCH 1/3] PCI: aardvark: Fix reading MSI interrupt number Pali Rohár ` (3 more replies) 0 siblings, 4 replies; 13+ messages in thread From: Pali Rohár @ 2021-08-15 10:36 UTC (permalink / raw) To: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, Rob Herring Cc: Krzysztof Wilczyński, Marek Behún, Marc Zyngier, linux-pci, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 1/3] PCI: aardvark: Fix reading MSI interrupt number 2021-08-15 10:36 [PATCH 0/3] PCI: aardvark: MSI interrupt fixes Pali Rohár @ 2021-08-15 10:36 ` Pali Rohár 2021-08-15 10:36 ` [PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts Pali Rohár ` (2 subsequent siblings) 3 siblings, 0 replies; 13+ messages in thread From: Pali Rohár @ 2021-08-15 10:36 UTC (permalink / raw) To: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, Rob Herring Cc: Krzysztof Wilczyński, Marek Behún, Marc Zyngier, linux-pci, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts 2021-08-15 10:36 [PATCH 0/3] PCI: aardvark: MSI interrupt fixes Pali Rohár 2021-08-15 10:36 ` [PATCH 1/3] PCI: aardvark: Fix reading MSI interrupt number Pali Rohár @ 2021-08-15 10:36 ` Pali Rohár 2021-08-15 16:56 ` Marc Zyngier 2021-08-15 10:36 ` [PATCH 3/3] PCI: aardvark: Enable MSI-X support Pali Rohár 2021-08-23 16:40 ` [PATCH v2 0/3] PCI: aardvark: MSI interrupt fixes Pali Rohár 3 siblings, 1 reply; 13+ messages in thread From: Pali Rohár @ 2021-08-15 10:36 UTC (permalink / raw) To: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, Rob Herring Cc: Krzysztof Wilczyński, Marek Behún, Marc Zyngier, linux-pci, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts 2021-08-15 10:36 ` [PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts Pali Rohár @ 2021-08-15 16:56 ` Marc Zyngier 2021-08-15 17:36 ` Pali Rohár 0 siblings, 1 reply; 13+ messages in thread From: Marc Zyngier @ 2021-08-15 16:56 UTC (permalink / raw) To: Pali Rohár Cc: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, Rob Herring, Krzysztof Wilczyński, Marek Behún, linux-pci, linux-kernel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts 2021-08-15 16:56 ` Marc Zyngier @ 2021-08-15 17:36 ` Pali Rohár 2021-08-15 21:55 ` Marc Zyngier 0 siblings, 1 reply; 13+ messages in thread From: Pali Rohár @ 2021-08-15 17:36 UTC (permalink / raw) To: Marc Zyngier Cc: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, Rob Herring, Krzysztof Wilczyński, Marek Behún, linux-pci, linux-kernel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts 2021-08-15 17:36 ` Pali Rohár @ 2021-08-15 21:55 ` Marc Zyngier 2021-08-15 23:10 ` Pali Rohár 0 siblings, 1 reply; 13+ messages in thread From: Marc Zyngier @ 2021-08-15 21:55 UTC (permalink / raw) To: Pali Rohár Cc: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, Rob Herring, Krzysztof Wilczyński, Marek Behún, linux-pci, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts 2021-08-15 21:55 ` Marc Zyngier @ 2021-08-15 23:10 ` Pali Rohár 0 siblings, 0 replies; 13+ messages in thread From: Pali Rohár @ 2021-08-15 23:10 UTC (permalink / raw) To: Marc Zyngier Cc: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, Rob Herring, Krzysztof Wilczyński, Marek Behún, linux-pci, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH 3/3] PCI: aardvark: Enable MSI-X support 2021-08-15 10:36 [PATCH 0/3] PCI: aardvark: MSI interrupt fixes Pali Rohár 2021-08-15 10:36 ` [PATCH 1/3] PCI: aardvark: Fix reading MSI interrupt number Pali Rohár 2021-08-15 10:36 ` [PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts Pali Rohár @ 2021-08-15 10:36 ` Pali Rohár 2021-08-23 16:40 ` [PATCH v2 0/3] PCI: aardvark: MSI interrupt fixes Pali Rohár 3 siblings, 0 replies; 13+ messages in thread From: Pali Rohár @ 2021-08-15 10:36 UTC (permalink / raw) To: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, Rob Herring Cc: Krzysztof Wilczyński, Marek Behún, Marc Zyngier, linux-pci, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 0/3] PCI: aardvark: MSI interrupt fixes 2021-08-15 10:36 [PATCH 0/3] PCI: aardvark: MSI interrupt fixes Pali Rohár ` (2 preceding siblings ...) 2021-08-15 10:36 ` [PATCH 3/3] PCI: aardvark: Enable MSI-X support Pali Rohár @ 2021-08-23 16:40 ` Pali Rohár 2021-08-23 16:40 ` [PATCH v2 1/3] PCI: aardvark: Fix reading MSI interrupt number Pali Rohár ` (2 more replies) 3 siblings, 3 replies; 13+ messages in thread From: Pali Rohár @ 2021-08-23 16:40 UTC (permalink / raw) To: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, Rob Herring Cc: Krzysztof Wilczyński, Marek Behún, Marc Zyngier, linux-pci, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 1/3] PCI: aardvark: Fix reading MSI interrupt number 2021-08-23 16:40 ` [PATCH v2 0/3] PCI: aardvark: MSI interrupt fixes Pali Rohár @ 2021-08-23 16:40 ` Pali Rohár 2021-08-23 16:52 ` Marc Zyngier 2021-08-23 16:40 ` [PATCH v2 2/3] PCI: aardvark: Fix masking MSI interrupts Pali Rohár 2021-08-23 16:40 ` [PATCH v2 3/3] PCI: aardvark: Enable MSI-X support Pali Rohár 2 siblings, 1 reply; 13+ messages in thread From: Pali Rohár @ 2021-08-23 16:40 UTC (permalink / raw) To: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, Rob Herring Cc: Krzysztof Wilczyński, Marek Behún, Marc Zyngier, linux-pci, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH v2 1/3] PCI: aardvark: Fix reading MSI interrupt number 2021-08-23 16:40 ` [PATCH v2 1/3] PCI: aardvark: Fix reading MSI interrupt number Pali Rohár @ 2021-08-23 16:52 ` Marc Zyngier 0 siblings, 0 replies; 13+ messages in thread From: Marc Zyngier @ 2021-08-23 16:52 UTC (permalink / raw) To: Pali Rohár Cc: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, Rob Herring, Krzysztof Wilczyński, Marek Behún, linux-pci, linux-kernel 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 2/3] PCI: aardvark: Fix masking MSI interrupts 2021-08-23 16:40 ` [PATCH v2 0/3] PCI: aardvark: MSI interrupt fixes Pali Rohár 2021-08-23 16:40 ` [PATCH v2 1/3] PCI: aardvark: Fix reading MSI interrupt number Pali Rohár @ 2021-08-23 16:40 ` Pali Rohár 2021-08-23 16:40 ` [PATCH v2 3/3] PCI: aardvark: Enable MSI-X support Pali Rohár 2 siblings, 0 replies; 13+ messages in thread From: Pali Rohár @ 2021-08-23 16:40 UTC (permalink / raw) To: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, Rob Herring Cc: Krzysztof Wilczyński, Marek Behún, Marc Zyngier, linux-pci, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 3/3] PCI: aardvark: Enable MSI-X support 2021-08-23 16:40 ` [PATCH v2 0/3] PCI: aardvark: MSI interrupt fixes Pali Rohár 2021-08-23 16:40 ` [PATCH v2 1/3] PCI: aardvark: Fix reading MSI interrupt number Pali Rohár 2021-08-23 16:40 ` [PATCH v2 2/3] PCI: aardvark: Fix masking MSI interrupts Pali Rohár @ 2021-08-23 16:40 ` Pali Rohár 2 siblings, 0 replies; 13+ messages in thread From: Pali Rohár @ 2021-08-23 16:40 UTC (permalink / raw) To: Lorenzo Pieralisi, Thomas Petazzoni, Bjorn Helgaas, Rob Herring Cc: Krzysztof Wilczyński, Marek Behún, Marc Zyngier, linux-pci, linux-kernel 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-08-23 16:52 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-15 10:36 [PATCH 0/3] PCI: aardvark: MSI interrupt fixes Pali Rohár 2021-08-15 10:36 ` [PATCH 1/3] PCI: aardvark: Fix reading MSI interrupt number Pali Rohár 2021-08-15 10:36 ` [PATCH 2/3] PCI: aardvark: Fix masking MSI interrupts Pali Rohár 2021-08-15 16:56 ` Marc Zyngier 2021-08-15 17:36 ` Pali Rohár 2021-08-15 21:55 ` Marc Zyngier 2021-08-15 23:10 ` Pali Rohár 2021-08-15 10:36 ` [PATCH 3/3] PCI: aardvark: Enable MSI-X support Pali Rohár 2021-08-23 16:40 ` [PATCH v2 0/3] PCI: aardvark: MSI interrupt fixes Pali Rohár 2021-08-23 16:40 ` [PATCH v2 1/3] PCI: aardvark: Fix reading MSI interrupt number Pali Rohár 2021-08-23 16:52 ` Marc Zyngier 2021-08-23 16:40 ` [PATCH v2 2/3] PCI: aardvark: Fix masking MSI interrupts Pali Rohár 2021-08-23 16:40 ` [PATCH v2 3/3] PCI: aardvark: Enable MSI-X support Pali Rohár
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).