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@xxxxxxxxxxxxx> wrote:
>
> From: Colin Ian King <colin.king@xxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxx>
> ---
>
> 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@xxxxxxxxx>

-Vladimir