[net-next PATCH RFC v6 00/16] Add support for qca8k mdio rw in Ethernet packet

[net-next PATCH RFC v6 00/16] Add support for qca8k mdio rw in Ethernet packet

[Ansuel Smith]

Hi, this is ready but require some additional test on a wider userbase.

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.

Currently we use Ethernet packet for
- MIB counter
- mdio read/write configuration
- phy read/write for each port

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.

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.

The tagger priv implement only the handler for the special packet. All the
other stuff is placed in the qca8k_priv and the tagger has to access
it under lock.

We use the new API from Vladimir to track if the master port is
operational or not. We had to track many thing to reach a usable state.
Checking if the port is UP is not enough and tracking a NETDEV_CHANGE is
also not enough since it use also for other task. The correct way was
both track for interface UP and if a qdisc was assigned to the
interface. That tells us the port (and the tagger indirectly) is ready
to accept and process packet.

I tested this with multicpu port and with port6 set as the unique port and
it's sad.
It seems they implemented this feature in a bad way and this is only
supported with cpu port0. When cpu port6 is the unique port, the switch
doesn't send ack packet. With multicpu port, packet ack are not duplicated
and only cpu port0 sends them. This is the same for the MIB counter.
For this reason this feature is enabled only when cpu port0 is enabled and
operational.

Current concern are:
- Any hint about the naming? Is calling this mdio Ethernet correct?
  Should we use a more ""standard""/significant name? (considering also
  other switch will implement this)

v6:
- Fix some error in ethtool handler caused by rebase/cleanup
v5:
- Adapt to new API fixes
- Fix a wrong logic for noop
- Add additional lock for master_state change
- Limit mdio Ethernet to cpu port0 (switch limitation)
- Add priority to these special packet
- Move mdio cache to qca8k_priv
v4:
- Remove duplicate patch sent by mistake.
v3:
- Include MIB with Ethernet packet.
- Include phy read/write with Ethernet packet.
- Reorganize code with new API.
- Introuce master tracking by Vladimir
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 (12):
  net: dsa: tag_qca: convert to FIELD macro
  net: dsa: tag_qca: move define to include linux/dsa
  net: dsa: tag_qca: enable promisc_on_master flag
  net: dsa: tag_qca: add define for handling mdio Ethernet packet
  net: dsa: tag_qca: add define for handling MIB packet
  net: dsa: tag_qca: add support for handling mdio Ethernet and MIB
    packet
  net: dsa: qca8k: add tracking state of master port
  net: dsa: qca8k: add support for mdio read/write in Ethernet packet
  net: dsa: qca8k: add support for mib autocast in Ethernet packet
  net: dsa: qca8k: add support for phy read/write with mdio Ethernet
  net: dsa: qca8k: move page cache to driver priv
  net: dsa: qca8k: cache lo and hi for mdio write

Vladimir Oltean (4):
  net: dsa: provide switch operations for tracking the master state
  net: dsa: stop updating master MTU from master.c
  net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
  net: dsa: replay master state events in
    dsa_tree_{setup,teardown}_master

 drivers/net/dsa/qca8k.c     | 600 ++++++++++++++++++++++++++++++++++--
 drivers/net/dsa/qca8k.h     |  46 ++-
 include/linux/dsa/tag_qca.h |  79 +++++
 include/net/dsa.h           |  17 +
 net/dsa/dsa2.c              |  81 ++++-
 net/dsa/dsa_priv.h          |  13 +
 net/dsa/master.c            |  29 +-
 net/dsa/slave.c             |  32 ++
 net/dsa/switch.c            |  15 +
 net/dsa/tag_qca.c           |  81 +++--
 10 files changed, 901 insertions(+), 92 deletions(-)
 create mode 100644 include/linux/dsa/tag_qca.h

-- 
2.33.1

[net-next PATCH RFC v6 01/16] net: dsa: provide switch operations for tracking the master state

[Ansuel Smith]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

Certain drivers may need to send management traffic to the switch for
things like register access, FDB dump, etc, to accelerate what their
slow bus (SPI, I2C, MDIO) can already do.

Ethernet is faster (especially in bulk transactions) but is also more
unreliable, since the user may decide to bring the DSA master down (or
not bring it up), therefore severing the link between the host and the
attached switch.

Drivers needing Ethernet-based register access already should have
fallback logic to the slow bus if the Ethernet method fails, but that
fallback may be based on a timeout, and the I/O to the switch may slow
down to a halt if the master is down, because every Ethernet packet will
have to time out. The driver also doesn't have the option to turn off
Ethernet-based I/O momentarily, because it wouldn't know when to turn it
back on.

Which is where this change comes in. By tracking NETDEV_CHANGE,
NETDEV_UP and NETDEV_GOING_DOWN events on the DSA master, we should know
the exact interval of time during which this interface is reliably
available for traffic. Provide this information to switches so they can
use it as they wish.

An helper is added dsa_port_master_is_operational() to check if a master
port is operational.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 include/net/dsa.h  | 17 +++++++++++++++++
 net/dsa/dsa2.c     | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 net/dsa/dsa_priv.h | 13 +++++++++++++
 net/dsa/slave.c    | 32 ++++++++++++++++++++++++++++++++
 net/dsa/switch.c   | 15 +++++++++++++++
 5 files changed, 123 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index f16959444ae1..70a1f21e4473 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -292,6 +292,10 @@ struct dsa_port {
 	struct list_head	fdbs;
 	struct list_head	mdbs;
 
+	/* Master state bits, valid only on CPU ports */
+	u8 master_admin_up:1,
+	   master_oper_up:1;
+
 	bool setup;
 };
 
@@ -458,6 +462,12 @@ static inline bool dsa_port_is_unused(struct dsa_port *dp)
 	return dp->type == DSA_PORT_TYPE_UNUSED;
 }
 
+static inline bool dsa_port_master_is_operational(struct dsa_port *dp)
+{
+	return dsa_port_is_cpu(dp) && dp->master_admin_up &&
+	       dp->master_oper_up;
+}
+
 static inline bool dsa_is_unused_port(struct dsa_switch *ds, int p)
 {
 	return dsa_to_port(ds, p)->type == DSA_PORT_TYPE_UNUSED;
@@ -1016,6 +1026,13 @@ 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);
+
+	/*
+	 * DSA master tracking operations
+	 */
+	void	(*master_state_change)(struct dsa_switch *ds,
+				       const struct net_device *master,
+				       bool operational);
 };
 
 #define DSA_DEVLINK_PARAM_DRIVER(_id, _name, _type, _cmodes)		\
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index c18b22c0bf55..eb466e92069c 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1244,6 +1244,52 @@ int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
 	return err;
 }
 
+static void dsa_tree_master_state_change(struct dsa_switch_tree *dst,
+					 struct net_device *master)
+{
+	struct dsa_notifier_master_state_info info;
+	struct dsa_port *cpu_dp = master->dsa_ptr;
+
+	info.master = master;
+	info.operational = dsa_port_master_is_operational(cpu_dp);
+
+	dsa_tree_notify(dst, DSA_NOTIFIER_MASTER_STATE_CHANGE, &info);
+}
+
+void dsa_tree_master_admin_state_change(struct dsa_switch_tree *dst,
+					struct net_device *master,
+					bool up)
+{
+	struct dsa_port *cpu_dp = master->dsa_ptr;
+	bool notify = false;
+
+	if ((dsa_port_master_is_operational(cpu_dp)) !=
+	    (up && cpu_dp->master_oper_up))
+		notify = true;
+
+	cpu_dp->master_admin_up = up;
+
+	if (notify)
+		dsa_tree_master_state_change(dst, master);
+}
+
+void dsa_tree_master_oper_state_change(struct dsa_switch_tree *dst,
+				       struct net_device *master,
+				       bool up)
+{
+	struct dsa_port *cpu_dp = master->dsa_ptr;
+	bool notify = false;
+
+	if ((dsa_port_master_is_operational(cpu_dp)) !=
+	    (cpu_dp->master_admin_up && up))
+		notify = true;
+
+	cpu_dp->master_oper_up = up;
+
+	if (notify)
+		dsa_tree_master_state_change(dst, master);
+}
+
 static struct dsa_port *dsa_port_touch(struct dsa_switch *ds, int index)
 {
 	struct dsa_switch_tree *dst = ds->dst;
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index edfaae7b5967..1566064ccfe2 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -45,6 +45,7 @@ enum {
 	DSA_NOTIFIER_MRP_DEL_RING_ROLE,
 	DSA_NOTIFIER_TAG_8021Q_VLAN_ADD,
 	DSA_NOTIFIER_TAG_8021Q_VLAN_DEL,
+	DSA_NOTIFIER_MASTER_STATE_CHANGE,
 };
 
 /* DSA_NOTIFIER_AGEING_TIME */
@@ -128,6 +129,12 @@ struct dsa_notifier_tag_8021q_vlan_info {
 	u16 vid;
 };
 
+/* DSA_NOTIFIER_MASTER_STATE_CHANGE */
+struct dsa_notifier_master_state_info {
+	const struct net_device *master;
+	bool operational;
+};
+
 struct dsa_switchdev_event_work {
 	struct dsa_switch *ds;
 	int port;
@@ -508,6 +515,12 @@ int dsa_tree_change_tag_proto(struct dsa_switch_tree *dst,
 			      struct net_device *master,
 			      const struct dsa_device_ops *tag_ops,
 			      const struct dsa_device_ops *old_tag_ops);
+void dsa_tree_master_admin_state_change(struct dsa_switch_tree *dst,
+					struct net_device *master,
+					bool up);
+void dsa_tree_master_oper_state_change(struct dsa_switch_tree *dst,
+				       struct net_device *master,
+				       bool up);
 unsigned int dsa_bridge_num_get(const struct net_device *bridge_dev, int max);
 void dsa_bridge_num_put(const struct net_device *bridge_dev,
 			unsigned int bridge_num);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 88f7b8686dac..9dfcfc5dae0a 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -2348,6 +2348,36 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
 		err = dsa_port_lag_change(dp, info->lower_state_info);
 		return notifier_from_errno(err);
 	}
+	case NETDEV_CHANGE:
+	case NETDEV_UP: {
+		/* Track state of master port.
+		 * DSA driver may require the master port (and indirectly
+		 * the tagger) to be available for some special operation.
+		 */
+		if (netdev_uses_dsa(dev)) {
+			struct dsa_port *cpu_dp = dev->dsa_ptr;
+			struct dsa_switch_tree *dst = cpu_dp->ds->dst;
+
+			/* Track when the master port is UP */
+			dsa_tree_master_oper_state_change(dst, dev,
+							  netif_oper_up(dev));
+
+			/* Track when the master port is ready and can accept
+			 * packet.
+			 * NETDEV_UP event is not enough to flag a port as ready.
+			 * We also have to wait for linkwatch_do_dev to dev_activate
+			 * and emit a NETDEV_CHANGE event.
+			 * We check if a master port is ready by checking if the dev
+			 * have a qdisc assigned and is not noop.
+			 */
+			dsa_tree_master_admin_state_change(dst, dev,
+							   !qdisc_tx_is_noop(dev));
+
+			return NOTIFY_OK;
+		}
+
+		return NOTIFY_DONE;
+	}
 	case NETDEV_GOING_DOWN: {
 		struct dsa_port *dp, *cpu_dp;
 		struct dsa_switch_tree *dst;
@@ -2359,6 +2389,8 @@ static int dsa_slave_netdevice_event(struct notifier_block *nb,
 		cpu_dp = dev->dsa_ptr;
 		dst = cpu_dp->ds->dst;
 
+		dsa_tree_master_admin_state_change(dst, dev, false);
+
 		list_for_each_entry(dp, &dst->ports, list) {
 			if (!dsa_port_is_user(dp))
 				continue;
diff --git a/net/dsa/switch.c b/net/dsa/switch.c
index 393f2d8a860a..726bb5ca1183 100644
--- a/net/dsa/switch.c
+++ b/net/dsa/switch.c
@@ -753,6 +753,18 @@ dsa_switch_mrp_del_ring_role(struct dsa_switch *ds,
 	return 0;
 }
 
+static int
+dsa_switch_master_state_change(struct dsa_switch *ds,
+			       struct dsa_notifier_master_state_info *info)
+{
+	if (!ds->ops->master_state_change)
+		return 0;
+
+	ds->ops->master_state_change(ds, info->master, info->operational);
+
+	return 0;
+}
+
 static int dsa_switch_event(struct notifier_block *nb,
 			    unsigned long event, void *info)
 {
@@ -844,6 +856,9 @@ static int dsa_switch_event(struct notifier_block *nb,
 	case DSA_NOTIFIER_TAG_8021Q_VLAN_DEL:
 		err = dsa_switch_tag_8021q_vlan_del(ds, info);
 		break;
+	case DSA_NOTIFIER_MASTER_STATE_CHANGE:
+		err = dsa_switch_master_state_change(ds, info);
+		break;
 	default:
 		err = -EOPNOTSUPP;
 		break;
-- 
2.33.1

[net-next PATCH RFC v6 02/16] net: dsa: stop updating master MTU from master.c

[Ansuel Smith]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The dev_set_mtu() call from dsa_master_setup() has been effectively
superseded by the dsa_slave_change_mtu(slave_dev, ETH_DATA_LEN) that is
done from dsa_slave_create() for each user port. This function also
updates the master MTU according to the largest user port MTU from the
tree. Therefore, updating the master MTU through a separate code path
isn't needed.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/master.c | 25 +------------------------
 1 file changed, 1 insertion(+), 24 deletions(-)

diff --git a/net/dsa/master.c b/net/dsa/master.c
index e8e19857621b..f4efb244f91d 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -330,28 +330,13 @@ static const struct attribute_group dsa_group = {
 	.attrs	= dsa_slave_attrs,
 };
 
-static void dsa_master_reset_mtu(struct net_device *dev)
-{
-	int err;
-
-	rtnl_lock();
-	err = dev_set_mtu(dev, ETH_DATA_LEN);
-	if (err)
-		netdev_dbg(dev,
-			   "Unable to reset MTU to exclude DSA overheads\n");
-	rtnl_unlock();
-}
-
 static struct lock_class_key dsa_master_addr_list_lock_key;
 
 int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
 {
-	const struct dsa_device_ops *tag_ops = cpu_dp->tag_ops;
 	struct dsa_switch *ds = cpu_dp->ds;
 	struct device_link *consumer_link;
-	int mtu, ret;
-
-	mtu = ETH_DATA_LEN + dsa_tag_protocol_overhead(tag_ops);
+	int ret;
 
 	/* The DSA master must use SET_NETDEV_DEV for this to work. */
 	consumer_link = device_link_add(ds->dev, dev->dev.parent,
@@ -361,13 +346,6 @@ int dsa_master_setup(struct net_device *dev, struct dsa_port *cpu_dp)
 			   "Failed to create a device link to DSA switch %s\n",
 			   dev_name(ds->dev));
 
-	rtnl_lock();
-	ret = dev_set_mtu(dev, mtu);
-	rtnl_unlock();
-	if (ret)
-		netdev_warn(dev, "error %d setting MTU to %d to include DSA overhead\n",
-			    ret, mtu);
-
 	/* If we use a tagging format that doesn't have an ethertype
 	 * field, make sure that all packets from this point on get
 	 * sent to the tag format's receive function.
@@ -405,7 +383,6 @@ void dsa_master_teardown(struct net_device *dev)
 	sysfs_remove_group(&dev->dev.kobj, &dsa_group);
 	dsa_netdev_ops_set(dev, NULL);
 	dsa_master_ethtool_teardown(dev);
-	dsa_master_reset_mtu(dev);
 	dsa_master_set_promiscuity(dev, -1);
 
 	dev->dsa_ptr = NULL;
-- 
2.33.1

[net-next PATCH RFC v6 03/16] net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}

[Ansuel Smith]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

DSA needs to simulate master tracking events when a binding is first
with a DSA master established and torn down, in order to give drivers
the simplifying guarantee that ->master_state_change calls are made
only when the master's readiness state to pass traffic changes.
master_state_change() provide a operational bool that DSA driver can use
to understand if DSA master is operational or not.
To avoid races, we need to block the reception of
NETDEV_UP/NETDEV_CHANGE/NETDEV_GOING_DOWN events in the netdev notifier
chain while we are changing the master's dev->dsa_ptr (this changes what
netdev_uses_dsa(dev) reports).

The dsa_master_setup() and dsa_master_teardown() functions optionally
require the rtnl_mutex to be held, if the tagger needs the master to be
promiscuous, these functions call dev_set_promiscuity(). Move the
rtnl_lock() from that function and make it top-level.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/dsa2.c   | 8 ++++++++
 net/dsa/master.c | 4 ++--
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index eb466e92069c..e8b56c84a417 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1038,6 +1038,8 @@ static int dsa_tree_setup_master(struct dsa_switch_tree *dst)
 	struct dsa_port *dp;
 	int err;
 
+	rtnl_lock();
+
 	list_for_each_entry(dp, &dst->ports, list) {
 		if (dsa_port_is_cpu(dp)) {
 			err = dsa_master_setup(dp->master, dp);
@@ -1046,6 +1048,8 @@ static int dsa_tree_setup_master(struct dsa_switch_tree *dst)
 		}
 	}
 
+	rtnl_unlock();
+
 	return 0;
 }
 
@@ -1053,9 +1057,13 @@ static void dsa_tree_teardown_master(struct dsa_switch_tree *dst)
 {
 	struct dsa_port *dp;
 
+	rtnl_lock();
+
 	list_for_each_entry(dp, &dst->ports, list)
 		if (dsa_port_is_cpu(dp))
 			dsa_master_teardown(dp->master);
+
+	rtnl_unlock();
 }
 
 static int dsa_tree_setup_lags(struct dsa_switch_tree *dst)
diff --git a/net/dsa/master.c b/net/dsa/master.c
index f4efb244f91d..2199104ca7df 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -267,9 +267,9 @@ static void dsa_master_set_promiscuity(struct net_device *dev, int inc)
 	if (!ops->promisc_on_master)
 		return;
 
-	rtnl_lock();
+	ASSERT_RTNL();
+
 	dev_set_promiscuity(dev, inc);
-	rtnl_unlock();
 }
 
 static ssize_t tagging_show(struct device *d, struct device_attribute *attr,
-- 
2.33.1

[net-next PATCH RFC v6 04/16] net: dsa: replay master state events in dsa_tree_{setup,teardown}_master

[Ansuel Smith]

From: Vladimir Oltean <vladimir.oltean@nxp.com>

In order for switch driver to be able to make simple and reliable use of
the master tracking operations, they must also be notified of the
initial state of the DSA master, not just of the changes. This is
because they might enable certain features only during the time when
they know that the DSA master is up and running.

Therefore, this change explicitly checks the state of the DSA master
under the same rtnl_mutex as we were holding during the
dsa_master_setup() and dsa_master_teardown() call. The idea being that
if the DSA master became operational in between the moment in which it
became a DSA master (dsa_master_setup set dev->dsa_ptr) and the moment
when we checked for the master being up, there is a chance that we
would emit a ->master_state_change() call with no actual state change.
We need to avoid that by serializing the concurrent netdevice event with
us. If the netdevice event started before, we force it to finish before
we begin, because we take rtnl_lock before making netdev_uses_dsa()
return true. So we also handle that early event and do nothing on it.
Similarly, if the dev_open() attempt is concurrent with us, it will
attempt to take the rtnl_mutex, but we're holding it. We'll see that
the master flag IFF_UP isn't set, then when we release the rtnl_mutex
we'll process the NETDEV_UP notifier.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 net/dsa/dsa2.c | 27 +++++++++++++++++++++++----
 1 file changed, 23 insertions(+), 4 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index e8b56c84a417..4ab6d1df2b19 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -1042,9 +1042,18 @@ static int dsa_tree_setup_master(struct dsa_switch_tree *dst)
 
 	list_for_each_entry(dp, &dst->ports, list) {
 		if (dsa_port_is_cpu(dp)) {
-			err = dsa_master_setup(dp->master, dp);
+			struct net_device *master = dp->master;
+			bool admin_up = (master->flags & IFF_UP) &&
+					!qdisc_tx_is_noop(master);
+
+			err = dsa_master_setup(master, dp);
 			if (err)
 				return err;
+
+			/* Replay master state event */
+			dsa_tree_master_admin_state_change(dst, master, admin_up);
+			dsa_tree_master_oper_state_change(dst, master,
+							  netif_oper_up(master));
 		}
 	}
 
@@ -1059,9 +1068,19 @@ static void dsa_tree_teardown_master(struct dsa_switch_tree *dst)
 
 	rtnl_lock();
 
-	list_for_each_entry(dp, &dst->ports, list)
-		if (dsa_port_is_cpu(dp))
-			dsa_master_teardown(dp->master);
+	list_for_each_entry(dp, &dst->ports, list) {
+		if (dsa_port_is_cpu(dp)) {
+			struct net_device *master = dp->master;
+
+			/* Synthesizing an "admin down" state is sufficient for
+			 * the switches to get a notification if the master is
+			 * currently up and running.
+			 */
+			dsa_tree_master_admin_state_change(dst, master, false);
+
+			dsa_master_teardown(master);
+		}
+	}
 
 	rtnl_unlock();
 }
-- 
2.33.1

[net-next PATCH RFC v6 05/16] 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.33.1

[net-next PATCH RFC v6 06/16] 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.33.1

[net-next PATCH RFC v6 07/16] net: dsa: tag_qca: enable promisc_on_master flag

[Ansuel Smith]

Ethernet MDIO packets are non-standard and DSA master expects the first
6 octets to be the MAC DA. To address these kind of packet, enable
promisc_on_master flag for the tagger.

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

diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index 34e565e00ece..f8df49d5956f 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -68,6 +68,7 @@ static const struct dsa_device_ops qca_netdev_ops = {
 	.xmit	= qca_tag_xmit,
 	.rcv	= qca_tag_rcv,
 	.needed_headroom = QCA_HDR_LEN,
+	.promisc_on_master = true,
 };
 
 MODULE_LICENSE("GPL");
-- 
2.33.1

[net-next PATCH RFC v6 08/16] net: dsa: tag_qca: add define for handling mdio 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..21cd0db5acc2 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 */
+	__be16 hdr;		/* qca hdr */
+} __packed;
+
 #endif /* __TAG_QCA_H */
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index f8df49d5956f..d30249b5205d 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);
+
+	/* Ethernet 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.33.1

[net-next PATCH RFC v6 09/16] net: dsa: tag_qca: add define for handling MIB packet

[Ansuel Smith]

Add struct to correctly parse a mib Ethernet packet.

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

diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
index 21cd0db5acc2..cd6275bac103 100644
--- a/include/linux/dsa/tag_qca.h
+++ b/include/linux/dsa/tag_qca.h
@@ -59,4 +59,14 @@ struct mdio_ethhdr {
 	__be16 hdr;		/* qca hdr */
 } __packed;
 
+enum mdio_cmd {
+	MDIO_WRITE = 0x0,
+	MDIO_READ
+};
+
+struct mib_ethhdr {
+	u32 data[3];		/* first 3 mib counter */
+	__be16 hdr;		/* qca hdr */
+} __packed;
+
 #endif /* __TAG_QCA_H */
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index d30249b5205d..f5547d357647 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -55,6 +55,10 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 	if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK)
 		return NULL;
 
+	/* Ethernet MIB counter packet */
+	if (pk_type == QCA_HDR_RECV_TYPE_MIB)
+		return NULL;
+
 	/* Remove QCA tag and recalculate checksum */
 	skb_pull_rcsum(skb, QCA_HDR_LEN);
 	dsa_strip_etype_header(skb, QCA_HDR_LEN);
-- 
2.33.1

[net-next PATCH RFC v6 10/16] net: dsa: tag_qca: add support for handling mdio Ethernet 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           | 35 +++++++++++++++++++++++++++++++++--
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
index cd6275bac103..203e72dad9bb 100644
--- a/include/linux/dsa/tag_qca.h
+++ b/include/linux/dsa/tag_qca.h
@@ -69,4 +69,11 @@ struct mib_ethhdr {
 	__be16 hdr;		/* qca hdr */
 } __packed;
 
+struct tag_qca_priv {
+	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 f5547d357647..59e04157f53b 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -32,11 +32,15 @@ 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 tag_qca_priv *priv;
 	u16  hdr, pk_type;
 	__be16 *phdr;
 	int port;
 	u8 ver;
 
+	priv = dp->ds->tagger_data;
+
 	if (unlikely(!pskb_may_pull(skb, QCA_HDR_LEN)))
 		return NULL;
 
@@ -52,12 +56,18 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
 	pk_type = FIELD_GET(QCA_HDR_RECV_TYPE, hdr);
 
 	/* Ethernet MDIO read/write packet */
-	if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK)
+	if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK) {
+		if (priv->rw_reg_ack_handler)
+			priv->rw_reg_ack_handler(dp, skb);
 		return NULL;
+	}
 
 	/* Ethernet MIB counter packet */
-	if (pk_type == QCA_HDR_RECV_TYPE_MIB)
+	if (pk_type == QCA_HDR_RECV_TYPE_MIB) {
+		if (priv->mib_autocast_handler)
+			priv->mib_autocast_handler(dp, skb);
 		return NULL;
+	}
 
 	/* Remove QCA tag and recalculate checksum */
 	skb_pull_rcsum(skb, QCA_HDR_LEN);
@@ -73,9 +83,30 @@ 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 *ds)
+{
+	struct tag_qca_priv *priv;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	ds->tagger_data = priv;
+
+	return 0;
+}
+
+static void qca_tag_disconnect(struct dsa_switch *ds)
+{
+	kfree(ds->tagger_data);
+	ds->tagger_data = NULL;
+}
+
 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.33.1

[net-next PATCH RFC v6 11/16] net: dsa: qca8k: add tracking state of master port

[Ansuel Smith]

MDIO/MIB Ethernet require the master port and the tagger availabale to
correctly work. Use the new api master_state_change to track when master
is operational or not and set a bool in qca8k_priv.
We cache the first cached master available and we check if other cpu
port are operational when the cached one goes down.
This cached master will later be used by mdio read/write and mib request to
correctly use the working function.

qca8k implementation for MDIO/MIB Ethernet is bad. CPU port0 is the only
one that answers with the ack packet or sends MIB Ethernet packets. For
this reason the master_state_change ignore CPU port6 and checkes only
CPU port0 if it's operational and enables this mode.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 18 ++++++++++++++++++
 drivers/net/dsa/qca8k.h |  1 +
 2 files changed, 19 insertions(+)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 039694518788..f317f527dd6d 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -2383,6 +2383,23 @@ qca8k_port_lag_leave(struct dsa_switch *ds, int port,
 	return qca8k_lag_refresh_portmap(ds, port, lag, true);
 }
 
+static void
+qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
+		    bool operational)
+{
+	struct dsa_port *dp = master->dsa_ptr;
+	struct qca8k_priv *priv = ds->priv;
+
+	/* Ethernet MIB/MDIO is only supported for CPU port 0 */
+	if (dp->index != 0)
+		return;
+
+	if (operational)
+		priv->master = master;
+	else
+		priv->master = NULL;
+}
+
 static const struct dsa_switch_ops qca8k_switch_ops = {
 	.get_tag_protocol	= qca8k_get_tag_protocol,
 	.setup			= qca8k_setup,
@@ -2418,6 +2435,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,
+	.master_state_change	= qca8k_master_change,
 };
 
 static int qca8k_read_switch_id(struct qca8k_priv *priv)
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index ab4a417b25a9..6edd6adc3063 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -353,6 +353,7 @@ struct qca8k_priv {
 	struct dsa_switch_ops ops;
 	struct gpio_desc *reset_gpio;
 	unsigned int port_mtu[QCA8K_NUM_PORTS];
+	const struct net_device *master; /* Track if mdio/mib Ethernet is available */
 };
 
 struct qca8k_mib_desc {
-- 
2.33.1

[net-next PATCH RFC v6 12/16] 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.

Locking is added to qca8k_master_change() to make sure no mdio Ethernet
are in progress.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 192 ++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/qca8k.h |  13 +++
 2 files changed, 205 insertions(+)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index f317f527dd6d..b35ba26a0696 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,6 +171,158 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
 	return regmap_update_bits(priv->regmap, reg, mask, write_val);
 }
 
+static void qca8k_rw_reg_ack_handler(struct dsa_port *dp, struct sk_buff *skb)
+{
+	struct qca8k_mdio_hdr_data *mdio_hdr_data;
+	struct qca8k_priv *priv = dp->ds->priv;
+	struct mdio_ethhdr *mdio_ethhdr;
+	u8 len, cmd;
+
+	mdio_ethhdr = (struct mdio_ethhdr *)skb_mac_header(skb);
+	mdio_hdr_data = &priv->mdio_hdr_data;
+
+	cmd = FIELD_GET(QCA_HDR_MDIO_CMD, mdio_ethhdr->command);
+	len = FIELD_GET(QCA_HDR_MDIO_LENGTH, mdio_ethhdr->command);
+
+	/* Make sure the seq match the requested packet */
+	if (mdio_ethhdr->seq == mdio_hdr_data->seq)
+		mdio_hdr_data->ack = true;
+
+	if (cmd == MDIO_READ) {
+		mdio_hdr_data->data[0] = mdio_ethhdr->mdio_data;
+
+		/* Get the rest of the 12 byte of data */
+		if (len > QCA_HDR_MDIO_DATA1_LEN)
+			memcpy(mdio_hdr_data->data + 1, skb->data,
+			       QCA_HDR_MDIO_DATA2_LEN);
+	}
+
+	complete(&mdio_hdr_data->rw_done);
+}
+
+static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
+					       int seq_num, int priority)
+{
+	struct mdio_ethhdr *mdio_ethhdr;
+	struct sk_buff *skb;
+	u16 hdr;
+
+	skb = dev_alloc_skb(QCA_HDR_MDIO_PKG_LEN);
+
+	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 |= FIELD_PREP(QCA_HDR_XMIT_PRIORITY, priority);
+	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, seq_num);
+
+	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_mdio_hdr_data *mdio_hdr_data = &priv->mdio_hdr_data;
+	struct sk_buff *skb;
+	bool ack;
+	int ret;
+
+	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
+	skb->dev = (struct net_device *)priv->master;
+
+	mutex_lock(&mdio_hdr_data->mutex);
+
+	reinit_completion(&mdio_hdr_data->rw_done);
+	mdio_hdr_data->seq = 200;
+	mdio_hdr_data->ack = false;
+
+	dev_queue_xmit(skb);
+
+	ret = wait_for_completion_timeout(&mdio_hdr_data->rw_done,
+					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
+
+	*val = mdio_hdr_data->data[0];
+	ack = mdio_hdr_data->ack;
+
+	mutex_unlock(&mdio_hdr_data->mutex);
+
+	if (ret <= 0)
+		return -ETIMEDOUT;
+
+	if (!ack)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
+{
+	struct qca8k_mdio_hdr_data *mdio_hdr_data = &priv->mdio_hdr_data;
+	struct sk_buff *skb;
+	bool ack;
+	int ret;
+
+	skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, &val, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
+	skb->dev = (struct net_device *)priv->master;
+
+	mutex_lock(&mdio_hdr_data->mutex);
+
+	reinit_completion(&mdio_hdr_data->rw_done);
+	mdio_hdr_data->ack = false;
+	mdio_hdr_data->seq = 200;
+
+	dev_queue_xmit(skb);
+
+	ret = wait_for_completion_timeout(&mdio_hdr_data->rw_done,
+					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
+
+	ack = mdio_hdr_data->ack;
+
+	mutex_unlock(&mdio_hdr_data->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)
 {
@@ -178,6 +331,9 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
 	u16 r1, r2, page;
 	int ret;
 
+	if (priv->master && !qca8k_read_eth(priv, reg, val))
+		return 0;
+
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -201,6 +357,9 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
 	u16 r1, r2, page;
 	int ret;
 
+	if (priv->master && !qca8k_write_eth(priv, reg, val))
+		return 0;
+
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
@@ -225,6 +384,10 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
 	u32 val;
 	int ret;
 
+	if (priv->master &&
+	    !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);
@@ -2394,10 +2557,38 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
 	if (dp->index != 0)
 		return;
 
+	mutex_lock(&priv->mdio_hdr_data.mutex);
+
 	if (operational)
 		priv->master = master;
 	else
 		priv->master = NULL;
+
+	mutex_unlock(&priv->mdio_hdr_data.mutex);
+}
+
+static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
+				      enum dsa_tag_protocol proto)
+{
+	struct qca8k_priv *qca8k_priv = ds->priv;
+
+	switch (proto) {
+	case DSA_TAG_PROTO_QCA:
+		struct tag_qca_priv *priv;
+
+		priv = ds->tagger_data;
+
+		mutex_init(&qca8k_priv->mdio_hdr_data.mutex);
+		init_completion(&qca8k_priv->mdio_hdr_data.rw_done);
+
+		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 = {
@@ -2436,6 +2627,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
 	.port_lag_join		= qca8k_port_lag_join,
 	.port_lag_leave		= qca8k_port_lag_leave,
 	.master_state_change	= qca8k_master_change,
+	.connect_tag_protocol	= qca8k_connect_tag_protocol,
 };
 
 static int qca8k_read_switch_id(struct qca8k_priv *priv)
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 6edd6adc3063..dbe8c74c9793 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -11,6 +11,10 @@
 #include <linux/delay.h>
 #include <linux/regmap.h>
 #include <linux/gpio.h>
+#include <linux/dsa/tag_qca.h>
+
+#define QCA8K_ETHERNET_MDIO_PRIORITY			7
+#define QCA8K_ETHERNET_TIMEOUT				100
 
 #define QCA8K_NUM_PORTS					7
 #define QCA8K_NUM_CPU_PORTS				2
@@ -328,6 +332,14 @@ enum {
 	QCA8K_CPU_PORT6,
 };
 
+struct qca8k_mdio_hdr_data {
+	struct completion rw_done;
+	struct mutex mutex; /* Enforce one mdio read/write at time */
+	bool ack;
+	u32 seq;
+	u32 data[4];
+};
+
 struct qca8k_ports_config {
 	bool sgmii_rx_clk_falling_edge;
 	bool sgmii_tx_clk_falling_edge;
@@ -354,6 +366,7 @@ struct qca8k_priv {
 	struct gpio_desc *reset_gpio;
 	unsigned int port_mtu[QCA8K_NUM_PORTS];
 	const struct net_device *master; /* Track if mdio/mib Ethernet is available */
+	struct qca8k_mdio_hdr_data mdio_hdr_data;
 };
 
 struct qca8k_mib_desc {
-- 
2.33.1

[net-next PATCH RFC v6 13/16] net: dsa: qca8k: add support for mib autocast in Ethernet packet

[Ansuel Smith]

The switch can autocast MIB counter using Ethernet packet.
Add support for this and provide a handler for the tagger.
The switch will send packet with MIB counter for each port, the switch
will use completion API to wait for the correct packet to be received
and will complete the task only when each packet is received.
Although the handler will drop all the other packet, we still have to
consume each MIB packet to complete the request. This is done to prevent
mixed data with concurrent ethtool request.

connect_tag_protocol() is used to add the handler to the tag_qca tagger,
master_state_change() use the MIB lock to make sure no MIB Ethernet is
in progress.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 106 +++++++++++++++++++++++++++++++++++++++-
 drivers/net/dsa/qca8k.h |  17 ++++++-
 2 files changed, 121 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index b35ba26a0696..15f265c6ef02 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -795,7 +795,10 @@ qca8k_mib_init(struct qca8k_priv *priv)
 	int ret;
 
 	mutex_lock(&priv->reg_mutex);
-	ret = regmap_set_bits(priv->regmap, QCA8K_REG_MIB, QCA8K_MIB_FLUSH | QCA8K_MIB_BUSY);
+	ret = regmap_update_bits(priv->regmap, QCA8K_REG_MIB,
+				 QCA8K_MIB_FUNC | QCA8K_MIB_BUSY,
+				 FIELD_PREP(QCA8K_MIB_FUNC, QCA8K_MIB_FLUSH) |
+				 QCA8K_MIB_BUSY);
 	if (ret)
 		goto exit;
 
@@ -1866,6 +1869,97 @@ qca8k_get_strings(struct dsa_switch *ds, int port, u32 stringset, uint8_t *data)
 			ETH_GSTRING_LEN);
 }
 
+static void qca8k_mib_autocast_handler(struct dsa_port *dp, struct sk_buff *skb)
+{
+	const struct qca8k_match_data *match_data;
+	struct qca8k_mib_hdr_data *mib_hdr_data;
+	struct qca8k_priv *priv = dp->ds->priv;
+	const struct qca8k_mib_desc *mib;
+	struct mib_ethhdr *mib_ethhdr;
+	int i, mib_len, offset = 0;
+	u64 *data;
+	u8 port;
+
+	mib_ethhdr = (struct mib_ethhdr *)skb_mac_header(skb);
+	mib_hdr_data = &priv->mib_hdr_data;
+
+	/* The switch autocast every port. Ignore other packet and
+	 * parse only the requested one.
+	 */
+	port = FIELD_GET(QCA_HDR_RECV_SOURCE_PORT, ntohs(mib_ethhdr->hdr));
+	if (port != mib_hdr_data->req_port)
+		goto exit;
+
+	match_data = device_get_match_data(priv->dev);
+	data = mib_hdr_data->data;
+
+	for (i = 0; i < match_data->mib_count; i++) {
+		mib = &ar8327_mib[i];
+
+		/* First 3 mib are present in the skb head */
+		if (i < 3) {
+			data[i] = mib_ethhdr->data[i];
+			continue;
+		}
+
+		mib_len = sizeof(uint32_t);
+
+		/* Some mib are 64 bit wide */
+		if (mib->size == 2)
+			mib_len = sizeof(uint64_t);
+
+		/* Copy the mib value from packet to the */
+		memcpy(data + i, skb->data + offset, mib_len);
+
+		/* Set the offset for the next mib */
+		offset += mib_len;
+	}
+
+exit:
+	/* Complete on receiving all the mib packet */
+	if (refcount_dec_and_test(&mib_hdr_data->port_parsed))
+		complete(&mib_hdr_data->rw_done);
+}
+
+static int
+qca8k_get_ethtool_stats_eth(struct dsa_switch *ds, int port, u64 *data)
+{
+	struct dsa_port *dp = dsa_to_port(ds, port);
+	struct qca8k_mib_hdr_data *mib_hdr_data;
+	struct qca8k_priv *priv = ds->priv;
+	int ret;
+
+	mib_hdr_data = &priv->mib_hdr_data;
+
+	mutex_lock(&mib_hdr_data->mutex);
+
+	reinit_completion(&mib_hdr_data->rw_done);
+
+	mib_hdr_data->req_port = dp->index;
+	mib_hdr_data->data = data;
+	refcount_set(&mib_hdr_data->port_parsed, QCA8K_NUM_PORTS);
+
+	mutex_lock(&priv->reg_mutex);
+
+	/* Send mib autocast request */
+	ret = regmap_update_bits(priv->regmap, QCA8K_REG_MIB,
+				 QCA8K_MIB_FUNC | QCA8K_MIB_BUSY,
+				 FIELD_PREP(QCA8K_MIB_FUNC, QCA8K_MIB_CAST) |
+				 QCA8K_MIB_BUSY);
+
+	mutex_unlock(&priv->reg_mutex);
+
+	if (ret)
+		goto exit;
+
+	ret = wait_for_completion_timeout(&mib_hdr_data->rw_done, QCA8K_ETHERNET_TIMEOUT);
+
+exit:
+	mutex_unlock(&mib_hdr_data->mutex);
+
+	return ret;
+}
+
 static void
 qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
 			uint64_t *data)
@@ -1877,6 +1971,10 @@ qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
 	u32 hi = 0;
 	int ret;
 
+	if (priv->master &&
+	    qca8k_get_ethtool_stats_eth(ds, port, data) > 0)
+		return;
+
 	match_data = of_device_get_match_data(priv->dev);
 
 	for (i = 0; i < match_data->mib_count; i++) {
@@ -2558,6 +2656,7 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
 		return;
 
 	mutex_lock(&priv->mdio_hdr_data.mutex);
+	mutex_lock(&priv->mib_hdr_data.mutex);
 
 	if (operational)
 		priv->master = master;
@@ -2565,6 +2664,7 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
 		priv->master = NULL;
 
 	mutex_unlock(&priv->mdio_hdr_data.mutex);
+	mutex_unlock(&priv->mib_hdr_data.mutex);
 }
 
 static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
@@ -2581,7 +2681,11 @@ static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
 		mutex_init(&qca8k_priv->mdio_hdr_data.mutex);
 		init_completion(&qca8k_priv->mdio_hdr_data.rw_done);
 
+		mutex_init(&qca8k_priv->mib_hdr_data.mutex);
+		init_completion(&qca8k_priv->mib_hdr_data.rw_done);
+
 		priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
+		priv->mib_autocast_handler = qca8k_mib_autocast_handler;
 
 		break;
 	default:
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index dbe8c74c9793..4aca07db0192 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -67,7 +67,7 @@
 #define QCA8K_REG_MODULE_EN				0x030
 #define   QCA8K_MODULE_EN_MIB				BIT(0)
 #define QCA8K_REG_MIB					0x034
-#define   QCA8K_MIB_FLUSH				BIT(24)
+#define   QCA8K_MIB_FUNC				GENMASK(26, 24)
 #define   QCA8K_MIB_CPU_KEEP				BIT(20)
 #define   QCA8K_MIB_BUSY				BIT(17)
 #define QCA8K_MDIO_MASTER_CTRL				0x3c
@@ -317,6 +317,12 @@ enum qca8k_vlan_cmd {
 	QCA8K_VLAN_READ = 6,
 };
 
+enum qca8k_mid_cmd {
+	QCA8K_MIB_FLUSH = 1,
+	QCA8K_MIB_FLUSH_PORT = 2,
+	QCA8K_MIB_CAST = 3,
+};
+
 struct ar8xxx_port_status {
 	int enabled;
 };
@@ -340,6 +346,14 @@ struct qca8k_mdio_hdr_data {
 	u32 data[4];
 };
 
+struct qca8k_mib_hdr_data {
+	struct completion rw_done;
+	struct mutex mutex; /* Process one command at time */
+	refcount_t port_parsed; /* Counter to track parsed port */
+	u8 req_port;
+	u64 *data; /* pointer to ethtool data */
+};
+
 struct qca8k_ports_config {
 	bool sgmii_rx_clk_falling_edge;
 	bool sgmii_tx_clk_falling_edge;
@@ -367,6 +381,7 @@ struct qca8k_priv {
 	unsigned int port_mtu[QCA8K_NUM_PORTS];
 	const struct net_device *master; /* Track if mdio/mib Ethernet is available */
 	struct qca8k_mdio_hdr_data mdio_hdr_data;
+	struct qca8k_mib_hdr_data mib_hdr_data;
 };
 
 struct qca8k_mib_desc {
-- 
2.33.1

[net-next PATCH RFC v6 14/16] net: dsa: qca8k: add support for phy read/write with mdio Ethernet

[Ansuel Smith]

Use mdio Ethernet also for phy read/write if availabale. Use a different
seq number to make sure we receive the correct packet.
On any error, we fallback to the legacy mdio read/write.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 177 ++++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/qca8k.h |   1 +
 2 files changed, 178 insertions(+)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 15f265c6ef02..b08db31933e0 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -832,6 +832,152 @@ qca8k_port_set_status(struct qca8k_priv *priv, int port, int enable)
 		regmap_clear_bits(priv->regmap, QCA8K_REG_PORT_STATUS(port), mask);
 }
 
+static int
+qca8k_mdio_eth_busy_wait(struct qca8k_mdio_hdr_data *phy_hdr_data,
+			 struct sk_buff *read_skb, u32 *val)
+{
+	struct sk_buff *skb = skb_copy(read_skb, GFP_KERNEL);
+	bool ack;
+	int ret;
+
+	reinit_completion(&phy_hdr_data->rw_done);
+	phy_hdr_data->seq = 400;
+	phy_hdr_data->ack = false;
+
+	dev_queue_xmit(skb);
+
+	ret = wait_for_completion_timeout(&phy_hdr_data->rw_done,
+					  QCA8K_ETHERNET_TIMEOUT);
+
+	ack = phy_hdr_data->ack;
+
+	if (ret <= 0)
+		return -ETIMEDOUT;
+
+	if (!ack)
+		return -EINVAL;
+
+	*val = phy_hdr_data->data[0];
+
+	return 0;
+}
+
+static int
+qca8k_mdio_eth_command(struct qca8k_priv *priv, bool read, int phy,
+		       int regnum, u16 data)
+{
+	struct sk_buff *write_skb, *clear_skb, *read_skb;
+	const struct net_device *dev = priv->master;
+	struct qca8k_mdio_hdr_data *phy_hdr_data;
+	u32 write_val, clear_val = 0, val;
+	int seq_num = 400;
+	int ret, ret1;
+	bool ack;
+
+	if (regnum >= QCA8K_MDIO_MASTER_MAX_REG)
+		return -EINVAL;
+
+	phy_hdr_data = &priv->mdio_hdr_data;
+
+	write_val = QCA8K_MDIO_MASTER_BUSY | QCA8K_MDIO_MASTER_EN |
+		    QCA8K_MDIO_MASTER_PHY_ADDR(phy) |
+		    QCA8K_MDIO_MASTER_REG_ADDR(regnum);
+
+	if (read) {
+		write_val |= QCA8K_MDIO_MASTER_READ;
+	} else {
+		write_val |= QCA8K_MDIO_MASTER_WRITE;
+		write_val |= QCA8K_MDIO_MASTER_DATA(data);
+	}
+
+	/* Prealloc all the needed skb before the lock */
+	write_skb = qca8k_alloc_mdio_header(MDIO_WRITE, QCA8K_MDIO_MASTER_CTRL,
+					    &write_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
+	write_skb->dev = (struct net_device *)dev;
+	clear_skb = qca8k_alloc_mdio_header(MDIO_WRITE, QCA8K_MDIO_MASTER_CTRL,
+					    &clear_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
+	clear_skb->dev = (struct net_device *)dev;
+	read_skb = qca8k_alloc_mdio_header(MDIO_READ, QCA8K_MDIO_MASTER_CTRL,
+					   &clear_val, seq_num, QCA8K_ETHERNET_PHY_PRIORITY);
+	read_skb->dev = (struct net_device *)dev;
+
+	/* Actually start the request:
+	 * 1. Send mdio master packet
+	 * 2. Busy Wait for mdio master command
+	 * 3. Get the data if we are reading
+	 * 4. Reset the mdio master (even with error)
+	 */
+	mutex_lock(&phy_hdr_data->mutex);
+
+	reinit_completion(&phy_hdr_data->rw_done);
+	phy_hdr_data->ack = false;
+	phy_hdr_data->seq = seq_num;
+
+	dev_queue_xmit(write_skb);
+
+	ret = wait_for_completion_timeout(&phy_hdr_data->rw_done,
+					  QCA8K_ETHERNET_TIMEOUT);
+
+	ack = phy_hdr_data->ack;
+
+	if (ret <= 0) {
+		ret = -ETIMEDOUT;
+		goto exit;
+	}
+
+	if (!ack) {
+		ret = -EINVAL;
+		goto exit;
+	}
+
+	ret = read_poll_timeout(qca8k_mdio_eth_busy_wait, ret1,
+				!(val & QCA8K_MDIO_MASTER_BUSY), 0,
+				QCA8K_BUSY_WAIT_TIMEOUT * USEC_PER_MSEC, false,
+				phy_hdr_data, read_skb, &val);
+
+	if (ret < 0 && ret1 < 0) {
+		ret = ret1;
+		goto exit;
+	}
+
+	if (read) {
+		reinit_completion(&phy_hdr_data->rw_done);
+		phy_hdr_data->ack = false;
+
+		dev_queue_xmit(read_skb);
+
+		ret = wait_for_completion_timeout(&phy_hdr_data->rw_done,
+						  QCA8K_ETHERNET_TIMEOUT);
+
+		ack = phy_hdr_data->ack;
+
+		if (ret <= 0) {
+			ret = -ETIMEDOUT;
+			goto exit;
+		}
+
+		if (!ack) {
+			ret = -EINVAL;
+			goto exit;
+		}
+
+		ret = phy_hdr_data->data[0] & QCA8K_MDIO_MASTER_DATA_MASK;
+	}
+
+exit:
+	reinit_completion(&phy_hdr_data->rw_done);
+	phy_hdr_data->ack = false;
+
+	dev_queue_xmit(clear_skb);
+
+	wait_for_completion_timeout(&phy_hdr_data->rw_done,
+				    QCA8K_ETHERNET_TIMEOUT);
+
+	mutex_unlock(&phy_hdr_data->mutex);
+
+	return ret;
+}
+
 static u32
 qca8k_port_to_phy(int port)
 {
@@ -954,6 +1100,14 @@ qca8k_internal_mdio_write(struct mii_bus *slave_bus, int phy, int regnum, u16 da
 {
 	struct qca8k_priv *priv = slave_bus->priv;
 	struct mii_bus *bus = priv->bus;
+	int ret;
+
+	/* Use mdio Ethernet when available, fallback to legacy one on error */
+	if (priv->master) {
+		ret = qca8k_mdio_eth_command(priv, false, phy, regnum, data);
+		if (!ret)
+			return 0;
+	}
 
 	return qca8k_mdio_write(bus, phy, regnum, data);
 }
@@ -963,6 +1117,14 @@ qca8k_internal_mdio_read(struct mii_bus *slave_bus, int phy, int regnum)
 {
 	struct qca8k_priv *priv = slave_bus->priv;
 	struct mii_bus *bus = priv->bus;
+	int ret;
+
+	/* Use mdio Ethernet when available, fallback to legacy one on error */
+	if (priv->master) {
+		ret = qca8k_mdio_eth_command(priv, true, phy, regnum, 0);
+		if (ret >= 0)
+			return ret;
+	}
 
 	return qca8k_mdio_read(bus, phy, regnum);
 }
@@ -971,6 +1133,7 @@ static int
 qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
 {
 	struct qca8k_priv *priv = ds->priv;
+	int ret;
 
 	/* Check if the legacy mapping should be used and the
 	 * port is not correctly mapped to the right PHY in the
@@ -979,6 +1142,13 @@ qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
 	if (priv->legacy_phy_port_mapping)
 		port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
 
+	/* Use mdio Ethernet when available, fallback to legacy one on error */
+	if (priv->master) {
+		ret = qca8k_mdio_eth_command(priv, true, port, regnum, 0);
+		if (!ret)
+			return ret;
+	}
+
 	return qca8k_mdio_write(priv->bus, port, regnum, data);
 }
 
@@ -995,6 +1165,13 @@ qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
 	if (priv->legacy_phy_port_mapping)
 		port = qca8k_port_to_phy(port) % PHY_MAX_ADDR;
 
+	/* Use mdio Ethernet when available, fallback to legacy one on error */
+	if (priv->master) {
+		ret = qca8k_mdio_eth_command(priv, true, port, regnum, 0);
+		if (ret >= 0)
+			return ret;
+	}
+
 	ret = qca8k_mdio_read(priv->bus, port, regnum);
 
 	if (ret < 0)
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 4aca07db0192..203220efa5c0 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -14,6 +14,7 @@
 #include <linux/dsa/tag_qca.h>
 
 #define QCA8K_ETHERNET_MDIO_PRIORITY			7
+#define QCA8K_ETHERNET_PHY_PRIORITY			6
 #define QCA8K_ETHERNET_TIMEOUT				100
 
 #define QCA8K_NUM_PORTS					7
-- 
2.33.1

[net-next PATCH RFC v6 15/16] net: dsa: qca8k: move page cache to driver priv

[Ansuel Smith]

There can be multiple qca8k switch on the same system. Move the static
qca8k_current_page to qca8k_priv and make it specific for each switch.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/net/dsa/qca8k.c | 47 +++++++++++++++++++++++------------------
 drivers/net/dsa/qca8k.h |  9 ++++++++
 2 files changed, 36 insertions(+), 20 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index b08db31933e0..4254cb12c7f7 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -75,12 +75,6 @@ static const struct qca8k_mib_desc ar8327_mib[] = {
 	MIB_DESC(1, 0xac, "TXUnicast"),
 };
 
-/* The 32bit switch registers are accessed indirectly. To achieve this we need
- * to set the page of the register. Track the last page that was set to reduce
- * mdio writes
- */
-static u16 qca8k_current_page = 0xffff;
-
 static void
 qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
 {
@@ -134,11 +128,11 @@ qca8k_mii_write32(struct mii_bus *bus, int phy_id, u32 regnum, u32 val)
 }
 
 static int
-qca8k_set_page(struct mii_bus *bus, u16 page)
+qca8k_set_page(struct mii_bus *bus, u16 page, u16 *cached_page)
 {
 	int ret;
 
-	if (page == qca8k_current_page)
+	if (page == *cached_page)
 		return 0;
 
 	ret = bus->write(bus, 0x18, 0, page);
@@ -148,7 +142,7 @@ qca8k_set_page(struct mii_bus *bus, u16 page)
 		return ret;
 	}
 
-	qca8k_current_page = page;
+	*cached_page = page;
 	usleep_range(1000, 2000);
 	return 0;
 }
@@ -327,6 +321,7 @@ static int
 qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
+	struct qca8k_mdio_cache *mdio_cache;
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	int ret;
@@ -334,11 +329,13 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
 	if (priv->master && !qca8k_read_eth(priv, reg, val))
 		return 0;
 
+	mdio_cache = &priv->mdio_cache;
+
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
 
-	ret = qca8k_set_page(bus, page);
+	ret = qca8k_set_page(bus, page, &mdio_cache->page);
 	if (ret < 0)
 		goto exit;
 
@@ -353,6 +350,7 @@ static int
 qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
 {
 	struct qca8k_priv *priv = (struct qca8k_priv *)ctx;
+	struct qca8k_mdio_cache *mdio_cache;
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	int ret;
@@ -360,11 +358,13 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
 	if (priv->master && !qca8k_write_eth(priv, reg, val))
 		return 0;
 
+	mdio_cache = &priv->mdio_cache;
+
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
 
-	ret = qca8k_set_page(bus, page);
+	ret = qca8k_set_page(bus, page, &mdio_cache->page);
 	if (ret < 0)
 		goto exit;
 
@@ -379,6 +379,7 @@ 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_mdio_cache *mdio_cache;
 	struct mii_bus *bus = priv->bus;
 	u16 r1, r2, page;
 	u32 val;
@@ -388,11 +389,13 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
 	    !qca8k_regmap_update_bits_eth(priv, reg, mask, write_val))
 		return 0;
 
+	mdio_cache = &priv->mdio_cache;
+
 	qca8k_split_addr(reg, &r1, &r2, &page);
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
 
-	ret = qca8k_set_page(bus, page);
+	ret = qca8k_set_page(bus, page, &mdio_cache->page);
 	if (ret < 0)
 		goto exit;
 
@@ -1016,7 +1019,8 @@ qca8k_mdio_busy_wait(struct mii_bus *bus, u32 reg, u32 mask)
 }
 
 static int
-qca8k_mdio_write(struct mii_bus *bus, int phy, int regnum, u16 data)
+qca8k_mdio_write(struct mii_bus *bus, struct qca8k_mdio_cache *cache,
+		 int phy, int regnum, u16 data)
 {
 	u16 r1, r2, page;
 	u32 val;
@@ -1034,7 +1038,7 @@ qca8k_mdio_write(struct mii_bus *bus, int phy, int regnum, u16 data)
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
 
-	ret = qca8k_set_page(bus, page);
+	ret = qca8k_set_page(bus, page, &cache->page);
 	if (ret)
 		goto exit;
 
@@ -1053,7 +1057,8 @@ qca8k_mdio_write(struct mii_bus *bus, int phy, int regnum, u16 data)
 }
 
 static int
-qca8k_mdio_read(struct mii_bus *bus, int phy, int regnum)
+qca8k_mdio_read(struct mii_bus *bus, struct qca8k_mdio_cache *cache,
+		int phy, int regnum)
 {
 	u16 r1, r2, page;
 	u32 val;
@@ -1070,7 +1075,7 @@ qca8k_mdio_read(struct mii_bus *bus, int phy, int regnum)
 
 	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
 
-	ret = qca8k_set_page(bus, page);
+	ret = qca8k_set_page(bus, page, &cache->page);
 	if (ret)
 		goto exit;
 
@@ -1109,7 +1114,7 @@ qca8k_internal_mdio_write(struct mii_bus *slave_bus, int phy, int regnum, u16 da
 			return 0;
 	}
 
-	return qca8k_mdio_write(bus, phy, regnum, data);
+	return qca8k_mdio_write(bus, &priv->mdio_cache, phy, regnum, data);
 }
 
 static int
@@ -1126,7 +1131,7 @@ qca8k_internal_mdio_read(struct mii_bus *slave_bus, int phy, int regnum)
 			return ret;
 	}
 
-	return qca8k_mdio_read(bus, phy, regnum);
+	return qca8k_mdio_read(bus, &priv->mdio_cache, phy, regnum);
 }
 
 static int
@@ -1149,7 +1154,7 @@ qca8k_phy_write(struct dsa_switch *ds, int port, int regnum, u16 data)
 			return ret;
 	}
 
-	return qca8k_mdio_write(priv->bus, port, regnum, data);
+	return qca8k_mdio_write(priv->bus, &priv->mdio_cache, port, regnum, data);
 }
 
 static int
@@ -1172,7 +1177,7 @@ qca8k_phy_read(struct dsa_switch *ds, int port, int regnum)
 			return ret;
 	}
 
-	ret = qca8k_mdio_read(priv->bus, port, regnum);
+	ret = qca8k_mdio_read(priv->bus, &priv->mdio_cache, port, regnum);
 
 	if (ret < 0)
 		return 0xffff;
@@ -2979,6 +2984,8 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 		return PTR_ERR(priv->regmap);
 	}
 
+	priv->mdio_cache.page = 0xffff;
+
 	/* Check the detected switch id */
 	ret = qca8k_read_switch_id(priv);
 	if (ret)
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index 203220efa5c0..c4800ee06c34 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -363,6 +363,14 @@ struct qca8k_ports_config {
 	u8 rgmii_tx_delay[QCA8K_NUM_CPU_PORTS]; /* 0: CPU port0, 1: CPU port6 */
 };
 
+struct qca8k_mdio_cache {
+/* The 32bit switch registers are accessed indirectly. To achieve this we need
+ * to set the page of the register. Track the last page that was set to reduce
+ * mdio writes
+ */
+	u16 page;
+};
+
 struct qca8k_priv {
 	u8 switch_id;
 	u8 switch_revision;
@@ -383,6 +391,7 @@ struct qca8k_priv {
 	const struct net_device *master; /* Track if mdio/mib Ethernet is available */
 	struct qca8k_mdio_hdr_data mdio_hdr_data;
 	struct qca8k_mib_hdr_data mib_hdr_data;
+	struct qca8k_mdio_cache mdio_cache;
 };
 
 struct qca8k_mib_desc {
-- 
2.33.1

[net-next PATCH RFC v6 16/16] 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 as 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 | 60 ++++++++++++++++++++++++++++++++---------
 drivers/net/dsa/qca8k.h |  5 ++++
 2 files changed, 53 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 4254cb12c7f7..cc8fac188389 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -88,6 +88,42 @@ qca8k_split_addr(u32 regaddr, u16 *r1, u16 *r2, u16 *page)
 	*page = regaddr & 0x3ff;
 }
 
+static int
+qca8k_set_lo(struct mii_bus *bus, int phy_id, u32 regnum,
+	     u16 lo, u16 *cached_lo)
+{
+	int ret;
+
+	if (lo == *cached_lo)
+		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");
+
+	*cached_lo = lo;
+	return 0;
+}
+
+static int
+qca8k_set_hi(struct mii_bus *bus, int phy_id, u32 regnum,
+	     u16 hi, u16 *cached_hi)
+{
+	int ret;
+
+	if (hi == *cached_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");
+
+	*cached_hi = hi;
+	return 0;
+}
+
 static int
 qca8k_mii_read32(struct mii_bus *bus, int phy_id, u32 regnum, u32 *val)
 {
@@ -111,7 +147,8 @@ qca8k_mii_read32(struct mii_bus *bus, int phy_id, u32 regnum, u32 *val)
 }
 
 static void
-qca8k_mii_write32(struct mii_bus *bus, int phy_id, u32 regnum, u32 val)
+qca8k_mii_write32(struct mii_bus *bus, struct qca8k_mdio_cache *cache,
+		  int phy_id, u32 regnum, u32 val)
 {
 	u16 lo, hi;
 	int ret;
@@ -119,12 +156,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, &cache->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, &cache->hi);
 }
 
 static int
@@ -368,7 +402,7 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
 	if (ret < 0)
 		goto exit;
 
-	qca8k_mii_write32(bus, 0x10 | r2, r1, val);
+	qca8k_mii_write32(bus, mdio_cache, 0x10 | r2, r1, val);
 
 exit:
 	mutex_unlock(&bus->mdio_lock);
@@ -405,7 +439,7 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
 
 	val &= ~mask;
 	val |= write_val;
-	qca8k_mii_write32(bus, 0x10 | r2, r1, val);
+	qca8k_mii_write32(bus, mdio_cache, 0x10 | r2, r1, val);
 
 exit:
 	mutex_unlock(&bus->mdio_lock);
@@ -1042,14 +1076,14 @@ qca8k_mdio_write(struct mii_bus *bus, struct qca8k_mdio_cache *cache,
 	if (ret)
 		goto exit;
 
-	qca8k_mii_write32(bus, 0x10 | r2, r1, val);
+	qca8k_mii_write32(bus, cache, 0x10 | r2, r1, val);
 
 	ret = qca8k_mdio_busy_wait(bus, QCA8K_MDIO_MASTER_CTRL,
 				   QCA8K_MDIO_MASTER_BUSY);
 
 exit:
 	/* even if the busy_wait timeouts try to clear the MASTER_EN */
-	qca8k_mii_write32(bus, 0x10 | r2, r1, 0);
+	qca8k_mii_write32(bus, cache, 0x10 | r2, r1, 0);
 
 	mutex_unlock(&bus->mdio_lock);
 
@@ -1079,7 +1113,7 @@ qca8k_mdio_read(struct mii_bus *bus, struct qca8k_mdio_cache *cache,
 	if (ret)
 		goto exit;
 
-	qca8k_mii_write32(bus, 0x10 | r2, r1, val);
+	qca8k_mii_write32(bus, cache, 0x10 | r2, r1, val);
 
 	ret = qca8k_mdio_busy_wait(bus, QCA8K_MDIO_MASTER_CTRL,
 				   QCA8K_MDIO_MASTER_BUSY);
@@ -1090,7 +1124,7 @@ qca8k_mdio_read(struct mii_bus *bus, struct qca8k_mdio_cache *cache,
 
 exit:
 	/* even if the busy_wait timeouts try to clear the MASTER_EN */
-	qca8k_mii_write32(bus, 0x10 | r2, r1, 0);
+	qca8k_mii_write32(bus, cache, 0x10 | r2, r1, 0);
 
 	mutex_unlock(&bus->mdio_lock);
 
@@ -2985,6 +3019,8 @@ qca8k_sw_probe(struct mdio_device *mdiodev)
 	}
 
 	priv->mdio_cache.page = 0xffff;
+	priv->mdio_cache.lo = 0xffff;
+	priv->mdio_cache.hi = 0xffff;
 
 	/* Check the detected switch id */
 	ret = qca8k_read_switch_id(priv);
diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
index c4800ee06c34..79cd35f48730 100644
--- a/drivers/net/dsa/qca8k.h
+++ b/drivers/net/dsa/qca8k.h
@@ -369,6 +369,11 @@ struct qca8k_mdio_cache {
  * mdio writes
  */
 	u16 page;
+/* lo and hi can also be cached and from Documentation we can skip one
+ * extra mdio write if lo or hi is didn't change.
+ */
+	u16 lo;
+	u16 hi;
 };
 
 struct qca8k_priv {
-- 
2.33.1

Re: [net-next PATCH RFC v6 00/16] Add support for qca8k mdio rw in Ethernet packet

[Ansuel Smith]

On Tue, Dec 14, 2021 at 11:43:53PM +0100, Ansuel Smith wrote:
> Hi, this is ready but require some additional test on a wider userbase.
> 
> 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.
> 
> Currently we use Ethernet packet for
> - MIB counter
> - mdio read/write configuration
> - phy read/write for each port
> 
> 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.
> 
> 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.
> 
> The tagger priv implement only the handler for the special packet. All the
> other stuff is placed in the qca8k_priv and the tagger has to access
> it under lock.
> 
> We use the new API from Vladimir to track if the master port is
> operational or not. We had to track many thing to reach a usable state.
> Checking if the port is UP is not enough and tracking a NETDEV_CHANGE is
> also not enough since it use also for other task. The correct way was
> both track for interface UP and if a qdisc was assigned to the
> interface. That tells us the port (and the tagger indirectly) is ready
> to accept and process packet.
> 
> I tested this with multicpu port and with port6 set as the unique port and
> it's sad.
> It seems they implemented this feature in a bad way and this is only
> supported with cpu port0. When cpu port6 is the unique port, the switch
> doesn't send ack packet. With multicpu port, packet ack are not duplicated
> and only cpu port0 sends them. This is the same for the MIB counter.
> For this reason this feature is enabled only when cpu port0 is enabled and
> operational.
> 
> Current concern are:
> - Any hint about the naming? Is calling this mdio Ethernet correct?
>   Should we use a more ""standard""/significant name? (considering also
>   other switch will implement this)
> 
> v6:
> - Fix some error in ethtool handler caused by rebase/cleanup
> v5:
> - Adapt to new API fixes
> - Fix a wrong logic for noop
> - Add additional lock for master_state change
> - Limit mdio Ethernet to cpu port0 (switch limitation)
> - Add priority to these special packet
> - Move mdio cache to qca8k_priv
> v4:
> - Remove duplicate patch sent by mistake.
> v3:
> - Include MIB with Ethernet packet.
> - Include phy read/write with Ethernet packet.
> - Reorganize code with new API.
> - Introuce master tracking by Vladimir
> 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 (12):
>   net: dsa: tag_qca: convert to FIELD macro
>   net: dsa: tag_qca: move define to include linux/dsa
>   net: dsa: tag_qca: enable promisc_on_master flag
>   net: dsa: tag_qca: add define for handling mdio Ethernet packet
>   net: dsa: tag_qca: add define for handling MIB packet
>   net: dsa: tag_qca: add support for handling mdio Ethernet and MIB
>     packet
>   net: dsa: qca8k: add tracking state of master port
>   net: dsa: qca8k: add support for mdio read/write in Ethernet packet
>   net: dsa: qca8k: add support for mib autocast in Ethernet packet
>   net: dsa: qca8k: add support for phy read/write with mdio Ethernet
>   net: dsa: qca8k: move page cache to driver priv
>   net: dsa: qca8k: cache lo and hi for mdio write
> 
> Vladimir Oltean (4):
>   net: dsa: provide switch operations for tracking the master state
>   net: dsa: stop updating master MTU from master.c
>   net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
>   net: dsa: replay master state events in
>     dsa_tree_{setup,teardown}_master
> 
>  drivers/net/dsa/qca8k.c     | 600 ++++++++++++++++++++++++++++++++++--
>  drivers/net/dsa/qca8k.h     |  46 ++-
>  include/linux/dsa/tag_qca.h |  79 +++++
>  include/net/dsa.h           |  17 +
>  net/dsa/dsa2.c              |  81 ++++-
>  net/dsa/dsa_priv.h          |  13 +
>  net/dsa/master.c            |  29 +-
>  net/dsa/slave.c             |  32 ++
>  net/dsa/switch.c            |  15 +
>  net/dsa/tag_qca.c           |  81 +++--
>  10 files changed, 901 insertions(+), 92 deletions(-)
>  create mode 100644 include/linux/dsa/tag_qca.h
> 
> -- 
> 2.33.1
> 

I just tested if the Documentation is correct about this alternative way
being able to read/write 16byte of data at times (instead of 4).

I confirm this work but I need to understand how and if this can be
used. I can see I should use the regmap bulk api... But I assume I
should change the val_bits. (think that is not acceptable if regmap is
already init and would be problematic for the fallback...)

Wonder if I should register a separate regmap for eth and use that...
(and create an helper to switch between the mdio and the ethernet one?)

This would be very useful for fib read/write that require 4 read/write
to put data in the db. (1 lock, 1 packet instead of 4 lock 4 packet)

Would love any hint if we have other way to handle this but I think the
double regmap and the helper seems to be the cleaner way for this.

(obviously this will come later and won't be part of this already big
patch)

-- 
	Ansuel

Re: [net-next PATCH RFC v6 12/16] net: dsa: qca8k: add support for mdio read/write in Ethernet packet

[Vladimir Oltean]

On Tue, Dec 14, 2021 at 11:44:05PM +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.
> 
> Locking is added to qca8k_master_change() to make sure no mdio Ethernet
> are in progress.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  drivers/net/dsa/qca8k.c | 192 ++++++++++++++++++++++++++++++++++++++++
>  drivers/net/dsa/qca8k.h |  13 +++
>  2 files changed, 205 insertions(+)
> 
> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> index f317f527dd6d..b35ba26a0696 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,6 +171,158 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
>  	return regmap_update_bits(priv->regmap, reg, mask, write_val);
>  }
>  
> +static void qca8k_rw_reg_ack_handler(struct dsa_port *dp, struct sk_buff *skb)
> +{
> +	struct qca8k_mdio_hdr_data *mdio_hdr_data;
> +	struct qca8k_priv *priv = dp->ds->priv;
> +	struct mdio_ethhdr *mdio_ethhdr;
> +	u8 len, cmd;
> +
> +	mdio_ethhdr = (struct mdio_ethhdr *)skb_mac_header(skb);
> +	mdio_hdr_data = &priv->mdio_hdr_data;
> +
> +	cmd = FIELD_GET(QCA_HDR_MDIO_CMD, mdio_ethhdr->command);
> +	len = FIELD_GET(QCA_HDR_MDIO_LENGTH, mdio_ethhdr->command);
> +
> +	/* Make sure the seq match the requested packet */
> +	if (mdio_ethhdr->seq == mdio_hdr_data->seq)
> +		mdio_hdr_data->ack = true;
> +
> +	if (cmd == MDIO_READ) {
> +		mdio_hdr_data->data[0] = mdio_ethhdr->mdio_data;
> +
> +		/* Get the rest of the 12 byte of data */
> +		if (len > QCA_HDR_MDIO_DATA1_LEN)
> +			memcpy(mdio_hdr_data->data + 1, skb->data,
> +			       QCA_HDR_MDIO_DATA2_LEN);
> +	}
> +
> +	complete(&mdio_hdr_data->rw_done);
> +}
> +
> +static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
> +					       int seq_num, int priority)
> +{
> +	struct mdio_ethhdr *mdio_ethhdr;
> +	struct sk_buff *skb;
> +	u16 hdr;
> +
> +	skb = dev_alloc_skb(QCA_HDR_MDIO_PKG_LEN);

Still no if (!skb) checking... Not only here, but also at the call sites
of this.

> +
> +	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 |= FIELD_PREP(QCA_HDR_XMIT_PRIORITY, priority);
> +	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, seq_num);
> +
> +	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);

Maybe a single call to skb_put_zero, and pass the sum as argument?

> +
> +	return skb;
> +}
> +
> +static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> +{
> +	struct qca8k_mdio_hdr_data *mdio_hdr_data = &priv->mdio_hdr_data;
> +	struct sk_buff *skb;
> +	bool ack;
> +	int ret;
> +
> +	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);

You hardcode "seq" to 200? Aren't you supposed to increment it or
something?

> +	skb->dev = (struct net_device *)priv->master;

You access priv->master outside of priv->mdio_hdr_data.mutex from
qca8k_master_change(), that can't be good.

> +
> +	mutex_lock(&mdio_hdr_data->mutex);
> +
> +	reinit_completion(&mdio_hdr_data->rw_done);
> +	mdio_hdr_data->seq = 200;

Why do you rewrite the seq here?

> +	mdio_hdr_data->ack = false;
> +
> +	dev_queue_xmit(skb);
> +
> +	ret = wait_for_completion_timeout(&mdio_hdr_data->rw_done,
> +					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
> +
> +	*val = mdio_hdr_data->data[0];
> +	ack = mdio_hdr_data->ack;
> +
> +	mutex_unlock(&mdio_hdr_data->mutex);
> +
> +	if (ret <= 0)
> +		return -ETIMEDOUT;
> +
> +	if (!ack)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
> +{
> +	struct qca8k_mdio_hdr_data *mdio_hdr_data = &priv->mdio_hdr_data;
> +	struct sk_buff *skb;
> +	bool ack;
> +	int ret;
> +
> +	skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, &val, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> +	skb->dev = (struct net_device *)priv->master;
> +
> +	mutex_lock(&mdio_hdr_data->mutex);
> +
> +	reinit_completion(&mdio_hdr_data->rw_done);
> +	mdio_hdr_data->ack = false;
> +	mdio_hdr_data->seq = 200;
> +
> +	dev_queue_xmit(skb);
> +
> +	ret = wait_for_completion_timeout(&mdio_hdr_data->rw_done,
> +					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
> +
> +	ack = mdio_hdr_data->ack;
> +
> +	mutex_unlock(&mdio_hdr_data->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)
>  {
> @@ -178,6 +331,9 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
>  	u16 r1, r2, page;
>  	int ret;
>  
> +	if (priv->master && !qca8k_read_eth(priv, reg, val))
> +		return 0;
> +
>  	qca8k_split_addr(reg, &r1, &r2, &page);
>  
>  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> @@ -201,6 +357,9 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
>  	u16 r1, r2, page;
>  	int ret;
>  
> +	if (priv->master && !qca8k_write_eth(priv, reg, val))
> +		return 0;
> +
>  	qca8k_split_addr(reg, &r1, &r2, &page);
>  
>  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> @@ -225,6 +384,10 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
>  	u32 val;
>  	int ret;
>  
> +	if (priv->master &&
> +	    !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);
> @@ -2394,10 +2557,38 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
>  	if (dp->index != 0)
>  		return;
>  
> +	mutex_lock(&priv->mdio_hdr_data.mutex);
> +
>  	if (operational)
>  		priv->master = master;
>  	else
>  		priv->master = NULL;
> +
> +	mutex_unlock(&priv->mdio_hdr_data.mutex);
> +}
> +
> +static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
> +				      enum dsa_tag_protocol proto)
> +{
> +	struct qca8k_priv *qca8k_priv = ds->priv;
> +
> +	switch (proto) {
> +	case DSA_TAG_PROTO_QCA:
> +		struct tag_qca_priv *priv;
> +
> +		priv = ds->tagger_data;
> +
> +		mutex_init(&qca8k_priv->mdio_hdr_data.mutex);
> +		init_completion(&qca8k_priv->mdio_hdr_data.rw_done);

I think having these initializations here dilutes the purpose of this
callback. Could you please move these two lines to qca8k_sw_probe()?

> +
> +		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 = {
> @@ -2436,6 +2627,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
>  	.port_lag_join		= qca8k_port_lag_join,
>  	.port_lag_leave		= qca8k_port_lag_leave,
>  	.master_state_change	= qca8k_master_change,
> +	.connect_tag_protocol	= qca8k_connect_tag_protocol,
>  };
>  
>  static int qca8k_read_switch_id(struct qca8k_priv *priv)
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index 6edd6adc3063..dbe8c74c9793 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -11,6 +11,10 @@
>  #include <linux/delay.h>
>  #include <linux/regmap.h>
>  #include <linux/gpio.h>
> +#include <linux/dsa/tag_qca.h>
> +
> +#define QCA8K_ETHERNET_MDIO_PRIORITY			7
> +#define QCA8K_ETHERNET_TIMEOUT				100
>  
>  #define QCA8K_NUM_PORTS					7
>  #define QCA8K_NUM_CPU_PORTS				2
> @@ -328,6 +332,14 @@ enum {
>  	QCA8K_CPU_PORT6,
>  };
>  
> +struct qca8k_mdio_hdr_data {

What do you think about the "qca8k_eth_mgmt_data" name rather than
"mdio_hdr_data"? I don't think this has anything to do with MDIO.

> +	struct completion rw_done;
> +	struct mutex mutex; /* Enforce one mdio read/write at time */
> +	bool ack;
> +	u32 seq;
> +	u32 data[4];
> +};
> +
>  struct qca8k_ports_config {
>  	bool sgmii_rx_clk_falling_edge;
>  	bool sgmii_tx_clk_falling_edge;
> @@ -354,6 +366,7 @@ struct qca8k_priv {
>  	struct gpio_desc *reset_gpio;
>  	unsigned int port_mtu[QCA8K_NUM_PORTS];
>  	const struct net_device *master; /* Track if mdio/mib Ethernet is available */
> +	struct qca8k_mdio_hdr_data mdio_hdr_data;
>  };
>  
>  struct qca8k_mib_desc {
> -- 
> 2.33.1
> 

Re: [net-next PATCH RFC v6 11/16] net: dsa: qca8k: add tracking state of master port

[Vladimir Oltean]

On Tue, Dec 14, 2021 at 11:44:04PM +0100, Ansuel Smith wrote:
> MDIO/MIB Ethernet require the master port and the tagger availabale to
> correctly work. Use the new api master_state_change to track when master
> is operational or not and set a bool in qca8k_priv.
> We cache the first cached master available and we check if other cpu
> port are operational when the cached one goes down.
> This cached master will later be used by mdio read/write and mib request to
> correctly use the working function.
> 
> qca8k implementation for MDIO/MIB Ethernet is bad. CPU port0 is the only
> one that answers with the ack packet or sends MIB Ethernet packets. For
> this reason the master_state_change ignore CPU port6 and checkes only
> CPU port0 if it's operational and enables this mode.

CPU port 0 may not always be wired, it depends on board design, right?
So the Ethernet management protocol may or may not be available to all users.

> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
> diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> index ab4a417b25a9..6edd6adc3063 100644
> --- a/drivers/net/dsa/qca8k.h
> +++ b/drivers/net/dsa/qca8k.h
> @@ -353,6 +353,7 @@ struct qca8k_priv {
>  	struct dsa_switch_ops ops;
>  	struct gpio_desc *reset_gpio;
>  	unsigned int port_mtu[QCA8K_NUM_PORTS];
> +	const struct net_device *master; /* Track if mdio/mib Ethernet is available */

Maybe "mgmt_master" would be a clearer naming scheme?

>  };
>  
>  struct qca8k_mib_desc {
> -- 
> 2.33.1
> 

Re: [net-next PATCH RFC v6 10/16] net: dsa: tag_qca: add support for handling mdio Ethernet and MIB packet

[Vladimir Oltean]

On Tue, Dec 14, 2021 at 11:44:03PM +0100, Ansuel Smith wrote:
> 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           | 35 +++++++++++++++++++++++++++++++++--
>  2 files changed, 40 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/dsa/tag_qca.h b/include/linux/dsa/tag_qca.h
> index cd6275bac103..203e72dad9bb 100644
> --- a/include/linux/dsa/tag_qca.h
> +++ b/include/linux/dsa/tag_qca.h
> @@ -69,4 +69,11 @@ struct mib_ethhdr {
>  	__be16 hdr;		/* qca hdr */
>  } __packed;
>  
> +struct tag_qca_priv {

Could you keep the "priv" name for later, in case you need to hide some
tagger storage from the switch driver? This could be renamed to
"qca_tagger_data".

> +	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 f5547d357647..59e04157f53b 100644
> --- a/net/dsa/tag_qca.c
> +++ b/net/dsa/tag_qca.c
> @@ -32,11 +32,15 @@ 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;

cpu_dp, for clarity

> +	struct tag_qca_priv *priv;
>  	u16  hdr, pk_type;
>  	__be16 *phdr;
>  	int port;
>  	u8 ver;
>  
> +	priv = dp->ds->tagger_data;
> +
>  	if (unlikely(!pskb_may_pull(skb, QCA_HDR_LEN)))
>  		return NULL;
>  
> @@ -52,12 +56,18 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev)
>  	pk_type = FIELD_GET(QCA_HDR_RECV_TYPE, hdr);
>  
>  	/* Ethernet MDIO read/write packet */
> -	if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK)
> +	if (pk_type == QCA_HDR_RECV_TYPE_RW_REG_ACK) {
> +		if (priv->rw_reg_ack_handler)
> +			priv->rw_reg_ack_handler(dp, skb);

Minor nitpick, but why not pass the "ds" as argument?

>  		return NULL;
> +	}
>  
>  	/* Ethernet MIB counter packet */
> -	if (pk_type == QCA_HDR_RECV_TYPE_MIB)
> +	if (pk_type == QCA_HDR_RECV_TYPE_MIB) {
> +		if (priv->mib_autocast_handler)
> +			priv->mib_autocast_handler(dp, skb);
>  		return NULL;
> +	}
>  
>  	/* Remove QCA tag and recalculate checksum */
>  	skb_pull_rcsum(skb, QCA_HDR_LEN);
> @@ -73,9 +83,30 @@ 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 *ds)
> +{
> +	struct tag_qca_priv *priv;
> +
> +	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	ds->tagger_data = priv;
> +
> +	return 0;
> +}
> +
> +static void qca_tag_disconnect(struct dsa_switch *ds)
> +{
> +	kfree(ds->tagger_data);
> +	ds->tagger_data = NULL;
> +}
> +
>  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.33.1
> 

Re: [net-next PATCH RFC v6 08/16] net: dsa: tag_qca: add define for handling mdio Ethernet packet

[Vladimir Oltean]

On Tue, Dec 14, 2021 at 11:44:01PM +0100, Ansuel Smith wrote:
> 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..21cd0db5acc2 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 */
> +	__be16 hdr;		/* qca hdr */
> +} __packed;
> +
>  #endif /* __TAG_QCA_H */
> diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
> index f8df49d5956f..d30249b5205d 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;

Why the two spaces between "u16" and "hdr"?

>  	__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);
> +
> +	/* Ethernet 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.33.1
> 

Re: [net-next PATCH RFC v6 07/16] net: dsa: tag_qca: enable promisc_on_master flag

[Vladimir Oltean]

On Tue, Dec 14, 2021 at 11:44:00PM +0100, Ansuel Smith wrote:
> Ethernet MDIO packets are non-standard and DSA master expects the first
> 6 octets to be the MAC DA. To address these kind of packet, enable
> promisc_on_master flag for the tagger.
> 
> Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> ---
>  net/dsa/tag_qca.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
> index 34e565e00ece..f8df49d5956f 100644
> --- a/net/dsa/tag_qca.c
> +++ b/net/dsa/tag_qca.c
> @@ -68,6 +68,7 @@ static const struct dsa_device_ops qca_netdev_ops = {
>  	.xmit	= qca_tag_xmit,
>  	.rcv	= qca_tag_rcv,
>  	.needed_headroom = QCA_HDR_LEN,
> +	.promisc_on_master = true,
>  };
>  
>  MODULE_LICENSE("GPL");
> -- 
> 2.33.1
> 

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [net-next PATCH RFC v6 00/16] Add support for qca8k mdio rw in Ethernet packet

[Vladimir Oltean]

On Tue, Dec 14, 2021 at 11:43:53PM +0100, Ansuel Smith wrote:
> Hi, this is ready but require some additional test on a wider userbase.
> 
> 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.
> 
> Currently we use Ethernet packet for
> - MIB counter
> - mdio read/write configuration
> - phy read/write for each port
> 
> 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.
> 
> 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.

I don't think this paragraph is true anymore, since the initial binding
between the switch and the tagger is done from dsa_switch_setup() ->
dsa_switch_setup_tag_protocol(), which is called once per switch (due to
the ds->setup bool) and does not require cross-chip notifiers.

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

Only the dsa_tree_change_tag_proto() code path emits the cross-chip
notifier event that you mention. The qca8k doesn't support changing the
tagging protocol, therefore this isn't relevant.

> Current use for this is adding handler for the Ethernet packet to keep
> the tagger code as dumb as possible.
> 
> The tagger priv implement only the handler for the special packet. All the
> other stuff is placed in the qca8k_priv and the tagger has to access
> it under lock.
> 
> We use the new API from Vladimir to track if the master port is
> operational or not. We had to track many thing to reach a usable state.
> Checking if the port is UP is not enough and tracking a NETDEV_CHANGE is
> also not enough since it use also for other task. The correct way was
> both track for interface UP and if a qdisc was assigned to the
> interface. That tells us the port (and the tagger indirectly) is ready
> to accept and process packet.
> 
> I tested this with multicpu port and with port6 set as the unique port and
> it's sad.
> It seems they implemented this feature in a bad way and this is only
> supported with cpu port0. When cpu port6 is the unique port, the switch
> doesn't send ack packet. With multicpu port, packet ack are not duplicated
> and only cpu port0 sends them. This is the same for the MIB counter.
> For this reason this feature is enabled only when cpu port0 is enabled and
> operational.

Let's discuss this a bit (not the hardware limitation, that one is what
it is). When DSA has multiple CPU ports, right now both host-side
Ethernet ports are set up as DSA masters. By being a DSA master, I mean
that dev->dsa_ptr is a non-NULL pointer, so these interfaces expect to
receive packets that are trapped by the DSA packet_type handlers.
But due to the way in which dsa_tree_setup_default_cpu() is written,
by default only the first CPU port will be used. So the host port
attached to the second CPU port will be a DSA master technically, but it
will be an inactive one and won't be anyone's master (no dp->cpu_dp will
point to this master's dev->dsa_ptr). My idea of DSA support for
multiple CPU ports would be to be able to change the dp->cpu_dp mapping
through rtnetlink, on a per user port basis (yes, this implies we don't
have a solution for DSA ports).
My second observation is based on the fact that some switches support a
single CPU port, yet they are wired using two Ethernet ports towards the
host. The Felix and Seville switches are structured this way. I think
some Broadcom switches too.
Using the rtnetlink user API, a user could be able to migrate all user
ports between one CPU port and the other, and as long as the
configuration is valid, the switch driver should accept this (we perform
DSA master changing while all ports are down, and we could refuse going
up if e.g. some user ports are assigned to CPU port A and some user
ports to CPU port B). Nonetheless, the key point is that when a single
CPU port is used, the other CPU port kinda sits there doing nothing. So
I also have some patches that make the host port attached to this other
CPU port be a normal interface (not a DSA master).
The switch side of things is still a CPU port (not a user port, since
there still isn't any net device registered for it), but nonetheless, it
is a CPU port with no DSA tagging over it, hence the reason why the host
port isn't a DSA master. The patch itself that changes this behavior
sounds something like "only set up a host port as a DSA master if some
user ports are assigned to it".
As to why I'm doing it this way: the device tree should be fixed, and I
do need to describe the connection between the switch CPU ports and the
DSA masters via the 'ethernet = <&phandle>;' property. From a hardware
perspective, both switch ports A and B are CPU ports, equally. But this
means that DSA won't create a user port for the CPU port B, which would
be the more natural way to use it.
Now why this pertains to you: Vivien's initial stab at management over
Ethernet wanted to decouple a bit the concept of a DSA master (used for
the network stack) from the concept of a host port used for in-band
management (used for register access). Whereas our approach here is to
keep the two coupled, due to us saying "hey, if there's a direct
connection to the switch, this is a DSA master anyway, is it not?".
Well, here's one thing which you wouldn't be able to do if I pursue my
idea with lazy DSA master setup: if you decide to move all your user
ports using rtnetlink to CPU port 6, then the DSA master of CPU port 0
will cease to be a DSA master. So that will also prevent the management
protocol from working.
I don't want to break your use case, but then again, I'm wondering what
we could do to support the second CPU port working without DSA tagging,
without changing the device trees to declare it as a user port (which in
itself isn't bad, it's just that we need to support all use cases with a
single, unified device tree).

> Current concern are:
> - Any hint about the naming? Is calling this mdio Ethernet correct?
>   Should we use a more ""standard""/significant name? (considering also
>   other switch will implement this)

Responded inline to this too, I think "Ethernet management" may be
clearer than "MDIO Ethernet", but I don't have a strong preference.

> v6:
> - Fix some error in ethtool handler caused by rebase/cleanup
> v5:
> - Adapt to new API fixes
> - Fix a wrong logic for noop
> - Add additional lock for master_state change
> - Limit mdio Ethernet to cpu port0 (switch limitation)
> - Add priority to these special packet
> - Move mdio cache to qca8k_priv
> v4:
> - Remove duplicate patch sent by mistake.
> v3:
> - Include MIB with Ethernet packet.
> - Include phy read/write with Ethernet packet.
> - Reorganize code with new API.
> - Introuce master tracking by Vladimir
> 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 (12):
>   net: dsa: tag_qca: convert to FIELD macro
>   net: dsa: tag_qca: move define to include linux/dsa
>   net: dsa: tag_qca: enable promisc_on_master flag
>   net: dsa: tag_qca: add define for handling mdio Ethernet packet
>   net: dsa: tag_qca: add define for handling MIB packet
>   net: dsa: tag_qca: add support for handling mdio Ethernet and MIB
>     packet
>   net: dsa: qca8k: add tracking state of master port
>   net: dsa: qca8k: add support for mdio read/write in Ethernet packet
>   net: dsa: qca8k: add support for mib autocast in Ethernet packet
>   net: dsa: qca8k: add support for phy read/write with mdio Ethernet
>   net: dsa: qca8k: move page cache to driver priv
>   net: dsa: qca8k: cache lo and hi for mdio write
> 
> Vladimir Oltean (4):
>   net: dsa: provide switch operations for tracking the master state
>   net: dsa: stop updating master MTU from master.c
>   net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
>   net: dsa: replay master state events in
>     dsa_tree_{setup,teardown}_master
> 
>  drivers/net/dsa/qca8k.c     | 600 ++++++++++++++++++++++++++++++++++--
>  drivers/net/dsa/qca8k.h     |  46 ++-
>  include/linux/dsa/tag_qca.h |  79 +++++
>  include/net/dsa.h           |  17 +
>  net/dsa/dsa2.c              |  81 ++++-
>  net/dsa/dsa_priv.h          |  13 +
>  net/dsa/master.c            |  29 +-
>  net/dsa/slave.c             |  32 ++
>  net/dsa/switch.c            |  15 +
>  net/dsa/tag_qca.c           |  81 +++--
>  10 files changed, 901 insertions(+), 92 deletions(-)
>  create mode 100644 include/linux/dsa/tag_qca.h
> 
> -- 
> 2.33.1
> 

Re: [net-next PATCH RFC v6 12/16] net: dsa: qca8k: add support for mdio read/write in Ethernet packet

[Vladimir Oltean]

On Tue, Dec 14, 2021 at 11:44:05PM +0100, Ansuel Smith wrote:
> +static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
> +				      enum dsa_tag_protocol proto)
> +{
> +	struct qca8k_priv *qca8k_priv = ds->priv;
> +
> +	switch (proto) {
> +	case DSA_TAG_PROTO_QCA:
> +		struct tag_qca_priv *priv;

Actually this fails to compile:

drivers/net/dsa/qca8k.c: In function ‘qca8k_connect_tag_protocol’:
drivers/net/dsa/qca8k.c:2893:3: error: a label can only be part of a statement and a declaration is not a statement
 2893 |   struct tag_qca_priv *priv;
      |   ^~~~~~
make[3]: *** [scripts/Makefile.build:287: drivers/net/dsa/qca8k.o] Error 1

This is what the {} brackets are for.

Also, while you're at this, please name "priv" "tagger_data".

> +
> +		priv = ds->tagger_data;
> +
> +		mutex_init(&qca8k_priv->mdio_hdr_data.mutex);
> +		init_completion(&qca8k_priv->mdio_hdr_data.rw_done);
> +
> +		priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
> +
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
>  }

Re: [net-next PATCH RFC v6 00/16] Add support for qca8k mdio rw in Ethernet packet

[Ansuel Smith]

On Wed, Dec 15, 2021 at 12:26:29PM +0200, Vladimir Oltean wrote:
> On Tue, Dec 14, 2021 at 11:43:53PM +0100, Ansuel Smith wrote:
> > Hi, this is ready but require some additional test on a wider userbase.
> > 
> > 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.
> > 
> > Currently we use Ethernet packet for
> > - MIB counter
> > - mdio read/write configuration
> > - phy read/write for each port
> > 
> > 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.
> > 
> > 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.
> 
> I don't think this paragraph is true anymore, since the initial binding
> between the switch and the tagger is done from dsa_switch_setup() ->
> dsa_switch_setup_tag_protocol(), which is called once per switch (due to
> the ds->setup bool) and does not require cross-chip notifiers.
> 
> > 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.
> 
> Only the dsa_tree_change_tag_proto() code path emits the cross-chip
> notifier event that you mention. The qca8k doesn't support changing the
> tagging protocol, therefore this isn't relevant.
> 
> > Current use for this is adding handler for the Ethernet packet to keep
> > the tagger code as dumb as possible.
> > 
> > The tagger priv implement only the handler for the special packet. All the
> > other stuff is placed in the qca8k_priv and the tagger has to access
> > it under lock.
> > 
> > We use the new API from Vladimir to track if the master port is
> > operational or not. We had to track many thing to reach a usable state.
> > Checking if the port is UP is not enough and tracking a NETDEV_CHANGE is
> > also not enough since it use also for other task. The correct way was
> > both track for interface UP and if a qdisc was assigned to the
> > interface. That tells us the port (and the tagger indirectly) is ready
> > to accept and process packet.
> > 
> > I tested this with multicpu port and with port6 set as the unique port and
> > it's sad.
> > It seems they implemented this feature in a bad way and this is only
> > supported with cpu port0. When cpu port6 is the unique port, the switch
> > doesn't send ack packet. With multicpu port, packet ack are not duplicated
> > and only cpu port0 sends them. This is the same for the MIB counter.
> > For this reason this feature is enabled only when cpu port0 is enabled and
> > operational.
> 
> Let's discuss this a bit (not the hardware limitation, that one is what
> it is). When DSA has multiple CPU ports, right now both host-side
> Ethernet ports are set up as DSA masters. By being a DSA master, I mean
> that dev->dsa_ptr is a non-NULL pointer, so these interfaces expect to
> receive packets that are trapped by the DSA packet_type handlers.
> But due to the way in which dsa_tree_setup_default_cpu() is written,
> by default only the first CPU port will be used. So the host port
> attached to the second CPU port will be a DSA master technically, but it
> will be an inactive one and won't be anyone's master (no dp->cpu_dp will
> point to this master's dev->dsa_ptr). My idea of DSA support for
> multiple CPU ports would be to be able to change the dp->cpu_dp mapping
> through rtnetlink, on a per user port basis (yes, this implies we don't
> have a solution for DSA ports).

I have a similar implementation that was proposed as RFC many times ago.

> My second observation is based on the fact that some switches support a
> single CPU port, yet they are wired using two Ethernet ports towards the
> host. The Felix and Seville switches are structured this way. I think
> some Broadcom switches too.
> Using the rtnetlink user API, a user could be able to migrate all user
> ports between one CPU port and the other, and as long as the
> configuration is valid, the switch driver should accept this (we perform
> DSA master changing while all ports are down, and we could refuse going
> up if e.g. some user ports are assigned to CPU port A and some user
> ports to CPU port B). Nonetheless, the key point is that when a single
> CPU port is used, the other CPU port kinda sits there doing nothing. So
> I also have some patches that make the host port attached to this other
> CPU port be a normal interface (not a DSA master).
> The switch side of things is still a CPU port (not a user port, since
> there still isn't any net device registered for it), but nonetheless, it
> is a CPU port with no DSA tagging over it, hence the reason why the host
> port isn't a DSA master. The patch itself that changes this behavior
> sounds something like "only set up a host port as a DSA master if some
> user ports are assigned to it".
> As to why I'm doing it this way: the device tree should be fixed, and I
> do need to describe the connection between the switch CPU ports and the
> DSA masters via the 'ethernet = <&phandle>;' property. From a hardware
> perspective, both switch ports A and B are CPU ports, equally. But this
> means that DSA won't create a user port for the CPU port B, which would
> be the more natural way to use it.
> Now why this pertains to you: Vivien's initial stab at management over
> Ethernet wanted to decouple a bit the concept of a DSA master (used for
> the network stack) from the concept of a host port used for in-band
> management (used for register access). Whereas our approach here is to
> keep the two coupled, due to us saying "hey, if there's a direct
> connection to the switch, this is a DSA master anyway, is it not?".
> Well, here's one thing which you wouldn't be able to do if I pursue my
> idea with lazy DSA master setup: if you decide to move all your user
> ports using rtnetlink to CPU port 6, then the DSA master of CPU port 0
> will cease to be a DSA master. So that will also prevent the management
> protocol from working.

About the migration problem, wonder if we can just use a refcount that
would represent the user of the master port. The port won't be DSA
master anymore if no user are connected. A switch can increase this ref
if the port is mandatory for some operation. (qca8k on state change
operational would increase the ref and decrease and then the port can be
removed from a DSA master) That should handle all the other switch and
still permit a driver to ""bypass"" this behaviour.

> I don't want to break your use case, but then again, I'm wondering what
> we could do to support the second CPU port working without DSA tagging,
> without changing the device trees to declare it as a user port (which in
> itself isn't bad, it's just that we need to support all use cases with a
> single, unified device tree).

Just some info about the secondary CPU port.
From Documentation the second cpu port in sgmii mode can be used also for
other task so yes we should understand how to handle this. (base-x, mac
and phy) This mode is set based on the phy mode and if the dsa port is a
cpu port. Device tree changes can be accepted as AFAIK due to DSA not
supporting multi cpu, CPU port 6 was never used/defined. (But I'm
not sure... that is the case for all the device we have on openwrt)

Considering that introducing multicpu port would require a
bit of rework, wonder if we should introduce some new bindings/node and
fallback to a legacy (aka force first cpu port as the unique cpu port
and ignore others) in the absence of this new implementation. (Hoping I
didn't get all wrong with the main problem here)
But if I'm not wrong there was at the start some years ago an idea of a
node used to declare master port separate from the generic port node but
it was rejected?

> 
> > Current concern are:
> > - Any hint about the naming? Is calling this mdio Ethernet correct?
> >   Should we use a more ""standard""/significant name? (considering also
> >   other switch will implement this)
> 
> Responded inline to this too, I think "Ethernet management" may be
> clearer than "MDIO Ethernet", but I don't have a strong preference.
> 
> > v6:
> > - Fix some error in ethtool handler caused by rebase/cleanup
> > v5:
> > - Adapt to new API fixes
> > - Fix a wrong logic for noop
> > - Add additional lock for master_state change
> > - Limit mdio Ethernet to cpu port0 (switch limitation)
> > - Add priority to these special packet
> > - Move mdio cache to qca8k_priv
> > v4:
> > - Remove duplicate patch sent by mistake.
> > v3:
> > - Include MIB with Ethernet packet.
> > - Include phy read/write with Ethernet packet.
> > - Reorganize code with new API.
> > - Introuce master tracking by Vladimir
> > 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 (12):
> >   net: dsa: tag_qca: convert to FIELD macro
> >   net: dsa: tag_qca: move define to include linux/dsa
> >   net: dsa: tag_qca: enable promisc_on_master flag
> >   net: dsa: tag_qca: add define for handling mdio Ethernet packet
> >   net: dsa: tag_qca: add define for handling MIB packet
> >   net: dsa: tag_qca: add support for handling mdio Ethernet and MIB
> >     packet
> >   net: dsa: qca8k: add tracking state of master port
> >   net: dsa: qca8k: add support for mdio read/write in Ethernet packet
> >   net: dsa: qca8k: add support for mib autocast in Ethernet packet
> >   net: dsa: qca8k: add support for phy read/write with mdio Ethernet
> >   net: dsa: qca8k: move page cache to driver priv
> >   net: dsa: qca8k: cache lo and hi for mdio write
> > 
> > Vladimir Oltean (4):
> >   net: dsa: provide switch operations for tracking the master state
> >   net: dsa: stop updating master MTU from master.c
> >   net: dsa: hold rtnl_mutex when calling dsa_master_{setup,teardown}
> >   net: dsa: replay master state events in
> >     dsa_tree_{setup,teardown}_master
> > 
> >  drivers/net/dsa/qca8k.c     | 600 ++++++++++++++++++++++++++++++++++--
> >  drivers/net/dsa/qca8k.h     |  46 ++-
> >  include/linux/dsa/tag_qca.h |  79 +++++
> >  include/net/dsa.h           |  17 +
> >  net/dsa/dsa2.c              |  81 ++++-
> >  net/dsa/dsa_priv.h          |  13 +
> >  net/dsa/master.c            |  29 +-
> >  net/dsa/slave.c             |  32 ++
> >  net/dsa/switch.c            |  15 +
> >  net/dsa/tag_qca.c           |  81 +++--
> >  10 files changed, 901 insertions(+), 92 deletions(-)
> >  create mode 100644 include/linux/dsa/tag_qca.h
> > 
> > -- 
> > 2.33.1
> > 
> 

-- 
	Ansuel

Re: [net-next PATCH RFC v6 12/16] net: dsa: qca8k: add support for mdio read/write in Ethernet packet

[Ansuel Smith]

On Wed, Dec 15, 2021 at 11:49:12AM +0200, Vladimir Oltean wrote:
> On Tue, Dec 14, 2021 at 11:44:05PM +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.
> > 
> > Locking is added to qca8k_master_change() to make sure no mdio Ethernet
> > are in progress.
> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> >  drivers/net/dsa/qca8k.c | 192 ++++++++++++++++++++++++++++++++++++++++
> >  drivers/net/dsa/qca8k.h |  13 +++
> >  2 files changed, 205 insertions(+)
> > 
> > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
> > index f317f527dd6d..b35ba26a0696 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,6 +171,158 @@ qca8k_rmw(struct qca8k_priv *priv, u32 reg, u32 mask, u32 write_val)
> >  	return regmap_update_bits(priv->regmap, reg, mask, write_val);
> >  }
> >  
> > +static void qca8k_rw_reg_ack_handler(struct dsa_port *dp, struct sk_buff *skb)
> > +{
> > +	struct qca8k_mdio_hdr_data *mdio_hdr_data;
> > +	struct qca8k_priv *priv = dp->ds->priv;
> > +	struct mdio_ethhdr *mdio_ethhdr;
> > +	u8 len, cmd;
> > +
> > +	mdio_ethhdr = (struct mdio_ethhdr *)skb_mac_header(skb);
> > +	mdio_hdr_data = &priv->mdio_hdr_data;
> > +
> > +	cmd = FIELD_GET(QCA_HDR_MDIO_CMD, mdio_ethhdr->command);
> > +	len = FIELD_GET(QCA_HDR_MDIO_LENGTH, mdio_ethhdr->command);
> > +
> > +	/* Make sure the seq match the requested packet */
> > +	if (mdio_ethhdr->seq == mdio_hdr_data->seq)
> > +		mdio_hdr_data->ack = true;
> > +
> > +	if (cmd == MDIO_READ) {
> > +		mdio_hdr_data->data[0] = mdio_ethhdr->mdio_data;
> > +
> > +		/* Get the rest of the 12 byte of data */
> > +		if (len > QCA_HDR_MDIO_DATA1_LEN)
> > +			memcpy(mdio_hdr_data->data + 1, skb->data,
> > +			       QCA_HDR_MDIO_DATA2_LEN);
> > +	}
> > +
> > +	complete(&mdio_hdr_data->rw_done);
> > +}
> > +
> > +static struct sk_buff *qca8k_alloc_mdio_header(enum mdio_cmd cmd, u32 reg, u32 *val,
> > +					       int seq_num, int priority)
> > +{
> > +	struct mdio_ethhdr *mdio_ethhdr;
> > +	struct sk_buff *skb;
> > +	u16 hdr;
> > +
> > +	skb = dev_alloc_skb(QCA_HDR_MDIO_PKG_LEN);
> 
> Still no if (!skb) checking... Not only here, but also at the call sites
> of this.
> 
> > +
> > +	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 |= FIELD_PREP(QCA_HDR_XMIT_PRIORITY, priority);
> > +	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, seq_num);
> > +
> > +	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);
> 
> Maybe a single call to skb_put_zero, and pass the sum as argument?
> 
> > +
> > +	return skb;
> > +}
> > +
> > +static int qca8k_read_eth(struct qca8k_priv *priv, u32 reg, u32 *val)
> > +{
> > +	struct qca8k_mdio_hdr_data *mdio_hdr_data = &priv->mdio_hdr_data;
> > +	struct sk_buff *skb;
> > +	bool ack;
> > +	int ret;
> > +
> > +	skb = qca8k_alloc_mdio_header(MDIO_READ, reg, NULL, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> 
> You hardcode "seq" to 200? Aren't you supposed to increment it or
> something?
>

We enforce one operation at time using lock. Seq is currently used to
check if the packet is correct.

> > +	skb->dev = (struct net_device *)priv->master;
> 
> You access priv->master outside of priv->mdio_hdr_data.mutex from
> qca8k_master_change(), that can't be good.
> 

Tell me if the logic is correct.
qca8k_master_change() removes or sets the priv->master under lock (so no
operation in progress)
A read/write checks if priv->master is not NULL and try to use this
alternative way. (not under lock)

Think I should remove the priv->master check from the read/write and
move it here under lock (and release the lock if it's not defined)
(The main idea is try to keep the skb alloc and packet setup outside of
locking to save some locking time)

Can I check priv->master 2 times? One outside the lock and one under
lock when is actually used by the skb? To skip locking and releasing for
every read/write if the alternative way is not available. 

> > +
> > +	mutex_lock(&mdio_hdr_data->mutex);
> > +
> > +	reinit_completion(&mdio_hdr_data->rw_done);
> > +	mdio_hdr_data->seq = 200;
> 
> Why do you rewrite the seq here?
> 

Seq is checked by the handler. Should I set it one time in probe?

> > +	mdio_hdr_data->ack = false;
> > +
> > +	dev_queue_xmit(skb);
> > +
> > +	ret = wait_for_completion_timeout(&mdio_hdr_data->rw_done,
> > +					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
> > +
> > +	*val = mdio_hdr_data->data[0];
> > +	ack = mdio_hdr_data->ack;
> > +
> > +	mutex_unlock(&mdio_hdr_data->mutex);
> > +
> > +	if (ret <= 0)
> > +		return -ETIMEDOUT;
> > +
> > +	if (!ack)
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int qca8k_write_eth(struct qca8k_priv *priv, u32 reg, u32 val)
> > +{
> > +	struct qca8k_mdio_hdr_data *mdio_hdr_data = &priv->mdio_hdr_data;
> > +	struct sk_buff *skb;
> > +	bool ack;
> > +	int ret;
> > +
> > +	skb = qca8k_alloc_mdio_header(MDIO_WRITE, reg, &val, 200, QCA8K_ETHERNET_MDIO_PRIORITY);
> > +	skb->dev = (struct net_device *)priv->master;
> > +
> > +	mutex_lock(&mdio_hdr_data->mutex);
> > +
> > +	reinit_completion(&mdio_hdr_data->rw_done);
> > +	mdio_hdr_data->ack = false;
> > +	mdio_hdr_data->seq = 200;
> > +
> > +	dev_queue_xmit(skb);
> > +
> > +	ret = wait_for_completion_timeout(&mdio_hdr_data->rw_done,
> > +					  msecs_to_jiffies(QCA8K_ETHERNET_TIMEOUT));
> > +
> > +	ack = mdio_hdr_data->ack;
> > +
> > +	mutex_unlock(&mdio_hdr_data->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)
> >  {
> > @@ -178,6 +331,9 @@ qca8k_regmap_read(void *ctx, uint32_t reg, uint32_t *val)
> >  	u16 r1, r2, page;
> >  	int ret;
> >  
> > +	if (priv->master && !qca8k_read_eth(priv, reg, val))
> > +		return 0;
> > +
> >  	qca8k_split_addr(reg, &r1, &r2, &page);
> >  
> >  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> > @@ -201,6 +357,9 @@ qca8k_regmap_write(void *ctx, uint32_t reg, uint32_t val)
> >  	u16 r1, r2, page;
> >  	int ret;
> >  
> > +	if (priv->master && !qca8k_write_eth(priv, reg, val))
> > +		return 0;
> > +
> >  	qca8k_split_addr(reg, &r1, &r2, &page);
> >  
> >  	mutex_lock_nested(&bus->mdio_lock, MDIO_MUTEX_NESTED);
> > @@ -225,6 +384,10 @@ qca8k_regmap_update_bits(void *ctx, uint32_t reg, uint32_t mask, uint32_t write_
> >  	u32 val;
> >  	int ret;
> >  
> > +	if (priv->master &&
> > +	    !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);
> > @@ -2394,10 +2557,38 @@ qca8k_master_change(struct dsa_switch *ds, const struct net_device *master,
> >  	if (dp->index != 0)
> >  		return;
> >  
> > +	mutex_lock(&priv->mdio_hdr_data.mutex);
> > +
> >  	if (operational)
> >  		priv->master = master;
> >  	else
> >  		priv->master = NULL;
> > +
> > +	mutex_unlock(&priv->mdio_hdr_data.mutex);
> > +}
> > +
> > +static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
> > +				      enum dsa_tag_protocol proto)
> > +{
> > +	struct qca8k_priv *qca8k_priv = ds->priv;
> > +
> > +	switch (proto) {
> > +	case DSA_TAG_PROTO_QCA:
> > +		struct tag_qca_priv *priv;
> > +
> > +		priv = ds->tagger_data;
> > +
> > +		mutex_init(&qca8k_priv->mdio_hdr_data.mutex);
> > +		init_completion(&qca8k_priv->mdio_hdr_data.rw_done);
> 
> I think having these initializations here dilutes the purpose of this
> callback. Could you please move these two lines to qca8k_sw_probe()?
> 
> > +
> > +		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 = {
> > @@ -2436,6 +2627,7 @@ static const struct dsa_switch_ops qca8k_switch_ops = {
> >  	.port_lag_join		= qca8k_port_lag_join,
> >  	.port_lag_leave		= qca8k_port_lag_leave,
> >  	.master_state_change	= qca8k_master_change,
> > +	.connect_tag_protocol	= qca8k_connect_tag_protocol,
> >  };
> >  
> >  static int qca8k_read_switch_id(struct qca8k_priv *priv)
> > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> > index 6edd6adc3063..dbe8c74c9793 100644
> > --- a/drivers/net/dsa/qca8k.h
> > +++ b/drivers/net/dsa/qca8k.h
> > @@ -11,6 +11,10 @@
> >  #include <linux/delay.h>
> >  #include <linux/regmap.h>
> >  #include <linux/gpio.h>
> > +#include <linux/dsa/tag_qca.h>
> > +
> > +#define QCA8K_ETHERNET_MDIO_PRIORITY			7
> > +#define QCA8K_ETHERNET_TIMEOUT				100
> >  
> >  #define QCA8K_NUM_PORTS					7
> >  #define QCA8K_NUM_CPU_PORTS				2
> > @@ -328,6 +332,14 @@ enum {
> >  	QCA8K_CPU_PORT6,
> >  };
> >  
> > +struct qca8k_mdio_hdr_data {
> 
> What do you think about the "qca8k_eth_mgmt_data" name rather than
> "mdio_hdr_data"? I don't think this has anything to do with MDIO.
> 
> > +	struct completion rw_done;
> > +	struct mutex mutex; /* Enforce one mdio read/write at time */
> > +	bool ack;
> > +	u32 seq;
> > +	u32 data[4];
> > +};
> > +
> >  struct qca8k_ports_config {
> >  	bool sgmii_rx_clk_falling_edge;
> >  	bool sgmii_tx_clk_falling_edge;
> > @@ -354,6 +366,7 @@ struct qca8k_priv {
> >  	struct gpio_desc *reset_gpio;
> >  	unsigned int port_mtu[QCA8K_NUM_PORTS];
> >  	const struct net_device *master; /* Track if mdio/mib Ethernet is available */
> > +	struct qca8k_mdio_hdr_data mdio_hdr_data;
> >  };
> >  
> >  struct qca8k_mib_desc {
> > -- 
> > 2.33.1
> > 

-- 
	Ansuel

Re: [net-next PATCH RFC v6 12/16] net: dsa: qca8k: add support for mdio read/write in Ethernet packet

[Ansuel Smith]

On Wed, Dec 15, 2021 at 02:47:58PM +0200, Vladimir Oltean wrote:
> On Tue, Dec 14, 2021 at 11:44:05PM +0100, Ansuel Smith wrote:
> > +static int qca8k_connect_tag_protocol(struct dsa_switch *ds,
> > +				      enum dsa_tag_protocol proto)
> > +{
> > +	struct qca8k_priv *qca8k_priv = ds->priv;
> > +
> > +	switch (proto) {
> > +	case DSA_TAG_PROTO_QCA:
> > +		struct tag_qca_priv *priv;
> 
> Actually this fails to compile:
> 
> drivers/net/dsa/qca8k.c: In function ‘qca8k_connect_tag_protocol’:
> drivers/net/dsa/qca8k.c:2893:3: error: a label can only be part of a statement and a declaration is not a statement
>  2893 |   struct tag_qca_priv *priv;
>       |   ^~~~~~
> make[3]: *** [scripts/Makefile.build:287: drivers/net/dsa/qca8k.o] Error 1
> 
> This is what the {} brackets are for.
> 
> Also, while you're at this, please name "priv" "tagger_data".
>

I didn't have this error, sorry. Just to make sure I didn't make these kind of
error anymore what compile did you use and with what flags? 

> > +
> > +		priv = ds->tagger_data;
> > +
> > +		mutex_init(&qca8k_priv->mdio_hdr_data.mutex);
> > +		init_completion(&qca8k_priv->mdio_hdr_data.rw_done);
> > +
> > +		priv->rw_reg_ack_handler = qca8k_rw_reg_ack_handler;
> > +
> > +		break;
> > +	default:
> > +		return -EOPNOTSUPP;
> > +	}
> > +
> > +	return 0;
> >  }

-- 
	Ansuel

Re: [net-next PATCH RFC v6 11/16] net: dsa: qca8k: add tracking state of master port

[Ansuel Smith]

On Wed, Dec 15, 2021 at 11:51:46AM +0200, Vladimir Oltean wrote:
> On Tue, Dec 14, 2021 at 11:44:04PM +0100, Ansuel Smith wrote:
> > MDIO/MIB Ethernet require the master port and the tagger availabale to
> > correctly work. Use the new api master_state_change to track when master
> > is operational or not and set a bool in qca8k_priv.
> > We cache the first cached master available and we check if other cpu
> > port are operational when the cached one goes down.
> > This cached master will later be used by mdio read/write and mib request to
> > correctly use the working function.
> > 
> > qca8k implementation for MDIO/MIB Ethernet is bad. CPU port0 is the only
> > one that answers with the ack packet or sends MIB Ethernet packets. For
> > this reason the master_state_change ignore CPU port6 and checkes only
> > CPU port0 if it's operational and enables this mode.
> 
> CPU port 0 may not always be wired, it depends on board design, right?
> So the Ethernet management protocol may or may not be available to all users.
>

Out of 130 device we found only 2 device that had cpu 0 disconnected and
were Xiaomi device (so not really top of following standard and advised
config from qcom). Only qca833x supports the use of cpu port6 as an
alternative port. qca8327 doesn't work with cpu port6 as the only cpu
port and cpu0 port is mandatory.
I also tested out of curiosity if mac swap handle this case and no luck,
the switch still doesn't answer with ack packet.
(config I tested was declaring only port6 as cpu port, forcing mac swap
and test if this works. Every request timeouts. Normal connection works 
and the tagger correctly works.)

> > 
> > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
> > ---
> > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h
> > index ab4a417b25a9..6edd6adc3063 100644
> > --- a/drivers/net/dsa/qca8k.h
> > +++ b/drivers/net/dsa/qca8k.h
> > @@ -353,6 +353,7 @@ struct qca8k_priv {
> >  	struct dsa_switch_ops ops;
> >  	struct gpio_desc *reset_gpio;
> >  	unsigned int port_mtu[QCA8K_NUM_PORTS];
> > +	const struct net_device *master; /* Track if mdio/mib Ethernet is available */
> 
> Maybe "mgmt_master" would be a clearer naming scheme?
> 
> >  };
> >  
> >  struct qca8k_mib_desc {
> > -- 
> > 2.33.1
> > 
> 

-- 
	Ansuel

Re: [net-next PATCH RFC v6 00/16] Add support for qca8k mdio rw in Ethernet packet

[Vladimir Oltean]

On Wed, Dec 15, 2021 at 05:34:45PM +0100, Ansuel Smith wrote:
> > > I tested this with multicpu port and with port6 set as the unique port and
> > > it's sad.
> > > It seems they implemented this feature in a bad way and this is only
> > > supported with cpu port0. When cpu port6 is the unique port, the switch
> > > doesn't send ack packet. With multicpu port, packet ack are not duplicated
> > > and only cpu port0 sends them. This is the same for the MIB counter.
> > > For this reason this feature is enabled only when cpu port0 is enabled and
> > > operational.
> > 
> > Let's discuss this a bit (not the hardware limitation, that one is what
> > it is). When DSA has multiple CPU ports, right now both host-side
> > Ethernet ports are set up as DSA masters. By being a DSA master, I mean
> > that dev->dsa_ptr is a non-NULL pointer, so these interfaces expect to
> > receive packets that are trapped by the DSA packet_type handlers.
> > But due to the way in which dsa_tree_setup_default_cpu() is written,
> > by default only the first CPU port will be used. So the host port
> > attached to the second CPU port will be a DSA master technically, but it
> > will be an inactive one and won't be anyone's master (no dp->cpu_dp will
> > point to this master's dev->dsa_ptr). My idea of DSA support for
> > multiple CPU ports would be to be able to change the dp->cpu_dp mapping
> > through rtnetlink, on a per user port basis (yes, this implies we don't
> > have a solution for DSA ports).
> 
> I have a similar implementation that was proposed as RFC many times ago.

Yes, well, how to assign a user port to a CPU port seems not to be the
biggest problem that needs to be solved before support for multiple CPU
ports can fully go in.

> > My second observation is based on the fact that some switches support a
> > single CPU port, yet they are wired using two Ethernet ports towards the
> > host. The Felix and Seville switches are structured this way. I think
> > some Broadcom switches too.
> > Using the rtnetlink user API, a user could be able to migrate all user
> > ports between one CPU port and the other, and as long as the
> > configuration is valid, the switch driver should accept this (we perform
> > DSA master changing while all ports are down, and we could refuse going
> > up if e.g. some user ports are assigned to CPU port A and some user
> > ports to CPU port B). Nonetheless, the key point is that when a single
> > CPU port is used, the other CPU port kinda sits there doing nothing. So
> > I also have some patches that make the host port attached to this other
> > CPU port be a normal interface (not a DSA master).
> > The switch side of things is still a CPU port (not a user port, since
> > there still isn't any net device registered for it), but nonetheless, it
> > is a CPU port with no DSA tagging over it, hence the reason why the host
> > port isn't a DSA master. The patch itself that changes this behavior
> > sounds something like "only set up a host port as a DSA master if some
> > user ports are assigned to it".
> > As to why I'm doing it this way: the device tree should be fixed, and I
> > do need to describe the connection between the switch CPU ports and the
> > DSA masters via the 'ethernet = <&phandle>;' property. From a hardware
> > perspective, both switch ports A and B are CPU ports, equally. But this
> > means that DSA won't create a user port for the CPU port B, which would
> > be the more natural way to use it.
> > Now why this pertains to you: Vivien's initial stab at management over
> > Ethernet wanted to decouple a bit the concept of a DSA master (used for
> > the network stack) from the concept of a host port used for in-band
> > management (used for register access). Whereas our approach here is to
> > keep the two coupled, due to us saying "hey, if there's a direct
> > connection to the switch, this is a DSA master anyway, is it not?".
> > Well, here's one thing which you wouldn't be able to do if I pursue my
> > idea with lazy DSA master setup: if you decide to move all your user
> > ports using rtnetlink to CPU port 6, then the DSA master of CPU port 0
> > will cease to be a DSA master. So that will also prevent the management
> > protocol from working.
> 
> About the migration problem, wonder if we can just use a refcount that
> would represent the user of the master port. The port won't be DSA
> master anymore if no user are connected. A switch can increase this ref
> if the port is mandatory for some operation. (qca8k on state change
> operational would increase the ref and decrease and then the port can be
> removed from a DSA master) That should handle all the other switch and
> still permit a driver to ""bypass"" this behaviour.

Maybe. Although not quite like the way in which you propose. Remember
that the idea is for a DSA master to be a regular interface until it
gains a user. So there's the chicken and egg problem if you want to
become a user on ->master_state_change()... because it's not a master.
You'd have to specify upfront.

> > I don't want to break your use case, but then again, I'm wondering what
> > we could do to support the second CPU port working without DSA tagging,
> > without changing the device trees to declare it as a user port (which in
> > itself isn't bad, it's just that we need to support all use cases with a
> > single, unified device tree).
> 
> Just some info about the secondary CPU port.
> From Documentation the second cpu port in sgmii mode can be used also for
> other task so yes we should understand how to handle this. (base-x, mac
> and phy) This mode is set based on the phy mode and if the dsa port is a
> cpu port. Device tree changes can be accepted as AFAIK due to DSA not
> supporting multi cpu, CPU port 6 was never used/defined. (But I'm
> not sure... that is the case for all the device we have on openwrt)

What do you mean exactly by "other tasks"?

> Considering that introducing multicpu port would require a
> bit of rework, wonder if we should introduce some new bindings/node and
> fallback to a legacy (aka force first cpu port as the unique cpu port
> and ignore others) in the absence of this new implementation. (Hoping I
> didn't get all wrong with the main problem here)

The defaults would stay the same. (I've no idea why we would introduce
new device tree bindings? the only device tree change IMO would be to
declare the link between the second CPU port and its DSA master, if you
haven't done that already) But my key point was that, to some extent,
some change to the current behavior will still be required. Like right
now, a kernel 5.15 when it sees a device tree with 2 CPU ports will have
2 DSA masters. Maybe kernel 5.17 will only start off with the first port
as a DSA master, and the other just a candidate. I'm hoping this won't
change observable behavior for the worse for anyone, because device
trees are supposed to be there to stay, not change on a whim. My hope is
based on the fact that as far as I can see, that second DSA master is
effectively useless. Which in fact creates the second problem: exactly
because the second host port is useless with the current code structure,
I can see people describing it as a user rather than CPU port in current
device trees, just to make some use out of it. But that restricts the
potential user base (and therefore appeal) of my change of behavior or
of multi-CPU port support in general, and this is a bit sad. I think we
should be all spending a bit more time with current-generation kernels
on "updated" device trees with multiple CPU ports defined, and see
what's broken and what could be improved, because otherwise we could be
leaving behind a huge mess when the device trees get updated, and we
need to run the occasional old kernel on them.

> But if I'm not wrong there was at the start some years ago an idea of a
> node used to declare master port separate from the generic port node but
> it was rejected?

I don't know anything about this.

Re: [net-next PATCH RFC v6 00/16] Add support for qca8k mdio rw in Ethernet packet

[Ansuel Smith]

On Fri, Dec 17, 2021 at 01:38:12AM +0200, Vladimir Oltean wrote:
> On Wed, Dec 15, 2021 at 05:34:45PM +0100, Ansuel Smith wrote:
> > > > I tested this with multicpu port and with port6 set as the unique port and
> > > > it's sad.
> > > > It seems they implemented this feature in a bad way and this is only
> > > > supported with cpu port0. When cpu port6 is the unique port, the switch
> > > > doesn't send ack packet. With multicpu port, packet ack are not duplicated
> > > > and only cpu port0 sends them. This is the same for the MIB counter.
> > > > For this reason this feature is enabled only when cpu port0 is enabled and
> > > > operational.
> > > 
> > > Let's discuss this a bit (not the hardware limitation, that one is what
> > > it is). When DSA has multiple CPU ports, right now both host-side
> > > Ethernet ports are set up as DSA masters. By being a DSA master, I mean
> > > that dev->dsa_ptr is a non-NULL pointer, so these interfaces expect to
> > > receive packets that are trapped by the DSA packet_type handlers.
> > > But due to the way in which dsa_tree_setup_default_cpu() is written,
> > > by default only the first CPU port will be used. So the host port
> > > attached to the second CPU port will be a DSA master technically, but it
> > > will be an inactive one and won't be anyone's master (no dp->cpu_dp will
> > > point to this master's dev->dsa_ptr). My idea of DSA support for
> > > multiple CPU ports would be to be able to change the dp->cpu_dp mapping
> > > through rtnetlink, on a per user port basis (yes, this implies we don't
> > > have a solution for DSA ports).
> > 
> > I have a similar implementation that was proposed as RFC many times ago.
> 
> Yes, well, how to assign a user port to a CPU port seems not to be the
> biggest problem that needs to be solved before support for multiple CPU
> ports can fully go in.
>

Hi,
sorry for the delay.

I honestly think a start for multicpu that would improve the state would
be start adding support to iproute for changing the master port.

> > > My second observation is based on the fact that some switches support a
> > > single CPU port, yet they are wired using two Ethernet ports towards the
> > > host. The Felix and Seville switches are structured this way. I think
> > > some Broadcom switches too.
> > > Using the rtnetlink user API, a user could be able to migrate all user
> > > ports between one CPU port and the other, and as long as the
> > > configuration is valid, the switch driver should accept this (we perform
> > > DSA master changing while all ports are down, and we could refuse going
> > > up if e.g. some user ports are assigned to CPU port A and some user
> > > ports to CPU port B). Nonetheless, the key point is that when a single
> > > CPU port is used, the other CPU port kinda sits there doing nothing. So
> > > I also have some patches that make the host port attached to this other
> > > CPU port be a normal interface (not a DSA master).
> > > The switch side of things is still a CPU port (not a user port, since
> > > there still isn't any net device registered for it), but nonetheless, it
> > > is a CPU port with no DSA tagging over it, hence the reason why the host
> > > port isn't a DSA master. The patch itself that changes this behavior
> > > sounds something like "only set up a host port as a DSA master if some
> > > user ports are assigned to it".
> > > As to why I'm doing it this way: the device tree should be fixed, and I
> > > do need to describe the connection between the switch CPU ports and the
> > > DSA masters via the 'ethernet = <&phandle>;' property. From a hardware
> > > perspective, both switch ports A and B are CPU ports, equally. But this
> > > means that DSA won't create a user port for the CPU port B, which would
> > > be the more natural way to use it.
> > > Now why this pertains to you: Vivien's initial stab at management over
> > > Ethernet wanted to decouple a bit the concept of a DSA master (used for
> > > the network stack) from the concept of a host port used for in-band
> > > management (used for register access). Whereas our approach here is to
> > > keep the two coupled, due to us saying "hey, if there's a direct
> > > connection to the switch, this is a DSA master anyway, is it not?".
> > > Well, here's one thing which you wouldn't be able to do if I pursue my
> > > idea with lazy DSA master setup: if you decide to move all your user
> > > ports using rtnetlink to CPU port 6, then the DSA master of CPU port 0
> > > will cease to be a DSA master. So that will also prevent the management
> > > protocol from working.
> > 
> > About the migration problem, wonder if we can just use a refcount that
> > would represent the user of the master port. The port won't be DSA
> > master anymore if no user are connected. A switch can increase this ref
> > if the port is mandatory for some operation. (qca8k on state change
> > operational would increase the ref and decrease and then the port can be
> > removed from a DSA master) That should handle all the other switch and
> > still permit a driver to ""bypass"" this behaviour.
> 
> Maybe. Although not quite like the way in which you propose. Remember
> that the idea is for a DSA master to be a regular interface until it
> gains a user. So there's the chicken and egg problem if you want to
> become a user on ->master_state_change()... because it's not a master.
> You'd have to specify upfront.
> 

I mean we can really think of adding an option or a flag for the port
that will be used to declare a cpu port as to be ignored by any disable
procedure. From what I can remember some broadcom switch have some
management port that can't be disabled for example so I can see an use
where a flag of this kind would be useful. Some thing like declaring a
port as a managament port and with this case any ""cleanup"" function
will be ignored? That would solve the chicken-egg problem and dts won't
have to be changed.

> > > I don't want to break your use case, but then again, I'm wondering what
> > > we could do to support the second CPU port working without DSA tagging,
> > > without changing the device trees to declare it as a user port (which in
> > > itself isn't bad, it's just that we need to support all use cases with a
> > > single, unified device tree).
> > 
> > Just some info about the secondary CPU port.
> > From Documentation the second cpu port in sgmii mode can be used also for
> > other task so yes we should understand how to handle this. (base-x, mac
> > and phy) This mode is set based on the phy mode and if the dsa port is a
> > cpu port. Device tree changes can be accepted as AFAIK due to DSA not
> > supporting multi cpu, CPU port 6 was never used/defined. (But I'm
> > not sure... that is the case for all the device we have on openwrt)
> 
> What do you mean exactly by "other tasks"?
> 

I never notice a device with this (actually we just find one that has 2
qca8k switch that seems to be connected with the port6 port) but other
task are for example interconnecting 2 switch or attach some external
port like sfp. (I assume?)

> > Considering that introducing multicpu port would require a
> > bit of rework, wonder if we should introduce some new bindings/node and
> > fallback to a legacy (aka force first cpu port as the unique cpu port
> > and ignore others) in the absence of this new implementation. (Hoping I
> > didn't get all wrong with the main problem here)
> 
> The defaults would stay the same. (I've no idea why we would introduce
> new device tree bindings? the only device tree change IMO would be to
> declare the link between the second CPU port and its DSA master, if you
> haven't done that already) But my key point was that, to some extent,
> some change to the current behavior will still be required. Like right
> now, a kernel 5.15 when it sees a device tree with 2 CPU ports will have
> 2 DSA masters. Maybe kernel 5.17 will only start off with the first port
> as a DSA master, and the other just a candidate. I'm hoping this won't

IMHO that would be the correct way. Just offer a secondary port and
leave the user decide to use it or not. (Single CPU port by default and
leave the user the choice to use the second with an init script)

> change observable behavior for the worse for anyone, because device
> trees are supposed to be there to stay, not change on a whim. My hope is
> based on the fact that as far as I can see, that second DSA master is
> effectively useless. Which in fact creates the second problem: exactly
> because the second host port is useless with the current code structure,
> I can see people describing it as a user rather than CPU port in current
> device trees, just to make some use out of it. But that restricts the
> potential user base (and therefore appeal) of my change of behavior or
> of multi-CPU port support in general, and this is a bit sad. I think we
> should be all spending a bit more time with current-generation kernels
> on "updated" device trees with multiple CPU ports defined, and see
> what's broken and what could be improved, because otherwise we could be
> leaving behind a huge mess when the device trees get updated, and we
> need to run the occasional old kernel on them.
> 

Anyway I think I didn't understand you from the start. Your problem was
with user declaring any additional cpu port as an user port (that is not
usable?) and not declaring the needed master.

Something like

port@6 {
				reg = <6>;
				label = "swp6";
				phy-mode = "sgmii"
			};

instead of (current way we use to declare secondary port on qca8k)

port@6 {
				reg = <6>;
				label = "cpu";
				ethernet = <&gmac2>;
				phy-mode = "sgmii";

				fixed-link {
					speed = <1000>;
					full-duplex;
				};
			};

> > But if I'm not wrong there was at the start some years ago an idea of a
> > node used to declare master port separate from the generic port node but
> > it was rejected?
> 
> I don't know anything about this.

One of the first RFC for multicpu port (somethin many years ago) from
some Marvell devs was to declare an additional node (separate from the
current one) that would declare cpu ports. But it was a bit useless and
was dropped after some version.

Aside from these concern how should we proceed with this series? Should
we first understand the multicpu problem?

Again sorry for the dealy.
-- 
	Ansuel

Re: [net-next PATCH RFC v6 00/16] Add support for qca8k mdio rw in Ethernet packet

[Vladimir Oltean]

On Thu, Jan 06, 2022 at 09:56:56PM +0100, Ansuel Smith wrote:
> On Fri, Dec 17, 2021 at 01:38:12AM +0200, Vladimir Oltean wrote:
> > On Wed, Dec 15, 2021 at 05:34:45PM +0100, Ansuel Smith wrote:
> > > > > I tested this with multicpu port and with port6 set as the unique port and
> > > > > it's sad.
> > > > > It seems they implemented this feature in a bad way and this is only
> > > > > supported with cpu port0. When cpu port6 is the unique port, the switch
> > > > > doesn't send ack packet. With multicpu port, packet ack are not duplicated
> > > > > and only cpu port0 sends them. This is the same for the MIB counter.
> > > > > For this reason this feature is enabled only when cpu port0 is enabled and
> > > > > operational.
> > > > 
> > > > Let's discuss this a bit (not the hardware limitation, that one is what
> > > > it is). When DSA has multiple CPU ports, right now both host-side
> > > > Ethernet ports are set up as DSA masters. By being a DSA master, I mean
> > > > that dev->dsa_ptr is a non-NULL pointer, so these interfaces expect to
> > > > receive packets that are trapped by the DSA packet_type handlers.
> > > > But due to the way in which dsa_tree_setup_default_cpu() is written,
> > > > by default only the first CPU port will be used. So the host port
> > > > attached to the second CPU port will be a DSA master technically, but it
> > > > will be an inactive one and won't be anyone's master (no dp->cpu_dp will
> > > > point to this master's dev->dsa_ptr). My idea of DSA support for
> > > > multiple CPU ports would be to be able to change the dp->cpu_dp mapping
> > > > through rtnetlink, on a per user port basis (yes, this implies we don't
> > > > have a solution for DSA ports).
> > > 
> > > I have a similar implementation that was proposed as RFC many times ago.
> > 
> > Yes, well, how to assign a user port to a CPU port seems not to be the
> > biggest problem that needs to be solved before support for multiple CPU
> > ports can fully go in.
> >
> 
> Hi,
> sorry for the delay.
> 
> I honestly think a start for multicpu that would improve the state would
> be start adding support to iproute for changing the master port.

Agree there, but since that would be new UAPI, we need to make sure it
works for all future work that we plan to do. And that future work also
includes LAG between CPU ports. In the current form that I have some
patches in, that would look like this:

ip link set eno2 down # old DSA master
ip link set eno2 master bond0
ip link set eno3 master bond0
# This is super annoying, because the bonding driver brings its ports up
# automatically, and on the other hand DSA wants its master to be down
# while changing the CPU port
ip link set eno2 down
ip link set eno3 down
ip link set bond0 down
# Changing the DSA master is done step by step. We pass through invalid
# intermediate steps, such as swp0 being under bond0, but swp1, swp2,
# swp3 still under eno2. We allow those invalid intermediate steps
# because the ports are down.
./ip link set swp0 type dsa master bond0
./ip link set swp1 type dsa master bond0
./ip link set swp2 type dsa master bond0
./ip link set swp3 type dsa master bond0
# Here is the point during which DSA should refuse to come up if it
# doesn't like something about the new configuration.
ip link set swp0 up # this also brings up the new DSA master, bond0

so yeah, it isn't great (not to mention it doesn't even work, yet).
The LAG FDB patch set I've posted today is part of the effort to see how
feasible it is to do LAG between CPU ports (FDB entries would be used
for CPU filtering).

> > > > My second observation is based on the fact that some switches support a
> > > > single CPU port, yet they are wired using two Ethernet ports towards the
> > > > host. The Felix and Seville switches are structured this way. I think
> > > > some Broadcom switches too.
> > > > Using the rtnetlink user API, a user could be able to migrate all user
> > > > ports between one CPU port and the other, and as long as the
> > > > configuration is valid, the switch driver should accept this (we perform
> > > > DSA master changing while all ports are down, and we could refuse going
> > > > up if e.g. some user ports are assigned to CPU port A and some user
> > > > ports to CPU port B). Nonetheless, the key point is that when a single
> > > > CPU port is used, the other CPU port kinda sits there doing nothing. So
> > > > I also have some patches that make the host port attached to this other
> > > > CPU port be a normal interface (not a DSA master).
> > > > The switch side of things is still a CPU port (not a user port, since
> > > > there still isn't any net device registered for it), but nonetheless, it
> > > > is a CPU port with no DSA tagging over it, hence the reason why the host
> > > > port isn't a DSA master. The patch itself that changes this behavior
> > > > sounds something like "only set up a host port as a DSA master if some
> > > > user ports are assigned to it".
> > > > As to why I'm doing it this way: the device tree should be fixed, and I
> > > > do need to describe the connection between the switch CPU ports and the
> > > > DSA masters via the 'ethernet = <&phandle>;' property. From a hardware
> > > > perspective, both switch ports A and B are CPU ports, equally. But this
> > > > means that DSA won't create a user port for the CPU port B, which would
> > > > be the more natural way to use it.
> > > > Now why this pertains to you: Vivien's initial stab at management over
> > > > Ethernet wanted to decouple a bit the concept of a DSA master (used for
> > > > the network stack) from the concept of a host port used for in-band
> > > > management (used for register access). Whereas our approach here is to
> > > > keep the two coupled, due to us saying "hey, if there's a direct
> > > > connection to the switch, this is a DSA master anyway, is it not?".
> > > > Well, here's one thing which you wouldn't be able to do if I pursue my
> > > > idea with lazy DSA master setup: if you decide to move all your user
> > > > ports using rtnetlink to CPU port 6, then the DSA master of CPU port 0
> > > > will cease to be a DSA master. So that will also prevent the management
> > > > protocol from working.
> > > 
> > > About the migration problem, wonder if we can just use a refcount that
> > > would represent the user of the master port. The port won't be DSA
> > > master anymore if no user are connected. A switch can increase this ref
> > > if the port is mandatory for some operation. (qca8k on state change
> > > operational would increase the ref and decrease and then the port can be
> > > removed from a DSA master) That should handle all the other switch and
> > > still permit a driver to ""bypass"" this behaviour.
> > 
> > Maybe. Although not quite like the way in which you propose. Remember
> > that the idea is for a DSA master to be a regular interface until it
> > gains a user. So there's the chicken and egg problem if you want to
> > become a user on ->master_state_change()... because it's not a master.
> > You'd have to specify upfront.
> > 
> 
> I mean we can really think of adding an option or a flag for the port
> that will be used to declare a cpu port as to be ignored by any disable
> procedure. From what I can remember some broadcom switch have some
> management port that can't be disabled for example so I can see an use
> where a flag of this kind would be useful. Some thing like declaring a
> port as a managament port and with this case any ""cleanup"" function
> will be ignored? That would solve the chicken-egg problem and dts won't
> have to be changed.

When you say "disable a CPU port", you mean "configure as not a CPU port"?
It might get tricky. DSA currently selects the lowest numbered port in
the device tree with 'ethernet = <&phandle>' as CPU port. If the second
possible CPU port, that we're adding, has a lower number than that, we'd
effectively change the default behavior with the new device tree.
We might need some hints from the driver even for that.

> > > > I don't want to break your use case, but then again, I'm wondering what
> > > > we could do to support the second CPU port working without DSA tagging,
> > > > without changing the device trees to declare it as a user port (which in
> > > > itself isn't bad, it's just that we need to support all use cases with a
> > > > single, unified device tree).
> > > 
> > > Just some info about the secondary CPU port.
> > > From Documentation the second cpu port in sgmii mode can be used also for
> > > other task so yes we should understand how to handle this. (base-x, mac
> > > and phy) This mode is set based on the phy mode and if the dsa port is a
> > > cpu port. Device tree changes can be accepted as AFAIK due to DSA not
> > > supporting multi cpu, CPU port 6 was never used/defined. (But I'm
> > > not sure... that is the case for all the device we have on openwrt)
> > 
> > What do you mean exactly by "other tasks"?
> > 
> 
> I never notice a device with this (actually we just find one that has 2
> qca8k switch that seems to be connected with the port6 port) but other
> task are for example interconnecting 2 switch or attach some external
> port like sfp. (I assume?)
> 
> > > Considering that introducing multicpu port would require a
> > > bit of rework, wonder if we should introduce some new bindings/node and
> > > fallback to a legacy (aka force first cpu port as the unique cpu port
> > > and ignore others) in the absence of this new implementation. (Hoping I
> > > didn't get all wrong with the main problem here)
> > 
> > The defaults would stay the same. (I've no idea why we would introduce
> > new device tree bindings? the only device tree change IMO would be to
> > declare the link between the second CPU port and its DSA master, if you
> > haven't done that already) But my key point was that, to some extent,
> > some change to the current behavior will still be required. Like right
> > now, a kernel 5.15 when it sees a device tree with 2 CPU ports will have
> > 2 DSA masters. Maybe kernel 5.17 will only start off with the first port
> > as a DSA master, and the other just a candidate. I'm hoping this won't
> 
> IMHO that would be the correct way. Just offer a secondary port and
> leave the user decide to use it or not. (Single CPU port by default and
> leave the user the choice to use the second with an init script)
> 
> > change observable behavior for the worse for anyone, because device
> > trees are supposed to be there to stay, not change on a whim. My hope is
> > based on the fact that as far as I can see, that second DSA master is
> > effectively useless. Which in fact creates the second problem: exactly
> > because the second host port is useless with the current code structure,
> > I can see people describing it as a user rather than CPU port in current
> > device trees, just to make some use out of it. But that restricts the
> > potential user base (and therefore appeal) of my change of behavior or
> > of multi-CPU port support in general, and this is a bit sad. I think we
> > should be all spending a bit more time with current-generation kernels
> > on "updated" device trees with multiple CPU ports defined, and see
> > what's broken and what could be improved, because otherwise we could be
> > leaving behind a huge mess when the device trees get updated, and we
> > need to run the occasional old kernel on them.
> > 
> 
> Anyway I think I didn't understand you from the start. Your problem was
> with user declaring any additional cpu port as an user port (that is not
> usable?) and not declaring the needed master.
> 
> Something like
> 
> port@6 {
> 				reg = <6>;
> 				label = "swp6";
> 				phy-mode = "sgmii"
> 			};
> 
> instead of (current way we use to declare secondary port on qca8k)
> 
> port@6 {
> 				reg = <6>;
> 				label = "cpu";
> 				ethernet = <&gmac2>;
> 				phy-mode = "sgmii";
> 
> 				fixed-link {
> 					speed = <1000>;
> 					full-duplex;
> 				};
> 			};

Yes, you basically have two options for your second CPU port, and you've
pointed out both options above.

If you describe it as you did in the second case, you'd have a 'correct'
device tree, but this makes that port unusable with all DSA code bases
that exist currently.

If you describe it as you did in the first case (CPU port as user port),
you'd have a functional port, but you wouldn't be able to make use of
multiple CPU ports in DSA without updating your device tree.

The idea is to be able to have the functionality gained by the first
device tree description (CPU port as user port), while having a device
tree that looks like the second description (both ports are proper CPU
ports). For the Felix driver, I've introduced the concept of a "forced
forwarding mask", and the second CPU port (port@6 in your example) is
part of that forced forwarding mask unconditionally, which emulates what
would happen if you'd put swp6 in a bridge with every other user port.

> > > But if I'm not wrong there was at the start some years ago an idea of a
> > > node used to declare master port separate from the generic port node but
> > > it was rejected?
> > 
> > I don't know anything about this.
> 
> One of the first RFC for multicpu port (somethin many years ago) from
> some Marvell devs was to declare an additional node (separate from the
> current one) that would declare cpu ports. But it was a bit useless and
> was dropped after some version.
> 
> Aside from these concern how should we proceed with this series? Should
> we first understand the multicpu problem?
> 
> Again sorry for the dealy.

I don't think this series has any dependency on the multiple CPU port
support. I just opened the discussion because I knew you were also
working on it, and I wanted to collect some information about the way in
which you need it to work for your systems.