netdev - Re: [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver

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:   Thu, 14 Jan 2021 12:35:48 -0600
From:   George McCollister <george.mccollister@...il.com>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        Vladimir Oltean <olteanv@...il.com>,
        Rob Herring <robh@...nel.org>,
        "David S . Miller" <davem@...emloft.net>, netdev@...r.kernel.org,
        "open list:OPEN FIRMWARE AND..." <devicetree@...r.kernel.org>
Subject: Re: [PATCH net-next v4 2/3] net: dsa: add Arrow SpeedChips XRS700x driver

On Thu, Jan 14, 2021 at 11:28 AM Florian Fainelli <f.fainelli@...il.com> wrote:
>
> On 1/13/21 6:59 AM, George McCollister wrote:
> > Add a driver with initial support for the Arrow SpeedChips XRS7000
> > series of gigabit Ethernet switch chips which are typically used in
> > critical networking applications.
> >
> > The switches have up to three RGMII ports and one RMII port.
> > Management to the switches can be performed over i2c or mdio.
> >
> > Support for advanced features such as PTP and
> > HSR/PRP (IEC 62439-3 Clause 5 & 4) is not included in this patch and
> > may be added at a later date.
> >
> > Signed-off-by: George McCollister <george.mccollister@...il.com>
> > ---
> [snip]
>
> This looks ready to me, just a few nits and suggestions.
>
>
> > +/* Add an inbound policy filter which matches the BPDU destination MAC
> > + * and forwards to the CPU port. Leave the policy disabled, it will be
> > + * enabled as needed.
> > + */
> > +static int xrs700x_port_add_bpdu_ipf(struct dsa_switch *ds, int port)
> > +{
> > +     struct xrs700x *priv = ds->priv;
> > +     unsigned int val = 0;
> > +     int i = 0;
> > +     int ret;
> > +
> > +     /* Compare all 48 bits of the destination MAC address. */
> > +     ret = regmap_write(priv->regmap, XRS_ETH_ADDR_CFG(port, 0), 48 << 2);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* match BPDU destination 01:80:c2:00:00:00 */
> > +     ret = regmap_write(priv->regmap, XRS_ETH_ADDR_0(port, 0), 0x8001);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_write(priv->regmap, XRS_ETH_ADDR_1(port, 0), 0xc2);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = regmap_write(priv->regmap, XRS_ETH_ADDR_2(port, 0), 0x0);
> > +     if (ret)
> > +             return ret;
>
> Not that this is likely to change, but you could write this as a for
> loop and use eth_stp_addr from include/linux/etherdevice.h.

Okay, changed to a loop using eth_stp_addr. I'll retest STP to make
sure I didn't break it (this would be my luck towards the end of a
patch series).

>
> [snip]
>
> > +static int xrs700x_port_setup(struct dsa_switch *ds, int port)
> > +{
> > +     bool cpu_port = dsa_is_cpu_port(ds, port);
> > +     struct xrs700x *priv = ds->priv;
> > +     unsigned int val = 0;
> > +     int ret, i;
> > +
> > +     xrs700x_port_stp_state_set(ds, port, BR_STATE_DISABLED);
>
> It looks like we should be standardizing at some point on having switch
> drivers do just the global configuration in the ->setup() callback and
> have the core call the ->port_disable() for each port except the CPU/DSA
> ports, and finally let the actual port configuration bet done in
> ->port_enable(). What you have is fine for now and easy to change in the
> future.

Sounds good.

>
> > +int xrs700x_switch_register(struct xrs700x *dev)
> > +{
> > +     int ret;
> > +     int i;
> > +
> > +     ret = xrs700x_detect(dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret = xrs700x_setup_regmap_range(dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     dev->ports = devm_kzalloc(dev->dev,
> > +                               sizeof(*dev->ports) * dev->ds->num_ports,
> > +                               GFP_KERNEL);
> > +     if (!dev->ports)
> > +             return -ENOMEM;
> > +
> > +     for (i = 0; i < dev->ds->num_ports; i++) {
> > +             ret = xrs700x_alloc_port_mib(dev, i);
> > +             if (ret)
> > +                     return ret;
>
> Nothing frees up the successfully allocated p->mib_data[] in case of
> errors so you would be leaking here.

In case of an error probe will end up returning an error and the
memory will be free'd since it was allocated with a devm_ function,
won't it?

>
> [snip]
>
> > +
> > +     /* Main DSA driver may not be started yet. */
> > +     if (ret)
> > +             return ret;
> > +
> > +     i2c_set_clientdata(i2c, dev);
>
> I would be tempted to move this before "publishing" the device, probably
> does not harm though, likewise for the MDIO stub.

Okay, I moved it.

>
> [snip]
>
> > +/* Switch Configuration Registers - VLAN */
> > +#define XRS_VLAN(v)                  (XRS_SWITCH_VLAN_BASE + 0x2 * (v))
> > +
> > +#define MAX_VLAN                     4095
>
> Can you use VLAN_N_VID - 1 here from include/linux/if_vlan.h?

Sure.

> --
> Florian