[PATCH RFC/RFT 0/2] W=1 by default for Ethernet PHY subsystem

[PATCH RFC/RFT 0/2] W=1 by default for Ethernet PHY subsystem

[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.

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?

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

[PATCH RFC/RFT 1/2] scripts: Makefile.extrawarn: Add W=1 warnings to a symbol

[Andrew Lunn]

There is a desire that subtrees can enable W=1 by default. To make
this possible, put the extra compiler flags into an exported variable,
so other Makefiles can make use of them.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 scripts/Makefile.extrawarn | 33 ++++++++++++++++++---------------
 1 file changed, 18 insertions(+), 15 deletions(-)

diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn
index 95e4cdb94fe9..bf0de3502849 100644
--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -20,23 +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_WARN1 += -Wextra -Wunused -Wno-unused-parameter
+KBUILD_CFLAGS_WARN1 += -Wmissing-declarations
+KBUILD_CFLAGS_WARN1 += -Wmissing-format-attribute
+KBUILD_CFLAGS_WARN1 += -Wmissing-prototypes
+KBUILD_CFLAGS_WARN1 += -Wold-style-definition
+KBUILD_CFLAGS_WARN1 += -Wmissing-include-dirs
+KBUILD_CFLAGS_WARN1 += $(call cc-option, -Wunused-but-set-variable)
+KBUILD_CFLAGS_WARN1 += $(call cc-option, -Wunused-const-variable)
+KBUILD_CFLAGS_WARN1 += $(call cc-option, -Wpacked-not-aligned)
+KBUILD_CFLAGS_WARN1 += $(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_WARN1 += -Wno-missing-field-initializers
+KBUILD_CFLAGS_WARN1 += -Wno-sign-compare
+KBUILD_CFLAGS_WARN1 += -Wno-type-limits
+
+export KBUILD_CFLAGS_WARN1
+
+ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)
 
+KBUILD_CFLAGS += $(KBUILD_CFLAGS_WARN1)
 KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN1
 
 else
-- 
2.28.0

[PATCH RFC/RFT 2/2] net: phylib: Enable W=1 by default

[Andrew Lunn]

Add to the subdirectory ccflags variable the additional compiler
warnings which W=1 adds at the top level when enabled.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/mdio/Makefile | 3 +++
 drivers/net/pcs/Makefile  | 3 +++
 drivers/net/phy/Makefile  | 3 +++
 3 files changed, 9 insertions(+)

diff --git a/drivers/net/mdio/Makefile b/drivers/net/mdio/Makefile
index 14d1beb633c9..14600552eb8b 100644
--- a/drivers/net/mdio/Makefile
+++ b/drivers/net/mdio/Makefile
@@ -1,6 +1,9 @@
 # SPDX-License-Identifier: GPL-2.0
 # Makefile for Linux MDIO bus drivers
 
+# Enable W=1 by default
+subdir-ccflags-y := $(KBUILD_CFLAGS_WARN1)
+
 obj-$(CONFIG_MDIO_ASPEED)		+= mdio-aspeed.o
 obj-$(CONFIG_MDIO_BCM_IPROC)		+= mdio-bcm-iproc.o
 obj-$(CONFIG_MDIO_BCM_UNIMAC)		+= mdio-bcm-unimac.o
diff --git a/drivers/net/pcs/Makefile b/drivers/net/pcs/Makefile
index c23146755972..385b5765e390 100644
--- a/drivers/net/pcs/Makefile
+++ b/drivers/net/pcs/Makefile
@@ -1,5 +1,8 @@
 # SPDX-License-Identifier: GPL-2.0
 # Makefile for Linux PCS drivers
 
+# Enable W=1 by default
+subdir-ccflags-y := $(KBUILD_CFLAGS_WARN1)
+
 obj-$(CONFIG_PCS_XPCS)		+= pcs-xpcs.o
 obj-$(CONFIG_PCS_LYNX)		+= pcs-lynx.o
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index a13e402074cf..c49d40dfb6ec 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -1,6 +1,9 @@
 # SPDX-License-Identifier: GPL-2.0
 # Makefile for Linux PHY drivers
 
+# Enable W=1 by default
+subdir-ccflags-y := $(KBUILD_CFLAGS_WARN1)
+
 libphy-y			:= phy.o phy-c45.o phy-core.o phy_device.o \
 				   linkmode.o
 mdio-bus-y			+= mdio_bus.o mdio_device.o
-- 
2.28.0

Re: [PATCH RFC/RFT 0/2] W=1 by default for Ethernet PHY subsystem

[Masahiro Yamada]

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

Re: [PATCH RFC/RFT 0/2] W=1 by default for Ethernet PHY subsystem

[Andrew Lunn]

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

Re: [PATCH RFC/RFT 0/2] W=1 by default for Ethernet PHY subsystem

[Jason Gunthorpe]

On Sun, Sep 20, 2020 at 04:53:51PM +0200, Andrew Lunn wrote:

> 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.

I think this is a reasonable idea.

I'm hitting exactly the issue this series is trying to solve, Lee
invested a lot of effort to make drivers/infiniband/ W=1 clean, but
as maintainer I can't sustain this since there is no easy way to have
a warning free compile and get all extra warnings.  Also all my
submitters are not running with W=1

I need kbuild to get everyone on the same page to be able to sustain
the warning clean up. We've already had a regression and it has only
been a few weeks :(

Andrew, would you consider respinning this series in the above form?

Thanks,
Jason

Re: [PATCH RFC/RFT 0/2] W=1 by default for Ethernet PHY subsystem

[Jakub Kicinski]

On Wed, 10 Feb 2021 14:39:17 -0400 Jason Gunthorpe wrote:
> On Sun, Sep 20, 2020 at 04:53:51PM +0200, Andrew Lunn wrote:
> 
> > 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.  
> 
> I think this is a reasonable idea.
> 
> I'm hitting exactly the issue this series is trying to solve, Lee
> invested a lot of effort to make drivers/infiniband/ W=1 clean, but
> as maintainer I can't sustain this since there is no easy way to have
> a warning free compile and get all extra warnings.  Also all my
> submitters are not running with W=1
> 
> I need kbuild to get everyone on the same page to be able to sustain
> the warning clean up. We've already had a regression and it has only
> been a few weeks :(

Do you use patchwork? A little bit of automation is all you need,
really. kbuild bot is too slow, anyway.

> Andrew, would you consider respinning this series in the above form?

Re: [PATCH RFC/RFT 0/2] W=1 by default for Ethernet PHY subsystem

[Jason Gunthorpe]

On Wed, Feb 10, 2021 at 10:43:29AM -0800, Jakub Kicinski wrote:
> On Wed, 10 Feb 2021 14:39:17 -0400 Jason Gunthorpe wrote:
> > On Sun, Sep 20, 2020 at 04:53:51PM +0200, Andrew Lunn wrote:
> > 
> > > 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.  
> > 
> > I think this is a reasonable idea.
> > 
> > I'm hitting exactly the issue this series is trying to solve, Lee
> > invested a lot of effort to make drivers/infiniband/ W=1 clean, but
> > as maintainer I can't sustain this since there is no easy way to have
> > a warning free compile and get all extra warnings.  Also all my
> > submitters are not running with W=1
> > 
> > I need kbuild to get everyone on the same page to be able to sustain
> > the warning clean up. We've already had a regression and it has only
> > been a few weeks :(
> 
> Do you use patchwork? A little bit of automation is all you need,
> really. kbuild bot is too slow, anyway.

Yes, I bookmarked your automation scripts, but getting it all setup
for the first time seems like a big mountain to climb..

I already do compile almost every patch, the problem is there is no
easy way to get W=1 set for just a subset of files and still compile
each file only once.

If this makefile change was done then my submitters would see the
warning even before posting a patch. It is already quite rare I see
patches with compile warnings by the time they get to me. Even
unlikable checkpatch warnings have been fairly low..

Jason

Re: [PATCH RFC/RFT 0/2] W=1 by default for Ethernet PHY subsystem

[Andrew Lunn]

On Wed, Feb 10, 2021 at 02:39:17PM -0400, Jason Gunthorpe wrote:
> On Sun, Sep 20, 2020 at 04:53:51PM +0200, Andrew Lunn wrote:
> 
> > 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.
> 
> I think this is a reasonable idea.
> 
> I'm hitting exactly the issue this series is trying to solve, Lee
> invested a lot of effort to make drivers/infiniband/ W=1 clean, but
> as maintainer I can't sustain this since there is no easy way to have
> a warning free compile and get all extra warnings.  Also all my
> submitters are not running with W=1
> 
> I need kbuild to get everyone on the same page to be able to sustain
> the warning clean up. We've already had a regression and it has only
> been a few weeks :(
> 
> Andrew, would you consider respinning this series in the above form?

Arnd has worked on the core parts. But i lost track of if his patches
got merged.

    Andrew