[PATCH net-next 0/2] net: dsa: realtek: add rtl8_4t tag

[PATCH net-next 0/2] net: dsa: realtek: add rtl8_4t tag

[Luiz Angelo Daros de Luca]

This DSA tag might not be useful in production as the conduit driver
might generate a broken checksum. It is useful, though, for test
purpose.

Luiz

[PATCH net-next 1/2] net: dsa: tag_rtl8_4: add rtl8_4t tailing variant

[Luiz Angelo Daros de Luca]

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

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 include/net/dsa.h    |   2 +
 net/dsa/tag_rtl8_4.c | 119 ++++++++++++++++++++++++++++++++-----------
 2 files changed, 90 insertions(+), 31 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..a9883b0c1d51 100644
--- a/net/dsa/tag_rtl8_4.c
+++ b/net/dsa/tag_rtl8_4.c
@@ -84,87 +84,123 @@
 #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 inline void rtl8_4_write_tag(struct sk_buff *skb, struct net_device *dev,
+				    char *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 = (__be16 *)tag;
 
 	/* 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)));
+}
+
+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)
+{
+	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,
+			   char *tag)
 {
-	__be16 *tag;
 	u16 etype;
 	u8 reason;
 	u8 proto;
 	u8 port;
-
-	if (unlikely(!pskb_may_pull(skb, RTL8_4_TAG_LEN)))
-		return NULL;
-
-	tag = dsa_etype_header_pos_rx(skb);
+	__be16 *tag16 = (__be16 *)tag;
 
 	/* 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)
+{
+	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 +208,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");
-- 
2.35.1

[PATCH net-next 2/2] net: dsa: realtek: rtl8365mb: add support for rtl8_4t

[Luiz Angelo Daros de Luca]

The tailing tag is also supported by this family. The default is still
rtl8_4 but now the switch supports changing the tag to rtl8_4t.

Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
---
 drivers/net/dsa/realtek/rtl8365mb.c | 63 +++++++++++++++++++++++++++++
 1 file changed, 63 insertions(+)

diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index e0c7f64bc56f..02d1521887ae 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -853,9 +853,70 @@ static enum dsa_tag_protocol
 rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
 			   enum dsa_tag_protocol mp)
 {
+	struct realtek_priv *priv = ds->priv;
+	u32 val;
+
+	/* No way to return error */
+	regmap_read(priv->map, RTL8365MB_CPU_CTRL_REG, &val);
+
+	switch (FIELD_GET(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, val)) {
+	case RTL8365MB_CPU_FORMAT_4BYTES:
+		/* Similar to DSA_TAG_PROTO_RTL4_A but with 1-byte version
+		 * To CPU: [0x88 0x99 0x04 portnum]. Not supported yet.
+		 */
+		break;
+	case RTL8365MB_CPU_FORMAT_8BYTES:
+		switch (FIELD_GET(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, val)) {
+		case RTL8365MB_CPU_POS_BEFORE_CRC:
+			return DSA_TAG_PROTO_RTL8_4T;
+		case RTL8365MB_CPU_POS_AFTER_SA:
+		default:
+			return DSA_TAG_PROTO_RTL8_4;
+		}
+		break;
+	}
+
 	return DSA_TAG_PROTO_RTL8_4;
 }
 
+static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds, int cpu,
+					 enum dsa_tag_protocol proto)
+{
+	struct realtek_priv *priv = ds->priv;
+	int tag_position;
+	int tag_format;
+	int ret;
+
+	switch (proto) {
+	case DSA_TAG_PROTO_RTL8_4:
+		tag_format = RTL8365MB_CPU_FORMAT_8BYTES;
+		tag_position = RTL8365MB_CPU_POS_AFTER_SA;
+		break;
+	case DSA_TAG_PROTO_RTL8_4T:
+		tag_format = RTL8365MB_CPU_FORMAT_8BYTES;
+		tag_position = RTL8365MB_CPU_POS_BEFORE_CRC;
+		break;
+	/* The switch also supports a 4-byte format, similar to rtl4a but with
+	 * the same 0x04 8-bit version and probably 8-bit port source/dest.
+	 * There is no public doc about it. Not supported yet.
+	 */
+	default:
+		return -EPROTONOSUPPORT;
+	}
+
+	ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG,
+				 RTL8365MB_CPU_CTRL_TAG_POSITION_MASK |
+				 RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK,
+				 FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK,
+					    tag_position) |
+				 FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK,
+					    tag_format));
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
 				      phy_interface_t interface)
 {
@@ -2079,6 +2140,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
 
 static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
 	.get_tag_protocol = rtl8365mb_get_tag_protocol,
+	.change_tag_protocol = rtl8365mb_change_tag_protocol,
 	.setup = rtl8365mb_setup,
 	.teardown = rtl8365mb_teardown,
 	.phylink_get_caps = rtl8365mb_phylink_get_caps,
@@ -2097,6 +2159,7 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
 
 static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
 	.get_tag_protocol = rtl8365mb_get_tag_protocol,
+	.change_tag_protocol = rtl8365mb_change_tag_protocol,
 	.setup = rtl8365mb_setup,
 	.teardown = rtl8365mb_teardown,
 	.phylink_get_caps = rtl8365mb_phylink_get_caps,
-- 
2.35.1

Re: [PATCH net-next 0/2] net: dsa: realtek: add rtl8_4t tag

[Alvin Šipraga]

Hi Luiz,

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

> This DSA tag might not be useful in production as the conduit driver
> might generate a broken checksum. It is useful, though, for test
> purpose.

You make it sound like the code is never useful in production, but is
that really the case? And what kind of test case do you have in mind?

Also, you call this "tailing" which sounds quite off. Do you mean
"trailing"? I think that is the more common terminology.

In any case the series looks fine :-)

Acked-by: Alvin Šipraga <alsi@bang-olufsen.dk>

Kind regards,
Alvin

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

[Vladimir Oltean]

Re: title. Tail or trailing?

On Wed, Feb 09, 2022 at 06:13:11PM -0300, Luiz Angelo Daros de Luca wrote:
> +static inline void rtl8_4_write_tag(struct sk_buff *skb, struct net_device *dev,
> +				    char *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 = (__be16 *)tag;

Can the tail tag be aligned to an odd offset? In that case, should you
access byte by byte, maybe? I'm not sure how arches handle this.

>  
>  	/* 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)));
> +}
> +
> +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)
> +{

Why don't you want to add:

	if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
		return NULL;

and then you'll make this tagging protocol useful in production too.

> +	rtl8_4_write_tag(skb, dev, skb_put(skb, RTL8_4_TAG_LEN));
> +
> +	return skb;
> +}

Re: [PATCH net-next 2/2] net: dsa: realtek: rtl8365mb: add support for rtl8_4t

[Vladimir Oltean]

On Wed, Feb 09, 2022 at 06:13:12PM -0300, Luiz Angelo Daros de Luca wrote:
> The tailing tag is also supported by this family. The default is still
> rtl8_4 but now the switch supports changing the tag to rtl8_4t.
> 
> Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> ---
>  drivers/net/dsa/realtek/rtl8365mb.c | 63 +++++++++++++++++++++++++++++
>  1 file changed, 63 insertions(+)
> 
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index e0c7f64bc56f..02d1521887ae 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -853,9 +853,70 @@ static enum dsa_tag_protocol
>  rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
>  			   enum dsa_tag_protocol mp)
>  {
> +	struct realtek_priv *priv = ds->priv;
> +	u32 val;
> +
> +	/* No way to return error */
> +	regmap_read(priv->map, RTL8365MB_CPU_CTRL_REG, &val);
> +
> +	switch (FIELD_GET(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, val)) {
> +	case RTL8365MB_CPU_FORMAT_4BYTES:
> +		/* Similar to DSA_TAG_PROTO_RTL4_A but with 1-byte version
> +		 * To CPU: [0x88 0x99 0x04 portnum]. Not supported yet.
> +		 */
> +		break;
> +	case RTL8365MB_CPU_FORMAT_8BYTES:
> +		switch (FIELD_GET(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, val)) {
> +		case RTL8365MB_CPU_POS_BEFORE_CRC:
> +			return DSA_TAG_PROTO_RTL8_4T;
> +		case RTL8365MB_CPU_POS_AFTER_SA:
> +		default:
> +			return DSA_TAG_PROTO_RTL8_4;
> +		}
> +		break;
> +	}
> +
>  	return DSA_TAG_PROTO_RTL8_4;

Not great. dsa_get_tag_protocol gets called from dsa_port_parse_cpu,
which is earlier than rtl8365mb_cpu_config, so you may temporarily
report a tagging protocol which you don't expect (depending on what is
in hardware at the time). Can you not keep the current tagging protocol
in a variable? You could then avoid the need to return an error on
regmap_read too.

>  }
>  
> +static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds, int cpu,
> +					 enum dsa_tag_protocol proto)
> +{
> +	struct realtek_priv *priv = ds->priv;
> +	int tag_position;
> +	int tag_format;
> +	int ret;
> +
> +	switch (proto) {
> +	case DSA_TAG_PROTO_RTL8_4:
> +		tag_format = RTL8365MB_CPU_FORMAT_8BYTES;
> +		tag_position = RTL8365MB_CPU_POS_AFTER_SA;
> +		break;
> +	case DSA_TAG_PROTO_RTL8_4T:
> +		tag_format = RTL8365MB_CPU_FORMAT_8BYTES;
> +		tag_position = RTL8365MB_CPU_POS_BEFORE_CRC;
> +		break;
> +	/* The switch also supports a 4-byte format, similar to rtl4a but with
> +	 * the same 0x04 8-bit version and probably 8-bit port source/dest.
> +	 * There is no public doc about it. Not supported yet.
> +	 */
> +	default:
> +		return -EPROTONOSUPPORT;
> +	}
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG,
> +				 RTL8365MB_CPU_CTRL_TAG_POSITION_MASK |
> +				 RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK,
> +				 FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK,
> +					    tag_position) |
> +				 FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK,
> +					    tag_format));
> +	if (ret)
> +		return ret;
> +
> +	return 0;
> +}
> +
>  static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
>  				      phy_interface_t interface)
>  {
> @@ -2079,6 +2140,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  
>  static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
>  	.get_tag_protocol = rtl8365mb_get_tag_protocol,
> +	.change_tag_protocol = rtl8365mb_change_tag_protocol,
>  	.setup = rtl8365mb_setup,
>  	.teardown = rtl8365mb_teardown,
>  	.phylink_get_caps = rtl8365mb_phylink_get_caps,
> @@ -2097,6 +2159,7 @@ static const struct dsa_switch_ops rtl8365mb_switch_ops_smi = {
>  
>  static const struct dsa_switch_ops rtl8365mb_switch_ops_mdio = {
>  	.get_tag_protocol = rtl8365mb_get_tag_protocol,
> +	.change_tag_protocol = rtl8365mb_change_tag_protocol,
>  	.setup = rtl8365mb_setup,
>  	.teardown = rtl8365mb_teardown,
>  	.phylink_get_caps = rtl8365mb_phylink_get_caps,
> -- 
> 2.35.1
> 

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

[Andrew Lunn]

On Wed, Feb 09, 2022 at 11:51:58PM +0200, Vladimir Oltean wrote:
> Re: title. Tail or trailing?
> 
> On Wed, Feb 09, 2022 at 06:13:11PM -0300, Luiz Angelo Daros de Luca wrote:
> > +static inline void rtl8_4_write_tag(struct sk_buff *skb, struct net_device *dev,
> > +				    char *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 = (__be16 *)tag;
> 
> Can the tail tag be aligned to an odd offset? In that case, should you
> access byte by byte, maybe? I'm not sure how arches handle this.

You should use get_unaligned_be16().

    Andrew

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

[Luiz Angelo Daros de Luca]

> Re: title. Tail or trailing?

I guess I'll stick with trailing. It looks like the most used term.

>
> On Wed, Feb 09, 2022 at 06:13:11PM -0300, Luiz Angelo Daros de Luca wrote:
> > +static inline void rtl8_4_write_tag(struct sk_buff *skb, struct net_device *dev,
> > +                                 char *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 = (__be16 *)tag;
>
> Can the tail tag be aligned to an odd offset? In that case, should you
> access byte by byte, maybe? I'm not sure how arches handle this.

I see. I'm not sure my big endian MIPS device is able to test it
properly. I tried a wide range of payload sizes without any issue. But
I believe we need to play safe here.

Andrew Lunn, you suggested using get_unaligned_be16(). However, as
using get_unaligned_be16() I will already save the tag (or at least
part of it) in the stack, could it be a memcpy from stack to the
buffer (and the opposite while reading the tag)? It will be less
intervention than rewriting the code to deal with each byte at a time.
I'm not sure if I need to or can help the compiler optimize it. In my
MIPS, it seems it did a good job, replacing the memcpy with four
swl/swr instructions (used to write unaligned 32-bit values).

> >
> >       /* 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)));
> > +}
> > +
> > +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)
> > +{
>
> Why don't you want to add:
>
>         if (skb->ip_summed == CHECKSUM_PARTIAL && skb_checksum_help(skb))
>                 return NULL;
>
> and then you'll make this tagging protocol useful in production too.

It works like a charm! Thanks! And now I have a pretty good use for
this new tag: force checksum in software. Whenever the cpu ethernet
driver cannot correctly deal with the checksum offloading, switch to
rtl8_4t and be happy. It will work either adding 'dsa-tag-protocol =
"rtl8_4t";' to the CPU port in the device-tree file or even changing
the tag at runtime.

Now checksum will only break if you stack two trailing tags and the
first one added does not calculate the checksum and the second one is
"rtl8_4t". When "rtl8_4t" does calculate the checksum, it will include
the other tag in the sum. But it might even be considered a bug in the
first tagger.

Regards,

Luiz

Re: [PATCH net-next 2/2] net: dsa: realtek: rtl8365mb: add support for rtl8_4t

[Luiz Angelo Daros de Luca]

> >  rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
> >                          enum dsa_tag_protocol mp)
> >  {
> > +     struct realtek_priv *priv = ds->priv;
> > +     u32 val;
> > +
> > +     /* No way to return error */
> > +     regmap_read(priv->map, RTL8365MB_CPU_CTRL_REG, &val);
> > +
> > +     switch (FIELD_GET(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, val)) {
> > +     case RTL8365MB_CPU_FORMAT_4BYTES:
> > +             /* Similar to DSA_TAG_PROTO_RTL4_A but with 1-byte version
> > +              * To CPU: [0x88 0x99 0x04 portnum]. Not supported yet.
> > +              */
> > +             break;
> > +     case RTL8365MB_CPU_FORMAT_8BYTES:
> > +             switch (FIELD_GET(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, val)) {
> > +             case RTL8365MB_CPU_POS_BEFORE_CRC:
> > +                     return DSA_TAG_PROTO_RTL8_4T;
> > +             case RTL8365MB_CPU_POS_AFTER_SA:
> > +             default:
> > +                     return DSA_TAG_PROTO_RTL8_4;
> > +             }
> > +             break;
> > +     }
> > +
> >       return DSA_TAG_PROTO_RTL8_4;
>
> Not great. dsa_get_tag_protocol gets called from dsa_port_parse_cpu,
> which is earlier than rtl8365mb_cpu_config, so you may temporarily
> report a tagging protocol which you don't expect (depending on what is
> in hardware at the time). Can you not keep the current tagging protocol
> in a variable? You could then avoid the need to return an error on
> regmap_read too.

Thanks Vladimir.

Sure. I added the variable to the chip_data struct. With that, I can
also remove the tag-related code and variables from
rtl8365mb_cpu_config() and rtl8365mb_cpu. Now I simply call the same
rtl8365mb_change_tag_protocol() during setup. I believe it is better
to have a single place that touches that config.

Regards,

Luiz