| Message ID | 20210113145922.92848-2-george.mccollister@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Arrow SpeedChips XRS700x DSA Driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | success | Link |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 1 maintainers not CCed: kuba@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 20 this patch: 20 |
| netdev/kdoc | success | Errors and warnings before: 1 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | warning | WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? WARNING: please write a paragraph that describes the config symbol fully |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 20 this patch: 20 |
| netdev/header_inline | success | Link |
| netdev/stable | success | Stable not CCed |
On Wed, Jan 13, 2021 at 08:59:20AM -0600, George McCollister wrote: > Add support for Arrow SpeedChips XRS700x single byte tag trailer. This > is modeled on tag_trailer.c which works in a similar way. > > Signed-off-by: George McCollister <george.mccollister@gmail.com> > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > --- Reviewed-by: Vladimir Oltean <olteanv@gmail.com> A few comments below. > diff --git a/net/dsa/tag_xrs700x.c b/net/dsa/tag_xrs700x.c > new file mode 100644 > index 000000000000..4ee7c260a8a9 > --- /dev/null > +++ b/net/dsa/tag_xrs700x.c > @@ -0,0 +1,67 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* > + * XRS700x tag format handling > + * Copyright (c) 2008-2009 Marvell Semiconductor Why does Marvell get copyright? > + * Copyright (c) 2020 NovaTech LLC > + */ > + > +#include <linux/etherdevice.h> > +#include <linux/list.h> > +#include <linux/slab.h> These 3 includes are not needed. You can probably remove them later though, if there is no other reason to resend. > +#include <linux/bitops.h> > + > +#include "dsa_priv.h" > + > +static struct sk_buff *xrs700x_xmit(struct sk_buff *skb, struct net_device *dev) > +{ > + struct dsa_port *dp = dsa_slave_to_port(dev); > + u8 *trailer; > + > + trailer = skb_put(skb, 1); > + trailer[0] = BIT(dp->index); > + > + return skb; > +} > + > +static struct sk_buff *xrs700x_rcv(struct sk_buff *skb, struct net_device *dev, > + struct packet_type *pt) > +{ > + int source_port; > + u8 *trailer; > + > + if (skb_linearize(skb)) > + return NULL; We've been through this, there should be no reason to linearize an skb for a one-byte tail tag.. > + > + trailer = skb_tail_pointer(skb) - 1; > + > + source_port = ffs((int)trailer[0]) - 1; > + > + if (source_port < 0) > + return NULL; > + > + skb->dev = dsa_master_find_slave(dev, 0, source_port); > + if (!skb->dev) > + return NULL; > + > + if (pskb_trim_rcsum(skb, skb->len - 1)) > + return NULL; > + > + /* Frame is forwarded by hardware, don't forward in software. */ > + skb->offload_fwd_mark = 1; > + > + return skb; > +}
On Thu, Jan 14, 2021 at 03:05:19AM +0200, Vladimir Oltean wrote: > On Wed, Jan 13, 2021 at 08:59:20AM -0600, George McCollister wrote: > > Add support for Arrow SpeedChips XRS700x single byte tag trailer. This > > is modeled on tag_trailer.c which works in a similar way. > > > > Signed-off-by: George McCollister <george.mccollister@gmail.com> > > Reviewed-by: Andrew Lunn <andrew@lunn.ch> > > Reviewed-by: Florian Fainelli <f.fainelli@gmail.com> > > --- > > Reviewed-by: Vladimir Oltean <olteanv@gmail.com> > > A few comments below. > > > diff --git a/net/dsa/tag_xrs700x.c b/net/dsa/tag_xrs700x.c > > new file mode 100644 > > index 000000000000..4ee7c260a8a9 > > --- /dev/null > > +++ b/net/dsa/tag_xrs700x.c > > @@ -0,0 +1,67 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * XRS700x tag format handling > > + * Copyright (c) 2008-2009 Marvell Semiconductor > > Why does Marvell get copyright? Probably because it started life as tag_trailer.c? Andrew
On Wed, Jan 13, 2021 at 7:05 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > +++ b/net/dsa/tag_xrs700x.c > > @@ -0,0 +1,67 @@ > > +// SPDX-License-Identifier: GPL-2.0+ > > +/* > > + * XRS700x tag format handling > > + * Copyright (c) 2008-2009 Marvell Semiconductor > > Why does Marvell get copyright? What Andrew said. I started with tag_trailer.c since it is quite similar and it seemed wrong to remove the copyright. > > > + * Copyright (c) 2020 NovaTech LLC > > + */ > > + > > +#include <linux/etherdevice.h> > > +#include <linux/list.h> > > +#include <linux/slab.h> > > These 3 includes are not needed. You can probably remove them later > though, if there is no other reason to resend. Removed. > > > +#include <linux/bitops.h> > > + > > +#include "dsa_priv.h" > > + > > +static struct sk_buff *xrs700x_xmit(struct sk_buff *skb, struct net_device *dev) > > +{ > > + struct dsa_port *dp = dsa_slave_to_port(dev); > > + u8 *trailer; > > + > > + trailer = skb_put(skb, 1); > > + trailer[0] = BIT(dp->index); > > + > > + return skb; > > +} > > + > > +static struct sk_buff *xrs700x_rcv(struct sk_buff *skb, struct net_device *dev, > > + struct packet_type *pt) > > +{ > > + int source_port; > > + u8 *trailer; > > + > > + if (skb_linearize(skb)) > > + return NULL; > > We've been through this, there should be no reason to linearize an skb > for a one-byte tail tag.. Sorry about this. You and Andrew started discussing it and I guess I got distracted fixing the other issues. Removed. I'll retest after making other changes to patches in the series but based on what you've said it should be fine without it. > > > + > > + trailer = skb_tail_pointer(skb) - 1; > > + > > + source_port = ffs((int)trailer[0]) - 1; > > + > > + if (source_port < 0) > > + return NULL; > > + > > + skb->dev = dsa_master_find_slave(dev, 0, source_port); > > + if (!skb->dev) > > + return NULL; > > + > > + if (pskb_trim_rcsum(skb, skb->len - 1)) > > + return NULL; > > + > > + /* Frame is forwarded by hardware, don't forward in software. */ > > + skb->offload_fwd_mark = 1; > > + > > + return skb; > > +}
diff --git a/include/net/dsa.h b/include/net/dsa.h index c3485ba6c312..74b5bf835657 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -46,6 +46,7 @@ struct phylink_link_state; #define DSA_TAG_PROTO_AR9331_VALUE 16 #define DSA_TAG_PROTO_RTL4_A_VALUE 17 #define DSA_TAG_PROTO_HELLCREEK_VALUE 18 +#define DSA_TAG_PROTO_XRS700X_VALUE 19 enum dsa_tag_protocol { DSA_TAG_PROTO_NONE = DSA_TAG_PROTO_NONE_VALUE, @@ -67,6 +68,7 @@ enum dsa_tag_protocol { DSA_TAG_PROTO_AR9331 = DSA_TAG_PROTO_AR9331_VALUE, DSA_TAG_PROTO_RTL4_A = DSA_TAG_PROTO_RTL4_A_VALUE, DSA_TAG_PROTO_HELLCREEK = DSA_TAG_PROTO_HELLCREEK_VALUE, + DSA_TAG_PROTO_XRS700X = DSA_TAG_PROTO_XRS700X_VALUE, }; struct packet_type; diff --git a/net/dsa/Kconfig b/net/dsa/Kconfig index dfecd7b22fd7..2d226a5c085f 100644 --- a/net/dsa/Kconfig +++ b/net/dsa/Kconfig @@ -139,4 +139,10 @@ config NET_DSA_TAG_TRAILER Say Y or M if you want to enable support for tagging frames at with a trailed. e.g. Marvell 88E6060. +config NET_DSA_TAG_XRS700X + tristate "Tag driver for XRS700x switches" + help + Say Y or M if you want to enable support for tagging frames for + Arrow SpeedChips XRS700x switches that use a single byte tag trailer. + endif diff --git a/net/dsa/Makefile b/net/dsa/Makefile index 0fb2b75a7ae3..92cea2132241 100644 --- a/net/dsa/Makefile +++ b/net/dsa/Makefile @@ -18,3 +18,4 @@ obj-$(CONFIG_NET_DSA_TAG_OCELOT) += tag_ocelot.o obj-$(CONFIG_NET_DSA_TAG_QCA) += tag_qca.o obj-$(CONFIG_NET_DSA_TAG_SJA1105) += tag_sja1105.o obj-$(CONFIG_NET_DSA_TAG_TRAILER) += tag_trailer.o +obj-$(CONFIG_NET_DSA_TAG_XRS700X) += tag_xrs700x.o diff --git a/net/dsa/tag_xrs700x.c b/net/dsa/tag_xrs700x.c new file mode 100644 index 000000000000..4ee7c260a8a9 --- /dev/null +++ b/net/dsa/tag_xrs700x.c @@ -0,0 +1,67 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * XRS700x tag format handling + * Copyright (c) 2008-2009 Marvell Semiconductor + * Copyright (c) 2020 NovaTech LLC + */ + +#include <linux/etherdevice.h> +#include <linux/list.h> +#include <linux/slab.h> +#include <linux/bitops.h> + +#include "dsa_priv.h" + +static struct sk_buff *xrs700x_xmit(struct sk_buff *skb, struct net_device *dev) +{ + struct dsa_port *dp = dsa_slave_to_port(dev); + u8 *trailer; + + trailer = skb_put(skb, 1); + trailer[0] = BIT(dp->index); + + return skb; +} + +static struct sk_buff *xrs700x_rcv(struct sk_buff *skb, struct net_device *dev, + struct packet_type *pt) +{ + int source_port; + u8 *trailer; + + if (skb_linearize(skb)) + return NULL; + + trailer = skb_tail_pointer(skb) - 1; + + source_port = ffs((int)trailer[0]) - 1; + + if (source_port < 0) + return NULL; + + skb->dev = dsa_master_find_slave(dev, 0, source_port); + if (!skb->dev) + return NULL; + + if (pskb_trim_rcsum(skb, skb->len - 1)) + return NULL; + + /* Frame is forwarded by hardware, don't forward in software. */ + skb->offload_fwd_mark = 1; + + return skb; +} + +static const struct dsa_device_ops xrs700x_netdev_ops = { + .name = "xrs700x", + .proto = DSA_TAG_PROTO_XRS700X, + .xmit = xrs700x_xmit, + .rcv = xrs700x_rcv, + .overhead = 1, + .tail_tag = true, +}; + +MODULE_LICENSE("GPL"); +MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_XRS700X); + +module_dsa_tag_driver(xrs700x_netdev_ops);