Re: [PATCH V5 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver

[+cc Thomas in case he wants to comment on chained interrupts]

On Thu, Jul 20, 2023 at 06:37:03AM +0000, Havalige, Thippeswamy wrote:

quoted

From: Bjorn Helgaas <helgaas@kernel.org>
...
On Wed, Jun 28, 2023 at 02:58:12PM +0530, Thippeswamy Havalige wrote:

quoted

Add support for Xilinx XDMA Soft IP core as Root Port.
...

quoted

quoted

+ * struct pl_dma_pcie - PCIe port information
+ * @intx_domain: Legacy IRQ domain pointer
+ * @pldma_domain: PL DMA IRQ domain pointer
+ * @irq_misc: Legacy and error interrupt number
+ * @intx_irq: legacy interrupt number

quoted

"Legacy and error interrupt number" and "legacy interrupt number"
sound like they overlap -- "legacy interrupt number" is part of both.
Is that an error?

- Agreed, I'll modify this comment to legacy interrupt number. (This
irq line is for both legacy interrupts and error interrupt bits)

Does "legacy" mean "INTx" in this context?  If so, I'd use "INTx"
because it's much more specific.  "Legacy" really doesn't contain any
information other than "this is something retained for some kind of
backward compatibility."

If you have more detail about the "error interrupt," that would be
useful as well.  Does this refer to an AER interrupt, a "System
Error", something else?  I'm looking at the diagram in PCIe r6.0,
Figure 6-3, wondering if this is related to anything there.  I suppose
likely it's some Xilinx-specific thing?

quoted

quoted

+	/* Plug the INTx chained handler */
+	irq_set_chained_handler_and_data(port->intx_irq,
+					 xilinx_pl_dma_pcie_intx_flow, port);
+
+	/* Plug the main event chained handler */
+	irq_set_chained_handler_and_data(port->irq,
+					 xilinx_pl_dma_pcie_event_flow,

port);

What's the reason for using chained IRQs?  Can this be done
without them?  I don't claim to understand all the issues here,
but it seems better to avoid chained IRQ handlers when possible:
https://lore.kernel.org/all/877csohcll.ffs@tglx/ (local)

- As per the comments in this
https://lkml.kernel.org/lkml/alpine.DEB.2.20.1705232307330.2409@nanos/T/
"It is fine to have chained interrupts when bootloader, device tree
and kernel under control. Only if BIOS/UEFI comes into play the user
is helpless against interrupt storm which will cause system to
hangs."

We are using ARM embedded platform with Bootloader, Devicetree flow.

I read Thomas' comments as "in general it's better to use regular
interrupts, but we can live with chained interrupts if we have control
of bootloader, device tree, and kernel."

I guess my questions are more like:

  - Could this be done with either chained interrupts or regular
    interrupts?

  - If so, what is the advantage to using chained interrupts?

Across the entire kernel, irq_set_chained_handler_and_data() is
relatively unusual, which makes me think it may be better to use the
more common path if it's possible.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel