Re: [PATCH net-next v8 3/6] net: dsa: Introduce dsa tagger data operation.
From: Vladimir Oltean <olteanv@gmail.com>
Date: 2022-09-11 13:46:41
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/
quoted hunk ↗ jump to hunk
Reviewed-by: Andrew Lunn <andrew@lunn.ch> Signed-off-by: Mattias Forsblad <redacted> --- 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.
quoted hunk ↗ jump to hunk
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?
quoted hunk ↗ jump to hunk
+ 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