| Message ID | 20161024154215.2863586-1-arnd@arndb.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
From: Of Arnd Bergmann > Sent: 24 October 2016 16:42 > On x86, the cw1200 driver produces a rather silly warning about the > possible use of the 'ret' variable without an initialization > presumably after being confused by the architecture specific definition > of WARN_ON: > > drivers/net/wireless/st/cw1200/wsm.c: In function wsm_handle_rx: > drivers/net/wireless/st/cw1200/wsm.c:1457:9: error: ret may be used uninitialized in this function [- > Werror=maybe-uninitialized] > > As the driver just checks the same variable twice here, we can simplify > it by removing the second condition, which makes it more readable and > avoids the warning. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/net/wireless/st/cw1200/wsm.c | 15 +++++++-------- > 1 file changed, 7 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/wireless/st/cw1200/wsm.c b/drivers/net/wireless/st/cw1200/wsm.c > index 680d60eabc75..094e6637ade2 100644 > --- a/drivers/net/wireless/st/cw1200/wsm.c > +++ b/drivers/net/wireless/st/cw1200/wsm.c > @@ -385,14 +385,13 @@ static int wsm_multi_tx_confirm(struct cw1200_common *priv, > if (WARN_ON(count <= 0)) > return -EINVAL; > > - if (count > 1) { > - /* We already released one buffer, now for the rest */ > - ret = wsm_release_tx_buffer(priv, count - 1); > - if (ret < 0) > - return ret; > - else if (ret > 0) > - cw1200_bh_wakeup(priv); > - } > + /* We already released one buffer, now for the rest */ > + ret = wsm_release_tx_buffer(priv, count - 1); > + if (ret < 0) > + return ret; > + > + if (ret > 0) > + cw1200_bh_wakeup(priv); That doesn't look equivalent to me (when count == 1). > > cw1200_debug_txed_multi(priv, count); > for (i = 0; i < count; ++i) { Convert this loop into a do ... while so the body executes at least once. David
On Tue, Oct 25, 2016 at 01:24:55PM +0000, David Laight wrote: > > - if (count > 1) { > > - /* We already released one buffer, now for the rest */ > > - ret = wsm_release_tx_buffer(priv, count - 1); > > - if (ret < 0) > > - return ret; > > - else if (ret > 0) > > - cw1200_bh_wakeup(priv); > > - } > > + /* We already released one buffer, now for the rest */ > > + ret = wsm_release_tx_buffer(priv, count - 1); > > + if (ret < 0) > > + return ret; > > + > > + if (ret > 0) > > + cw1200_bh_wakeup(priv); > > That doesn't look equivalent to me (when count == 1). I concur, this patch should not be applied in its current form. - Solomon
On Tuesday, October 25, 2016 1:24:55 PM CEST David Laight wrote: > > diff --git a/drivers/net/wireless/st/cw1200/wsm.c b/drivers/net/wireless/st/cw1200/wsm.c > > index 680d60eabc75..094e6637ade2 100644 > > --- a/drivers/net/wireless/st/cw1200/wsm.c > > +++ b/drivers/net/wireless/st/cw1200/wsm.c > > @@ -385,14 +385,13 @@ static int wsm_multi_tx_confirm(struct cw1200_common *priv, > > if (WARN_ON(count <= 0)) > > return -EINVAL; > > > > - if (count > 1) { > > - /* We already released one buffer, now for the rest */ > > - ret = wsm_release_tx_buffer(priv, count - 1); > > - if (ret < 0) > > - return ret; > > - else if (ret > 0) > > - cw1200_bh_wakeup(priv); > > - } > > + /* We already released one buffer, now for the rest */ > > + ret = wsm_release_tx_buffer(priv, count - 1); > > + if (ret < 0) > > + return ret; > > + > > + if (ret > 0) > > + cw1200_bh_wakeup(priv); > > That doesn't look equivalent to me (when count == 1). Ah, that's what I missed, thanks for pointing that out! > > > > cw1200_debug_txed_multi(priv, count); > > for (i = 0; i < count; ++i) { > > Convert this loop into a do ... while so the body executes at least once. Good idea. Version 2 coming now. Arnd
diff --git a/drivers/net/wireless/st/cw1200/wsm.c b/drivers/net/wireless/st/cw1200/wsm.c index 680d60eabc75..094e6637ade2 100644 --- a/drivers/net/wireless/st/cw1200/wsm.c +++ b/drivers/net/wireless/st/cw1200/wsm.c @@ -385,14 +385,13 @@ static int wsm_multi_tx_confirm(struct cw1200_common *priv, if (WARN_ON(count <= 0)) return -EINVAL; - if (count > 1) { - /* We already released one buffer, now for the rest */ - ret = wsm_release_tx_buffer(priv, count - 1); - if (ret < 0) - return ret; - else if (ret > 0) - cw1200_bh_wakeup(priv); - } + /* We already released one buffer, now for the rest */ + ret = wsm_release_tx_buffer(priv, count - 1); + if (ret < 0) + return ret; + + if (ret > 0) + cw1200_bh_wakeup(priv); cw1200_debug_txed_multi(priv, count); for (i = 0; i < count; ++i) {
On x86, the cw1200 driver produces a rather silly warning about the possible use of the 'ret' variable without an initialization presumably after being confused by the architecture specific definition of WARN_ON: drivers/net/wireless/st/cw1200/wsm.c: In function ‘wsm_handle_rx’: drivers/net/wireless/st/cw1200/wsm.c:1457:9: error: ‘ret’ may be used uninitialized in this function [-Werror=maybe-uninitialized] As the driver just checks the same variable twice here, we can simplify it by removing the second condition, which makes it more readable and avoids the warning. Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/net/wireless/st/cw1200/wsm.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)