linux-kernel - Re: [PATCH v8 2/7] PCI: dwc: Use devicetree 'ranges' property to get rid of cpu_addr

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: <20250116232916.GA617353@bhelgaas>
Date: Thu, 16 Jan 2025 17:29:16 -0600
From: Bjorn Helgaas <helgaas@...nel.org>
To: Frank Li <Frank.Li@....com>
Cc: Rob Herring <robh@...nel.org>, Saravana Kannan <saravanak@...gle.com>,
	Jingoo Han <jingoohan1@...il.com>,
	Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
	Lorenzo Pieralisi <lpieralisi@...nel.org>,
	Krzysztof Wilczyński <kw@...ux.com>,
	Bjorn Helgaas <bhelgaas@...gle.com>,
	Richard Zhu <hongxing.zhu@....com>,
	Lucas Stach <l.stach@...gutronix.de>,
	Shawn Guo <shawnguo@...nel.org>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Pengutronix Kernel Team <kernel@...gutronix.de>,
	Fabio Estevam <festevam@...il.com>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-pci@...r.kernel.org,
	linux-arm-kernel@...ts.infradead.org, imx@...ts.linux.dev
Subject: Re: [PATCH v8 2/7] PCI: dwc: Use devicetree 'ranges' property to get
 rid of cpu_addr_fixup() callback

On Thu, Jan 16, 2025 at 05:14:00PM -0600, Bjorn Helgaas wrote:
> On Tue, Nov 19, 2024 at 02:44:20PM -0500, Frank Li wrote:
> > parent_bus_addr in struct of_range can indicate address information just
> > ahead of PCIe controller. Most system's bus fabric use 1:1 map between
> > input and output address. but some hardware like i.MX8QXP doesn't use 1:1
> > map. See below diagram:
> > 
> >             ┌─────────┐                    ┌────────────┐
> >  ┌─────┐    │         │ IA: 0x8ff8_0000    │            │
> >  │ CPU ├───►│   ┌────►├─────────────────┐  │ PCI        │
> >  └─────┘    │   │     │ IA: 0x8ff0_0000 │  │            │
> >   CPU Addr  │   │  ┌─►├─────────────┐   │  │ Controller │
> > 0x7ff8_0000─┼───┘  │  │             │   │  │            │
> >             │      │  │             │   │  │            │   PCI Addr
> > 0x7ff0_0000─┼──────┘  │             │   └──► IOSpace   ─┼────────────►
> >             │         │             │      │            │    0
> > 0x7000_0000─┼────────►├─────────┐   │      │            │
> >             └─────────┘         │   └──────► CfgSpace  ─┼────────────►
> >              BUS Fabric         │          │            │    0
> >                                 │          │            │
> >                                 └──────────► MemSpace  ─┼────────────►
> >                         IA: 0x8000_0000    │            │  0x8000_0000
> >                                            └────────────┘
> > 
> > bus@...00000 {
> > 	compatible = "simple-bus";
> > 	#address-cells = <1>;
> > 	#size-cells = <1>;
> > 	ranges = <0x80000000 0x0 0x70000000 0x10000000>;
> > 
> > 	pcie@...10000 {
> > 		compatible = "fsl,imx8q-pcie";
> > 		reg = <0x5f010000 0x10000>, <0x8ff00000 0x80000>;
> > 		reg-names = "dbi", "config";
> > 		#address-cells = <3>;
> > 		#size-cells = <2>;
> > 		device_type = "pci";
> > 		bus-range = <0x00 0xff>;
> > 		ranges = <0x81000000 0 0x00000000 0x8ff80000 0 0x00010000>,
> > 			 <0x82000000 0 0x80000000 0x80000000 0 0x0ff00000>;
> > 	...
> > 	};
> > };
> > 
> > Term internal address (IA) here means the address just before PCIe
> > controller. After ATU use this IA instead CPU address, cpu_addr_fixup() can
> > be removed.
> 
> > @@ -730,9 +779,15 @@ static int dw_pcie_iatu_setup(struct dw_pcie_rp *pp)
> >  
> >  		atu.index = i;
> >  		atu.type = PCIE_ATU_TYPE_MEM;
> > -		atu.cpu_addr = entry->res->start;
> > +		parent_bus_addr = entry->res->start;
> >  		atu.pci_addr = entry->res->start - entry->offset;
> >  
> > +		ret = dw_pcie_get_parent_addr(pci, entry->res->start, &parent_bus_addr);
> > +		if (ret)
> > +			return ret;
> > +
> > +		atu.cpu_addr = parent_bus_addr;
> 
> Here you set atu.cpu_addr to the intermediate bus address instead
> of the CPU physical address before calling
> dw_pcie_prog_outbound_atu().
> 
> But what about other callers of dw_pcie_prog_outbound_atu()?  Don't
> all of them need to use the intermediate bus address?

Somehow I expected the patch to skip calling ->cpu_addr_fixup() if the
driver had set 'use_parent_dt_ranges'.  In fact, I think that's a
requirement.

Since dw_pcie_prog_outbound_atu() is the only dwc caller, maybe the
parent_bus_addr change should go *there* instead of in the callers?

Bjorn