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!
