Re: [PATCH v2 0/4] kbuild: Per arch/platform dtc warning levels

[Masahiro Yamada]

On Wed, Nov 29, 2023 at 7:25 AM Rob Herring <robh@xxxxxxxxxx> wrote:
>
> On Tue, Nov 28, 2023 at 6:03 AM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:
> >
> > On Mon, Nov 27, 2023 at 11:03 PM Rob Herring <robh@xxxxxxxxxx> wrote:
> > >
> > > On Thu, Nov 23, 2023 at 1:39 AM Masahiro Yamada <masahiroy@xxxxxxxxxx> wrote:
> > > >
> > > > On Thu, Nov 23, 2023 at 7:12 AM Rob Herring <robh@xxxxxxxxxx> wrote:
> > > > >
> > > > > This series adds support to set the dtc extra warning level on a per
> > > > > arch or per platform (directory really) basis.
> > > > >
> > > > > The first version of this was just a simple per directory override for
> > > > > Samsung platforms, but Conor asked to be able to do this for all of
> > > > > riscv.
> > > > >
> > > > > For merging, either I can take the whole thing or the riscv and samsung
> > > > > patches can go via their normal trees. The added variable will have no
> > > > > effect until merged with patch 2.
> > > > >
> > > > > v1:
> > > > >  - https://lore.kernel.org/all/20231116211739.3228239-1-robh@xxxxxxxxxx/
> > > > >
> > > > > Signed-off-by: Rob Herring <robh@xxxxxxxxxx>
> > > > > ---
> > > >
> > > >
> > > > There were some attempts in the past to enable W=1 in particular subsystems,
> > > > so here is a similar comment.
> > > >
> > > > Adding a new warning flag to W=1 is always safe without doing any compile test.
> > > >
> > > > With this series, it would not be true any more because a new warning in W=1
> > > > would potentially break riscv/samsung platforms.
> > >
> > > The difference here is the people potentially adding warnings are also
> > > the ones ensuring no warnings.
> > >
> > > > Linus requires a clean build (i.e. zero warning) when W= option is not given.
> > >
> > > Linus doesn't build any of this AFAICT. We are not always warning free
> > > for W=0 with dtbs.
> >
> >
> >
> > Does it mean, you can enable all warnings by default?
>
> No, Linus might not care, but others (me) do. The whole point of not
> allowing warnings is the same. Get to zero warnings so any new
> warnings stand out. We now have some subset of platforms which are
> warning free and want warnings enabled by default to keep them that
> way. How do you suggest we do that?





You may not like it, but an alternative solution could be,
hard-code extra warning flags.


In my compile-tests, Samsung platform is not W=1 clean yet.
I see -Wunit_address_vs_reg, -Wsimple_bus_reg,
-Wunique_unit_address_if_enabled warnings.

I do not see anything else, so you can add the following three
flags to keep it warning-free.






diff --git a/arch/arm/boot/dts/samsung/Makefile
b/arch/arm/boot/dts/samsung/Makefile
index 7becf36656b1..1e15784ec51f 100644
--- a/arch/arm/boot/dts/samsung/Makefile
+++ b/arch/arm/boot/dts/samsung/Makefile
@@ -1,4 +1,10 @@
 # SPDX-License-Identifier: GPL-2.0
+
+dtcflags := \
+       -Wavoid_unnecessary_addr_size \
+       -Walias_paths \
+       -Wgraph_child_address
+
 dtb-$(CONFIG_ARCH_EXYNOS3) += \
        exynos3250-artik5-eval.dtb \
        exynos3250-monk.dtb \
diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index 1a965fe68e01..aa5a5fc39cec 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -362,6 +362,9 @@ DTC_FLAGS += -Wnode_name_chars_strict \
        -Wunique_unit_address
 endif

+# per-directory flags
+DTC_FLAGS += $(dtcflags)
+# per-file flags
 DTC_FLAGS += $(DTC_FLAGS_$(basetarget))

 # Set -@ if the target is a base DTB that overlay is applied onto







>
> I understand your point on W=1 in general, but I think it just doesn't
> apply in this case. In general,
> someone may be testing a new compiler and there's some new warning to
> enable, so they add it to W=1. They are working independently of any
> subsystem (and Linus) and introducing new warnings would be a burden
> to fix and a problem to leave. For DT, it is a bit different as adding
> new warnings, updating dtc version, and selecting warnings to enable
> are pretty much all done together.
> Plus, schema warnings have pretty
> much superseded dtc warnings. If we do add new warnings which can't be
> fixed up front, then we could still only enable the warning for W=1
> from the command line. Something like this on top of this series:
>
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index 53a74e53e0ca..41307c6e1fee 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -341,6 +341,10 @@ quiet_cmd_gzip = GZIP    $@
>  # ---------------------------------------------------------------------------
>  DTC ?= $(objtree)/scripts/dtc/dtc
>
> +ifeq ($(findstring 1,$(KBUILD_EXTRA_WARN)),)
> +DTC_FLAGS += -Wno-some_new_warning_we_need_off_globally
> +endif
> +

Hmm. Tricky, but works.

KBUILD_EXTRA_WARN_DTC=1 is weaker than KBUILD_EXTRA_WARN=1




>  KBUILD_EXTRA_WARN_DTC += $(KBUILD_EXTRA_WARN)
>
>  # Disable noisy checks by default
>
--
Best Regards
Masahiro Yamada