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