[regression] of: mis-parsing Depthcharge's /firmware

[Brian Norris <briannorris@chromium.org>]

Hi all,

(New subject; was "Re: [GIT PULL] Devicetree updates for v6.13")

On Mon, Dec 09, 2024 at 05:28:09PM +0800, Chen-Yu Tsai wrote:
> steelix.dtb is the same, plus the firmware now inserts #address-cells
> and #size-cells under /firmware. This fix has landed for all future
> ChromeOS devices via our main firmware branch [1].
> 
> AFAIK they also have a bad FDT END symbol. This was only recently
> discovered and fixed for future devices [2].
> 
> 
> ChenYu
> 
> [1] Gerrit: https://crrev.com/c/6051580
> [2] Gerrit: https://review.coreboot.org/c/coreboot/+/85462

This all comes back to bite us, since nobody went back to patch the
existing Chromebook device trees, and now we've added a true regression
on top:

In commit 6e5773d52f4a ("of/address: Fix WARN when attempting
translating non-translatable addresses") we now reject devices without
'#address-cells', and this breaks the DTs generated by bootloaders
without Chen-Yu's https://crrev.com/c/6051580 fix (this is ... pretty
much all Chromebooks). Specifically, Linux now refuses to add 'reg'
resources to the /firmware/coreboot device, and we fail with:

[   11.886271] coreboot_table firmware:coreboot: probe with driver coreboot_table failed with error -22

This is almost certainly a DTB ABI regression.

This was noticed here (OpenWrt supports some Chromium-based WiFi routers
that use Depthcharge-based bootloaders from many years ago):

https://github.com/openwrt/openwrt/issues/21243

For now, I just patched up the OpenWrt DTS files like so:
https://github.com/openwrt/openwrt/pull/22951

But what should we do going forward? I note that Rob says "We may
revisit this later and address with a fixup to the DT itself" in commit
8600058ba28a ("of: Add coreboot firmware to excluded default cells
list").

That never happened, and a ton of Chromium devices are still broken.
(They don't have WARNINGs, but /sys/firmware/vpd, etc., is still
missing.)

Can we patch of_bus_default_match() to accept an empty 'ranges' [1]? Or
should I go patch every Chromium-device DTS file I can find? So far, I
think I can get that done in 17 files in the upstream tree...

Brian

[1] From ePAPR:

  "If the [ranges] property is defined with an <empty> value, it
  specifies that the parent and child address 28 space is identical, and
  no address translation is required."

And:

  "An ePAPR-compliant boot program shall supply #address-cells and
  #size-cells on all nodes 16 that have children.

  If missing, a client program should assume a default value of 2 for
  #address-cells, and a value of 1 for #size-cells."

So far, this does the trick, but I didn't review all the ramifications
here.

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 4034d798c55a..f86386c407d4 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -328,7 +328,15 @@ static int of_bus_default_flags_match(struct device_node *np)
 
 static int of_bus_default_match(struct device_node *np)
 {
-	return of_property_present(np, "#address-cells");
+	int len;
+
+	if (of_property_present(np, "#address-cells"))
+		return true;
+
+	if (of_find_property(np, "ranges", &len) && len == 0)
+		return true;
+
+	return false;
 }
 
 /*