Re: [PATCH net-next v14 3/7] net: dsa: Introduce dsa tagger data operation.

[Vladimir Oltean <olteanv@gmail.com>]

Regarding the commit title. There is a difference between DSA the
framework and DSA the Marvell implementation of a tagging protocol.
This patch implements something pertaining to the latter, hence the
prefix should be "net: dsa: tag_dsa: ".

Why do I have a feeling I've said this before?
https://patchwork.kernel.org/project/netdevbpf/patch/20220909085138.3539952-4-mattias.forsblad@gmail.com/#25006260

Also, the body of the commit title, and the commit message, are
meaningless. What dsa tagger data operations? Connect what to what and why?
What does this patch really achieve? What I'd like to see as a reviewer
is a summary of the change, plus a justification for how the solution
came to be what it is.

On Mon, Sep 19, 2022 at 01:08:43PM +0200, Mattias Forsblad wrote:
> Support connecting dsa tagger for frame2reg decoding
> with its associated hookup functions.
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
> ---
>  include/linux/dsa/mv88e6xxx.h |  6 ++++++
>  net/dsa/dsa_priv.h            |  2 ++
>  net/dsa/tag_dsa.c             | 40 +++++++++++++++++++++++++++++++----
>  3 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/include/linux/dsa/mv88e6xxx.h b/include/linux/dsa/mv88e6xxx.h
> index 8c3d45eca46b..a8b6f3c110e5 100644
> --- a/include/linux/dsa/mv88e6xxx.h
> +++ b/include/linux/dsa/mv88e6xxx.h
> @@ -5,9 +5,15 @@
>  #ifndef _NET_DSA_TAG_MV88E6XXX_H
>  #define _NET_DSA_TAG_MV88E6XXX_H
>  
> +#include <net/dsa.h>
>  #include <linux/if_vlan.h>
>  
>  #define MV88E6XXX_VID_STANDALONE	0
>  #define MV88E6XXX_VID_BRIDGED		(VLAN_N_VID - 1)
>  
> +struct dsa_tagger_data {
> +	void (*decode_frame2reg)(struct dsa_switch *ds,
> +				 struct sk_buff *skb);
> +};
> +
>  #endif
> diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
> index 614fbba8fe39..3b23b37eb0f4 100644
> --- a/net/dsa/dsa_priv.h
> +++ b/net/dsa/dsa_priv.h
> @@ -17,6 +17,8 @@
>  
>  #define DSA_MAX_NUM_OFFLOADING_BRIDGES		BITS_PER_LONG
>  
> +#define DSA_FRAME2REG_SOURCE_DEV		GENMASK(5, 0)
> +

So in v8 I said that struct dsa_tagger_data is a hardware-specific data
structure, and it has no place in the common net/dsa/dsa_priv.h header
for the framework, and that you should move it to include/linux/dsa/mv88e6xxx.h.
https://patchwork.kernel.org/project/netdevbpf/patch/20220909085138.3539952-4-mattias.forsblad@gmail.com/#25006260
Which you did in v9. But whoop, one more hardware-specific definition
appears in v9, this DSA_FRAME2REG_SOURCE_DEV, again in net/dsa/dsa_priv.h.
https://patchwork.kernel.org/project/netdevbpf/patch/20220912112855.339804-4-mattias.forsblad@gmail.com/
Please use your own judgement to infer the fact that multiple attempts
to code up things in the same way will just lead to more of the same
observations during review.

>  enum {
>  	DSA_NOTIFIER_AGEING_TIME,
>  	DSA_NOTIFIER_BRIDGE_JOIN,
> diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
> index e4b6e3f2a3db..e7fdf3b5cb4a 100644
> --- a/net/dsa/tag_dsa.c
> +++ b/net/dsa/tag_dsa.c
> @@ -198,8 +198,11 @@ static struct sk_buff *dsa_xmit_ll(struct sk_buff *skb, struct net_device *dev,
>  static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
>  				  u8 extra)
>  {
> +	struct dsa_port *cpu_dp = dev->dsa_ptr;
> +	struct dsa_tagger_data *tagger_data;
>  	bool trap = false, trunk = false;
>  	int source_device, source_port;
> +	struct dsa_switch *ds;
>  	enum dsa_code code;
>  	enum dsa_cmd cmd;
>  	u8 *dsa_header;
> @@ -218,9 +221,16 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
>  
>  		switch (code) {
>  		case DSA_CODE_FRAME2REG:
> -			/* Remote management is not implemented yet,
> -			 * drop.
> -			 */
> +			source_device = FIELD_GET(DSA_FRAME2REG_SOURCE_DEV, dsa_header[0]);
> +			ds = dsa_switch_find(cpu_dp->dst->index, source_device);
> +			if (ds) {
> +				tagger_data = ds->tagger_data;

Can you please also parse the sequence number here, so the
decode_frame2reg() data consumer doesn't have to concern itself with the
dsa_header at all?

> +				if (likely(tagger_data->decode_frame2reg))
> +					tagger_data->decode_frame2reg(ds, skb);
> +			} else {
> +				net_err_ratelimited("RMU: Didn't find switch with index %d",
> +						    source_device);
> +			}
>  			return NULL;
>  		case DSA_CODE_ARP_MIRROR:
>  		case DSA_CODE_POLICY_MIRROR:
> @@ -254,7 +264,6 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
>  	source_port = (dsa_header[1] >> 3) & 0x1f;
>  
>  	if (trunk) {
> -		struct dsa_port *cpu_dp = dev->dsa_ptr;
>  		struct dsa_lag *lag;
>  
>  		/* The exact source port is not available in the tag,
> @@ -323,6 +332,25 @@ static struct sk_buff *dsa_rcv_ll(struct sk_buff *skb, struct net_device *dev,
>  	return skb;
>  }
>  
> +static int dsa_tag_connect(struct dsa_switch *ds)
> +{
> +	struct dsa_tagger_data *tagger_data;
> +
> +	tagger_data = kzalloc(sizeof(*tagger_data), GFP_KERNEL);
> +	if (!tagger_data)
> +		return -ENOMEM;
> +
> +	ds->tagger_data = tagger_data;
> +
> +	return 0;
> +}
> +
> +static void dsa_tag_disconnect(struct dsa_switch *ds)
> +{
> +	kfree(ds->tagger_data);
> +	ds->tagger_data = NULL;
> +}
> +
>  #if IS_ENABLED(CONFIG_NET_DSA_TAG_DSA)
>  
>  static struct sk_buff *dsa_xmit(struct sk_buff *skb, struct net_device *dev)
> @@ -343,6 +371,8 @@ static const struct dsa_device_ops dsa_netdev_ops = {
>  	.proto	  = DSA_TAG_PROTO_DSA,
>  	.xmit	  = dsa_xmit,
>  	.rcv	  = dsa_rcv,
> +	.connect  = dsa_tag_connect,
> +	.disconnect = dsa_tag_disconnect,
>  	.needed_headroom = DSA_HLEN,
>  };
>  
> @@ -385,6 +415,8 @@ static const struct dsa_device_ops edsa_netdev_ops = {
>  	.proto	  = DSA_TAG_PROTO_EDSA,
>  	.xmit	  = edsa_xmit,
>  	.rcv	  = edsa_rcv,
> +	.connect  = dsa_tag_connect,
> +	.disconnect = dsa_tag_disconnect,
>  	.needed_headroom = EDSA_HLEN,
>  };
>  
> -- 
> 2.25.1
>