diff mbox series

[v3] kbuild: Enable DT schema checks for %.dtb targets

Message ID 20220623144357.297252-1-dmitry.baryshkov@linaro.org (mailing list archive)
State New, archived
Headers show
Series [v3] kbuild: Enable DT schema checks for %.dtb targets | expand

Commit Message

Dmitry Baryshkov June 23, 2022, 2:43 p.m. UTC 
It is possible to build a single dtb, but not with DT schema validation
enabled. Enable the schema validation to run for %.dtb and %.dtbo
targets. Anyone building a dtb for a specific platform *should* pay
attention to schema warnings.

This could be supported with a separate %.dt.yaml target instead.
However, the .dt.yaml format is considered an intermediate format and
could possibly go away at some point if schema checking is integrated
into dtc. Also, the plan is to enable the schema checks by default once
platforms are free of warnings, and this is a move in that direction.

This patch differs from the previous one ([1]) in the fact that it
requires specifying VALIDATE_DT=1 to run the checks while doing the
build. Thus default build procedures would not obtain additional build
dependency, while maintainers can still build a single DTB file an get
only corresponding warnings.

[1] https://lore.kernel.org/all/20210913145146.766080-1-robh@kernel.org/

Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
Cc: Tom Rini <trini@konsulko.com>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Cc: linux-kbuild@vger.kernel.org
Co-developed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Rob Herring <robh@kernel.org>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 Makefile | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

Comments

Marijn Suijten July 14, 2022, 8:17 p.m. UTC | #1 
On 2022-06-23 17:43:57, Dmitry Baryshkov wrote:
> It is possible to build a single dtb, but not with DT schema validation
> enabled. Enable the schema validation to run for %.dtb and %.dtbo
> targets. Anyone building a dtb for a specific platform *should* pay
> attention to schema warnings.
> 
> This could be supported with a separate %.dt.yaml target instead.
> However, the .dt.yaml format is considered an intermediate format and
> could possibly go away at some point if schema checking is integrated
> into dtc. Also, the plan is to enable the schema checks by default once
> platforms are free of warnings, and this is a move in that direction.
> 
> This patch differs from the previous one ([1]) in the fact that it
> requires specifying VALIDATE_DT=1 to run the checks while doing the
> build. Thus default build procedures would not obtain additional build
> dependency, while maintainers can still build a single DTB file an get
> only corresponding warnings.
> 
> [1] https://lore.kernel.org/all/20210913145146.766080-1-robh@kernel.org/
> 
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: linux-kbuild@vger.kernel.org
> Co-developed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Thanks, glad to see this back rather than having to resort to reverting
75e895343d5a ("Revert "kbuild: Enable DT schema checks for %.dtb
targets"") locally.

Tested-by: Marijn Suijten <marijn.suijten@somainline.org>

> ---
>  Makefile | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index c43d825a3c4c..0942922384c4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1365,11 +1365,17 @@ endif
>  
>  ifneq ($(dtstree),)
>  
> -%.dtb: include/config/kernel.release scripts_dtc
> -	$(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
> +ifneq ($(VALIDATE_DT),)
> +DT_YAML = $(dtstree)/$*.dt.yaml
> +DT_CHECK = dt_binding_check
> +export CHECK_DTBS=y
> +endif
>  
> -%.dtbo: include/config/kernel.release scripts_dtc
> -	$(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
> +%.dtb: include/config/kernel.release scripts_dtc $(DT_CHECK)
> +	$(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@ $(DT_YAML)
> +
> +%.dtbo: include/config/kernel.release scripts_dtc $(DT_CHECK)
> +	$(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@ $(DT_YAML)
>  
>  PHONY += dtbs dtbs_install dtbs_check
>  dtbs: include/config/kernel.release scripts_dtc
> @@ -1654,6 +1660,10 @@ help:
>  	@echo  '		3: more obscure warnings, can most likely be ignored'
>  	@echo  '		e: warnings are being treated as errors'
>  	@echo  '		Multiple levels can be combined with W=12 or W=123'
> +	@$(if $(dtstree), \
> +		echo '  make VALIDATE_DT=y [targets] Validate all DT processsed during the build'; \
> +		echo '         This can be applied both to "dtbs" and to individual "foo.dtb" targets' ; \
> +		)
>  	@echo  ''
>  	@echo  'Execute "make" or "make all" to build all targets marked with [*] '
>  	@echo  'For further info see the ./README file'
> -- 
> 2.35.1
>
Rob Herring (Arm) July 15, 2022, 11:01 p.m. UTC | #2 
On Thu, Jun 23, 2022 at 8:44 AM Dmitry Baryshkov
<dmitry.baryshkov@linaro.org> wrote:
>
> It is possible to build a single dtb, but not with DT schema validation
> enabled. Enable the schema validation to run for %.dtb and %.dtbo
> targets. Anyone building a dtb for a specific platform *should* pay
> attention to schema warnings.
>
> This could be supported with a separate %.dt.yaml target instead.
> However, the .dt.yaml format is considered an intermediate format and
> could possibly go away at some point if schema checking is integrated
> into dtc. Also, the plan is to enable the schema checks by default once
> platforms are free of warnings, and this is a move in that direction.
>
> This patch differs from the previous one ([1]) in the fact that it
> requires specifying VALIDATE_DT=1 to run the checks while doing the
> build. Thus default build procedures would not obtain additional build
> dependency, while maintainers can still build a single DTB file an get
> only corresponding warnings.

I'd rather this be a kconfig option, so that eventually 'make
allmodconfig; make dtbs' also runs the schema checks. If something can
be enabled for allmodconfig, then builders will automatically start
testing it. Though the extra dependency is a problem here.

>
> [1] https://lore.kernel.org/all/20210913145146.766080-1-robh@kernel.org/
>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Cc: linux-kbuild@vger.kernel.org
> Co-developed-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Rob Herring <robh@kernel.org>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  Makefile | 18 ++++++++++++++----
>  1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/Makefile b/Makefile
> index c43d825a3c4c..0942922384c4 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1365,11 +1365,17 @@ endif
>
>  ifneq ($(dtstree),)
>
> -%.dtb: include/config/kernel.release scripts_dtc
> -       $(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
> +ifneq ($(VALIDATE_DT),)
> +DT_YAML = $(dtstree)/$*.dt.yaml

.dt.yaml files are deprecated now. This probably doesn't do anything.

> +DT_CHECK = dt_binding_check
> +export CHECK_DTBS=y
> +endif
>
> -%.dtbo: include/config/kernel.release scripts_dtc
> -       $(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
> +%.dtb: include/config/kernel.release scripts_dtc $(DT_CHECK)
> +       $(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@ $(DT_YAML)
> +
> +%.dtbo: include/config/kernel.release scripts_dtc $(DT_CHECK)
> +       $(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@ $(DT_YAML)
>
>  PHONY += dtbs dtbs_install dtbs_check
>  dtbs: include/config/kernel.release scripts_dtc
> @@ -1654,6 +1660,10 @@ help:
>         @echo  '                3: more obscure warnings, can most likely be ignored'
>         @echo  '                e: warnings are being treated as errors'
>         @echo  '                Multiple levels can be combined with W=12 or W=123'
> +       @$(if $(dtstree), \
> +               echo '  make VALIDATE_DT=y [targets] Validate all DT processsed during the build'; \

Typo.


> +               echo '         This can be applied both to "dtbs" and to individual "foo.dtb" targets' ; \
> +               )
>         @echo  ''
>         @echo  'Execute "make" or "make all" to build all targets marked with [*] '
>         @echo  'For further info see the ./README file'
> --
> 2.35.1
>
Masahiro Yamada July 16, 2022, 8:12 a.m. UTC | #3 
On Sat, Jul 16, 2022 at 8:02 AM Rob Herring <robh@kernel.org> wrote:
>
> On Thu, Jun 23, 2022 at 8:44 AM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
> >
> > It is possible to build a single dtb, but not with DT schema validation
> > enabled. Enable the schema validation to run for %.dtb and %.dtbo
> > targets. Anyone building a dtb for a specific platform *should* pay
> > attention to schema warnings.
> >
> > This could be supported with a separate %.dt.yaml target instead.
> > However, the .dt.yaml format is considered an intermediate format and
> > could possibly go away at some point if schema checking is integrated
> > into dtc. Also, the plan is to enable the schema checks by default once
> > platforms are free of warnings, and this is a move in that direction.
> >
> > This patch differs from the previous one ([1]) in the fact that it
> > requires specifying VALIDATE_DT=1 to run the checks while doing the
> > build. Thus default build procedures would not obtain additional build
> > dependency, while maintainers can still build a single DTB file an get
> > only corresponding warnings.
>
> I'd rather this be a kconfig option, so that eventually 'make
> allmodconfig; make dtbs' also runs the schema checks. If something can
> be enabled for allmodconfig, then builders will automatically start
> testing it. Though the extra dependency is a problem here.


The dependency on libyaml is gone.

As for the dependency on dt-schema, is it a good idea to
pull it into the kernel tree somewhere,
like we periodically sync scripts/dtc/ with its upstream?

Any other problematic tool dependency?





>
> >
> > [1] https://lore.kernel.org/all/20210913145146.766080-1-robh@kernel.org/
> >
> > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Cc: Tom Rini <trini@konsulko.com>
> > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > Cc: linux-kbuild@vger.kernel.org
> > Co-developed-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > ---
> >  Makefile | 18 ++++++++++++++----
> >  1 file changed, 14 insertions(+), 4 deletions(-)
> >
> > diff --git a/Makefile b/Makefile
> > index c43d825a3c4c..0942922384c4 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -1365,11 +1365,17 @@ endif
> >
> >  ifneq ($(dtstree),)
> >
> > -%.dtb: include/config/kernel.release scripts_dtc
> > -       $(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
> > +ifneq ($(VALIDATE_DT),)
> > +DT_YAML = $(dtstree)/$*.dt.yaml
>
> .dt.yaml files are deprecated now. This probably doesn't do anything.

Right, this causes a build error.


masahiro@grover:~/ref/linux$ make ARCH=arm64 VALIDATE_DT=1
arm/foundation-v8.dtb
arch/arm64/Makefile:36: Detected assembler with broken .inst;
disassembly will be unreliable
  DTC     arch/arm64/boot/dts/arm/foundation-v8.dtb
  CHECK   arch/arm64/boot/dts/arm/foundation-v8.dtb
/home/masahiro/ref/linux/arch/arm64/boot/dts/arm/foundation-v8.dtb:
sysreg@10000: '#address-cells' is a required property
From schema: /home/masahiro/ref/linux/Documentation/devicetree/bindings/arm/vexpress-sysreg.yaml
/home/masahiro/ref/linux/arch/arm64/boot/dts/arm/foundation-v8.dtb:
sysreg@10000: '#size-cells' is a required property
From schema: /home/masahiro/ref/linux/Documentation/devicetree/bindings/arm/vexpress-sysreg.yaml
make[1]: *** No rule to make target
'arch/arm64/boot/dts/arm/foundation-v8.dt.yaml'.  Stop.
make: *** [Makefile:1379: arm/foundation-v8.dtb] Error 2







--
Best Regards

Masahiro Yamada
Masahiro Yamada July 16, 2022, 9:48 a.m. UTC | #4 
On Sat, Jul 16, 2022 at 5:12 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> On Sat, Jul 16, 2022 at 8:02 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, Jun 23, 2022 at 8:44 AM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > It is possible to build a single dtb, but not with DT schema validation
> > > enabled. Enable the schema validation to run for %.dtb and %.dtbo
> > > targets. Anyone building a dtb for a specific platform *should* pay
> > > attention to schema warnings.
> > >
> > > This could be supported with a separate %.dt.yaml target instead.
> > > However, the .dt.yaml format is considered an intermediate format and
> > > could possibly go away at some point if schema checking is integrated
> > > into dtc. Also, the plan is to enable the schema checks by default once
> > > platforms are free of warnings, and this is a move in that direction.
> > >
> > > This patch differs from the previous one ([1]) in the fact that it
> > > requires specifying VALIDATE_DT=1 to run the checks while doing the
> > > build. Thus default build procedures would not obtain additional build
> > > dependency, while maintainers can still build a single DTB file an get
> > > only corresponding warnings.
> >
> > I'd rather this be a kconfig option, so that eventually 'make
> > allmodconfig; make dtbs' also runs the schema checks. If something can
> > be enabled for allmodconfig, then builders will automatically start
> > testing it. Though the extra dependency is a problem here.
>
>
> The dependency on libyaml is gone.
>
> As for the dependency on dt-schema, is it a good idea to
> pull it into the kernel tree somewhere,
> like we periodically sync scripts/dtc/ with its upstream?
>
> Any other problematic tool dependency?
>
>
>
>
>
> >
> > >
> > > [1] https://lore.kernel.org/all/20210913145146.766080-1-robh@kernel.org/
> > >
> > > Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> > > Cc: Tom Rini <trini@konsulko.com>
> > > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > > Cc: linux-kbuild@vger.kernel.org
> > > Co-developed-by: Rob Herring <robh@kernel.org>
> > > Signed-off-by: Rob Herring <robh@kernel.org>
> > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> > > ---
> > >  Makefile | 18 ++++++++++++++----
> > >  1 file changed, 14 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/Makefile b/Makefile
> > > index c43d825a3c4c..0942922384c4 100644
> > > --- a/Makefile
> > > +++ b/Makefile
> > > @@ -1365,11 +1365,17 @@ endif
> > >
> > >  ifneq ($(dtstree),)
> > >
> > > -%.dtb: include/config/kernel.release scripts_dtc
> > > -       $(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
> > > +ifneq ($(VALIDATE_DT),)
> > > +DT_YAML = $(dtstree)/$*.dt.yaml
> >
> > .dt.yaml files are deprecated now. This probably doesn't do anything.
>
> Right, this causes a build error.
>
>
> masahiro@grover:~/ref/linux$ make ARCH=arm64 VALIDATE_DT=1
> arm/foundation-v8.dtb
> arch/arm64/Makefile:36: Detected assembler with broken .inst;
> disassembly will be unreliable
>   DTC     arch/arm64/boot/dts/arm/foundation-v8.dtb
>   CHECK   arch/arm64/boot/dts/arm/foundation-v8.dtb
> /home/masahiro/ref/linux/arch/arm64/boot/dts/arm/foundation-v8.dtb:
> sysreg@10000: '#address-cells' is a required property
> From schema: /home/masahiro/ref/linux/Documentation/devicetree/bindings/arm/vexpress-sysreg.yaml
> /home/masahiro/ref/linux/arch/arm64/boot/dts/arm/foundation-v8.dtb:
> sysreg@10000: '#size-cells' is a required property
> From schema: /home/masahiro/ref/linux/Documentation/devicetree/bindings/arm/vexpress-sysreg.yaml
> make[1]: *** No rule to make target
> 'arch/arm64/boot/dts/arm/foundation-v8.dt.yaml'.  Stop.
> make: *** [Makefile:1379: arm/foundation-v8.dtb] Error 2
>
>
>
>
>
>
>
> --
> Best Regards
>
> Masahiro Yamada





I think v2 was better than v3 at least.
(v2 reuses existing CHECK_DTBS instead of adding new VALIDATE_DT)

v2: https://lore.kernel.org/linux-kbuild/20220706114407.1507412-1-dmitry.baryshkov@linaro.org/

I commented there for a simpler implementation if we go this way.
Rob Herring (Arm) Aug. 4, 2022, 4:34 p.m. UTC | #5 
On Sat, Jul 16, 2022 at 05:12:38PM +0900, Masahiro Yamada wrote:
> On Sat, Jul 16, 2022 at 8:02 AM Rob Herring <robh@kernel.org> wrote:
> >
> > On Thu, Jun 23, 2022 at 8:44 AM Dmitry Baryshkov
> > <dmitry.baryshkov@linaro.org> wrote:
> > >
> > > It is possible to build a single dtb, but not with DT schema validation
> > > enabled. Enable the schema validation to run for %.dtb and %.dtbo
> > > targets. Anyone building a dtb for a specific platform *should* pay
> > > attention to schema warnings.
> > >
> > > This could be supported with a separate %.dt.yaml target instead.
> > > However, the .dt.yaml format is considered an intermediate format and
> > > could possibly go away at some point if schema checking is integrated
> > > into dtc. Also, the plan is to enable the schema checks by default once
> > > platforms are free of warnings, and this is a move in that direction.
> > >
> > > This patch differs from the previous one ([1]) in the fact that it
> > > requires specifying VALIDATE_DT=1 to run the checks while doing the
> > > build. Thus default build procedures would not obtain additional build
> > > dependency, while maintainers can still build a single DTB file an get
> > > only corresponding warnings.
> >
> > I'd rather this be a kconfig option, so that eventually 'make
> > allmodconfig; make dtbs' also runs the schema checks. If something can
> > be enabled for allmodconfig, then builders will automatically start
> > testing it. Though the extra dependency is a problem here.
> 
> 
> The dependency on libyaml is gone.
> 
> As for the dependency on dt-schema, is it a good idea to
> pull it into the kernel tree somewhere,
> like we periodically sync scripts/dtc/ with its upstream?

I don't want that. We want DT stuff less coupled to the kernel. Doing 
that also means the person syncing dtschema into the kernel is the one 
stuck fixing any new warnings. That mostly ends up being me anyways, but 
I don't want to guarantee that. Also, that would only shift the 
dependencies to json-schema, ruamel.yaml, pylibfdt, etc. python 
packages.

For dtc, I'd actually like to remove it from the kernel.

Rob
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index c43d825a3c4c..0942922384c4 100644
--- a/Makefile
+++ b/Makefile
@@ -1365,11 +1365,17 @@  endif
 
 ifneq ($(dtstree),)
 
-%.dtb: include/config/kernel.release scripts_dtc
-	$(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
+ifneq ($(VALIDATE_DT),)
+DT_YAML = $(dtstree)/$*.dt.yaml
+DT_CHECK = dt_binding_check
+export CHECK_DTBS=y
+endif
 
-%.dtbo: include/config/kernel.release scripts_dtc
-	$(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@
+%.dtb: include/config/kernel.release scripts_dtc $(DT_CHECK)
+	$(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@ $(DT_YAML)
+
+%.dtbo: include/config/kernel.release scripts_dtc $(DT_CHECK)
+	$(Q)$(MAKE) $(build)=$(dtstree) $(dtstree)/$@ $(DT_YAML)
 
 PHONY += dtbs dtbs_install dtbs_check
 dtbs: include/config/kernel.release scripts_dtc
@@ -1654,6 +1660,10 @@  help:
 	@echo  '		3: more obscure warnings, can most likely be ignored'
 	@echo  '		e: warnings are being treated as errors'
 	@echo  '		Multiple levels can be combined with W=12 or W=123'
+	@$(if $(dtstree), \
+		echo '  make VALIDATE_DT=y [targets] Validate all DT processsed during the build'; \
+		echo '         This can be applied both to "dtbs" and to individual "foo.dtb" targets' ; \
+		)
 	@echo  ''
 	@echo  'Execute "make" or "make all" to build all targets marked with [*] '
 	@echo  'For further info see the ./README file'