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 [off-list ref] wrote:

On 1/13/21 6:59 AM, George McCollister wrote:

quoted

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@gmail.com>
---

[snip]

This looks ready to me, just a few nits and suggestions.

quoted

+/* 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]

quoted

+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.

quoted

+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]

quoted

+
+     /* 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]

quoted

+/* 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