Re: [PATCH net-next v4 1/3] dsa: add support for Arrow XRS700x tag trailer

On Wed, Jan 13, 2021 at 7:05 PM Vladimir Oltean [off-list ref] wrote:

quoted

+++ 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.

quoted

+ * 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.

quoted

+#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.

quoted

+
+     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;
+}