On 23/03/2018 at 14:41:25 -0700, Florian Fainelli wrote:

On 03/23/2018 01:11 PM, Alexandre Belloni wrote:

quoted

Add a driver for Microsemi Ocelot Ethernet switch support.

This makes two modules:
mscc_ocelot_common handles all the common features that doesn't depend on
how the switch is integrated in the SoC. Currently, it handles offloading
bridging to the hardware. ocelot_io.c handles register accesses. This is
unfortunately needed because the register layout is packed and then depends
on the number of ports available on the switch. The register definition
files are automatically generated.

ocelot_board handles the switch integration on the SoC and on the board.

Frame injection and extraction to/from the CPU port is currently done using
register accesses which is quite slow. DMA is possible but the port is not
able to absorb the whole switch bandwidth.

Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

Random drive by comments because this is quite a number of lines to review!

Overall, looks quite good for a first version. Out of curiosity, is
there a particular switch test you ran this driver against? LNST?

We have a really small custom test suite.

quoted

+	/* Add dummy CRC */
+	ocelot_write_rix(ocelot, 0, QS_INJ_WR, grp);
+	skb_tx_timestamp(skb);
+
+	dev->stats.tx_packets++;
+	dev->stats.tx_bytes += skb->len;
+	dev_kfree_skb_any(skb);

No interrupt to indicate transmit completion?

No, unfortunately, the TX interrupts only indicates there is room to
start injecting frames, not that they have been transmitted.

quoted

+static int ocelot_fdb_add(struct ndmsg *ndm, struct nlattr *tb[],
+			  struct net_device *dev, const unsigned char *addr,
+			  u16 vid, u16 flags)
+{
+	struct ocelot_port *port = netdev_priv(dev);
+	struct ocelot *ocelot = port->ocelot;
+
+	if (!vid) {
+		if (!port->vlan_aware)
+			/* If the bridge is not VLAN aware and no VID was
+			 * provided, set it to 1 as bridges have a default VID
+			 * of 1. Otherwise the MAC entry wouldn't match incoming
+			 * packets as the VID would differ (0 != 1).
+			 */
+			vid = 1;
+		else
+			/* If the bridge is VLAN aware a VID must be provided as
+			 * otherwise the learnt entry wouldn't match any frame.
+			 */
+			return -EINVAL;
+	}

So if we are targeting vid = 0 we end-up with vid = 1 possibly?

I've removed that part that is not needed for now and will rework when
sending VLAN support.

quoted

+	ocelot_write_gix(ocelot, port_cfg, ANA_PORT_PORT_CFG,
+			 ocelot_port->chip_port);
+
+	/* Apply FWD mask. The loop is needed to add/remove the current port as
+	 * a source for the other ports.
+	 */
+	for (port = 0; port < ocelot->num_phys_ports; port++) {
+		if (ocelot->bridge_fwd_mask & BIT(port)) {
+			unsigned long mask = ocelot->bridge_fwd_mask & ~BIT(port);
+
+			for (i = 0; i < ocelot->num_phys_ports; i++) {
+				unsigned long bond_mask = ocelot->lags[i];
+
+				if (!bond_mask)
+					continue;
+
+				if (bond_mask & BIT(port)) {
+					mask &= ~bond_mask;
+					break;
+				}
+			}
+
+			ocelot_write_rix(ocelot,
+					 BIT(ocelot->num_phys_ports) | mask,
+					 ANA_PGID_PGID, PGID_SRC + port);
+		} else {
+			/* Only the CPU port, this is compatible with link
+			 * aggregation.
+			 */
+			ocelot_write_rix(ocelot,
+					 BIT(ocelot->num_phys_ports),
+					 ANA_PGID_PGID, PGID_SRC + port);
+		}

All of this sounds like it should be moved into the br_join/leave, this
does not appear to be the right place to do that.

No, I've triple checked because this is a comment that both Andrew and
you had. Once a port is added to the PGID MASK, it will start forwarding
frames so we really want that to happen only when the port is in
BR_STATE_FORWARDING state. Else, we may forward frames between the
addition of the port to the bridge and setting the port to the
BR_STATE_BLOCKING state.


-- 
Alexandre Belloni, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com