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

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

[Luiz Angelo Daros de Luca]

This series add support for rtl8_4t tag. It is a variant of rtl8_4 tag,
with identical values but placed at the end of the packet. 

It forces checksum in software before adding the tag as those extra
bytes at the end of the packet would be summed together with the rest of
the payload. When the switch removes the tag before sending the packet
to the network, that checksum will not match.

It might be useful to diagnose or avoid checksum offload issues. With an
ethertype tag like rtl8_4, the cpu port ethernet driver must work with
cksum_start and chksum_offset in order to correctly calculate checksums.
If not, the checksum field will be broken (it will contain the fake ip
header sum).  In those cases, using 'rtl8_4t' might be an alternative
way to avoid checksum offload, either using runtime or device-tree
property.

Regards,

Luiz

v1-v2)
- Remove mention to tail tagger, use trailing tagger.
- use void* instead of char* for pointing to tag beginning
- use memcpy to avoid problems with unaligned tags
- calculate checksum if it still pending
- keep in-use tag protocol in memory instead of reading from switch
  register

[PATCH net-next v2 1/2] net: dsa: tag_rtl8_4: add rtl8_4t trailing 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 | 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);
+}
+
+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];
 
-	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);
 
 	/* 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 +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");
-- 
2.35.1

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

[Alvin Šipraga]

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");

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

[Luiz Angelo Daros de Luca]

Em sex., 18 de fev. de 2022 às 08:46, Alvin Šipraga
<ALSI@bang-olufsen.dk> escreveu:
>
> 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/?

Thanks!

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

OK

>
> >
> > 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?

The last version I sent, there was a valid concern about unaligned
access. With ethertype tags, we know the exact position the tag will
be. However, at the end of the packet, the two bytes might fall into
different words depending on the payload. I did test different
payloads without any issues but my big endian system might have
helped.

I checked the machine code and the compiler seems to be doing a good
job here, converting the memcpy to a simple "register to memory" each
byte at a time.

>
> > +}
> > +
> > +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?

Sure! My bad.

>
> >
> > -     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.

I didn't add it because no trailing tag used it. I checked
tag_hellcreek.c, tag_ksz.c, tag_xrs700x.c. tag_sja1105.c seems to use
both head and tail space and it indeed use pskb_may_pull() but only
related to the head space (SJA1110_HEADER_LEN).

>
> > +     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");

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

[Luiz Angelo Daros de Luca]

The trailing 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 | 78 ++++++++++++++++++++++++-----
 1 file changed, 66 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
index 2ed592147c20..043cac34e906 100644
--- a/drivers/net/dsa/realtek/rtl8365mb.c
+++ b/drivers/net/dsa/realtek/rtl8365mb.c
@@ -524,9 +524,7 @@ enum rtl8365mb_cpu_rxlen {
  * @mask: port mask of ports that parse should parse CPU tags
  * @trap_port: forward trapped frames to this port
  * @insert: CPU tag insertion mode in switch->CPU frames
- * @position: position of CPU tag in frame
  * @rx_length: minimum CPU RX length
- * @format: CPU tag format
  *
  * Represents the CPU tagging and CPU port configuration of the switch. These
  * settings are configurable at runtime.
@@ -536,9 +534,7 @@ struct rtl8365mb_cpu {
 	u32 mask;
 	u32 trap_port;
 	enum rtl8365mb_cpu_insert insert;
-	enum rtl8365mb_cpu_position position;
 	enum rtl8365mb_cpu_rxlen rx_length;
-	enum rtl8365mb_cpu_format format;
 };
 
 /**
@@ -566,6 +562,7 @@ struct rtl8365mb_port {
  * @chip_ver: chip silicon revision
  * @port_mask: mask of all ports
  * @learn_limit_max: maximum number of L2 addresses the chip can learn
+ * @tag_protocol: current switch CPU tag protocol
  * @mib_lock: prevent concurrent reads of MIB counters
  * @ports: per-port data
  * @jam_table: chip-specific initialization jam table
@@ -580,6 +577,7 @@ struct rtl8365mb {
 	u32 chip_ver;
 	u32 port_mask;
 	u32 learn_limit_max;
+	enum dsa_tag_protocol tag_protocol;
 	struct mutex mib_lock;
 	struct rtl8365mb_port ports[RTL8365MB_MAX_NUM_PORTS];
 	const struct rtl8365mb_jam_tbl_entry *jam_table;
@@ -770,7 +768,54 @@ static enum dsa_tag_protocol
 rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
 			   enum dsa_tag_protocol mp)
 {
-	return DSA_TAG_PROTO_RTL8_4;
+	struct realtek_priv *priv = ds->priv;
+	struct rtl8365mb *chip_data;
+
+	chip_data = priv->chip_data;
+
+	return chip_data->tag_protocol;
+}
+
+static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds, int cpu,
+					 enum dsa_tag_protocol proto)
+{
+	struct realtek_priv *priv = ds->priv;
+	struct rtl8365mb *chip_data;
+	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;
+
+	chip_data = priv->chip_data;
+	chip_data->tag_protocol = proto;
+
+	return 0;
 }
 
 static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
@@ -1739,13 +1784,18 @@ static int rtl8365mb_cpu_config(struct realtek_priv *priv, const struct rtl8365m
 
 	val = FIELD_PREP(RTL8365MB_CPU_CTRL_EN_MASK, cpu->enable ? 1 : 0) |
 	      FIELD_PREP(RTL8365MB_CPU_CTRL_INSERTMODE_MASK, cpu->insert) |
-	      FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, cpu->position) |
 	      FIELD_PREP(RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK, cpu->rx_length) |
-	      FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, cpu->format) |
 	      FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_MASK, cpu->trap_port & 0x7) |
 	      FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK,
 			 cpu->trap_port >> 3 & 0x1);
-	ret = regmap_write(priv->map, RTL8365MB_CPU_CTRL_REG, val);
+
+	ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG,
+				 RTL8365MB_CPU_CTRL_EN_MASK |
+				 RTL8365MB_CPU_CTRL_INSERTMODE_MASK |
+				 RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK |
+				 RTL8365MB_CPU_CTRL_TRAP_PORT_MASK |
+				 RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK,
+				 val);
 	if (ret)
 		return ret;
 
@@ -1827,6 +1877,11 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
 		dev_info(priv->dev, "no interrupt support\n");
 
 	/* Configure CPU tagging */
+	ret = rtl8365mb_change_tag_protocol(priv->ds, -1, DSA_TAG_PROTO_RTL8_4);
+	if (ret) {
+		dev_err(priv->dev, "failed to set default tag protocol: %d\n", ret);
+		return ret;
+	}
 	cpu.trap_port = RTL8365MB_MAX_NUM_PORTS;
 	dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) {
 		cpu.mask |= BIT(cpu_dp->index);
@@ -1834,13 +1889,9 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
 		if (cpu.trap_port == RTL8365MB_MAX_NUM_PORTS)
 			cpu.trap_port = cpu_dp->index;
 	}
-
 	cpu.enable = cpu.mask > 0;
 	cpu.insert = RTL8365MB_CPU_INSERT_TO_ALL;
-	cpu.position = RTL8365MB_CPU_POS_AFTER_SA;
 	cpu.rx_length = RTL8365MB_CPU_RXLEN_64BYTES;
-	cpu.format = RTL8365MB_CPU_FORMAT_8BYTES;
-
 	ret = rtl8365mb_cpu_config(priv, &cpu);
 	if (ret)
 		goto out_teardown_irq;
@@ -1982,6 +2033,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
 		mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX;
 		mb->jam_table = rtl8365mb_init_jam_8365mb_vc;
 		mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc);
+		mb->tag_protocol = DSA_TAG_PROTO_RTL8_4;
 
 		break;
 	default:
@@ -1996,6 +2048,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,
@@ -2014,6 +2067,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 v2 2/2] net: dsa: realtek: rtl8365mb: add support for rtl8_4t

[Alvin Šipraga]

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

> The trailing 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 | 78 ++++++++++++++++++++++++-----
>  1 file changed, 66 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> index 2ed592147c20..043cac34e906 100644
> --- a/drivers/net/dsa/realtek/rtl8365mb.c
> +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> @@ -524,9 +524,7 @@ enum rtl8365mb_cpu_rxlen {
>   * @mask: port mask of ports that parse should parse CPU tags
>   * @trap_port: forward trapped frames to this port
>   * @insert: CPU tag insertion mode in switch->CPU frames
> - * @position: position of CPU tag in frame
>   * @rx_length: minimum CPU RX length
> - * @format: CPU tag format
>   *
>   * Represents the CPU tagging and CPU port configuration of the switch. These
>   * settings are configurable at runtime.
> @@ -536,9 +534,7 @@ struct rtl8365mb_cpu {
>  	u32 mask;
>  	u32 trap_port;
>  	enum rtl8365mb_cpu_insert insert;
> -	enum rtl8365mb_cpu_position position;
>  	enum rtl8365mb_cpu_rxlen rx_length;
> -	enum rtl8365mb_cpu_format format;

This struct is meant to represent the whole CPU config register. Rather
than pulling it out and adding tag_protocol to struct rtl8365mb, can you
instead do something like:

- keep these members of _cpu
- put back the cpu member of struct rtl8365mb (I don't know why it was removed...)
- in get_tag_protocol: return mb->cpu.position == AFTER_SA ? RTL8_4 : RTL8_4T;
- in change_tag_protocol: just update mb->cpu.position and call
  rtl8365mb_cpu_config again
- avoid the arcane call to rtl8365mb_change_tag_protocol in _setup
- avoid the need to do regmap_update_bits instead of a clean
  regmap_write in one place

The reason I'm saying this is because, in the original version of the
driver, CPU configuration was in a single place. Now it is scattered. I
would kindly ask that you try to respect the existing design because I
can already see that things are starting to get a bit messy.

If we subsequently want to configure other CPU parameters on the fly, it
will be as easy as updating the cpu struct and calling cpu_config
again. This register is also non-volatile so the state we keep will
always conform with the switch configuration.

Sorry if you find the feedback too opinionated - I don't mean anything
personally. But the original design was not by accident, so I would
appreciate if we can keep it that way unless there is a good reason to
change it.

>  };
>  
>  /**
> @@ -566,6 +562,7 @@ struct rtl8365mb_port {
>   * @chip_ver: chip silicon revision
>   * @port_mask: mask of all ports
>   * @learn_limit_max: maximum number of L2 addresses the chip can learn
> + * @tag_protocol: current switch CPU tag protocol
>   * @mib_lock: prevent concurrent reads of MIB counters
>   * @ports: per-port data
>   * @jam_table: chip-specific initialization jam table
> @@ -580,6 +577,7 @@ struct rtl8365mb {
>  	u32 chip_ver;
>  	u32 port_mask;
>  	u32 learn_limit_max;
> +	enum dsa_tag_protocol tag_protocol;
>  	struct mutex mib_lock;
>  	struct rtl8365mb_port ports[RTL8365MB_MAX_NUM_PORTS];
>  	const struct rtl8365mb_jam_tbl_entry *jam_table;
> @@ -770,7 +768,54 @@ static enum dsa_tag_protocol
>  rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
>  			   enum dsa_tag_protocol mp)
>  {
> -	return DSA_TAG_PROTO_RTL8_4;
> +	struct realtek_priv *priv = ds->priv;
> +	struct rtl8365mb *chip_data;

Please stick to the convention and call this struct rtl8365mb pointer mb.

> +
> +	chip_data = priv->chip_data;
> +
> +	return chip_data->tag_protocol;
> +}
> +
> +static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds, int cpu,
> +					 enum dsa_tag_protocol proto)
> +{
> +	struct realtek_priv *priv = ds->priv;
> +	struct rtl8365mb *chip_data;

s/chip_data/mb/ per convention

> +	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;
> +
> +	chip_data = priv->chip_data;

nit: I would put this assignment up top like in the rest of the driver,
respecting reverse-christmass-tree order. It's nice to stick to the
existing style.

> +	chip_data->tag_protocol = proto;
> +
> +	return 0;
>  }
>  
>  static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
> @@ -1739,13 +1784,18 @@ static int rtl8365mb_cpu_config(struct realtek_priv *priv, const struct rtl8365m
>  
>  	val = FIELD_PREP(RTL8365MB_CPU_CTRL_EN_MASK, cpu->enable ? 1 : 0) |
>  	      FIELD_PREP(RTL8365MB_CPU_CTRL_INSERTMODE_MASK, cpu->insert) |
> -	      FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, cpu->position) |
>  	      FIELD_PREP(RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK, cpu->rx_length) |
> -	      FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, cpu->format) |
>  	      FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_MASK, cpu->trap_port & 0x7) |
>  	      FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK,
>  			 cpu->trap_port >> 3 & 0x1);
> -	ret = regmap_write(priv->map, RTL8365MB_CPU_CTRL_REG, val);
> +
> +	ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG,
> +				 RTL8365MB_CPU_CTRL_EN_MASK |
> +				 RTL8365MB_CPU_CTRL_INSERTMODE_MASK |
> +				 RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK |
> +				 RTL8365MB_CPU_CTRL_TRAP_PORT_MASK |
> +				 RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK,
> +				 val);
>  	if (ret)
>  		return ret;
>  
> @@ -1827,6 +1877,11 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
>  		dev_info(priv->dev, "no interrupt support\n");
>  
>  	/* Configure CPU tagging */
> +	ret = rtl8365mb_change_tag_protocol(priv->ds, -1, DSA_TAG_PROTO_RTL8_4);
> +	if (ret) {
> +		dev_err(priv->dev, "failed to set default tag protocol: %d\n", ret);
> +		return ret;
> +	}
>  	cpu.trap_port = RTL8365MB_MAX_NUM_PORTS;
>  	dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) {
>  		cpu.mask |= BIT(cpu_dp->index);
> @@ -1834,13 +1889,9 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
>  		if (cpu.trap_port == RTL8365MB_MAX_NUM_PORTS)
>  			cpu.trap_port = cpu_dp->index;
>  	}
> -
>  	cpu.enable = cpu.mask > 0;
>  	cpu.insert = RTL8365MB_CPU_INSERT_TO_ALL;
> -	cpu.position = RTL8365MB_CPU_POS_AFTER_SA;
>  	cpu.rx_length = RTL8365MB_CPU_RXLEN_64BYTES;
> -	cpu.format = RTL8365MB_CPU_FORMAT_8BYTES;

Like I said above, I think it would be nice to put this cpu struct back
in the rtl8365mb private data.

> -
>  	ret = rtl8365mb_cpu_config(priv, &cpu);
>  	if (ret)
>  		goto out_teardown_irq;
> @@ -1982,6 +2033,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>  		mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX;
>  		mb->jam_table = rtl8365mb_init_jam_8365mb_vc;
>  		mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc);
> +		mb->tag_protocol = DSA_TAG_PROTO_RTL8_4;
>  
>  		break;
>  	default:
> @@ -1996,6 +2048,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,
> @@ -2014,6 +2067,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,

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

[Luiz Angelo Daros de Luca]

> > The trailing 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 | 78 ++++++++++++++++++++++++-----
> >  1 file changed, 66 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> > index 2ed592147c20..043cac34e906 100644
> > --- a/drivers/net/dsa/realtek/rtl8365mb.c
> > +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> > @@ -524,9 +524,7 @@ enum rtl8365mb_cpu_rxlen {
> >   * @mask: port mask of ports that parse should parse CPU tags
> >   * @trap_port: forward trapped frames to this port
> >   * @insert: CPU tag insertion mode in switch->CPU frames
> > - * @position: position of CPU tag in frame
> >   * @rx_length: minimum CPU RX length
> > - * @format: CPU tag format
> >   *
> >   * Represents the CPU tagging and CPU port configuration of the switch. These
> >   * settings are configurable at runtime.
> > @@ -536,9 +534,7 @@ struct rtl8365mb_cpu {
> >       u32 mask;
> >       u32 trap_port;
> >       enum rtl8365mb_cpu_insert insert;
> > -     enum rtl8365mb_cpu_position position;
> >       enum rtl8365mb_cpu_rxlen rx_length;
> > -     enum rtl8365mb_cpu_format format;
>
> This struct is meant to represent the whole CPU config register. Rather
> than pulling it out and adding tag_protocol to struct rtl8365mb, can you
> instead do something like:
>
> - keep these members of _cpu
> - put back the cpu member of struct rtl8365mb (I don't know why it was removed...)

The cpu was dropped from the struct rtl8365mb because it had no use
for it. It was only used outside setup to unreliably detect ext int
ports. When I got no other use for it, I removed it (stingily saving
some bytes).

> - in get_tag_protocol: return mb->cpu.position == AFTER_SA ? RTL8_4 : RTL8_4T;

I was doing just that but I changed to an enum dsa_tag_protocol.
mb->cpu.position works together with mb->cpu.format and if it is
RTL8365MB_CPU_FORMAT_4BYTES, the code will have an undefined behavior
(and get_tag_protocol() cannot return an error). My idea was to always
do "DSA tag" to "Realtek registers" and never the opposite to avoid
that situation. get_tag_protocol() is called even before the CPU port
is configured. And although AFTER_SA and cpu format bits unset is the
desired default value, I would like to make it safe by design, not
coincidence.

> - in change_tag_protocol: just update mb->cpu.position and call
>   rtl8365mb_cpu_config again
> - avoid the arcane call to rtl8365mb_change_tag_protocol in _setup
> - avoid the need to do regmap_update_bits instead of a clean
>   regmap_write in one place

The rtl8365mb_cpu_config() was already a multi-register update, doing
a regmap_update_bits(RTL8365MB_CPU_PORT_MASK_REG) and a
regmap_write(RTL8365MB_CPU_CTRL_REG). I thought it would touch too
much just to change a single bit. After the indirect reg access, I'm
trying to touch exclusively what is strictly necessary.

> The reason I'm saying this is because, in the original version of the
> driver, CPU configuration was in a single place. Now it is scattered. I
> would kindly ask that you try to respect the existing design because I
> can already see that things are starting to get a bit messy.

My idea was to bring closer what was asked with what strictly needs to
be done. We agree on having a single place where a setting is applied.
We disagree on the granularity: I think it should be the smallest unit
a caller might be interested to change (a bit in this case), and you
that it should be the cpu-related registers. I don't know which one is
the best option.

I think it is easier to track changes when there is an individual
function that touches it (like adding a printk), instead of
conditionally printing that message from a shared function. Anyway, I
might be exaggerating for this case.

> If we subsequently want to configure other CPU parameters on the fly, it
> will be as easy as updating the cpu struct and calling cpu_config
> again. This register is also non-volatile so the state we keep will
> always conform with the switch configuration.

I'm averse to any copies of data when I could have them at a single
place. Using the CPU struct, it is a two step job: 1) change the
driver cpu struct, 2) apply. In a similar generic situation, I need to
be cautious if someone could potentially change the struct between
step 1) and 2), or even something else before step 1) could have it
changed in memory (row hammer, for example). It might not apply to
this driver but I always try to be skeptical "by design".

> Sorry if you find the feedback too opinionated - I don't mean anything
> personally. But the original design was not by accident, so I would
> appreciate if we can keep it that way unless there is a good reason to
> change it.

Thanks, Alvin. No need to feel sorry. The worst you can do is to
offend my code, my ideas, not me. ;-) It's always good to hear from
you and other devs. I always learn something.

>
> >  };
> >
> >  /**
> > @@ -566,6 +562,7 @@ struct rtl8365mb_port {
> >   * @chip_ver: chip silicon revision
> >   * @port_mask: mask of all ports
> >   * @learn_limit_max: maximum number of L2 addresses the chip can learn
> > + * @tag_protocol: current switch CPU tag protocol
> >   * @mib_lock: prevent concurrent reads of MIB counters
> >   * @ports: per-port data
> >   * @jam_table: chip-specific initialization jam table
> > @@ -580,6 +577,7 @@ struct rtl8365mb {
> >       u32 chip_ver;
> >       u32 port_mask;
> >       u32 learn_limit_max;
> > +     enum dsa_tag_protocol tag_protocol;
> >       struct mutex mib_lock;
> >       struct rtl8365mb_port ports[RTL8365MB_MAX_NUM_PORTS];
> >       const struct rtl8365mb_jam_tbl_entry *jam_table;
> > @@ -770,7 +768,54 @@ static enum dsa_tag_protocol
> >  rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
> >                          enum dsa_tag_protocol mp)
> >  {
> > -     return DSA_TAG_PROTO_RTL8_4;
> > +     struct realtek_priv *priv = ds->priv;
> > +     struct rtl8365mb *chip_data;
>
> Please stick to the convention and call this struct rtl8365mb pointer mb.

That's a great opportunity to ask. I always wondered what mb really
means. I was already asked in an old thread but nobody answered it.
The only "mb" I found is the driver suffix (rtl8365'mb') but it would
not make sense.

>
> > +
> > +     chip_data = priv->chip_data;
> > +
> > +     return chip_data->tag_protocol;
> > +}
> > +
> > +static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds, int cpu,
> > +                                      enum dsa_tag_protocol proto)
> > +{
> > +     struct realtek_priv *priv = ds->priv;
> > +     struct rtl8365mb *chip_data;
>
> s/chip_data/mb/ per convention
>
> > +     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;
> > +
> > +     chip_data = priv->chip_data;
>
> nit: I would put this assignment up top like in the rest of the driver,
> respecting reverse-christmass-tree order. It's nice to stick to the
> existing style.

ok

>
> > +     chip_data->tag_protocol = proto;
> > +
> > +     return 0;
> >  }
> >
> >  static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
> > @@ -1739,13 +1784,18 @@ static int rtl8365mb_cpu_config(struct realtek_priv *priv, const struct rtl8365m
> >
> >       val = FIELD_PREP(RTL8365MB_CPU_CTRL_EN_MASK, cpu->enable ? 1 : 0) |
> >             FIELD_PREP(RTL8365MB_CPU_CTRL_INSERTMODE_MASK, cpu->insert) |
> > -           FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, cpu->position) |
> >             FIELD_PREP(RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK, cpu->rx_length) |
> > -           FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, cpu->format) |
> >             FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_MASK, cpu->trap_port & 0x7) |
> >             FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK,
> >                        cpu->trap_port >> 3 & 0x1);
> > -     ret = regmap_write(priv->map, RTL8365MB_CPU_CTRL_REG, val);
> > +
> > +     ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG,
> > +                              RTL8365MB_CPU_CTRL_EN_MASK |
> > +                              RTL8365MB_CPU_CTRL_INSERTMODE_MASK |
> > +                              RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK |
> > +                              RTL8365MB_CPU_CTRL_TRAP_PORT_MASK |
> > +                              RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK,
> > +                              val);
> >       if (ret)
> >               return ret;
> >
> > @@ -1827,6 +1877,11 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
> >               dev_info(priv->dev, "no interrupt support\n");
> >
> >       /* Configure CPU tagging */
> > +     ret = rtl8365mb_change_tag_protocol(priv->ds, -1, DSA_TAG_PROTO_RTL8_4);
> > +     if (ret) {
> > +             dev_err(priv->dev, "failed to set default tag protocol: %d\n", ret);
> > +             return ret;
> > +     }
> >       cpu.trap_port = RTL8365MB_MAX_NUM_PORTS;
> >       dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) {
> >               cpu.mask |= BIT(cpu_dp->index);
> > @@ -1834,13 +1889,9 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
> >               if (cpu.trap_port == RTL8365MB_MAX_NUM_PORTS)
> >                       cpu.trap_port = cpu_dp->index;
> >       }
> > -
> >       cpu.enable = cpu.mask > 0;
> >       cpu.insert = RTL8365MB_CPU_INSERT_TO_ALL;
> > -     cpu.position = RTL8365MB_CPU_POS_AFTER_SA;
> >       cpu.rx_length = RTL8365MB_CPU_RXLEN_64BYTES;
> > -     cpu.format = RTL8365MB_CPU_FORMAT_8BYTES;
>
> Like I said above, I think it would be nice to put this cpu struct back
> in the rtl8365mb private data.

It would require to split CPU initialization between pre dsa register
(where format must be defined) and dsa_setup (where cpu port is read
from dsa ports and settings applied to the switch). get_tag_protocol()
is called between these two to get the default tag protocol. DSA calls
change_tag_protocol afterwards if the defined tag protocol in the
devicetree does not match.

> > -
> >       ret = rtl8365mb_cpu_config(priv, &cpu);
> >       if (ret)
> >               goto out_teardown_irq;
> > @@ -1982,6 +2033,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
> >               mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX;
> >               mb->jam_table = rtl8365mb_init_jam_8365mb_vc;
> >               mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc);
> > +             mb->tag_protocol = DSA_TAG_PROTO_RTL8_4;
> >
> >               break;
> >       default:
> > @@ -1996,6 +2048,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,
> > @@ -2014,6 +2067,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,

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

[Alvin Šipraga]

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

>> > The trailing 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 | 78 ++++++++++++++++++++++++-----
>> >  1 file changed, 66 insertions(+), 12 deletions(-)
>> >
>> > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
>> > index 2ed592147c20..043cac34e906 100644
>> > --- a/drivers/net/dsa/realtek/rtl8365mb.c
>> > +++ b/drivers/net/dsa/realtek/rtl8365mb.c
>> > @@ -524,9 +524,7 @@ enum rtl8365mb_cpu_rxlen {
>> >   * @mask: port mask of ports that parse should parse CPU tags
>> >   * @trap_port: forward trapped frames to this port
>> >   * @insert: CPU tag insertion mode in switch->CPU frames
>> > - * @position: position of CPU tag in frame
>> >   * @rx_length: minimum CPU RX length
>> > - * @format: CPU tag format
>> >   *
>> >   * Represents the CPU tagging and CPU port configuration of the switch. These
>> >   * settings are configurable at runtime.
>> > @@ -536,9 +534,7 @@ struct rtl8365mb_cpu {
>> >       u32 mask;
>> >       u32 trap_port;
>> >       enum rtl8365mb_cpu_insert insert;
>> > -     enum rtl8365mb_cpu_position position;
>> >       enum rtl8365mb_cpu_rxlen rx_length;
>> > -     enum rtl8365mb_cpu_format format;
>>
>> This struct is meant to represent the whole CPU config register. Rather
>> than pulling it out and adding tag_protocol to struct rtl8365mb, can you
>> instead do something like:
>>
>> - keep these members of _cpu
>> - put back the cpu member of struct rtl8365mb (I don't know why it was removed...)
>
> The cpu was dropped from the struct rtl8365mb because it had no use
> for it. It was only used outside setup to unreliably detect ext int
> ports. When I got no other use for it, I removed it (stingily saving
> some bytes).

This is not a good approach in general as evidenced by this
discussion.

>
>> - in get_tag_protocol: return mb->cpu.position == AFTER_SA ? RTL8_4 : RTL8_4T;
>
> I was doing just that but I changed to an enum dsa_tag_protocol.
> mb->cpu.position works together with mb->cpu.format and if it is
> RTL8365MB_CPU_FORMAT_4BYTES, the code will have an undefined behavior
> (and get_tag_protocol() cannot return an error). My idea was to always
> do "DSA tag" to "Realtek registers" and never the opposite to avoid
> that situation. get_tag_protocol() is called even before the CPU port
> is configured. And although AFTER_SA and cpu format bits unset is the
> desired default value, I would like to make it safe by design, not
> coincidence.

Just check mb->cpu.format as well when adding support for 4-byte tags
then?

BTW, I don't suggest adding the 4-byte tag support unless there is a
very good reason. It contains much less information and will complicate
matters when we want to add e.g. TX forward offloading. That will not be
possible with the 4-byte format, and so we will have to put traps all
over the driver to ensure coherency. Ultimately it is better just not to
add support unless somebody has a hardware requirement which demands it.

>
>> - in change_tag_protocol: just update mb->cpu.position and call
>>   rtl8365mb_cpu_config again
>> - avoid the arcane call to rtl8365mb_change_tag_protocol in _setup
>> - avoid the need to do regmap_update_bits instead of a clean
>>   regmap_write in one place
>
> The rtl8365mb_cpu_config() was already a multi-register update, doing
> a regmap_update_bits(RTL8365MB_CPU_PORT_MASK_REG) and a
> regmap_write(RTL8365MB_CPU_CTRL_REG). I thought it would touch too
> much just to change a single bit. After the indirect reg access, I'm
> trying to touch exclusively what is strictly necessary.

Luiz, it is more important for the code to be coherent. Register writes
are very cheap here. Changing tag protocol is not something that happens
all the time. Same applies to dropping bytes due to stinginess - never
sacrifice the code for some purely theoretical performance optimization.

Also, don't worry about the indirect PHY register access affair. We
fixed it, right? Do not be afraid of the hardware, it is not that
scary. Be afraid of long review cycles because people don't like your
code ;-)

>
>> The reason I'm saying this is because, in the original version of the
>> driver, CPU configuration was in a single place. Now it is scattered. I
>> would kindly ask that you try to respect the existing design because I
>> can already see that things are starting to get a bit messy.
>
> My idea was to bring closer what was asked with what strictly needs to
> be done. We agree on having a single place where a setting is applied.
> We disagree on the granularity: I think it should be the smallest unit
> a caller might be interested to change (a bit in this case), and you
> that it should be the cpu-related registers. I don't know which one is
> the best option.

Have you looked at the implementation of regmap_update_bits? It actually
does two operations: a read and a write.

I fear we have a very divergent philosophy on this matter. By far and
away the most important thing is to keep the code clean and
consistent.

I already explained that these registers are non-volatile.

>
> I think it is easier to track changes when there is an individual
> function that touches it (like adding a printk), instead of
> conditionally printing that message from a shared function. Anyway, I
> might be exaggerating for this case.

This argument is not helpful at all.

>
>> If we subsequently want to configure other CPU parameters on the fly, it
>> will be as easy as updating the cpu struct and calling cpu_config
>> again. This register is also non-volatile so the state we keep will
>> always conform with the switch configuration.
>
> I'm averse to any copies of data when I could have them at a single
> place. Using the CPU struct, it is a two step job: 1) change the
> driver cpu struct, 2) apply. In a similar generic situation, I need to
> be cautious if someone could potentially change the struct between
> step 1) and 2), or even something else before step 1) could have it
> changed in memory (row hammer, for example). It might not apply to
> this driver but I always try to be skeptical "by design".

This argument is also pie-in-the-sky.

>
>> Sorry if you find the feedback too opinionated - I don't mean anything
>> personally. But the original design was not by accident, so I would
>> appreciate if we can keep it that way unless there is a good reason to
>> change it.
>
> Thanks, Alvin. No need to feel sorry. The worst you can do is to
> offend my code, my ideas, not me. ;-) It's always good to hear from
> you and other devs. I always learn something.
>
>>
>> >  };
>> >
>> >  /**
>> > @@ -566,6 +562,7 @@ struct rtl8365mb_port {
>> >   * @chip_ver: chip silicon revision
>> >   * @port_mask: mask of all ports
>> >   * @learn_limit_max: maximum number of L2 addresses the chip can learn
>> > + * @tag_protocol: current switch CPU tag protocol
>> >   * @mib_lock: prevent concurrent reads of MIB counters
>> >   * @ports: per-port data
>> >   * @jam_table: chip-specific initialization jam table
>> > @@ -580,6 +577,7 @@ struct rtl8365mb {
>> >       u32 chip_ver;
>> >       u32 port_mask;
>> >       u32 learn_limit_max;
>> > +     enum dsa_tag_protocol tag_protocol;
>> >       struct mutex mib_lock;
>> >       struct rtl8365mb_port ports[RTL8365MB_MAX_NUM_PORTS];
>> >       const struct rtl8365mb_jam_tbl_entry *jam_table;
>> > @@ -770,7 +768,54 @@ static enum dsa_tag_protocol
>> >  rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
>> >                          enum dsa_tag_protocol mp)
>> >  {
>> > -     return DSA_TAG_PROTO_RTL8_4;
>> > +     struct realtek_priv *priv = ds->priv;
>> > +     struct rtl8365mb *chip_data;
>>
>> Please stick to the convention and call this struct rtl8365mb pointer mb.
>
> That's a great opportunity to ask. I always wondered what mb really
> means. I was already asked in an old thread but nobody answered it.
> The only "mb" I found is the driver suffix (rtl8365'mb') but it would
> not make sense.

Yeah it was just the suffix. You can think of it as the nickname of the
chip in the driver. Many other drivers do this too. Sorry that it
doesn't make sense.

>
>>
>> > +
>> > +     chip_data = priv->chip_data;
>> > +
>> > +     return chip_data->tag_protocol;
>> > +}
>> > +
>> > +static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds, int cpu,
>> > +                                      enum dsa_tag_protocol proto)
>> > +{
>> > +     struct realtek_priv *priv = ds->priv;
>> > +     struct rtl8365mb *chip_data;
>>
>> s/chip_data/mb/ per convention
>>
>> > +     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;
>> > +
>> > +     chip_data = priv->chip_data;
>>
>> nit: I would put this assignment up top like in the rest of the driver,
>> respecting reverse-christmass-tree order. It's nice to stick to the
>> existing style.
>
> ok
>
>>
>> > +     chip_data->tag_protocol = proto;
>> > +
>> > +     return 0;
>> >  }
>> >
>> >  static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
>> > @@ -1739,13 +1784,18 @@ static int rtl8365mb_cpu_config(struct realtek_priv *priv, const struct rtl8365m
>> >
>> >       val = FIELD_PREP(RTL8365MB_CPU_CTRL_EN_MASK, cpu->enable ? 1 : 0) |
>> >             FIELD_PREP(RTL8365MB_CPU_CTRL_INSERTMODE_MASK, cpu->insert) |
>> > -           FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, cpu->position) |
>> >             FIELD_PREP(RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK, cpu->rx_length) |
>> > -           FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, cpu->format) |
>> >             FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_MASK, cpu->trap_port & 0x7) |
>> >             FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK,
>> >                        cpu->trap_port >> 3 & 0x1);
>> > -     ret = regmap_write(priv->map, RTL8365MB_CPU_CTRL_REG, val);
>> > +
>> > +     ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG,
>> > +                              RTL8365MB_CPU_CTRL_EN_MASK |
>> > +                              RTL8365MB_CPU_CTRL_INSERTMODE_MASK |
>> > +                              RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK |
>> > +                              RTL8365MB_CPU_CTRL_TRAP_PORT_MASK |
>> > +                              RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK,
>> > +                              val);
>> >       if (ret)
>> >               return ret;
>> >
>> > @@ -1827,6 +1877,11 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
>> >               dev_info(priv->dev, "no interrupt support\n");
>> >
>> >       /* Configure CPU tagging */
>> > +     ret = rtl8365mb_change_tag_protocol(priv->ds, -1, DSA_TAG_PROTO_RTL8_4);
>> > +     if (ret) {
>> > +             dev_err(priv->dev, "failed to set default tag protocol: %d\n", ret);
>> > +             return ret;
>> > +     }
>> >       cpu.trap_port = RTL8365MB_MAX_NUM_PORTS;
>> >       dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) {
>> >               cpu.mask |= BIT(cpu_dp->index);
>> > @@ -1834,13 +1889,9 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
>> >               if (cpu.trap_port == RTL8365MB_MAX_NUM_PORTS)
>> >                       cpu.trap_port = cpu_dp->index;
>> >       }
>> > -
>> >       cpu.enable = cpu.mask > 0;
>> >       cpu.insert = RTL8365MB_CPU_INSERT_TO_ALL;
>> > -     cpu.position = RTL8365MB_CPU_POS_AFTER_SA;
>> >       cpu.rx_length = RTL8365MB_CPU_RXLEN_64BYTES;
>> > -     cpu.format = RTL8365MB_CPU_FORMAT_8BYTES;
>>
>> Like I said above, I think it would be nice to put this cpu struct back
>> in the rtl8365mb private data.
>
> It would require to split CPU initialization between pre dsa register
> (where format must be defined) and dsa_setup (where cpu port is read
> from dsa ports and settings applied to the switch). get_tag_protocol()
> is called between these two to get the default tag protocol. DSA calls
> change_tag_protocol afterwards if the defined tag protocol in the
> devicetree does not match.

I don't see the problem, sorry. I am basically suggesting to go back to
the way it was done before. You can always change the tag protocol as
you can the rest of the CPU configuration. I even put a comment at the
top of struct rtl8365mb_cpu to this effect:

/**
 * ...
 * Represents the CPU tagging and CPU port configuration of the switch. These
 * settings are configurable at runtime.
 */
struct rtl8365mb_cpu {

>
>> > -
>> >       ret = rtl8365mb_cpu_config(priv, &cpu);
>> >       if (ret)
>> >               goto out_teardown_irq;
>> > @@ -1982,6 +2033,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
>> >               mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX;
>> >               mb->jam_table = rtl8365mb_init_jam_8365mb_vc;
>> >               mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc);
>> > +             mb->tag_protocol = DSA_TAG_PROTO_RTL8_4;
>> >
>> >               break;
>> >       default:
>> > @@ -1996,6 +2048,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,
>> > @@ -2014,6 +2067,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,

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

[Luiz Angelo Daros de Luca]

> >> > The trailing 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 | 78 ++++++++++++++++++++++++-----
> >> >  1 file changed, 66 insertions(+), 12 deletions(-)
> >> >
> >> > diff --git a/drivers/net/dsa/realtek/rtl8365mb.c b/drivers/net/dsa/realtek/rtl8365mb.c
> >> > index 2ed592147c20..043cac34e906 100644
> >> > --- a/drivers/net/dsa/realtek/rtl8365mb.c
> >> > +++ b/drivers/net/dsa/realtek/rtl8365mb.c
> >> > @@ -524,9 +524,7 @@ enum rtl8365mb_cpu_rxlen {
> >> >   * @mask: port mask of ports that parse should parse CPU tags
> >> >   * @trap_port: forward trapped frames to this port
> >> >   * @insert: CPU tag insertion mode in switch->CPU frames
> >> > - * @position: position of CPU tag in frame
> >> >   * @rx_length: minimum CPU RX length
> >> > - * @format: CPU tag format
> >> >   *
> >> >   * Represents the CPU tagging and CPU port configuration of the switch. These
> >> >   * settings are configurable at runtime.
> >> > @@ -536,9 +534,7 @@ struct rtl8365mb_cpu {
> >> >       u32 mask;
> >> >       u32 trap_port;
> >> >       enum rtl8365mb_cpu_insert insert;
> >> > -     enum rtl8365mb_cpu_position position;
> >> >       enum rtl8365mb_cpu_rxlen rx_length;
> >> > -     enum rtl8365mb_cpu_format format;
> >>
> >> This struct is meant to represent the whole CPU config register. Rather
> >> than pulling it out and adding tag_protocol to struct rtl8365mb, can you
> >> instead do something like:
> >>
> >> - keep these members of _cpu
> >> - put back the cpu member of struct rtl8365mb (I don't know why it was removed...)
> >
> > The cpu was dropped from the struct rtl8365mb because it had no use
> > for it. It was only used outside setup to unreliably detect ext int
> > ports. When I got no other use for it, I removed it (stingily saving
> > some bytes).
>
> This is not a good approach in general as evidenced by this
> discussion.

OK. I'll revert it back and use the old code path. I do not have a
strong opinion on either approach.

>
> >
> >> - in get_tag_protocol: return mb->cpu.position == AFTER_SA ? RTL8_4 : RTL8_4T;
> >
> > I was doing just that but I changed to an enum dsa_tag_protocol.
> > mb->cpu.position works together with mb->cpu.format and if it is
> > RTL8365MB_CPU_FORMAT_4BYTES, the code will have an undefined behavior
> > (and get_tag_protocol() cannot return an error). My idea was to always
> > do "DSA tag" to "Realtek registers" and never the opposite to avoid
> > that situation. get_tag_protocol() is called even before the CPU port
> > is configured. And although AFTER_SA and cpu format bits unset is the
> > desired default value, I would like to make it safe by design, not
> > coincidence.
>
> Just check mb->cpu.format as well when adding support for 4-byte tags
> then?
>
> BTW, I don't suggest adding the 4-byte tag support unless there is a
> very good reason. It contains much less information and will complicate
> matters when we want to add e.g. TX forward offloading. That will not be
> possible with the 4-byte format, and so we will have to put traps all
> over the driver to ensure coherency. Ultimately it is better just not to
> add support unless somebody has a hardware requirement which demands it.
>
> >
> >> - in change_tag_protocol: just update mb->cpu.position and call
> >>   rtl8365mb_cpu_config again
> >> - avoid the arcane call to rtl8365mb_change_tag_protocol in _setup
> >> - avoid the need to do regmap_update_bits instead of a clean
> >>   regmap_write in one place
> >
> > The rtl8365mb_cpu_config() was already a multi-register update, doing
> > a regmap_update_bits(RTL8365MB_CPU_PORT_MASK_REG) and a
> > regmap_write(RTL8365MB_CPU_CTRL_REG). I thought it would touch too
> > much just to change a single bit. After the indirect reg access, I'm
> > trying to touch exclusively what is strictly necessary.
>
> Luiz, it is more important for the code to be coherent. Register writes
> are very cheap here. Changing tag protocol is not something that happens
> all the time. Same applies to dropping bytes due to stinginess - never
> sacrifice the code for some purely theoretical performance optimization.
>
> Also, don't worry about the indirect PHY register access affair. We
> fixed it, right? Do not be afraid of the hardware, it is not that
> scary. Be afraid of long review cycles because people don't like your
> code ;-)
>
> >
> >> The reason I'm saying this is because, in the original version of the
> >> driver, CPU configuration was in a single place. Now it is scattered. I
> >> would kindly ask that you try to respect the existing design because I
> >> can already see that things are starting to get a bit messy.
> >
> > My idea was to bring closer what was asked with what strictly needs to
> > be done. We agree on having a single place where a setting is applied.
> > We disagree on the granularity: I think it should be the smallest unit
> > a caller might be interested to change (a bit in this case), and you
> > that it should be the cpu-related registers. I don't know which one is
> > the best option.
>
> Have you looked at the implementation of regmap_update_bits? It actually
> does two operations: a read and a write.

Indeed I did, but it does it in a locked context.

>
> I fear we have a very divergent philosophy on this matter. By far and
> away the most important thing is to keep the code clean and
> consistent.

As I said, no strong feelings for any option. I'll do whatever is the
best option.

> I already explained that these registers are non-volatile.
>
> >
> > I think it is easier to track changes when there is an individual
> > function that touches it (like adding a printk), instead of
> > conditionally printing that message from a shared function. Anyway, I
> > might be exaggerating for this case.
>
> This argument is not helpful at all.
>
> >
> >> If we subsequently want to configure other CPU parameters on the fly, it
> >> will be as easy as updating the cpu struct and calling cpu_config
> >> again. This register is also non-volatile so the state we keep will
> >> always conform with the switch configuration.
> >
> > I'm averse to any copies of data when I could have them at a single
> > place. Using the CPU struct, it is a two step job: 1) change the
> > driver cpu struct, 2) apply. In a similar generic situation, I need to
> > be cautious if someone could potentially change the struct between
> > step 1) and 2), or even something else before step 1) could have it
> > changed in memory (row hammer, for example). It might not apply to
> > this driver but I always try to be skeptical "by design".
>
> This argument is also pie-in-the-sky.
>
> >
> >> Sorry if you find the feedback too opinionated - I don't mean anything
> >> personally. But the original design was not by accident, so I would
> >> appreciate if we can keep it that way unless there is a good reason to
> >> change it.
> >
> > Thanks, Alvin. No need to feel sorry. The worst you can do is to
> > offend my code, my ideas, not me. ;-) It's always good to hear from
> > you and other devs. I always learn something.
> >
> >>
> >> >  };
> >> >
> >> >  /**
> >> > @@ -566,6 +562,7 @@ struct rtl8365mb_port {
> >> >   * @chip_ver: chip silicon revision
> >> >   * @port_mask: mask of all ports
> >> >   * @learn_limit_max: maximum number of L2 addresses the chip can learn
> >> > + * @tag_protocol: current switch CPU tag protocol
> >> >   * @mib_lock: prevent concurrent reads of MIB counters
> >> >   * @ports: per-port data
> >> >   * @jam_table: chip-specific initialization jam table
> >> > @@ -580,6 +577,7 @@ struct rtl8365mb {
> >> >       u32 chip_ver;
> >> >       u32 port_mask;
> >> >       u32 learn_limit_max;
> >> > +     enum dsa_tag_protocol tag_protocol;
> >> >       struct mutex mib_lock;
> >> >       struct rtl8365mb_port ports[RTL8365MB_MAX_NUM_PORTS];
> >> >       const struct rtl8365mb_jam_tbl_entry *jam_table;
> >> > @@ -770,7 +768,54 @@ static enum dsa_tag_protocol
> >> >  rtl8365mb_get_tag_protocol(struct dsa_switch *ds, int port,
> >> >                          enum dsa_tag_protocol mp)
> >> >  {
> >> > -     return DSA_TAG_PROTO_RTL8_4;
> >> > +     struct realtek_priv *priv = ds->priv;
> >> > +     struct rtl8365mb *chip_data;
> >>
> >> Please stick to the convention and call this struct rtl8365mb pointer mb.
> >
> > That's a great opportunity to ask. I always wondered what mb really
> > means. I was already asked in an old thread but nobody answered it.
> > The only "mb" I found is the driver suffix (rtl8365'mb') but it would
> > not make sense.
>
> Yeah it was just the suffix. You can think of it as the nickname of the
> chip in the driver. Many other drivers do this too. Sorry that it
> doesn't make sense.

MB is some kind of model classification, like cars use S, SV, SL. It's
as strange as to name a variable that represents the car family as SV.
MB is used by incompatible models, like RTL8304MB-CG. As it is only
used internally and it is purely cosmetic, I think it might be nice to
rename it, even to a generic name as chip_data, just like "car_data"
is better than "sv".

For now, I'll stick with mb.

>
> >
> >>
> >> > +
> >> > +     chip_data = priv->chip_data;
> >> > +
> >> > +     return chip_data->tag_protocol;
> >> > +}
> >> > +
> >> > +static int rtl8365mb_change_tag_protocol(struct dsa_switch *ds, int cpu,
> >> > +                                      enum dsa_tag_protocol proto)
> >> > +{
> >> > +     struct realtek_priv *priv = ds->priv;
> >> > +     struct rtl8365mb *chip_data;
> >>
> >> s/chip_data/mb/ per convention
> >>
> >> > +     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;
> >> > +
> >> > +     chip_data = priv->chip_data;
> >>
> >> nit: I would put this assignment up top like in the rest of the driver,
> >> respecting reverse-christmass-tree order. It's nice to stick to the
> >> existing style.
> >
> > ok
> >
> >>
> >> > +     chip_data->tag_protocol = proto;
> >> > +
> >> > +     return 0;
> >> >  }
> >> >
> >> >  static int rtl8365mb_ext_config_rgmii(struct realtek_priv *priv, int port,
> >> > @@ -1739,13 +1784,18 @@ static int rtl8365mb_cpu_config(struct realtek_priv *priv, const struct rtl8365m
> >> >
> >> >       val = FIELD_PREP(RTL8365MB_CPU_CTRL_EN_MASK, cpu->enable ? 1 : 0) |
> >> >             FIELD_PREP(RTL8365MB_CPU_CTRL_INSERTMODE_MASK, cpu->insert) |
> >> > -           FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_POSITION_MASK, cpu->position) |
> >> >             FIELD_PREP(RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK, cpu->rx_length) |
> >> > -           FIELD_PREP(RTL8365MB_CPU_CTRL_TAG_FORMAT_MASK, cpu->format) |
> >> >             FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_MASK, cpu->trap_port & 0x7) |
> >> >             FIELD_PREP(RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK,
> >> >                        cpu->trap_port >> 3 & 0x1);
> >> > -     ret = regmap_write(priv->map, RTL8365MB_CPU_CTRL_REG, val);
> >> > +
> >> > +     ret = regmap_update_bits(priv->map, RTL8365MB_CPU_CTRL_REG,
> >> > +                              RTL8365MB_CPU_CTRL_EN_MASK |
> >> > +                              RTL8365MB_CPU_CTRL_INSERTMODE_MASK |
> >> > +                              RTL8365MB_CPU_CTRL_RXBYTECOUNT_MASK |
> >> > +                              RTL8365MB_CPU_CTRL_TRAP_PORT_MASK |
> >> > +                              RTL8365MB_CPU_CTRL_TRAP_PORT_EXT_MASK,
> >> > +                              val);
> >> >       if (ret)
> >> >               return ret;
> >> >
> >> > @@ -1827,6 +1877,11 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
> >> >               dev_info(priv->dev, "no interrupt support\n");
> >> >
> >> >       /* Configure CPU tagging */
> >> > +     ret = rtl8365mb_change_tag_protocol(priv->ds, -1, DSA_TAG_PROTO_RTL8_4);
> >> > +     if (ret) {
> >> > +             dev_err(priv->dev, "failed to set default tag protocol: %d\n", ret);
> >> > +             return ret;
> >> > +     }
> >> >       cpu.trap_port = RTL8365MB_MAX_NUM_PORTS;
> >> >       dsa_switch_for_each_cpu_port(cpu_dp, priv->ds) {
> >> >               cpu.mask |= BIT(cpu_dp->index);
> >> > @@ -1834,13 +1889,9 @@ static int rtl8365mb_setup(struct dsa_switch *ds)
> >> >               if (cpu.trap_port == RTL8365MB_MAX_NUM_PORTS)
> >> >                       cpu.trap_port = cpu_dp->index;
> >> >       }
> >> > -
> >> >       cpu.enable = cpu.mask > 0;
> >> >       cpu.insert = RTL8365MB_CPU_INSERT_TO_ALL;
> >> > -     cpu.position = RTL8365MB_CPU_POS_AFTER_SA;
> >> >       cpu.rx_length = RTL8365MB_CPU_RXLEN_64BYTES;
> >> > -     cpu.format = RTL8365MB_CPU_FORMAT_8BYTES;
> >>
> >> Like I said above, I think it would be nice to put this cpu struct back
> >> in the rtl8365mb private data.
> >
> > It would require to split CPU initialization between pre dsa register
> > (where format must be defined) and dsa_setup (where cpu port is read
> > from dsa ports and settings applied to the switch). get_tag_protocol()
> > is called between these two to get the default tag protocol. DSA calls
> > change_tag_protocol afterwards if the defined tag protocol in the
> > devicetree does not match.
>
> I don't see the problem, sorry. I am basically suggesting to go back to
> the way it was done before. You can always change the tag protocol as
> you can the rest of the CPU configuration. I even put a comment at the
> top of struct rtl8365mb_cpu to this effect:
>
> /**
>  * ...
>  * Represents the CPU tagging and CPU port configuration of the switch. These
>  * settings are configurable at runtime.
>  */
> struct rtl8365mb_cpu {
>
> >
> >> > -
> >> >       ret = rtl8365mb_cpu_config(priv, &cpu);
> >> >       if (ret)
> >> >               goto out_teardown_irq;
> >> > @@ -1982,6 +2033,7 @@ static int rtl8365mb_detect(struct realtek_priv *priv)
> >> >               mb->learn_limit_max = RTL8365MB_LEARN_LIMIT_MAX;
> >> >               mb->jam_table = rtl8365mb_init_jam_8365mb_vc;
> >> >               mb->jam_size = ARRAY_SIZE(rtl8365mb_init_jam_8365mb_vc);
> >> > +             mb->tag_protocol = DSA_TAG_PROTO_RTL8_4;
> >> >
> >> >               break;
> >> >       default:
> >> > @@ -1996,6 +2048,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,
> >> > @@ -2014,6 +2067,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,