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: Saturday, July 1, 2023 4:49 AM
> 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>
> Subject: Re: [PATCH V5 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver
> 
> On Wed, Jun 28, 2023 at 02:58:12PM +0530, Thippeswamy Havalige wrote:
> > Add support for Xilinx XDMA Soft IP core as Root Port.
> > ...
> 
> > |Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > |Reported-by: Dan Carpenter <error27@xxxxxxxxx>
> > |Closes:
> > |https://lore.kernel.org/r/202305261250.2cs1phTS-lkp@xxxxxxxxx/
> 
> Remove these.  I mentioned this before:
> https://lore.kernel.org/r/ZHd/7AaLaGyr1jNA@bhelgaas
- Agreed, I'll remove this in next patch
> > + * struct pl_dma_pcie - PCIe port information
> > + * @dev: Device pointer
> > + * @reg_base: IO Mapped Register Base
> > + * @irq: Interrupt number
> > + * @cfg: Holds mappings of config space window
> > + * @phys_reg_base: Physical address of reg base
> > + * @intx_domain: Legacy IRQ domain pointer
> > + * @pldma_domain: PL DMA IRQ domain pointer
> > + * @resources: Bus Resources
> > + * @msi: MSI information
> > + * @irq_misc: Legacy and error interrupt number
> > + * @intx_irq: legacy interrupt number
> > + * @lock: lock protecting shared register access
> 
> Capitalize the intx_irq and lock descriptions so they match the others.
- Agreed, I'll fix it in the next patch
> "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)
> > +static bool xilinx_pl_dma_pcie_valid_device(struct pci_bus *bus,
> > +unsigned int devfn) {
> > +	struct pl_dma_pcie *port = bus->sysdata;
> > +
> > +	/* Check if link is up when trying to access downstream ports */
> > +	if (!pci_is_root_bus(bus)) {
> > +		/*
> > +		 * If the link goes down after we check for link-up, we have a
> problem:
> > +		 * if a PIO request is initiated while link-down, the whole
> controller
> > +		 * hangs, and even after link comes up again, previous PIO
> requests
> > +		 * won't work, and a reset of the whole PCIe controller is
> needed.
> > +		 * Henceforth we need link-up check here to avoid sending
> PIO request
> > +		 * when link is down.
> 
> Wrap this comment so it fits in 80 columns like the rest of the file.
> 
> I think the comment was added because I pointed out that this is racy.
> Obviously the comment doesn't *fix* the race, and it actually doesn't even
> describe the race.
- Agreed, I'll add comments regarding race condition.
> Even with the xilinx_pl_dma_pcie_link_up() check, this is racy because
> xilinx_pl_dma_pcie_link_up() may tell you the link is up, but the link may go
> down before the driver attempts the config transaction.  THAT is the race.
> 
> If the controller hangs in that situation, that's a hardware defect, and from
> your comment, it sounds like it's unrecoverable.
> 
> > +		 */
> > +		if (!xilinx_pl_dma_pcie_link_up(port))
> > +			return false;
> 
> > +static int xilinx_pl_dma_pcie_intx_map(struct irq_domain *domain,
> unsigned int irq,
> > +				       irq_hw_number_t hwirq)
> 
> Wrap to fit in 80 columns like the rest of the file.
> 
> > +static struct irq_chip xilinx_msi_irq_chip = {
> > +	.name = "pl_dma_pciepcie:msi",
> 
> Why does this name have two copies of "pcie" in it?  This driver has four
> irq_chip structs; maybe the names could be more similar?
- Agreed, I'll modify all irq_chip names this in next patch 
Example: 
static struct irq_chip xilinx_msi_irq_chip = {
	.name = "pl_dma:PCIe MSI",

>   xilinx_leg_irq_chip			INTx
>   xilinx_msi_irq_chip		 	pl_dma_pciepcie:msi
>   xilinx_irq_chip			Xilinx MSI
>   xilinx_pl_dma_pcie_event_irq_chip	RC-Event
> 
> > +	/* 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:
- 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.

> https://lore.kernel.org/all/877csohcll.ffs@tglx/
> 
> > +	/*set the Bridge enable bit */
- Agreed, I ll modify it in next patch.
> Space before "Set".  I mentioned this before at
> https://lore.kernel.org/r/ZHd/7AaLaGyr1jNA@bhelgaas
> 
> > +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(dev, "missing \"reg\" property\n");
> 
> All your other error messages are capitalized.  Make this one match.
> 
> > +	bridge->ops = (struct pci_ops *)&xilinx_pl_dma_pcie_ops.pci_ops;
> 
> I don't think this cast is needed.
-Agreed, will modify it in next patch.
> 
> Bjorn