[PATCH 1/2] dt-bindings: Fix command line length limit calling dt-mk-schema

[PATCH 1/2] dt-bindings: Fix command line length limit calling dt-mk-schema

[Rob Herring]

As the number of schemas has increased, we're starting to hit the error
"execvp: /bin/sh: Argument list too long". This is due to passing all the
schema files on the command line to dt-mk-schema. It currently is only
with out of tree builds and is intermittent depending on the file path
lengths.

Commit 2ba06cd8565b ("kbuild: Always validate DT binding examples") made
hitting this proplem more likely since the example validation now always
gets the full list of schemas.

Fix this by putting the schema file list into a temp file and using xargs.

Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Masahiro Yamada <masahiroy@kernel.org>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/.gitignore | 2 +-
 Documentation/devicetree/bindings/Makefile   | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/.gitignore b/Documentation/devicetree/bindings/.gitignore
index 5c6d8ea1a09c..0a6aef915fa4 100644
--- a/Documentation/devicetree/bindings/.gitignore
+++ b/Documentation/devicetree/bindings/.gitignore
@@ -1,3 +1,3 @@
 # SPDX-License-Identifier: GPL-2.0-only
 *.example.dts
-processed-schema*.yaml
+processed-schema*.yaml*
diff --git a/Documentation/devicetree/bindings/Makefile b/Documentation/devicetree/bindings/Makefile
index 1df680d07461..1c1cad860b7c 100644
--- a/Documentation/devicetree/bindings/Makefile
+++ b/Documentation/devicetree/bindings/Makefile
@@ -14,7 +14,8 @@ $(obj)/%.example.dts: $(src)/%.yaml FORCE
 DT_TMP_SCHEMA := $(obj)/processed-schema-examples.yaml
 
 quiet_cmd_mk_schema = SCHEMA  $@
-      cmd_mk_schema = $(DT_MK_SCHEMA) $(DT_MK_SCHEMA_FLAGS) -o $@ $(real-prereqs)
+      cmd_mk_schema = $(file >$@.tmp, $(real-prereqs)) \
+                      cat $@.tmp | xargs $(DT_MK_SCHEMA) $(DT_MK_SCHEMA_FLAGS) -o $@
 
 DT_DOCS = $(addprefix $(src)/, \
 	$(shell \
-- 
2.20.1

[PATCH 2/2] dt-bindings: Re-enable core schemas for dtbs_check

[Rob Herring]

In commit 2ba06cd8565b ("kbuild: Always validate DT binding examples"),
the core schemas (from dtschema repo) were inadvertently disabled for
dtbs_checks. Re-enable them.

Fixes: 2ba06cd8565b ("kbuild: Always validate DT binding examples")
Cc: Masahiro Yamada <masahiroy@kernel.org>
Signed-off-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/Makefile | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/Makefile b/Documentation/devicetree/bindings/Makefile
index 1c1cad860b7c..c66e8329a645 100644
--- a/Documentation/devicetree/bindings/Makefile
+++ b/Documentation/devicetree/bindings/Makefile
@@ -2,6 +2,7 @@
 DT_DOC_CHECKER ?= dt-doc-validate
 DT_EXTRACT_EX ?= dt-extract-example
 DT_MK_SCHEMA ?= dt-mk-schema
+DT_MK_SCHEMA_USERONLY_FLAG := $(if $(DT_SCHEMA_FILES), -u)
 
 quiet_cmd_chk_binding = CHKDT   $(patsubst $(srctree)/%,%,$<)
       cmd_chk_binding = $(DT_DOC_CHECKER) -u $(srctree)/$(src) $< ; \
@@ -38,7 +39,7 @@ override DTC_FLAGS := \
 $(obj)/processed-schema-examples.yaml: $(DT_DOCS) FORCE
 	$(call if_changed,mk_schema)
 
-$(obj)/processed-schema.yaml: DT_MK_SCHEMA_FLAGS := -u
+$(obj)/processed-schema.yaml: DT_MK_SCHEMA_FLAGS := $(DT_MK_SCHEMA_USERONLY_FLAG)
 $(obj)/processed-schema.yaml: $(DT_SCHEMA_FILES) FORCE
 	$(call if_changed,mk_schema)
 
-- 
2.20.1

Re: [PATCH 1/2] dt-bindings: Fix command line length limit calling dt-mk-schema

[Laurent Pinchart]

Hi Rob,

Thank you for the patch.

On Tue, Apr 21, 2020 at 04:20:03PM -0500, Rob Herring wrote:
> As the number of schemas has increased, we're starting to hit the error
> "execvp: /bin/sh: Argument list too long". This is due to passing all the
> schema files on the command line to dt-mk-schema. It currently is only
> with out of tree builds and is intermittent depending on the file path
> lengths.
> 
> Commit 2ba06cd8565b ("kbuild: Always validate DT binding examples") made
> hitting this proplem more likely since the example validation now always
> gets the full list of schemas.
> 
> Fix this by putting the schema file list into a temp file and using xargs.
> 
> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Signed-off-by: Rob Herring <robh@kernel.org>

Quite a bit slower than v5.6 when passing DT_SCHEMA_FILES, but
reasonable, and working now :-)

Tested-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  Documentation/devicetree/bindings/.gitignore | 2 +-
>  Documentation/devicetree/bindings/Makefile   | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/.gitignore b/Documentation/devicetree/bindings/.gitignore
> index 5c6d8ea1a09c..0a6aef915fa4 100644
> --- a/Documentation/devicetree/bindings/.gitignore
> +++ b/Documentation/devicetree/bindings/.gitignore
> @@ -1,3 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  *.example.dts
> -processed-schema*.yaml
> +processed-schema*.yaml*
> diff --git a/Documentation/devicetree/bindings/Makefile b/Documentation/devicetree/bindings/Makefile
> index 1df680d07461..1c1cad860b7c 100644
> --- a/Documentation/devicetree/bindings/Makefile
> +++ b/Documentation/devicetree/bindings/Makefile
> @@ -14,7 +14,8 @@ $(obj)/%.example.dts: $(src)/%.yaml FORCE
>  DT_TMP_SCHEMA := $(obj)/processed-schema-examples.yaml
>  
>  quiet_cmd_mk_schema = SCHEMA  $@
> -      cmd_mk_schema = $(DT_MK_SCHEMA) $(DT_MK_SCHEMA_FLAGS) -o $@ $(real-prereqs)
> +      cmd_mk_schema = $(file >$@.tmp, $(real-prereqs)) \
> +                      cat $@.tmp | xargs $(DT_MK_SCHEMA) $(DT_MK_SCHEMA_FLAGS) -o $@
>  
>  DT_DOCS = $(addprefix $(src)/, \
>  	$(shell \

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] dt-bindings: Fix command line length limit calling dt-mk-schema

[Rob Herring]

On Tue, Apr 21, 2020 at 6:37 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Rob,
>
> Thank you for the patch.
>
> On Tue, Apr 21, 2020 at 04:20:03PM -0500, Rob Herring wrote:
> > As the number of schemas has increased, we're starting to hit the error
> > "execvp: /bin/sh: Argument list too long". This is due to passing all the
> > schema files on the command line to dt-mk-schema. It currently is only
> > with out of tree builds and is intermittent depending on the file path
> > lengths.
> >
> > Commit 2ba06cd8565b ("kbuild: Always validate DT binding examples") made
> > hitting this proplem more likely since the example validation now always
> > gets the full list of schemas.
> >
> > Fix this by putting the schema file list into a temp file and using xargs.
> >
> > Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > Signed-off-by: Rob Herring <robh@kernel.org>
>
> Quite a bit slower than v5.6 when passing DT_SCHEMA_FILES, but
> reasonable, and working now :-)

That's expected. It's validating with ~700 vs. 1 schema. The problem
was folks only checking with DT_SCHEMA_FILES set, but a new schema can
affect another example or existing schemas (including core schema) may
fail on the new example.

Rob

Re: [PATCH 1/2] dt-bindings: Fix command line length limit calling dt-mk-schema

[Masahiro Yamada]

Hi Rob,


On Wed, Apr 22, 2020 at 6:20 AM Rob Herring <robh@kernel.org> wrote:
>
> As the number of schemas has increased, we're starting to hit the error
> "execvp: /bin/sh: Argument list too long". This is due to passing all the
> schema files on the command line to dt-mk-schema. It currently is only
> with out of tree builds and is intermittent depending on the file path
> lengths.
>
> Commit 2ba06cd8565b ("kbuild: Always validate DT binding examples") made
> hitting this proplem more likely since the example validation now always
> gets the full list of schemas.
>
> Fix this by putting the schema file list into a temp file and using xargs.
>
> Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Signed-off-by: Rob Herring <robh@kernel.org>
> ---
>  Documentation/devicetree/bindings/.gitignore | 2 +-
>  Documentation/devicetree/bindings/Makefile   | 3 ++-
>  2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/.gitignore b/Documentation/devicetree/bindings/.gitignore
> index 5c6d8ea1a09c..0a6aef915fa4 100644
> --- a/Documentation/devicetree/bindings/.gitignore
> +++ b/Documentation/devicetree/bindings/.gitignore
> @@ -1,3 +1,3 @@
>  # SPDX-License-Identifier: GPL-2.0-only
>  *.example.dts
> -processed-schema*.yaml
> +processed-schema*.yaml*
> diff --git a/Documentation/devicetree/bindings/Makefile b/Documentation/devicetree/bindings/Makefile
> index 1df680d07461..1c1cad860b7c 100644
> --- a/Documentation/devicetree/bindings/Makefile
> +++ b/Documentation/devicetree/bindings/Makefile
> @@ -14,7 +14,8 @@ $(obj)/%.example.dts: $(src)/%.yaml FORCE
>  DT_TMP_SCHEMA := $(obj)/processed-schema-examples.yaml
>
>  quiet_cmd_mk_schema = SCHEMA  $@
> -      cmd_mk_schema = $(DT_MK_SCHEMA) $(DT_MK_SCHEMA_FLAGS) -o $@ $(real-prereqs)
> +      cmd_mk_schema = $(file >$@.tmp, $(real-prereqs)) \
> +                      cat $@.tmp | xargs $(DT_MK_SCHEMA) $(DT_MK_SCHEMA_FLAGS) -o $@
>


The built-in function $(file ...) is supported on GNU Make 4.0 or later.
The current minimal version is GNU Make 3.81.

If you want to use this function, you must update
Documentation/process/changes.rst first.

I am pretty sure some conservative distros
still stick to GNU Make 3.8*
but I am open to raising the minimal version
if it is useful.



But, does this code work in the first place?

When a very long command is given, xargs
splits it into smaller chunks, and invokes
the command multiple times, right?


So, it boils down to this question:

Are the following two commands work equivalently?


"dt-mk-schema -o processed-schema-examples.yaml foo.yaml &&
dt-mk-schema -o processed-schema-examples.yaml bar.yaml &&
dt-mk-schema -o processed-schema-examples.yaml baz.yaml"

"dt-mk-schema -o processed-schema-examples.yaml foo.yaml bar.yaml baz.yaml"


I think the answer is no.

I confirmed the produced processed-schema-examples.yaml is broken.

In a normal case, it is more than 50000 lines,
but if xargs split the command, it is much smaller.



masahiro@oscar:~/ref/linux$ make -j24  O=foo/bar dt_binding_check
masahiro@oscar:~/ref/linux$ wc
foo/bar/Documentation/devicetree/bindings/processed-schema-examples.yaml
  51461  115967 1516186
foo/bar/Documentation/devicetree/bindings/processed-schema-examples.yaml




masahiro@oscar:~/ref/this/is/a/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/src/path/to/linux$
make -j24 O=foo/bar dt_binding_check
masahiro@oscar:~/ref/this/is/a/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/long/src/path/to/linux$
wc  foo/bar/Documentation/devicetree/bindings/processed-schema-examples.yaml
  9625  21510 291993
foo/bar/Documentation/devicetree/bindings/processed-schema-examples.yaml



Can dt-mk-schema read the list of schema files
by other means?


>  DT_DOCS = $(addprefix $(src)/, \
>         $(shell \
> --
> 2.20.1
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] dt-bindings: Re-enable core schemas for dtbs_check

[Masahiro Yamada]

On Wed, Apr 22, 2020 at 6:20 AM Rob Herring <robh@kernel.org> wrote:
>
> In commit 2ba06cd8565b ("kbuild: Always validate DT binding examples"),
> the core schemas (from dtschema repo) were inadvertently disabled for
> dtbs_checks. Re-enable them.
>
> Fixes: 2ba06cd8565b ("kbuild: Always validate DT binding examples")
> Cc: Masahiro Yamada <masahiroy@kernel.org>
> Signed-off-by: Rob Herring <robh@kernel.org>


Reviewed-by: Masahiro Yamada <masahiroy@kernel.org>


> ---
>  Documentation/devicetree/bindings/Makefile | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/Makefile b/Documentation/devicetree/bindings/Makefile
> index 1c1cad860b7c..c66e8329a645 100644
> --- a/Documentation/devicetree/bindings/Makefile
> +++ b/Documentation/devicetree/bindings/Makefile
> @@ -2,6 +2,7 @@
>  DT_DOC_CHECKER ?= dt-doc-validate
>  DT_EXTRACT_EX ?= dt-extract-example
>  DT_MK_SCHEMA ?= dt-mk-schema
> +DT_MK_SCHEMA_USERONLY_FLAG := $(if $(DT_SCHEMA_FILES), -u)
>
>  quiet_cmd_chk_binding = CHKDT   $(patsubst $(srctree)/%,%,$<)
>        cmd_chk_binding = $(DT_DOC_CHECKER) -u $(srctree)/$(src) $< ; \
> @@ -38,7 +39,7 @@ override DTC_FLAGS := \
>  $(obj)/processed-schema-examples.yaml: $(DT_DOCS) FORCE
>         $(call if_changed,mk_schema)
>
> -$(obj)/processed-schema.yaml: DT_MK_SCHEMA_FLAGS := -u
> +$(obj)/processed-schema.yaml: DT_MK_SCHEMA_FLAGS := $(DT_MK_SCHEMA_USERONLY_FLAG)
>  $(obj)/processed-schema.yaml: $(DT_SCHEMA_FILES) FORCE
>         $(call if_changed,mk_schema)
>
> --
> 2.20.1
>


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 1/2] dt-bindings: Fix command line length limit calling dt-mk-schema

[Rob Herring]

On Wed, Apr 22, 2020 at 12:59 AM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> Hi Rob,
>
>
> On Wed, Apr 22, 2020 at 6:20 AM Rob Herring <robh@kernel.org> wrote:
> >
> > As the number of schemas has increased, we're starting to hit the error
> > "execvp: /bin/sh: Argument list too long". This is due to passing all the
> > schema files on the command line to dt-mk-schema. It currently is only
> > with out of tree builds and is intermittent depending on the file path
> > lengths.
> >
> > Commit 2ba06cd8565b ("kbuild: Always validate DT binding examples") made
> > hitting this proplem more likely since the example validation now always
> > gets the full list of schemas.
> >
> > Fix this by putting the schema file list into a temp file and using xargs.
> >
> > Reported-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Cc: Masahiro Yamada <masahiroy@kernel.org>
> > Signed-off-by: Rob Herring <robh@kernel.org>
> > ---
> >  Documentation/devicetree/bindings/.gitignore | 2 +-
> >  Documentation/devicetree/bindings/Makefile   | 3 ++-
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/.gitignore b/Documentation/devicetree/bindings/.gitignore
> > index 5c6d8ea1a09c..0a6aef915fa4 100644
> > --- a/Documentation/devicetree/bindings/.gitignore
> > +++ b/Documentation/devicetree/bindings/.gitignore
> > @@ -1,3 +1,3 @@
> >  # SPDX-License-Identifier: GPL-2.0-only
> >  *.example.dts
> > -processed-schema*.yaml
> > +processed-schema*.yaml*
> > diff --git a/Documentation/devicetree/bindings/Makefile b/Documentation/devicetree/bindings/Makefile
> > index 1df680d07461..1c1cad860b7c 100644
> > --- a/Documentation/devicetree/bindings/Makefile
> > +++ b/Documentation/devicetree/bindings/Makefile
> > @@ -14,7 +14,8 @@ $(obj)/%.example.dts: $(src)/%.yaml FORCE
> >  DT_TMP_SCHEMA := $(obj)/processed-schema-examples.yaml
> >
> >  quiet_cmd_mk_schema = SCHEMA  $@
> > -      cmd_mk_schema = $(DT_MK_SCHEMA) $(DT_MK_SCHEMA_FLAGS) -o $@ $(real-prereqs)
> > +      cmd_mk_schema = $(file >$@.tmp, $(real-prereqs)) \
> > +                      cat $@.tmp | xargs $(DT_MK_SCHEMA) $(DT_MK_SCHEMA_FLAGS) -o $@
> >
>
>
> The built-in function $(file ...) is supported on GNU Make 4.0 or later.
> The current minimal version is GNU Make 3.81.
>
> If you want to use this function, you must update
> Documentation/process/changes.rst first.
>
> I am pretty sure some conservative distros
> still stick to GNU Make 3.8*
> but I am open to raising the minimal version
> if it is useful.

I'd like to avoid that. I've come up with another solution.

> But, does this code work in the first place?
>
> When a very long command is given, xargs
> splits it into smaller chunks, and invokes
> the command multiple times, right?
>
>
> So, it boils down to this question:
>
> Are the following two commands work equivalently?
>
>
> "dt-mk-schema -o processed-schema-examples.yaml foo.yaml &&
> dt-mk-schema -o processed-schema-examples.yaml bar.yaml &&
> dt-mk-schema -o processed-schema-examples.yaml baz.yaml"
>
> "dt-mk-schema -o processed-schema-examples.yaml foo.yaml bar.yaml baz.yaml"
>
>
> I think the answer is no.
>
> I confirmed the produced processed-schema-examples.yaml is broken.

Indeed. We need to output to stdout and append the file instead.
Sending a v2 out now.

[...]

> Can dt-mk-schema read the list of schema files
> by other means?

Not currently. Happy to add it, but I'd rather not require everyone
update right away.

Rob