Re: [PATCH net-next 2/8] net: dsa: tag_ksz: add tag handling for Microchip LAN937x

On Sat, 2021-01-30 at 04:27 +0200, Vladimir Oltean wrote:

EXTERNAL EMAIL: Do not click links or open attachments unless you
know the content is safe

On Thu, Jan 28, 2021 at 12:11:06PM +0530, Prasanna Vengateshan wrote:

quoted

diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index 4820dbcedfa2..6fac39c2b7d5 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c

@@ -190,10 +190,84 @@ static const struct dsa_device_ops

ksz9893_netdev_ops = {
 DSA_TAG_DRIVER(ksz9893_netdev_ops);
 MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_KSZ9893);

+/* For Ingress (Host -> LAN937x), 2 bytes are added before FCS.
+ * -------------------------------------------------------------
--------------
+ *
DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|tag1(1byte)|FCS
(4bytes)
+ * -------------------------------------------------------------
--------------
+ * tag0 : represents tag override, lookup and valid
+ * tag1 : each bit represents port (eg, 0x01=port1, 0x02=port2,
0x80=port8)
+ *
+ * For Egress (LAN937x -> Host), 1 byte is added before FCS.
+ * -------------------------------------------------------------
--------------
+ * DA(6bytes)|SA(6bytes)|....|Data(nbytes)|tag0(1byte)|FCS(4bytes)
+ * -------------------------------------------------------------
--------------
+ * tag0 : zero-based value represents port
+ *     (eg, 0x00=port1, 0x02=port3, 0x07=port8)
+ */

You messed up the comment, right now it's as good as not having it.
The one-hot port encoding is for xmit. The zero-based encoding is for
rcv, not the other way around.

I understand the problem is with the term Egress & Ingress w.r.to
LAN937x. I will make sure that the comment is added w.r.to xmit() &
rcv().

quoted

+#define LAN937X_INGRESS_TAG_LEN              2
+
+#define LAN937X_TAIL_TAG_OVERRIDE    BIT(11)
+#define LAN937X_TAIL_TAG_LOOKUP              BIT(12)
+#define LAN937X_TAIL_TAG_VALID               BIT(13)
+#define LAN937X_TAIL_TAG_PORT_MASK   7
+
+static struct sk_buff *lan937x_xmit(struct sk_buff *skb,
+                                 struct net_device *dev)
+{
+     struct dsa_port *dp = dsa_slave_to_port(dev);
+     __be16 *tag;
+     u8 *addr;
+     u16 val;
+
+     /* Tag encoding */

Do we really need this comment and the one with "Tag decoding" from
rcv?

Okay, will correct it.

quoted

+     tag = skb_put(skb, LAN937X_INGRESS_TAG_LEN);
+     addr = skb_mac_header(skb);
+
+     val = BIT(dp->index);
+
+     if (is_link_local_ether_addr(addr))
+             val |= LAN937X_TAIL_TAG_OVERRIDE;
+
+     /* Tail tag valid bit - This bit should always be set by the
CPU*/
+     val |= LAN937X_TAIL_TAG_VALID;
+
+     *tag = cpu_to_be16(val);
+
+     return skb;
+}
+
+static struct sk_buff *lan937x_rcv(struct sk_buff *skb, struct
net_device *dev,
+                                struct packet_type *pt)

You can reuse ksz9477_rcv.

Sure, will reuse ksz9477_rcv. 

quoted

+{
+     /* Tag decoding */
+     u8 *tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
+     unsigned int port = tag[0] & LAN937X_TAIL_TAG_PORT_MASK;
+     unsigned int len = KSZ_EGRESS_TAG_LEN;
+
+     /* Extra 4-bytes PTP timestamp */
+     if (tag[0] & KSZ9477_PTP_TAG_INDICATION)
+             len += KSZ9477_PTP_TAG_LEN;
+
+     return ksz_common_rcv(skb, dev, port, len);
+}
+
+static const struct dsa_device_ops lan937x_netdev_ops = {
+     .name   = "lan937x",
+     .proto  = DSA_TAG_PROTO_LAN937X,
+     .xmit   = lan937x_xmit,
+     .rcv    = lan937x_rcv,
+     .overhead = LAN937X_INGRESS_TAG_LEN,
+     .tail_tag = true,
+};
+
+DSA_TAG_DRIVER(lan937x_netdev_ops);
+MODULE_ALIAS_DSA_TAG_DRIVER(DSA_TAG_PROTO_LAN937X);
+
 static struct dsa_tag_driver *dsa_tag_driver_array[] = {
      &DSA_TAG_DRIVER_NAME(ksz8795_netdev_ops),
      &DSA_TAG_DRIVER_NAME(ksz9477_netdev_ops),
      &DSA_TAG_DRIVER_NAME(ksz9893_netdev_ops),
+     &DSA_TAG_DRIVER_NAME(lan937x_netdev_ops),
 };

 module_dsa_tag_drivers(dsa_tag_driver_array);
--
2.25.1