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

[Frank Li]

On Wed, Jan 29, 2025 at 05:19:37PM -0600, Bjorn Helgaas wrote:
> 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.

Thanks, will update commit message. The key change is patch 3

PCI: Add parent_bus_offset to resource_entry

Frank

>
> > Signed-off-by: Frank Li <Frank.Li@xxxxxxx>
> > ---
> > 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
> >