[net-next RFC PATCH v2 0/8] Add support for qca8k mdio rw in Ethernet packet

[net-next RFC PATCH v2 0/8] Add support for qca8k mdio rw in Ethernet packet

[Ansuel Smith]

Hi, this is still WIP and currently has some problem but I would love if
someone can give this a superficial review and answer to some problem
with this.

The main reason for this is that we notice some routing problem in the
switch and it seems assisted learning is needed. Considering mdio is
quite slow due to the indirect write using this Ethernet alternative way
seems to be quicker.

The qca8k switch supports a special way to pass mdio read/write request
using specially crafted Ethernet packet.
This works by putting some defined data in the Ethernet header where the
mac source and dst should be placed. The Ethernet type header is set to qca
header and is set to a mdio read/write type.
This is used to communicate to the switch that this is a special packet
and should be parsed differently.

Current implementation of this use completion API to wait for the packet
to be processed by the tagger and has a timeout that fallback to the
legacy mdio way and mutex to enforce one transaction at time.

Here I list the main concern I have about this:
- Is the changes done to the tagger acceptable? (moving stuff to global
  include)
- Is it correct to put the skb generation code in the qca8k source?
- Is the changes generally correct? (referring to how this is
  implemented with part of the implementation split between the tagger
  and the driver)

I still have to find a solution to a slowdown problem and this is where
I would love to get some hint.
Currently I still didn't find a good way to understand when the tagger
starts to accept packets and because of this the initial setup is slow
as every completion timeouts. Am I missing something or is there a way
to check for this?
After the initial slowdown, as soon as the cpu port is ready and starts
to accept packet, every transaction is near instant and no completion
timeouts.

As I said this is still WIP but it does work correctly aside from the
initial slowdown problem. (the slowdown is in the first port init and at
the first port init... from port 2 the tagger starts to accept packet
and this starts to work)

Additional changes to the original implementation:

We now have connect()/disconnect() ops for the tagger. They are used to
allocate priv data in the dsa priv. The header still has to be put in
global include to make it usable by a dsa driver.
They are called when the tag is connect to the dst and the data is freed
using discconect on tagger change.

(if someone wonder why the bind function is put at in the general setup
function it's because tag is set in the cpu port where the notifier is
still not available and we require the notifier to sen the
tag_proto_connect() event.

We now have a tag_proto_connect() for the dsa driver used to put
additional data in the tagger priv (that is actually the dsa priv).
This is called using a switch event DSA_NOTIFIER_TAG_PROTO_CONNECT.
Current use for this is adding handler for the Ethernet packet to keep
the tagger code as dumb as possible.

From what I read in the old series we probably need to drop the priv and
move to a more specific use to prevent any abuse... (or actually just
add an additional priv just for the tagger to prevent any breakage by
removing priv from dsa_port)

I still didn't investigate the slowdown problem that is still present in
some part when the port are actually init.

Hope Andrew is not too angry about this implementation but it seems
flexible and not that bad.

(also in the current code I assume a tagger is always present. This
should be the case or a check if the tagger is not present is needed?)

Also still have to work on the autocast handler but it's really a
function to add with the current implementation. Tagger is already have
support to handle them.

v2:
- Address all suggestion from Vladimir.
  Try to generilize this with connect/disconnect function from the
  tagger and tag_proto_connect for the driver.

Ansuel Smith (8):
  net: das: Introduce support for tagger private data control
  net: dsa: Permit dsa driver to configure additional tagger data
  net: dsa: tag_qca: convert to FIELD macro
  net: dsa: tag_qca: move define to include linux/dsa
  net: dsa: tag_qca: add define for mdio read/write in ethernet packet
  net: dsa: tag_qca: Add support for handling Ethernet mdio and MIB
    packet
  net: dsa: qca8k: Add support for mdio read/write in Ethernet packet
  net: dsa: qca8k: cache lo and hi for mdio write

 drivers/net/dsa/qca8k.c     | 263 +++++++++++++++++++++++++++++++++++-
 drivers/net/dsa/qca8k.h     |   4 +
 include/linux/dsa/tag_qca.h |  79 +++++++++++
 include/net/dsa.h           |  11 ++
 net/dsa/dsa2.c              |  37 +++++
 net/dsa/dsa_priv.h          |   1 +
 net/dsa/switch.c            |  14 ++
 net/dsa/tag_qca.c           |  90 ++++++++----
 8 files changed, 464 insertions(+), 35 deletions(-)
 create mode 100644 include/linux/dsa/tag_qca.h

-- 
2.32.0

[net-next RFC PATCH v2 1/8] net: das: Introduce support for tagger private data control

[Ansuel Smith]

Introduce 2 new function for the tagger ops to permit the tagger to
allocate private data. This is useful for case where the tagger receive
some data that should be by the switch driver or require some special
handling for some special packet (example Ethernet mdio packet)

The tagger will use the dsa port priv to store his priv data.

connect() is used to allocate the private data. It's the tagger choice
how to allocate the data to the different ports.
disconnect() will free the priv data in the dsa port.

On switch setup the connect() ops is called.
On tagger change the disconnect() is called, the tagger will free the
priv data in dsa port and a connect() is called to allocate the new priv
data (if the new tagger requires it)

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 include/net/dsa.h |  3 +++
 net/dsa/dsa2.c    | 26 ++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 8ca9d50cbbc2..33391d74be5c 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -82,8 +82,11 @@ enum dsa_tag_protocol {
 };
 
 struct dsa_switch;
+struct dsa_switch_tree;
 
 struct dsa_device_ops {
+	int (*connect)(struct dsa_switch_tree *dst);
+	void (*disconnect)(struct dsa_switch_tree *dst);
 	struct sk_buff *(*xmit)(struct sk_buff *skb, struct net_device *dev);
 	struct sk_buff *(*rcv)(struct sk_buff *skb, struct net_device *dev);
 	void (*flow_dissect)(const struct sk_buff *skb, __be16 *proto,
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 826957b6442b..15566c5ae8ae 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1043,6 +1043,20 @@ static void dsa_tree_teardown_lags(struct dsa_switch_tree *dst)
 	kfree(dst->lags);
 }
 
+static int dsa_tree_bind_tag_proto(struct dsa_switch_tree *dst)
+{
+	const struct dsa_device_ops *tag_ops = dst->tag_ops;
+	int err;
+
+	if (tag_ops->connect) {
+		err = tag_ops->connect(dst);
+		if (err)
+			return err;
+	}
+
+	return 0;
+}
+
 static int dsa_tree_setup(struct dsa_switch_tree *dst)
 {
 	bool complete;
@@ -1066,6 +1080,10 @@ static int dsa_tree_setup(struct dsa_switch_tree *dst)
 	if (err)
 		goto teardown_cpu_ports;
 
+	err = dsa_tree_bind_tag_proto(dst);
+	if (err)
+		goto teardown_switches;
+
 	err = dsa_tree_setup_master(dst);
 	if (err)
 		goto teardown_switches;
@@ -1155,13 +1173,21 @@ int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
 	if (err)
 		goto out_unwind_tagger;
 
+	if (dst->tag_ops->disconnect)
+		dst->tag_ops->disconnect(dst);
+
 	dst->tag_ops = tag_ops;
 
+	err = dsa_tree_bind_tag_proto(dst);
+	if (err)
+		goto out_unwind_tagger;
+
 	rtnl_unlock();
 
 	return 0;
 
 out_unwind_tagger:
+	dst->tag_ops = old_tag_ops;
 	info.tag_ops = old_tag_ops;
 	dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO, &info);
 out_unlock:
-- 
2.32.0

[net-next RFC PATCH v2 2/8] net: dsa: Permit dsa driver to configure additional tagger data

[Ansuel Smith]

Permit a dsa driver to configure additional tagger data for the current
active tagger. A new ops is introduced tag_proto_connect() that will be
called on every tagger bind event using the
DSA_NOTIFIER_TAG_PROTO_CONNECT event.

This is used if the driver require to set additional driver or some
handler that the tagger should use to handle special packet.

The dsa driver require to provide explicit support for the current
tagger and to understand the current private data set in the dsa ports.

tag_proto_connect() should parse the tagger proto, check if it does
support it and do the required task to each ports.

An example of this is a dsa driver that supports Ethernet mdio and
require to provide to the tagger a handler function to parse these
packets.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 include/net/dsa.h  |  8 ++++++++
 net/dsa/dsa2.c     | 11 +++++++++++
 net/dsa/dsa_priv.h |  1 +
 net/dsa/switch.c   | 14 ++++++++++++++
 4 files changed, 34 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 33391d74be5c..3af8720e0caf 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -946,6 +946,14 @@ struct dsa_switch_ops {
 	int	(*tag_8021q_vlan_add)(struct dsa_switch *ds, int port, u16 vid,
 				      u16 flags);
 	int	(*tag_8021q_vlan_del)(struct dsa_switch *ds, int port, u16 vid);
+
+	/*
+	 * Tagger connect operations. Use this to set special data/handler
+	 * for the current tagger set. The function require to provide explicit
+	 * support for the current tagger.
+	 */
+	int	(*tag_proto_connect)(struct dsa_switch *ds,
+				     const struct dsa_device_ops *tag_ops);
 };
 
 #define DSA_DEVLINK_PARAM_DRIVER(_id, _name, _type, _cmodes)		\
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 15566c5ae8ae..15d6c52dbf53 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1046,6 +1046,7 @@ static void dsa_tree_teardown_lags(struct dsa_switch_tree *dst)
 static int dsa_tree_bind_tag_proto(struct dsa_switch_tree *dst)
 {
 	const struct dsa_device_ops *tag_ops = dst->tag_ops;
+	struct dsa_notifier_tag_proto_info info;
 	int err;
 
 	if (tag_ops->connect) {
@@ -1054,6 +1055,16 @@ static int dsa_tree_bind_tag_proto(struct dsa_switch_tree *dst)
 			return err;
 	}
 
+	info.tag_ops = tag_ops;
+	err = dsa_tree_notify(dst, DSA_NOTIFIER_TAG_PROTO_CONNECT, &info);
+	if (err && err != -EOPNOTSUPP)
+		goto out_disconnect;
+
+	return 0;
+out_disconnect:
+	if (tag_ops->disconnect)
+		tag_ops->disconnect(dst);
+
 	return 0;
 }
 
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 3fb2c37c9b88..e69843c4aa6d 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -37,6 +37,7 @@ enum {
 	DSA_NOTIFIER_VLAN_DEL,
 	DSA_NOTIFIER_MTU,
 	DSA_NOTIFIER_TAG_PROTO,
+	DSA_NOTIFIER_TAG_PROTO_CONNECT,
 	DSA_NOTIFIER_MRP_ADD,
 	DSA_NOTIFIER_MRP_DEL,
 	DSA_NOTIFIER_MRP_ADD_RING_ROLE,
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index bb155a16d454..4b7434d709fb 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -647,6 +647,17 @@ static int dsa_switch_change_tag_proto(struct dsa_switch *ds,
 	return 0;
 }
 
+static int dsa_switch_connect_tag_proto(struct dsa_switch *ds,
+					struct dsa_notifier_tag_proto_info *info)
+{
+	const struct dsa_device_ops *tag_ops = info->tag_ops;
+
+	if (!ds->ops->tag_proto_connect)
+		return -EOPNOTSUPP;
+
+	return ds->ops->tag_proto_connect(ds, tag_ops);
+}
+
 static int dsa_switch_mrp_add(struct dsa_switch *ds,
 			      struct dsa_notifier_mrp_info *info)
 {
@@ -766,6 +777,9 @@ static int dsa_switch_event(struct notifier_block *nb,
 	case DSA_NOTIFIER_TAG_PROTO:
 		err = dsa_switch_change_tag_proto(ds, info);
 		break;
+	case DSA_NOTIFIER_TAG_PROTO_CONNECT:
+		err = dsa_switch_connect_tag_proto(ds, info);
+		break;
 	case DSA_NOTIFIER_MRP_ADD:
 		err = dsa_switch_mrp_add(ds, info);
 		break;
-- 
2.32.0

[net-next RFC PATCH v2 3/8] net: dsa: tag_qca: convert to FIELD macro

[Ansuel Smith]

Convert driver to FIELD macro to drop redundant define.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 net/dsa/tag_qca.c | 34 +++++++++++++++-------------------
 1 file changed, 15 insertions(+), 19 deletions(-)

diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index 1ea9401b8ace..55fa6b96b4eb 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -4,29 +4,24 @@
  */
 
 #include <linux/etherdevice.h>
+#include <linux/bitfield.h>
 
 #include "dsa_priv.h"
 
 #define QCA_HDR_LEN	2
 #define QCA_HDR_VERSION	0x2
 
-#define QCA_HDR_RECV_VERSION_MASK	GENMASK(15, 14)
-#define QCA_HDR_RECV_VERSION_S		14
-#define QCA_HDR_RECV_PRIORITY_MASK	GENMASK(13, 11)
-#define QCA_HDR_RECV_PRIORITY_S		11
-#define QCA_HDR_RECV_TYPE_MASK		GENMASK(10, 6)
-#define QCA_HDR_RECV_TYPE_S		6
+#define QCA_HDR_RECV_VERSION		GENMASK(15, 14)
+#define QCA_HDR_RECV_PRIORITY		GENMASK(13, 11)
+#define QCA_HDR_RECV_TYPE		GENMASK(10, 6)
 #define QCA_HDR_RECV_FRAME_IS_TAGGED	BIT(3)
-#define QCA_HDR_RECV_SOURCE_PORT_MASK	GENMASK(2, 0)
-
-#define QCA_HDR_XMIT_VERSION_MASK	GENMASK(15, 14)
-#define QCA_HDR_XMIT_VERSION_S		14
-#define QCA_HDR_XMIT_PRIORITY_MASK	GENMASK(13, 11)
-#define QCA_HDR_XMIT_PRIORITY_S		11
-#define QCA_HDR_XMIT_CONTROL_MASK	GENMASK(10, 8)
-#define QCA_HDR_XMIT_CONTROL_S		8
+#define QCA_HDR_RECV_SOURCE_PORT	GENMASK(2, 0)
+
+#define QCA_HDR_XMIT_VERSION		GENMASK(15, 14)
+#define QCA_HDR_XMIT_PRIORITY		GENMASK(13, 11)
+#define QCA_HDR_XMIT_CONTROL		GENMASK(10, 8)
 #define QCA_HDR_XMIT_FROM_CPU		BIT(7)
-#define QCA_HDR_XMIT_DP_BIT_MASK	GENMASK(6, 0)
+#define QCA_HDR_XMIT_DP_BIT		GENMASK(6, 0)
 
 static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
 {
@@ -40,8 +35,9 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
 	phdr = dsa_etype_header_pos_tx(skb);
 
 	/* Set the version field, and set destination port information */
-	hdr = QCA_HDR_VERSION << QCA_HDR_XMIT_VERSION_S |
-		QCA_HDR_XMIT_FROM_CPU | BIT(dp->index);
+	hdr = FIELD_PREP(QCA_HDR_XMIT_VERSION, QCA_HDR_VERSION);
+	hdr |= QCA_HDR_XMIT_FROM_CPU;
+	hdr |= FIELD_PREP(QCA_HDR_XMIT_DP_BIT, BIT(dp->index));
 
 	*phdr = htons(hdr);
 
@@ -62,7 +58,7 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 	hdr = ntohs(*phdr);
 
 	/* Make sure the version is correct */
-	ver = (hdr & QCA_HDR_RECV_VERSION_MASK) >> QCA_HDR_RECV_VERSION_S;
+	ver = FIELD_GET(QCA_HDR_RECV_VERSION, hdr);
 	if (unlikely(ver != QCA_HDR_VERSION))
 		return NULL;
 
@@ -71,7 +67,7 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 	dsa_strip_etype_header(skb, QCA_HDR_LEN);
 
 	/* Get source port information */
-	port = (hdr & QCA_HDR_RECV_SOURCE_PORT_MASK);
+	port = FIELD_GET(QCA_HDR_RECV_SOURCE_PORT, hdr);
 
 	skb->dev = dsa_master_find_slave(dev, 0, port);
 	if (!skb->dev)
-- 
2.32.0

[net-next RFC PATCH v2 4/8] net: dsa: tag_qca: move define to include linux/dsa

[Ansuel Smith]

Move tag_qca define to include dir linux/dsa as the qca8k require access
to the tagger define to support in-band mdio read/write using ethernet
packet.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 include/linux/dsa/tag_qca.h | 21 +++++++++++++++++++++
 net/dsa/tag_qca.c           | 16 +---------------
 2 files changed, 22 insertions(+), 15 deletions(-)
 create mode 100644 include/linux/dsa/tag_qca.h

diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
new file mode 100644
index 000000000000..c02d2d39ff4a
--- /dev/null
+++ b/include/linux/dsa/tag_qca.h
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __TAG_QCA_H
+#define __TAG_QCA_H
+
+#define QCA_HDR_LEN	2
+#define QCA_HDR_VERSION	0x2
+
+#define QCA_HDR_RECV_VERSION		GENMASK(15, 14)
+#define QCA_HDR_RECV_PRIORITY		GENMASK(13, 11)
+#define QCA_HDR_RECV_TYPE		GENMASK(10, 6)
+#define QCA_HDR_RECV_FRAME_IS_TAGGED	BIT(3)
+#define QCA_HDR_RECV_SOURCE_PORT	GENMASK(2, 0)
+
+#define QCA_HDR_XMIT_VERSION		GENMASK(15, 14)
+#define QCA_HDR_XMIT_PRIORITY		GENMASK(13, 11)
+#define QCA_HDR_XMIT_CONTROL		GENMASK(10, 8)
+#define QCA_HDR_XMIT_FROM_CPU		BIT(7)
+#define QCA_HDR_XMIT_DP_BIT		GENMASK(6, 0)
+
+#endif /* __TAG_QCA_H */
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index 55fa6b96b4eb..34e565e00ece 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -5,24 +5,10 @@
 
 #include <linux/etherdevice.h>
 #include <linux/bitfield.h>
+#include <linux/dsa/tag_qca.h>
 
 #include "dsa_priv.h"
 
-#define QCA_HDR_LEN	2
-#define QCA_HDR_VERSION	0x2
-
-#define QCA_HDR_RECV_VERSION		GENMASK(15, 14)
-#define QCA_HDR_RECV_PRIORITY		GENMASK(13, 11)
-#define QCA_HDR_RECV_TYPE		GENMASK(10, 6)
-#define QCA_HDR_RECV_FRAME_IS_TAGGED	BIT(3)
-#define QCA_HDR_RECV_SOURCE_PORT	GENMASK(2, 0)
-
-#define QCA_HDR_XMIT_VERSION		GENMASK(15, 14)
-#define QCA_HDR_XMIT_PRIORITY		GENMASK(13, 11)
-#define QCA_HDR_XMIT_CONTROL		GENMASK(10, 8)
-#define QCA_HDR_XMIT_FROM_CPU		BIT(7)
-#define QCA_HDR_XMIT_DP_BIT		GENMASK(6, 0)
-
 static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct dsa_port *dp = dsa_slave_to_port(dev);
-- 
2.32.0

[net-next RFC PATCH v2 5/8] net: dsa: tag_qca: add define for mdio read/write in ethernet packet

[Ansuel Smith]

Add all the required define to prepare support for mdio read/write in
Ethernet packet. Any packet of this type has to be dropped as the only
use of these special packet is receive ack for an mdio write request or
receive data for an mdio read request.
A struct is used that emulates the Ethernet header but is used for a
different purpose.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 include/linux/dsa/tag_qca.h | 41 +++++++++++++++++++++++++++++++++++++
 net/dsa/tag_qca.c           | 13 +++++++++---
 2 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
index c02d2d39ff4a..578a4aeafd92 100644
--- a/include/linux/dsa/tag_qca.h
+++ b/include/linux/dsa/tag_qca.h
@@ -12,10 +12,51 @@
 #define QCA_HDR_RECV_FRAME_IS_TAGGED	BIT(3)
 #define QCA_HDR_RECV_SOURCE_PORT	GENMASK(2, 0)
 
+/* Packet type for recv */
+#define QCA_HDR_RECV_TYPE_NORMAL	0x0
+#define QCA_HDR_RECV_TYPE_MIB		0x1
+#define QCA_HDR_RECV_TYPE_RW_REG_ACK	0x2
+
 #define QCA_HDR_XMIT_VERSION		GENMASK(15, 14)
 #define QCA_HDR_XMIT_PRIORITY		GENMASK(13, 11)
 #define QCA_HDR_XMIT_CONTROL		GENMASK(10, 8)
 #define QCA_HDR_XMIT_FROM_CPU		BIT(7)
 #define QCA_HDR_XMIT_DP_BIT		GENMASK(6, 0)
 
+/* Packet type for xmit */
+#define QCA_HDR_XMIT_TYPE_NORMAL	0x0
+#define QCA_HDR_XMIT_TYPE_RW_REG	0x1
+
+#define MDIO_CHECK_CODE_VAL		0x5
+
+/* Specific define for in-band MDIO read/write with Ethernet packet */
+#define QCA_HDR_MDIO_SEQ_LEN		4 /* 4 byte for the seq */
+#define QCA_HDR_MDIO_COMMAND_LEN	4 /* 4 byte for the command */
+#define QCA_HDR_MDIO_DATA1_LEN		4 /* First 4 byte for the mdio data */
+#define QCA_HDR_MDIO_HEADER_LEN		(QCA_HDR_MDIO_SEQ_LEN + \
+					QCA_HDR_MDIO_COMMAND_LEN + \
+					QCA_HDR_MDIO_DATA1_LEN)
+
+#define QCA_HDR_MDIO_DATA2_LEN		12 /* Other 12 byte for the mdio data */
+#define QCA_HDR_MDIO_PADDING_LEN	34 /* Padding to reach the min Ethernet packet */
+
+#define QCA_HDR_MDIO_PKG_LEN		(QCA_HDR_MDIO_HEADER_LEN + \
+					QCA_HDR_LEN + \
+					QCA_HDR_MDIO_DATA2_LEN + \
+					QCA_HDR_MDIO_PADDING_LEN)
+
+#define QCA_HDR_MDIO_SEQ_NUM		GENMASK(31, 0)  /* 63, 32 */
+#define QCA_HDR_MDIO_CHECK_CODE		GENMASK(31, 29) /* 31, 29 */
+#define QCA_HDR_MDIO_CMD		BIT(28)		/* 28 */
+#define QCA_HDR_MDIO_LENGTH		GENMASK(23, 20) /* 23, 20 */
+#define QCA_HDR_MDIO_ADDR		GENMASK(18, 0)  /* 18, 0 */
+
+/* Special struct emulating a Ethernet header */
+struct mdio_ethhdr {
+	u32 command;		/* command bit 31:0 */
+	u32 seq;		/* seq 63:32 */
+	u32 mdio_data;		/* first 4byte mdio */
+	u16 hdr;		/* qca hdr */
+} __packed;
+
 #endif /* __TAG_QCA_H */
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index 34e565e00ece..b8b05d54a74c 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -32,10 +32,10 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
 
 static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 {
-	u8 ver;
-	u16  hdr;
-	int port;
+	u16  hdr, pk_type;
 	__be16 *phdr;
+	int port;
+	u8 ver;
 
 	if (unlikely(!pskb_may_pull(skb, QCA_HDR_LEN)))
 		return NULL;
@@ -48,6 +48,13 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 	if (unlikely(ver != QCA_HDR_VERSION))
 		return NULL;
 
+	/* Get pk type */
+	pk_type = FIELD_GET(QCA_HDR_RECV_TYPE, hdr);
+
+	/* MDIO read/write packet */
+	if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK)
+		return NULL;
+
 	/* Remove QCA tag and recalculate checksum */
 	skb_pull_rcsum(skb, QCA_HDR_LEN);
 	dsa_strip_etype_header(skb, QCA_HDR_LEN);
-- 
2.32.0

[net-next RFC PATCH v2 6/8] net: dsa: tag_qca: Add support for handling Ethernet mdio and MIB packet

[Ansuel Smith]

Add connect/disconnect helper to assign private struct to the cpu port
dsa priv.
Add support for Ethernet mdio packet and MIB packet if the dsa driver
provide an handler to correctly parse and elaborate the data.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 include/linux/dsa/tag_qca.h |  7 ++++++
 net/dsa/tag_qca.c           | 49 +++++++++++++++++++++++++++++++++++--
 2 files changed, 54 insertions(+), 2 deletions(-)

diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
index 578a4aeafd92..f403fdab6f29 100644
--- a/include/linux/dsa/tag_qca.h
+++ b/include/linux/dsa/tag_qca.h
@@ -59,4 +59,11 @@ struct mdio_ethhdr {
 	u16 hdr;		/* qca hdr */
 } __packed;
 
+struct qca8k_port_tag {
+	void (*rw_reg_ack_handler)(struct dsa_port *dp,
+				   struct sk_buff *skb);
+	void (*mib_autocast_handler)(struct dsa_port *dp,
+				     struct sk_buff *skb);
+};
+
 #endif /* __TAG_QCA_H */
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index b8b05d54a74c..451183f0461a 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -32,6 +32,8 @@ static struct sk_buff *qca_tag_xmit(struct sk_buff *skb, struct net_device *dev)
 
 static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 {
+	struct dsa_port *dp = dev->dsa_ptr;
+	struct qca8k_port_tag *header = dp->priv;
 	u16  hdr, pk_type;
 	__be16 *phdr;
 	int port;
@@ -51,9 +53,19 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 	/* Get pk type */
 	pk_type = FIELD_GET(QCA_HDR_RECV_TYPE, hdr);
 
-	/* MDIO read/write packet */
-	if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK)
+	/* Ethernet MDIO read/write packet */
+	if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK) {
+		if (header->rw_reg_ack_handler)
+			header->rw_reg_ack_handler(dp, skb);
 		return NULL;
+	}
+
+	/* Ethernet MIB counter packet */
+	if (pk_type == QCA_HDR_RECV_TYPE_MIB) {
+		if (header->mib_autocast_handler)
+			header->mib_autocast_handler(dp, skb);
+		return NULL;
+	}
 
 	/* Remove QCA tag and recalculate checksum */
 	skb_pull_rcsum(skb, QCA_HDR_LEN);
@@ -69,9 +81,42 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 	return skb;
 }
 
+static int qca_tag_connect(struct dsa_switch_tree *dst)
+{
+	struct qca8k_port_tag *header;
+	struct dsa_port *dp;
+
+	list_for_each_entry(dp, &dst->ports, list) {
+		if (!dsa_port_is_cpu(dp))
+			continue;
+
+		header = kzalloc(sizeof(*header), GFP_KERNEL);
+		if (!header)
+			return -ENOMEM;
+
+		dp->priv = header;
+	}
+
+	return 0;
+}
+
+static void qca_tag_disconnect(struct dsa_switch_tree *dst)
+{
+	struct dsa_port *dp;
+
+	list_for_each_entry(dp, &dst->ports, list) {
+		if (!dsa_port_is_cpu(dp))
+			continue;
+
+		kfree(dp->priv);
+	}
+}
+
 static const struct dsa_device_ops qca_netdev_ops = {
 	.name	= "qca",
 	.proto	= DSA_TAG_PROTO_QCA,
+	.connect = qca_tag_connect,
+	.disconnect = qca_tag_disconnect,
 	.xmit	= qca_tag_xmit,
 	.rcv	= qca_tag_rcv,
 	.needed_headroom = QCA_HDR_LEN,
-- 
2.32.0

[net-next RFC PATCH v2 7/8] net: dsa: qca8k: Add support for mdio read/write in Ethernet packet

[Ansuel Smith]

Add qca8k side support for mdio read/write in Ethernet packet.
qca8k supports some specially crafted Ethernet packet that can be used
for mdio read/write instead of the legacy method uart/internal mdio.
This add support for the qca8k side to craft the packet and enqueue it.
Each port and the qca8k_priv have a special struct to put data in it.
The completion API is used to wait for the packet to be received back
with the requested data.

The various steps are:
1. Craft the special packet with the qca hdr set to mdio read/write
   mode.
2. Set the lock in the dedicated mdio struct.
3. Reinit the completion.
4. Enqueue the packet.
5. Wait the packet to be received.
6. Use the data set by the tagger to complete the mdio operation.

If the completion timeouts or the ack value is not true, the legacy
mdio way is used.

It has to be considered that in the initial setup mdio is still used and
mdio is still used until DSA is ready to accept and tag packet.

tag_proto_connect() is used to fill the required handler for the tagger
to correctly parse and elaborate the special Ethernet mdio packet.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c     | 214 +++++++++++++++++++++++++++++++++++-
 drivers/net/dsa/qca8k.h     |   4 +
 include/linux/dsa/tag_qca.h |  10 ++
 3 files changed, 226 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 96a7fbf8700c..5b7508a6e4ba 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -20,6 +20,7 @@
 #include <linux/phylink.h>
 #include <linux/gpio/consumer.h>
 #include <linux/etherdevice.h>
+#include <linux/dsa/tag_qca.h>
 
 #include "qca8k.h"
 
@@ -170,14 +171,162 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
 	return regmap_update_bits(priv->regmap, reg, mask, write_val);
 }
 
+static struct sk_buff *qca8k_alloc_mdio_header(struct qca8k_port_tag *header, enum mdio_cmd cmd,
+					       u32 reg, u32 *val)
+{
+	struct mdio_ethhdr *mdio_ethhdr;
+	struct sk_buff *skb;
+	u16 hdr;
+
+	skb = dev_alloc_skb(QCA_HDR_MDIO_PKG_LEN);
+
+	prefetchw(skb->data);
+
+	skb_reset_mac_header(skb);
+	skb_set_network_header(skb, skb->len);
+
+	mdio_ethhdr = skb_push(skb, QCA_HDR_MDIO_HEADER_LEN + QCA_HDR_LEN);
+
+	hdr = FIELD_PREP(QCA_HDR_XMIT_VERSION, QCA_HDR_VERSION);
+	hdr |= QCA_HDR_XMIT_FROM_CPU;
+	hdr |= FIELD_PREP(QCA_HDR_XMIT_DP_BIT, BIT(0));
+	hdr |= FIELD_PREP(QCA_HDR_XMIT_CONTROL, QCA_HDR_XMIT_TYPE_RW_REG);
+
+	mdio_ethhdr->seq = FIELD_PREP(QCA_HDR_MDIO_SEQ_NUM, 200);
+
+	mdio_ethhdr->command = FIELD_PREP(QCA_HDR_MDIO_ADDR, reg);
+	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_LENGTH, 4);
+	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_CMD, cmd);
+	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_CHECK_CODE, MDIO_CHECK_CODE_VAL);
+
+	if (cmd == MDIO_WRITE)
+		mdio_ethhdr->mdio_data = *val;
+
+	mdio_ethhdr->hdr = htons(hdr);
+
+	skb_put_zero(skb, QCA_HDR_MDIO_DATA2_LEN);
+	skb_put_zero(skb, QCA_HDR_MDIO_PADDING_LEN);
+
+	return skb;
+}
+
+static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
+{
+	struct qca8k_port_tag *header = priv->header_mdio;
+	struct sk_buff *skb;
+	bool ack;
+	int ret;
+
+	skb = qca8k_alloc_mdio_header(header, MDIO_READ, reg, 0);
+	skb->dev = dsa_to_port(priv->ds, 0)->master;
+
+	mutex_lock(&header->mdio_mutex);
+
+	reinit_completion(&header->rw_done);
+	header->seq = 200;
+	header->ack = false;
+
+	dev_queue_xmit(skb);
+
+	ret = wait_for_completion_timeout(&header->rw_done, QCA8K_MDIO_RW_ETHERNET);
+
+	*val = header->data[0];
+	ack = header->ack;
+
+	mutex_unlock(&header->mdio_mutex);
+
+	if (ret <= 0)
+		return -ETIMEDOUT;
+
+	if (!ack)
+		return -EINVAL;
+
+	return 0;
+}
+
+static void qca8k_rw_reg_ack_handler(struct dsa_port *dp, struct sk_buff *skb)
+{
+	struct qca8k_port_tag *header = dp->priv;
+	struct mdio_ethhdr *mdio_ethhdr;
+
+	mdio_ethhdr = (struct mdio_ethhdr *)skb_mac_header(skb);
+
+	header->data[0] = mdio_ethhdr->mdio_data;
+
+	/* Get the rest of the 12 byte of data */
+	memcpy(header->data + 1, skb->data, QCA_HDR_MDIO_DATA2_LEN);
+
+	/* Make sure the seq match the requested packet */
+	if (mdio_ethhdr->seq == header->seq)
+		header->ack = true;
+
+	complete(&header->rw_done);
+}
+
+static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
+{
+	struct qca8k_port_tag *header = priv->header_mdio;
+	struct sk_buff *skb;
+	bool ack;
+	int ret;
+
+	skb = qca8k_alloc_mdio_header(header, MDIO_WRITE, reg, &val);
+	skb->dev = dsa_to_port(priv->ds, 0)->master;
+
+	mutex_lock(&header->mdio_mutex);
+
+	dev_queue_xmit(skb);
+
+	reinit_completion(&header->rw_done);
+	header->ack = false;
+	header->seq = 200;
+
+	ret = wait_for_completion_timeout(&header->rw_done, QCA8K_MDIO_RW_ETHERNET);
+
+	ack = header->ack;
+
+	mutex_unlock(&header->mdio_mutex);
+
+	if (ret <= 0)
+		return -ETIMEDOUT;
+
+	if (!ack)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int
+qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
+{
+	u32 val = 0;
+	int ret;
+
+	ret = qca8k_read_eth(priv, reg, &val);
+	if (ret)
+		return ret;
+
+	val &= ~mask;
+	val |= write_val;
+
+	return qca8k_write_eth(priv, reg, val);
+}
+
 static int
 qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
+	struct qca8k_port_tag *header_mdio;
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	int ret;
 
+	header_mdio = priv->header_mdio;
+
+	if (header_mdio)
+		if (!qca8k_read_eth(priv, reg, val))
+			return 0;
+
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -197,10 +346,17 @@ static int
 qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
+	struct qca8k_port_tag *header_mdio;
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	int ret;
 
+	header_mdio = priv->header_mdio;
+
+	if (header_mdio)
+		if (!qca8k_write_eth(priv, reg, val))
+			return 0;
+
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -220,11 +376,18 @@ static int
 qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_val)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
+	struct qca8k_port_tag *header_mdio;
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	u32 val;
 	int ret;
 
+	header_mdio = priv->header_mdio;
+
+	if (header_mdio)
+		if (!qca8k_regmap_update_bits_eth(priv, reg, mask, write_val))
+			return 0;
+
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -1223,8 +1386,13 @@ qca8k_setup(struct dsa_switch *ds)
 	 * Configure specific switch configuration for ports
 	 */
 	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
+		struct dsa_port *dp = dsa_to_port(ds, i);
+
+		/* Set the header_mdio to be accessible by the qca tagger */
+		dp->priv = priv->header_mdio;
+
 		/* CPU port gets connected to all user ports of the switch */
-		if (dsa_is_cpu_port(ds, i)) {
+		if (dsa_port_is_cpu(dp)) {
 			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
 					QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
 			if (ret)
@@ -1232,7 +1400,7 @@ qca8k_setup(struct dsa_switch *ds)
 		}
 
 		/* Individual user ports get connected to CPU port only */
-		if (dsa_is_user_port(ds, i)) {
+		if (dsa_port_is_user(dp)) {
 			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
 					QCA8K_PORT_LOOKUP_MEMBER,
 					BIT(cpu_port));
@@ -2382,6 +2550,38 @@ qca8k_port_lag_leave(struct dsa_switch *ds, int port,
 	return qca8k_lag_refresh_portmap(ds, port, lag, true);
 }
 
+static int qca8k_tag_proto_connect(struct dsa_switch *ds,
+				   const struct dsa_device_ops *tag_ops)
+{
+	struct qca8k_priv *qca8k_priv = ds->priv;
+	struct qca8k_port_tag *priv;
+	struct dsa_port *dp;
+	int i;
+
+	switch (tag_ops->proto) {
+	case DSA_TAG_PROTO_QCA:
+		for (i = 0; i < QCA8K_NUM_PORTS; i++) {
+			dp = dsa_to_port(ds, i);
+			priv = dp->priv;
+
+			if (!dsa_port_is_cpu(dp))
+				continue;
+
+			mutex_init(&priv->mdio_mutex);
+			init_completion(&priv->rw_done);
+			/* Cache the header mdio */
+			qca8k_priv->header_mdio = priv;
+			priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
+		}
+
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	return 0;
+}
+
 static const struct dsa_switch_ops qca8k_switch_ops = {
 	.get_tag_protocol	= qca8k_get_tag_protocol,
 	.setup			= qca8k_setup,
@@ -2417,6 +2617,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.get_phy_flags		= qca8k_get_phy_flags,
 	.port_lag_join		= qca8k_port_lag_join,
 	.port_lag_leave		= qca8k_port_lag_leave,
+	.tag_proto_connect	= qca8k_tag_proto_connect,
 };
 
 static int qca8k_read_switch_id(struct qca8k_priv *priv)
@@ -2452,6 +2653,7 @@ static int qca8k_read_switch_id(struct qca8k_priv *priv)
 static int
 qca8k_sw_probe(struct mdio_device *mdiodev)
 {
+	struct qca8k_port_tag *header_mdio;
 	struct qca8k_priv *priv;
 	int ret;
 
@@ -2462,6 +2664,13 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 	if (!priv)
 		return -ENOMEM;
 
+	header_mdio = devm_kzalloc(&mdiodev->dev, sizeof(*header_mdio), GFP_KERNEL);
+	if (!header_mdio)
+		return -ENOMEM;
+
+	mutex_init(&header_mdio->mdio_mutex);
+	init_completion(&header_mdio->rw_done);
+
 	priv->bus = mdiodev->bus;
 	priv->dev = &mdiodev->dev;
 
@@ -2501,6 +2710,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 	priv->ds->priv = priv;
 	priv->ops = qca8k_switch_ops;
 	priv->ds->ops = &priv->ops;
+	priv->header_mdio = header_mdio;
 	mutex_init(&priv->reg_mutex);
 	dev_set_drvdata(&mdiodev->dev, priv);
 
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index ab4a417b25a9..25aa1509e0c0 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -11,6 +11,9 @@
 #include <linux/delay.h>
 #include <linux/regmap.h>
 #include <linux/gpio.h>
+#include <linux/dsa/tag_qca.h>
+
+#define QCA8K_MDIO_RW_ETHERNET				100
 
 #define QCA8K_NUM_PORTS					7
 #define QCA8K_NUM_CPU_PORTS				2
@@ -353,6 +356,7 @@ struct qca8k_priv {
 	struct dsa_switch_ops ops;
 	struct gpio_desc *reset_gpio;
 	unsigned int port_mtu[QCA8K_NUM_PORTS];
+	struct qca8k_port_tag *header_mdio;
 };
 
 struct qca8k_mib_desc {
diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
index f403fdab6f29..2b823d971ae9 100644
--- a/include/linux/dsa/tag_qca.h
+++ b/include/linux/dsa/tag_qca.h
@@ -59,11 +59,21 @@ struct mdio_ethhdr {
 	u16 hdr;		/* qca hdr */
 } __packed;
 
+enum mdio_cmd {
+	MDIO_WRITE = 0x0,
+	MDIO_READ
+};
+
 struct qca8k_port_tag {
 	void (*rw_reg_ack_handler)(struct dsa_port *dp,
 				   struct sk_buff *skb);
 	void (*mib_autocast_handler)(struct dsa_port *dp,
 				     struct sk_buff *skb);
+	struct completion rw_done;
+	struct mutex mdio_mutex; /* Enforce one mdio read/write at time */
+	bool ack;
+	u32 seq;
+	u32 data[4];
 };
 
 #endif /* __TAG_QCA_H */
-- 
2.32.0

[net-next RFC PATCH v2 8/8] net: dsa: qca8k: cache lo and hi for mdio write

[Ansuel Smith]

From Documentation, we can cache lo and hi the same way we do with the
page. This massively reduce the mdio write as 3/4 of the time we only
require to write the lo or hi part for a mdio write.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 49 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 44 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 5b7508a6e4ba..6f4a99032cdd 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -94,6 +94,48 @@ qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
 	*page = regaddr & 0x3ff;
 }
 
+static u16 qca8k_current_lo = 0xffff;
+
+static int
+qca8k_set_lo(struct mii_bus *bus, int phy_id, u32 regnum, u16 lo)
+{
+	int ret;
+
+	if (lo == qca8k_current_lo) {
+		// pr_info("SAME LOW");
+		return 0;
+	}
+
+	ret = bus->write(bus, phy_id, regnum, lo);
+	if (ret < 0)
+		dev_err_ratelimited(&bus->dev,
+				    "failed to write qca8k 32bit lo register\n");
+
+	qca8k_current_lo = lo;
+	return 0;
+}
+
+static u16 qca8k_current_hi = 0xffff;
+
+static int
+qca8k_set_hi(struct mii_bus *bus, int phy_id, u32 regnum, u16 hi)
+{
+	int ret;
+
+	if (hi == qca8k_current_hi) {
+		// pr_info("SAME HI");
+		return 0;
+	}
+
+	ret = bus->write(bus, phy_id, regnum, hi);
+	if (ret < 0)
+		dev_err_ratelimited(&bus->dev,
+				    "failed to write qca8k 32bit hi register\n");
+
+	qca8k_current_hi = hi;
+	return 0;
+}
+
 static int
 qca8k_mii_read32(struct mii_bus *bus, int phy_id, u32 regnum, u32 *val)
 {
@@ -125,12 +167,9 @@ qca8k_mii_write32(struct mii_bus *bus, int phy_id, u32 regnum, u32 val)
 	lo = val & 0xffff;
 	hi = (u16)(val >> 16);
 
-	ret = bus->write(bus, phy_id, regnum, lo);
+	ret = qca8k_set_lo(bus, phy_id, regnum, lo);
 	if (ret >= 0)
-		ret = bus->write(bus, phy_id, regnum + 1, hi);
-	if (ret < 0)
-		dev_err_ratelimited(&bus->dev,
-				    "failed to write qca8k 32bit register\n");
+		ret = qca8k_set_hi(bus, phy_id, regnum + 1, hi);
 }
 
 static int
-- 
2.32.0

Re: [net-next RFC PATCH v2 7/8] net: dsa: qca8k: Add support for mdio read/write in Ethernet packet

[Vladimir Oltean]

On Wed, Dec 08, 2021 at 04:40:39AM +0100, Ansuel Smith wrote:
> Add qca8k side support for mdio read/write in Ethernet packet.
> qca8k supports some specially crafted Ethernet packet that can be used
> for mdio read/write instead of the legacy method uart/internal mdio.
> This add support for the qca8k side to craft the packet and enqueue it.
> Each port and the qca8k_priv have a special struct to put data in it.
> The completion API is used to wait for the packet to be received back
> with the requested data.
> 
> The various steps are:
> 1. Craft the special packet with the qca hdr set to mdio read/write
>    mode.
> 2. Set the lock in the dedicated mdio struct.
> 3. Reinit the completion.
> 4. Enqueue the packet.
> 5. Wait the packet to be received.
> 6. Use the data set by the tagger to complete the mdio operation.
> 
> If the completion timeouts or the ack value is not true, the legacy
> mdio way is used.
> 
> It has to be considered that in the initial setup mdio is still used and
> mdio is still used until DSA is ready to accept and tag packet.
> 
> tag_proto_connect() is used to fill the required handler for the tagger
> to correctly parse and elaborate the special Ethernet mdio packet.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca8k.c     | 214 +++++++++++++++++++++++++++++++++++-
>  drivers/net/dsa/qca8k.h     |   4 +
>  include/linux/dsa/tag_qca.h |  10 ++
>  3 files changed, 226 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index 96a7fbf8700c..5b7508a6e4ba 100644
> --- a/drivers/net/dsa/qca8k.c
> +++ b/drivers/net/dsa/qca8k.c
> @@ -20,6 +20,7 @@
>  #include <linux/phylink.h>
>  #include <linux/gpio/consumer.h>
>  #include <linux/etherdevice.h>
> +#include <linux/dsa/tag_qca.h>
>  
>  #include "qca8k.h"
>  
> @@ -170,14 +171,162 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
>  	return regmap_update_bits(priv->regmap, reg, mask, write_val);
>  }
>  
> +static struct sk_buff *qca8k_alloc_mdio_header(struct qca8k_port_tag *header, enum mdio_cmd cmd,
> +					       u32 reg, u32 *val)
> +{
> +	struct mdio_ethhdr *mdio_ethhdr;
> +	struct sk_buff *skb;
> +	u16 hdr;
> +
> +	skb = dev_alloc_skb(QCA_HDR_MDIO_PKG_LEN);
> +
> +	prefetchw(skb->data);
> +
> +	skb_reset_mac_header(skb);
> +	skb_set_network_header(skb, skb->len);
> +
> +	mdio_ethhdr = skb_push(skb, QCA_HDR_MDIO_HEADER_LEN + QCA_HDR_LEN);
> +
> +	hdr = FIELD_PREP(QCA_HDR_XMIT_VERSION, QCA_HDR_VERSION);
> +	hdr |= QCA_HDR_XMIT_FROM_CPU;
> +	hdr |= FIELD_PREP(QCA_HDR_XMIT_DP_BIT, BIT(0));
> +	hdr |= FIELD_PREP(QCA_HDR_XMIT_CONTROL, QCA_HDR_XMIT_TYPE_RW_REG);
> +
> +	mdio_ethhdr->seq = FIELD_PREP(QCA_HDR_MDIO_SEQ_NUM, 200);
> +
> +	mdio_ethhdr->command = FIELD_PREP(QCA_HDR_MDIO_ADDR, reg);
> +	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_LENGTH, 4);
> +	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_CMD, cmd);
> +	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_CHECK_CODE, MDIO_CHECK_CODE_VAL);
> +
> +	if (cmd == MDIO_WRITE)
> +		mdio_ethhdr->mdio_data = *val;
> +
> +	mdio_ethhdr->hdr = htons(hdr);
> +
> +	skb_put_zero(skb, QCA_HDR_MDIO_DATA2_LEN);
> +	skb_put_zero(skb, QCA_HDR_MDIO_PADDING_LEN);
> +
> +	return skb;
> +}
> +
> +static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> +{
> +	struct qca8k_port_tag *header = priv->header_mdio;
> +	struct sk_buff *skb;
> +	bool ack;
> +	int ret;
> +
> +	skb = qca8k_alloc_mdio_header(header, MDIO_READ, reg, 0);
> +	skb->dev = dsa_to_port(priv->ds, 0)->master;
> +
> +	mutex_lock(&header->mdio_mutex);
> +
> +	reinit_completion(&header->rw_done);
> +	header->seq = 200;
> +	header->ack = false;
> +
> +	dev_queue_xmit(skb);
> +
> +	ret = wait_for_completion_timeout(&header->rw_done, QCA8K_MDIO_RW_ETHERNET);
> +
> +	*val = header->data[0];
> +	ack = header->ack;
> +
> +	mutex_unlock(&header->mdio_mutex);
> +
> +	if (ret <= 0)
> +		return -ETIMEDOUT;
> +
> +	if (!ack)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static void qca8k_rw_reg_ack_handler(struct dsa_port *dp, struct sk_buff *skb)
> +{
> +	struct qca8k_port_tag *header = dp->priv;
> +	struct mdio_ethhdr *mdio_ethhdr;
> +
> +	mdio_ethhdr = (struct mdio_ethhdr *)skb_mac_header(skb);
> +
> +	header->data[0] = mdio_ethhdr->mdio_data;
> +
> +	/* Get the rest of the 12 byte of data */
> +	memcpy(header->data + 1, skb->data, QCA_HDR_MDIO_DATA2_LEN);
> +
> +	/* Make sure the seq match the requested packet */
> +	if (mdio_ethhdr->seq == header->seq)
> +		header->ack = true;
> +
> +	complete(&header->rw_done);
> +}
> +
> +static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
> +{
> +	struct qca8k_port_tag *header = priv->header_mdio;
> +	struct sk_buff *skb;
> +	bool ack;
> +	int ret;
> +
> +	skb = qca8k_alloc_mdio_header(header, MDIO_WRITE, reg, &val);
> +	skb->dev = dsa_to_port(priv->ds, 0)->master;
> +
> +	mutex_lock(&header->mdio_mutex);
> +
> +	dev_queue_xmit(skb);
> +
> +	reinit_completion(&header->rw_done);
> +	header->ack = false;
> +	header->seq = 200;
> +
> +	ret = wait_for_completion_timeout(&header->rw_done, QCA8K_MDIO_RW_ETHERNET);
> +
> +	ack = header->ack;
> +
> +	mutex_unlock(&header->mdio_mutex);
> +
> +	if (ret <= 0)
> +		return -ETIMEDOUT;
> +
> +	if (!ack)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int
> +qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
> +{
> +	u32 val = 0;
> +	int ret;
> +
> +	ret = qca8k_read_eth(priv, reg, &val);
> +	if (ret)
> +		return ret;
> +
> +	val &= ~mask;
> +	val |= write_val;
> +
> +	return qca8k_write_eth(priv, reg, val);
> +}
> +
>  static int
>  qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
>  {
>  	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
> +	struct qca8k_port_tag *header_mdio;
>  	struct mii_bus *bus = priv->bus;
>  	u16 r1, r2, page;
>  	int ret;
>  
> +	header_mdio = priv->header_mdio;
> +
> +	if (header_mdio)
> +		if (!qca8k_read_eth(priv, reg, val))
> +			return 0;
> +
>  	qca8k_split_addr(reg, &r1, &r2, &page);
>  
>  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> @@ -197,10 +346,17 @@ static int
>  qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
>  {
>  	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
> +	struct qca8k_port_tag *header_mdio;
>  	struct mii_bus *bus = priv->bus;
>  	u16 r1, r2, page;
>  	int ret;
>  
> +	header_mdio = priv->header_mdio;
> +
> +	if (header_mdio)
> +		if (!qca8k_write_eth(priv, reg, val))
> +			return 0;
> +
>  	qca8k_split_addr(reg, &r1, &r2, &page);
>  
>  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> @@ -220,11 +376,18 @@ static int
>  qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_val)
>  {
>  	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
> +	struct qca8k_port_tag *header_mdio;
>  	struct mii_bus *bus = priv->bus;
>  	u16 r1, r2, page;
>  	u32 val;
>  	int ret;
>  
> +	header_mdio = priv->header_mdio;
> +
> +	if (header_mdio)
> +		if (!qca8k_regmap_update_bits_eth(priv, reg, mask, write_val))
> +			return 0;
> +
>  	qca8k_split_addr(reg, &r1, &r2, &page);
>  
>  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> @@ -1223,8 +1386,13 @@ qca8k_setup(struct dsa_switch *ds)
>  	 * Configure specific switch configuration for ports
>  	 */
>  	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> +		struct dsa_port *dp = dsa_to_port(ds, i);
> +
> +		/* Set the header_mdio to be accessible by the qca tagger */
> +		dp->priv = priv->header_mdio;

Why are you setting up the dp->priv pointer from the switch driver?
I though the whole point of qca_tag_connect is to set up that memory by
itself.

> +
>  		/* CPU port gets connected to all user ports of the switch */
> -		if (dsa_is_cpu_port(ds, i)) {
> +		if (dsa_port_is_cpu(dp)) {
>  			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
>  					QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
>  			if (ret)
> @@ -1232,7 +1400,7 @@ qca8k_setup(struct dsa_switch *ds)
>  		}
>  
>  		/* Individual user ports get connected to CPU port only */
> -		if (dsa_is_user_port(ds, i)) {
> +		if (dsa_port_is_user(dp)) {
>  			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
>  					QCA8K_PORT_LOOKUP_MEMBER,
>  					BIT(cpu_port));
> @@ -2382,6 +2550,38 @@ qca8k_port_lag_leave(struct dsa_switch *ds, int port,
>  	return qca8k_lag_refresh_portmap(ds, port, lag, true);
>  }
>  
> +static int qca8k_tag_proto_connect(struct dsa_switch *ds,
> +				   const struct dsa_device_ops *tag_ops)
> +{
> +	struct qca8k_priv *qca8k_priv = ds->priv;
> +	struct qca8k_port_tag *priv;
> +	struct dsa_port *dp;
> +	int i;
> +
> +	switch (tag_ops->proto) {
> +	case DSA_TAG_PROTO_QCA:
> +		for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> +			dp = dsa_to_port(ds, i);
> +			priv = dp->priv;
> +
> +			if (!dsa_port_is_cpu(dp))
> +				continue;
> +
> +			mutex_init(&priv->mdio_mutex);
> +			init_completion(&priv->rw_done);
> +			/* Cache the header mdio */
> +			qca8k_priv->header_mdio = priv;
> +			priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
> +		}
> +
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}
> +
>  static const struct dsa_switch_ops qca8k_switch_ops = {
>  	.get_tag_protocol	= qca8k_get_tag_protocol,
>  	.setup			= qca8k_setup,
> @@ -2417,6 +2617,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
>  	.get_phy_flags		= qca8k_get_phy_flags,
>  	.port_lag_join		= qca8k_port_lag_join,
>  	.port_lag_leave		= qca8k_port_lag_leave,
> +	.tag_proto_connect	= qca8k_tag_proto_connect,
>  };
>  
>  static int qca8k_read_switch_id(struct qca8k_priv *priv)
> @@ -2452,6 +2653,7 @@ static int qca8k_read_switch_id(struct qca8k_priv *priv)
>  static int
>  qca8k_sw_probe(struct mdio_device *mdiodev)
>  {
> +	struct qca8k_port_tag *header_mdio;
>  	struct qca8k_priv *priv;
>  	int ret;
>  
> @@ -2462,6 +2664,13 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
>  	if (!priv)
>  		return -ENOMEM;
>  
> +	header_mdio = devm_kzalloc(&mdiodev->dev, sizeof(*header_mdio), GFP_KERNEL);
> +	if (!header_mdio)
> +		return -ENOMEM;
> +
> +	mutex_init(&header_mdio->mdio_mutex);
> +	init_completion(&header_mdio->rw_done);
> +
>  	priv->bus = mdiodev->bus;
>  	priv->dev = &mdiodev->dev;
>  
> @@ -2501,6 +2710,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
>  	priv->ds->priv = priv;
>  	priv->ops = qca8k_switch_ops;
>  	priv->ds->ops = &priv->ops;
> +	priv->header_mdio = header_mdio;
>  	mutex_init(&priv->reg_mutex);
>  	dev_set_drvdata(&mdiodev->dev, priv);
>  
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index ab4a417b25a9..25aa1509e0c0 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -11,6 +11,9 @@
>  #include <linux/delay.h>
>  #include <linux/regmap.h>
>  #include <linux/gpio.h>
> +#include <linux/dsa/tag_qca.h>
> +
> +#define QCA8K_MDIO_RW_ETHERNET				100
>  
>  #define QCA8K_NUM_PORTS					7
>  #define QCA8K_NUM_CPU_PORTS				2
> @@ -353,6 +356,7 @@ struct qca8k_priv {
>  	struct dsa_switch_ops ops;
>  	struct gpio_desc *reset_gpio;
>  	unsigned int port_mtu[QCA8K_NUM_PORTS];
> +	struct qca8k_port_tag *header_mdio;
>  };
>  
>  struct qca8k_mib_desc {
> diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
> index f403fdab6f29..2b823d971ae9 100644
> --- a/include/linux/dsa/tag_qca.h
> +++ b/include/linux/dsa/tag_qca.h
> @@ -59,11 +59,21 @@ struct mdio_ethhdr {
>  	u16 hdr;		/* qca hdr */
>  } __packed;
>  
> +enum mdio_cmd {
> +	MDIO_WRITE = 0x0,
> +	MDIO_READ
> +};
> +
>  struct qca8k_port_tag {
>  	void (*rw_reg_ack_handler)(struct dsa_port *dp,
>  				   struct sk_buff *skb);
>  	void (*mib_autocast_handler)(struct dsa_port *dp,
>  				     struct sk_buff *skb);
> +	struct completion rw_done;
> +	struct mutex mdio_mutex; /* Enforce one mdio read/write at time */
> +	bool ack;
> +	u32 seq;
> +	u32 data[4];

None of these structures need to stay in the data structure shared with
the tagger. They can be in qca8k_priv. The tagger should only see the
function pointers. It doesn't care what is done with the packets.

>  };
>  
>  #endif /* __TAG_QCA_H */
> -- 
> 2.32.0
> 

Re: [net-next RFC PATCH v2 0/8] Add support for qca8k mdio rw in Ethernet packet

[Vladimir Oltean]

On Wed, Dec 08, 2021 at 04:40:32AM +0100, Ansuel Smith wrote:
> I still have to find a solution to a slowdown problem and this is where
> I would love to get some hint.
> Currently I still didn't find a good way to understand when the tagger
> starts to accept packets and because of this the initial setup is slow
> as every completion timeouts. Am I missing something or is there a way
> to check for this?
> After the initial slowdown, as soon as the cpu port is ready and starts
> to accept packet, every transaction is near instant and no completion
> timeouts.

My guess is that the problem with the initial slowdown is that you try
to use the Ethernet based register access before things are set up:
before the master is up and ready, before the switch is minimally set
up, etc.

I think what this Ethernet-based register access technique needs to be
more reliable is a notification about the DSA master going up or down.
Otherwise it won't be very efficient at all, to wait for every single
Ethernet access attempt to time out before attempting a direct MDIO
access.

But there are some problems with offering a "master_going_up/master_going_down"
set of callbacks. Specifically, we could easily hook into the NETDEV_PRE_UP/
NETDEV_GOING_DOWN netdev notifiers and transform these into DSA switch
API calls. The goal would be for the qca8k tagger to mark the
Ethernet-based register access method as available/unavailable, and in
the regmap implementation, to use that or the other. DSA would then also
be responsible for calling "master_going_up" when the switch ports and
master are sufficiently initialized that traffic should be possible.
But that first "master_going_up" notification is in fact the most
problematic one, because we may not receive a NETDEV_PRE_UP event,
because the DSA master may already be up when we probe our switch tree.
This would be a bit finicky to get right. We may, for instance, hold
rtnl_lock for the entirety of dsa_tree_setup_master(). This will block
potentially concurrent netdevice notifiers handled by dsa_slave_nb.
And while holding rtnl_lock() and immediately after each dsa_master_setup(),
we may check whether master->flags & IFF_UP is true, and if it is,
synthesize a call to ds->ops->master_going_up(). We also need to do the
reverse in dsa_tree_teardown_master().

Re: [net-next RFC PATCH v2 7/8] net: dsa: qca8k: Add support for mdio read/write in Ethernet packet

[Ansuel Smith]

On Wed, Dec 08, 2021 at 02:18:50PM +0200, Vladimir Oltean wrote:
> On Wed, Dec 08, 2021 at 04:40:39AM +0100, Ansuel Smith wrote:
> > Add qca8k side support for mdio read/write in Ethernet packet.
> > qca8k supports some specially crafted Ethernet packet that can be used
> > for mdio read/write instead of the legacy method uart/internal mdio.
> > This add support for the qca8k side to craft the packet and enqueue it.
> > Each port and the qca8k_priv have a special struct to put data in it.
> > The completion API is used to wait for the packet to be received back
> > with the requested data.
> > 
> > The various steps are:
> > 1. Craft the special packet with the qca hdr set to mdio read/write
> >    mode.
> > 2. Set the lock in the dedicated mdio struct.
> > 3. Reinit the completion.
> > 4. Enqueue the packet.
> > 5. Wait the packet to be received.
> > 6. Use the data set by the tagger to complete the mdio operation.
> > 
> > If the completion timeouts or the ack value is not true, the legacy
> > mdio way is used.
> > 
> > It has to be considered that in the initial setup mdio is still used and
> > mdio is still used until DSA is ready to accept and tag packet.
> > 
> > tag_proto_connect() is used to fill the required handler for the tagger
> > to correctly parse and elaborate the special Ethernet mdio packet.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/dsa/qca8k.c     | 214 +++++++++++++++++++++++++++++++++++-
> >  drivers/net/dsa/qca8k.h     |   4 +
> >  include/linux/dsa/tag_qca.h |  10 ++
> >  3 files changed, 226 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index 96a7fbf8700c..5b7508a6e4ba 100644
> > --- a/drivers/net/dsa/qca8k.c
> > +++ b/drivers/net/dsa/qca8k.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/phylink.h>
> >  #include <linux/gpio/consumer.h>
> >  #include <linux/etherdevice.h>
> > +#include <linux/dsa/tag_qca.h>
> >  
> >  #include "qca8k.h"
> >  
> > @@ -170,14 +171,162 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
> >  	return regmap_update_bits(priv->regmap, reg, mask, write_val);
> >  }
> >  
> > +static struct sk_buff *qca8k_alloc_mdio_header(struct qca8k_port_tag *header, enum mdio_cmd cmd,
> > +					       u32 reg, u32 *val)
> > +{
> > +	struct mdio_ethhdr *mdio_ethhdr;
> > +	struct sk_buff *skb;
> > +	u16 hdr;
> > +
> > +	skb = dev_alloc_skb(QCA_HDR_MDIO_PKG_LEN);
> > +
> > +	prefetchw(skb->data);
> > +
> > +	skb_reset_mac_header(skb);
> > +	skb_set_network_header(skb, skb->len);
> > +
> > +	mdio_ethhdr = skb_push(skb, QCA_HDR_MDIO_HEADER_LEN + QCA_HDR_LEN);
> > +
> > +	hdr = FIELD_PREP(QCA_HDR_XMIT_VERSION, QCA_HDR_VERSION);
> > +	hdr |= QCA_HDR_XMIT_FROM_CPU;
> > +	hdr |= FIELD_PREP(QCA_HDR_XMIT_DP_BIT, BIT(0));
> > +	hdr |= FIELD_PREP(QCA_HDR_XMIT_CONTROL, QCA_HDR_XMIT_TYPE_RW_REG);
> > +
> > +	mdio_ethhdr->seq = FIELD_PREP(QCA_HDR_MDIO_SEQ_NUM, 200);
> > +
> > +	mdio_ethhdr->command = FIELD_PREP(QCA_HDR_MDIO_ADDR, reg);
> > +	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_LENGTH, 4);
> > +	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_CMD, cmd);
> > +	mdio_ethhdr->command |= FIELD_PREP(QCA_HDR_MDIO_CHECK_CODE, MDIO_CHECK_CODE_VAL);
> > +
> > +	if (cmd == MDIO_WRITE)
> > +		mdio_ethhdr->mdio_data = *val;
> > +
> > +	mdio_ethhdr->hdr = htons(hdr);
> > +
> > +	skb_put_zero(skb, QCA_HDR_MDIO_DATA2_LEN);
> > +	skb_put_zero(skb, QCA_HDR_MDIO_PADDING_LEN);
> > +
> > +	return skb;
> > +}
> > +
> > +static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> > +{
> > +	struct qca8k_port_tag *header = priv->header_mdio;
> > +	struct sk_buff *skb;
> > +	bool ack;
> > +	int ret;
> > +
> > +	skb = qca8k_alloc_mdio_header(header, MDIO_READ, reg, 0);
> > +	skb->dev = dsa_to_port(priv->ds, 0)->master;
> > +
> > +	mutex_lock(&header->mdio_mutex);
> > +
> > +	reinit_completion(&header->rw_done);
> > +	header->seq = 200;
> > +	header->ack = false;
> > +
> > +	dev_queue_xmit(skb);
> > +
> > +	ret = wait_for_completion_timeout(&header->rw_done, QCA8K_MDIO_RW_ETHERNET);
> > +
> > +	*val = header->data[0];
> > +	ack = header->ack;
> > +
> > +	mutex_unlock(&header->mdio_mutex);
> > +
> > +	if (ret <= 0)
> > +		return -ETIMEDOUT;
> > +
> > +	if (!ack)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static void qca8k_rw_reg_ack_handler(struct dsa_port *dp, struct sk_buff *skb)
> > +{
> > +	struct qca8k_port_tag *header = dp->priv;
> > +	struct mdio_ethhdr *mdio_ethhdr;
> > +
> > +	mdio_ethhdr = (struct mdio_ethhdr *)skb_mac_header(skb);
> > +
> > +	header->data[0] = mdio_ethhdr->mdio_data;
> > +
> > +	/* Get the rest of the 12 byte of data */
> > +	memcpy(header->data + 1, skb->data, QCA_HDR_MDIO_DATA2_LEN);
> > +
> > +	/* Make sure the seq match the requested packet */
> > +	if (mdio_ethhdr->seq == header->seq)
> > +		header->ack = true;
> > +
> > +	complete(&header->rw_done);
> > +}
> > +
> > +static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
> > +{
> > +	struct qca8k_port_tag *header = priv->header_mdio;
> > +	struct sk_buff *skb;
> > +	bool ack;
> > +	int ret;
> > +
> > +	skb = qca8k_alloc_mdio_header(header, MDIO_WRITE, reg, &val);
> > +	skb->dev = dsa_to_port(priv->ds, 0)->master;
> > +
> > +	mutex_lock(&header->mdio_mutex);
> > +
> > +	dev_queue_xmit(skb);
> > +
> > +	reinit_completion(&header->rw_done);
> > +	header->ack = false;
> > +	header->seq = 200;
> > +
> > +	ret = wait_for_completion_timeout(&header->rw_done, QCA8K_MDIO_RW_ETHERNET);
> > +
> > +	ack = header->ack;
> > +
> > +	mutex_unlock(&header->mdio_mutex);
> > +
> > +	if (ret <= 0)
> > +		return -ETIMEDOUT;
> > +
> > +	if (!ack)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int
> > +qca8k_regmap_update_bits_eth(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
> > +{
> > +	u32 val = 0;
> > +	int ret;
> > +
> > +	ret = qca8k_read_eth(priv, reg, &val);
> > +	if (ret)
> > +		return ret;
> > +
> > +	val &= ~mask;
> > +	val |= write_val;
> > +
> > +	return qca8k_write_eth(priv, reg, val);
> > +}
> > +
> >  static int
> >  qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> >  {
> >  	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
> > +	struct qca8k_port_tag *header_mdio;
> >  	struct mii_bus *bus = priv->bus;
> >  	u16 r1, r2, page;
> >  	int ret;
> >  
> > +	header_mdio = priv->header_mdio;
> > +
> > +	if (header_mdio)
> > +		if (!qca8k_read_eth(priv, reg, val))
> > +			return 0;
> > +
> >  	qca8k_split_addr(reg, &r1, &r2, &page);
> >  
> >  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> > @@ -197,10 +346,17 @@ static int
> >  qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
> >  {
> >  	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
> > +	struct qca8k_port_tag *header_mdio;
> >  	struct mii_bus *bus = priv->bus;
> >  	u16 r1, r2, page;
> >  	int ret;
> >  
> > +	header_mdio = priv->header_mdio;
> > +
> > +	if (header_mdio)
> > +		if (!qca8k_write_eth(priv, reg, val))
> > +			return 0;
> > +
> >  	qca8k_split_addr(reg, &r1, &r2, &page);
> >  
> >  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> > @@ -220,11 +376,18 @@ static int
> >  qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_val)
> >  {
> >  	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
> > +	struct qca8k_port_tag *header_mdio;
> >  	struct mii_bus *bus = priv->bus;
> >  	u16 r1, r2, page;
> >  	u32 val;
> >  	int ret;
> >  
> > +	header_mdio = priv->header_mdio;
> > +
> > +	if (header_mdio)
> > +		if (!qca8k_regmap_update_bits_eth(priv, reg, mask, write_val))
> > +			return 0;
> > +
> >  	qca8k_split_addr(reg, &r1, &r2, &page);
> >  
> >  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> > @@ -1223,8 +1386,13 @@ qca8k_setup(struct dsa_switch *ds)
> >  	 * Configure specific switch configuration for ports
> >  	 */
> >  	for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> > +		struct dsa_port *dp = dsa_to_port(ds, i);
> > +
> > +		/* Set the header_mdio to be accessible by the qca tagger */
> > +		dp->priv = priv->header_mdio;
> 
> Why are you setting up the dp->priv pointer from the switch driver?
> I though the whole point of qca_tag_connect is to set up that memory by
> itself.
>

Yes sorry. I forgot to remove this from the my testing buildroot to the
clean linux repo. In the testing this wasn't present just to make it
clear.

> > +
> >  		/* CPU port gets connected to all user ports of the switch */
> > -		if (dsa_is_cpu_port(ds, i)) {
> > +		if (dsa_port_is_cpu(dp)) {
> >  			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
> >  					QCA8K_PORT_LOOKUP_MEMBER, dsa_user_ports(ds));
> >  			if (ret)
> > @@ -1232,7 +1400,7 @@ qca8k_setup(struct dsa_switch *ds)
> >  		}
> >  
> >  		/* Individual user ports get connected to CPU port only */
> > -		if (dsa_is_user_port(ds, i)) {
> > +		if (dsa_port_is_user(dp)) {
> >  			ret = qca8k_rmw(priv, QCA8K_PORT_LOOKUP_CTRL(i),
> >  					QCA8K_PORT_LOOKUP_MEMBER,
> >  					BIT(cpu_port));
> > @@ -2382,6 +2550,38 @@ qca8k_port_lag_leave(struct dsa_switch *ds, int port,
> >  	return qca8k_lag_refresh_portmap(ds, port, lag, true);
> >  }
> >  
> > +static int qca8k_tag_proto_connect(struct dsa_switch *ds,
> > +				   const struct dsa_device_ops *tag_ops)
> > +{
> > +	struct qca8k_priv *qca8k_priv = ds->priv;
> > +	struct qca8k_port_tag *priv;
> > +	struct dsa_port *dp;
> > +	int i;
> > +
> > +	switch (tag_ops->proto) {
> > +	case DSA_TAG_PROTO_QCA:
> > +		for (i = 0; i < QCA8K_NUM_PORTS; i++) {
> > +			dp = dsa_to_port(ds, i);
> > +			priv = dp->priv;
> > +
> > +			if (!dsa_port_is_cpu(dp))
> > +				continue;
> > +
> > +			mutex_init(&priv->mdio_mutex);
> > +			init_completion(&priv->rw_done);
> > +			/* Cache the header mdio */
> > +			qca8k_priv->header_mdio = priv;
> > +			priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
> > +		}
> > +
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static const struct dsa_switch_ops qca8k_switch_ops = {
> >  	.get_tag_protocol	= qca8k_get_tag_protocol,
> >  	.setup			= qca8k_setup,
> > @@ -2417,6 +2617,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
> >  	.get_phy_flags		= qca8k_get_phy_flags,
> >  	.port_lag_join		= qca8k_port_lag_join,
> >  	.port_lag_leave		= qca8k_port_lag_leave,
> > +	.tag_proto_connect	= qca8k_tag_proto_connect,
> >  };
> >  
> >  static int qca8k_read_switch_id(struct qca8k_priv *priv)
> > @@ -2452,6 +2653,7 @@ static int qca8k_read_switch_id(struct qca8k_priv *priv)
> >  static int
> >  qca8k_sw_probe(struct mdio_device *mdiodev)
> >  {
> > +	struct qca8k_port_tag *header_mdio;
> >  	struct qca8k_priv *priv;
> >  	int ret;
> >  
> > @@ -2462,6 +2664,13 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
> >  	if (!priv)
> >  		return -ENOMEM;
> >  
> > +	header_mdio = devm_kzalloc(&mdiodev->dev, sizeof(*header_mdio), GFP_KERNEL);
> > +	if (!header_mdio)
> > +		return -ENOMEM;
> > +
> > +	mutex_init(&header_mdio->mdio_mutex);
> > +	init_completion(&header_mdio->rw_done);
> > +
> >  	priv->bus = mdiodev->bus;
> >  	priv->dev = &mdiodev->dev;
> >  
> > @@ -2501,6 +2710,7 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
> >  	priv->ds->priv = priv;
> >  	priv->ops = qca8k_switch_ops;
> >  	priv->ds->ops = &priv->ops;
> > +	priv->header_mdio = header_mdio;
> >  	mutex_init(&priv->reg_mutex);
> >  	dev_set_drvdata(&mdiodev->dev, priv);
> >  
> > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> > index ab4a417b25a9..25aa1509e0c0 100644
> > --- a/drivers/net/dsa/qca8k.h
> > +++ b/drivers/net/dsa/qca8k.h
> > @@ -11,6 +11,9 @@
> >  #include <linux/delay.h>
> >  #include <linux/regmap.h>
> >  #include <linux/gpio.h>
> > +#include <linux/dsa/tag_qca.h>
> > +
> > +#define QCA8K_MDIO_RW_ETHERNET				100
> >  
> >  #define QCA8K_NUM_PORTS					7
> >  #define QCA8K_NUM_CPU_PORTS				2
> > @@ -353,6 +356,7 @@ struct qca8k_priv {
> >  	struct dsa_switch_ops ops;
> >  	struct gpio_desc *reset_gpio;
> >  	unsigned int port_mtu[QCA8K_NUM_PORTS];
> > +	struct qca8k_port_tag *header_mdio;
> >  };
> >  
> >  struct qca8k_mib_desc {
> > diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
> > index f403fdab6f29..2b823d971ae9 100644
> > --- a/include/linux/dsa/tag_qca.h
> > +++ b/include/linux/dsa/tag_qca.h
> > @@ -59,11 +59,21 @@ struct mdio_ethhdr {
> >  	u16 hdr;		/* qca hdr */
> >  } __packed;
> >  
> > +enum mdio_cmd {
> > +	MDIO_WRITE = 0x0,
> > +	MDIO_READ
> > +};
> > +
> >  struct qca8k_port_tag {
> >  	void (*rw_reg_ack_handler)(struct dsa_port *dp,
> >  				   struct sk_buff *skb);
> >  	void (*mib_autocast_handler)(struct dsa_port *dp,
> >  				     struct sk_buff *skb);
> > +	struct completion rw_done;
> > +	struct mutex mdio_mutex; /* Enforce one mdio read/write at time */
> > +	bool ack;
> > +	u32 seq;
> > +	u32 data[4];
> 
> None of these structures need to stay in the data structure shared with
> the tagger. They can be in qca8k_priv. The tagger should only see the
> function pointers. It doesn't care what is done with the packets.
> 
> >  };

Ok so the handler should access these data by reaching the qca8k_priv
from the dsa port. Correct?

> >  
> >  #endif /* __TAG_QCA_H */
> > -- 
> > 2.32.0
> > 

-- 
	Ansuel

Re: [net-next RFC PATCH v2 0/8] Add support for qca8k mdio rw in Ethernet packet

[Ansuel Smith]

On Wed, Dec 08, 2021 at 02:32:22PM +0200, Vladimir Oltean wrote:
> On Wed, Dec 08, 2021 at 04:40:32AM +0100, Ansuel Smith wrote:
> > I still have to find a solution to a slowdown problem and this is where
> > I would love to get some hint.
> > Currently I still didn't find a good way to understand when the tagger
> > starts to accept packets and because of this the initial setup is slow
> > as every completion timeouts. Am I missing something or is there a way
> > to check for this?
> > After the initial slowdown, as soon as the cpu port is ready and starts
> > to accept packet, every transaction is near instant and no completion
> > timeouts.
> 
> My guess is that the problem with the initial slowdown is that you try
> to use the Ethernet based register access before things are set up:
> before the master is up and ready, before the switch is minimally set
> up, etc.
> 
> I think what this Ethernet-based register access technique needs to be
> more reliable is a notification about the DSA master going up or down.
> Otherwise it won't be very efficient at all, to wait for every single
> Ethernet access attempt to time out before attempting a direct MDIO
> access.
>

Yes that is the main problem. My idea would be a notification fired as
soon as the tagger starts to send/process packet. That way we should be
certain that Ethernet mdio is ready. (then use a bool to comunicate
that the tagger is ready? And a dsa driver would use that or a helper to
understand what is the correct I/O path to use? I would love to remove
all these extra check and make something more direct but I think it
would spam the dsa ops even more)

The timeout has to stay anyway to prevent any type of breakage by the
Ethernet mdio not working.

> But there are some problems with offering a "master_going_up/master_going_down"
> set of callbacks. Specifically, we could easily hook into the NETDEV_PRE_UP/
> NETDEV_GOING_DOWN netdev notifiers and transform these into DSA switch
> API calls. The goal would be for the qca8k tagger to mark the
> Ethernet-based register access method as available/unavailable, and in
> the regmap implementation, to use that or the other. DSA would then also
> be responsible for calling "master_going_up" when the switch ports and
> master are sufficiently initialized that traffic should be possible.
> But that first "master_going_up" notification is in fact the most
> problematic one, because we may not receive a NETDEV_PRE_UP event,
> because the DSA master may already be up when we probe our switch tree.
> This would be a bit finicky to get right. We may, for instance, hold
> rtnl_lock for the entirety of dsa_tree_setup_master(). This will block
> potentially concurrent netdevice notifiers handled by dsa_slave_nb.
> And while holding rtnl_lock() and immediately after each dsa_master_setup(),
> we may check whether master->flags & IFF_UP is true, and if it is,
> synthesize a call to ds->ops->master_going_up(). We also need to do the
> reverse in dsa_tree_teardown_master().

Should we care about holding the lock for that much time? Will do some
test hoping the IFF_UP is sufficient to make the Ethernet mdio work.

-- 
	Ansuel

Re: [net-next RFC PATCH v2 7/8] net: dsa: qca8k: Add support for mdio read/write in Ethernet packet

[Vladimir Oltean]

On Wed, Dec 08, 2021 at 03:21:11PM +0100, Ansuel Smith wrote:
> > None of these structures need to stay in the data structure shared with
> > the tagger. They can be in qca8k_priv. The tagger should only see the
> > function pointers. It doesn't care what is done with the packets.
> > 
> > >  };
> 
> Ok so the handler should access these data by reaching the qca8k_priv
> from the dsa port. Correct?

Yes, this is exactly what I think, the tagger should be stateless with
regard to the completion, if it can be stateless. Only with stuff
related to the tagging protocol itself it can be stateful, things like
sequence numbers if you need them, etc. But the MDIO access is payload
as far as the tagger is concerned.

Re: [net-next RFC PATCH v2 0/8] Add support for qca8k mdio rw in Ethernet packet

[Vladimir Oltean]

On Wed, Dec 08, 2021 at 03:33:27PM +0100, Ansuel Smith wrote:
> > But there are some problems with offering a "master_going_up/master_going_down"
> > set of callbacks. Specifically, we could easily hook into the NETDEV_PRE_UP/
> > NETDEV_GOING_DOWN netdev notifiers and transform these into DSA switch
> > API calls. The goal would be for the qca8k tagger to mark the
> > Ethernet-based register access method as available/unavailable, and in
> > the regmap implementation, to use that or the other. DSA would then also
> > be responsible for calling "master_going_up" when the switch ports and
> > master are sufficiently initialized that traffic should be possible.
> > But that first "master_going_up" notification is in fact the most
> > problematic one, because we may not receive a NETDEV_PRE_UP event,
> > because the DSA master may already be up when we probe our switch tree.
> > This would be a bit finicky to get right. We may, for instance, hold
> > rtnl_lock for the entirety of dsa_tree_setup_master(). This will block
> > potentially concurrent netdevice notifiers handled by dsa_slave_nb.
> > And while holding rtnl_lock() and immediately after each dsa_master_setup(),
> > we may check whether master->flags & IFF_UP is true, and if it is,
> > synthesize a call to ds->ops->master_going_up(). We also need to do the
> > reverse in dsa_tree_teardown_master().
> 
> Should we care about holding the lock for that much time? Will do some
> test hoping the IFF_UP is sufficient to make the Ethernet mdio work.

I'm certainly not smart enough to optimize things, so I'd rather hold
the rtnl_lock for as long as I'm comfortable is enough to avoid races.
The reason why we must hold rtnl_lock is because during
dsa_master_setup(), the value of netdev_uses_dsa(dp->master) changes
from false to true.
The idea is that if IFF_UP isn't set right now, no problem, release the
lock and we'll catch the NETDEV_UP notifier when that will appear.
But we want to
(a) replay the master up state if it was already up while it wasn't a
    DSA master
(b) avoid a potential race where the master does go up, we receive that
    notification, but netdev_uses_dsa() doesn't yet return true for it.

The model would be similar to what we have for the NETDEV_GOING_DOWN
handler.

Please wait for me to finish the sja1105 conversion. There are some
issues I've noticed in your connect/disconnect implementation that I
haven't had a chance to comment on, yet. I've tested ocelot-8021q plus
the tagging protocol change and these appear fine.
I'd like to post the changes I have, to make sure that what works for me
works for you, and what works for you works for me. I may also have some
patches laying around that track the master up/down state (I needed
those for some RFC DSA master change patches). I'll build a mini patch
series and post it soon-ish.

Re: [net-next RFC PATCH v2 0/8] Add support for qca8k mdio rw in Ethernet packet

[Ansuel Smith]

On Wed, Dec 08, 2021 at 04:53:41PM +0200, Vladimir Oltean wrote:
> On Wed, Dec 08, 2021 at 03:33:27PM +0100, Ansuel Smith wrote:
> > > But there are some problems with offering a "master_going_up/master_going_down"
> > > set of callbacks. Specifically, we could easily hook into the NETDEV_PRE_UP/
> > > NETDEV_GOING_DOWN netdev notifiers and transform these into DSA switch
> > > API calls. The goal would be for the qca8k tagger to mark the
> > > Ethernet-based register access method as available/unavailable, and in
> > > the regmap implementation, to use that or the other. DSA would then also
> > > be responsible for calling "master_going_up" when the switch ports and
> > > master are sufficiently initialized that traffic should be possible.
> > > But that first "master_going_up" notification is in fact the most
> > > problematic one, because we may not receive a NETDEV_PRE_UP event,
> > > because the DSA master may already be up when we probe our switch tree.
> > > This would be a bit finicky to get right. We may, for instance, hold
> > > rtnl_lock for the entirety of dsa_tree_setup_master(). This will block
> > > potentially concurrent netdevice notifiers handled by dsa_slave_nb.
> > > And while holding rtnl_lock() and immediately after each dsa_master_setup(),
> > > we may check whether master->flags & IFF_UP is true, and if it is,
> > > synthesize a call to ds->ops->master_going_up(). We also need to do the
> > > reverse in dsa_tree_teardown_master().
> > 
> > Should we care about holding the lock for that much time? Will do some
> > test hoping the IFF_UP is sufficient to make the Ethernet mdio work.
> 
> I'm certainly not smart enough to optimize things, so I'd rather hold
> the rtnl_lock for as long as I'm comfortable is enough to avoid races.
> The reason why we must hold rtnl_lock is because during
> dsa_master_setup(), the value of netdev_uses_dsa(dp->master) changes
> from false to true.
> The idea is that if IFF_UP isn't set right now, no problem, release the
> lock and we'll catch the NETDEV_UP notifier when that will appear.
> But we want to
> (a) replay the master up state if it was already up while it wasn't a
>     DSA master
> (b) avoid a potential race where the master does go up, we receive that
>     notification, but netdev_uses_dsa() doesn't yet return true for it.
> 
> The model would be similar to what we have for the NETDEV_GOING_DOWN
> handler.
> 
> Please wait for me to finish the sja1105 conversion. There are some
> issues I've noticed in your connect/disconnect implementation that I
> haven't had a chance to comment on, yet. I've tested ocelot-8021q plus
> the tagging protocol change and these appear fine.
> I'd like to post the changes I have, to make sure that what works for me
> works for you, and what works for you works for me. I may also have some
> patches laying around that track the master up/down state (I needed
> those for some RFC DSA master change patches). I'll build a mini patch
> series and post it soon-ish.

Sure no problem. In the meantime we can also wait if Andrew notice other
problem with this new implementation. (I will work on the mib handler as
it's still wip and on fixing the error on the last patch)

-- 
	Ansuel

Re: [net-next RFC PATCH v2 0/8] Add support for qca8k mdio rw in Ethernet packet

[Ansuel Smith]

On Wed, Dec 08, 2021 at 04:53:41PM +0200, Vladimir Oltean wrote:
> On Wed, Dec 08, 2021 at 03:33:27PM +0100, Ansuel Smith wrote:
> > > But there are some problems with offering a "master_going_up/master_going_down"
> > > set of callbacks. Specifically, we could easily hook into the NETDEV_PRE_UP/
> > > NETDEV_GOING_DOWN netdev notifiers and transform these into DSA switch
> > > API calls. The goal would be for the qca8k tagger to mark the
> > > Ethernet-based register access method as available/unavailable, and in
> > > the regmap implementation, to use that or the other. DSA would then also
> > > be responsible for calling "master_going_up" when the switch ports and
> > > master are sufficiently initialized that traffic should be possible.
> > > But that first "master_going_up" notification is in fact the most
> > > problematic one, because we may not receive a NETDEV_PRE_UP event,
> > > because the DSA master may already be up when we probe our switch tree.
> > > This would be a bit finicky to get right. We may, for instance, hold
> > > rtnl_lock for the entirety of dsa_tree_setup_master(). This will block
> > > potentially concurrent netdevice notifiers handled by dsa_slave_nb.
> > > And while holding rtnl_lock() and immediately after each dsa_master_setup(),
> > > we may check whether master->flags & IFF_UP is true, and if it is,
> > > synthesize a call to ds->ops->master_going_up(). We also need to do the
> > > reverse in dsa_tree_teardown_master().
> > 
> > Should we care about holding the lock for that much time? Will do some
> > test hoping the IFF_UP is sufficient to make the Ethernet mdio work.
> 
> I'm certainly not smart enough to optimize things, so I'd rather hold
> the rtnl_lock for as long as I'm comfortable is enough to avoid races.
> The reason why we must hold rtnl_lock is because during
> dsa_master_setup(), the value of netdev_uses_dsa(dp->master) changes
> from false to true.
> The idea is that if IFF_UP isn't set right now, no problem, release the
> lock and we'll catch the NETDEV_UP notifier when that will appear.
> But we want to
> (a) replay the master up state if it was already up while it wasn't a
>     DSA master
> (b) avoid a potential race where the master does go up, we receive that
>     notification, but netdev_uses_dsa() doesn't yet return true for it.
> 
> The model would be similar to what we have for the NETDEV_GOING_DOWN
> handler.
> 
> Please wait for me to finish the sja1105 conversion. There are some
> issues I've noticed in your connect/disconnect implementation that I
> haven't had a chance to comment on, yet. I've tested ocelot-8021q plus
> the tagging protocol change and these appear fine.
> I'd like to post the changes I have, to make sure that what works for me
> works for you, and what works for you works for me. I may also have some
> patches laying around that track the master up/down state (I needed
> those for some RFC DSA master change patches). I'll build a mini patch
> series and post it soon-ish.

Anyway just as an info, I implemented also the mib handler and it does
work correctly. Vladimir implementation works just good and it seems
pretty clean with the tagger used only to implement stuff and the driver
to do the ""dirty"" work.

But anyway for this kind of transaction, we really need a way to track
when it's possible to use it. We need to polish that. Just to not mix
things I will had my comments in the other series about tracking master
up.

-- 
	Ansuel