linux-kernel - Re: [PATCH] PCI: mvebu: Fix the use of the for_each_of

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: <hiu2ouj4f7zak2ovtwtigf6fylz4c7fdyyqiqezsddoouzr4n5@bfs7kudjfnp5>
Date: Wed, 3 Sep 2025 14:44:40 +0200
From: Jan Palus <jpalus@...tmail.com>
To: Klaus Kudielka <klaus.kudielka@...il.com>
Cc: Thomas Petazzoni <thomas.petazzoni@...tlin.com>, 
	Pali Rohár <pali@...nel.org>, Lorenzo Pieralisi <lpieralisi@...nel.org>, 
	Krzysztof Wilczyński <kwilczynski@...nel.org>, Manivannan Sadhasivam <mani@...nel.org>, 
	Rob Herring <robh@...nel.org>, Bjorn Helgaas <bhelgaas@...gle.com>, linux-pci@...r.kernel.org, 
	linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org, Bjorn Helgaas <helgaas@...nel.org>
Subject: Re: [PATCH] PCI: mvebu: Fix the use of the for_each_of_range()
 iterator

On 02.09.2025 17:13, Klaus Kudielka wrote:
> The blamed commit simplifies code, by using the for_each_of_range()
> iterator. But it results in no pci devices being detected anymore on
> Turris Omnia (and probably other mvebu targets).
> 
> Analysis:
> 
> To determine range.flags, of_pci_range_parser_one() uses bus->get_flags(),
> which resolves to of_bus_pci_get_flags(). That function already returns an
> IORESOURCE bit field, and NOT the original flags from the "ranges"
> resource.
> 
> Then mvebu_get_tgt_attr() attempts the very same conversion again.
> But this is a misinterpretation of range.flags.
> 
> Remove the misinterpretation of range.flags in mvebu_get_tgt_addr(),
> to restore the intended behavior.
> 
> Signed-off-by: Klaus Kudielka <klaus.kudielka@...il.com>
> Fixes: 5da3d94a23c6 ("PCI: mvebu: Use for_each_of_range() iterator for parsing "ranges"")
> Reported-by: Bjorn Helgaas <helgaas@...nel.org>
> Closes: https://lore.kernel.org/r/20250820184603.GA633069@bhelgaas/
> Reported-by: Jan Palus <jpalus@...tmail.com>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220479
> ---
>  drivers/pci/controller/pci-mvebu.c | 14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/pci/controller/pci-mvebu.c b/drivers/pci/controller/pci-mvebu.c
> index 755651f338..4e2e1fa197 100644
> --- a/drivers/pci/controller/pci-mvebu.c
> +++ b/drivers/pci/controller/pci-mvebu.c
> @@ -1168,9 +1168,6 @@ static void __iomem *mvebu_pcie_map_registers(struct platform_device *pdev,
>  	return devm_ioremap_resource(&pdev->dev, &port->regs);
>  }
>  
> -#define DT_FLAGS_TO_TYPE(flags)       (((flags) >> 24) & 0x03)
> -#define    DT_TYPE_IO                 0x1
> -#define    DT_TYPE_MEM32              0x2
>  #define DT_CPUADDR_TO_TARGET(cpuaddr) (((cpuaddr) >> 56) & 0xFF)
>  #define DT_CPUADDR_TO_ATTR(cpuaddr)   (((cpuaddr) >> 48) & 0xFF)
>  
> @@ -1189,17 +1186,10 @@ static int mvebu_get_tgt_attr(struct device_node *np, int devfn,
>  		return -EINVAL;
>  
>  	for_each_of_range(&parser, &range) {
> -		unsigned long rtype;
>  		u32 slot = upper_32_bits(range.bus_addr);
>  
> -		if (DT_FLAGS_TO_TYPE(range.flags) == DT_TYPE_IO)
> -			rtype = IORESOURCE_IO;
> -		else if (DT_FLAGS_TO_TYPE(range.flags) == DT_TYPE_MEM32)
> -			rtype = IORESOURCE_MEM;
> -		else
> -			continue;
> -
> -		if (slot == PCI_SLOT(devfn) && type == rtype) {
> +		if (slot == PCI_SLOT(devfn) &&
> +		    type == (range.flags & IORESOURCE_TYPE_BITS)) {
>  			*tgt = DT_CPUADDR_TO_TARGET(range.cpu_addr);
>  			*attr = DT_CPUADDR_TO_ATTR(range.cpu_addr);

Thanks for the patch Klaus! While it does improve situation we're not
quite there yet. It appears that what used to be stored in `cpuaddr` var
is also very different from `range.cpu_addr` value so the results
in both `*tgt` and `*attr` are both wrong.

Previously `cpuaddr` had a value like ie 0x8e8000000000000 or
0x4d0000000000000. Now `range.cpu_addr` is always 0xffffffffffffffff.
Luckily what used to be stored in `cpuaddr`:

u64 cpuaddr = of_read_number(range + na, pna)

appears to be stored in range.pci_bus_addr now. I can't make any
informed comment about this discrepancy however I can confirm following
change (in addition to your patch) makes mvebu driver work again (or at
least like it used to work in 6.15, it still needs Pali's patches to
have some devices working):

-			*tgt = DT_CPUADDR_TO_TARGET(range.cpu_addr);
-			*attr = DT_CPUADDR_TO_ATTR(range.cpu_addr);
+			*tgt = DT_CPUADDR_TO_TARGET(range.parent_bus_addr);
+			*attr = DT_CPUADDR_TO_ATTR(range.parent_bus_addr);