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 <[email protected]>
---
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
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) ||
base-commit: 4f6a5caf187ff5807cd5b4ea5678982c249bd964
--
2.28.0
From: Nathan Chancellor <[email protected]>
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 <[email protected]>
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 <[email protected]>
> 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 <[email protected]>
>
> 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 <[email protected]>
> > 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 <[email protected]>
> >
> > 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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
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, 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 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.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!