[PATCH] drivers: pci: dwc: dynamically set pci region limit

[Shyam Saini <shyamsaini@linux.microsoft.com>]

Hi Serge,

On Wed, May 22, 2024 at 05:10:46PM -0700, Shyam Saini wrote:
> commit 89473aa9ab26 ("PCI: dwc: Add iATU regions size detection procedure")
>> hardcodes the pci region limit to 4G.

> In what part does it _harcode_ the region limit to 4G? The procedure
> _auto-detects_ the actual iATU region limits. The limits aren't
> hardcoded.  The upper bound is 4G for DW PCIe IP-core older than
> v4.60. If the IP-core is younger than that the upper bound will be
> determined by the CX_ATU_MAX_REGION_SIZE IP-core synthesize parameter.
> The auto-detection procedure is about the CX_ATU_MAX_REGION_SIZE
> parameter detection.

>> This causes regression on
>> systems with PCI memory region higher than 4G.

> I am sure it doesn't. If it did we would have got multiple bug-reports
> right after the patch was merged into the mainline kernel repo. So
> please provide a comprehensive description of the problem you have.
> 
Sorry, I am certainly not a PCI expert but here is how I got here:

With [1] this change I started to see PCI driver throwing "Failed to set MEM range" error messages
and as consequence the driver probe fails with " error -22"

When I tracked the code, I found [1] this check was causing the driver probe failure 
and pci->region_limit was set to 4G in [2] this commit

So, to fix this issue I prepared this patch and it solves the problem I was having.
Based on your reply it seems problem is some where else.

I didn't see any problem with PCI memory <= 4G

>> 
>> Fix this by dynamically setting pci region limit based on maximum
>> size of memory ranges in the PCI device tree node.

> It seems to me that your patch is an attempt to workaround some
> problem you met. Give more insight about the problem in order to find
> a proper fix. The justification you've provided so far seems incorrect.
> 
> Note you can't use the ranges DT-property specified on your platform
> to determine the actual iATU regions size, because the later entity is
> a primary/root parameter of the PCIe controller. The DT-node memory
> ranges could be defined with a size greater than the actual iATU
> region size. In that case the address translation logic will be broken
> in the current driver implementation. AFAICS from the DW PCIe IP-core
> HW-manuals the IO-transaction will be passed further to the PCIe bus with
> no address translated and with the TLP fields filled in with the data
> retrieved on the application interface (XALI/AXI):
> 
> "3.10.5.6 No Address Match Result Overview: When there is no address
> match then the address is untranslated but the TLP header information
> (for fields that are programmable) comes from the relevant fields on
> the application transmit interface XALI* or AXI slave."
> 
> That isn't what could be allowed, because it may cause unpredictable
> results up to the system crash, for instance, if the TLPs with the
> untranslated TLPs reached a device they weren't targeted to.

> If what you met in your system was a memory range greater than the
> permitted iATU region limit, a proper fix would have been to allocate
> a one more iATU region for the out of bounds part of the memory range.

based on your suggestions I have prepared a new patch, which i will send shortly.
Thank you for the reviews.

[1] https://elixir.bootlin.com/linux/v6.9.1/source/drivers/pci/controller/dwc/pcie-designware.c#L480
[2] https://git.kernel.org/pub/scm/linux/kernel/git/pci/pci.git/commit/?id=89473aa9ab261948ed13b16effe841a675efed77