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

[Stefan Roese <stefan.roese@mailbox.org>]

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