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: <77b5d0efa6e56bb391575aeeb4d1be80@manjaro.org>
Date: Tue, 04 Feb 2025 12:22:04 +0100
From: Dragan Simic <dsimic@...jaro.org>
To: Quentin Schulz <foss+kernel@...il.net>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
 Conor Dooley <conor+dt@...nel.org>, Heiko Stuebner <heiko@...ech.de>, Jagan
 Teki <jagan@...eble.ai>, Niklas Cassel <cassel@...nel.org>, Michael Riesch
 <michael.riesch@...fvision.net>, Jonas Karlman <jonas@...boo.se>,
 devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org, Quentin
 Schulz <quentin.schulz@...rry.de>, Krzysztof Kozlowski
 <krzysztof.kozlowski@...aro.org>
Subject: Re: [PATCH v4 3/4] arm64: dts: rockchip: add overlay tests for Rock
 5B PCIe overlays

Hello all,

I'm sorry for being late to the party.  Please, see some important
notes below;  to sum it up, we'll need to rework this a bit.

On 2025-01-31 11:40, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@...rry.de>
> 
> According to commit 40658534756f ("arm64: dts: rockchip: Add rock5b
> overlays for PCIe endpoint mode"), Rock 5B can operate in PCIe endpoint
> mode. For that to work, the rk3588-rock-5b-pcie-ep.dtbo overlay needs 
> to
> be applied on Rock 5B base Device Tree. If that Rock 5B is connected to
> another Rock 5B, the latter needs to apply the
> rk3588-rock-5b-pcie-srns.dtbo overlay.
> 
> In order to make sure the overlays are still valid in the future, let's
> add a validation test by applying the overlays on top of the main base
> at build time.
> 
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
> Reviewed-by: Niklas Cassel <cassel@...nel.org>
> Signed-off-by: Quentin Schulz <quentin.schulz@...rry.de>
> ---
>  arch/arm64/boot/dts/rockchip/Makefile | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/boot/dts/rockchip/Makefile
> b/arch/arm64/boot/dts/rockchip/Makefile
> index
> 267966ea69b194887d59e38a4220239a90a91306..ebcd16ce976ebe56a98d9685228980cd1f2f180a
> 100644
> --- a/arch/arm64/boot/dts/rockchip/Makefile
> +++ b/arch/arm64/boot/dts/rockchip/Makefile
> @@ -150,8 +150,6 @@ dtb-$(CONFIG_ARCH_ROCKCHIP) += 
> rk3588-orangepi-5-plus.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-quartzpro64.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5-itx.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b.dtb
> -dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtbo
> -dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtbo
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-tiger-haikou.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-toybrick-x0.dtb
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-turing-rk1.dtb
> @@ -186,3 +184,11 @@ rk3568-wolfvision-pf5-vz-2-uhd-dtbs :=
> rk3568-wolfvision-pf5.dtb \
>  dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-edgeble-neu6a-wifi.dtb
>  rk3588-edgeble-neu6a-wifi-dtbs := rk3588-edgeble-neu6a-io.dtb \
>  	rk3588-edgeble-neu6a-wifi.dtbo
> +
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-ep.dtb
> +rk3588-rock-5b-pcie-ep-dtbs := rk3588-rock-5b.dtb \
> +	rk3588-rock-5b-pcie-ep.dtbo
> +
> +dtb-$(CONFIG_ARCH_ROCKCHIP) += rk3588-rock-5b-pcie-srns.dtb
> +rk3588-rock-5b-pcie-srns-dtbs := rk3588-rock-5b.dtb \
> +	rk3588-rock-5b-pcie-srns.dtbo

After a quite lengthy excursion into the scripts/Makefile.dtbs and
scripts/Makefile.lib files, I'm afraid that the approach taken in
this patch isn't right.  Please allow me to explain.

Let's have a look at arch/arm64/boot/dts/ti/Makefile first, as an
example that provides already existing DT overlay checks.  Here's
an excerpt from that Makefile, which also provides an important cue
by mentioning CONFIG_OF_ALL_DTBS:

   12 dtb-$(CONFIG_ARCH_K3) += k3-am625-beagleplay.dtb
   13 dtb-$(CONFIG_ARCH_K3) += k3-am625-beagleplay-csi2-ov5640.dtbo
   14 dtb-$(CONFIG_ARCH_K3) += k3-am625-beagleplay-csi2-tevi-ov5640.dtbo

  135 # Build time test only, enabled by CONFIG_OF_ALL_DTBS
  136 k3-am625-beagleplay-csi2-ov5640-dtbs := k3-am625-beagleplay.dtb \
  137         k3-am625-beagleplay-csi2-ov5640.dtbo
  138 k3-am625-beagleplay-csi2-tevi-ov5640-dtbs := 
k3-am625-beagleplay.dtb \
  139         k3-am625-beagleplay-csi2-tevi-ov5640.dtbo

  213 dtb- += k3-am625-beagleplay-csi2-ov5640.dtb \
  214         k3-am625-beagleplay-csi2-tevi-ov5640.dtb \

As visible above, the DT overlays get added to dtb-$(CONFIG_ARCH_K3)
as .dtbo files (not as .dtb files), while the build-time DT overlay
tests specify the dependency chains for the overlays.

Even more importantly, the build-time overlay tests aren't supposed
to be executed until CONFIG_OF_ALL_DTBS is selected, which actually
gets handled at the very start of scripts/Makefile.dtbs:

    3 # If CONFIG_OF_ALL_DTBS is enabled, all DT blobs are built
    4 dtb-$(CONFIG_OF_ALL_DTBS) += $(dtb-)

The way this patch modifies arch/arm64/boot/dts/rockchip/Makefile
actually isn't correct, despite apparently producing the desired
outcome, because the way it adds rk3588-rock-5b-pcie-ep.dtb and
rk3588-rock-5b-pcie-srns.dtb (instead of adding proper .dtbo names)
to dtb-$(CONFIG_ARCH_ROCKCHIP) effectively creates some kind of
"phony targets" that indeed "get the job done", but unfortunately
in a wrong way.  The "phony targets" are handled by the following
"magic" in scripts/Makefile.dtbs:

    6 # Composite DTB (i.e. DTB constructed by overlay)
    7 multi-dtb-y := $(call multi-search, $(dtb-y), .dtb, -dtbs)
    8 # Primitive DTB compiled from *.dts
    9 real-dtb-y := $(call real-search, $(dtb-y), .dtb, -dtbs)
   10 # Base DTB that overlay is applied onto
   11 base-dtb-y := $(filter %.dtb, $(call real-search, $(multi-dtb-y), 
.dtb, -dtbs))

   18 targets         += $(real-dtb-y)

Let's have a look at the relevant part of the output produced when
"make dtbs" is executed with this patch applied:

   DTC     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtbo
   OVL     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtb
   DTC     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtbo
   OVL     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtb

Note the "OVL .*dtb" lines above, in which the ".*dtb" parts are
the above-mentioned "phony targets".  The above-quoted "magic" in
scripts/Makefile.dtbs is what "unfolds" those "phony targets" to
make them apparently produce the desired outcome, by populating
the $(real-dtb-y) variable with "rk3588-rock-5b-pcie-ep.dtbo
rk3588-rock-5b.dtb rk3588-rock-5b-pcie-srns.dtbo", as the proper
.dtbo names that get passed to the dtc utility.

Even more "magic" in scripts/Makefile.dtbs "unfolds" the "phony
targets" to have the build-time DT overlay tests performed in the
apparently proper way, while they actually shouldn't be performed
unless CONFIG_OF_ALL_DTBS is also selected.

With all this is mind, this patch should be reworked to follow
the right approach for defining build-time DT overlay tests, which
is illustrated in arch/arm64/boot/dts/ti/Makefile.  In particular,
we should just add the following DT overlay test definitions to
arch/arm64/boot/dts/rockchip/Makefile:

174 # Build-time tests only, enabled by CONFIG_OF_ALL_DTBS
175 rk3588-rock-5b-pcie-ep-dtbs := rk3588-rock-5b.dtb \
176         rk3588-rock-5b-pcie-ep.dtbo
177 rk3588-rock-5b-pcie-srns-dtbs := rk3588-rock-5b.dtb \
178         rk3588-rock-5b-pcie-srns.dtbo
179 dtb- += rk3588-rock-5b-pcie-ep.dtb \
180         rk3588-rock-5b-pcie-srns.dtb

You'll notice that the $(dtb-) variable pretty much again contains
the same "phony targets", but that's the way they should actually
be used.  I'm not very happy with the way they're specified, but
we should follow the approach from arch/arm64/boot/dts/ti/Makefile
anyway, and possibly improve the whole thing later.

I'd actually prefer to just have these test definitions added to the
end of arch/arm64/boot/dts/ti/Makefile, without moving the .dtbo lines
around.  That would keep the tests separate, which should be more
readable when we get more of them defined.

With the above-proposed changes in place, and with CONFIG_OF_ALL_DTBS
selected, the relevant part of the "make dtbs" output looks like this:

   DTC     arch/arm64/boot/dts/rockchip/rk3588-rock-5b.dtb
   DTC     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtbo
   DTC     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtbo
   OVL     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-ep.dtb
   OVL     arch/arm64/boot/dts/rockchip/rk3588-rock-5b-pcie-srns.dtb

No more "phony targets" in the produced output. :)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ