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

[Tim Harvey]

On Sat, Apr 2, 2022 at 4:17 PM Vladimir Oltean <vladimir.oltean at nxp.com> wrote:
>
> 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.

Vladimir,

I also want to avoid the proliferation of bloatware which is primarily
why I want to avoid having to muck with packets when there is only one
port active at a time which I feel is completely unnecessary.

>
> 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 can add this to my series if I submit a follow-up.

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

Very likely.

I guess I'll have to invest in tagging packets if you won't accept the
simplistic approach of not having to tag frames knowing that only one
port is active at a time.

That said, I have no idea if or when I will re-visit this. Adding a
DSA version of this driver was something on my personal wish list and
not something that was necessary by any means by my employer so I may
have to just drop it as I don't have the personal time to work through
this part of it or unravelling the mii bus mess in the fec_mxc driver.
It seems to me there is an issue with trying to create DM_MDIO drivers
in general as most dt's I've seen wouldn't support the requirements
yet configure DM_MDIO anyway (meaning if you implemented it you would
break those boards as I found).

Best Regards,

Tim