Re: [v4 PATCH 1/1] PCI: of: fix non-prefetchable region address range check.

[Bjorn Helgaas <helgaas@kernel.org>]

In subject, capitalize "Fix" and drop period at end.

On Fri, Jun 20, 2025 at 09:32:35AM +0000, Wannes Bouwen (Nokia) wrote:
>  
> [v4 PATCH 1/1] PCI: of: fix non-prefetchable region address range check.

Drop this.

> According to the PCIe spec (PCIe r6.3, sec 7.5.1.3.8), non-prefetchable
> memory supports only 32-bit host bridge windows (both base address as
> limit address).

7.5.1.3.8 is about PCI-to-PCI bridge windows, not host bridge windows.

I'm confused about what's going on here.  The word "prefetch" doesn't
even appear in PCIe r7.0, but historically, issue was that we have to
be careful about putting a non-prefetchable BAR in a prefetchable
window because a read (which might be a prefetch) in a
non-prefetchable BAR is allowed to have side effects.

But if we put a prefetchable BAR in a non-prefetchable window, nothing
bad happens other than performance might be bad.

Are we trying to warn about a potential performance problem?  Or is
there some functional problem here?

> In the kernel there is a check that prints a warning if a
> non-prefetchable resource's size exceeds the 32-bit limit.
> 
> The check currently checks the size of the resource, while actually the
> check should be done on the PCIe end address of the non-prefetchable
> window.
> 
> Move the check to devm_of_pci_get_host_bridge_resources() where the PCIe
> addresses are available and use the end address instead of the size of
> the window.

Are you seeing an issue here?  Can we include a dmesg snippet that
illustrates it?

> Fixes: fede8526cc48 (PCI: of: Warn if non-prefetchable memory aperture
> size is > 32-bit)
> Signed-off-by: Wannes Bouwen <wannes.bouwen@nokia.com>
> ---
> 
> v4:
>   - Update warning text
> 
> v3:
>   - Update subject and description + add changelog
> 
> v2:
>   - Use PCI address range instead of window size to check that window is
>     within a 32bit boundary.
> 
> ---
>  drivers/pci/of.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/of.c b/drivers/pci/of.c
> index 3579265f1198..16405985a53a 100644
> --- a/drivers/pci/of.c
> +++ b/drivers/pci/of.c
> @@ -400,6 +400,13 @@ static int devm_of_pci_get_host_bridge_resources(struct device *dev,
>  			*io_base = range.cpu_addr;
>  		} else if (resource_type(res) == IORESOURCE_MEM) {
>  			res->flags &= ~IORESOURCE_MEM_64;
> +
> +			if (!(res->flags & IORESOURCE_PREFETCH))
> +				if (upper_32_bits(range.pci_addr + range.size - 1))
> +					dev_warn(dev,
> +						"host bridge non-prefetchable window: pci range end address exceeds 32 bit boundary %pR"
> +						" (pci address range [%#012llx-%#012llx])\n",
> +						res, range.pci_addr, range.pci_addr + range.size - 1);

I gave you bad advice because I hadn't looked earlier in this
function.  devm_of_pci_get_host_bridge_resources() printed this
earlier:

  MEM  %#012llx..%#012llx -> %#012llx

where the first part is basically the %pR information in a different
format and the last part is the bus address, and I think a warning
here should look similar, e.g.,

  dev_warn(dev, "Bus address %#012llx..%#012llx end is past 32-bit boundary\n",

>  		}
>  
>  		pci_add_resource_offset(resources, res,	res->start - range.pci_addr);
> @@ -622,10 +629,6 @@ static int pci_parse_request_of_pci_ranges(struct device *dev,
>  		case IORESOURCE_MEM:
>  			res_valid |= !(res->flags & IORESOURCE_PREFETCH);
>  
> -			if (!(res->flags & IORESOURCE_PREFETCH))
> -				if (upper_32_bits(resource_size(res)))
> -					dev_warn(dev, "Memory resource size exceeds max for 32 bits\n");
> -
>  			break;
>  		}
>  	}
> -- 
> 2.43.5