From: Andrew Lunn <andrew@lunn•ch>
To: Vladimir Oltean <olteanv@gmail•com>
Cc: f.fainelli@gmail•com, vivien.didelot@gmail•com,
davem@davemloft•net, netdev@vger•kernel.org
Subject: Re: [PATCH v2 net 1/1] net: dsa: sja1105: Fix link speed not working at 100 Mbps and below
Date: Mon, 3 Jun 2019 02:50:53 +0200 [thread overview]
Message-ID: <20190603005053.GH19081@lunn.ch> (raw)
In-Reply-To: <20190602233137.17930-2-olteanv@gmail.com>
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@gmail•com>
> Suggested-by: Andrew Lunn <andrew@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 <andrew@lunn•ch>
Andrew
next prev parent reply other threads:[~2019-06-03 0:51 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-02 23:31 [PATCH v2 net 0/1] Fix link speed handling for SJA1105 DSA driver Vladimir Oltean
2019-06-02 23:31 ` [PATCH v2 net 1/1] net: dsa: sja1105: Fix link speed not working at 100 Mbps and below Vladimir Oltean
2019-06-03 0:50 ` Andrew Lunn [this message]
2019-06-03 13:04 ` Vladimir Oltean
2019-06-03 13:06 ` Andrew Lunn
2019-06-04 18:52 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190603005053.GH19081@lunn.ch \
--to=andrew@lunn$(echo .)ch \
--cc=davem@davemloft$(echo .)net \
--cc=f.fainelli@gmail$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=olteanv@gmail$(echo .)com \
--cc=vivien.didelot@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox