Discussion:
[PATCH v2] drivers: net: xgene: Rewrite loop in xgene_enet_ecc_init()
(too old to reply)
Geert Uytterhoeven
2014-10-22 07:39:41 UTC
Permalink
drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c: In function =E2=80=98=
xgene_enet_ecc_init=E2=80=99:
drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:126: warning: =E2=80=98=
data=E2=80=99 may be used uninitialized in this function

Depending on the arbitrary value on the stack, the loop may terminate
too early, and cause a bogus -ENODEV failure.

Signed-off-by: Geert Uytterhoeven <***@linux-m68k.org>
---
v2: Rewrite the loop instead of pre-initializing data.

drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c | 12 +++++-------
1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c b/driver=
s/net/ethernet/apm/xgene/xgene_enet_sgmac.c
index e6d24c2101982444..2a497b38ed420495 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c
@@ -127,17 +127,15 @@ static int xgene_enet_ecc_init(struct xgene_enet_=
pdata *p)
int i;
=20
xgene_enet_wr_diag_csr(p, ENET_CFG_MEM_RAM_SHUTDOWN_ADDR, 0);
- for (i =3D 0; i < 10 && data !=3D ~0U ; i++) {
+ for (i =3D 0; i < 10; i++) {
usleep_range(100, 110);
data =3D xgene_enet_rd_diag_csr(p, ENET_BLOCK_MEM_RDY_ADDR);
+ if (data =3D=3D ~0U)
+ return 0;
}
=20
- if (data !=3D ~0U) {
- netdev_err(ndev, "Failed to release memory from shutdown\n");
- return -ENODEV;
- }
-
- return 0;
+ netdev_err(ndev, "Failed to release memory from shutdown\n");
+ return -ENODEV;
}
=20
static void xgene_enet_config_ring_if_assoc(struct xgene_enet_pdata *p=
)
--=20
1.9.1
David Miller
2014-10-22 19:34:36 UTC
Permalink
From: Geert Uytterhoeven <***@linux-m68k.org>
Date: Wed, 22 Oct 2014 09:39:41 +0200
drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:126: warning: ‘data’ may be used uninitialized in this function
Depending on the arbitrary value on the stack, the loop may terminate
too early, and cause a bogus -ENODEV failure.
---
v2: Rewrite the loop instead of pre-initializing data.
I hate to be a pest, but like the other patch of your's I think
a do { } while() works best here because the intent is clearly
to
Geert Uytterhoeven
2014-10-22 19:50:06 UTC
Permalink
Post by David Miller
Date: Wed, 22 Oct 2014 09:39:41 +0200
Post by Geert Uytterhoeven
drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c: In function =E2=80=
drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:126: warning: =E2=80=
=98data=E2=80=99 may be used uninitialized in this function
Post by David Miller
Post by Geert Uytterhoeven
Depending on the arbitrary value on the stack, the loop may terminat=
e
Post by David Miller
Post by Geert Uytterhoeven
too early, and cause a bogus -ENODEV failure.
---
v2: Rewrite the loop instead of pre-initializing data.
I hate to be a pest, but like the other patch of your's I think
a do { } while() works best here because the intent is clearly
to run the loop at least once, right?
I wanted to avoid checking for "data !=3D ~0U" twice: once to abort the=
loop,
and once to check if a timeout happened.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ***@linux-=
m68k.org

In personal conversations with technical people, I call myself a hacker=
=2E But
when I'm talking to journalists I just say "programmer" or something li=
ke that.
-- Linus Torvalds
David Miller
2014-10-22 20:12:57 UTC
Permalink
From: Geert Uytterhoeven <***@linux-m68k.org>
Date: Wed, 22 Oct 2014 21:50:06 +0200
Post by David Miller
Date: Wed, 22 Oct 2014 09:39:41 +0200
drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:126: warning: ‘data’ may be used uninitialized in this function
Depending on the arbitrary value on the stack, the loop may terminate
too early, and cause a bogus -ENODEV failure.
---
v2: Rewrite the loop instead of pre-initializing data.
I hate to be a pest, but like the other patch of your's I think
a do { } while() works best here because the intent is clearly
to run the loop at least once, right?
I wanted to avoid checking for "data != ~0U" twice: once to abort the loop,
and once to check if a timeout happened.
Hmmm:

do {
usleep_range(...);
data = ...();
if (data == ~0)
return 0;
} while (++i < 10);

netdev_err(...);
return -ENOD
Geert Uytterhoeven
2014-10-23 07:05:27 UTC
Permalink
Post by David Miller
Date: Wed, 22 Oct 2014 21:50:06 +0200
Post by David Miller
Date: Wed, 22 Oct 2014 09:39:41 +0200
Post by Geert Uytterhoeven
drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c: In function =E2=
drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:126: warning: =E2=
=80=98data=E2=80=99 may be used uninitialized in this function
Post by David Miller
Post by David Miller
Post by Geert Uytterhoeven
Depending on the arbitrary value on the stack, the loop may termin=
ate
Post by David Miller
Post by David Miller
Post by Geert Uytterhoeven
too early, and cause a bogus -ENODEV failure.
---
v2: Rewrite the loop instead of pre-initializing data.
I hate to be a pest, but like the other patch of your's I think
a do { } while() works best here because the intent is clearly
to run the loop at least once, right?
I wanted to avoid checking for "data !=3D ~0U" twice: once to abort =
the loop,
Post by David Miller
and once to check if a timeout happened.
do {
usleep_range(...);
data =3D ...();
if (data =3D=3D ~0)
return 0;
} while (++i < 10);
netdev_err(...);
return -ENODEV;
Why would you have to check data twice?
--=20
Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ***@linux-=
m68k.org

In personal conversations with technical people, I call myself a hacker=
=2E But
when I'm talking to journalists I just say "programmer" or something li=
ke that.
-- Linus Torvalds
Geert Uytterhoeven
2014-10-23 07:06:48 UTC
Permalink
Post by David Miller
Date: Wed, 22 Oct 2014 21:50:06 +0200
Post by David Miller
Date: Wed, 22 Oct 2014 09:39:41 +0200
Post by Geert Uytterhoeven
drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c: In function =E2=
drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:126: warning: =E2=
=80=98data=E2=80=99 may be used uninitialized in this function
Post by David Miller
Post by David Miller
Post by Geert Uytterhoeven
Depending on the arbitrary value on the stack, the loop may termin=
ate
Post by David Miller
Post by David Miller
Post by Geert Uytterhoeven
too early, and cause a bogus -ENODEV failure.
---
v2: Rewrite the loop instead of pre-initializing data.
I hate to be a pest, but like the other patch of your's I think
a do { } while() works best here because the intent is clearly
to run the loop at least once, right?
I wanted to avoid checking for "data !=3D ~0U" twice: once to abort =
the loop,
Post by David Miller
and once to check if a timeout happened.
do {
usleep_range(...);
data =3D ...();
if (data =3D=3D ~0)
return 0;
} while (++i < 10);
netdev_err(...);
return -ENODEV;
Why would you have to check data twice?
Yes, that would work to.

=46eel free to do s/for (i =3D 0; i < 10; i++)/do/ and s/}/} while (++i=
< 10);/

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ***@linux-=
m68k.org

In personal conversations with technical people, I call myself a hacker=
=2E But
when I'm talking to journalists I just say "programmer" or something li=
ke that.
-- Linus Torvalds

Loading...