Re: [PATCH v2 net-next 1/2] net: dsa: tag_dsa: Unify regular and ethertype DSA taggers

["Tobias Waldekranz" <tobias@waldekranz.com>]

On Sat Nov 14, 2020 at 4:08 AM CET, Andrew Lunn wrote:
> Hi Tobias
>
> > +/**
> > + * enum dsa_cmd - DSA Command
> > + * @DSA_CMD_TO_CPU: Set on packets that were trapped or mirrored to
> > + *     the CPU port. This is needed to implement control protocols,
> > + *     e.g. STP and LLDP, that must not allow those control packets to
> > + *     be switched according to the normal rules.
>
> Maybe we want to mention that this only makes sense for packets
> egressing the switch?
>
> > + * @DSA_CMD_FROM_CPU: Used by the CPU to send a packet to a specific
> > + *     port, ignoring all the barriers that the switch normally
> > + *     enforces (VLANs, STP port states etc.). "sudo send packet"
>
> This only make sense for packets ingressing the switch. The
> TO_CPU/FROM_CPU kind of make that clear but..

Honestly yes, I think it is pretty clear. But I am happy to change it
if you have any particular formulation you would like in there.

> > + * @DSA_CMD_TO_SNIFFER: Set on packets that where mirrored to the CPU
> > + *     as a result of matching some user configured ingress or egress
> > + *     monitor criteria.
> > + * @DSA_CMD_FORWARD: Everything else, i.e. the bulk data traffic.
>
> I assume this can be used in both direction?

Yes. I can add a sentence about that.

> > + *
> > + * A 3-bit code is used to relay why a particular frame was sent to
> > + * the CPU. We only use this to determine if the packet was mirrored
> > + * or trapped, i.e. whether the packet has been forwarded by hardware
> > + * or not.
>
> Maybe add that, not all generations support all codes.

Not sure I have that information. The oldest chipset I've worked with
is Jade (6095/6185) and in that datasheet the TO_CPU tag is not even
documented. From Opal+(6097) all the way through Agate, Peridot, and
Amethyst, the definitions have not changed from what I can see?

> > +			/* Remote management frames originate from the
> > +			 * switch itself, there is no DSA port for us
> > +			 * to ingress it on (the port field is always
> > +			 * 0 in these tags).
>
> If/when we get around to implementing this, i doubt we will ingress it
> like a frame. It will get picked up here and probably added to some
> queue and a thread woken up. So maybe just say, not implemented yet,
> so drop it.

v1 actually had a sentence about this :) I can put it back.

> > +			 */
> > +			return NULL;
> > +		case DSA_CODE_ARP_MIRROR:
> > +		case DSA_CODE_POLICY_MIRROR:
> > +			/* Mark mirrored packets to notify any upper
> > +			 * device (like a bridge) that forwarding has
> > +			 * already been done by hardware.
> > +			 */
> > +			skb->offload_fwd_mark = 1;
> > +			break;
> > +		case DSA_CODE_MGMT_TRAP:
> > +		case DSA_CODE_IGMP_MLD_TRAP:
> > +		case DSA_CODE_POLICY_TRAP:
> > +			/* Traps have, by definition, not been
> > +			 * forwarded by hardware, so don't mark them.
> > +			 */
>
> Humm, yes, they have not been forwarded by hardware. But is the
> software bridge going to do the right thing and not flood them? Up

The bridge is free to flood them if it wants to (e.g. IGMP/MLD
snooping is off) or not (e.g. IGMP/MLD snooping enabled). The point
is, that is not for a lowly switchdev driver to decide. Our job is to
relay to the bridge if this skb has been forwarded or not, the end.

> until now, i think we did mark them. So this is a clear change in
> behaviour. I wonder if we want to break this out into a separate
> patch? If something breaks, we can then bisect was it the combining
> which broke it, or the change of this mark.

Since mv88e6xxx can not configure anything that generates
DSA_CODE_MGMT_TRAP or DSA_CODE_POLICY_TRAP yet, we do not have to
worry about any change in behavior there.

That leaves us with DSA_CODE_IGMP_MLD_TRAP. Here is the problem:

Currenly, tag_dsa.c will set skb->offload_fwd_mark for IGMP/MLD
packets, whereas tag_edsa.c will exempt them. So we can not unify the
two without changing the behavior of one.

I posit that tag_edsa does the right thing, the packet has not been
forwarded, so we should go with that.

This is precisely the reason why we want to unify these! :)

> I will try to test this on my hardware, but it is probably same as
> your 6390X and 6352.

Thank you!