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

[Havalige, Thippeswamy]

Hi Bjorn,

> -----Original Message-----
> From: Bjorn Helgaas <helgaas@xxxxxxxxxx>
> Sent: Thursday, July 20, 2023 8:54 PM
> To: Havalige, Thippeswamy <thippeswamy.havalige@xxxxxxx>
> Cc: krzysztof.kozlowski@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> pci@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx;
> bhelgaas@xxxxxxxxxx; lorenzo.pieralisi@xxxxxxx; linux-arm-
> kernel@xxxxxxxxxxxxxxxxxxx; Gogada, Bharat Kumar
> <bharat.kumar.gogada@xxxxxxx>; Simek, Michal
> <michal.simek@xxxxxxx>; Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Subject: 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:
> > > From: Bjorn Helgaas <helgaas@xxxxxxxxxx> ...
> > > On Wed, Jun 28, 2023 at 02:58:12PM +0530, Thippeswamy Havalige wrote:
> > > > Add support for Xilinx XDMA Soft IP core as Root Port.
> > > > ...
> 
> > > > + * 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
> 
> > > "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?


- Agreed, ll modify Legacy to INTx, and regarding error interrupts these are Xilinx controller specific interrupts which are used to notify the user about errors such as cfg timeout, slave unsupported requests,Fatal and non fatal error.

> > > > +	/* 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/
> 
> > - 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?
With regular interrupts, these interrupts are self-consumed interrupts (interrupt is handled within driver) but where as chained interrupts are not self consumed (interrupts are not handled within the driver, but forwarded to different driver for which the actual interrupt is raised) but these interrupts are demultiplexed and forwards interrupt to another subsystem by calling generic_handle_irq(). 

As, MSI generic handlers are consumed by Endpoints and end point drivers, chained handlers forward the interrupt to the specific EP driver (For example NVME subsystem or any other subsystem).
> 
> 
>
> 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

Regards,
Thippeswamy H