Re: [PATCH net-next 2/8] net: dsa: tag_ksz: add tag handling for Microchip LAN937x

[Vladimir Oltean]

On Thu, Jan 28, 2021 at 12:11:06PM +0530, Prasanna Vengateshan wrote:
> diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
> index 4820dbcedfa2..6fac39c2b7d5 100644
> --- a/net/dsa/tag_ksz.c
> +++ b/net/dsa/tag_ksz.c
> @@ -190,10 +190,84 @@ 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 Ingress (Host -> LAN937x), 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 Egress (LAN937x -> Host), 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)
> + */

You messed up the comment, right now it's as good as not having it.
The one-hot port encoding is for xmit. The zero-based encoding is for
rcv, not the other way around.

> +#define LAN937X_INGRESS_TAG_LEN		2
> +
> +#define LAN937X_TAIL_TAG_OVERRIDE	BIT(11)
> +#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 encoding */

Do we really need this comment and the one with "Tag decoding" from rcv?

> +	tag = skb_put(skb, LAN937X_INGRESS_TAG_LEN);
> +	addr = skb_mac_header(skb);
> +
> +	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 struct sk_buff *lan937x_rcv(struct sk_buff *skb, struct net_device *dev,
> +				   struct packet_type *pt)

You can reuse ksz9477_rcv.

> +{
> +	/* Tag decoding */
> +	u8 *tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
> +	unsigned int port = tag[0] & LAN937X_TAIL_TAG_PORT_MASK;
> +	unsigned int len = KSZ_EGRESS_TAG_LEN;
> +
> +	/* Extra 4-bytes PTP timestamp */
> +	if (tag[0] & KSZ9477_PTP_TAG_INDICATION)
> +		len += KSZ9477_PTP_TAG_LEN;
> +
> +	return ksz_common_rcv(skb, dev, port, len);
> +}
> +
> +static const struct dsa_device_ops lan937x_netdev_ops = {
> +	.name	= "lan937x",
> +	.proto	= DSA_TAG_PROTO_LAN937X,
> +	.xmit	= lan937x_xmit,
> +	.rcv	= lan937x_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.25.1
>