On Mon, 3 Jun 2019 at 03:50, Andrew Lunn <and...@lunn.ch> wrote:
>
> 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
Hi Andrew,
If I made the change you're suggesting, I would need to replace 0 with
SPEED_UNKNOWN and thus I would conflict with this net-next change:
https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=af7cd0366ee994e8b35985d407261dc0ed9dfb4d
I think it's simpler to wait until Dave merges net into net-next and
then submit this 1000 -> SPEED_1000 change as a net-next one, and let
the fix itself go into net.
Sounds ok?
Regards,
-Vladimir
