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

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

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