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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CA+h21hoaOLassrEjHGoOortesCBkTUamCw8Efoc=vobwgwH25w@mail.gmail.com>
Date:   Mon, 3 Jun 2019 16:04:18 +0300
From:   Vladimir Oltean <olteanv@...il.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        netdev <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, 3 Jun 2019 at 03:50, Andrew Lunn <andrew@...n.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 <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

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