Re: [PATCH] PCI: pcie-xilinx-dma-pl: Fix off-by-one INTx IRQ handling

[Sean Anderson <sean.anderson@linux.dev>]

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;
>>>>   }
>>>>   
>