linux-kernel - Re: [PATCH v9 1/7] PCI: dwc: Use resource start as iomap() input in dw_pcie_pme_turn

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: <20250129231937.GA523281@bhelgaas>
Date: Wed, 29 Jan 2025 17:19:37 -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,
	Niklas Cassel <cassel@...nel.org>
Subject: Re: [PATCH v9 1/7] PCI: dwc: Use resource start as iomap() input in
 dw_pcie_pme_turn_off()

On Tue, Jan 28, 2025 at 05:07:34PM -0500, Frank Li wrote:
> Pass resource start as the input argument to iomap() instead of
> atu.cpu_address in dw_pcie_pme_turn_off(). While atu.cpu_address happens to
> be the same here, it actually represents the parent bus address before ATU,
> which may not always align with the CPU address. Using resource start
> ensures correctness and clarity.

1) "atu.cpu_address happens to be the same here" is currently true
   but only because this function is buggy and assumes the ATU input
   address is the same as a CPU physical address.

2) We're trying to make a correctness fix, not a clarity fix.  This
   patch by itself isn't a functional change because of the cpu_addr
   bug, but without this patch, fixing the cpu_addr bug would break
   the iomap.

3) "Pass resource start as the input to iomap()" just restates the
   patch.  We should say *why* this is important.  Something like
   this:

     The msg_res region translates writes into PCIe Message TLPs.
     Previously we mapped this region using atu.cpu_addr, the input
     address programmed into the ATU.

     "cpu_addr" is a misnomer because when a bus fabric translates
     addresses between the CPU and the ATU, the ATU input address is
     different from the CPU address.  A future patch will rename
     "cpu_addr" and correct the value to be the ATU input address
     instead of the CPU physical address.

     Map the msg_res region before writing to it using the msg_res
     resource start, a CPU physical address.

> Signed-off-by: Frank Li <Frank.Li@....com>
> ---
> change from v9 to v10
> - new patch
> ---
>  drivers/pci/controller/dwc/pcie-designware-host.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c
> index ffaded8f2df7b..ae3fd2a5dbf85 100644
> --- a/drivers/pci/controller/dwc/pcie-designware-host.c
> +++ b/drivers/pci/controller/dwc/pcie-designware-host.c
> @@ -908,7 +908,7 @@ static int dw_pcie_pme_turn_off(struct dw_pcie *pci)
>  	if (ret)
>  		return ret;
>  
> -	mem = ioremap(atu.cpu_addr, pci->region_align);
> +	mem = ioremap(pci->pp.msg_res->start, pci->region_align);
>  	if (!mem)
>  		return -ENOMEM;
>  
> 
> -- 
> 2.34.1
>