* [PATCH] PCI: pcie-xilinx-dma-pl: Fix off-by-one INTx IRQ handling @ 2025-10-21 13:39 Stefan Roese 2025-10-21 14:59 ` Sean Anderson 0 siblings, 1 reply; 6+ messages in thread From: Stefan Roese @ 2025-10-21 13:39 UTC (permalink / raw) To: linux-pci Cc: Sean Anderson, Manivannan Sadhasivam, Ravi Kumar Bandi, Thippeswamy Havalige, Michal Simek, Bjorn Helgaas While testing with NVMe drives connected to the Versal QDMA PL PCIe RP on our platform I noticed that with MSI disabled (e.g. via pci=nomsi) the NVMe interrupts are not delivered to the host CPU resulting in timeouts while probing. Debugging has shown, that the hwirq numbers passed to this device driver (1...4, 1=INTA etc) need to get adjusted to match the numbers in the controller registers bits (0...3). This patch now correctly matches the hwirq number to the PCIe controller register bits. Signed-off-by: Stefan Roese <stefan.roese@mailbox.org> Cc: Sean Anderson <sean.anderson@linux.dev> Cc: Manivannan Sadhasivam <mani@kernel.org> Cc: Ravi Kumar Bandi <ravib@amazon.com> Cc: Thippeswamy Havalige <thippeswamy.havalige@amd.com> Cc: Michal Simek <michal.simek@amd.com> Cc: Bjorn Helgaas <bhelgaas@google.com> --- drivers/pci/controller/pcie-xilinx-dma-pl.c | 21 ++++++++++++++++++--- 1 file changed, 18 insertions(+), 3 deletions(-) diff --git a/drivers/pci/controller/pcie-xilinx-dma-pl.c b/drivers/pci/controller/pcie-xilinx-dma-pl.c index 84888eda990b2..5cca9d018bc89 100644 --- a/drivers/pci/controller/pcie-xilinx-dma-pl.c +++ b/drivers/pci/controller/pcie-xilinx-dma-pl.c @@ -331,7 +331,12 @@ static void xilinx_mask_intx_irq(struct irq_data *data) unsigned long flags; u32 mask, val; - mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT); + /* + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD + * In the controller regs this is represented in bits 0...3, so we need + * to subtract 1 here + */ + mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT - 1); raw_spin_lock_irqsave(&port->lock, flags); val = pcie_read(port, XILINX_PCIE_DMA_REG_IDRN_MASK); pcie_write(port, (val & (~mask)), XILINX_PCIE_DMA_REG_IDRN_MASK); @@ -344,7 +349,12 @@ static void xilinx_unmask_intx_irq(struct irq_data *data) unsigned long flags; u32 mask, val; - mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT); + /* + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD + * In the controller regs this is represented in bits 0...3, so we need + * to subtract 1 here + */ + mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT - 1); raw_spin_lock_irqsave(&port->lock, flags); val = pcie_read(port, XILINX_PCIE_DMA_REG_IDRN_MASK); pcie_write(port, (val | mask), XILINX_PCIE_DMA_REG_IDRN_MASK); @@ -620,8 +630,13 @@ static irqreturn_t xilinx_pl_dma_pcie_intx_flow(int irq, void *args) val = FIELD_GET(XILINX_PCIE_DMA_IDRN_MASK, pcie_read(port, XILINX_PCIE_DMA_REG_IDRN)); + /* + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD + * In the controller regs this is represented in bits 0...3, so we need + * to add 1 here again for the registered handler + */ for_each_set_bit(i, &val, PCI_NUM_INTX) - generic_handle_domain_irq(port->intx_domain, i); + generic_handle_domain_irq(port->intx_domain, i + 1); return IRQ_HANDLED; } -- 2.51.1 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: pcie-xilinx-dma-pl: Fix off-by-one INTx IRQ handling 2025-10-21 13:39 [PATCH] PCI: pcie-xilinx-dma-pl: Fix off-by-one INTx IRQ handling Stefan Roese @ 2025-10-21 14:59 ` Sean Anderson 2025-10-21 15:04 ` Sean Anderson 0 siblings, 1 reply; 6+ messages in thread From: Sean Anderson @ 2025-10-21 14:59 UTC (permalink / raw) To: Stefan Roese, linux-pci Cc: Manivannan Sadhasivam, Ravi Kumar Bandi, Thippeswamy Havalige, Michal Simek, Bjorn Helgaas On 10/21/25 09:39, Stefan Roese wrote: > While testing with NVMe drives connected to the Versal QDMA PL PCIe RP > on our platform I noticed that with MSI disabled (e.g. via pci=nomsi) > the NVMe interrupts are not delivered to the host CPU resulting in > timeouts while probing. > > Debugging has shown, that the hwirq numbers passed to this device driver > (1...4, 1=INTA etc) need to get adjusted to match the numbers in the > controller registers bits (0...3). This patch now correctly matches the > hwirq number to the PCIe controller register bits. > > Signed-off-by: Stefan Roese <stefan.roese@mailbox.org> > Cc: Sean Anderson <sean.anderson@linux.dev> I assume I'm CC'd because of commit 02a370c4fc0f ("PCI: xilinx-nwl: Fix off-by-one in INTx IRQ handler"), which does the exact opposite of this patch. And I'm rather confused as to how you determined that hwirq is one-based, since it seems to come directly from for_each_set_bit(i, &val, PCI_NUM_INTX) generic_handle_domain_irq(port->intx_domain, i); which is zero-based. --Sean > Cc: Manivannan Sadhasivam <mani@kernel.org> > Cc: Ravi Kumar Bandi <ravib@amazon.com> > Cc: Thippeswamy Havalige <thippeswamy.havalige@amd.com> > Cc: Michal Simek <michal.simek@amd.com> > Cc: Bjorn Helgaas <bhelgaas@google.com> > --- > drivers/pci/controller/pcie-xilinx-dma-pl.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/drivers/pci/controller/pcie-xilinx-dma-pl.c b/drivers/pci/controller/pcie-xilinx-dma-pl.c > index 84888eda990b2..5cca9d018bc89 100644 > --- a/drivers/pci/controller/pcie-xilinx-dma-pl.c > +++ b/drivers/pci/controller/pcie-xilinx-dma-pl.c > @@ -331,7 +331,12 @@ static void xilinx_mask_intx_irq(struct irq_data *data) > unsigned long flags; > u32 mask, val; > > - mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT); > + /* > + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD > + * In the controller regs this is represented in bits 0...3, so we need > + * to subtract 1 here > + */ > + mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT - 1); > raw_spin_lock_irqsave(&port->lock, flags); > val = pcie_read(port, XILINX_PCIE_DMA_REG_IDRN_MASK); > pcie_write(port, (val & (~mask)), XILINX_PCIE_DMA_REG_IDRN_MASK); > @@ -344,7 +349,12 @@ static void xilinx_unmask_intx_irq(struct irq_data *data) > unsigned long flags; > u32 mask, val; > > - mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT); > + /* > + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD > + * In the controller regs this is represented in bits 0...3, so we need > + * to subtract 1 here > + */ > + mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT - 1); > raw_spin_lock_irqsave(&port->lock, flags); > val = pcie_read(port, XILINX_PCIE_DMA_REG_IDRN_MASK); > pcie_write(port, (val | mask), XILINX_PCIE_DMA_REG_IDRN_MASK); > @@ -620,8 +630,13 @@ static irqreturn_t xilinx_pl_dma_pcie_intx_flow(int irq, void *args) > val = FIELD_GET(XILINX_PCIE_DMA_IDRN_MASK, > pcie_read(port, XILINX_PCIE_DMA_REG_IDRN)); > > + /* > + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD > + * In the controller regs this is represented in bits 0...3, so we need > + * to add 1 here again for the registered handler > + */ > for_each_set_bit(i, &val, PCI_NUM_INTX) > - generic_handle_domain_irq(port->intx_domain, i); > + generic_handle_domain_irq(port->intx_domain, i + 1); > return IRQ_HANDLED; > } > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: pcie-xilinx-dma-pl: Fix off-by-one INTx IRQ handling 2025-10-21 14:59 ` Sean Anderson @ 2025-10-21 15:04 ` Sean Anderson 2025-10-21 15:10 ` Stefan Roese 0 siblings, 1 reply; 6+ messages in thread From: Sean Anderson @ 2025-10-21 15:04 UTC (permalink / raw) To: Stefan Roese, linux-pci Cc: Manivannan Sadhasivam, Ravi Kumar Bandi, Thippeswamy Havalige, Michal Simek, Bjorn Helgaas On 10/21/25 10:59, Sean Anderson wrote: > On 10/21/25 09:39, Stefan Roese wrote: >> While testing with NVMe drives connected to the Versal QDMA PL PCIe RP >> on our platform I noticed that with MSI disabled (e.g. via pci=nomsi) >> the NVMe interrupts are not delivered to the host CPU resulting in >> timeouts while probing. >> >> Debugging has shown, that the hwirq numbers passed to this device driver >> (1...4, 1=INTA etc) need to get adjusted to match the numbers in the >> controller registers bits (0...3). This patch now correctly matches the >> hwirq number to the PCIe controller register bits. >> >> Signed-off-by: Stefan Roese <stefan.roese@mailbox.org> >> Cc: Sean Anderson <sean.anderson@linux.dev> > > I assume I'm CC'd because of commit 02a370c4fc0f ("PCI: xilinx-nwl: Fix > off-by-one in INTx IRQ handler"), which does the exact opposite of this > patch. > > And I'm rather confused as to how you determined that hwirq is > one-based, since it seems to come directly from > > for_each_set_bit(i, &val, PCI_NUM_INTX) > generic_handle_domain_irq(port->intx_domain, i); > > which is zero-based. OK, upon re-reading the patch it looks like you also adjusted the above hunk to be one-based. After this patch the drivers adds one just to subtract it off again. So why do the interrupts need to be one-based and not zero-based? --Sean > >> Cc: Manivannan Sadhasivam <mani@kernel.org> >> Cc: Ravi Kumar Bandi <ravib@amazon.com> >> Cc: Thippeswamy Havalige <thippeswamy.havalige@amd.com> >> Cc: Michal Simek <michal.simek@amd.com> >> Cc: Bjorn Helgaas <bhelgaas@google.com> >> --- >> drivers/pci/controller/pcie-xilinx-dma-pl.c | 21 ++++++++++++++++++--- >> 1 file changed, 18 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/pci/controller/pcie-xilinx-dma-pl.c b/drivers/pci/controller/pcie-xilinx-dma-pl.c >> index 84888eda990b2..5cca9d018bc89 100644 >> --- a/drivers/pci/controller/pcie-xilinx-dma-pl.c >> +++ b/drivers/pci/controller/pcie-xilinx-dma-pl.c >> @@ -331,7 +331,12 @@ static void xilinx_mask_intx_irq(struct irq_data *data) >> unsigned long flags; >> u32 mask, val; >> >> - mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT); >> + /* >> + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD >> + * In the controller regs this is represented in bits 0...3, so we need >> + * to subtract 1 here >> + */ >> + mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT - 1); >> raw_spin_lock_irqsave(&port->lock, flags); >> val = pcie_read(port, XILINX_PCIE_DMA_REG_IDRN_MASK); >> pcie_write(port, (val & (~mask)), XILINX_PCIE_DMA_REG_IDRN_MASK); >> @@ -344,7 +349,12 @@ static void xilinx_unmask_intx_irq(struct irq_data *data) >> unsigned long flags; >> u32 mask, val; >> >> - mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT); >> + /* >> + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD >> + * In the controller regs this is represented in bits 0...3, so we need >> + * to subtract 1 here >> + */ >> + mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT - 1); >> raw_spin_lock_irqsave(&port->lock, flags); >> val = pcie_read(port, XILINX_PCIE_DMA_REG_IDRN_MASK); >> pcie_write(port, (val | mask), XILINX_PCIE_DMA_REG_IDRN_MASK); >> @@ -620,8 +630,13 @@ static irqreturn_t xilinx_pl_dma_pcie_intx_flow(int irq, void *args) >> val = FIELD_GET(XILINX_PCIE_DMA_IDRN_MASK, >> pcie_read(port, XILINX_PCIE_DMA_REG_IDRN)); >> >> + /* >> + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD >> + * In the controller regs this is represented in bits 0...3, so we need >> + * to add 1 here again for the registered handler >> + */ >> for_each_set_bit(i, &val, PCI_NUM_INTX) >> - generic_handle_domain_irq(port->intx_domain, i); >> + generic_handle_domain_irq(port->intx_domain, i + 1); >> return IRQ_HANDLED; >> } >> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: pcie-xilinx-dma-pl: Fix off-by-one INTx IRQ handling 2025-10-21 15:04 ` Sean Anderson @ 2025-10-21 15:10 ` Stefan Roese 2025-10-21 15:12 ` Sean Anderson 0 siblings, 1 reply; 6+ messages in thread From: Stefan Roese @ 2025-10-21 15:10 UTC (permalink / raw) To: Sean Anderson, linux-pci Cc: Manivannan Sadhasivam, Ravi Kumar Bandi, Thippeswamy Havalige, Michal Simek, Bjorn Helgaas Hi Sean, On 10/21/25 17:04, Sean Anderson wrote: > On 10/21/25 10:59, Sean Anderson wrote: >> On 10/21/25 09:39, Stefan Roese wrote: >>> While testing with NVMe drives connected to the Versal QDMA PL PCIe RP >>> on our platform I noticed that with MSI disabled (e.g. via pci=nomsi) >>> the NVMe interrupts are not delivered to the host CPU resulting in >>> timeouts while probing. >>> >>> Debugging has shown, that the hwirq numbers passed to this device driver >>> (1...4, 1=INTA etc) need to get adjusted to match the numbers in the >>> controller registers bits (0...3). This patch now correctly matches the >>> hwirq number to the PCIe controller register bits. >>> >>> Signed-off-by: Stefan Roese <stefan.roese@mailbox.org> >>> Cc: Sean Anderson <sean.anderson@linux.dev> >> >> I assume I'm CC'd because of commit 02a370c4fc0f ("PCI: xilinx-nwl: Fix >> off-by-one in INTx IRQ handler"), which does the exact opposite of this >> patch. Correct. PCI legacy interrupts are rarely used as it seems lately. >> And I'm rather confused as to how you determined that hwirq is >> one-based, since it seems to come directly from >> >> for_each_set_bit(i, &val, PCI_NUM_INTX) >> generic_handle_domain_irq(port->intx_domain, i); >> >> which is zero-based. > > OK, upon re-reading the patch it looks like you also adjusted the above > hunk to be one-based. After this patch the drivers adds one just to > subtract it off again. > > So why do the interrupts need to be one-based and not zero-based? My testing / debugging showed, that hwirq as an input to these functions is always 1 in my case. My assumption was, that this is because of these defines in include/linux/pci.h: /** * enum pci_interrupt_pin - PCI INTx interrupt values * @PCI_INTERRUPT_UNKNOWN: Unknown or unassigned interrupt * @PCI_INTERRUPT_INTA: PCI INTA pin * @PCI_INTERRUPT_INTB: PCI INTB pin * @PCI_INTERRUPT_INTC: PCI INTC pin * @PCI_INTERRUPT_INTD: PCI INTD pin * * Corresponds to values for legacy PCI INTx interrupts, as can be found in the * PCI_INTERRUPT_PIN register. */ enum pci_interrupt_pin { PCI_INTERRUPT_UNKNOWN, PCI_INTERRUPT_INTA, PCI_INTERRUPT_INTB, PCI_INTERRUPT_INTC, PCI_INTERRUPT_INTD, }; So 0 is unknown and 1 is INTA. I might have gotten something totally wrong this patch with these changes enables PCIe INTx generation on my Versal platform. Thanks, Stefan > --Sean > >> >>> Cc: Manivannan Sadhasivam <mani@kernel.org> >>> Cc: Ravi Kumar Bandi <ravib@amazon.com> >>> Cc: Thippeswamy Havalige <thippeswamy.havalige@amd.com> >>> Cc: Michal Simek <michal.simek@amd.com> >>> Cc: Bjorn Helgaas <bhelgaas@google.com> >>> --- >>> drivers/pci/controller/pcie-xilinx-dma-pl.c | 21 ++++++++++++++++++--- >>> 1 file changed, 18 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/pci/controller/pcie-xilinx-dma-pl.c b/drivers/pci/controller/pcie-xilinx-dma-pl.c >>> index 84888eda990b2..5cca9d018bc89 100644 >>> --- a/drivers/pci/controller/pcie-xilinx-dma-pl.c >>> +++ b/drivers/pci/controller/pcie-xilinx-dma-pl.c >>> @@ -331,7 +331,12 @@ static void xilinx_mask_intx_irq(struct irq_data *data) >>> unsigned long flags; >>> u32 mask, val; >>> >>> - mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT); >>> + /* >>> + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD >>> + * In the controller regs this is represented in bits 0...3, so we need >>> + * to subtract 1 here >>> + */ >>> + mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT - 1); >>> raw_spin_lock_irqsave(&port->lock, flags); >>> val = pcie_read(port, XILINX_PCIE_DMA_REG_IDRN_MASK); >>> pcie_write(port, (val & (~mask)), XILINX_PCIE_DMA_REG_IDRN_MASK); >>> @@ -344,7 +349,12 @@ static void xilinx_unmask_intx_irq(struct irq_data *data) >>> unsigned long flags; >>> u32 mask, val; >>> >>> - mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT); >>> + /* >>> + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD >>> + * In the controller regs this is represented in bits 0...3, so we need >>> + * to subtract 1 here >>> + */ >>> + mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT - 1); >>> raw_spin_lock_irqsave(&port->lock, flags); >>> val = pcie_read(port, XILINX_PCIE_DMA_REG_IDRN_MASK); >>> pcie_write(port, (val | mask), XILINX_PCIE_DMA_REG_IDRN_MASK); >>> @@ -620,8 +630,13 @@ static irqreturn_t xilinx_pl_dma_pcie_intx_flow(int irq, void *args) >>> val = FIELD_GET(XILINX_PCIE_DMA_IDRN_MASK, >>> pcie_read(port, XILINX_PCIE_DMA_REG_IDRN)); >>> >>> + /* >>> + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD >>> + * In the controller regs this is represented in bits 0...3, so we need >>> + * to add 1 here again for the registered handler >>> + */ >>> for_each_set_bit(i, &val, PCI_NUM_INTX) >>> - generic_handle_domain_irq(port->intx_domain, i); >>> + generic_handle_domain_irq(port->intx_domain, i + 1); >>> return IRQ_HANDLED; >>> } >>> ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: pcie-xilinx-dma-pl: Fix off-by-one INTx IRQ handling 2025-10-21 15:10 ` Stefan Roese @ 2025-10-21 15:12 ` Sean Anderson 2025-10-21 15:24 ` Stefan Roese 0 siblings, 1 reply; 6+ messages in thread From: Sean Anderson @ 2025-10-21 15:12 UTC (permalink / raw) To: Stefan Roese, linux-pci Cc: Manivannan Sadhasivam, Ravi Kumar Bandi, Thippeswamy Havalige, Michal Simek, Bjorn Helgaas On 10/21/25 11:10, Stefan Roese wrote: > Hi Sean, > > On 10/21/25 17:04, Sean Anderson wrote: >> On 10/21/25 10:59, Sean Anderson wrote: >>> On 10/21/25 09:39, Stefan Roese wrote: >>>> While testing with NVMe drives connected to the Versal QDMA PL PCIe RP >>>> on our platform I noticed that with MSI disabled (e.g. via pci=nomsi) >>>> the NVMe interrupts are not delivered to the host CPU resulting in >>>> timeouts while probing. >>>> >>>> Debugging has shown, that the hwirq numbers passed to this device driver >>>> (1...4, 1=INTA etc) need to get adjusted to match the numbers in the >>>> controller registers bits (0...3). This patch now correctly matches the >>>> hwirq number to the PCIe controller register bits. >>>> >>>> Signed-off-by: Stefan Roese <stefan.roese@mailbox.org> >>>> Cc: Sean Anderson <sean.anderson@linux.dev> >>> >>> I assume I'm CC'd because of commit 02a370c4fc0f ("PCI: xilinx-nwl: Fix >>> off-by-one in INTx IRQ handler"), which does the exact opposite of this >>> patch. > > Correct. PCI legacy interrupts are rarely used as it seems lately. > >>> And I'm rather confused as to how you determined that hwirq is >>> one-based, since it seems to come directly from >>> >>> for_each_set_bit(i, &val, PCI_NUM_INTX) >>> generic_handle_domain_irq(port->intx_domain, i); >>> >>> which is zero-based. >> >> OK, upon re-reading the patch it looks like you also adjusted the above >> hunk to be one-based. After this patch the drivers adds one just to >> subtract it off again. >> >> So why do the interrupts need to be one-based and not zero-based? > > My testing / debugging showed, that hwirq as an input to these > functions is always 1 in my case. My assumption was, that this is > because of these defines in include/linux/pci.h: > > /** > * enum pci_interrupt_pin - PCI INTx interrupt values > * @PCI_INTERRUPT_UNKNOWN: Unknown or unassigned interrupt > * @PCI_INTERRUPT_INTA: PCI INTA pin > * @PCI_INTERRUPT_INTB: PCI INTB pin > * @PCI_INTERRUPT_INTC: PCI INTC pin > * @PCI_INTERRUPT_INTD: PCI INTD pin > * > * Corresponds to values for legacy PCI INTx interrupts, as can be found in the > * PCI_INTERRUPT_PIN register. > */ > enum pci_interrupt_pin { > PCI_INTERRUPT_UNKNOWN, > PCI_INTERRUPT_INTA, > PCI_INTERRUPT_INTB, > PCI_INTERRUPT_INTC, > PCI_INTERRUPT_INTD, > }; > > So 0 is unknown and 1 is INTA. > > I might have gotten something totally wrong this patch with these > changes enables PCIe INTx generation on my Versal platform. > Maybe you need to set intx_domain_ops.xlate to pci_irqd_intx_xlate? --Sean >> >>> >>>> Cc: Manivannan Sadhasivam <mani@kernel.org> >>>> Cc: Ravi Kumar Bandi <ravib@amazon.com> >>>> Cc: Thippeswamy Havalige <thippeswamy.havalige@amd.com> >>>> Cc: Michal Simek <michal.simek@amd.com> >>>> Cc: Bjorn Helgaas <bhelgaas@google.com> >>>> --- >>>> drivers/pci/controller/pcie-xilinx-dma-pl.c | 21 ++++++++++++++++++--- >>>> 1 file changed, 18 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/pci/controller/pcie-xilinx-dma-pl.c b/drivers/pci/controller/pcie-xilinx-dma-pl.c >>>> index 84888eda990b2..5cca9d018bc89 100644 >>>> --- a/drivers/pci/controller/pcie-xilinx-dma-pl.c >>>> +++ b/drivers/pci/controller/pcie-xilinx-dma-pl.c >>>> @@ -331,7 +331,12 @@ static void xilinx_mask_intx_irq(struct irq_data *data) >>>> unsigned long flags; >>>> u32 mask, val; >>>> - mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT); >>>> + /* >>>> + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD >>>> + * In the controller regs this is represented in bits 0...3, so we need >>>> + * to subtract 1 here >>>> + */ >>>> + mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT - 1); >>>> raw_spin_lock_irqsave(&port->lock, flags); >>>> val = pcie_read(port, XILINX_PCIE_DMA_REG_IDRN_MASK); >>>> pcie_write(port, (val & (~mask)), XILINX_PCIE_DMA_REG_IDRN_MASK); >>>> @@ -344,7 +349,12 @@ static void xilinx_unmask_intx_irq(struct irq_data *data) >>>> unsigned long flags; >>>> u32 mask, val; >>>> - mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT); >>>> + /* >>>> + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD >>>> + * In the controller regs this is represented in bits 0...3, so we need >>>> + * to subtract 1 here >>>> + */ >>>> + mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT - 1); >>>> raw_spin_lock_irqsave(&port->lock, flags); >>>> val = pcie_read(port, XILINX_PCIE_DMA_REG_IDRN_MASK); >>>> pcie_write(port, (val | mask), XILINX_PCIE_DMA_REG_IDRN_MASK); >>>> @@ -620,8 +630,13 @@ static irqreturn_t xilinx_pl_dma_pcie_intx_flow(int irq, void *args) >>>> val = FIELD_GET(XILINX_PCIE_DMA_IDRN_MASK, >>>> pcie_read(port, XILINX_PCIE_DMA_REG_IDRN)); >>>> + /* >>>> + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD >>>> + * In the controller regs this is represented in bits 0...3, so we need >>>> + * to add 1 here again for the registered handler >>>> + */ >>>> for_each_set_bit(i, &val, PCI_NUM_INTX) >>>> - generic_handle_domain_irq(port->intx_domain, i); >>>> + generic_handle_domain_irq(port->intx_domain, i + 1); >>>> return IRQ_HANDLED; >>>> } >>>> > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] PCI: pcie-xilinx-dma-pl: Fix off-by-one INTx IRQ handling 2025-10-21 15:12 ` Sean Anderson @ 2025-10-21 15:24 ` Stefan Roese 0 siblings, 0 replies; 6+ messages in thread From: Stefan Roese @ 2025-10-21 15:24 UTC (permalink / raw) To: Sean Anderson, linux-pci Cc: Manivannan Sadhasivam, Ravi Kumar Bandi, Thippeswamy Havalige, Michal Simek, Bjorn Helgaas Hi Sean, On 10/21/25 17:12, Sean Anderson wrote: > On 10/21/25 11:10, Stefan Roese wrote: >> Hi Sean, >> >> On 10/21/25 17:04, Sean Anderson wrote: >>> On 10/21/25 10:59, Sean Anderson wrote: >>>> On 10/21/25 09:39, Stefan Roese wrote: >>>>> While testing with NVMe drives connected to the Versal QDMA PL PCIe RP >>>>> on our platform I noticed that with MSI disabled (e.g. via pci=nomsi) >>>>> the NVMe interrupts are not delivered to the host CPU resulting in >>>>> timeouts while probing. >>>>> >>>>> Debugging has shown, that the hwirq numbers passed to this device driver >>>>> (1...4, 1=INTA etc) need to get adjusted to match the numbers in the >>>>> controller registers bits (0...3). This patch now correctly matches the >>>>> hwirq number to the PCIe controller register bits. >>>>> >>>>> Signed-off-by: Stefan Roese <stefan.roese@mailbox.org> >>>>> Cc: Sean Anderson <sean.anderson@linux.dev> >>>> >>>> I assume I'm CC'd because of commit 02a370c4fc0f ("PCI: xilinx-nwl: Fix >>>> off-by-one in INTx IRQ handler"), which does the exact opposite of this >>>> patch. >> >> Correct. PCI legacy interrupts are rarely used as it seems lately. >> >>>> And I'm rather confused as to how you determined that hwirq is >>>> one-based, since it seems to come directly from >>>> >>>> for_each_set_bit(i, &val, PCI_NUM_INTX) >>>> generic_handle_domain_irq(port->intx_domain, i); >>>> >>>> which is zero-based. >>> >>> OK, upon re-reading the patch it looks like you also adjusted the above >>> hunk to be one-based. After this patch the drivers adds one just to >>> subtract it off again. >>> >>> So why do the interrupts need to be one-based and not zero-based? >> >> My testing / debugging showed, that hwirq as an input to these >> functions is always 1 in my case. My assumption was, that this is >> because of these defines in include/linux/pci.h: >> >> /** >> * enum pci_interrupt_pin - PCI INTx interrupt values >> * @PCI_INTERRUPT_UNKNOWN: Unknown or unassigned interrupt >> * @PCI_INTERRUPT_INTA: PCI INTA pin >> * @PCI_INTERRUPT_INTB: PCI INTB pin >> * @PCI_INTERRUPT_INTC: PCI INTC pin >> * @PCI_INTERRUPT_INTD: PCI INTD pin >> * >> * Corresponds to values for legacy PCI INTx interrupts, as can be found in the >> * PCI_INTERRUPT_PIN register. >> */ >> enum pci_interrupt_pin { >> PCI_INTERRUPT_UNKNOWN, >> PCI_INTERRUPT_INTA, >> PCI_INTERRUPT_INTB, >> PCI_INTERRUPT_INTC, >> PCI_INTERRUPT_INTD, >> }; >> >> So 0 is unknown and 1 is INTA. >> >> I might have gotten something totally wrong this patch with these >> changes enables PCIe INTx generation on my Versal platform. >> > > Maybe you need to set intx_domain_ops.xlate to pci_irqd_intx_xlate? Ah, much better. I was not aware of this. Will send v2 using this xlate function instead. Thanks a lot, Stefan > --Sean > >>> >>>> >>>>> Cc: Manivannan Sadhasivam <mani@kernel.org> >>>>> Cc: Ravi Kumar Bandi <ravib@amazon.com> >>>>> Cc: Thippeswamy Havalige <thippeswamy.havalige@amd.com> >>>>> Cc: Michal Simek <michal.simek@amd.com> >>>>> Cc: Bjorn Helgaas <bhelgaas@google.com> >>>>> --- >>>>> drivers/pci/controller/pcie-xilinx-dma-pl.c | 21 ++++++++++++++++++--- >>>>> 1 file changed, 18 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/pci/controller/pcie-xilinx-dma-pl.c b/drivers/pci/controller/pcie-xilinx-dma-pl.c >>>>> index 84888eda990b2..5cca9d018bc89 100644 >>>>> --- a/drivers/pci/controller/pcie-xilinx-dma-pl.c >>>>> +++ b/drivers/pci/controller/pcie-xilinx-dma-pl.c >>>>> @@ -331,7 +331,12 @@ static void xilinx_mask_intx_irq(struct irq_data *data) >>>>> unsigned long flags; >>>>> u32 mask, val; >>>>> - mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT); >>>>> + /* >>>>> + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD >>>>> + * In the controller regs this is represented in bits 0...3, so we need >>>>> + * to subtract 1 here >>>>> + */ >>>>> + mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT - 1); >>>>> raw_spin_lock_irqsave(&port->lock, flags); >>>>> val = pcie_read(port, XILINX_PCIE_DMA_REG_IDRN_MASK); >>>>> pcie_write(port, (val & (~mask)), XILINX_PCIE_DMA_REG_IDRN_MASK); >>>>> @@ -344,7 +349,12 @@ static void xilinx_unmask_intx_irq(struct irq_data *data) >>>>> unsigned long flags; >>>>> u32 mask, val; >>>>> - mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT); >>>>> + /* >>>>> + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD >>>>> + * In the controller regs this is represented in bits 0...3, so we need >>>>> + * to subtract 1 here >>>>> + */ >>>>> + mask = BIT(data->hwirq + XILINX_PCIE_DMA_IDRN_SHIFT - 1); >>>>> raw_spin_lock_irqsave(&port->lock, flags); >>>>> val = pcie_read(port, XILINX_PCIE_DMA_REG_IDRN_MASK); >>>>> pcie_write(port, (val | mask), XILINX_PCIE_DMA_REG_IDRN_MASK); >>>>> @@ -620,8 +630,13 @@ static irqreturn_t xilinx_pl_dma_pcie_intx_flow(int irq, void *args) >>>>> val = FIELD_GET(XILINX_PCIE_DMA_IDRN_MASK, >>>>> pcie_read(port, XILINX_PCIE_DMA_REG_IDRN)); >>>>> + /* >>>>> + * INTx hwirq: 1=INTA, 2=INTB, 3=INTC, 4=INTD >>>>> + * In the controller regs this is represented in bits 0...3, so we need >>>>> + * to add 1 here again for the registered handler >>>>> + */ >>>>> for_each_set_bit(i, &val, PCI_NUM_INTX) >>>>> - generic_handle_domain_irq(port->intx_domain, i); >>>>> + generic_handle_domain_irq(port->intx_domain, i + 1); >>>>> return IRQ_HANDLED; >>>>> } >>>>> >> > Thanks a ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-10-21 15:25 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-10-21 13:39 [PATCH] PCI: pcie-xilinx-dma-pl: Fix off-by-one INTx IRQ handling Stefan Roese 2025-10-21 14:59 ` Sean Anderson 2025-10-21 15:04 ` Sean Anderson 2025-10-21 15:10 ` Stefan Roese 2025-10-21 15:12 ` Sean Anderson 2025-10-21 15:24 ` Stefan Roese
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox