| Message ID | 20201125193740.36825-2-george.mccollister@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [net-next,v2,1/3] dsa: add support for Arrow XRS700x tag trailer | expand |
> +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; Something for Vladimir: Could this linearise be moved into the core, depending on the tail_tag? > + if (pskb_trim_rcsum(skb, skb->len - 1)) > + return NULL; And the overhead is also in dsa_devlink_ops, so maybe this can be moved as well? Andrew
On 11/25/2020 11:37 AM, 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: Florian Fainelli <f.fainelli@gmail.com>
On Wed, Nov 25, 2020 at 09:34:29PM +0100, Andrew Lunn wrote: > > +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; > > Something for Vladimir: > > Could this linearise be moved into the core, depending on the > tail_tag? Honestly I believe that the skb_linearize is not needed at all. It is copy-pasted from tag_trailer.c, a driver that has not exercised at runtime by anybody for a long time now. The pskb_trim_rcsum function used for removing the tail tag should do the right thing even with fragmented packets. > > + if (pskb_trim_rcsum(skb, skb->len - 1)) > > + return NULL; > > And the overhead is also in dsa_devlink_ops, so maybe this can be > moved as well? Sorry, I don't understand this comment.
On Thu, Nov 26, 2020 at 03:50:04PM +0200, Vladimir Oltean wrote: > On Wed, Nov 25, 2020 at 09:34:29PM +0100, Andrew Lunn wrote: > > > +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; > > > > Something for Vladimir: > > > > Could this linearise be moved into the core, depending on the > > tail_tag? > > Honestly I believe that the skb_linearize is not needed at all. Humm I'm assuming this is here in case the frame is in fragments, and the trailer could be spread over two fragments? If so, you cannot access the trailer using straight pointers. Linearize should copy it into one buffer. For the normal case of a 1500 byte frame, i doubt we have hardware which uses multiple scatter/gather buffers. But for jumbo frames? > > > + if (pskb_trim_rcsum(skb, skb->len - 1)) > > > + return NULL; > > > > And the overhead is also in dsa_devlink_ops, so maybe this can be > > moved as well? > > Sorry, I don't understand this comment. I'm meaning, could that also be moved into the core? We seem to have the needed information to do it in the core. Andrew
On Thu, Nov 26, 2020 at 03:01:26PM +0100, Andrew Lunn wrote: > On Thu, Nov 26, 2020 at 03:50:04PM +0200, Vladimir Oltean wrote: > > On Wed, Nov 25, 2020 at 09:34:29PM +0100, Andrew Lunn wrote: > > > > +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; > > > > > > Something for Vladimir: > > > > > > Could this linearise be moved into the core, depending on the > > > tail_tag? > > > > Honestly I believe that the skb_linearize is not needed at all. > > Humm > > I'm assuming this is here in case the frame is in fragments, and the > trailer could be spread over two fragments? If so, you cannot access > the trailer using straight pointers. Linearize should copy it into one > buffer. > > For the normal case of a 1500 byte frame, i doubt we have hardware > which uses multiple scatter/gather buffers. But for jumbo frames? In this particular case, I don't think that one byte can be in multiple fragments at once, unless it is a quantum byte. So skb_linearize should still be removed. Are you saying that we should do something like this? I would need to performance-test it: diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c index a1b1dc8a4d87..4c2065bec8d5 100644 --- a/net/dsa/dsa.c +++ b/net/dsa/dsa.c @@ -212,6 +212,13 @@ static int dsa_switch_rcv(struct sk_buff *skb, struct net_device *dev, if (!skb) return 0; + if (cpu_dp->tag_ops->tail_tag && cpu_dp->tag_ops->overhead > 1) { + if (unlikely(skb_linearize(skb))) { + dev_kfree_skb_any(skb); + return 0; + } + } + nskb = cpu_dp->rcv(skb, dev, pt); if (!nskb) { kfree_skb(skb); Also, since we are now finally touching the cacheline with tag_ops, maybe we could remove the copy of rcv() from struct dsa_port? Although there is a chance that this might destabilize things and could need a bit of tweaking to get right. > > > > + if (pskb_trim_rcsum(skb, skb->len - 1)) > > > > + return NULL; > > > > > > And the overhead is also in dsa_devlink_ops, so maybe this can be > > > moved as well? > > > > Sorry, I don't understand this comment. > > I'm meaning, could that also be moved into the core? We seem to have > the needed information to do it in the core. Ok, I got confused by the devlink reference. Currently the tag is always stripped by the tagger driver, because there are 3 cases: - tag is before Ethernet MAC DA - tag is before Ethernet Type - tag is before FCS We do not have a way to distinguish between cases 1 and 2 such that DSA could strip the tag in all cases and provide a uniform interface to all types of taggers.
diff --git a/include/net/dsa.h b/include/net/dsa.h index 2e64e8de4631..eb46ecdcf165 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);
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> --- include/net/dsa.h | 2 ++ net/dsa/Kconfig | 6 +++++ net/dsa/Makefile | 1 + net/dsa/tag_xrs700x.c | 67 +++++++++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 76 insertions(+) create mode 100644 net/dsa/tag_xrs700x.c