| Message ID | 20220919110847.744712-4-mattias.forsblad@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: qca8k, mv88e6xxx: rmon: Add RMU support | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 9 of 9 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | warning | WARNING: line length of 91 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
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 >
On 2022-09-20 00:00, Vladimir Oltean wrote: >> 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? > The sequence number is in the chip structure which isn't available here. Should we really access that here in the dsa layer? /Mattias >> + 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 >> >
On Tue, Sep 20, 2022 at 08:41:30AM +0200, Mattias Forsblad wrote: > > 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? > > The sequence number is in the chip structure which isn't available here. > Should we really access that here in the dsa layer? I'm talking about this sequence number: mv88e6xxx_decode_frame2reg_handler: /* Decode Frame2Reg DSA portion */ dsa_header = skb->data - 2; seqno = dsa_header[3]; I'm saying, if you get the seqno in net/dsa/tag_dsa.c and pass it as argument to mv88e6xxx_decode_frame2reg_handler(), then you should no longer have a reason to look at the dsa_header from drivers/net/dsa/mv88e6xxx/.
On 2022-09-20 12:31, Vladimir Oltean wrote: > On Tue, Sep 20, 2022 at 08:41:30AM +0200, Mattias Forsblad wrote: >>> 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? >> >> The sequence number is in the chip structure which isn't available here. >> Should we really access that here in the dsa layer? > > I'm talking about this sequence number: > > mv88e6xxx_decode_frame2reg_handler: > > /* Decode Frame2Reg DSA portion */ > dsa_header = skb->data - 2; > > seqno = dsa_header[3]; > > I'm saying, if you get the seqno in net/dsa/tag_dsa.c and pass it as > argument to mv88e6xxx_decode_frame2reg_handler(), then you should no > longer have a reason to look at the dsa_header from drivers/net/dsa/mv88e6xxx/. Ok, I misunderstood your intent. I'll change. Thanks. /Mattias
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) + 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; + 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, };