This patch introduces a new build flag 'K=1' which controls whether kerneldoc
warnings should be issued, separating them from the compiler warnings that W=
controls.
Signed-off-by: Valdis Kletnieks <[email protected]>
diff --git a/Makefile b/Makefile
index 29abe44ada91..b1c0f9484a66 100644
--- a/Makefile
+++ b/Makefile
@@ -1605,6 +1605,7 @@ PHONY += help
@echo ' (sparse by default)'
@echo ' make C=2 [targets] Force check of all c source with $$CHECK'
@echo ' make RECORDMCOUNT_WARN=1 [targets] Warn about ignored mcount sections'
+ @echo ' make K=1 [targets] Warn about problems in kerneldoc comments'
@echo ' make W=n [targets] Enable extra build checks, n=1,2,3 where'
@echo ' 1: warnings which may be relevant and do not occur too often'
@echo ' 2: warnings which occur quite often but may still be relevant'
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 2e8810b7e5ed..9bcb77f5a5f1 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -100,7 +100,7 @@ else ifeq ($(KBUILD_CHECKSRC),2)
cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(c_flags) $<
endif
-ifneq ($(KBUILD_EXTRA_WARN),)
+ifneq ($(KBUILD_KDOC_WARN),)
cmd_checkdoc = $(srctree)/scripts/kernel-doc -none $<
endif
diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 4aea7cf71d11..3fd5881c91b0 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -17,6 +17,12 @@ endif
export KBUILD_EXTRA_WARN
+ifeq ("$(origin K)", "command line")
+ KBUILD_KDOC_WARN := $(K)
+endif
+
+export KBUILD_KDOC_WARN
+
#
# W=1 - warnings which may be relevant and do not occur too often
#
On Sat, 2020-06-20 at 14:52 -0400, Valdis Klētnieks wrote:
> This patch introduces a new build flag 'K=1' which controls whether kerneldoc
> warnings should be issued, separating them from the compiler warnings that W=
> controls.
[]
> diff --git a/Makefile b/Makefile
[]
> @@ -1605,6 +1605,7 @@ PHONY += help
> @echo ' (sparse by default)'
> @echo ' make C=2 [targets] Force check of all c source with $$CHECK'
> @echo ' make RECORDMCOUNT_WARN=1 [targets] Warn about ignored mcount sections'
> + @echo ' make K=1 [targets] Warn about problems in kerneldoc comments'
Seems sensible. Thanks.
On Sun, Jun 21, 2020 at 3:52 AM Valdis Klētnieks
<[email protected]> wrote:
>
> This patch introduces a new build flag 'K=1' which controls whether kerneldoc
> warnings should be issued, separating them from the compiler warnings that W=
> controls.
I do not understand why this change is needed.
IIRC, our goal was to enable this check by default.
https://patchwork.kernel.org/patch/10030521/
but there are so many warnings.
Meanwhile, this is checked only when W= is given
because 0-day bot tests with W=1 to
block new kerneldoc warnings.
K=1 ? Do people need to learn this new switch?
> Signed-off-by: Valdis Kletnieks <[email protected]>
>
> diff --git a/Makefile b/Makefile
> index 29abe44ada91..b1c0f9484a66 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1605,6 +1605,7 @@ PHONY += help
> @echo ' (sparse by default)'
> @echo ' make C=2 [targets] Force check of all c source with $$CHECK'
> @echo ' make RECORDMCOUNT_WARN=1 [targets] Warn about ignored mcount sections'
> + @echo ' make K=1 [targets] Warn about problems in kerneldoc comments'
> @echo ' make W=n [targets] Enable extra build checks, n=1,2,3 where'
> @echo ' 1: warnings which may be relevant and do not occur too often'
> @echo ' 2: warnings which occur quite often but may still be relevant'
> diff --git a/scripts/Makefile.build b/scripts/Makefile.build
> index 2e8810b7e5ed..9bcb77f5a5f1 100644
> --- a/scripts/Makefile.build
> +++ b/scripts/Makefile.build
> @@ -100,7 +100,7 @@ else ifeq ($(KBUILD_CHECKSRC),2)
> cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(c_flags) $<
> endif
>
> -ifneq ($(KBUILD_EXTRA_WARN),)
> +ifneq ($(KBUILD_KDOC_WARN),)
> cmd_checkdoc = $(srctree)/scripts/kernel-doc -none $<
> endif
>
> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
> index 4aea7cf71d11..3fd5881c91b0 100644
> --- a/scripts/Makefile.extrawarn
> +++ b/scripts/Makefile.extrawarn
> @@ -17,6 +17,12 @@ endif
>
> export KBUILD_EXTRA_WARN
>
> +ifeq ("$(origin K)", "command line")
> + KBUILD_KDOC_WARN := $(K)
> +endif
> +
> +export KBUILD_KDOC_WARN
> +
> #
> # W=1 - warnings which may be relevant and do not occur too often
> #
>
>
--
Best Regards
Masahiro Yamada
On Mon, 22 Jun 2020 14:10:13 +0900, Masahiro Yamada said:
> > This patch introduces a new build flag 'K=1' which controls whether kerneldoc
> > warnings should be issued, separating them from the compiler warnings that W=
> > controls.
> I do not understand why this change is needed.
> IIRC, our goal was to enable this check by default.
> https://patchwork.kernel.org/patch/10030521/
> but there are so many warnings.
So are we getting any closer to actually achieving that goal?
I've done a fair number of cleanup patches to make the kernel
safe(r) to build with W=1, but there's still quite the pile.
And actually, if you want people to actually fix up the kerneldoc
issues, making it easier helps the chances of getting patches. If
somebody is in the mood to do kerneldoc clean-ups, having an easy
way to get just the kerneldoc messages rather than having to find
them mixed in with all the rest helps...
So I ran some numbers...
A plain "make" for an arm allmodconfig weighs in at 40,184 lines.
Building with K=1 produces 10,358 additional lines of output - that's what
needs patching if you want the kerneldocs cleaned up.
Building with W=1 (w/ this patch) adds 155,773 lines. Not A Typo. Of those, a
whole whopping 116,699 are complaints from DTS issues, and 39,074 for all other
gcc warnings. (Though I have 2 patches that I'll send later that will knock
about 3,000 off the "all other gcc warnings" numbers).
(And for completeness, building with C=1 for sparse adds 18,936 lines that say
'CHECK', and 56,915 lines of sparse warnings)
> Meanwhile, this is checked only when W= is given
> because 0-day bot tests with W=1 to
> block new kerneldoc warnings.
Looking at the numbers, I really need to say "So how is that working out for
us, anyhow?"
In particular, is it just flagging them, or do we have an actual procedure that
stops patches from being accepted if they add new kerneldoc warnings?
Another issue that needs to be considered is how high-quality a fix for a
kerneldoc warning is. Getting rid of a warning by adding a comment line that
says the 3rd parameter is a pointer to a 'struct wombat' does nobody any good
if looking at the formal parameter list clearly states that the third parameter
is a 'struct wombat *foo'. Heck, I could probably create a Perl script that
automates that level of fixing.
But making an *informative* comment requires doing a bunch of research so that
you understand why *this* struct wombat is the one we care about (and whether
we care *so* much that somebody better be holding a lock....)
> K=1 ? Do people need to learn this new switch?
On Tue, Jun 23, 2020 at 11:27 AM Valdis Klētnieks
<[email protected]> wrote:
>
> On Mon, 22 Jun 2020 14:10:13 +0900, Masahiro Yamada said:
>
> > > This patch introduces a new build flag 'K=1' which controls whether kerneldoc
> > > warnings should be issued, separating them from the compiler warnings that W=
> > > controls.
>
> > I do not understand why this change is needed.
>
> > IIRC, our goal was to enable this check by default.
> > https://patchwork.kernel.org/patch/10030521/
> > but there are so many warnings.
>
> So are we getting any closer to actually achieving that goal?
> I've done a fair number of cleanup patches to make the kernel
> safe(r) to build with W=1, but there's still quite the pile.
>
> And actually, if you want people to actually fix up the kerneldoc
> issues, making it easier helps the chances of getting patches. If
> somebody is in the mood to do kerneldoc clean-ups, having an easy
> way to get just the kerneldoc messages rather than having to find
> them mixed in with all the rest helps...
>
> So I ran some numbers...
>
> A plain "make" for an arm allmodconfig weighs in at 40,184 lines.
>
> Building with K=1 produces 10,358 additional lines of output - that's what
> needs patching if you want the kerneldocs cleaned up.
>
> Building with W=1 (w/ this patch) adds 155,773 lines. Not A Typo. Of those, a
> whole whopping 116,699 are complaints from DTS issues, and 39,074 for all other
> gcc warnings. (Though I have 2 patches that I'll send later that will knock
> about 3,000 off the "all other gcc warnings" numbers).
>
> (And for completeness, building with C=1 for sparse adds 18,936 lines that say
> 'CHECK', and 56,915 lines of sparse warnings)
>
> > Meanwhile, this is checked only when W= is given
> > because 0-day bot tests with W=1 to
> > block new kerneldoc warnings.
>
> Looking at the numbers, I really need to say "So how is that working out for
> us, anyhow?"
>
> In particular, is it just flagging them, or do we have an actual procedure that
> stops patches from being accepted if they add new kerneldoc warnings?
>
> Another issue that needs to be considered is how high-quality a fix for a
> kerneldoc warning is. Getting rid of a warning by adding a comment line that
> says the 3rd parameter is a pointer to a 'struct wombat' does nobody any good
> if looking at the formal parameter list clearly states that the third parameter
> is a 'struct wombat *foo'. Heck, I could probably create a Perl script that
> automates that level of fixing.
>
> But making an *informative* comment requires doing a bunch of research so that
> you understand why *this* struct wombat is the one we care about (and whether
> we care *so* much that somebody better be holding a lock....)
>
> > K=1 ? Do people need to learn this new switch?
In my understanding, the intel 0-day bot tests
with W=1, and sends an alert for new warnings.
For example,
https://lkml.org/lkml/2020/6/27/328
This is a warning with W=1 builds.
So, new W=1 warnings will be blocked
(unless people ignore the 0-day bot).
Actually, the number of kernel-doc warnings are decreasing,
but we still have so many.
The progress depends on how much effort people will make.
It does not make much difference
if you separate out particular warnings
into a different switch.
One more thing, there is no need to add a new syntax
to separate out kernel-doc warnings.
W=1, W=2, W=3 can be combined.
Adding a new warning group, W=d, is enough.
The following should work.
diff --git a/Makefile b/Makefile
index ac2c61c37a73..1e8428ca8f10 100644
--- a/Makefile
+++ b/Makefile
@@ -1597,11 +1597,12 @@ help:
@echo ' (sparse by default)'
@echo ' make C=2 [targets] Force check of all c source with $$CHECK'
@echo ' make RECORDMCOUNT_WARN=1 [targets] Warn about
ignored mcount sections'
- @echo ' make W=n [targets] Enable extra build checks, n=1,2,3 where'
+ @echo ' make W=n [targets] Enable extra build checks,
n=1,2,3,d where'
@echo ' 1: warnings which may be relevant and
do not occur too often'
@echo ' 2: warnings which occur quite often
but may still be relevant'
@echo ' 3: more obscure warnings, can most
likely be ignored'
- @echo ' Multiple levels can be combined with
W=12 or W=123'
+ @echo ' d: warnings from kernel-doc'
+ @echo ' Multiple levels can be combined with
W=12 or W=123d'
@echo ''
@echo 'Execute "make" or "make all" to build all targets
marked with [*] '
@echo 'For further info see the ./README file'
diff --git a/scripts/Makefile.build b/scripts/Makefile.build
index 2e8810b7e5ed..1781b6ff16f0 100644
--- a/scripts/Makefile.build
+++ b/scripts/Makefile.build
@@ -100,7 +100,7 @@ else ifeq ($(KBUILD_CHECKSRC),2)
cmd_force_checksrc = $(CHECK) $(CHECKFLAGS) $(c_flags) $<
endif
-ifneq ($(KBUILD_EXTRA_WARN),)
+ifeq ($(KBUILD_EXTRA_WARN),d)
cmd_checkdoc = $(srctree)/scripts/kernel-doc -none $<
endif
But, you need to ask the 0-day maintainers to
change the flag W=1 into W=1d to keep the
current test coverage.
--
Best Regards
Masahiro Yamada