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

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

[Stefan Roese]

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

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

[Sean Anderson]

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

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

[Sean Anderson]

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

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

[Stefan Roese]

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

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

[Sean Anderson]

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

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

[Stefan Roese]

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