| Message ID | 1604641050-6004-1-git-send-email-alex.shi@linux.alibaba.com |
|---|---|
| State | New |
| Headers | show |
| Series | net/dsa: remove unused macros to tame gcc warning | expand |
On Fri, 2020-11-06 at 13:37 +0800, Alex Shi wrote: > There are some macros unused, they causes much gcc warnings. Let's > remove them to tame gcc. I believe these to be essentially poor warnings. Aren't these warnings generated only when adding W=2 to the make command line? Perhaps it's better to move the warning to level 3 --- diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn index 95e4cdb94fe9..5c3c220ddb32 100644 --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -68,7 +68,6 @@ KBUILD_CFLAGS += $(call cc-option, -Wlogical-op) KBUILD_CFLAGS += -Wmissing-field-initializers KBUILD_CFLAGS += -Wtype-limits KBUILD_CFLAGS += $(call cc-option, -Wmaybe-uninitialized) -KBUILD_CFLAGS += $(call cc-option, -Wunused-macros) KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN2 @@ -89,6 +88,7 @@ KBUILD_CFLAGS += -Wredundant-decls KBUILD_CFLAGS += -Wsign-compare KBUILD_CFLAGS += -Wswitch-default KBUILD_CFLAGS += $(call cc-option, -Wpacked-bitfield-compat) +KBUILD_CFLAGS += $(call cc-option, -Wunused-macros) KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN3
在 2020/11/6 下午2:36, Joe Perches 写道: > On Fri, 2020-11-06 at 13:37 +0800, Alex Shi wrote: >> There are some macros unused, they causes much gcc warnings. Let's >> remove them to tame gcc. > > I believe these to be essentially poor warnings. > > Aren't these warnings generated only when adding W=2 to the make > command line? > > Perhaps it's better to move the warning to level 3 > --- > diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn > index 95e4cdb94fe9..5c3c220ddb32 100644 > --- a/scripts/Makefile.extrawarn > +++ b/scripts/Makefile.extrawarn > @@ -68,7 +68,6 @@ KBUILD_CFLAGS += $(call cc-option, -Wlogical-op) > KBUILD_CFLAGS += -Wmissing-field-initializers > KBUILD_CFLAGS += -Wtype-limits > KBUILD_CFLAGS += $(call cc-option, -Wmaybe-uninitialized) > -KBUILD_CFLAGS += $(call cc-option, -Wunused-macros) This changed too much, and impact others. May not good. :) Thanks Alex > > KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN2 > > @@ -89,6 +88,7 @@ KBUILD_CFLAGS += -Wredundant-decls > KBUILD_CFLAGS += -Wsign-compare > KBUILD_CFLAGS += -Wswitch-default > KBUILD_CFLAGS += $(call cc-option, -Wpacked-bitfield-compat) > +KBUILD_CFLAGS += $(call cc-option, -Wunused-macros) > > KBUILD_CPPFLAGS += -DKBUILD_EXTRA_WARN3 > >
On Fri, 2020-11-06 at 16:28 +0800, Alex Shi wrote: > > 在 2020/11/6 下午2:36, Joe Perches 写道: > > On Fri, 2020-11-06 at 13:37 +0800, Alex Shi wrote: > > > There are some macros unused, they causes much gcc warnings. Let's > > > remove them to tame gcc. > > > > I believe these to be essentially poor warnings. > > > > Aren't these warnings generated only when adding W=2 to the make > > command line? > > > > Perhaps it's better to move the warning to level 3 > > --- > > diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn > > index 95e4cdb94fe9..5c3c220ddb32 100644 > > --- a/scripts/Makefile.extrawarn > > +++ b/scripts/Makefile.extrawarn > > @@ -68,7 +68,6 @@ KBUILD_CFLAGS += $(call cc-option, -Wlogical-op) > > KBUILD_CFLAGS += -Wmissing-field-initializers > > KBUILD_CFLAGS += -Wtype-limits > > KBUILD_CFLAGS += $(call cc-option, -Wmaybe-uninitialized) > > -KBUILD_CFLAGS += $(call cc-option, -Wunused-macros) > > This changed too much, and impact others. May not good. :) Can you clarify what you mean?
在 2020/11/6 下午4:52, Joe Perches 写道: > On Fri, 2020-11-06 at 16:28 +0800, Alex Shi wrote: >> >> 在 2020/11/6 下午2:36, Joe Perches 写道: >>> On Fri, 2020-11-06 at 13:37 +0800, Alex Shi wrote: >>>> There are some macros unused, they causes much gcc warnings. Let's >>>> remove them to tame gcc. >>> >>> I believe these to be essentially poor warnings. >>> >>> Aren't these warnings generated only when adding W=2 to the make >>> command line? >>> >>> Perhaps it's better to move the warning to level 3 >>> --- >>> diff --git a/scripts/Makefile.extrawarn b/scripts/Makefile.extrawarn >>> index 95e4cdb94fe9..5c3c220ddb32 100644 >>> --- a/scripts/Makefile.extrawarn >>> +++ b/scripts/Makefile.extrawarn >>> @@ -68,7 +68,6 @@ KBUILD_CFLAGS += $(call cc-option, -Wlogical-op) >>> KBUILD_CFLAGS += -Wmissing-field-initializers >>> KBUILD_CFLAGS += -Wtype-limits >>> KBUILD_CFLAGS += $(call cc-option, -Wmaybe-uninitialized) >>> -KBUILD_CFLAGS += $(call cc-option, -Wunused-macros) >> >> This changed too much, and impact others. May not good. :) > > Can you clarify what you mean? > Uh, change in this file will impact all kernels not only for net/dsa. If you want to keep these unused macros, maybe better to put them into comments? Thanks! Alex
From: Joe Perches > Sent: 06 November 2020 06:36 > > On Fri, 2020-11-06 at 13:37 +0800, Alex Shi wrote: > > There are some macros unused, they causes much gcc warnings. Let's > > remove them to tame gcc. > > I believe these to be essentially poor warnings. Indeed. One 'solution' is to move the #defines into an included .h file. > Aren't these warnings generated only when adding W=2 to the make > command line? > > Perhaps it's better to move the warning to level 3 I'd move then to level 9999999999. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Fri, Nov 06, 2020 at 01:37:30PM +0800, Alex Shi wrote: > There are some macros unused, they causes much gcc warnings. Let's > remove them to tame gcc. > > net/dsa/tag_brcm.c:51:0: warning: macro "BRCM_EG_RC_SWITCH" is not used > [-Wunused-macros] > net/dsa/tag_brcm.c:53:0: warning: macro "BRCM_EG_RC_MIRROR" is not used > [-Wunused-macros] > net/dsa/tag_brcm.c:55:0: warning: macro "BRCM_EG_TC_MASK" is not used > [-Wunused-macros] > net/dsa/tag_brcm.c:35:0: warning: macro "BRCM_IG_TS_SHIFT" is not used > [-Wunused-macros] > net/dsa/tag_brcm.c:46:0: warning: macro "BRCM_EG_RC_MASK" is not used > [-Wunused-macros] > net/dsa/tag_brcm.c:49:0: warning: macro "BRCM_EG_RC_PROT_SNOOP" is not > used [-Wunused-macros] > net/dsa/tag_brcm.c:34:0: warning: macro "BRCM_IG_TE_MASK" is not used > [-Wunused-macros] > net/dsa/tag_brcm.c:43:0: warning: macro "BRCM_EG_CID_MASK" is not used > [-Wunused-macros] > net/dsa/tag_brcm.c:50:0: warning: macro "BRCM_EG_RC_PROT_TERM" is not > used [-Wunused-macros] > net/dsa/tag_brcm.c:54:0: warning: macro "BRCM_EG_TC_SHIFT" is not used > [-Wunused-macros] > net/dsa/tag_brcm.c:52:0: warning: macro "BRCM_EG_RC_MAC_LEARN" is not > used [-Wunused-macros] > net/dsa/tag_brcm.c:48:0: warning: macro "BRCM_EG_RC_EXCEPTION" is not > used [-Wunused-macros] > > Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> > Cc: Andrew Lunn <andrew@lunn.ch> > Cc: Vivien Didelot <vivien.didelot@gmail.com> > Cc: Florian Fainelli <f.fainelli@gmail.com> > Cc: Vladimir Oltean <olteanv@gmail.com> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: netdev@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > --- > net/dsa/tag_brcm.c | 15 --------------- > 1 file changed, 15 deletions(-) > > diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c > index e934dace3922..ce23b5d4c6b8 100644 > --- a/net/dsa/tag_brcm.c > +++ b/net/dsa/tag_brcm.c > @@ -30,29 +30,14 @@ > /* 1st byte in the tag */ > #define BRCM_IG_TC_SHIFT 2 > #define BRCM_IG_TC_MASK 0x7 > -/* 2nd byte in the tag */ > -#define BRCM_IG_TE_MASK 0x3 > -#define BRCM_IG_TS_SHIFT 7 > /* 3rd byte in the tag */ > #define BRCM_IG_DSTMAP2_MASK 1 > #define BRCM_IG_DSTMAP1_MASK 0xff Hi Alex It is good to remember that there are multiple readers of source files. There is the compiler which generates code from it, and there is the human trying to understand what is going on, what the hardware can do, how we could maybe extend the code in the future to make use of bits are currently don't, etc. The compiler has no use of these macros, at the moment. But i as a human do. It is valuable documentation, given that there is no open datasheet for this hardware. I would say these warnings are bogus, and the code should be left alone. Andrew
On 11/6/2020 6:18 AM, Andrew Lunn wrote: > On Fri, Nov 06, 2020 at 01:37:30PM +0800, Alex Shi wrote: >> There are some macros unused, they causes much gcc warnings. Let's >> remove them to tame gcc. >> >> net/dsa/tag_brcm.c:51:0: warning: macro "BRCM_EG_RC_SWITCH" is not used >> [-Wunused-macros] >> net/dsa/tag_brcm.c:53:0: warning: macro "BRCM_EG_RC_MIRROR" is not used >> [-Wunused-macros] >> net/dsa/tag_brcm.c:55:0: warning: macro "BRCM_EG_TC_MASK" is not used >> [-Wunused-macros] >> net/dsa/tag_brcm.c:35:0: warning: macro "BRCM_IG_TS_SHIFT" is not used >> [-Wunused-macros] >> net/dsa/tag_brcm.c:46:0: warning: macro "BRCM_EG_RC_MASK" is not used >> [-Wunused-macros] >> net/dsa/tag_brcm.c:49:0: warning: macro "BRCM_EG_RC_PROT_SNOOP" is not >> used [-Wunused-macros] >> net/dsa/tag_brcm.c:34:0: warning: macro "BRCM_IG_TE_MASK" is not used >> [-Wunused-macros] >> net/dsa/tag_brcm.c:43:0: warning: macro "BRCM_EG_CID_MASK" is not used >> [-Wunused-macros] >> net/dsa/tag_brcm.c:50:0: warning: macro "BRCM_EG_RC_PROT_TERM" is not >> used [-Wunused-macros] >> net/dsa/tag_brcm.c:54:0: warning: macro "BRCM_EG_TC_SHIFT" is not used >> [-Wunused-macros] >> net/dsa/tag_brcm.c:52:0: warning: macro "BRCM_EG_RC_MAC_LEARN" is not >> used [-Wunused-macros] >> net/dsa/tag_brcm.c:48:0: warning: macro "BRCM_EG_RC_EXCEPTION" is not >> used [-Wunused-macros] >> >> Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> >> Cc: Andrew Lunn <andrew@lunn.ch> >> Cc: Vivien Didelot <vivien.didelot@gmail.com> >> Cc: Florian Fainelli <f.fainelli@gmail.com> >> Cc: Vladimir Oltean <olteanv@gmail.com> >> Cc: "David S. Miller" <davem@davemloft.net> >> Cc: Jakub Kicinski <kuba@kernel.org> >> Cc: netdev@vger.kernel.org >> Cc: linux-kernel@vger.kernel.org >> --- >> net/dsa/tag_brcm.c | 15 --------------- >> 1 file changed, 15 deletions(-) >> >> diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c >> index e934dace3922..ce23b5d4c6b8 100644 >> --- a/net/dsa/tag_brcm.c >> +++ b/net/dsa/tag_brcm.c >> @@ -30,29 +30,14 @@ >> /* 1st byte in the tag */ >> #define BRCM_IG_TC_SHIFT 2 >> #define BRCM_IG_TC_MASK 0x7 >> -/* 2nd byte in the tag */ >> -#define BRCM_IG_TE_MASK 0x3 >> -#define BRCM_IG_TS_SHIFT 7 >> /* 3rd byte in the tag */ >> #define BRCM_IG_DSTMAP2_MASK 1 >> #define BRCM_IG_DSTMAP1_MASK 0xff > > Hi Alex > > It is good to remember that there are multiple readers of source > files. There is the compiler which generates code from it, and there > is the human trying to understand what is going on, what the hardware > can do, how we could maybe extend the code in the future to make use > of bits are currently don't, etc. > > The compiler has no use of these macros, at the moment. But i as a > human do. It is valuable documentation, given that there is no open > datasheet for this hardware. > > I would say these warnings are bogus, and the code should be left > alone. Agreed, these definitions are intended to document what the hardware does. These warnings are getting too far. -- Florian
在 2020/11/7 上午12:39, Florian Fainelli 写道: >> Hi Alex >> >> It is good to remember that there are multiple readers of source >> files. There is the compiler which generates code from it, and there >> is the human trying to understand what is going on, what the hardware >> can do, how we could maybe extend the code in the future to make use >> of bits are currently don't, etc. >> >> The compiler has no use of these macros, at the moment. But i as a >> human do. It is valuable documentation, given that there is no open >> datasheet for this hardware. >> >> I would say these warnings are bogus, and the code should be left >> alone. > Agreed, these definitions are intended to document what the hardware > does. These warnings are getting too far. > -- Thanks for all comments! I agree these info are much meaningful. Is there other way to tame the gcc warning? like put them into a .h file or covered by comments? Thanks Alex
On Sat, 2020-11-07 at 20:54 +0800, Alex Shi wrote: > 在 2020/11/7 上午12:39, Florian Fainelli 写道: > > > It is good to remember that there are multiple readers of source > > > files. There is the compiler which generates code from it, and there > > > is the human trying to understand what is going on, what the hardware > > > can do, how we could maybe extend the code in the future to make use > > > of bits are currently don't, etc. > > > > > > The compiler has no use of these macros, at the moment. But i as a > > > human do. It is valuable documentation, given that there is no open > > > datasheet for this hardware. > > > > > > I would say these warnings are bogus, and the code should be left > > > alone. > > Agreed, these definitions are intended to document what the hardware > > does. These warnings are getting too far. > > Thanks for all comments! I agree these info are much meaningful. > Is there other way to tame the gcc warning? like put them into a .h file > or covered by comments? Does _any_ version of gcc have this warning on by default? I still think my proposal of moving the warning from W=2 to W=3 quite reasonable. Another possibility is to turn the warning off altogether.
On Sat, Nov 07, 2020 at 09:39:42AM -0800, Joe Perches wrote: > On Sat, 2020-11-07 at 20:54 +0800, Alex Shi wrote: > > 在 2020/11/7 上午12:39, Florian Fainelli 写道: > > > > It is good to remember that there are multiple readers of source > > > > files. There is the compiler which generates code from it, and there > > > > is the human trying to understand what is going on, what the hardware > > > > can do, how we could maybe extend the code in the future to make use > > > > of bits are currently don't, etc. > > > > > > > > The compiler has no use of these macros, at the moment. But i as a > > > > human do. It is valuable documentation, given that there is no open > > > > datasheet for this hardware. > > > > > > > > I would say these warnings are bogus, and the code should be left > > > > alone. > > > Agreed, these definitions are intended to document what the hardware > > > does. These warnings are getting too far. > > > > Thanks for all comments! I agree these info are much meaningful. > > Is there other way to tame the gcc warning? like put them into a .h file > > or covered by comments? > > Does _any_ version of gcc have this warning on by default? > > I still think my proposal of moving the warning from W=2 to W=3 > quite reasonable. > > Another possibility is to turn the warning off altogether. Lets tern the question around first. How many real bugs have you found with this warning? Places where the #define should of been used, but was not? Then we can get an idea of the value of this warning. My guess would be, its value is ~ 0 for the kernel. If so, we should just turn it off. Andrew
From: Andrew Lunn > Sent: 07 November 2020 22:33 > > On Sat, Nov 07, 2020 at 09:39:42AM -0800, Joe Perches wrote: > > On Sat, 2020-11-07 at 20:54 +0800, Alex Shi wrote: > > > 在 2020/11/7 上午12:39, Florian Fainelli 写道: > > > > > It is good to remember that there are multiple readers of source > > > > > files. There is the compiler which generates code from it, and there > > > > > is the human trying to understand what is going on, what the hardware > > > > > can do, how we could maybe extend the code in the future to make use > > > > > of bits are currently don't, etc. > > > > > > > > > > The compiler has no use of these macros, at the moment. But i as a > > > > > human do. It is valuable documentation, given that there is no open > > > > > datasheet for this hardware. > > > > > > > > > > I would say these warnings are bogus, and the code should be left > > > > > alone. > > > > Agreed, these definitions are intended to document what the hardware > > > > does. These warnings are getting too far. > > > > > > Thanks for all comments! I agree these info are much meaningful. > > > Is there other way to tame the gcc warning? like put them into a .h file > > > or covered by comments? > > > > Does _any_ version of gcc have this warning on by default? > > > > I still think my proposal of moving the warning from W=2 to W=3 > > quite reasonable. > > > > Another possibility is to turn the warning off altogether. > > Lets tern the question around first. How many real bugs have you found > with this warning? Places where the #define should of been used, but > was not? Then we can get an idea of the value of this warning. My > guess would be, its value is ~ 0 for the kernel. If so, we should just > turn it off. Sometimes there are defines for 0, 1 and 2 (enumeration or bitmap) and the one for 0 is never used. Rather than delete the define for 0 (which has been proposed at least once in this series) the 'fix' would be to find the initialisation and replace the literal 0 with the define. OTOH it is probably implicit from a memset(). Even if never actually used the define for 0 helps the human reading the code. The same is true when there are defines for hardware register bits. It is useful to know what they all mean - even if some aren't used. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c index e934dace3922..ce23b5d4c6b8 100644 --- a/net/dsa/tag_brcm.c +++ b/net/dsa/tag_brcm.c @@ -30,29 +30,14 @@ /* 1st byte in the tag */ #define BRCM_IG_TC_SHIFT 2 #define BRCM_IG_TC_MASK 0x7 -/* 2nd byte in the tag */ -#define BRCM_IG_TE_MASK 0x3 -#define BRCM_IG_TS_SHIFT 7 /* 3rd byte in the tag */ #define BRCM_IG_DSTMAP2_MASK 1 #define BRCM_IG_DSTMAP1_MASK 0xff /* Egress fields */ -/* 2nd byte in the tag */ -#define BRCM_EG_CID_MASK 0xff - /* 3rd byte in the tag */ -#define BRCM_EG_RC_MASK 0xff #define BRCM_EG_RC_RSVD (3 << 6) -#define BRCM_EG_RC_EXCEPTION (1 << 5) -#define BRCM_EG_RC_PROT_SNOOP (1 << 4) -#define BRCM_EG_RC_PROT_TERM (1 << 3) -#define BRCM_EG_RC_SWITCH (1 << 2) -#define BRCM_EG_RC_MAC_LEARN (1 << 1) -#define BRCM_EG_RC_MIRROR (1 << 0) -#define BRCM_EG_TC_SHIFT 5 -#define BRCM_EG_TC_MASK 0x7 #define BRCM_EG_PID_MASK 0x1f #if IS_ENABLED(CONFIG_NET_DSA_TAG_BRCM) || \
There are some macros unused, they causes much gcc warnings. Let's remove them to tame gcc. net/dsa/tag_brcm.c:51:0: warning: macro "BRCM_EG_RC_SWITCH" is not used [-Wunused-macros] net/dsa/tag_brcm.c:53:0: warning: macro "BRCM_EG_RC_MIRROR" is not used [-Wunused-macros] net/dsa/tag_brcm.c:55:0: warning: macro "BRCM_EG_TC_MASK" is not used [-Wunused-macros] net/dsa/tag_brcm.c:35:0: warning: macro "BRCM_IG_TS_SHIFT" is not used [-Wunused-macros] net/dsa/tag_brcm.c:46:0: warning: macro "BRCM_EG_RC_MASK" is not used [-Wunused-macros] net/dsa/tag_brcm.c:49:0: warning: macro "BRCM_EG_RC_PROT_SNOOP" is not used [-Wunused-macros] net/dsa/tag_brcm.c:34:0: warning: macro "BRCM_IG_TE_MASK" is not used [-Wunused-macros] net/dsa/tag_brcm.c:43:0: warning: macro "BRCM_EG_CID_MASK" is not used [-Wunused-macros] net/dsa/tag_brcm.c:50:0: warning: macro "BRCM_EG_RC_PROT_TERM" is not used [-Wunused-macros] net/dsa/tag_brcm.c:54:0: warning: macro "BRCM_EG_TC_SHIFT" is not used [-Wunused-macros] net/dsa/tag_brcm.c:52:0: warning: macro "BRCM_EG_RC_MAC_LEARN" is not used [-Wunused-macros] net/dsa/tag_brcm.c:48:0: warning: macro "BRCM_EG_RC_EXCEPTION" is not used [-Wunused-macros] Signed-off-by: Alex Shi <alex.shi@linux.alibaba.com> Cc: Andrew Lunn <andrew@lunn.ch> Cc: Vivien Didelot <vivien.didelot@gmail.com> Cc: Florian Fainelli <f.fainelli@gmail.com> Cc: Vladimir Oltean <olteanv@gmail.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> Cc: netdev@vger.kernel.org Cc: linux-kernel@vger.kernel.org --- net/dsa/tag_brcm.c | 15 --------------- 1 file changed, 15 deletions(-)