Re: [PATCH net-next v2 1/2] net: dsa: tag_rtl8_4: add rtl8_4t trailing variant

["Alvin Šipraga" <ALSI@bang-olufsen.dk>]

Luiz Angelo Daros de Luca <luizluca@gmail.com> writes:

> The switch supports the same tag both before ethertype or before CRC.

s/The switch supports/Realtek switches support/?

I think you should update the documentation at the top of the file as
well.

>
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>  include/net/dsa.h    |   2 +
>  net/dsa/tag_rtl8_4.c | 127 +++++++++++++++++++++++++++++++++----------
>  2 files changed, 99 insertions(+), 30 deletions(-)
>
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index fd1f62a6e0a8..b688ced04b0e 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -52,6 +52,7 @@ struct phylink_link_state;
>  #define DSA_TAG_PROTO_BRCM_LEGACY_VALUE		22
>  #define DSA_TAG_PROTO_SJA1110_VALUE		23
>  #define DSA_TAG_PROTO_RTL8_4_VALUE		24
> +#define DSA_TAG_PROTO_RTL8_4T_VALUE		25
>  
>  enum dsa_tag_protocol {
>  	DSA_TAG_PROTO_NONE		= DSA_TAG_PROTO_NONE_VALUE,
> @@ -79,6 +80,7 @@ enum dsa_tag_protocol {
>  	DSA_TAG_PROTO_SEVILLE		= DSA_TAG_PROTO_SEVILLE_VALUE,
>  	DSA_TAG_PROTO_SJA1110		= DSA_TAG_PROTO_SJA1110_VALUE,
>  	DSA_TAG_PROTO_RTL8_4		= DSA_TAG_PROTO_RTL8_4_VALUE,
> +	DSA_TAG_PROTO_RTL8_4T		= DSA_TAG_PROTO_RTL8_4T_VALUE,
>  };
>  
>  struct dsa_switch;
> diff --git a/net/dsa/tag_rtl8_4.c b/net/dsa/tag_rtl8_4.c
> index 02686ad4045d..d80357cb74b0 100644
> --- a/net/dsa/tag_rtl8_4.c
> +++ b/net/dsa/tag_rtl8_4.c
> @@ -84,87 +84,133 @@
>  #define RTL8_4_TX			GENMASK(3, 0)
>  #define RTL8_4_RX			GENMASK(10, 0)
>  
> -static struct sk_buff *rtl8_4_tag_xmit(struct sk_buff *skb,
> -				       struct net_device *dev)
> +static void rtl8_4_write_tag(struct sk_buff *skb, struct net_device *dev,
> +			     void *tag)
>  {
>  	struct dsa_port *dp = dsa_slave_to_port(dev);
> -	__be16 *tag;
> -
> -	skb_push(skb, RTL8_4_TAG_LEN);
> -
> -	dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN);
> -	tag = dsa_etype_header_pos_tx(skb);
> +	__be16 tag16[RTL8_4_TAG_LEN / 2];
>  
>  	/* Set Realtek EtherType */
> -	tag[0] = htons(ETH_P_REALTEK);
> +	tag16[0] = htons(ETH_P_REALTEK);
>  
>  	/* Set Protocol; zero REASON */
> -	tag[1] = htons(FIELD_PREP(RTL8_4_PROTOCOL, RTL8_4_PROTOCOL_RTL8365MB));
> +	tag16[1] = htons(FIELD_PREP(RTL8_4_PROTOCOL, RTL8_4_PROTOCOL_RTL8365MB));
>  
>  	/* Zero FID_EN, FID, PRI_EN, PRI, KEEP; set LEARN_DIS */
> -	tag[2] = htons(FIELD_PREP(RTL8_4_LEARN_DIS, 1));
> +	tag16[2] = htons(FIELD_PREP(RTL8_4_LEARN_DIS, 1));
>  
>  	/* Zero ALLOW; set RX (CPU->switch) forwarding port mask */
> -	tag[3] = htons(FIELD_PREP(RTL8_4_RX, BIT(dp->index)));
> +	tag16[3] = htons(FIELD_PREP(RTL8_4_RX, BIT(dp->index)));
> +
> +	memcpy(tag, tag16, RTL8_4_TAG_LEN);

Why not just cast tag to __be16 and avoid an extra memcpy for each xmit?

> +}
> +
> +static struct sk_buff *rtl8_4_tag_xmit(struct sk_buff *skb,
> +				       struct net_device *dev)
> +{
> +	skb_push(skb, RTL8_4_TAG_LEN);
> +
> +	dsa_alloc_etype_header(skb, RTL8_4_TAG_LEN);
> +
> +	rtl8_4_write_tag(skb, dev, dsa_etype_header_pos_tx(skb));
>  
>  	return skb;
>  }
>  
> -static struct sk_buff *rtl8_4_tag_rcv(struct sk_buff *skb,
> -				      struct net_device *dev)
> +static struct sk_buff *rtl8_4t_tag_xmit(struct sk_buff *skb,
> +					struct net_device *dev)
> +{
> +	/* Calculate the checksum here if not done yet as trailing tags will
> +	 * break either software and hardware based checksum
> +	 */
> +	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
> +		return NULL;
> +
> +	rtl8_4_write_tag(skb, dev, skb_put(skb, RTL8_4_TAG_LEN));
> +
> +	return skb;
> +}
> +
> +static int rtl8_4_read_tag(struct sk_buff *skb, struct net_device *dev,
> +			   void *tag)
>  {
> -	__be16 *tag;
>  	u16 etype;
>  	u8 reason;
>  	u8 proto;
>  	u8 port;
> +	__be16 tag16[RTL8_4_TAG_LEN / 2];

nit: Reverse christmas-tree order?

>  
> -	if (unlikely(!pskb_may_pull(skb, RTL8_4_TAG_LEN)))
> -		return NULL;
> -
> -	tag = dsa_etype_header_pos_rx(skb);
> +	memcpy(tag16, tag, RTL8_4_TAG_LEN);

Likewise can you avoid this memcpy?

>  
>  	/* Parse Realtek EtherType */
> -	etype = ntohs(tag[0]);
> +	etype = ntohs(tag16[0]);
>  	if (unlikely(etype != ETH_P_REALTEK)) {
>  		dev_warn_ratelimited(&dev->dev,
>  				     "non-realtek ethertype 0x%04x\n", etype);
> -		return NULL;
> +		return -EPROTO;
>  	}
>  
>  	/* Parse Protocol */
> -	proto = FIELD_GET(RTL8_4_PROTOCOL, ntohs(tag[1]));
> +	proto = FIELD_GET(RTL8_4_PROTOCOL, ntohs(tag16[1]));
>  	if (unlikely(proto != RTL8_4_PROTOCOL_RTL8365MB)) {
>  		dev_warn_ratelimited(&dev->dev,
>  				     "unknown realtek protocol 0x%02x\n",
>  				     proto);
> -		return NULL;
> +		return -EPROTO;
>  	}
>  
>  	/* Parse REASON */
> -	reason = FIELD_GET(RTL8_4_REASON, ntohs(tag[1]));
> +	reason = FIELD_GET(RTL8_4_REASON, ntohs(tag16[1]));
>  
>  	/* Parse TX (switch->CPU) */
> -	port = FIELD_GET(RTL8_4_TX, ntohs(tag[3]));
> +	port = FIELD_GET(RTL8_4_TX, ntohs(tag16[3]));
>  	skb->dev = dsa_master_find_slave(dev, 0, port);
>  	if (!skb->dev) {
>  		dev_warn_ratelimited(&dev->dev,
>  				     "could not find slave for port %d\n",
>  				     port);
> -		return NULL;
> +		return -ENOENT;
>  	}
>  
> +	if (reason != RTL8_4_REASON_TRAP)
> +		dsa_default_offload_fwd_mark(skb);
> +
> +	return 0;
> +}
> +
> +static struct sk_buff *rtl8_4_tag_rcv(struct sk_buff *skb,
> +				      struct net_device *dev)
> +{
> +	if (unlikely(!pskb_may_pull(skb, RTL8_4_TAG_LEN)))
> +		return NULL;
> +
> +	if (unlikely(rtl8_4_read_tag(skb, dev, dsa_etype_header_pos_rx(skb))))
> +		return NULL;
> +
>  	/* Remove tag and recalculate checksum */
>  	skb_pull_rcsum(skb, RTL8_4_TAG_LEN);
>  
>  	dsa_strip_etype_header(skb, RTL8_4_TAG_LEN);
>  
> -	if (reason != RTL8_4_REASON_TRAP)
> -		dsa_default_offload_fwd_mark(skb);
> +	return skb;
> +}
> +
> +static struct sk_buff *rtl8_4t_tag_rcv(struct sk_buff *skb,
> +				       struct net_device *dev)
> +{

I wonder if it's necessary to check pskb_may_pull() here too.

> +	if (skb_linearize(skb))
> +		return NULL;
> +
> +	if (unlikely(rtl8_4_read_tag(skb, dev, skb_tail_pointer(skb) - RTL8_4_TAG_LEN)))
> +		return NULL;
> +
> +	if (pskb_trim_rcsum(skb, skb->len - RTL8_4_TAG_LEN))
> +		return NULL;
>  
>  	return skb;
>  }
>  
> +/* Ethertype version */
>  static const struct dsa_device_ops rtl8_4_netdev_ops = {
>  	.name = "rtl8_4",
>  	.proto = DSA_TAG_PROTO_RTL8_4,
> @@ -172,7 +218,28 @@ static const struct dsa_device_ops rtl8_4_netdev_ops = {
>  	.rcv = rtl8_4_tag_rcv,
>  	.needed_headroom = RTL8_4_TAG_LEN,
>  };
> -module_dsa_tag_driver(rtl8_4_netdev_ops);
>  
> -MODULE_LICENSE("GPL");
> +DSA_TAG_DRIVER(rtl8_4_netdev_ops);
> +
>  MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_RTL8_4);
> +
> +/* Tail version */
> +static const struct dsa_device_ops rtl8_4t_netdev_ops = {
> +	.name = "rtl8_4t",
> +	.proto = DSA_TAG_PROTO_RTL8_4T,
> +	.xmit = rtl8_4t_tag_xmit,
> +	.rcv = rtl8_4t_tag_rcv,
> +	.needed_tailroom = RTL8_4_TAG_LEN,
> +};
> +
> +DSA_TAG_DRIVER(rtl8_4t_netdev_ops);
> +
> +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_RTL8_4L);
> +
> +static struct dsa_tag_driver *dsa_tag_drivers[] = {
> +	&DSA_TAG_DRIVER_NAME(rtl8_4_netdev_ops),
> +	&DSA_TAG_DRIVER_NAME(rtl8_4t_netdev_ops),
> +};
> +module_dsa_tag_drivers(dsa_tag_drivers);
> +
> +MODULE_LICENSE("GPL");