Re: [PATCH v1] net: mvpp2: Add parser configuration for DSA tags

[Michal Swiatkowski <michal.swiatkowski@linux.intel.com>]

On Tue, Jan 21, 2025 at 03:18:04PM +1300, Aryan Srivastava wrote:
> Allow the header parser to consider DSA and EDSA tagging. Currently the
> parser is always configured to use the MH tag, but this results in poor
> traffic distribution across queues and sub-optimal performance (in the
> case where DSA or EDSA tags are in the header).
> 
> Add mechanism to check for tag type in use and then configure the
> parser correctly for this tag. This results in proper traffic
> distribution and hash calculation.
> 
> Use mvpp2_get_tag instead of reading the MH register to determine tag
> type. As the MH register is set during mvpp2_open it is subject to
> change and not a proper reflection of the tagging type in use.
> 
> Signed-off-by: Aryan Srivastava <aryan.srivastava@alliedtelesis.co.nz>

You didn't specify the tree, but it look like a feature implementation,
not a fix; net-next is closed now [1].

[1] https://lore.kernel.org/netdev/20250120163446.3bd93e30@kernel.org/#t

> ---
> Changes in v1:
> - use mvpp2_get_tag to ascertain tagging type in use.
> 
>  drivers/net/ethernet/marvell/mvpp2/mvpp2.h    |  3 ++
>  .../net/ethernet/marvell/mvpp2/mvpp2_main.c   | 37 ++++++++++++++++++-
>  .../net/ethernet/marvell/mvpp2/mvpp2_prs.c    | 16 +++++---
>  3 files changed, 49 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> index 44fe9b68d1c2..456f9aeb4d82 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2.h
> @@ -59,6 +59,8 @@
>  
>  /* Top Registers */
>  #define MVPP2_MH_REG(port)			(0x5040 + 4 * (port))
> +#define MVPP2_MH_EN				BIT(0)
Defined, but not used.

> +#define MVPP2_DSA_NON_EXTENDED			BIT(4)
>  #define MVPP2_DSA_EXTENDED			BIT(5)
>  #define MVPP2_VER_ID_REG			0x50b0
>  #define MVPP2_VER_PP22				0x10
> @@ -1538,6 +1540,7 @@ void mvpp2_dbgfs_cleanup(struct mvpp2 *priv);
>  void mvpp2_dbgfs_exit(void);
>  
>  void mvpp23_rx_fifo_fc_en(struct mvpp2 *priv, int port, bool en);
> +int mvpp2_get_tag(struct net_device *dev);
>  
>  #ifdef CONFIG_MVPP2_PTP
>  int mvpp22_tai_probe(struct device *dev, struct mvpp2 *priv);
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> index dd76c1b7ed3a..3a954da7660f 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
> @@ -38,6 +38,7 @@
>  #include <net/page_pool/helpers.h>
>  #include <net/tso.h>
>  #include <linux/bpf_trace.h>
> +#include <net/dsa.h>

Nit, can be in alphabetic order (rest is, so it will be cleaner to add
new following alphabetic order).
>  
>  #include "mvpp2.h"
>  #include "mvpp2_prs.h"
> @@ -4769,6 +4770,36 @@ static bool mvpp22_rss_is_supported(struct mvpp2_port *port)
>  		!(port->flags & MVPP2_F_LOOPBACK);
>  }
>  
> +int mvpp2_get_tag(struct net_device *dev)
> +{
> +	int tag;
> +	int dsa_proto = DSA_TAG_PROTO_NONE;

RCT (int dsa_proto before int tag)

> +
> +#if IS_ENABLED(CONFIG_NET_DSA)
> +	if (netdev_uses_dsa(dev))
> +		dsa_proto = dev->dsa_ptr->tag_ops->proto;
> +#endif
> +
> +	switch (dsa_proto) {
> +	case DSA_TAG_PROTO_DSA:
> +		tag = MVPP2_TAG_TYPE_DSA;
> +		break;
> +	case DSA_TAG_PROTO_EDSA:
> +	/**
> +	 * DSA_TAG_PROTO_EDSA and MVPP2_TAG_TYPE_EDSA are
> +	 * referring to separate things. MVPP2_TAG_TYPE_EDSA
> +	 * refers to extended DSA, while DSA_TAG_PROTO_EDSA
> +	 * refers to Ethertype DSA. Ethertype DSA requires no
> +	 * setting in the parser.
> +	 */
> +	default:
> +		tag = MVPP2_TAG_TYPE_MH;
> +		break;

if (dsa_proto == DSA_TAG_PROTO_DSA)
	return MVPP2_TAG_TYPE_DSA

resturn MVPP2_TAG_TYPE_MH;

looks simpler, you can move comment above the if.

> +	}
> +
> +	return tag;
> +}
> +
>  static int mvpp2_open(struct net_device *dev)
>  {
>  	struct mvpp2_port *port = netdev_priv(dev);
> @@ -4788,7 +4819,11 @@ static int mvpp2_open(struct net_device *dev)
>  		netdev_err(dev, "mvpp2_prs_mac_da_accept own addr failed\n");
>  		return err;
>  	}
> -	err = mvpp2_prs_tag_mode_set(port->priv, port->id, MVPP2_TAG_TYPE_MH);
> +
> +	if (netdev_uses_dsa(dev))
> +		err = mvpp2_prs_tag_mode_set(port->priv, port->id, mvpp2_get_tag(dev));

In mvpp2_get_tag() netdev_uses_dsa() is guarded by CONFIG_NET_DSA,
shouldn't it be here too? Or better, check for it is already in
mvpp2_get_tag() you can simple call

err = mvpp2_prs_tag_mode_set(port->priv, port->id, mvpp2_get_tag(dev));

as mvpp2_get_tag(dev) returns MVPP2_TAG_TYPE_MH in case
netdev_uses_dsa() returns 0.

> +	else
> +		err = mvpp2_prs_tag_mode_set(port->priv, port->id, MVPP2_TAG_TYPE_MH);
>  	if (err) {
>  		netdev_err(dev, "mvpp2_prs_tag_mode_set failed\n");
>  		return err;
> diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_prs.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_prs.c
> index 9af22f497a40..f14b9e8c301e 100644
> --- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_prs.c
> +++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_prs.c
> @@ -1963,7 +1963,7 @@ int mvpp2_prs_vid_entry_add(struct mvpp2_port *port, u16 vid)
>  {
>  	unsigned int vid_start = MVPP2_PE_VID_FILT_RANGE_START +
>  				 port->id * MVPP2_PRS_VLAN_FILT_MAX;
> -	unsigned int mask = 0xfff, reg_val, shift;
> +	unsigned int mask = 0xfff, tag, shift;
>  	struct mvpp2 *priv = port->priv;
>  	struct mvpp2_prs_entry pe;
>  	int tid;
> @@ -1973,8 +1973,8 @@ int mvpp2_prs_vid_entry_add(struct mvpp2_port *port, u16 vid)
>  	/* Scan TCAM and see if entry with this <vid,port> already exist */
>  	tid = mvpp2_prs_vid_range_find(port, vid, mask);
>  
> -	reg_val = mvpp2_read(priv, MVPP2_MH_REG(port->id));
> -	if (reg_val & MVPP2_DSA_EXTENDED)
> +	tag = mvpp2_get_tag(port->dev);
> +	if (tag & MVPP2_DSA_EXTENDED)

I will drop tag and use mvpp2_get_tag() directly (it isn't used anywhere
else in this function). This is only preference, ignore it if you prefer
to store it somewhere.

Am I right that code under if is unreachable? tag can be 1 or 2
(MVPP2_TAG_TYPE_MH or MVPP2_TAG_TYPE_DSA) and MVPP2_DSA_EXTENDED is
BIT(5). It looks like sth is missing between getting the tag and
checking it with this value.

>  		shift = MVPP2_VLAN_TAG_EDSA_LEN;
>  	else
>  		shift = MVPP2_VLAN_TAG_LEN;
> @@ -2071,7 +2071,7 @@ void mvpp2_prs_vid_enable_filtering(struct mvpp2_port *port)
>  {
>  	unsigned int tid = MVPP2_PRS_VID_PORT_DFLT(port->id);
>  	struct mvpp2 *priv = port->priv;
> -	unsigned int reg_val, shift;
> +	unsigned int tag, shift;
>  	struct mvpp2_prs_entry pe;
>  
>  	if (priv->prs_shadow[tid].valid)
> @@ -2081,8 +2081,8 @@ void mvpp2_prs_vid_enable_filtering(struct mvpp2_port *port)
>  
>  	pe.index = tid;
>  
> -	reg_val = mvpp2_read(priv, MVPP2_MH_REG(port->id));
> -	if (reg_val & MVPP2_DSA_EXTENDED)
> +	tag = mvpp2_get_tag(port->dev);
> +	if (tag & MVPP2_DSA_EXTENDED)

The same here, unreachable.

>  		shift = MVPP2_VLAN_TAG_EDSA_LEN;
>  	else
>  		shift = MVPP2_VLAN_TAG_LEN;
> @@ -2393,6 +2393,8 @@ int mvpp2_prs_tag_mode_set(struct mvpp2 *priv, int port, int type)
>  				      MVPP2_PRS_TAGGED, MVPP2_PRS_DSA);
>  		mvpp2_prs_dsa_tag_set(priv, port, false,
>  				      MVPP2_PRS_UNTAGGED, MVPP2_PRS_DSA);
> +		/* Set Marvell Header register for Ext. DSA tag */
> +		mvpp2_write(priv, MVPP2_MH_REG(port), MVPP2_DSA_EXTENDED);
>  		break;
>  
>  	case MVPP2_TAG_TYPE_DSA:
> @@ -2406,6 +2408,8 @@ int mvpp2_prs_tag_mode_set(struct mvpp2 *priv, int port, int type)
>  				      MVPP2_PRS_TAGGED, MVPP2_PRS_EDSA);
>  		mvpp2_prs_dsa_tag_set(priv, port, false,
>  				      MVPP2_PRS_UNTAGGED, MVPP2_PRS_EDSA);
> +		/* Set Marvell Header register for DSA tag */
> +		mvpp2_write(priv, MVPP2_MH_REG(port), MVPP2_DSA_NON_EXTENDED);
>  		break;
>  
>  	case MVPP2_TAG_TYPE_MH:
> -- 
> 2.47.1