| Message ID | 20200919190258.3673246-1-andrew@lunn.ch |
|---|---|
| Headers | show |
| Series | W=1 by default for Ethernet PHY subsystem | expand |
On Sun, Sep 20, 2020 at 4:03 AM Andrew Lunn <andrew@lunn.ch> wrote: > > 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. > > This patchset refactors the core Makefile warning code to allow the > additional warnings W=1 adds available to any Makefile. The Ethernet > PHY subsystem Makefiles then make use of this to make W=1 the default > for this subsystem. > > RFT since i've only tested with x86 and arm with a modern gcc. Is the > code really clean for older compilers? For clang? I appreciate your efforts for keeping your subsystems clean for W=1 builds, and I hope this work will be extended towards upper directory level, drivers/net/phy -> drivers/net -> drivers/. However, when we talk about W=1, we consider not only the current option set in W=1, but also options that might be added by future compilers because every GCC/Clang release adds new warning options. Let's say, the future release, GCC 14 would add a new option -Wfoo-bar, which is reasonable enough to be enabled by default, but doing so would emit a lot of warnings in the current kernel tree. We cannot add -Wfoo-bar to W=0 right away, because our general consensus is that the normal build should be warning-free. In the current routine, we add -Wfoo-bar to W=1 with hope we can gradually fix the code and eventually migrate it to W=0. It is not always easy to move W=1 options to W=0 when we have lots of code fixed. At least, 0-day bot iterates compile tests with W=1, so new code violating -Wfoo-bar would be blocked. 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. Do we require to fix the code under driver/net/ first? Or, should we add it to W=2 temporarily, then move it to W=1 once we fix drivers/net/? So, another idea might be hard-coding extra warnings like drivers/gpu/drm/i915/Makefile. For example, your subsystem already achieved -Wmissing-declarations free. You can add subdir-ccflags-y += -Wmissing-declarations to drivers/net/phy/Makefile. Once you fix all net drivers, you can move it to the parent, drivers/net/Makefile. Then, drivers/Makefile next, and if it reaches the top directory level, we can move it to W=0. Some W=1 options stay there just because we cannot fix lots of code. So, our code should be improved with regard to W=1 warnings, but we might need some clarification about how to do it gradually. Comments are appreciated. > Andrew Lunn (2): > scripts: Makefile.extrawarn: Add W=1 warnings to a symbol > net: phylib: Enable W=1 by default > > drivers/net/mdio/Makefile | 3 +++ > drivers/net/pcs/Makefile | 3 +++ > drivers/net/phy/Makefile | 3 +++ > scripts/Makefile.extrawarn | 33 ++++++++++++++++++--------------- > 4 files changed, 27 insertions(+), 15 deletions(-) > > -- > 2.28.0 > -- Best Regards Masahiro Yamada
On Sun, Sep 20, 2020 at 12:42:51PM +0900, Masahiro Yamada wrote: > On Sun, Sep 20, 2020 at 4:03 AM Andrew Lunn <andrew@lunn.ch> wrote: > > > > 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. > > > > This patchset refactors the core Makefile warning code to allow the > > additional warnings W=1 adds available to any Makefile. The Ethernet > > PHY subsystem Makefiles then make use of this to make W=1 the default > > for this subsystem. > > > > RFT since i've only tested with x86 and arm with a modern gcc. Is the > > code really clean for older compilers? For clang? > > > I appreciate your efforts for keeping your subsystems > clean for W=1 builds, and I hope this work will be > extended towards upper directory level, > drivers/net/phy -> drivers/net -> drivers/. It definitely is. drivers/net: https://www.spinics.net/lists/netdev/msg683687.html drivers/spi https://www.spinics.net/lists/linux-spi/msg23280.html drivers/mfd https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg2211644.html etc. > So, another idea might be hard-coding extra warnings > like drivers/gpu/drm/i915/Makefile. > > For example, your subsystem already achieved > -Wmissing-declarations free. > > You can add > > subdir-ccflags-y += -Wmissing-declarations > > to drivers/net/phy/Makefile. > > Once you fix all net drivers, you can move it to > the parent, drivers/net/Makefile. > > Then, drivers/Makefile next, and if it reaches > the top directory level, we can move it to W=0. Do you think this will scale? Lets just assume we do this at driver/ level. We have 141 subdirectories in driver/ . So we will end up with 141 subdir-ccflags-y += lines which we need to maintain. Given the current cleanup effort, many are going to be identical to todays W=1. How do we maintain those 141 lines when it is time to add a new flag to W=1? How often are new W=1 flags added? My patch exported KBUILD_CFLAGS_WARN1. How about instead we export KBUILD_CFLAGS_WARN1_20200920. A subsystem can then sign up to being W=1 clean as for the 20200920 definition of W=1. If you want to add a new warning KBUILD_CFLAGS_WARN1_20201031 := KBUILD_CFLAGS_WARN1_20200920 + "-Wghosts" W=1 will always use the latest. You then build with W=1, maybe by throwing it at 0-day, find which subsystems are still clean, and update their subdir-ccflags-y += line with the new timestamp? This should help with scaling, in that a subsystem is not dealing with a list of warnings, just a symbol that represents the warnings from a particular date? Or maybe others have better ideas? Andrew