netdev - Re: [PATCH v2 net 1/1] net: dsa: sja1105: Fix link speed not working at 100 Mbps and below

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190603005053.GH19081@lunn.ch>
Date:   Mon, 3 Jun 2019 02:50:53 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     f.fainelli@...il.com, vivien.didelot@...il.com,
        davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH v2 net 1/1] net: dsa: sja1105: Fix link speed not working
 at 100 Mbps and below

On Mon, Jun 03, 2019 at 02:31:37AM +0300, Vladimir Oltean wrote:
> The hardware values for link speed are held in the sja1105_speed_t enum.
> However they do not increase in the order that sja1105_get_speed_cfg was
> iterating over them (basically from SJA1105_SPEED_AUTO - 0 - to
> SJA1105_SPEED_1000MBPS - 1 - skipping the other two).
> 
> Another bug is that the code in sja1105_adjust_port_config relies on the
> fact that an invalid link speed is detected by sja1105_get_speed_cfg and
> returned as -EINVAL.  However storing this into an enum that only has
> positive members will cast it into an unsigned value, and it will miss
> the negative check.
> 
> So take the simplest approach and remove the sja1105_get_speed_cfg
> function and replace it with a simple switch-case statement.
> 
> Fixes: 8aa9ebccae87 ("net: dsa: Introduce driver for NXP SJA1105 5-port L2 switch")
> Signed-off-by: Vladimir Oltean <olteanv@...il.com>
> Suggested-by: Andrew Lunn <andrew@...n.ch>
> ---
>  drivers/net/dsa/sja1105/sja1105_main.c | 32 +++++++++++++-------------
>  1 file changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/dsa/sja1105/sja1105_main.c b/drivers/net/dsa/sja1105/sja1105_main.c
> index 5412c3551bcc..25bb64ce0432 100644
> --- a/drivers/net/dsa/sja1105/sja1105_main.c
> +++ b/drivers/net/dsa/sja1105/sja1105_main.c
> @@ -710,16 +710,6 @@ static int sja1105_speed[] = {
>  	[SJA1105_SPEED_1000MBPS] = 1000,
>  };
>  
> -static sja1105_speed_t sja1105_get_speed_cfg(unsigned int speed_mbps)
> -{
> -	int i;
> -
> -	for (i = SJA1105_SPEED_AUTO; i <= SJA1105_SPEED_1000MBPS; i++)
> -		if (sja1105_speed[i] == speed_mbps)
> -			return i;
> -	return -EINVAL;
> -}
> -
>  /* Set link speed and enable/disable traffic I/O in the MAC configuration
>   * for a specific port.
>   *
> @@ -742,8 +732,21 @@ static int sja1105_adjust_port_config(struct sja1105_private *priv, int port,
>  	mii = priv->static_config.tables[BLK_IDX_XMII_PARAMS].entries;
>  	mac = priv->static_config.tables[BLK_IDX_MAC_CONFIG].entries;
>  
> -	speed = sja1105_get_speed_cfg(speed_mbps);
> -	if (speed_mbps && speed < 0) {
> +	switch (speed_mbps) {
> +	case 0:
> +		/* No speed update requested */
> +		speed = SJA1105_SPEED_AUTO;
> +		break;
> +	case 10:
> +		speed = SJA1105_SPEED_10MBPS;
> +		break;
> +	case 100:
> +		speed = SJA1105_SPEED_100MBPS;
> +		break;
> +	case 1000:
> +		speed = SJA1105_SPEED_1000MBPS;
> +		break;
> +	default:
>  		dev_err(dev, "Invalid speed %iMbps\n", speed_mbps);
>  		return -EINVAL;
>  	}

Thanks for the re-write. This looks more obviously correct. One minor
nit-pick. We have SPEED_10, SPEED_100, SPEED_1000, etc. It would be
good to use them.

With that change

Reviewed-by: Andrew Lunn <andrew@...n.ch>

    Andrew