linux-kernel - Re: [PATCH v8 2/3] PCI: microchip: Fix inbound address translation tables

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: <20240821171712.GA256242@bhelgaas>
Date: Wed, 21 Aug 2024 12:17:12 -0500
From: Bjorn Helgaas <helgaas@...nel.org>
To: daire.mcnamara@...rochip.com
Cc: linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
	conor.dooley@...rochip.com, lpieralisi@...nel.org, kw@...ux.com,
	robh@...nel.org, bhelgaas@...gle.com, linux-kernel@...r.kernel.org,
	linux-riscv@...ts.infradead.org, krzk+dt@...nel.org,
	conor+dt@...nel.org, ilpo.jarvinen@...ux.intel.com
Subject: Re: [PATCH v8 2/3] PCI: microchip: Fix inbound address translation
 tables

On Wed, Aug 21, 2024 at 02:02:16PM +0100, daire.mcnamara@...rochip.com wrote:
> From: Daire McNamara <daire.mcnamara@...rochip.com>
> 
> On Microchip PolarFire SoC the PCIe Root Port can be behind one of three
> general purpose Fabric Interface Controller (FIC) buses that encapsulates
> an AXI-S bus. Depending on which FIC(s) the Root Port is connected
> through to CPU space, and what address translation is done by that FIC,
> the Root Port driver's inbound address translation may vary.
> 
> For all current supported designs and all future expected designs,
> inbound address translation done by a FIC on PolarFire SoC varies
> depending on whether PolarFire SoC in operating in coherent DMA mode or
> noncoherent DMA mode.

s/in operating/is operating/

> The setup of the outbound address translation tables in the Root Port
> driver only needs to handle these two cases.
> 
> Setup the inbound address translation tables to one of two address
> translations, depending on whether the rootport is being used with coherent
> DMA or noncoherent DMA.

s/rootport/Root Port/ to match above

> +static void mc_pcie_setup_inbound_atr(int window_index, u64 axi_addr, u64 pcie_addr, u64 size)

Most of this file fits in 80 columns, maybe these new decls could, too.

> +static int mc_pcie_setup_inbound_ranges(struct platform_device *pdev, struct mc_pcie *port)

> @@ -525,13 +529,20 @@ void plda_pcie_setup_window(void __iomem *bridge_base_addr, u32 index,
>  	val = upper_32_bits(pci_addr);
>  	writel(val, bridge_base_addr + (index * ATR_ENTRY_SIZE) +
>  	       ATR0_AXI4_SLV0_TRSL_ADDR_UDW);
> +}
> +EXPORT_SYMBOL_GPL(plda_pcie_setup_window);

I think the caller that needs this export is in a previous patch?

I wish we didn't need to export symbols like these since they're
really private to the driver, but I didn't look into the module
structure here.

Also, I get this error when building after both patch 1/3 and 2/3:

  drivers/pci/controller/plda/pcie-microchip-host.c:617:5: error: no previous prototype for ‘mc_pcie_setup_iomems’ [-Werror=missing-prototypes]
    617 | int mc_pcie_setup_iomems(struct pci_host_bridge *bridge,
	|     ^~~~~~~~~~~~~~~~~~~~

> +++ b/drivers/pci/controller/plda/pcie-starfive.c
> @@ -355,6 +355,11 @@ static int starfive_pcie_host_init(struct plda_pcie_rp *plda)
>  	 */
>  	plda_pcie_set_pref_win_64bit(plda);
>  
> +	/*
> +	 * Setup the inbound address translation
> +	 */

Could be a single-line comment: /* Setup the ... */

> +	plda_pcie_setup_inbound_address_translation(plda);