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 <olte...@gmail.com>
> Suggested-by: Andrew Lunn <and...@lunn.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 <and...@lunn.ch>
Andrew
