[RFC PATCH net-next v2 0/2] driver/net/ethernet W=1 by default

[Andrew Lunn]

There is a movement to make the code base compile clean with W=1. Some
subsystems are already clean. In order to keep them clean, we need
developers to build new code with W=1 by default in these
subsystems. Otherwise new warnings will be added. To prove the point:

commit e666a4c668528ae1f5b8b3a2e7cb6a5be488dfbb
Merge: d0186842ec5f d0ea5cbdc286
Author: David S. Miller <da...@davemloft.net>
Date: Fri Sep 25 16:29:00 2020 -0700

Merge branch 'drivers-net-warning-clean'

Jesse Brandeburg says:

====================
make drivers/net/ethernet W=1 clean

Then 4 days later a new W=1 warning has added:

drivers/net/ethernet//chelsio/inline_crypto/ch_ktls/chcr_ktls.c: In function ‘chcr_ktls_cpl_set_tcb_rpl’:
drivers/net/ethernet//chelsio/inline_crypto/ch_ktls/chcr_ktls.c:684:22: warning: implicit conversion from ‘enum <anonymous>’ to ‘enum ch_ktls_open_state’ [-Wenum-conversion]
684 | tx_info->open_state = false;

This patchset refactors the core Makefile warning code to allow the
additional warnings W=1 adds available to any Makefile. The Ethernet
driver subsystem Makefiles then make use of this to make W=1 the
default for this subsystem.

v2:

Address the comment that we need to be able to add new W=1 compiler
flags without actually causing new warnings for builds which don't have W=1

Andrew Lunn (2):
Makefile.extrawarn: Add symbol for W=1 warnings for today
driver/net/ethernet: Sign up for W=1 as defined on 20200930

drivers/net/ethernet/Makefile | 3 +++
scripts/Makefile.extrawarn | 34 ++++++++++++++++++----------------
2 files changed, 21 insertions(+), 16 deletions(-)

--
2.28.0

[Andrew Lunn]

Make all Ethernet drivers be compiled with the equivalent of W=1
as defined today.

Signed-off-by: Andrew Lunn <and...@lunn.ch>
---
drivers/net/ethernet/Makefile | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/Makefile b/drivers/net/ethernet/Makefile
index f8f38dcb5f8a..8162b2f6ec81 100644
--- a/drivers/net/ethernet/Makefile
+++ b/drivers/net/ethernet/Makefile
@@ -3,6 +3,9 @@
# Makefile for the Linux network Ethernet device drivers.
#

+# Enable W=1, as defined on the given date
+subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930)
+
obj-$(CONFIG_NET_VENDOR_3COM) += 3com/
obj-$(CONFIG_NET_VENDOR_8390) += 8390/
obj-$(CONFIG_NET_VENDOR_ADAPTEC) += adaptec/
--
2.28.0

[Andrew Lunn]

There is a movement to try to make more and more of /drivers W=1
clean. But it will only stay clean if new warnings are quickly
detected and fixed, ideally by the developer adding the new code.

To allow subdirectories to sign up to being W=1 clean for a given
definition of W=1, export the current set of additional compile flags
using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can
then use:

subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930)

To indicate they want to W=1 warnings as defined on 20200930.

Additional warnings can be added to the W=1 definition. This will not
affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will
start appearing unless W=1 is actually added to the command
line. Developers can then take their time to fix any new W=1 warnings,
and then update to the latest KBUILD_CFLAGS_W1_<DATESTAMP> symbol.

Signed-off-by: Andrew Lunn <and...@lunn.ch>
---

scripts/Makefile.extrawarn | 34 ++++++++++++++++++----------------
1 file changed, 18 insertions(+), 16 deletions(-)

diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 95e4cdb94fe9..957dca35ae3e 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -20,24 +20,26 @@ export KBUILD_EXTRA_WARN
#
# W=1 - warnings which may be relevant and do not occur too often
#
-ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)
-
-KBUILD_CFLAGS += -Wextra -Wunused -Wno-unused-parameter
-KBUILD_CFLAGS += -Wmissing-declarations
-KBUILD_CFLAGS += -Wmissing-format-attribute
-KBUILD_CFLAGS += -Wmissing-prototypes
-KBUILD_CFLAGS += -Wold-style-definition
-KBUILD_CFLAGS += -Wmissing-include-dirs
-KBUILD_CFLAGS += $(call cc-option, -Wunused-but-set-variable)
-KBUILD_CFLAGS += $(call cc-option, -Wunused-const-variable)
-KBUILD_CFLAGS += $(call cc-option, -Wpacked-not-aligned)
-KBUILD_CFLAGS += $(call cc-option, -Wstringop-truncation)
+KBUILD_CFLAGS_W1_20200930 += -Wextra -Wunused -Wno-unused-parameter
+KBUILD_CFLAGS_W1_20200930 += -Wmissing-declarations
+KBUILD_CFLAGS_W1_20200930 += -Wmissing-format-attribute
+KBUILD_CFLAGS_W1_20200930 += -Wmissing-prototypes
+KBUILD_CFLAGS_W1_20200930 += -Wold-style-definition
+KBUILD_CFLAGS_W1_20200930 += -Wmissing-include-dirs
+KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wunused-but-set-variable)
+KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wunused-const-variable)
+KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wpacked-not-aligned)
+KBUILD_CFLAGS_W1_20200930 += $(call cc-option, -Wstringop-truncation)
# The following turn off the warnings enabled by -Wextra
-KBUILD_CFLAGS += -Wno-missing-field-initializers
-KBUILD_CFLAGS += -Wno-sign-compare
-KBUILD_CFLAGS += -Wno-type-limits
+KBUILD_CFLAGS_W1_20200930 += -Wno-missing-field-initializers
+KBUILD_CFLAGS_W1_20200930 += -Wno-sign-compare
+KBUILD_CFLAGS_W1_20200930 += -Wno-type-limits
+
+export KBUILD_CFLAGS_W1_20200930
+
+ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)

-KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN1
+KBUILD_CPPFLAGS += $(KBUILD_CFLAGS_W1_20200930) -DKBUILD_EXTRA_WARN1

else

--
2.28.0

[Nick Desaulniers]

On Wed, Sep 30, 2020 at 6:12 PM Andrew Lunn <and...@lunn.ch> wrote:
>
> There is a movement to try to make more and more of /drivers W=1
> clean. But it will only stay clean if new warnings are quickly
> detected and fixed, ideally by the developer adding the new code.
>
> To allow subdirectories to sign up to being W=1 clean for a given
> definition of W=1, export the current set of additional compile flags
> using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can
> then use:
>
> subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930)
>
> To indicate they want to W=1 warnings as defined on 20200930.
>
> Additional warnings can be added to the W=1 definition. This will not
> affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will
> start appearing unless W=1 is actually added to the command
> line. Developers can then take their time to fix any new W=1 warnings,
> and then update to the latest KBUILD_CFLAGS_W1_<DATESTAMP> symbol.

I'm not a fan of this approach. Are DATESTAMP configs just going to
keep being added? Is it going to complicate isolating the issue from a
randconfig build? If we want more things to build warning-free at
W=1, then why don't we start moving warnings from W=1 into the
default, until this is no W=1 left? That way we're cutting down on
the kernel's configuration combinatorial explosion, rather than adding
to it?

> --
> You received this message because you are subscribed to the Google Groups "Clang Built Linux" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to clang-built-li...@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/clang-built-linux/20201001011232.4050282-2-andrew%40lunn.ch.



--
Thanks,
~Nick Desaulniers

[Andrew Lunn]

On Thu, Oct 01, 2020 at 04:09:43PM -0700, Nick Desaulniers wrote:
> On Wed, Sep 30, 2020 at 6:12 PM Andrew Lunn <and...@lunn.ch> wrote:
> >
> > There is a movement to try to make more and more of /drivers W=1
> > clean. But it will only stay clean if new warnings are quickly
> > detected and fixed, ideally by the developer adding the new code.
> >
> > To allow subdirectories to sign up to being W=1 clean for a given
> > definition of W=1, export the current set of additional compile flags
> > using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can
> > then use:
> >
> > subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930)
> >
> > To indicate they want to W=1 warnings as defined on 20200930.
> >
> > Additional warnings can be added to the W=1 definition. This will not
> > affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will
> > start appearing unless W=1 is actually added to the command
> > line. Developers can then take their time to fix any new W=1 warnings,
> > and then update to the latest KBUILD_CFLAGS_W1_<DATESTAMP> symbol.
>
> I'm not a fan of this approach. Are DATESTAMP configs just going to
> keep being added? Is it going to complicate isolating the issue from a
> randconfig build? If we want more things to build warning-free at
> W=1, then why don't we start moving warnings from W=1 into the
> default, until this is no W=1 left? That way we're cutting down on
> the kernel's configuration combinatorial explosion, rather than adding
> to it?

Hi Nick

I don't see randconfig being an issue. driver/net/ethernet would
always be build W=1, by some stable definition of W=1. randconfig
would not enable or disable additional warnings. It to make it clear,
KBUILD_CFLAGS_W1_20200930 is not a Kconfig option you can select. It
is a Makefile constant, a list of warnings which define what W=1 means
on that specific day. See patch 1/2.

I see a few issues with moving individual warnings from W=1 to the
default:

One of the comments for v1 of this patchset is that we cannot
introduce new warnings in the build. The complete tree needs to clean
of a particularly warning, before it can be added to the default list.
But that is not how people are cleaning up code, nor how the
infrastructure is designed. Those doing the cleanup are not take the
first from the list, -Wextra and cleanup up the whole tree for that
one warnings. They are rather enabling W=1 on a subdirectory, and
cleanup up all warnings on that subdirectory. So using this approach,
in order to move a warning from W=1 to the default, we are going to
have to get the entire tree W=1 clean, and move them all the warnings
are once.

People generally don't care about the tree as a whole. They care about
their own corner. The idea of fixing one warning thought the whole
tree is 'slicing and dicing' the kernel the wrong way. As we have seen
with the recent work with W=1, the more natural way to slice/dice the
kernel is by subdirectories.

I do however understand the fear that we end up with lots of
KBUILD_CFLAGS_W1_<DATESTAMP> constants. So i looked at the git history
of script/Makefile.extrawarn. These are historically the symbols we
would have, if we started this idea 1/1/2018:

KBUILD_CFLAGS_W1_20200326 # CLANG only change
KBUILD_CFLAGS_W1_20190907
KBUILD_CFLAGS_W1_20190617 # CLANG only change
KBUILD_CFLAGS_W1_20190614 # CLANG only change
KBUILD_CFLAGS_W1_20190509
KBUILD_CFLAGS_W1_20180919
KBUILD_CFLAGS_W1_20180111

So not too many.

Andrew

[kernel test robot]

Hi Andrew,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url: https://github.com/0day-ci/linux/commits/Andrew-Lunn/driver-net-ethernet-W-1-by-default/20201001-091431
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git e13dbc4f41db7f7b86f17a2efd7fbe19fc5b6d7a
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/b50d78df08d105cf0f0f2a1f4d2225656fd531cc
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Andrew-Lunn/driver-net-ethernet-W-1-by-default/20201001-091431
git checkout b50d78df08d105cf0f0f2a1f4d2225656fd531cc
# save the attached .config to linux build tree
make W=1 ARCH=i386

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <l...@intel.com>

All warnings (new ones prefixed by >>):

>> drivers/firmware/efi/libstub/x86-stub.c:669:15: warning: no previous prototype for 'efi_main' [-Wmissing-prototypes]
669 | unsigned long efi_main(efi_handle_t handle,
| ^~~~~~~~

vim +/efi_main +669 drivers/firmware/efi/libstub/x86-stub.c

291f36325f9f252 arch/x86/boot/compressed/eboot.c Matt Fleming 2011-12-12 663
9ca8f72a9297f20 arch/x86/boot/compressed/eboot.c Matt Fleming 2012-07-19 664 /*
8acf63efa1712fa drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 665 * On success, we return the address of startup_32, which has potentially been
8acf63efa1712fa drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 666 * relocated by efi_relocate_kernel.
8acf63efa1712fa drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 667 * On failure, we exit to the firmware via efi_exit instead of returning.
9ca8f72a9297f20 arch/x86/boot/compressed/eboot.c Matt Fleming 2012-07-19 668 */
8acf63efa1712fa drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 @669 unsigned long efi_main(efi_handle_t handle,
c3710de5065d63f arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2019-12-24 670 efi_system_table_t *sys_table_arg,
796eb8d26a57f91 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-01-13 671 struct boot_params *boot_params)
9ca8f72a9297f20 arch/x86/boot/compressed/eboot.c Matt Fleming 2012-07-19 672 {
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 673 unsigned long bzimage_addr = (unsigned long)startup_32;
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 674 unsigned long buffer_start, buffer_end;
9ca8f72a9297f20 arch/x86/boot/compressed/eboot.c Matt Fleming 2012-07-19 675 struct setup_header *hdr = &boot_params->hdr;
9ca8f72a9297f20 arch/x86/boot/compressed/eboot.c Matt Fleming 2012-07-19 676 efi_status_t status;
54b52d872680348 arch/x86/boot/compressed/eboot.c Matt Fleming 2014-01-10 677
ccc27ae77494252 drivers/firmware/efi/libstub/x86-stub.c Ard Biesheuvel 2020-04-16 678 efi_system_table = sys_table_arg;
9ca8f72a9297f20 arch/x86/boot/compressed/eboot.c Matt Fleming 2012-07-19 679
9ca8f72a9297f20 arch/x86/boot/compressed/eboot.c Matt Fleming 2012-07-19 680 /* Check if we were booted by the EFI firmware */
ccc27ae77494252 drivers/firmware/efi/libstub/x86-stub.c Ard Biesheuvel 2020-04-16 681 if (efi_system_table->hdr.signature != EFI_SYSTEM_TABLE_SIGNATURE)
3b8f44fc0810d51 drivers/firmware/efi/libstub/x86-stub.c Ard Biesheuvel 2020-02-16 682 efi_exit(handle, EFI_INVALID_PARAMETER);
9ca8f72a9297f20 arch/x86/boot/compressed/eboot.c Matt Fleming 2012-07-19 683
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 684 /*
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 685 * If the kernel isn't already loaded at a suitable address,
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 686 * relocate it.
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 687 *
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 688 * It must be loaded above LOAD_PHYSICAL_ADDR.
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 689 *
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 690 * The maximum address for 64-bit is 1 << 46 for 4-level paging. This
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 691 * is defined as the macro MAXMEM, but unfortunately that is not a
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 692 * compile-time constant if 5-level paging is configured, so we instead
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 693 * define our own macro for use here.
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 694 *
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 695 * For 32-bit, the maximum address is complicated to figure out, for
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 696 * now use KERNEL_IMAGE_SIZE, which will be 512MiB, the same as what
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 697 * KASLR uses.
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 698 *
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 699 * Also relocate it if image_offset is zero, i.e. the kernel wasn't
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 700 * loaded by LoadImage, but rather by a bootloader that called the
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 701 * handover entry. The reason we must always relocate in this case is
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 702 * to handle the case of systemd-boot booting a unified kernel image,
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 703 * which is a PE executable that contains the bzImage and an initrd as
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 704 * COFF sections. The initrd section is placed after the bzImage
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 705 * without ensuring that there are at least init_size bytes available
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 706 * for the bzImage, and thus the compressed kernel's startup code may
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 707 * overwrite the initrd unless it is moved out of the way.
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 708 */
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 709
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 710 buffer_start = ALIGN(bzimage_addr - image_offset,
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 711 hdr->kernel_alignment);
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 712 buffer_end = buffer_start + hdr->init_size;
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 713
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 714 if ((buffer_start < LOAD_PHYSICAL_ADDR) ||
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 715 (IS_ENABLED(CONFIG_X86_32) && buffer_end > KERNEL_IMAGE_SIZE) ||
d5cdf4cfeac9146 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 716 (IS_ENABLED(CONFIG_X86_64) && buffer_end > MAXMEM_X86_64_4LEVEL) ||
21cb9b414301c76 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-09 717 (image_offset == 0)) {
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 718 status = efi_relocate_kernel(&bzimage_addr,
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 719 hdr->init_size, hdr->init_size,
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 720 hdr->pref_address,
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 721 hdr->kernel_alignment,
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 722 LOAD_PHYSICAL_ADDR);
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 723 if (status != EFI_SUCCESS) {
36bdd0a78d56831 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-04-30 724 efi_err("efi_relocate_kernel() failed!\n");
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 725 goto fail;
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 726 }
1887c9b653f9957 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 727 /*
1887c9b653f9957 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 728 * Now that we've copied the kernel elsewhere, we no longer
1887c9b653f9957 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 729 * have a set up block before startup_32(), so reset image_offset
1887c9b653f9957 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 730 * to zero in case it was set earlier.
1887c9b653f9957 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 731 */
1887c9b653f9957 drivers/firmware/efi/libstub/x86-stub.c Arvind Sankar 2020-03-08 732 image_offset = 0;
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 733 }
04a7d0e15606769 arch/x86/boot/compressed/eboot.c Ard Biesheuvel 2020-02-10 734

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuil...@lists.01.org

[kernel test robot]

Hi Andrew,

I love your patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url: https://github.com/0day-ci/linux/commits/Andrew-Lunn/driver-net-ethernet-W-1-by-default/20201001-091431
base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git e13dbc4f41db7f7b86f17a2efd7fbe19fc5b6d7a

config: parisc-randconfig-r016-20200930 (attached as .config)
compiler: hppa64-linux-gcc (GCC) 9.3.0

reproduce (this is a W=1 build):

wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross

# https://github.com/0day-ci/linux/commit/b50d78df08d105cf0f0f2a1f4d2225656fd531cc
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Andrew-Lunn/driver-net-ethernet-W-1-by-default/20201001-091431
git checkout b50d78df08d105cf0f0f2a1f4d2225656fd531cc
# save the attached .config to linux build tree

COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=parisc

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <l...@intel.com>

All warnings (new ones prefixed by >>):

arch/parisc/boot/compressed/firmware.c: In function 'set_firmware_width_unlocked':
>> arch/parisc/boot/compressed/firmware.c:159:6: warning: variable 'ret' set but not used [-Wunused-but-set-variable]
159 | int ret;
| ^~~
arch/parisc/boot/compressed/firmware.c: In function 'set_firmware_width':
arch/parisc/boot/compressed/firmware.c:176:16: warning: variable 'flags' set but not used [-Wunused-but-set-variable]
176 | unsigned long flags;
| ^~~~~
arch/parisc/boot/compressed/firmware.c: In function 'pdc_iodc_print':
arch/parisc/boot/compressed/firmware.c:1234:16: warning: variable 'flags' set but not used [-Wunused-but-set-variable]
1234 | unsigned long flags;
| ^~~~~
At top level:
arch/parisc/boot/compressed/firmware.c:121:22: warning: 'f_extend' defined but not used [-Wunused-function]
121 | static unsigned long f_extend(unsigned long address)
| ^~~~~~~~

[Arnd Bergmann]

On Fri, Oct 2, 2020 at 3:44 AM Andrew Lunn <and...@lunn.ch> wrote:
> On Thu, Oct 01, 2020 at 04:09:43PM -0700, Nick Desaulniers wrote:
> > On Wed, Sep 30, 2020 at 6:12 PM Andrew Lunn <and...@lunn.ch> wrote:
> > >
> > > There is a movement to try to make more and more of /drivers W=1
> > > clean. But it will only stay clean if new warnings are quickly
> > > detected and fixed, ideally by the developer adding the new code.

Nice, I think everyone agrees that this is a good goal.

> > > To allow subdirectories to sign up to being W=1 clean for a given
> > > definition of W=1, export the current set of additional compile flags
> > > using the symbol KBUILD_CFLAGS_W1_20200930. Subdirectory Makefiles can
> > > then use:
> > >
> > > subdir-ccflags-y := $(KBUILD_CFLAGS_W1_20200930)
> > >
> > > To indicate they want to W=1 warnings as defined on 20200930.
> > >
> > > Additional warnings can be added to the W=1 definition. This will not
> > > affect KBUILD_CFLAGS_W1_20200930 and hence no additional warnings will
> > > start appearing unless W=1 is actually added to the command
> > > line. Developers can then take their time to fix any new W=1 warnings,
> > > and then update to the latest KBUILD_CFLAGS_W1_<DATESTAMP> symbol.
> >
> > I'm not a fan of this approach. Are DATESTAMP configs just going to
> > keep being added? Is it going to complicate isolating the issue from a
> > randconfig build? If we want more things to build warning-free at
> > W=1, then why don't we start moving warnings from W=1 into the
> > default, until this is no W=1 left? That way we're cutting down on
> > the kernel's configuration combinatorial explosion, rather than adding
> > to it?

I'm also a little sceptical about the datestamp.

> Hi Nick
>
> I don't see randconfig being an issue. driver/net/ethernet would
> always be build W=1, by some stable definition of W=1. randconfig
> would not enable or disable additional warnings. It to make it clear,
> KBUILD_CFLAGS_W1_20200930 is not a Kconfig option you can select. It
> is a Makefile constant, a list of warnings which define what W=1 means
> on that specific day. See patch 1/2.

It won't change with the configuration, but it will change with the
specific compiler version. When you enable a warning in a
particular driver or directory, this may have different effects
on one compiler compared to another: clang and gcc sometimes
differ in their interpretation of which specific forms of an expression
to warn about or not, and any multiplexing options like -Wextra
or -Wformat may turn on additional warnings in later releases.

> I see a few issues with moving individual warnings from W=1 to the
> default:
>
> One of the comments for v1 of this patchset is that we cannot
> introduce new warnings in the build. The complete tree needs to clean
> of a particularly warning, before it can be added to the default list.
> But that is not how people are cleaning up code, nor how the
> infrastructure is designed. Those doing the cleanup are not take the
> first from the list, -Wextra and cleanup up the whole tree for that
> one warnings. They are rather enabling W=1 on a subdirectory, and
> cleanup up all warnings on that subdirectory. So using this approach,
> in order to move a warning from W=1 to the default, we are going to
> have to get the entire tree W=1 clean, and move them all the warnings
> are once.

I think the two approaches are orthogonal, and I would like to
see both happening as much as possible:

- any warning flag in the W=1 set (including many things implied
by -Wextra that have or should have their own flags) that
only causes a few lines of output should just be enabled by
default after we address the warnings

- Code with maintainers that care should have a way to enable
the entire W=1 set per directory or per file after addressing all
the warnings they do see, with new flags only getting added to
W=1 when they don't cause regressions.

There are more things that we might want to do on top of this:

- identify additional warning flags that we be good to add to W=1

- identify warning flags that are better off being turned into errors,
like we do with -Werror=strict-prototypes

- Fix the warnings in W=2 that show up in common header files,
to actually make it realistic to build specific drivers with W=2
and not have interesting issues drowned out in the noise.

- ensure that any warning flag we turn *off* in W=1 or by default
is turned on again in one of the higher levels

> People generally don't care about the tree as a whole. They care about
> their own corner. The idea of fixing one warning thought the whole
> tree is 'slicing and dicing' the kernel the wrong way. As we have seen
> with the recent work with W=1, the more natural way to slice/dice the
> kernel is by subdirectories.

I do care about the tree as a whole, and I'm particularly interested in
having -Wmissing-declarations/-Wmissing-prototypes enabled globally
at some point in the future.

Arnd

[Andrew Lunn]

Hi Arnd

Any suggestions for an alternative?

> It won't change with the configuration, but it will change with the
> specific compiler version. When you enable a warning in a
> particular driver or directory, this may have different effects
> on one compiler compared to another: clang and gcc sometimes
> differ in their interpretation of which specific forms of an expression
> to warn about or not, and any multiplexing options like -Wextra
> or -Wformat may turn on additional warnings in later releases.

How do we deal with this at the moment? Or will -Wextra and -Wformat
never get moved into the default?

> I think the two approaches are orthogonal, and I would like to
> see both happening as much as possible:
>
> - any warning flag in the W=1 set (including many things implied
> by -Wextra that have or should have their own flags) that
> only causes a few lines of output should just be enabled by
> default after we address the warnings

Is there currently any simple way to get statistics about how many
actual warnings there are per warnings flag in W=1? W=1 on the tree as
a whole does not look good, but maybe there is some low hanging fruit
and we can quickly promote some from W=1 to default?

> - Code with maintainers that care should have a way to enable
> the entire W=1 set per directory or per file after addressing all
> the warnings they do see, with new flags only getting added to
> W=1 when they don't cause regressions.

Yes, this is what i'm trying to push forward here. I don't
particularly care how it happen, so if somebody comes up with a
generally acceptable idea, i will go away and implement it.

> > People generally don't care about the tree as a whole. They care about
> > their own corner. The idea of fixing one warning thought the whole
> > tree is 'slicing and dicing' the kernel the wrong way. As we have seen
> > with the recent work with W=1, the more natural way to slice/dice the
> > kernel is by subdirectories.
>
> I do care about the tree as a whole, and I'm particularly interested in
> having -Wmissing-declarations/-Wmissing-prototypes enabled globally
> at some point in the future.

I know you care. But you are vastly out numbered by developers who
care about their own little corner. Which is why i said 'generally'.

We definitely should come at the problem from both directions, but i
guess we can make more progress with tools for the large number of
developers each in their own corner, than tools for the few who work
tree wide.

Andrew

[Arnd Bergmann]

On Fri, Oct 2, 2020 at 2:51 PM Andrew Lunn <and...@lunn.ch> wrote:
> On Fri, Oct 02, 2020 at 02:20:50PM +0200, Arnd Bergmann wrote:
> > On Fri, Oct 2, 2020 at 3:44 AM Andrew Lunn <and...@lunn.ch> wrote:
> > > On Thu, Oct 01, 2020 at 04:09:43PM -0700, Nick Desaulniers wrote:
> > > >
> > > > I'm not a fan of this approach. Are DATESTAMP configs just going to
> > > > keep being added? Is it going to complicate isolating the issue from a
> > > > randconfig build? If we want more things to build warning-free at
> > > > W=1, then why don't we start moving warnings from W=1 into the
> > > > default, until this is no W=1 left? That way we're cutting down on
> > > > the kernel's configuration combinatorial explosion, rather than adding
> > > > to it?
> >
> > I'm also a little sceptical about the datestamp.
>
> Hi Arnd
>
> Any suggestions for an alternative?

I think we should deal with it the same way we deal with new
compiler versions: before a new compiler starts getting widely
used, someone has to address the new warnings that show up,
or at the minimum they have to get turned off by default until
they are addressed.

Today, moving a warning flag from W=1 to default requires that
there won't be any regressions in the output. The same should
apply to adding W=1 warnings if there is a way for drivers to
default-enable them.

> > It won't change with the configuration, but it will change with the
> > specific compiler version. When you enable a warning in a
> > particular driver or directory, this may have different effects
> > on one compiler compared to another: clang and gcc sometimes
> > differ in their interpretation of which specific forms of an expression
> > to warn about or not, and any multiplexing options like -Wextra
> > or -Wformat may turn on additional warnings in later releases.
>
> How do we deal with this at the moment? Or will -Wextra and -Wformat
> never get moved into the default?

I think for Wextra, that would likely stay with W=1, though individual
warnings in that set should be enabled by default whenever they
make sense. For -Wformat, we probably want the opposite and
enable the global option by default but make individual sub-options
W=1 or W=2 if there is too much undesired output.

> > I think the two approaches are orthogonal, and I would like to
> > see both happening as much as possible:
> >
> > - any warning flag in the W=1 set (including many things implied
> > by -Wextra that have or should have their own flags) that
> > only causes a few lines of output should just be enabled by
> > default after we address the warnings
>
> Is there currently any simple way to get statistics about how many
> actual warnings there are per warnings flag in W=1? W=1 on the tree as
> a whole does not look good, but maybe there is some low hanging fruit
> and we can quickly promote some from W=1 to default?

I have done this a few times in the past, essentially building a
few hundred randconfig kernels across multiple architectures
and then processing the output in a script. I usually treat a
file:line:warning tuple as a single instance and then count
how many there are.

> > - Code with maintainers that care should have a way to enable
> > the entire W=1 set per directory or per file after addressing all
> > the warnings they do see, with new flags only getting added to
> > W=1 when they don't cause regressions.
>
> Yes, this is what i'm trying to push forward here. I don't
> particularly care how it happen, so if somebody comes up with a
> generally acceptable idea, i will go away and implement it.

I actually have a set of patches that I started a while ago to
move the logic from scripts/Makefile.extrawarn into
include/linux/warnings.h, using '_Pragma("GCC diagnostic ...")'
with some infrastructure around it, to also allow drivers to
set the level as well as individual warnings when needed.

I never managed to get that patch series into a state for submission
so far, with a few things that need to be addressed first:

- any Makefile that changes warning options needs to be
converted to use macro syntax

- I need to check that the patches don't accidentally disable
warnings that are currently enabled (this is harder than
checking the reverse)

- some specific warnings have problems with this new method
for options that control multiple other options.

Arnd

[Nick Desaulniers]

Sorry, to be more specific about my concern; I like the idea of
exporting the W=* flags, then selectively applying them via
subdir-ccflags-y. I don't like the idea of supporting W=1 as defined
at a precise point in time via multiple date specific symbols. If
someone adds something to W=1, then they should need to ensure subdirs
build warning-free, so I don't think you need to "snapshot" W=1 based
on what it looked like on 20200930.

>
> People generally don't care about the tree as a whole. They care about
> their own corner. The idea of fixing one warning thought the whole
> tree is 'slicing and dicing' the kernel the wrong way. As we have seen
> with the recent work with W=1, the more natural way to slice/dice the
> kernel is by subdirectories.

I'm not sure I agree with this paragraph. ^ If a warning is not
enabled by default implicitly, then someone would need to clean the
tree to turn it on. It's very messy to apply it on a child directory,
then try to work up. We've done multiple tree wide warning cleanups
and it's not too bad.

>
> I do however understand the fear that we end up with lots of
> KBUILD_CFLAGS_W1_<DATESTAMP> constants. So i looked at the git history
> of script/Makefile.extrawarn. These are historically the symbols we
> would have, if we started this idea 1/1/2018:
>
> KBUILD_CFLAGS_W1_20200326 # CLANG only change
> KBUILD_CFLAGS_W1_20190907
> KBUILD_CFLAGS_W1_20190617 # CLANG only change
> KBUILD_CFLAGS_W1_20190614 # CLANG only change
> KBUILD_CFLAGS_W1_20190509
> KBUILD_CFLAGS_W1_20180919
> KBUILD_CFLAGS_W1_20180111
>
> So not too many.

It's a useful visualization. I still would prefer W=1 to get enabled
by default if all of the CI systems have flushed out the existing
warnings. That way we have one less combination of things to test;
not more.
--
Thanks,
~Nick Desaulniers

[Andrew Lunn]

> Sorry, to be more specific about my concern; I like the idea of
> exporting the W=* flags, then selectively applying them via
> subdir-ccflags-y. I don't like the idea of supporting W=1 as defined
> at a precise point in time via multiple date specific symbols. If
> someone adds something to W=1, then they should need to ensure subdirs
> build warning-free, so I don't think you need to "snapshot" W=1 based
> on what it looked like on 20200930.

Hi Nick

That then contradicts what Masahiro Yamada said to the first version i
posted:

https://www.spinics.net/lists/netdev/msg685284.html
> With this patch series applied, where should we add -Wfoo-bar?
> Adding it to W=1 would emit warnings under drivers/net/ since W=1 is
> now the default for the net subsystem.

The idea with the date stamps was to allow new warnings to be added to
W=1 without them immediately causing warnings on normal builds. You
are saying that whoever adds a new warning to W=1 needs to cleanup the
tree which is already W=1 clean? That might have the side effect that
no more warnings are added to W=1 :-(

Andrew

[Arnd Bergmann]

On Mon, Oct 5, 2020 at 9:49 PM Andrew Lunn <and...@lunn.ch> wrote:
>
> > Sorry, to be more specific about my concern; I like the idea of
> > exporting the W=* flags, then selectively applying them via
> > subdir-ccflags-y. I don't like the idea of supporting W=1 as defined
> > at a precise point in time via multiple date specific symbols. If
> > someone adds something to W=1, then they should need to ensure subdirs
> > build warning-free, so I don't think you need to "snapshot" W=1 based
> > on what it looked like on 20200930.
>

> That then contradicts what Masahiro Yamada said to the first version i
> posted:
>
> https://www.spinics.net/lists/netdev/msg685284.html
> > With this patch series applied, where should we add -Wfoo-bar?
> > Adding it to W=1 would emit warnings under drivers/net/ since W=1 is
> > now the default for the net subsystem.
>
> The idea with the date stamps was to allow new warnings to be added to
> W=1 without them immediately causing warnings on normal builds. You
> are saying that whoever adds a new warning to W=1 needs to cleanup the
> tree which is already W=1 clean? That might have the side effect that
> no more warnings are added to W=1 :-(

It depends a lot on what portion of the kernel gets enabled for W=1.

As long as it's only drivers that are actively maintained, and they
make up a fairly small portion of all code, it should not be a problem
to find someone to fix useful warnings.

The only reason to add a flag to W=1 would be that the bugs it reports
are important enough to look at the false positives and address
those as well. Whoever decided to enable W=1 by default for their
subsystem should then also be interested in adding the new warnings.

If I wanted to add a new flag to W=1 and this introduces output
for allmodconfig, I would start by mailing that output to the
respective maintainers.

Arnd

[Andrew Lunn]

> It depends a lot on what portion of the kernel gets enabled for W=1.
>
> As long as it's only drivers that are actively maintained, and they
> make up a fairly small portion of all code, it should not be a problem
> to find someone to fix useful warnings.

Well, drivers/net/ethernet is around 1.5M LOC. The tree as a whole is
just short of 23M LOC. So i guess that is a small portion of all the
code.

Andrew

[Masahiro Yamada]

I am not a big fan of KBUILD_CFLAGS_W1_<timestamp>
since it is ugly.

I'd like to start with adding individual flags
like drivers/gpu/drm/i915/Makefile, and see
how difficult it would be to maintain it.

One drawback of your approach is that
you cannot set KBUILD_CFLAGS_W1_20200930
until you eliminate all the warnings in the
sub-directory in interest.
(i.e. all or nothing approach)

At best, you can only work out from 'old -> new' order
because KBUILD_CFLAGS_W1_20200326 is a suer-set of
KBUILD_CFLAGS_W1_20190907, which is a suer-set of
KBUILD_CFLAGS_W1_20190617 ...



If you add flags individually, you can start with
low-hanging fruits, or ones with higher priority
as Arnd mentions about -Wmissing-{declaration,prototypes}.


For example, you might be able to set
'subdir-ccflags-y += -Wmissing-declarations'
to drivers/net/Makefile, while
'subdir-ccflags-y += -Wunused-but-set-variable'
stays in drivers/net/ethernet/Makefile.



--
Best Regards
Masahiro Yamada

[Masahiro Yamada]

On Fri, Oct 2, 2020 at 9:21 PM Arnd Bergmann <ar...@arndb.de> wrote:
>
> I do care about the tree as a whole, and I'm particularly interested in
> having -Wmissing-declarations/-Wmissing-prototypes enabled globally
> at some point in the future.
>
> Arnd

BTW, if possible, please educate me about the difference
between -Wmissing-declarations and -Wmissing-prototypes.


The GCC manual says as follows:


-Wmissing-prototypes (C and Objective-C only)

Warn if a global function is defined without a previous prototype
declaration. This warning is issued even if the definition itself
provides a prototype. Use this option to detect global functions that
do not have a matching prototype declaration in a header file. This
option is not valid for C++ because all function declarations provide
prototypes and a non-matching declaration declares an overload rather
than conflict with an earlier declaration. Use -Wmissing-declarations
to detect missing declarations in C++.

-Wmissing-declarations

Warn if a global function is defined without a previous declaration.
Do so even if the definition itself provides a prototype. Use this
option to detect global functions that are not declared in header
files. In C, no warnings are issued for functions with previous
non-prototype declarations; use -Wmissing-prototypes to detect missing
prototypes. In C++, no warnings are issued for function templates, or
for inline functions, or for functions in anonymous namespaces.



The difference is still unclear to me...



For example, if I add -Wmissing-declarations, I get the following:


kernel/sched/core.c:2380:6: warning: no previous declaration for
‘sched_set_stop_task’ [-Wmissing-declarations]
2380 | void sched_set_stop_task(int cpu, struct task_struct *stop)
| ^~~~~~~~~~~~~~~~~~~



But, if I add both -Wmissing-declarations and -Wmissing-prototypes,
-Wmissing-declarations is superseded by -Wmissing-prototypes.



kernel/sched/core.c:2380:6: warning: no previous prototype for
‘sched_set_stop_task’ [-Wmissing-prototypes]
2380 | void sched_set_stop_task(int cpu, struct task_struct *stop)
| ^~~~~~~~~~~~~~~~~~~




Do we need to specify both in W=1 ?
If yes, what is the difference between them?

[Arnd Bergmann]

On Mon, Oct 12, 2020 at 3:00 AM Masahiro Yamada <masa...@kernel.org> wrote:
>
> BTW, if possible, please educate me about the difference
> between -Wmissing-declarations and -Wmissing-prototypes.
>

...

> Do we need to specify both in W=1 ?
> If yes, what is the difference between them?

I think they do the same thing in the kernel, as we already set
'-Wstrict-prototypes', which requires that there are no declarations
without having a prototype first. Adding either one should be sufficient.

Arnd

[Arnd Bergmann]

On Sun, Oct 11, 2020 at 3:04 PM Masahiro Yamada <masa...@kernel.org> wrote:
> On Tue, Oct 6, 2020 at 6:08 AM Andrew Lunn <and...@lunn.ch> wrote:
>
>
> I am not a big fan of KBUILD_CFLAGS_W1_<timestamp>
> since it is ugly.
>
> I'd like to start with adding individual flags
> like drivers/gpu/drm/i915/Makefile, and see
> how difficult it would be to maintain it.

I don't really like that either, to be honest, mostly because that is
somewhat incompatible with my plan to move all the warning flags
out the command line and into _Pragma() lines in header files.

> One drawback of your approach is that
> you cannot set KBUILD_CFLAGS_W1_20200930
> until you eliminate all the warnings in the
> sub-directory in interest.
> (i.e. all or nothing approach)
>
> At best, you can only work out from 'old -> new' order
> because KBUILD_CFLAGS_W1_20200326 is a suer-set of
> KBUILD_CFLAGS_W1_20190907, which is a suer-set of
> KBUILD_CFLAGS_W1_20190617 ...
>
>
>
> If you add flags individually, you can start with
> low-hanging fruits, or ones with higher priority
> as Arnd mentions about -Wmissing-{declaration,prototypes}.
>
>
> For example, you might be able to set
> 'subdir-ccflags-y += -Wmissing-declarations'
> to drivers/net/Makefile, while
> 'subdir-ccflags-y += -Wunused-but-set-variable'
> stays in drivers/net/ethernet/Makefile.

I agree with the goal though. In particular it may be helpful to pick
a set of warning flags to always be enabled without also setting
-Wextra, which has different meanings depending on compiler
version, or between gcc and clang.

I wonder how different they really are, and we can probably find
out from https://github.com/Barro/compiler-warnings, but I have
not checked myself.

Arnd

[Andrew Lunn]

> One drawback of your approach is that
> you cannot set KBUILD_CFLAGS_W1_20200930
> until you eliminate all the warnings in the
> sub-directory in interest.
> (i.e. all or nothing approach)

Hi Mashiro

That actual works well for my use case. drivers/net/ethernet is W=1
clean. So is drivers/net/phy, drivers/net/mdio. Developers generally
clean up a subsystem by adding W=1 to the command line because that is
the simple tool they have.

And my aim here is to keep those subsystem W=1 clean. I don't care
about individual warnings within W=1, because those subsystems are
passed that stage already.

Andrew

[Arnd Bergmann]

I tried to get a better grasp of what kind of warnings we are actually talking
about and looked at the x86 allmodconfig W=1 output on today's linux-next.

What I found is that most subsystems are clean for most warnings, a
few dozen subsystems have a very small number of warnings and a
handful of subsystems have a ton of (mostly identical) warnings, see
below for the full list and how to reproduce.

Based on the numbers, I would still suggest an approach that starts
from multiple angles:

- Add a patch similar to yours that turns on W=1 for the largest
known-clean subsystems (drivers/net/ethernet, drivers/media,
drivers/mfd, ...), but without the date magic that won't be needed
if we follow through with the rest of the plan.

- Turn on -Wold-style-definition -Wmissing-include-dirs -Wstringop-truncation
globally, since they don't seem to cause any output any more.

- Work on fixing all -Wextra warnings (except -Woverride-init
-Wunused-parameter -Wmissing-field-initializers -Wtype-limits)
across the tree, then enable -Wextra unconditionally. I see this
being relevant to only 25 files in the allmodconfig build, which is
much less than I expected for -Wextra. There are a couple more
for clang as well, but not that many either.

- Same for Wpacked-not-aligned, which happens in only 10 files
and almost exclusively in wireless network drivers

- For each of -Wmissing-prototypes, -Wsuggest-attribute=format,
-Wunused-but-set-variable, -Wunused-const-variable, and
-Woverride-init, come up with a patch that disables or fixes
the warning for the largest offender(s) and enables it globally
in linux-next but not mainline, with the hope that others fix the
reported issues. Do it one at a time, starting with
-Wmissing-prototypes. When most of the output is gone
for one of them, work on the final patch to deal with the remaining
files and enable the warning unconditionally, then move on to the
next in the list. This will take a while.

- When most of the above are done, redefine W=1 to include
a larger set of warnings that are considered the most useful
but are currently disabled at this level. For each subsystem that
enables the current W=1 warnings unconditionally, either
find a way to only enable the remainder of the old set or send
fixes addressing the new issues and leave it at W=1.

Arnd


8<---
grep warning: allmod-warnings-gcc-9 | grep '\[-W' | sed -e
's:^../\([\./a-zA-Z0-9_-]*\)\:.* \(\[-W.*\]\):\2 \1:' | cut -f -2 -d/
| sort | uniq -c
1 [-Wcast-function-type] drivers/firewire
12 [-Wcast-function-type] drivers/net
10 [-Wcast-function-type] include/linux
2 [-Wempty-body] drivers/base
2 [-Wempty-body] drivers/dax
1 [-Wempty-body] drivers/input
1 [-Wempty-body] drivers/isdn
2 [-Wempty-body] drivers/net
2 [-Wempty-body] drivers/platform
4 [-Wempty-body] drivers/scsi
2 [-Wempty-body] drivers/staging
1 [-Wempty-body] drivers/target
2 [-Wempty-body] fs/hfs
4 [-Wempty-body] fs/hfsplus
5 [-Wempty-body] fs/jffs2
1 [-Wempty-body] fs/posix_acl.c
1 [-Wignored-qualifiers] drivers/block
1 [-Wignored-qualifiers] drivers/gpu
1 [-Wignored-qualifiers] drivers/net
573 [-Wmissing-prototypes] arch/x86 # mostly asm/syscall_wrapper.h
12 [-Wmissing-prototypes] drivers/acpi
1 [-Wmissing-prototypes] drivers/base
14 [-Wmissing-prototypes] drivers/char
1 [-Wmissing-prototypes] drivers/clk
34 [-Wmissing-prototypes] drivers/crypto
1 [-Wmissing-prototypes] drivers/dax
5 [-Wmissing-prototypes] drivers/firmware
324 [-Wmissing-prototypes] drivers/gpu # mostly amdgpu
1 [-Wmissing-prototypes] drivers/memstick
3 [-Wmissing-prototypes] drivers/net
20 [-Wmissing-prototypes] drivers/nvdimm
9 [-Wmissing-prototypes] drivers/ptp
1 [-Wmissing-prototypes] drivers/rpmsg
1 [-Wmissing-prototypes] drivers/rtc
2 [-Wmissing-prototypes] drivers/scsi
3 [-Wmissing-prototypes] drivers/tty
1 [-Wmissing-prototypes] drivers/vfio
1 [-Wmissing-prototypes] drivers/xen
1 [-Wmissing-prototypes] fs/d_path.c
2 [-Wmissing-prototypes] fs/nfs
1 [-Wmissing-prototypes] include/linux
1 [-Wmissing-prototypes] kernel/bpf
1 [-Wmissing-prototypes] kernel/debug
1 [-Wmissing-prototypes] kernel/events
1 [-Wmissing-prototypes] kernel/exit.c
9 [-Wmissing-prototypes] kernel/gcov
1 [-Wmissing-prototypes] kernel/irq
1 [-Wmissing-prototypes] kernel/kallsyms.c
10 [-Wmissing-prototypes] kernel/kcov.c
3 [-Wmissing-prototypes] kernel/locking
1 [-Wmissing-prototypes] kernel/module.c
4 [-Wmissing-prototypes] kernel/panic.c
1 [-Wmissing-prototypes] kernel/power
2 [-Wmissing-prototypes] kernel/sched
1 [-Wmissing-prototypes] kernel/signal.c
1 [-Wmissing-prototypes] kernel/smp.c
9 [-Wmissing-prototypes] kernel/trace
1 [-Wmissing-prototypes] lib/decompress_inflate.c
1 [-Wmissing-prototypes] lib/decompress_unxz.c
1 [-Wmissing-prototypes] lib/decompress_unzstd.c
4 [-Wmissing-prototypes] lib/kunit
2 [-Wmissing-prototypes] lib/lz4
1 [-Wmissing-prototypes] lib/lzo
1 [-Wmissing-prototypes] lib/radix-tree.c
1 [-Wmissing-prototypes] lib/test_ida.c
10 [-Wmissing-prototypes] lib/zstd
1 [-Wmissing-prototypes] mm/early_ioremap.c
1 [-Wmissing-prototypes] mm/filemap.c
2 [-Wmissing-prototypes] mm/page_alloc.c
1 [-Wmissing-prototypes] mm/truncate.c
1 [-Wmissing-prototypes] mm/vmalloc.c
4 [-Wmissing-prototypes] net/ipv6
2 [-Wmissing-prototypes] net/netfilter
4 [-Wmissing-prototypes] samples/ftrace
5 [-Wmissing-prototypes] security/integrity
1 [-Wmissing-prototypes] security/selinux
1 [-Woverflow] lib/bitfield_kunit.c
1 [-Woverride-init] arch/x86
27 [-Woverride-init] drivers/ata
1 [-Woverride-init] drivers/block
86 [-Woverride-init] drivers/gpu
1 [-Woverride-init] include/linux
1 [-Woverride-init] kernel/bpf
4 [-Woverride-init] kernel/time
1 [-Woverride-init] lib/errname.c
3 [-Woverride-init] net/wimax
77 [-Wpacked-not-aligned] drivers/net
3 [-Wpacked-not-aligned] drivers/scsi
155 [-Wpacked-not-aligned] drivers/staging
1 [-Wsuggest-attribute=format] arch/x86
1 [-Wsuggest-attribute=format] drivers/acpi
1 [-Wsuggest-attribute=format] drivers/clk
2 [-Wsuggest-attribute=format] drivers/gpu
1 [-Wsuggest-attribute=format] drivers/hid
2 [-Wsuggest-attribute=format] drivers/isdn
1 [-Wsuggest-attribute=format] drivers/pnp
2 [-Wsuggest-attribute=format] drivers/scsi
1 [-Wsuggest-attribute=format] drivers/xen
2 [-Wsuggest-attribute=format] fs/reiserfs
10 [-Wsuggest-attribute=format] include/trace
2 [-Wsuggest-attribute=format] kernel/audit.c
2 [-Wsuggest-attribute=format] kernel/bpf
1 [-Wsuggest-attribute=format] kernel/panic.c
28 [-Wsuggest-attribute=format] kernel/trace
3 [-Wsuggest-attribute=format] lib/kunit
1 [-Wsuggest-attribute=format] lib/vsprintf.c
1 [-Wsuggest-attribute=format] net/dccp
1 [-Wsuggest-attribute=format] net/netfilter
1 [-Wsuggest-attribute=format] net/tipc
2 [-Wsuggest-attribute=format] security/tomoyo
12 [-Wunused-but-set-variable] arch/x86
1 [-Wunused-but-set-variable] drivers/base
1 [-Wunused-but-set-variable] drivers/block
8 [-Wunused-but-set-variable] drivers/char
1 [-Wunused-but-set-variable] drivers/clk
1 [-Wunused-but-set-variable] drivers/clocksource
1 [-Wunused-but-set-variable] drivers/firewire
67 [-Wunused-but-set-variable] drivers/gpu
4 [-Wunused-but-set-variable] drivers/hid
1 [-Wunused-but-set-variable] drivers/i2c
4 [-Wunused-but-set-variable] drivers/ide
9 [-Wunused-but-set-variable] drivers/input
1 [-Wunused-but-set-variable] drivers/isdn
1 [-Wunused-but-set-variable] drivers/leds
1 [-Wunused-but-set-variable] drivers/memstick
7 [-Wunused-but-set-variable] drivers/message
2 [-Wunused-but-set-variable] drivers/mmc
2 [-Wunused-but-set-variable] drivers/mtd
47 [-Wunused-but-set-variable] drivers/net
3 [-Wunused-but-set-variable] drivers/platform
1 [-Wunused-but-set-variable] drivers/rapidio
30 [-Wunused-but-set-variable] drivers/scsi
1 [-Wunused-but-set-variable] drivers/soc
1 [-Wunused-but-set-variable] drivers/spmi
47 [-Wunused-but-set-variable] drivers/staging
1 [-Wunused-but-set-variable] drivers/thermal
1 [-Wunused-but-set-variable] drivers/thunderbolt
9 [-Wunused-but-set-variable] drivers/tty
1 [-Wunused-but-set-variable] drivers/usb
1 [-Wunused-but-set-variable] drivers/vhost
47 [-Wunused-but-set-variable] drivers/video
2 [-Wunused-but-set-variable] drivers/w1
12 [-Wunused-but-set-variable] fs/ceph
11 [-Wunused-but-set-variable] fs/coda
2 [-Wunused-but-set-variable] fs/jffs2
3 [-Wunused-but-set-variable] fs/ntfs
3 [-Wunused-but-set-variable] kernel/trace
1 [-Wunused-but-set-variable] lib/decompress_unlzo.c
1 [-Wunused-but-set-variable] lib/test_blackhole_dev.c
1 [-Wunused-but-set-variable] lib/test_kasan_module.c
4 [-Wunused-but-set-variable] lib/test_ubsan.c
1 [-Wunused-but-set-variable] net/nfc
1 [-Wunused-but-set-variable] security/smack
2 [-Wunused-const-variable=] drivers/clk
2513 [-Wunused-const-variable=] drivers/gpu # amdgpu
1 [-Wunused-const-variable=] drivers/hid
1 [-Wunused-const-variable=] drivers/ide
3 [-Wunused-const-variable=] drivers/input
17 [-Wunused-const-variable=] drivers/net
122 [-Wunused-const-variable=] drivers/staging # rtl8xxx
1 [-Wunused-const-variable=] drivers/usb
279 [-Wunused-const-variable=] drivers/video # sis-fbdev
5 [-Wunused-const-variable=] drivers/visorbus
6 [-Wunused-const-variable=] fs/efs
1 [-Wunused-const-variable=] fs/fscache
16 [-Wunused-const-variable=] fs/orangefs
236 [-Wunused-const-variable=] include/linux # lsm_hook_defs.h, zstd.h
32 [-Wunused-const-variable=] lib/zstd

$ grep -h warning: allmod-warnings-clang-11 | grep '\[-W' | sed -e
's:^../\([\./a-zA-Z0-9_-]*\)\:.* \(\[-W.*\]\):\2 \1:' | cut -f -2 -d/
| sort | uniq -c | grep -w Wmissing-prototypes
1 [-Wconstant-conversion] lib/bitfield_kunit.c
1 [-Wformat] drivers/acpi
4 [-Wformat] drivers/atm
1 [-Wformat] drivers/gpio
23 [-Wformat] drivers/gpu
2 [-Wformat] drivers/hid
1 [-Wformat] drivers/hwmon
4 [-Wformat] drivers/iio
1 [-Wformat] drivers/infiniband
5 [-Wformat] drivers/media
6 [-Wformat] drivers/misc
48 [-Wformat] drivers/net
3 [-Wformat] drivers/ntb
1 [-Wformat] drivers/platform
6 [-Wformat] drivers/scsi
2 [-Wformat] drivers/soc
14 [-Wformat] drivers/target
1 [-Wformat] fs/afs
2 [-Wformat] fs/nfsd
1 [-Wformat] kernel/locking
13 [-Wformat] lib/test_printf.c
2 [-Wformat] net/8021q
1 [-Wformat] net/batman-adv
2 [-Wformat] net/ceph
16 [-Wformat] net/ipv4
2 [-Wformat] net/ipv6
9 [-Wformat] net/l2tp
24 [-Wformat] net/netfilter
1 [-Wformat] net/rxrpc
1 [-Wignored-qualifiers] drivers/block
1 [-Wignored-qualifiers] drivers/gpu
1 [-Wignored-qualifiers] drivers/net
1 [-Winitializer-overrides] arch/x86
27 [-Winitializer-overrides] drivers/ata
1 [-Winitializer-overrides] drivers/block
86 [-Winitializer-overrides] drivers/gpu
259 [-Winitializer-overrides] drivers/net
1 [-Winitializer-overrides] kernel/bpf
4 [-Winitializer-overrides] kernel/time
1 [-Winitializer-overrides] lib/errname.c
3 [-Winitializer-overrides] net/wimax
1 [-Wnull-pointer-arithmetic] drivers/atm
1 [-Wnull-pointer-arithmetic] fs/kernfs
1 [-Wnull-pointer-arithmetic] fs/seq_file.c
1 [-Wnull-pointer-arithmetic] security/tomoyo
1 [-Wpointer-bool-conversion] drivers/net
1 [-Wsometimes-uninitialized] drivers/vfio
1 [-Wtautological-constant-out-of-range-compare] drivers/block
1 [-Wtautological-constant-out-of-range-compare] drivers/gpu
1 [-Wtautological-constant-out-of-range-compare] drivers/md
2 [-Wtautological-constant-out-of-range-compare] drivers/media
1 [-Wtautological-constant-out-of-range-compare] drivers/net
1 [-Wtautological-constant-out-of-range-compare] drivers/staging
1 [-Wtautological-constant-out-of-range-compare] fs/ceph
3 [-Wtautological-constant-out-of-range-compare] fs/vboxsf
1 [-Wtautological-constant-out-of-range-compare] kernel/bpf
1 [-Wtautological-constant-out-of-range-compare] kernel/kcov.c
2 [-Wtautological-constant-out-of-range-compare] net/ceph
1 [-Wtautological-constant-out-of-range-compare] net/ipv4
1 [-Wuninitialized] drivers/staging
2 [-Wunneeded-internal-declaration] drivers/gpu
2 [-Wunused-const-variable] drivers/clk
19 [-Wunused-const-variable] drivers/gpu
1 [-Wunused-const-variable] drivers/ide
12 [-Wunused-const-variable] drivers/net
2 [-Wunused-const-variable] drivers/staging
1 [-Wunused-const-variable] drivers/usb
1 [-Wunused-const-variable] fs/fscache
2 [-Wunused-function] arch/x86
1 [-Wunused-function] block/blk-zoned.c
1 [-Wunused-function] block/partitions
1 [-Wunused-function] drivers/acpi
3 [-Wunused-function] drivers/atm
2 [-Wunused-function] drivers/bcma
2 [-Wunused-function] drivers/cpufreq
2 [-Wunused-function] drivers/crypto
14 [-Wunused-function] drivers/dma
1 [-Wunused-function] drivers/dma-buf
1 [-Wunused-function] drivers/edac
1 [-Wunused-function] drivers/fpga
3 [-Wunused-function] drivers/gpio
15 [-Wunused-function] drivers/gpu
1 [-Wunused-function] drivers/hv
3 [-Wunused-function] drivers/hwmon
1 [-Wunused-function] drivers/hwtracing
4 [-Wunused-function] drivers/infiniband
1 [-Wunused-function] drivers/isdn
1 [-Wunused-function] drivers/leds
1 [-Wunused-function] drivers/md
37 [-Wunused-function] drivers/media
2 [-Wunused-function] drivers/mfd
5 [-Wunused-function] drivers/misc
3 [-Wunused-function] drivers/mmc
2 [-Wunused-function] drivers/mtd
162 [-Wunused-function] drivers/net
3 [-Wunused-function] drivers/nvme
2 [-Wunused-function] drivers/pci
1 [-Wunused-function] drivers/pcmcia
2 [-Wunused-function] drivers/phy
1 [-Wunused-function] drivers/pwm
1 [-Wunused-function] drivers/rtc
29 [-Wunused-function] drivers/scsi
1 [-Wunused-function] drivers/soc
1 [-Wunused-function] drivers/spi
6 [-Wunused-function] drivers/staging
1 [-Wunused-function] drivers/thermal
6 [-Wunused-function] drivers/tty
7 [-Wunused-function] drivers/usb
1 [-Wunused-function] drivers/vdpa
5 [-Wunused-function] drivers/video
3 [-Wunused-function] drivers/watchdog
1 [-Wunused-function] fs/cifs
1 [-Wunused-function] fs/dlm
1 [-Wunused-function] fs/fuse
1 [-Wunused-function] fs/lockd
1 [-Wunused-function] fs/nfsd
1 [-Wunused-function] fs/ocfs2
2 [-Wunused-function] kernel/locking
1 [-Wunused-function] kernel/power
3 [-Wunused-function] kernel/time
4 [-Wunused-function] kernel/trace
1 [-Wunused-function] lib/bitfield_kunit.c
1 [-Wunused-function] lib/zlib_inflate
1 [-Wunused-function] mm/vmalloc.c
2 [-Wunused-function] net/bluetooth
2 [-Wunused-function] net/ipv6
1 [-Wunused-function] net/sunrpc
2 [-Wunused-function] security/apparmor
2 [-Wunused-function] sound/drivers
3 [-Wunused-function] sound/pci
2 [-Wunused-function] sound/soc
4 [-Wvoid-pointer-to-enum-cast] drivers/ata
1 [-Wvoid-pointer-to-enum-cast] drivers/char
2 [-Wvoid-pointer-to-enum-cast] drivers/crypto
1 [-Wvoid-pointer-to-enum-cast] drivers/devfreq
2 [-Wvoid-pointer-to-enum-cast] drivers/dma
1 [-Wvoid-pointer-to-enum-cast] drivers/gpio
5 [-Wvoid-pointer-to-enum-cast] drivers/gpu
15 [-Wvoid-pointer-to-enum-cast] drivers/hwmon
3 [-Wvoid-pointer-to-enum-cast] drivers/i2c
10 [-Wvoid-pointer-to-enum-cast] drivers/iio
1 [-Wvoid-pointer-to-enum-cast] drivers/input
6 [-Wvoid-pointer-to-enum-cast] drivers/media
9 [-Wvoid-pointer-to-enum-cast] drivers/mfd
2 [-Wvoid-pointer-to-enum-cast] drivers/mtd
1 [-Wvoid-pointer-to-enum-cast] drivers/mux
9 [-Wvoid-pointer-to-enum-cast] drivers/net
7 [-Wvoid-pointer-to-enum-cast] drivers/phy
1 [-Wvoid-pointer-to-enum-cast] drivers/platform
2 [-Wvoid-pointer-to-enum-cast] drivers/regulator
1 [-Wvoid-pointer-to-enum-cast] drivers/reset
4 [-Wvoid-pointer-to-enum-cast] drivers/rtc
1 [-Wvoid-pointer-to-enum-cast] drivers/scsi
1 [-Wvoid-pointer-to-enum-cast] drivers/soc
2 [-Wvoid-pointer-to-enum-cast] drivers/spi
1 [-Wvoid-pointer-to-enum-cast] drivers/thermal
3 [-Wvoid-pointer-to-enum-cast] sound/soc

[Andrew Lunn]

On Sat, Oct 17, 2020 at 02:48:56PM +0200, Arnd Bergmann wrote:
> On Fri, Oct 16, 2020 at 4:12 PM Andrew Lunn <and...@lunn.ch> wrote:
> >
> > > One drawback of your approach is that
> > > you cannot set KBUILD_CFLAGS_W1_20200930
> > > until you eliminate all the warnings in the
> > > sub-directory in interest.
> > > (i.e. all or nothing approach)
> >
> > Hi Mashiro
> >
> > That actual works well for my use case. drivers/net/ethernet is W=1
> > clean. So is drivers/net/phy, drivers/net/mdio. Developers generally
> > clean up a subsystem by adding W=1 to the command line because that is
> > the simple tool they have.
> >
> > And my aim here is to keep those subsystem W=1 clean. I don't care
> > about individual warnings within W=1, because those subsystems are
> > passed that stage already.
>
> I tried to get a better grasp of what kind of warnings we are actually talking
> about and looked at the x86 allmodconfig W=1 output on today's linux-next.

Hi Arnd

The work done to cleanup drivers/net/ethernet was mostly done by an
Intel team. When built for ARM there are few warnings left, mostly due
to missing COMPILE_TEST. I have fixes for that.

But this raises the question, can we be a bit more tolerant of
warnings for not x86 to start with? 0-day should help us weed out the
remaining warnings on other architectures.

As for the plan, it looks O.K. to me. I can definitely help with
driver/net and net.

Andrew

[Arnd Bergmann]

On Sat, Oct 17, 2020 at 4:57 PM Andrew Lunn <and...@lunn.ch> wrote:
> On Sat, Oct 17, 2020 at 02:48:56PM +0200, Arnd Bergmann wrote:
> > I tried to get a better grasp of what kind of warnings we are actually talking
> > about and looked at the x86 allmodconfig W=1 output on today's linux-next.
>

> The work done to cleanup drivers/net/ethernet was mostly done by an
> Intel team. When built for ARM there are few warnings left, mostly due
> to missing COMPILE_TEST. I have fixes for that.
>
> But this raises the question, can we be a bit more tolerant of
> warnings for not x86 to start with? 0-day should help us weed out the
> remaining warnings on other architectures.
>
> As for the plan, it looks O.K. to me. I can definitely help with
> driver/net and net.

I think there is a bit of a gradient: I generally try to keep arm (both 32
and 64 bit) warning free, and I know some of the other architecture
maintainers try the same, but generally x86 is ahead because of the
number of people looking at it. Other architectures (powerpc, s390,
mips) are somewhat behind but not too bad, and a lot of the rest
don't generally build cleanly to start with once you try anything beyond
a plain 'make defconfig' build.

While we don't have a formal definition of primary and secondary
architectures, I think we can take the number of existing warnings
as a baseline: if a build is currently clean (such as x86 allmodconfig
typically), don't break it by enabling additional warnings, but don't
worry too much about adding a small number of warnings on builds
that already have a lot of them. This is similar to how we deal with
warnings added by new compiler versions: the more actively
maintained architectures are the first to fix them, and anything that
seems like too much work gets disabled, but the less active
architectures just end up with more warnings.

Arnd

[Arnd Bergmann]

On Sat, Oct 17, 2020 at 2:48 PM Arnd Bergmann <ar...@arndb.de> wrote:
> On Fri, Oct 16, 2020 at 4:12 PM Andrew Lunn <and...@lunn.ch> wrote:

> - Turn on -Wold-style-definition -Wmissing-include-dirs -Wstringop-truncation
> globally, since they don't seem to cause any output any more.
>
> - Work on fixing all -Wextra warnings (except -Woverride-init
> -Wunused-parameter -Wmissing-field-initializers -Wtype-limits)
> across the tree, then enable -Wextra unconditionally. I see this
> being relevant to only 25 files in the allmodconfig build, which is
> much less than I expected for -Wextra. There are a couple more
> for clang as well, but not that many either.

I've done these now, uploaded a branch to

git://git.kernel.org:/pub/scm/linux/kernel/git/arnd/playground.git Wextra

Arnd