Message ID | 20190507103434.16174-1-colin.king@canonical.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | [next] net: dsa: sja1105: fix check on while loop exit | expand |
On Tue, 7 May 2019 at 13:34, Colin King <colin.king@canonical.com> wrote: > > From: Colin Ian King <colin.king@canonical.com> > > The while-loop exit condition check is not correct; the > loop should continue if the returns from the function calls are > negative or the CRC status returns are invalid. Currently it > is ignoring the returns from the function calls. Fix this by > removing the status return checks and only break from the loop > at the very end when we know that all the success condtions have > been met. > > Kudos to Dan Carpenter for describing the correct fix. > > Addresses-Coverity: ("Uninitialized scalar variable") > Fixes: 8aa9ebccae87 ("net: dsa: Introduce driver for NXP SJA1105 5-port L2 switch") > Signed-off-by: Colin Ian King <colin.king@canonical.com> > --- > > V2: Discard my broken origina fix. Use correct fix as described by > Dan Carpenter. > --- > drivers/net/dsa/sja1105/sja1105_spi.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c > index 244a94ccfc18..40ac696adf63 100644 > --- a/drivers/net/dsa/sja1105/sja1105_spi.c > +++ b/drivers/net/dsa/sja1105/sja1105_spi.c > @@ -465,9 +465,11 @@ int sja1105_static_config_upload(struct sja1105_private *priv) > dev_err(dev, "Switch reported that configuration is " > "invalid, retrying...\n"); > continue; > + > } > - } while (--retries && (status.crcchkl == 1 || status.crcchkg == 1 || > - status.configs == 0 || status.ids == 1)); > + /* Success! */ > + break; > + } while (--retries); > > if (!retries) { > rc = -EIO; > -- > 2.20.1 > Hi Colin, Dan, Thanks for the fix. This portion below needs to be changed as well: "else if (retries != RETRIES - 1)" should become "else if (retries != RETRIES)" because on success, it now does not decrement retries due to the early break. Otherwise this message gets printed: "Succeeded after 0 tried" which was not the original intention. Please also remove the extraneous newline introduced at 468. Tested-by: Vladimir Oltean <olteanv@gmail.com> -Vladimir
diff --git a/drivers/net/dsa/sja1105/sja1105_spi.c b/drivers/net/dsa/sja1105/sja1105_spi.c index 244a94ccfc18..40ac696adf63 100644 --- a/drivers/net/dsa/sja1105/sja1105_spi.c +++ b/drivers/net/dsa/sja1105/sja1105_spi.c @@ -465,9 +465,11 @@ int sja1105_static_config_upload(struct sja1105_private *priv) dev_err(dev, "Switch reported that configuration is " "invalid, retrying...\n"); continue; + } - } while (--retries && (status.crcchkl == 1 || status.crcchkg == 1 || - status.configs == 0 || status.ids == 1)); + /* Success! */ + break; + } while (--retries); if (!retries) { rc = -EIO;