Re: [PATCH net-next v6 2/3] net: dsa: tag_yt921x: add support for Motorcomm YT921x tags

[Andrew Lunn]

> > > +static struct sk_buff *
> > > +yt921x_tag_xmit(struct sk_buff *skb, struct net_device *netdev)
> > > +{
> > > +     struct dsa_port *dp = dsa_user_to_port(netdev);
> > > +     unsigned int port = dp->index;
> > > +     __be16 *tag;
> > > +     u16 tx;
> > > +
> > > +     skb_push(skb, YT921X_TAG_LEN);
> > > +     dsa_alloc_etype_header(skb, YT921X_TAG_LEN);
> > > +
> > > +     tag = dsa_etype_header_pos_tx(skb);
> > > +
> > > +     /* We might use yt921x_priv::tag_eth_p, but
> > > +      * 1. CPU_TAG_TPID could be configured anyway;
> > > +      * 2. Are you using the right chip?
> >
> > The tag format sort of becomes fixed ABI as soon as user space is able
> > to run "cat /sys/class/net/eth0/dsa/tagging", see "yt921x", and record
> > it to a pcap file. Unless the EtherType bears some other meaning rather
> > than being a fixed value, then if you change it later to some other
> > value than 0x9988, you'd better also change the protocol name to
> > distinguish it from "yt921x".
> >
> 
> "EtherType" here does not necessarily become EtherType; better to
> think it is a key to enable port control over the switch. It could be
> a dynamic random value as long as everyone gets the same value all
> over the kernel, see the setup process of the switch driver. Ideally
> only the remaining content of the tag should become the ABI (and is
> actually enforced by the switch), but making a dynamic "EtherType" is
> clearly a worse idea so I don't know how to clarify the fact...

If i remember correctly, the Marvell switches allow you to set the
EtherType they use. We just use the reset default value. It has been
like this since somewhere around 2008, and nobody has needed another
value.

What use case do you have for using a different value?

	Andrew