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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20230821201400.GA367570@bhelgaas>
Date:   Mon, 21 Aug 2023 15:14:00 -0500
From:   Bjorn Helgaas <helgaas@...nel.org>
To:     Thippeswamy Havalige <thippeswamy.havalige@....com>
Cc:     linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
        linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
        bhelgaas@...gle.com, lpieralisi@...nel.org, robh@...nel.org,
        kw@...ux.com, michal.simek@....com,
        krzysztof.kozlowski+dt@...aro.org, bharat.kumar.gogada@....com
Subject: Re: [PATCH v6 3/3] PCI: xilinx-xdma: Add Xilinx XDMA Root Port driver

On Fri, Aug 18, 2023 at 03:05:07PM +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@....com>
> Signed-off-by: Bharat Kumar Gogada <bharat.kumar.gogada@....com>
> ---
> changes in v6:
> - Replaced chained irq's with regular interrupts.

Thanks a million for working this out!

Trivial comments below, wait a couple days before reposting in case
there are other comments.

> +static inline bool xilinx_pl_dma_pcie_link_up(struct pl_dma_pcie *port)
> +{
> +	return (pcie_read(port, XILINX_PCIE_DMA_REG_PSCR) &
> +		XILINX_PCIE_DMA_REG_PSCR_LNKUP) ? 1 : 0;

This function returns bool, so I think true/false would be more
appropriate than 1/0.

> +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)) {
> +		/*
> +		 * Checking whether link is up here is a last line of defence,
> +		 * 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. This
> +		 * check is racy by definition and does not make controller hang
> +		 * if the link goes down after this check is performed.

This comment doesn't make sense to me.  "If PIO request initiated
while link-down, controller hangs ... This check is racy and does not
make controller hang if link goes down."  Which is it?

My *guess* is that this check narrows the window but doesn't close it,
so if xilinx_pl_dma_pcie_link_up() finds the link up, but the link
goes down before pci_generic_config_read() initiates the PIO request,
the controller hangs, and a reset is required.

> +		 */
> +		if (!xilinx_pl_dma_pcie_link_up(port))
> +			return false;
> +	} else if (devfn > 0)
> +		/* Only one device down on each root port */
> +		return false;
> +
> +	return true;
> +}

> +/* INTx error interrupts are Xilinx controller specific interrupt, used to
> + * notify user about error's such as cfg timeout, slave unsupported requests,

s/error's/errors/

> + * fatal and non fatal error etc.

> +		err = devm_request_irq(dev, irq, xilinx_pl_dma_pcie_intr_handler,
> +				       IRQF_SHARED | IRQF_NO_THREAD, intr_cause[i].sym, port);

Rewrap to fit in 80 columns.

> +	/* Needed for MSI DECODE MODE */
> +	pcie_write(port, XILINX_PCIE_DMA_IDR_ALL_MASK, XILINX_PCIE_DMA_REG_MSI_LOW_MASK);
> +	pcie_write(port, XILINX_PCIE_DMA_IDR_ALL_MASK, XILINX_PCIE_DMA_REG_MSI_HI_MASK);

Rewrap.

Bjorn