[PATCH][next] net: dsa: sja1105: fix check on while loop exit

[PATCH][next] net: dsa: sja1105: fix check on while loop exit

[Colin King]

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

[PATCH][next] net: dsa: sja1105: fix check on while loop exit

[Colin King]

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

Re: [PATCH][next] net: dsa: sja1105: fix check on while loop exit

[Vladimir Oltean]

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