netdev - Re: [PATCH net-next] net: dsa: sja1105: Add support for the SGMII port

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] [day] [month] [year] [list]

Message-ID: <CA+h21howFajxEWhmRDDcZhvrA6Rr10pX8MJUkE9f0CAeOVOeSA@mail.gmail.com>
Date:   Fri, 20 Mar 2020 02:38:30 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     "David S. Miller" <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>, Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Vivien Didelot <vivien.didelot@...il.com>
Subject: Re: [PATCH net-next] net: dsa: sja1105: Add support for the SGMII port

On Fri, 20 Mar 2020 at 02:06, Russell King - ARM Linux admin
<linux@...linux.org.uk> wrote:
>
> On Fri, Mar 20, 2020 at 01:53:13AM +0200, Vladimir Oltean wrote:
> > @@ -774,10 +881,14 @@ static void sja1105_mac_config(struct dsa_switch *ds, int port,
> >               return;
> >       }
> >
> > -     if (link_an_mode == MLO_AN_INBAND) {
> > +     if (link_an_mode == MLO_AN_INBAND && !is_sgmii) {
> >               dev_err(ds->dev, "In-band AN not supported!\n");
> >               return;
> >       }
> > +
> > +     if (is_sgmii)
> > +             sja1105_sgmii_config(priv, port, link_an_mode == MLO_AN_INBAND,
> > +                                  state->speed);
>
> Please avoid new usages of state->speed in mac_config() - I'm trying
> to eliminate them now that the mac_link_up() patches are in.  If you
> need to set the PCS for the link speed, please hook that into
> mac_link_up() instead.
>

Well, duh. I forward-ported this from a 5.4 kernel and I simply forgot
to do this extra change. I'll still have to turn it around when I
backport it again, but whatever.

> >  }
> >
> >  static void sja1105_mac_link_down(struct dsa_switch *ds, int port,
> > @@ -833,7 +944,8 @@ static void sja1105_phylink_validate(struct dsa_switch *ds, int port,
> >       phylink_set(mask, 10baseT_Full);
> >       phylink_set(mask, 100baseT_Full);
> >       phylink_set(mask, 100baseT1_Full);
> > -     if (mii->xmii_mode[port] == XMII_MODE_RGMII)
> > +     if (mii->xmii_mode[port] == XMII_MODE_RGMII ||
> > +         mii->xmii_mode[port] == XMII_MODE_SGMII)
> >               phylink_set(mask, 1000baseT_Full);
> >
> >       bitmap_and(supported, supported, mask, __ETHTOOL_LINK_MODE_MASK_NBITS);
> > @@ -841,6 +953,82 @@ static void sja1105_phylink_validate(struct dsa_switch *ds, int port,
> >                  __ETHTOOL_LINK_MODE_MASK_NBITS);
> >  }
> >
> > +static int sja1105_mac_pcs_get_state(struct dsa_switch *ds, int port,
> > +                                  struct phylink_link_state *state)
> > +{
> > +     struct sja1105_private *priv = ds->priv;
> > +     int bmcr;
> > +
> > +     bmcr = sja1105_sgmii_read(priv, MII_BMCR);
> > +     if (bmcr < 0)
> > +             return bmcr;
> > +
> > +     state->an_enabled = !!(bmcr & BMCR_ANENABLE);
> > +
> > +     if (state->an_enabled) {
>
> mac_pcs_get_state() is only called when in in-band AN mode, so this
> is not useful.  If it's called with AN disabled, that's a bug.
>

Correct. In fact I only 'tested' in-band AN via register dumps. On my
board it is physically impossible to really make use of
mac_pcs_get_state because port 4 is a PHY-less interface (the CPU
port), and if there were any in-band AN to take place at all, this
switch port would have to be the AN master, and there isn't logic for
that at the moment.
Because there isn't more than 1 SGMII port in this 5-port switch, I
suspect that AN disabled is going to be the case for pretty much every
other board out there.
Since I did spend the time to figure out how it should work, I thought
it might have been useful to have some code, even if just
blind-tested, for whomever needed it. But after your comment, I'm not
willing to figure out now how to split the PCS config function to be
called from the AN as well as non-AN code paths. So I'll just drop any
sort of AN code in v2.

> --
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Thanks,
-Vladimir