Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
MIME-Version: 1.0
References: <20220303224237.2497570-1-robh@kernel.org> <20220303224237.2497570-3-robh@kernel.org>
In-Reply-To: <20220303224237.2497570-3-robh@kernel.org>
From:   Geert Uytterhoeven <geert@linux-m68k.org>
Date:   Mon, 7 Mar 2022 13:20:29 +0100
Message-ID: <CAMuHMdUzu2PMN7ZerX=D0KsiqKZFDW5dVe3BY77wR=H-grrPVA@mail.gmail.com>
Subject: Re: [PATCH 2/2] dt-bindings: kbuild: Use DTB files for validation
To:     Rob Herring <robh@kernel.org>
Cc:     Masahiro Yamada <masahiroy@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>,
        Michal Marek <michal.lkml@markovi.net>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Frank Rowand <frowand.list@gmail.com>,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
        Maxime Ripard <maxime@cerno.tech>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@vger.kernel.org>,
        linux-kbuild <linux-kbuild@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hi Rob,

On Thu, Mar 3, 2022 at 11:43 PM Rob Herring <robh@kernel.org> wrote:
> Switch the DT validation to use DTB files directly instead of a DTS to
> YAML conversion.
>
> The original motivation for supporting validation on DTB files was to
> enable running validation on a running system (e.g. 'dt-validate
> /sys/firmware/fdt') or other cases where the original source DTS is not
> available.
>
> The YAML format was not without issues. Using DTBs with the schema type
> information solves some of those problems. The YAML format relies on the
> DTS source level information including bracketing of properties, size
> directives, and phandle tags all of which are lost in a DTB file. While
> standardizing the bracketing is a good thing, it does cause a lot of
> extra warnings and churn to fix them.
>
> Another issue has been signed types are not validated correctly as sign
> information is not propagated to YAML. Using the schema type information
> allows for proper handling of signed types. YAML also can't represent
> the full range of 64-bit integers as numbers are stored as floats by
> most/all parsers.
>
> The DTB validation works by decoding property values using the type
> information in the schemas themselves. The main corner case this does
> not work for is matrix types where neither dimension is fixed. For
> now, checking the dimensions in these cases are skipped.
>
> Signed-off-by: Rob Herring <robh@kernel.org>

Thanks for your patch!

While investigating why a newly added device node to DTS was not
instantiated as a platform device, I discovered an issue with this
patch: "make dtbs" no longer rebuilds DTB files that need a rebuild.

How to reproduce:

    $ git checkout next-20220307
    # apply this series and its dependency:
    # dt-bindings: kbuild: Support partial matches with DT_SCHEMA_FILES
    # dt-bindings: kbuild: Pass DT_SCHEMA_FILES to dt-validate
    # dt-bindings: kbuild: Use DTB files for validation
    $ make ARCH=arm shmobile_defconfig
    $ make ARCH=arm dtbs
    $ touch arch/arm/boot/dts/r8a7791.dtsi
    $ make ARCH=arm dtbs
    # The above command does NOT cause:
    # DTC     arch/arm/boot/dts/r8a7791-koelsch.dtb
    # DTC     arch/arm/boot/dts/r8a7791-porter.dtb

I don't see anything wrong with this patch at first sight, though.

> --- a/Documentation/devicetree/bindings/Makefile
> +++ b/Documentation/devicetree/bindings/Makefile
> @@ -26,8 +26,7 @@ $(obj)/%.example.dts: $(src)/%.yaml check_dtschema_version FORCE
>         $(call if_changed,extract_ex)
>
>  find_all_cmd = find $(srctree)/$(src) \( -name '*.yaml' ! \
> -               -name 'processed-schema*' ! \
> -               -name '*.example.dt.yaml' \)
> +               -name 'processed-schema*' \)
>
>  find_cmd = $(find_all_cmd) | grep -F "$(DT_SCHEMA_FILES)"
>  CHK_DT_DOCS := $(shell $(find_cmd))
> @@ -72,9 +71,9 @@ $(obj)/processed-schema.json: $(DT_DOCS) $(src)/.yamllint check_dtschema_version
>
>  always-y += processed-schema.json
>  always-$(CHECK_DT_BINDING) += $(patsubst $(srctree)/$(src)/%.yaml,%.example.dts, $(CHK_DT_DOCS))
> -always-$(CHECK_DT_BINDING) += $(patsubst $(srctree)/$(src)/%.yaml,%.example.dt.yaml, $(CHK_DT_DOCS))
> +always-$(CHECK_DT_BINDING) += $(patsubst $(srctree)/$(src)/%.yaml,%.example.dtb, $(CHK_DT_DOCS))
>
>  # Hack: avoid 'Argument list too long' error for 'make clean'. Remove most of
>  # build artifacts here before they are processed by scripts/Makefile.clean
>  clean-files = $(shell find $(obj) \( -name '*.example.dts' -o \
> -                       -name '*.example.dt.yaml' \) -delete 2>/dev/null)
> +                       -name '*.example.dtb' \) -delete 2>/dev/null)
> diff --git a/Documentation/devicetree/bindings/writing-schema.rst b/Documentation/devicetree/bindings/writing-schema.rst
> index 3b00fe981494..b4258bf81be5 100644
> --- a/Documentation/devicetree/bindings/writing-schema.rst
> +++ b/Documentation/devicetree/bindings/writing-schema.rst
> @@ -123,18 +123,6 @@ project can be installed with pip::
>  Several executables (dt-doc-validate, dt-mk-schema, dt-validate) will be
>  installed. Ensure they are in your PATH (~/.local/bin by default).
>
> -dtc must also be built with YAML output support enabled. This requires that
> -libyaml and its headers be installed on the host system. For some distributions
> -that involves installing the development package, such as:
> -
> -Debian::
> -
> -  apt-get install libyaml-dev
> -
> -Fedora::
> -
> -  dnf -y install libyaml-devel
> -
>  Running checks
>  ~~~~~~~~~~~~~~
>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 9f1e8442564e..4629af60160b 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -87,11 +87,6 @@ base-dtb-y := $(foreach m, $(multi-dtb-y), $(firstword $(call suffix-search, $m,
>
>  always-y                       += $(dtb-y)
>
> -ifneq ($(CHECK_DTBS),)
> -always-y += $(patsubst %.dtb,%.dt.yaml, $(real-dtb-y))
> -always-y += $(patsubst %.dtbo,%.dt.yaml, $(real-dtb-y))
> -endif
> -
>  # Add subdir path
>
>  extra-y                := $(addprefix $(obj)/,$(extra-y))
> @@ -347,12 +342,6 @@ cmd_dtc = $(HOSTCC) -E $(dtc_cpp_flags) -x assembler-with-cpp -o $(dtc-tmp) $< ;
>                 -d $(depfile).dtc.tmp $(dtc-tmp) ; \
>         cat $(depfile).pre.tmp $(depfile).dtc.tmp > $(depfile)
>
> -$(obj)/%.dtb: $(src)/%.dts $(DTC) FORCE
> -       $(call if_changed_dep,dtc)
> -
> -$(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
> -       $(call if_changed_dep,dtc)
> -
>  quiet_cmd_fdtoverlay = DTOVL   $@
>        cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $(real-prereqs)
>
> @@ -365,17 +354,22 @@ DT_CHECKER_FLAGS ?= $(if $(DT_SCHEMA_FILES),-l $(DT_SCHEMA_FILES),-m)
>  DT_BINDING_DIR := Documentation/devicetree/bindings
>  DT_TMP_SCHEMA ?= $(objtree)/$(DT_BINDING_DIR)/processed-schema.json
>
> +ifneq ($(CHECK_DTBS)$(CHECK_DT_BINDING),)
>  quiet_cmd_dtb_check =  CHECK   $@
> -      cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@
> +      cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
> +endif
>
>  define rule_dtc
>         $(call cmd_and_fixdep,dtc)
>         $(call cmd,dtb_check)
>  endef
>
> -$(obj)/%.dt.yaml: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE
> +$(obj)/%.dtb: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE
>         $(call if_changed_rule,dtc)
>
> +$(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
> +       $(call if_changed_dep,dtc)
> +
>  dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp)
>
>  # Bzip2
> diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile
> index 1cba78e1dce6..4d32b9497da9 100644
> --- a/scripts/dtc/Makefile
> +++ b/scripts/dtc/Makefile
> @@ -17,20 +17,7 @@ fdtoverlay-objs      := $(libfdt) fdtoverlay.o util.o
>
>  # Source files need to get at the userspace version of libfdt_env.h to compile
>  HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt
> -
> -ifeq ($(shell pkg-config --exists yaml-0.1 2>/dev/null && echo yes),)
> -ifneq ($(CHECK_DT_BINDING)$(CHECK_DTBS),)
> -$(error dtc needs libyaml for DT schema validation support. \
> -       Install the necessary libyaml development package.)
> -endif
>  HOST_EXTRACFLAGS += -DNO_YAML
> -else
> -dtc-objs       += yamltree.o
> -# To include <yaml.h> installed in a non-default path
> -HOSTCFLAGS_yamltree.o := $(shell pkg-config --cflags yaml-0.1)
> -# To link libyaml installed in a non-default path
> -HOSTLDLIBS_dtc := $(shell pkg-config --libs yaml-0.1)
> -endif
>
>  # Generated files need one more search path to include headers in source tree
>  HOSTCFLAGS_dtc-lexer.lex.o := -I $(srctree)/$(src)
> diff --git a/scripts/dtc/update-dtc-source.sh b/scripts/dtc/update-dtc-source.sh
> index 32ff17ffd089..94627541533e 100755
> --- a/scripts/dtc/update-dtc-source.sh
> +++ b/scripts/dtc/update-dtc-source.sh
> @@ -32,7 +32,7 @@ DTC_UPSTREAM_PATH=`pwd`/../dtc
>  DTC_LINUX_PATH=`pwd`/scripts/dtc
>
>  DTC_SOURCE="checks.c data.c dtc.c dtc.h flattree.c fstree.c livetree.c srcpos.c \
> -               srcpos.h treesource.c util.c util.h version_gen.h yamltree.c \
> +               srcpos.h treesource.c util.c util.h version_gen.h \
>                 dtc-lexer.l dtc-parser.y"
>  LIBFDT_SOURCE="fdt.c fdt.h fdt_addresses.c fdt_empty_tree.c \
>                 fdt_overlay.c fdt_ro.c fdt_rw.c fdt_strerror.c fdt_sw.c \
> --
> 2.32.0

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds