Re: [PATCH v2 net-next 3/9] net: dsa: tag_ksz: add tag handling for Microchip LAN937x

[Vladimir Oltean]

On Thu, Apr 22, 2021 at 03:12:51PM +0530, Prasanna Vengateshan wrote:
> The Microchip LAN937X switches have a tagging protocol which is
> very similar to KSZ tagging. So that the implementation is added to
> tag_ksz.c and reused common APIs
> 
> Signed-off-by: Prasanna Vengateshan <prasanna.vengateshan@xxxxxxxxxxxxx>
> ---
>  include/net/dsa.h |  2 ++
>  net/dsa/Kconfig   |  4 ++--
>  net/dsa/tag_ksz.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 62 insertions(+), 2 deletions(-)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 507082959aa4..98c1ab6dc4dc 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -50,6 +50,7 @@ struct phylink_link_state;
>  #define DSA_TAG_PROTO_OCELOT_8021Q_VALUE	20
>  #define DSA_TAG_PROTO_SEVILLE_VALUE		21
>  #define DSA_TAG_PROTO_BRCM_LEGACY_VALUE		22
> +#define DSA_TAG_PROTO_LAN937X_VALUE		23
>  
>  enum dsa_tag_protocol {
>  	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
> @@ -75,6 +76,7 @@ enum dsa_tag_protocol {
>  	DSA_TAG_PROTO_XRS700X		= DSA_TAG_PROTO_XRS700X_VALUE,
>  	DSA_TAG_PROTO_OCELOT_8021Q	= DSA_TAG_PROTO_OCELOT_8021Q_VALUE,
>  	DSA_TAG_PROTO_SEVILLE		= DSA_TAG_PROTO_SEVILLE_VALUE,
> +	DSA_TAG_PROTO_LAN937X		= DSA_TAG_PROTO_LAN937X_VALUE,
>  };
>  
>  struct packet_type;
> diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig
> index cbc2bd643ab2..888eb79a85f2 100644
> --- a/net/dsa/Kconfig
> +++ b/net/dsa/Kconfig
> @@ -97,10 +97,10 @@ config NET_DSA_TAG_MTK
>  	  Mediatek switches.
>  
>  config NET_DSA_TAG_KSZ
> -	tristate "Tag driver for Microchip 8795/9477/9893 families of switches"
> +	tristate "Tag driver for Microchip 8795/937x/9477/9893 families of switches"
>  	help
>  	  Say Y if you want to enable support for tagging frames for the
> -	  Microchip 8795/9477/9893 families of switches.
> +	  Microchip 8795/937x/9477/9893 families of switches.
>  
>  config NET_DSA_TAG_RTL4_A
>  	tristate "Tag driver for Realtek 4 byte protocol A tags"
> diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> index 4820dbcedfa2..a67f21bdab8f 100644
> --- a/net/dsa/tag_ksz.c
> +++ b/net/dsa/tag_ksz.c
> @@ -190,10 +190,68 @@ static const struct dsa_device_ops ksz9893_netdev_ops = {
>  DSA_TAG_DRIVER(ksz9893_netdev_ops);
>  MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9893);
>  
> +/* For xmit, 2 bytes are added before FCS.
> + * ---------------------------------------------------------------------------
> + * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS(4bytes)
> + * ---------------------------------------------------------------------------
> + * tag0 : represents tag override, lookup and valid
> + * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2, 0x80=port8)
> + *
> + * For rcv, 1 byte is added before FCS.
> + * ---------------------------------------------------------------------------
> + * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|FCS(4bytes)
> + * ---------------------------------------------------------------------------
> + * tag0 : zero-based value represents port
> + *	  (eg, 0x00=port1, 0x02=port3, 0x07=port8)
> + */
> +#define LAN937X_INGRESS_TAG_LEN		2
> +
> +#define LAN937X_TAIL_TAG_OVERRIDE	BIT(11)

I had to look up the datasheet, to see what this is, because "override"
can mean many things, not all of them are desirable.

Port Blocking Override
When set, packets will be sent from the specified port(s) regardless, and any port
blocking (see Port Transmit Enable in Port MSTP State Register) is ignored.

Do you think you can name it LAN937X_TAIL_TAG_BLOCKING_OVERRIDE? I know
it's longer, but it's also more suggestive.

> +#define LAN937X_TAIL_TAG_LOOKUP		BIT(12)
> +#define LAN937X_TAIL_TAG_VALID		BIT(13)
> +#define LAN937X_TAIL_TAG_PORT_MASK	7
> +
> +static struct sk_buff *lan937x_xmit(struct sk_buff *skb,
> +				    struct net_device *dev)
> +{
> +	struct dsa_port *dp = dsa_slave_to_port(dev);
> +	__be16 *tag;
> +	u8 *addr;
> +	u16 val;
> +
> +	tag = skb_put(skb, LAN937X_INGRESS_TAG_LEN);

Here we go again.. why is it called INGRESS_TAG_LEN if it is used during
xmit, and only during xmit?

> +	addr = skb_mac_header(skb);

My personal choice would have been:

	const struct ethhdr *hdr = eth_hdr(skb);

	if (is_link_local_ether_addr(hdr->h_dest))

> +
> +	val = BIT(dp->index);
> +
> +	if (is_link_local_ether_addr(addr))
> +		val |= LAN937X_TAIL_TAG_OVERRIDE;
> +
> +	/* Tail tag valid bit - This bit should always be set by the CPU*/
> +	val |= LAN937X_TAIL_TAG_VALID;
> +
> +	*tag = cpu_to_be16(val);
> +
> +	return skb;
> +}
> +
> +static const struct dsa_device_ops lan937x_netdev_ops = {
> +	.name	= "lan937x",
> +	.proto	= DSA_TAG_PROTO_LAN937X,
> +	.xmit	= lan937x_xmit,
> +	.rcv	= ksz9477_rcv,
> +	.overhead = LAN937X_INGRESS_TAG_LEN,
> +	.tail_tag = true,
> +};
> +
> +DSA_TAG_DRIVER(lan937x_netdev_ops);
> +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_LAN937X);
> +
>  static struct dsa_tag_driver *dsa_tag_driver_array[] = {
>  	&DSA_TAG_DRIVER_NAME(ksz8795_netdev_ops),
>  	&DSA_TAG_DRIVER_NAME(ksz9477_netdev_ops),
>  	&DSA_TAG_DRIVER_NAME(ksz9893_netdev_ops),
> +	&DSA_TAG_DRIVER_NAME(lan937x_netdev_ops),
>  };
>  
>  module_dsa_tag_drivers(dsa_tag_driver_array);
> -- 
> 2.27.0
>