Message-ID: <CAK7LNARZUkkyjUZCp5cXs_ROz+KzJPPGkoFpTKFXJaixXGofkw@mail.gmail.com>
Date:   Fri, 16 Sep 2022 10:09:53 +0900
From:   Masahiro Yamada <masahiroy@...nel.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Michal Marek <michal.lkml@...kovi.net>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Linux Kbuild mailing list <linux-kbuild@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>
Subject: Re: [PATCH] kbuild: take into account DT_SCHEMA_FILES changes while
 checking dtbs

On Thu, Sep 15, 2022 at 8:40 PM Dmitry Baryshkov
<dmitry.baryshkov@...aro.org> wrote:
>
> On Thu, 15 Sept 2022 at 07:51, Masahiro Yamada <masahiroy@...nel.org> wrote:
> >
> > On Sun, Sep 11, 2022 at 2:36 AM Dmitry Baryshkov
> >
> > <dmitry.baryshkov@...aro.org> wrote:
> > >
> > > On Thu, 4 Aug 2022 at 18:27, Rob Herring <robh+dt@...nel.org> wrote:
> > > >
> > > > On Fri, Jul 29, 2022 at 1:46 AM Dmitry Baryshkov
> > > > <dmitry.baryshkov@...aro.org> wrote:
> > > > >
> > > > > On Fri, 29 Jul 2022 at 10:05, Masahiro Yamada <masahiroy@...nel.org> wrote:
> > > > > >
> > > > > > On Fri, Jul 29, 2022 at 3:53 PM Dmitry Baryshkov
> > > > > > <dmitry.baryshkov@...aro.org> wrote:
> > > > > > >
> > > > > > > On Fri, 29 Jul 2022 at 08:55, Masahiro Yamada <masahiroy@...nel.org> wrote:
> > > > > > > >
> > > > > > > > On Thu, Jul 28, 2022 at 2:36 AM Rob Herring <robh+dt@...nel.org> wrote:
> > > > > > > > >
> > > > > > > > > On Wed, Jul 27, 2022 at 4:06 AM Dmitry Baryshkov
> > > > > > > > > <dmitry.baryshkov@...aro.org> wrote:
> > > > > > > > > >
> > > > > > > > > > It is useful to be able to recheck dtbs files against a limited set of
> > > > > > > > > > DT schema files. This can be accomplished by using differnt
> > > > > > > > > > DT_SCHEMA_FILES argument values while rerunning make dtbs_check. However
> > > > > > > > > > for some reason if_changed_rule doesn't pick up the rule_dtc changes
> > > > > > > > > > (and doesn't retrigger the build).
> > > > > > > > > >
> > > > > > > > > > Fix this by changing if_changed_rule to if_changed_dep and squashing DTC
> > > > > > > > > > and dt-validate into a single new command. Then if_changed_dep triggers
> > > > > > > > > > on DT_SCHEMA_FILES changes and reruns the build/check.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> > > > > > > > > > ---
> > > > > > > > > >  scripts/Makefile.lib | 14 ++++++--------
> > > > > > > > > >  1 file changed, 6 insertions(+), 8 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > > > > > > > > > index c88b98b5dc44..3df470289382 100644
> > > > > > > > > > --- a/scripts/Makefile.lib
> > > > > > > > > > +++ b/scripts/Makefile.lib
> > > > > > > > > > @@ -383,17 +383,15 @@ 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
> > > > > > > > > >
> > > > > > > > > > -quiet_cmd_dtb_check =  CHECK   $@
> > > > > > > > > > -      cmd_dtb_check =  $(DT_CHECKER) $(DT_CHECKER_FLAGS) -u $(srctree)/$(DT_BINDING_DIR) -p $(DT_TMP_SCHEMA) $@ || true
> > > > > > > > > > +quiet_cmd_dtb =        DTC/CHECK   $@
> > > > > > > > >
> > > > > > > > > This is supposed to be 7 chars or less. DTCCHK or DTC_CHK perhaps. Or
> > > > > > > > > always do just 'DTC'. I can fixup when applying.
> > > > > > > > >
> > > > > > > > > I'll give it a few days for other comments.
> > > > > > > >
> > > > > > > >
> > > > > > > >
> > > > > > > > When you change DT_SCHEMA_FILES, re-running dt-validate should be enough.
> > > > > > > > You do not need to re-run dtc.
> > > > > > > >
> > > > > > > > I guess the strangeness comes from the fact that you are trying to do the
> > > > > > > >  two different things in a single rule.
> > > > > > >
> > > > > > > The issue is that with the current rules the dt-validate isn't
> > > > > > > re-executed on DT_SCHEMA_FILES changes. Thus comes my proposal.
> > > > > >
> > > > > > Correct.
> > > > > >
> > > > > > What I said is like this.
> > > > > >
> > > > > > # touch the timestamp file, %.dtb.checked
> > > > > > $(obj)/%.dtb.checked: $(obj)/%.dtb $(DT_TMP_SCHEMA) FORCE
> > > >
> > > > Not really a fan of the thousands of files that creates. Maybe if it
> > > > was turned into something useful like a list of schemas that apply to
> > > > the dtb. IOW, a dependency list. That would speed up re-running after
> > > > a schema change. Though if a schema change created new dependencies,
> > > > that wouldn't work.
> > > >
> > > > > >         $(call if_changed_rule,dtb_check)
> > > > > >
> > > > > > $(obj)/%.dtb: $(src)/%.dts $(DTC) $FORCE
> > > > > >         $(call if_changed_rule,dtc)
> > > > > >
> > > > > > $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
> > > > > >         $(call if_changed_dep,dtc)
> > > > > >
> > > > > >
> > > > > > With the dtc/check split, we can avoid unneeded regeneration of
> > > > > > %.dtb when DT_TMP_SCHEMA or DT_SCHEMA_FILES is
> > > > > > changed.
> > > > > >
> > > > > >
> > > > > > One drawback is we track %.dtb.checked and and %.dtb separately,
> > > > > > so something like 53182e81f47d4ea0c727c49ad23cb782173ab849
> > > > > > may come back.
> > > > >
> > > > > It's up to you and Rob, but I'd really prefer a simpler solution here.
> > > > > Regenerating dtbs sounds like a minor pain compared to hacking the
> > > > > top-level Makefile again. What I really like is that if one has
> > > > > CHECK_DTBS=y (for whatever reason), he can not generate dtb without
> > > > > validation.
> > > >
> > > > I lean towards just rebuilding the dtbs. That's pretty quick and
> > > > ensures we get dtc warnings with schema warnings. In the long run, I
> > > > would like to make the schema checks not optional to run. The
> > > > impediment to doing that is lots of warnings (but not not some
> > > > platforms), adding a tool dependency, and validation time.
> > >
> > > Rob, Masahiro, do we have any conclusion here? I can change my patch,
> > > but I'd like to understand in which way I should change it.
> > > Fixing/testing yaml changes is a bit painful w/o this change.
> > >
> > > --
> > > With best wishes
> > > Dmitry
> >
> >
> >
> >
> > I am fine with the patch as long as you fix DTC/CHECK to DTC.
>
> Ack, I completely forgot about this part. Please excuse me. I'll
> change it to DTC_CHK (as suggested before) to fit into 7 chars in v2.
>
> >
> > Another idea is to re-check the schema every time,
> > like this:
>
> I think this will add the external build dependency. We tried a
> similar approach several releases ago and had to revert the change.


If you are talking about
75e895343d5a2fcbdf4cb3d31ab7492bd65925f0,
you are completely misunderstanding.

Look at my code closely.
It is how the sparse checker works with C=2.





> > diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> > index 3fb6a99e78c4..9fc9f39fb12e 100644
> > --- a/scripts/Makefile.lib
> > +++ b/scripts/Makefile.lib
> > @@ -375,13 +375,9 @@ quiet_cmd_dtb_check =      CHECK   $@
> >        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)/%.dtb: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE
> > -       $(call if_changed_rule,dtc)
> > +       $(call if_changed_dep,dtc)
> > +       $(call cmd,dtb_check)
> >
> >  $(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
> >         $(call if_changed_dep,dtc)
> >
> >
>
> --
> With best wishes
> Dmitry

--
Best Regards
Masahiro Yamada

Powered by blists - more mailing lists