| Message ID | 20200910174826.511423-1-natechancellor@gmail.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: mvpp2: Initialize link in mvpp2_isr_handle_{xlg,gmac_internal} | expand |
From: Nathan Chancellor <natechancellor@gmail.com> Date: Thu, 10 Sep 2020 10:48:27 -0700 > Clang warns (trimmed for brevity): > > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3073:7: warning: > variable 'link' is used uninitialized whenever 'if' condition is false > [-Wsometimes-uninitialized] > if (val & MVPP22_XLG_STATUS_LINK_UP) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3075:31: note: > uninitialized use occurs here > mvpp2_isr_handle_link(port, link); > ^~~~ > ... > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3090:8: warning: > variable 'link' is used uninitialized whenever 'if' condition is false > [-Wsometimes-uninitialized] > if (val & MVPP2_GMAC_STATUS0_LINK_UP) > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3092:32: note: > uninitialized use occurs here > mvpp2_isr_handle_link(port, link); > ^~~~ > > Initialize link to false like it was before the refactoring that > happened around link status so that a valid valid is always passed into > mvpp2_isr_handle_link. > > Fixes: 36cfd3a6e52b ("net: mvpp2: restructure "link status" interrupt handling") > Link: https://github.com/ClangBuiltLinux/linux/issues/1151 > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> This got fixed via another change, a much mode simply one in fact, changing the existing assignments to be unconditional and of the form "link = (bits & MASK);"
On Thu, Sep 10, 2020 at 03:28:11PM -0700, David Miller wrote: > From: Nathan Chancellor <natechancellor@gmail.com> > Date: Thu, 10 Sep 2020 10:48:27 -0700 > > > Clang warns (trimmed for brevity): > > > > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3073:7: warning: > > variable 'link' is used uninitialized whenever 'if' condition is false > > [-Wsometimes-uninitialized] > > if (val & MVPP22_XLG_STATUS_LINK_UP) > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3075:31: note: > > uninitialized use occurs here > > mvpp2_isr_handle_link(port, link); > > ^~~~ > > ... > > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3090:8: warning: > > variable 'link' is used uninitialized whenever 'if' condition is false > > [-Wsometimes-uninitialized] > > if (val & MVPP2_GMAC_STATUS0_LINK_UP) > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3092:32: note: > > uninitialized use occurs here > > mvpp2_isr_handle_link(port, link); > > ^~~~ > > > > Initialize link to false like it was before the refactoring that > > happened around link status so that a valid valid is always passed into > > mvpp2_isr_handle_link. > > > > Fixes: 36cfd3a6e52b ("net: mvpp2: restructure "link status" interrupt handling") > > Link: https://github.com/ClangBuiltLinux/linux/issues/1151 > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > This got fixed via another change, a much mode simply one in fact, > changing the existing assignments to be unconditional and of the > form "link = (bits & MASK);" Ah great, that is indeed cleaner, thank you for letting me know! Cheers, Nathan
On Thu, Sep 10, 2020 at 05:31:42PM -0700, Nathan Chancellor wrote: > On Thu, Sep 10, 2020 at 03:28:11PM -0700, David Miller wrote: > > From: Nathan Chancellor <natechancellor@gmail.com> > > Date: Thu, 10 Sep 2020 10:48:27 -0700 > > > > > Clang warns (trimmed for brevity): > > > > > > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3073:7: warning: > > > variable 'link' is used uninitialized whenever 'if' condition is false > > > [-Wsometimes-uninitialized] > > > if (val & MVPP22_XLG_STATUS_LINK_UP) > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3075:31: note: > > > uninitialized use occurs here > > > mvpp2_isr_handle_link(port, link); > > > ^~~~ > > > ... > > > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3090:8: warning: > > > variable 'link' is used uninitialized whenever 'if' condition is false > > > [-Wsometimes-uninitialized] > > > if (val & MVPP2_GMAC_STATUS0_LINK_UP) > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3092:32: note: > > > uninitialized use occurs here > > > mvpp2_isr_handle_link(port, link); > > > ^~~~ > > > > > > Initialize link to false like it was before the refactoring that > > > happened around link status so that a valid valid is always passed into > > > mvpp2_isr_handle_link. > > > > > > Fixes: 36cfd3a6e52b ("net: mvpp2: restructure "link status" interrupt handling") > > > Link: https://github.com/ClangBuiltLinux/linux/issues/1151 > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > > This got fixed via another change, a much mode simply one in fact, > > changing the existing assignments to be unconditional and of the > > form "link = (bits & MASK);" > > Ah great, that is indeed cleaner, thank you for letting me know! Hmm, I'm not sure why gcc didn't find that. Strangely, the 0-day bot seems to have only picked up on it with clang, not gcc. Thanks for fixing.
On Fri, 11 Sep 2020 12:11:58 +0100 Russell King - ARM Linux admin wrote: > On Thu, Sep 10, 2020 at 05:31:42PM -0700, Nathan Chancellor wrote: > > Ah great, that is indeed cleaner, thank you for letting me know! > > Hmm, I'm not sure why gcc didn't find that. Strangely, the 0-day bot > seems to have only picked up on it with clang, not gcc. May be similar to: https://lkml.org/lkml/2019/2/25/1092 Recent GCC is so bad at catching uninitialized vars I was considering build testing with GCC8 :/
On Fri, Sep 11, 2020 at 08:22:36AM -0700, Jakub Kicinski wrote: > On Fri, 11 Sep 2020 12:11:58 +0100 Russell King - ARM Linux admin wrote: > > On Thu, Sep 10, 2020 at 05:31:42PM -0700, Nathan Chancellor wrote: > > > Ah great, that is indeed cleaner, thank you for letting me know! > > > > Hmm, I'm not sure why gcc didn't find that. Strangely, the 0-day bot > > seems to have only picked up on it with clang, not gcc. > > May be similar to: https://lkml.org/lkml/2019/2/25/1092 > > Recent GCC is so bad at catching uninitialized vars I was considering > build testing with GCC8 :/ It is even simpler than that, the warning was straight up disabled in commit 78a5255ffb6a ("Stop the ad-hoc games with -Wno-maybe-initialized"). clang's -Wuninitialized and -Wsometimes-uninitialized are generally more accurate but can have some false positives because the semantic analysis phase happens before inlining and dead code elimination. Cheers, Nathan
On Fri, Sep 11, 2020 at 09:01:01AM -0700, Nathan Chancellor wrote: > On Fri, Sep 11, 2020 at 08:22:36AM -0700, Jakub Kicinski wrote: > > On Fri, 11 Sep 2020 12:11:58 +0100 Russell King - ARM Linux admin wrote: > > > On Thu, Sep 10, 2020 at 05:31:42PM -0700, Nathan Chancellor wrote: > > > > Ah great, that is indeed cleaner, thank you for letting me know! > > > > > > Hmm, I'm not sure why gcc didn't find that. Strangely, the 0-day bot > > > seems to have only picked up on it with clang, not gcc. > > > > May be similar to: https://lkml.org/lkml/2019/2/25/1092 > > > > Recent GCC is so bad at catching uninitialized vars I was considering > > build testing with GCC8 :/ > > It is even simpler than that, the warning was straight up disabled in > commit 78a5255ffb6a ("Stop the ad-hoc games with -Wno-maybe-initialized"). Great, so now rather than getting false positive warnings, we now get buggy code. That sounds like a good improvement to me. Not.
diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c index 7d86940747d1..0d5ee96f89b4 100644 --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c @@ -3064,7 +3064,7 @@ static void mvpp2_isr_handle_link(struct mvpp2_port *port, bool link) static void mvpp2_isr_handle_xlg(struct mvpp2_port *port) { - bool link; + bool link = false; u32 val; val = readl(port->base + MVPP22_XLG_INT_STAT); @@ -3078,7 +3078,7 @@ static void mvpp2_isr_handle_xlg(struct mvpp2_port *port) static void mvpp2_isr_handle_gmac_internal(struct mvpp2_port *port) { - bool link; + bool link = false; u32 val; if (phy_interface_mode_is_rgmii(port->phy_interface) ||
Clang warns (trimmed for brevity): drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3073:7: warning: variable 'link' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] if (val & MVPP22_XLG_STATUS_LINK_UP) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3075:31: note: uninitialized use occurs here mvpp2_isr_handle_link(port, link); ^~~~ ... drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3090:8: warning: variable 'link' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized] if (val & MVPP2_GMAC_STATUS0_LINK_UP) ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c:3092:32: note: uninitialized use occurs here mvpp2_isr_handle_link(port, link); ^~~~ Initialize link to false like it was before the refactoring that happened around link status so that a valid valid is always passed into mvpp2_isr_handle_link. Fixes: 36cfd3a6e52b ("net: mvpp2: restructure "link status" interrupt handling") Link: https://github.com/ClangBuiltLinux/linux/issues/1151 Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) base-commit: 4f6a5caf187ff5807cd5b4ea5678982c249bd964