netdev - Re: [PATCH net-next v2 4/4] net: dsa: xrs700x: add HSR offloading support

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]

Date:   Mon, 8 Feb 2021 08:46:29 -0600
From:   George McCollister <george.mccollister@...il.com>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Jakub Kicinski <kuba@...nel.org>, Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Jonathan Corbet <corbet@....net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 4/4] net: dsa: xrs700x: add HSR offloading support

On Sat, Feb 6, 2021 at 5:53 PM Vladimir Oltean <olteanv@...il.com> wrote:
>
> On Thu, Feb 04, 2021 at 03:59:26PM -0600, George McCollister wrote:
> > +static int xrs700x_hsr_join(struct dsa_switch *ds, int port,
> > +                         struct net_device *hsr)
> > +{
> > +     unsigned int val = XRS_HSR_CFG_HSR_PRP;
> > +     struct dsa_port *partner = NULL, *dp;
> > +     struct xrs700x *priv = ds->priv;
> > +     struct net_device *slave;
> > +     enum hsr_version ver;
> > +     int ret;
> > +
> > +     ret = hsr_get_version(hsr, &ver);
> > +     if (ret)
> > +             return ret;
> > +
> > +     if (ver == HSR_V1)
> > +             val |= XRS_HSR_CFG_HSR;
> > +     else if (ver == PRP_V1)
> > +             val |= XRS_HSR_CFG_PRP;
> > +     else
> > +             return -EOPNOTSUPP;
> > +
> > +     dsa_hsr_foreach_port(dp, ds, hsr) {
> > +             partner = dp;
> > +     }
> > +
> > +     /* We can't enable redundancy on the switch until both
> > +      * redundant ports have signed up.
> > +      */
> > +     if (!partner)
> > +             return 0;
> > +
> > +     regmap_fields_write(priv->ps_forward, partner->index,
> > +                         XRS_PORT_DISABLED);
> > +     regmap_fields_write(priv->ps_forward, port, XRS_PORT_DISABLED);
> > +
> > +     regmap_write(priv->regmap, XRS_HSR_CFG(partner->index),
> > +                  val | XRS_HSR_CFG_LANID_A);
> > +     regmap_write(priv->regmap, XRS_HSR_CFG(port),
> > +                  val | XRS_HSR_CFG_LANID_B);
> > +
> > +     /* Clear bits for both redundant ports (HSR only) and the CPU port to
> > +      * enable forwarding.
> > +      */
> > +     val = GENMASK(ds->num_ports - 1, 0);
> > +     if (ver == HSR_V1) {
> > +             val &= ~BIT(partner->index);
> > +             val &= ~BIT(port);
> > +     }
> > +     val &= ~BIT(dsa_upstream_port(ds, port));
> > +     regmap_write(priv->regmap, XRS_PORT_FWD_MASK(partner->index), val);
> > +     regmap_write(priv->regmap, XRS_PORT_FWD_MASK(port), val);
> > +
> > +     regmap_fields_write(priv->ps_forward, partner->index,
> > +                         XRS_PORT_FORWARDING);
> > +     regmap_fields_write(priv->ps_forward, port, XRS_PORT_FORWARDING);
> > +
> > +     slave = dsa_to_port(ds, port)->slave;
> > +
> > +     slave->features |= NETIF_F_HW_HSR_TAG_INS | NETIF_F_HW_HSR_TAG_RM |
> > +                        NETIF_F_HW_HSR_FWD | NETIF_F_HW_HSR_DUP;
> > +
> > +     return 0;
> > +}
>
> Is it deliberate that only one slave HSR/PRP port will have the offload
> ethtool features set? If yes, then I find that a bit odd from a user
> point of view.

No. Good catch. This is a mistake I introduced when I added the code
for finding the partner. Originally for testing I had hacks that hard
coded the ports used and reconfigured HSR for each join.