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

[Vladimir Oltean <olteanv@gmail.com>]

A proper prefix for this patch would be "net: dsa: tag_dsa: ", since
"net: dsa:" refers to the whole subsystem.

On Fri, Sep 09, 2022 at 10:51:35AM +0200, Mattias Forsblad wrote:
> Support connecting dsa tagger for frame2reg decoding
> with it's associated hookup functions.

s/it's/its/

> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> Signed-off-by: Mattias Forsblad <mattias.forsblad@gmail.com>
> ---
>  include/net/dsa.h |  5 +++++
>  net/dsa/tag_dsa.c | 32 +++++++++++++++++++++++++++++---
>  2 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/include/net/dsa.h b/include/net/dsa.h
> index 0e8a7ef17490..8510267d6188 100644
> --- a/include/net/dsa.h
> +++ b/include/net/dsa.h
> @@ -130,6 +130,11 @@ struct dsa_lag {
>  	refcount_t refcount;
>  };
>  
> +struct dsa_tagger_data {
> +	void (*decode_frame2reg)(struct net_device *netdev,
> +				 struct sk_buff *skb);
> +};
> +

You probably mean to put this in include/linux/dsa/mv88e6xxx.h. Despite
the common naming, there is a big difference between DSA the framework
and DSA the Marvell implementation.

>  struct dsa_switch_tree {
>  	struct list_head	list;
>  
> diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
> index e4b6e3f2a3db..3dd1dcddaf05 100644
> --- a/net/dsa/tag_dsa.c
> +++ b/net/dsa/tag_dsa.c
> @@ -198,7 +198,10 @@ 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_tagger_data *tagger_data;
> +	struct dsa_port *dp = dev->dsa_ptr;
>  	bool trap = false, trunk = false;
> +	struct dsa_switch *ds = dp->ds;
>  	int source_device, source_port;
>  	enum dsa_code code;
>  	enum dsa_cmd cmd;
> @@ -218,9 +221,9 @@ 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.
> -			 */
> +			tagger_data = ds->tagger_data;

You allocate one ds->tagger_data structure for each switch in the tree,
but you always use the tagger_data of the upstream-most "ds", the one
associated with the cpu_dp behind master->dsa_ptr.

How about minimally parsing the skb within the tagger, to figure out the
proper destination switch, and pass to tagger_data->decode_frame2reg()
the actual correct ds, plus a pointer to the skb so you can take a
reference on it?

> +			if (likely(tagger_data->decode_frame2reg))
> +				tagger_data->decode_frame2reg(dev, skb);
>  			return NULL;
>  		case DSA_CODE_ARP_MIRROR:
>  		case DSA_CODE_POLICY_MIRROR:
> @@ -323,6 +326,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 +365,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 +409,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
>