[PATCH 5/6] net: add MV88E61xx DSA driver

[Vladimir Oltean]

On Fri, Apr 01, 2022 at 01:24:48PM -0700, Tim Harvey wrote:
> > > > Why is mv88e61xx_dsa_xmit() no-op?
> > >
> > > For DSA dsa-uclass calls the switch master eth device send function
> > > after calling the dsa_ops->xmit function so that a dsa driver can add
> > > any header/footer if needed. The function is required but in my case I
> > > don't care about header/footer tagging or vlan as only 1 port is
> > > active at a time in U-Boot so I just return success.
> >
> > So if I make one port active, the other are completely disabled? They
> > won't even switch? Is that how DSA uclass is supposed to work in U-Boot?
> >
> > I would think that it should be somehow configurable instead.
> 
> Marek,
> 
> I'll let Vladimir correct me if I'm wrong but my understanding is DSA
> in U-Boot is not intended to allow switches to forward packets on
> their own from port to port but instead just for forwarding packets
> between the active port and the MAC connected to the CPU (at least
> that's what I intended when I wrote the ksz9477 dsa driver
> previously).
> 
> In my opinion what a DSA driver provides is avoidance of putting
> switches in forwarding mode having the potential of easily creating
> bridge loops. With the existing mv88e61xx driver I've had users create
> bridge loops often.

DM_DSA follows the guiding principle that the user doesn't care about
the switch more than being able to transfer a boot image through TFTP
without causing switching loops in the network. The way this is achieved
is by following the "port extender" model which is exactly the way in
which Linux DSA was first introduced, prior to having support for L2
bridging offloads via switchdev. For U-Boot, packet forwarding across
front ports is effectively a non-goal. In fact, what we want to avoid is
the proliferation of bloatware such as cmd/ethsw.c, which can and will
be abused to preconfigure a switch from the bootloader so you won't have
to manage it from Linux.

Tim, I accept that U-Boot's Ethernet usage model (a single device active
at a time, with RX in synchronous poll mode) simplifies DM_DSA to the
point that creating/parsing DSA tags (which serve the purpose of
multiplexing/demultiplexing packets to/from switch ports) on the CPU is
more or less redundant when said multiplexing can be achieved on a time
basis by disabling all the ports except the required source/destination
switch port and the CPU port. This holds true even when DM_DSA gains
support for multi-switch setups.

What I would like, however, is to avoid driver code putting pressure on
the DSA uclass code. I don't really like the tracking of the priv->active_port
as being the port on which ->port_enable was last called. The only reason
this is done is to appease the "port_index != port_pdata->index" check
from dsa_port_recv(). In turn, this is because the ->xmit() and ->rcv()
DSA ops are not optional. Let's make them optional instead, instead of
adding code to a C file that indirectly depends on the code structure
from another C file.

But ->port_enable() and ->port_disable() are optional too, and that's
not okay, because if the driver doesn't provide a way to enable/disable
ports, packets can flow anywhere.

I suggest making it clear to driver writers that it has to be one or the
other by sanitizing the ops:

diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
index 9ff55a02fb23..ea860fa715bb 100644
--- a/net/dsa-uclass.c
+++ b/net/dsa-uclass.c
@@ -342,6 +342,19 @@ U_BOOT_DRIVER(dsa_port) = {
 	.plat_auto = sizeof(struct eth_pdata),
 };
 
+static int dsa_sanitize_ops(struct udevice *dev)
+{
+	struct dsa_ops *ops = dsa_get_ops(dev);
+
+	if ((!ops->xmit || !ops->rcv) &&
+	    (!ops->port_enable && !ops->port_disable)) {
+		dev_err(dev, "Packets cannot be steered to ports\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 /*
  * This function mostly deals with pulling information out of the device tree
  * into the pdata structure.
@@ -358,6 +371,10 @@ static int dsa_post_bind(struct udevice *dev)
 	if (!ofnode_valid(node))
 		return -ENODEV;
 
+	err = dsa_sanitize_ops(dev);
+	if (err)
+		return err;
+
 	pdata->master_node = ofnode_null();
 
 	node = ofnode_find_subnode(node, "ports");


Then we can properly make rcv() and xmit() optional at the level of the
DSA uclass. This way we avoid the tail chasing approach of pretending
that "return priv->active_port" _is_ the way to decode a packet for a
certain driver. It isn't, and it should just be DSA's fallback if no
such method is known. I'd like to preserve the option to implement a
rcv() and xmit() procedure, because it offers one extra layer of safety
that the CPU is really talking to the switch port it thinks it's
talking, plus inter-switch traffic is reduced in cross-chip topologies
(which are not supported now, but maybe they will be).

I think that fallback could look like this:

diff --git a/net/dsa-uclass.c b/net/dsa-uclass.c
index ea860fa715bb..64654ec1ad72 100644
--- a/net/dsa-uclass.c
+++ b/net/dsa-uclass.c
@@ -131,16 +131,15 @@ static void dsa_port_stop(struct udevice *pdev)
  * We copy the frame to a stack buffer where we have reserved headroom and
  * tailroom space.  Headroom and tailroom are set to 0.
  */
-static int dsa_port_send(struct udevice *pdev, void *packet, int length)
+static int dsa_port_mangle_packet(struct udevice *pdev, void *packet,
+				  int length)
 {
+	struct dsa_port_pdata *port_pdata = dev_get_parent_plat(pdev);
 	struct udevice *dev = dev_get_parent(pdev);
 	struct dsa_priv *priv = dev_get_uclass_priv(dev);
 	int head = priv->headroom, tail = priv->tailroom;
-	struct udevice *master = dsa_get_master(dev);
 	struct dsa_ops *ops = dsa_get_ops(dev);
 	uchar dsa_packet_tmp[PKTSIZE_ALIGN];
-	struct dsa_port_pdata *port_pdata;
-	int err;
 
 	if (length + head + tail > PKTSIZE_ALIGN)
 		return -EINVAL;
@@ -152,10 +151,21 @@ static int dsa_port_send(struct udevice *pdev, void *packet, int length)
 	/* copy back to preserve original buffer alignment */
 	memcpy(packet, dsa_packet_tmp, length);
 
-	port_pdata = dev_get_parent_plat(pdev);
-	err = ops->xmit(dev, port_pdata->index, packet, length);
-	if (err)
-		return err;
+	return ops->xmit(dev, port_pdata->index, packet, length);
+}
+
+static int dsa_port_send(struct udevice *pdev, void *packet, int length)
+{
+	struct udevice *dev = dev_get_parent(pdev);
+	struct udevice *master = dsa_get_master(dev);
+	struct dsa_ops *ops = dsa_get_ops(dev);
+	int err;
+
+	if (ops->xmit) {
+		err = dsa_port_mangle_packet(pdev, packet, length);
+		if (err)
+			return err;
+	}
 
 	return eth_get_ops(master)->send(master, packet, length);
 }
@@ -172,7 +182,7 @@ static int dsa_port_recv(struct udevice *pdev, int flags, uchar **packetp)
 	int length, port_index, err;
 
 	length = eth_get_ops(master)->recv(master, flags, packetp);
-	if (length <= 0)
+	if (length <= 0 || !ops->rcv)
 		return length;
 
 	/*

I think part of the reason for which Tim is actively trying to avoid DSA
tag inserting and parsing is because fec_mxc.c refuses to send packets
larger than 1500 bytes. As a side note, I do think that limitation
should be lifted and do allow for a reasonable amount of extra bytes,
because as things stand, if somebody will want to implement rcv() and
xmit() for either the mv88e6xxx or ksz9477 drivers, they'll involuntarily
break the boards where fec is a DSA master, which isn't really very fair.