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

From: Bjorn Helgaas <helgaas@kernel.org>
To: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
Cc: krzysztof.kozlowski@linaro.org, bhelgaas@google.com,
	devicetree@vger.kernel.org, linux-pci@vger.kernel.org,
	linux-kernel@vger.kernel.org, robh+dt@kernel.org,
	lorenzo.pieralisi@arm.com, linux-arm-kernel@lists.infradead.org,
	bharat.kumar.gogada@amd.com, michals@amd.com,
	nagaradhesh.yeleswarapu@amd.com
Subject: Re: [PATCH v4 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver
Date: Wed, 31 May 2023 12:12:12 -0500	[thread overview]
Message-ID: <ZHd/7AaLaGyr1jNA@bhelgaas> (raw)
In-Reply-To: <20230531083825.985584-4-thippeswamy.havalige@amd.com>

On Wed, May 31, 2023 at 02:08:25PM +0530, Thippeswamy Havalige wrote:
> Add support for Xilinx XDMA Soft IP core as Root Port.
> 
> The Zynq UltraScale+ MPSoCs devices support XDMA soft IP module in
> programmable logic.
> 
> The integrated XDMA soft IP block has integrated bridge function that
> can act as PCIe Root Port.
> 
> Signed-off-by: Thippeswamy Havalige <thippeswamy.havalige@amd.com>
> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@amd.com>

> |Reported-by: kernel test robot <lkp@intel.com>
> |Reported-by: Dan Carpenter <error27@gmail.com>
> |Closes: https://lore.kernel.org/r/202305261250.2cs1phTS-lkp@intel.com/

Not relevant.  These are basically review comments.

> +	  Add support for the Xilinx PL DMA PCIe host bridge,
> +	  The controller is an soft IP which can act as Root Port.
> +	  If you know your system provides xilinx PCIe host controller
> +	  bridge DMA as soft IP say Y; if you are not sure, say N.

s/is an soft/is soft/
s/xilinx/Xilinx/

> +#define XILINX_PCIE_DMA_REG_IMR			0x0000013c
> +#define XILINX_PCIE_DMA_REG_MSIBASE1		0x0000014c
> +#define XILINX_PCIE_DMA_REG_MSI_HI_MASK		0x0000017c
> ...
> +#define XILINX_PCIE_DMA_IMR_ALL_MASK	0x0FF30FE9
> +#define XILINX_PCIE_DMA_IDR_ALL_MASK	0xFFFFFFFF

Pick upper-case hex or lower-case hex and use it consistently.

> +static inline bool xilinx_pl_dma_pcie_linkup(struct pl_dma_pcie *port)

Name this *_pcie_link_up() (not *_pcie_linkup()) to match other drivers.

> +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 (!xilinx_pl_dma_pcie_linkup(port))
> +			return false;
> +	} else if (devfn > 0)
> +		/* Only one device down on each root port */
> +		return false;
> +
> +	return true;
> +}
> +
> +static void __iomem *xilinx_pl_dma_pcie_map_bus(struct pci_bus *bus,
> +						unsigned int devfn, int where)
> +{
> +	struct pl_dma_pcie *port = bus->sysdata;
> +
> +	if (!xilinx_pl_dma_pcie_valid_device(bus, devfn))
> +		return NULL;

Checking whether the link is up is racy because the link may be up, so
xilinx_pl_dma_pcie_valid_device() returns true, then the link may go
down before the read below.

What happens then?  If it's an error that you can recover from, it
would better to skip the link up check and just handle the error.

> +	return port->reg_base + PCIE_ECAM_OFFSET(bus->number, devfn, where);
> +}

> +	/*set the Bridge enable bit */

  /* Set ... */ (add space before "Set" and capitalize it)

Bjorn