[PATCH net-next v2 0/5] net: dsa: Multi-CPU ground work (v2)

[PATCH net-next v2 0/5] net: dsa: Multi-CPU ground work (v2)

[Florian Fainelli]

Hi all,

This patch series prepares the ground for adding mutliple CPU port support to
DSA, and starts by removing redundant pieces of information such as
master_netdev which is cpu_dp->ethernet. Finally drivers are moved away from
directly accessing ds->dst->cpu_dp and use appropriate helper functions.

Note that if you have Device Tree blobs/platform configurations that are
currently listing multiple CPU ports, the proposed behavior in
dsa_ds_get_cpu_dp() will be to return the last bit set in ds->cpu_port_mask.

Future plans include:
- making dst->cpu_dp a flexible data structure (array, list, you name it)
- having the ability for drivers to return a default/preferred CPU port (if
  necessary)

Changes in v2:

- added Reviewed-by tags
- assign port->cpu_dp earlier before ops->setup() has run

Florian Fainelli (5):
  net: dsa: Remove master_netdev and use dst->cpu_dp->netdev
  net: dsa: Relocate master ethtool operations
  net: dsa: Associate slave network device with CPU port
  net: dsa: Introduce dsa_dst_get_cpu_dp()
  net: dsa: Stop accessing ds->dst->cpu_dp in drivers

 drivers/net/dsa/b53/b53_common.c |  4 +--
 drivers/net/dsa/bcm_sf2.c        | 10 +++++---
 drivers/net/dsa/mt7530.c         |  6 +++--
 drivers/net/dsa/mv88e6060.c      |  2 +-
 drivers/net/dsa/qca8k.c          |  2 +-
 include/net/dsa.h                | 29 +++++++++-------------
 net/dsa/dsa.c                    | 19 ++++----------
 net/dsa/dsa2.c                   | 27 ++++++++++++--------
 net/dsa/dsa_priv.h               | 10 ++++++++
 net/dsa/legacy.c                 | 23 ++++++++++-------
 net/dsa/slave.c                  | 53 ++++++++++++++++++++--------------------
 net/dsa/tag_brcm.c               |  5 ++--
 net/dsa/tag_ksz.c                |  5 ++--
 net/dsa/tag_qca.c                |  3 ++-
 net/dsa/tag_trailer.c            |  5 ++--
 15 files changed, 107 insertions(+), 96 deletions(-)

-- 
2.9.3

[PATCH net-next v2 1/5] net: dsa: Remove master_netdev and use dst->cpu_dp->netdev

[Florian Fainelli]

In preparation for supporting multiple CPU ports, remove
dst->master_netdev and ds->master_netdev and replace them with only one
instance of the common object we have for a port: struct
dsa_port::netdev. ds->master_netdev is currently write only and would be
helpful in the case where we have two switches, both with CPU ports, and
also connected within each other, which the multi-CPU port patch series
would address.

While at it, introduce a helper function used in net/dsa/slave.c to
immediately get a reference on the master network device called
dsa_master_netdev().

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2.c |  4 ++--
 drivers/net/dsa/mt7530.c  |  4 ++--
 include/net/dsa.h         |  5 -----
 net/dsa/dsa.c             |  9 ++-------
 net/dsa/dsa2.c            | 18 +++++++-----------
 net/dsa/dsa_priv.h        |  5 +++++
 net/dsa/legacy.c          | 22 +++++++++++++---------
 net/dsa/slave.c           | 20 +++++++++-----------
 8 files changed, 40 insertions(+), 47 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 687a8bae5d73..76e98e8ed315 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -806,7 +806,7 @@ static int bcm_sf2_sw_resume(struct dsa_switch *ds)
 static void bcm_sf2_sw_get_wol(struct dsa_switch *ds, int port,
 			       struct ethtool_wolinfo *wol)
 {
-	struct net_device *p = ds->dst[ds->index].master_netdev;
+	struct net_device *p = ds->dst[ds->index].cpu_dp->netdev;
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
 	struct ethtool_wolinfo pwol;
 
@@ -829,7 +829,7 @@ static void bcm_sf2_sw_get_wol(struct dsa_switch *ds, int port,
 static int bcm_sf2_sw_set_wol(struct dsa_switch *ds, int port,
 			      struct ethtool_wolinfo *wol)
 {
-	struct net_device *p = ds->dst[ds->index].master_netdev;
+	struct net_device *p = ds->dst[ds->index].cpu_dp->netdev;
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
 	s8 cpu_port = ds->dst->cpu_dp->index;
 	struct ethtool_wolinfo pwol;
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 25e00d5e0eec..1e46418a3b74 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -912,11 +912,11 @@ mt7530_setup(struct dsa_switch *ds)
 	struct device_node *dn;
 	struct mt7530_dummy_poll p;
 
-	/* The parent node of master_netdev which holds the common system
+	/* The parent node of cpu_dp->netdev which holds the common system
 	 * controller also is the container for two GMACs nodes representing
 	 * as two netdev instances.
 	 */
-	dn = ds->master_netdev->dev.of_node->parent;
+	dn = ds->dst->cpu_dp->netdev->dev.of_node->parent;
 	priv->ethernet = syscon_node_to_regmap(dn);
 	if (IS_ERR(priv->ethernet))
 		return PTR_ERR(priv->ethernet);
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 2effb0af9d7c..b2fb53f5e28e 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -227,11 +227,6 @@ struct dsa_switch {
 	s8		rtable[DSA_MAX_SWITCHES];
 
 	/*
-	 * The lower device this switch uses to talk to the host
-	 */
-	struct net_device *master_netdev;
-
-	/*
 	 * Slave mii_bus and devices for the individual ports.
 	 */
 	u32			dsa_port_mask;
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index fdc448b30e56..eaab1affeeeb 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -118,10 +118,7 @@ int dsa_cpu_port_ethtool_setup(struct dsa_port *cpu_dp)
 	struct net_device *master;
 	struct ethtool_ops *cpu_ops;
 
-	master = ds->dst->master_netdev;
-	if (ds->master_netdev)
-		master = ds->master_netdev;
-
+	master = ds->dst->cpu_dp->netdev;
 	cpu_ops = devm_kzalloc(ds->dev, sizeof(*cpu_ops), GFP_KERNEL);
 	if (!cpu_ops)
 		return -ENOMEM;
@@ -142,9 +139,7 @@ void dsa_cpu_port_ethtool_restore(struct dsa_port *cpu_dp)
 	struct dsa_switch *ds = cpu_dp->ds;
 	struct net_device *master;
 
-	master = ds->dst->master_netdev;
-	if (ds->master_netdev)
-		master = ds->master_netdev;
+	master = ds->dst->cpu_dp->netdev;
 
 	master->ethtool_ops = ds->dst->master_orig_ethtool_ops;
 }
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index cd13bb54a30c..2674bdf03fef 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -337,7 +337,7 @@ static int dsa_ds_apply(struct dsa_switch_tree *dst, struct dsa_switch *ds)
 		return err;
 
 	if (ds->ops->set_addr) {
-		err = ds->ops->set_addr(ds, dst->master_netdev->dev_addr);
+		err = ds->ops->set_addr(ds, dst->cpu_dp->netdev->dev_addr);
 		if (err < 0)
 			return err;
 	}
@@ -444,7 +444,7 @@ static int dsa_dst_apply(struct dsa_switch_tree *dst)
 	 * sent to the tag format's receive function.
 	 */
 	wmb();
-	dst->master_netdev->dsa_ptr = dst;
+	dst->cpu_dp->netdev->dsa_ptr = dst;
 	dst->applied = true;
 
 	return 0;
@@ -458,7 +458,7 @@ static void dsa_dst_unapply(struct dsa_switch_tree *dst)
 	if (!dst->applied)
 		return;
 
-	dst->master_netdev->dsa_ptr = NULL;
+	dst->cpu_dp->netdev->dsa_ptr = NULL;
 
 	/* If we used a tagging format that doesn't have an ethertype
 	 * field, make sure that all packets from this point get sent
@@ -502,14 +502,10 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index,
 	if (!ethernet_dev)
 		return -EPROBE_DEFER;
 
-	if (!ds->master_netdev)
-		ds->master_netdev = ethernet_dev;
-
-	if (!dst->master_netdev)
-		dst->master_netdev = ethernet_dev;
-
-	if (!dst->cpu_dp)
+	if (!dst->cpu_dp) {
 		dst->cpu_dp = port;
+		dst->cpu_dp->netdev = ethernet_dev;
+	}
 
 	tag_protocol = ds->ops->get_tag_protocol(ds);
 	dst->tag_ops = dsa_resolve_tag_protocol(tag_protocol);
@@ -576,7 +572,7 @@ static int dsa_dst_parse(struct dsa_switch_tree *dst)
 			return err;
 	}
 
-	if (!dst->master_netdev) {
+	if (!dst->cpu_dp->netdev) {
 		pr_warn("Tree has no master device\n");
 		return -EINVAL;
 	}
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 66ee248796c8..5c510f4ba0ce 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -183,4 +183,9 @@ extern const struct dsa_device_ops qca_netdev_ops;
 /* tag_trailer.c */
 extern const struct dsa_device_ops trailer_netdev_ops;
 
+static inline struct net_device *dsa_master_netdev(struct dsa_slave_priv *p)
+{
+	return p->dp->ds->dst->cpu_dp->netdev;
+}
+
 #endif
diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c
index d534d8f4b9cf..443ebf3b5e5b 100644
--- a/net/dsa/legacy.c
+++ b/net/dsa/legacy.c
@@ -101,9 +101,12 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 	struct dsa_switch_tree *dst = ds->dst;
 	struct dsa_chip_data *cd = ds->cd;
 	bool valid_name_found = false;
+	struct net_device *master;
 	int index = ds->index;
 	int i, ret;
 
+	master = dst->cpu_dp->netdev;
+
 	/*
 	 * Validate supplied switch configuration.
 	 */
@@ -116,7 +119,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 
 		if (!strcmp(name, "cpu")) {
 			if (dst->cpu_dp) {
-				netdev_err(dst->master_netdev,
+				netdev_err(master,
 					   "multiple cpu ports?!\n");
 				return -EINVAL;
 			}
@@ -168,7 +171,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 		return ret;
 
 	if (ops->set_addr) {
-		ret = ops->set_addr(ds, dst->master_netdev->dev_addr);
+		ret = ops->set_addr(ds, master->dev_addr);
 		if (ret < 0)
 			return ret;
 	}
@@ -195,14 +198,14 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 
 		ret = dsa_slave_create(ds, parent, i, cd->port_names[i]);
 		if (ret < 0)
-			netdev_err(dst->master_netdev, "[%d]: can't create dsa slave device for port %d(%s): %d\n",
+			netdev_err(master, "[%d]: can't create dsa slave device for port %d(%s): %d\n",
 				   index, i, cd->port_names[i], ret);
 	}
 
 	/* Perform configuration of the CPU and DSA ports */
 	ret = dsa_cpu_dsa_setups(ds, parent);
 	if (ret < 0)
-		netdev_err(dst->master_netdev, "[%d] : can't configure CPU and DSA ports\n",
+		netdev_err(master, "[%d] : can't configure CPU and DSA ports\n",
 			   index);
 
 	ret = dsa_cpu_port_ethtool_setup(ds->dst->cpu_dp);
@@ -217,6 +220,7 @@ dsa_switch_setup(struct dsa_switch_tree *dst, int index,
 		 struct device *parent, struct device *host_dev)
 {
 	struct dsa_chip_data *cd = dst->pd->chip + index;
+	struct net_device *master = dst->cpu_dp->netdev;
 	const struct dsa_switch_ops *ops;
 	struct dsa_switch *ds;
 	int ret;
@@ -228,11 +232,11 @@ dsa_switch_setup(struct dsa_switch_tree *dst, int index,
 	 */
 	ops = dsa_switch_probe(parent, host_dev, cd->sw_addr, &name, &priv);
 	if (!ops) {
-		netdev_err(dst->master_netdev, "[%d]: could not detect attached switch\n",
+		netdev_err(master, "[%d]: could not detect attached switch\n",
 			   index);
 		return ERR_PTR(-EINVAL);
 	}
-	netdev_info(dst->master_netdev, "[%d]: detected a %s switch\n",
+	netdev_info(master, "[%d]: detected a %s switch\n",
 		    index, name);
 
 
@@ -622,7 +626,7 @@ static int dsa_setup_dst(struct dsa_switch_tree *dst, struct net_device *dev,
 	unsigned configured = 0;
 
 	dst->pd = pd;
-	dst->master_netdev = dev;
+	dst->cpu_dp->netdev = dev;
 
 	for (i = 0; i < pd->nr_chips; i++) {
 		struct dsa_switch *ds;
@@ -718,7 +722,7 @@ static void dsa_remove_dst(struct dsa_switch_tree *dst)
 {
 	int i;
 
-	dst->master_netdev->dsa_ptr = NULL;
+	dst->cpu_dp->netdev->dsa_ptr = NULL;
 
 	/* If we used a tagging format that doesn't have an ethertype
 	 * field, make sure that all packets from this point get sent
@@ -735,7 +739,7 @@ static void dsa_remove_dst(struct dsa_switch_tree *dst)
 
 	dsa_cpu_port_ethtool_restore(dst->cpu_dp);
 
-	dev_put(dst->master_netdev);
+	dev_put(dst->cpu_dp->netdev);
 }
 
 static int dsa_remove(struct platform_device *pdev)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 1cfdb31a2f44..d52c9ceb0566 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -66,7 +66,7 @@ static int dsa_slave_get_iflink(const struct net_device *dev)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
 
-	return p->dp->ds->dst->master_netdev->ifindex;
+	return dsa_master_netdev(p)->ifindex;
 }
 
 static int dsa_slave_open(struct net_device *dev)
@@ -74,7 +74,7 @@ static int dsa_slave_open(struct net_device *dev)
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_port *dp = p->dp;
 	struct dsa_switch *ds = dp->ds;
-	struct net_device *master = ds->dst->master_netdev;
+	struct net_device *master = dsa_master_netdev(p);
 	u8 stp_state = dp->bridge_dev ? BR_STATE_BLOCKING : BR_STATE_FORWARDING;
 	int err;
 
@@ -127,7 +127,7 @@ static int dsa_slave_open(struct net_device *dev)
 static int dsa_slave_close(struct net_device *dev)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct net_device *master = p->dp->ds->dst->master_netdev;
+	struct net_device *master = dsa_master_netdev(p);
 	struct dsa_switch *ds = p->dp->ds;
 
 	if (p->phy)
@@ -154,7 +154,7 @@ static int dsa_slave_close(struct net_device *dev)
 static void dsa_slave_change_rx_flags(struct net_device *dev, int change)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct net_device *master = p->dp->ds->dst->master_netdev;
+	struct net_device *master = dsa_master_netdev(p);
 
 	if (change & IFF_ALLMULTI)
 		dev_set_allmulti(master, dev->flags & IFF_ALLMULTI ? 1 : -1);
@@ -165,7 +165,7 @@ static void dsa_slave_change_rx_flags(struct net_device *dev, int change)
 static void dsa_slave_set_rx_mode(struct net_device *dev)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct net_device *master = p->dp->ds->dst->master_netdev;
+	struct net_device *master = dsa_master_netdev(p);
 
 	dev_mc_sync(master, dev);
 	dev_uc_sync(master, dev);
@@ -174,7 +174,7 @@ static void dsa_slave_set_rx_mode(struct net_device *dev)
 static int dsa_slave_set_mac_address(struct net_device *dev, void *a)
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
-	struct net_device *master = p->dp->ds->dst->master_netdev;
+	struct net_device *master = dsa_master_netdev(p);
 	struct sockaddr *addr = a;
 	int err;
 
@@ -375,7 +375,7 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
 	/* Queue the SKB for transmission on the parent interface, but
 	 * do not modify its EtherType
 	 */
-	nskb->dev = p->dp->ds->dst->master_netdev;
+	nskb->dev = dsa_master_netdev(p);
 	dev_queue_xmit(nskb);
 
 	return NETDEV_TX_OK;
@@ -684,7 +684,7 @@ static int dsa_slave_netpoll_setup(struct net_device *dev,
 {
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_switch *ds = p->dp->ds;
-	struct net_device *master = ds->dst->master_netdev;
+	struct net_device *master = dsa_master_netdev(p);
 	struct netpoll *netpoll;
 	int err = 0;
 
@@ -1141,9 +1141,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 	struct dsa_slave_priv *p;
 	int ret;
 
-	master = ds->dst->master_netdev;
-	if (ds->master_netdev)
-		master = ds->master_netdev;
+	master = ds->dst->cpu_dp->netdev;
 
 	slave_dev = alloc_netdev(sizeof(struct dsa_slave_priv), name,
 				 NET_NAME_UNKNOWN, ether_setup);
-- 
2.9.3

[PATCH net-next v2 2/5] net: dsa: Relocate master ethtool operations

[Florian Fainelli]

Relocate master_ethtool_ops and master_orig_ethtool_ops into struct
dsa_port in order to be both consistent, and make things self contained
within the dsa_port structure.

This is a preliminary change to supporting multiple CPU port interfaces.

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/net/dsa.h | 17 +++++------------
 net/dsa/dsa.c     | 16 ++++++----------
 net/dsa/slave.c   | 16 ++++++++--------
 3 files changed, 19 insertions(+), 30 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index b2fb53f5e28e..7e93869819f9 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -122,12 +122,6 @@ struct dsa_switch_tree {
 	 */
 	struct dsa_platform_data	*pd;
 
-	/*
-	 * Reference to network device to use, and which tagging
-	 * protocol to use.
-	 */
-	struct net_device	*master_netdev;
-
 	/* Copy of tag_ops->rcv for faster access in hot path */
 	struct sk_buff *	(*rcv)(struct sk_buff *skb,
 				       struct net_device *dev,
@@ -135,12 +129,6 @@ struct dsa_switch_tree {
 				       struct net_device *orig_dev);
 
 	/*
-	 * Original copy of the master netdev ethtool_ops
-	 */
-	struct ethtool_ops	master_ethtool_ops;
-	const struct ethtool_ops *master_orig_ethtool_ops;
-
-	/*
 	 * The switch port to which the CPU is attached.
 	 */
 	struct dsa_port		*cpu_dp;
@@ -189,6 +177,11 @@ struct dsa_port {
 	u8			stp_state;
 	struct net_device	*bridge_dev;
 	struct devlink_port	devlink_port;
+	/*
+	 * Original copy of the master netdev ethtool_ops
+	 */
+	struct ethtool_ops	ethtool_ops;
+	const struct ethtool_ops *orig_ethtool_ops;
 };
 
 struct dsa_switch {
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index eaab1affeeeb..2665a66e833d 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -118,15 +118,16 @@ int dsa_cpu_port_ethtool_setup(struct dsa_port *cpu_dp)
 	struct net_device *master;
 	struct ethtool_ops *cpu_ops;
 
-	master = ds->dst->cpu_dp->netdev;
+	master = cpu_dp->netdev;
+
 	cpu_ops = devm_kzalloc(ds->dev, sizeof(*cpu_ops), GFP_KERNEL);
 	if (!cpu_ops)
 		return -ENOMEM;
 
-	memcpy(&ds->dst->master_ethtool_ops, master->ethtool_ops,
+	memcpy(&cpu_dp->ethtool_ops, master->ethtool_ops,
 	       sizeof(struct ethtool_ops));
-	ds->dst->master_orig_ethtool_ops = master->ethtool_ops;
-	memcpy(cpu_ops, &ds->dst->master_ethtool_ops,
+	cpu_dp->orig_ethtool_ops = master->ethtool_ops;
+	memcpy(cpu_ops, &cpu_dp->ethtool_ops,
 	       sizeof(struct ethtool_ops));
 	dsa_cpu_port_ethtool_init(cpu_ops);
 	master->ethtool_ops = cpu_ops;
@@ -136,12 +137,7 @@ int dsa_cpu_port_ethtool_setup(struct dsa_port *cpu_dp)
 
 void dsa_cpu_port_ethtool_restore(struct dsa_port *cpu_dp)
 {
-	struct dsa_switch *ds = cpu_dp->ds;
-	struct net_device *master;
-
-	master = ds->dst->cpu_dp->netdev;
-
-	master->ethtool_ops = ds->dst->master_orig_ethtool_ops;
+	cpu_dp->netdev->ethtool_ops = cpu_dp->orig_ethtool_ops;
 }
 
 void dsa_cpu_dsa_destroy(struct dsa_port *port)
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index d52c9ceb0566..ea4ed0285922 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -523,10 +523,10 @@ static void dsa_cpu_port_get_ethtool_stats(struct net_device *dev,
 	s8 cpu_port = dst->cpu_dp->index;
 	int count = 0;
 
-	if (dst->master_ethtool_ops.get_sset_count) {
-		count = dst->master_ethtool_ops.get_sset_count(dev,
+	if (dst->cpu_dp->ethtool_ops.get_sset_count) {
+		count = dst->cpu_dp->ethtool_ops.get_sset_count(dev,
 							       ETH_SS_STATS);
-		dst->master_ethtool_ops.get_ethtool_stats(dev, stats, data);
+		dst->cpu_dp->ethtool_ops.get_ethtool_stats(dev, stats, data);
 	}
 
 	if (ds->ops->get_ethtool_stats)
@@ -539,8 +539,8 @@ static int dsa_cpu_port_get_sset_count(struct net_device *dev, int sset)
 	struct dsa_switch *ds = dst->cpu_dp->ds;
 	int count = 0;
 
-	if (dst->master_ethtool_ops.get_sset_count)
-		count += dst->master_ethtool_ops.get_sset_count(dev, sset);
+	if (dst->cpu_dp->ethtool_ops.get_sset_count)
+		count += dst->cpu_dp->ethtool_ops.get_sset_count(dev, sset);
 
 	if (sset == ETH_SS_STATS && ds->ops->get_sset_count)
 		count += ds->ops->get_sset_count(ds);
@@ -564,10 +564,10 @@ static void dsa_cpu_port_get_strings(struct net_device *dev,
 	/* We do not want to be NULL-terminated, since this is a prefix */
 	pfx[sizeof(pfx) - 1] = '_';
 
-	if (dst->master_ethtool_ops.get_sset_count) {
-		mcount = dst->master_ethtool_ops.get_sset_count(dev,
+	if (dst->cpu_dp->ethtool_ops.get_sset_count) {
+		mcount = dst->cpu_dp->ethtool_ops.get_sset_count(dev,
 								ETH_SS_STATS);
-		dst->master_ethtool_ops.get_strings(dev, stringset, data);
+		dst->cpu_dp->ethtool_ops.get_strings(dev, stringset, data);
 	}
 
 	if (stringset == ETH_SS_STATS && ds->ops->get_strings) {
-- 
2.9.3

[PATCH net-next v2 3/5] net: dsa: Associate slave network device with CPU port

[Florian Fainelli]

In preparation for supporting multiple CPU ports with DSA, have the
dsa_port structure know which CPU it is associated with. This will be
important in order to make sure the correct CPU is used for transmission
of the frames. If not for functional reasons, for performance (e.g: load
balancing) and forwarding decisions.

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/net/dsa.h  | 1 +
 net/dsa/dsa_priv.h | 2 +-
 net/dsa/slave.c    | 5 ++++-
 3 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 7e93869819f9..58969b9a090c 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -171,6 +171,7 @@ struct dsa_port {
 	struct dsa_switch	*ds;
 	unsigned int		index;
 	const char		*name;
+	struct dsa_port		*cpu_dp;
 	struct net_device	*netdev;
 	struct device_node	*dn;
 	unsigned int		ageing_time;
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 5c510f4ba0ce..7c2326f3b538 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -185,7 +185,7 @@ extern const struct dsa_device_ops trailer_netdev_ops;
 
 static inline struct net_device *dsa_master_netdev(struct dsa_slave_priv *p)
 {
-	return p->dp->ds->dst->cpu_dp->netdev;
+	return p->dp->cpu_dp->netdev;
 }
 
 #endif
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index ea4ed0285922..de1ab41cfd38 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1139,9 +1139,11 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 	struct net_device *master;
 	struct net_device *slave_dev;
 	struct dsa_slave_priv *p;
+	struct dsa_port *cpu_dp;
 	int ret;
 
-	master = ds->dst->cpu_dp->netdev;
+	cpu_dp = ds->dst->cpu_dp;
+	master = cpu_dp->netdev;
 
 	slave_dev = alloc_netdev(sizeof(struct dsa_slave_priv), name,
 				 NET_NAME_UNKNOWN, ether_setup);
@@ -1176,6 +1178,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 	p->old_duplex = -1;
 
 	ds->ports[port].netdev = slave_dev;
+	p->dp->cpu_dp = cpu_dp;
 	ret = register_netdev(slave_dev);
 	if (ret) {
 		netdev_err(master, "error %d registering interface %s\n",
-- 
2.9.3

[PATCH net-next v2 4/5] net: dsa: Introduce dsa_dst_get_cpu_dp()

[Florian Fainelli]

Introduce a helper function which will return a reference to the CPU
port used in a dsa_switch_tree. Right now this is a singleton, but this
will change once we introduce multi-CPU port support, so ease the
transition by converting the affected code paths.

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/dsa_priv.h    |  5 +++++
 net/dsa/slave.c       | 31 ++++++++++++++++---------------
 net/dsa/tag_brcm.c    |  5 ++---
 net/dsa/tag_ksz.c     |  5 ++---
 net/dsa/tag_qca.c     |  3 ++-
 net/dsa/tag_trailer.c |  5 ++---
 6 files changed, 29 insertions(+), 25 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 7c2326f3b538..49b4b047aed0 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -188,4 +188,9 @@ static inline struct net_device *dsa_master_netdev(struct dsa_slave_priv *p)
 	return p->dp->cpu_dp->netdev;
 }
 
+static inline struct dsa_port *dsa_dst_get_cpu_dp(struct dsa_switch_tree *dst)
+{
+	return dst->cpu_dp;
+}
+
 #endif
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index de1ab41cfd38..a73c1de398b5 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -519,14 +519,14 @@ static void dsa_cpu_port_get_ethtool_stats(struct net_device *dev,
 					   uint64_t *data)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_switch *ds = dst->cpu_dp->ds;
-	s8 cpu_port = dst->cpu_dp->index;
+	struct dsa_port *cpu_dp = dsa_dst_get_cpu_dp(dst);
+	struct dsa_switch *ds = cpu_dp->ds;
+	s8 cpu_port = cpu_dp->index;
 	int count = 0;
 
-	if (dst->cpu_dp->ethtool_ops.get_sset_count) {
-		count = dst->cpu_dp->ethtool_ops.get_sset_count(dev,
-							       ETH_SS_STATS);
-		dst->cpu_dp->ethtool_ops.get_ethtool_stats(dev, stats, data);
+	if (cpu_dp->ethtool_ops.get_sset_count) {
+		count = cpu_dp->ethtool_ops.get_sset_count(dev, ETH_SS_STATS);
+		cpu_dp->ethtool_ops.get_ethtool_stats(dev, stats, data);
 	}
 
 	if (ds->ops->get_ethtool_stats)
@@ -536,11 +536,12 @@ static void dsa_cpu_port_get_ethtool_stats(struct net_device *dev,
 static int dsa_cpu_port_get_sset_count(struct net_device *dev, int sset)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_switch *ds = dst->cpu_dp->ds;
+	struct dsa_port *cpu_dp = dsa_dst_get_cpu_dp(dst);
+	struct dsa_switch *ds = cpu_dp->ds;
 	int count = 0;
 
-	if (dst->cpu_dp->ethtool_ops.get_sset_count)
-		count += dst->cpu_dp->ethtool_ops.get_sset_count(dev, sset);
+	if (cpu_dp->ethtool_ops.get_sset_count)
+		count += cpu_dp->ethtool_ops.get_sset_count(dev, sset);
 
 	if (sset == ETH_SS_STATS && ds->ops->get_sset_count)
 		count += ds->ops->get_sset_count(ds);
@@ -552,8 +553,9 @@ static void dsa_cpu_port_get_strings(struct net_device *dev,
 				     uint32_t stringset, uint8_t *data)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_switch *ds = dst->cpu_dp->ds;
-	s8 cpu_port = dst->cpu_dp->index;
+	struct dsa_port *cpu_dp = dsa_dst_get_cpu_dp(dst);
+	struct dsa_switch *ds = cpu_dp->ds;
+	s8 cpu_port = cpu_dp->index;
 	int len = ETH_GSTRING_LEN;
 	int mcount = 0, count;
 	unsigned int i;
@@ -564,10 +566,9 @@ static void dsa_cpu_port_get_strings(struct net_device *dev,
 	/* We do not want to be NULL-terminated, since this is a prefix */
 	pfx[sizeof(pfx) - 1] = '_';
 
-	if (dst->cpu_dp->ethtool_ops.get_sset_count) {
-		mcount = dst->cpu_dp->ethtool_ops.get_sset_count(dev,
-								ETH_SS_STATS);
-		dst->cpu_dp->ethtool_ops.get_strings(dev, stringset, data);
+	if (cpu_dp->ethtool_ops.get_sset_count) {
+		mcount = cpu_dp->ethtool_ops.get_sset_count(dev, ETH_SS_STATS);
+		cpu_dp->ethtool_ops.get_strings(dev, stringset, data);
 	}
 
 	if (stringset == ETH_SS_STATS && ds->ops->get_strings) {
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index c03860907f28..d7ef2b35e61e 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -93,12 +93,11 @@ static struct sk_buff *brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 				    struct net_device *orig_dev)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_switch *ds;
+	struct dsa_port *cpu_dp = dsa_dst_get_cpu_dp(dst);
+	struct dsa_switch *ds = cpu_dp->ds;
 	int source_port;
 	u8 *brcm_tag;
 
-	ds = dst->cpu_dp->ds;
-
 	if (unlikely(!pskb_may_pull(skb, BRCM_TAG_LEN)))
 		return NULL;
 
diff --git a/net/dsa/tag_ksz.c b/net/dsa/tag_ksz.c
index b94a334a1d02..c41a24e83e83 100644
--- a/net/dsa/tag_ksz.c
+++ b/net/dsa/tag_ksz.c
@@ -75,12 +75,11 @@ static struct sk_buff *ksz_rcv(struct sk_buff *skb, struct net_device *dev,
 			       struct net_device *orig_dev)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_switch *ds;
+	struct dsa_port *cpu_dp = dsa_dst_get_cpu_dp(dst);
+	struct dsa_switch *ds = cpu_dp->ds;
 	u8 *tag;
 	int source_port;
 
-	ds = dst->cpu_dp->ds;
-
 	tag = skb_tail_pointer(skb) - KSZ_EGRESS_TAG_LEN;
 
 	source_port = tag[0] & 7;
diff --git a/net/dsa/tag_qca.c b/net/dsa/tag_qca.c
index 4f43cf0b4eff..73244ae388df 100644
--- a/net/dsa/tag_qca.c
+++ b/net/dsa/tag_qca.c
@@ -67,6 +67,7 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 				   struct net_device *orig_dev)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
+	struct dsa_port *cpu_dp = dsa_dst_get_cpu_dp(dst);
 	struct dsa_switch *ds;
 	u8 ver;
 	int port;
@@ -95,7 +96,7 @@ static struct sk_buff *qca_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 	/* This protocol doesn't support cascading multiple switches so it's
 	 * safe to assume the switch is first in the tree
 	 */
-	ds = dst->cpu_dp->ds;
+	ds = cpu_dp->ds;
 	if (!ds)
 		return NULL;
 
diff --git a/net/dsa/tag_trailer.c b/net/dsa/tag_trailer.c
index b4f6db094409..68f664cb7517 100644
--- a/net/dsa/tag_trailer.c
+++ b/net/dsa/tag_trailer.c
@@ -61,12 +61,11 @@ static struct sk_buff *trailer_rcv(struct sk_buff *skb, struct net_device *dev,
 				   struct net_device *orig_dev)
 {
 	struct dsa_switch_tree *dst = dev->dsa_ptr;
-	struct dsa_switch *ds;
+	struct dsa_port *cpu_dp = dsa_dst_get_cpu_dp(dst);
+	struct dsa_switch *ds = cpu_dp->ds;
 	u8 *trailer;
 	int source_port;
 
-	ds = dst->cpu_dp->ds;
-
 	if (skb_linearize(skb))
 		return NULL;
 
-- 
2.9.3

[PATCH net-next v2 5/5] net: dsa: Stop accessing ds->dst->cpu_dp in drivers

[Florian Fainelli]

Out of the few drivers that do access ds->dst->cpu_dp, there is only a
handful for which we cannot substitute that for either an existing and
equivalent piece of information (b53, bcm_sf2, qca8k), and there is only
one for which we need to introduce a helper: mt7530. We do introduce
dsa_ds_get_cpu_dp() which reads the CPU port from ds->cpu_port_mask.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c |  4 ++--
 drivers/net/dsa/bcm_sf2.c        | 10 ++++++----
 drivers/net/dsa/mt7530.c         |  4 +++-
 drivers/net/dsa/mv88e6060.c      |  2 +-
 drivers/net/dsa/qca8k.c          |  2 +-
 include/net/dsa.h                |  6 ++++++
 net/dsa/dsa2.c                   | 11 +++++++++++
 net/dsa/legacy.c                 |  1 +
 net/dsa/slave.c                  |  1 -
 9 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index e68d368e20ac..faec6fcacd31 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -1341,7 +1341,7 @@ EXPORT_SYMBOL(b53_fdb_dump);
 int b53_br_join(struct dsa_switch *ds, int port, struct net_device *br)
 {
 	struct b53_device *dev = ds->priv;
-	s8 cpu_port = ds->dst->cpu_dp->index;
+	s8 cpu_port = dev->cpu_port;
 	u16 pvlan, reg;
 	unsigned int i;
 
@@ -1387,7 +1387,7 @@ void b53_br_leave(struct dsa_switch *ds, int port, struct net_device *br)
 {
 	struct b53_device *dev = ds->priv;
 	struct b53_vlan *vl = &dev->vlans[0];
-	s8 cpu_port = ds->dst->cpu_dp->index;
+	s8 cpu_port = dev->cpu_port;
 	unsigned int i;
 	u16 pvlan, reg, pvid;
 
diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 76e98e8ed315..9744100d0276 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -227,7 +227,7 @@ static int bcm_sf2_port_setup(struct dsa_switch *ds, int port,
 			      struct phy_device *phy)
 {
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
-	s8 cpu_port = ds->dst->cpu_dp->index;
+	s8 cpu_port = priv->dev->cpu_port;
 	unsigned int i;
 	u32 reg;
 
@@ -806,8 +806,9 @@ static int bcm_sf2_sw_resume(struct dsa_switch *ds)
 static void bcm_sf2_sw_get_wol(struct dsa_switch *ds, int port,
 			       struct ethtool_wolinfo *wol)
 {
-	struct net_device *p = ds->dst[ds->index].cpu_dp->netdev;
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
+	struct dsa_port *cpu_dp = ds->ports[port].cpu_dp;
+	struct net_device *p = cpu_dp->netdev;
 	struct ethtool_wolinfo pwol;
 
 	/* Get the parent device WoL settings */
@@ -829,9 +830,10 @@ static void bcm_sf2_sw_get_wol(struct dsa_switch *ds, int port,
 static int bcm_sf2_sw_set_wol(struct dsa_switch *ds, int port,
 			      struct ethtool_wolinfo *wol)
 {
-	struct net_device *p = ds->dst[ds->index].cpu_dp->netdev;
 	struct bcm_sf2_priv *priv = bcm_sf2_to_priv(ds);
-	s8 cpu_port = ds->dst->cpu_dp->index;
+	struct dsa_port *cpu_dp = ds->ports[port].cpu_dp;
+	struct net_device *p = cpu_dp->netdev;
+	s8 cpu_port = cpu_dp->index;
 	struct ethtool_wolinfo pwol;
 
 	p->ethtool_ops->get_wol(p, &pwol);
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 1e46418a3b74..9b1b76c7b927 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -907,6 +907,7 @@ static int
 mt7530_setup(struct dsa_switch *ds)
 {
 	struct mt7530_priv *priv = ds->priv;
+	struct dsa_port *cpu_dp;
 	int ret, i;
 	u32 id, val;
 	struct device_node *dn;
@@ -916,7 +917,8 @@ mt7530_setup(struct dsa_switch *ds)
 	 * controller also is the container for two GMACs nodes representing
 	 * as two netdev instances.
 	 */
-	dn = ds->dst->cpu_dp->netdev->dev.of_node->parent;
+	cpu_dp = dsa_ds_get_cpu_dp(ds);
+	dn = cpu_dp->netdev->dev.of_node->parent;
 	priv->ethernet = syscon_node_to_regmap(dn);
 	if (IS_ERR(priv->ethernet))
 		return PTR_ERR(priv->ethernet);
diff --git a/drivers/net/dsa/mv88e6060.c b/drivers/net/dsa/mv88e6060.c
index dce7fa57eb55..621cdc46ad81 100644
--- a/drivers/net/dsa/mv88e6060.c
+++ b/drivers/net/dsa/mv88e6060.c
@@ -176,7 +176,7 @@ static int mv88e6060_setup_port(struct dsa_switch *ds, int p)
 		  ((p & 0xf) << PORT_VLAN_MAP_DBNUM_SHIFT) |
 		   (dsa_is_cpu_port(ds, p) ?
 			ds->enabled_port_mask :
-			BIT(ds->dst->cpu_dp->index)));
+			BIT(ds->ports[p].cpu_dp->index)));
 
 	/* Port Association Vector: when learning source addresses
 	 * of packets, add the address to the address database using
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index b3bee7eab45f..68b45298f6d1 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -506,7 +506,7 @@ qca8k_setup(struct dsa_switch *ds)
 		pr_warn("regmap initialization failed");
 
 	/* Initialize CPU port pad mode (xMII type, delays...) */
-	phy_mode = of_get_phy_mode(ds->dst->cpu_dp->dn);
+	phy_mode = of_get_phy_mode(ds->ports[QCA8K_CPU_PORT].dn);
 	if (phy_mode < 0) {
 		pr_err("Can't find phy-mode for master device\n");
 		return phy_mode;
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 58969b9a090c..264cb3f93764 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -11,6 +11,7 @@
 #ifndef __LINUX_NET_DSA_H
 #define __LINUX_NET_DSA_H
 
+#include <linux/bitops.h>
 #include <linux/if.h>
 #include <linux/if_ether.h>
 #include <linux/list.h>
@@ -261,6 +262,11 @@ static inline bool dsa_is_port_initialized(struct dsa_switch *ds, int p)
 	return ds->enabled_port_mask & (1 << p) && ds->ports[p].netdev;
 }
 
+static inline struct dsa_port *dsa_ds_get_cpu_dp(struct dsa_switch *ds)
+{
+	return &ds->ports[fls(ds->cpu_port_mask) - 1];
+}
+
 static inline u8 dsa_upstream_port(struct dsa_switch *ds)
 {
 	struct dsa_switch_tree *dst = ds->dst;
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 2674bdf03fef..3878dcd081bb 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -488,6 +488,8 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index,
 	enum dsa_tag_protocol tag_protocol;
 	struct net_device *ethernet_dev;
 	struct device_node *ethernet;
+	struct dsa_port *p;
+	unsigned int i;
 
 	if (port->dn) {
 		ethernet = of_parse_phandle(port->dn, "ethernet", 0);
@@ -505,6 +507,15 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index,
 	if (!dst->cpu_dp) {
 		dst->cpu_dp = port;
 		dst->cpu_dp->netdev = ethernet_dev;
+
+		for (i = 0; i < ds->num_ports; i++) {
+			p = &ds->ports[i];
+			if (!dsa_port_is_valid(p) ||
+			    i == index)
+				continue;
+
+			p->cpu_dp = port;
+		}
 	}
 
 	tag_protocol = ds->ops->get_tag_protocol(ds);
diff --git a/net/dsa/legacy.c b/net/dsa/legacy.c
index 443ebf3b5e5b..217034cb25cd 100644
--- a/net/dsa/legacy.c
+++ b/net/dsa/legacy.c
@@ -129,6 +129,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 			ds->dsa_port_mask |= 1 << i;
 		} else {
 			ds->enabled_port_mask |= 1 << i;
+			ds->ports[i].cpu_dp = dst->cpu_dp;
 		}
 		valid_name_found = true;
 	}
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index a73c1de398b5..46fd4a985e0a 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1179,7 +1179,6 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 	p->old_duplex = -1;
 
 	ds->ports[port].netdev = slave_dev;
-	p->dp->cpu_dp = cpu_dp;
 	ret = register_netdev(slave_dev);
 	if (ret) {
 		netdev_err(master, "error %d registering interface %s\n",
-- 
2.9.3

Re: [PATCH net-next v2 0/5] net: dsa: Multi-CPU ground work (v2)

[Florian Fainelli]

On 06/06/2017 05:03 PM, Florian Fainelli wrote:
> Hi all,
> 
> This patch series prepares the ground for adding mutliple CPU port support to
> DSA, and starts by removing redundant pieces of information such as
> master_netdev which is cpu_dp->ethernet. Finally drivers are moved away from
> directly accessing ds->dst->cpu_dp and use appropriate helper functions.
> 
> Note that if you have Device Tree blobs/platform configurations that are
> currently listing multiple CPU ports, the proposed behavior in
> dsa_ds_get_cpu_dp() will be to return the last bit set in ds->cpu_port_mask.
> 
> Future plans include:
> - making dst->cpu_dp a flexible data structure (array, list, you name it)
> - having the ability for drivers to return a default/preferred CPU port (if
>   necessary)
> 
> Changes in v2:
> 
> - added Reviewed-by tags
> - assign port->cpu_dp earlier before ops->setup() has run

There are some hunks in patch 5 that actually belong in patch 3, I will
post a v3 after getting some more feedback.

> 
> Florian Fainelli (5):
>   net: dsa: Remove master_netdev and use dst->cpu_dp->netdev
>   net: dsa: Relocate master ethtool operations
>   net: dsa: Associate slave network device with CPU port
>   net: dsa: Introduce dsa_dst_get_cpu_dp()
>   net: dsa: Stop accessing ds->dst->cpu_dp in drivers
> 
>  drivers/net/dsa/b53/b53_common.c |  4 +--
>  drivers/net/dsa/bcm_sf2.c        | 10 +++++---
>  drivers/net/dsa/mt7530.c         |  6 +++--
>  drivers/net/dsa/mv88e6060.c      |  2 +-
>  drivers/net/dsa/qca8k.c          |  2 +-
>  include/net/dsa.h                | 29 +++++++++-------------
>  net/dsa/dsa.c                    | 19 ++++----------
>  net/dsa/dsa2.c                   | 27 ++++++++++++--------
>  net/dsa/dsa_priv.h               | 10 ++++++++
>  net/dsa/legacy.c                 | 23 ++++++++++-------
>  net/dsa/slave.c                  | 53 ++++++++++++++++++++--------------------
>  net/dsa/tag_brcm.c               |  5 ++--
>  net/dsa/tag_ksz.c                |  5 ++--
>  net/dsa/tag_qca.c                |  3 ++-
>  net/dsa/tag_trailer.c            |  5 ++--
>  15 files changed, 107 insertions(+), 96 deletions(-)
> 


-- 
Florian

Re: [PATCH net-next v2 5/5] net: dsa: Stop accessing ds->dst->cpu_dp in drivers

[Vivien Didelot]

Hi Florian,

Florian Fainelli <f.fainelli@gmail.com> writes:

> +static inline struct dsa_port *dsa_ds_get_cpu_dp(struct dsa_switch *ds)
> +{
> +	return &ds->ports[fls(ds->cpu_port_mask) - 1];
> +}

So as I said in v2, now that a driver is guaranteed that dp->cpu_dp is
correctly assigned at setup time, isn't better (especially for future
multi-CPU support) to provide an helper which returns the CPU port for a
given port? i.e. dsa_get_cpu_port(struct dsa_switch *ds, int port).

Or is there something blocking? I might be wrong.

Note that I'm suggesting s/dsa_ds_get_cpu_dp/dsa_get_cpu_port/ since
public DSA API does not need to use variable shortcuts such as ds or dp,
but that's a minor suggestion?

Thanks,

        Vivien

Re: [PATCH net-next v2 0/5] net: dsa: Multi-CPU ground work (v2)

[Vivien Didelot]

Hi Florian,

Since you'll respin a v3, there are still a few typos here:

Florian Fainelli <f.fainelli@gmail.com> writes:

> This patch series prepares the ground for adding mutliple CPU port support to

                                                   multiple

> DSA, and starts by removing redundant pieces of information such as
> master_netdev which is cpu_dp->ethernet. Finally drivers are moved away from

                         cpu_dp->netdev

> directly accessing ds->dst->cpu_dp and use appropriate helper functions.


Thanks,

        Vivien

Re: [PATCH net-next v2 1/5] net: dsa: Remove master_netdev and use dst->cpu_dp->netdev

[Vivien Didelot]

Florian Fainelli <f.fainelli@gmail.com> writes:

> In preparation for supporting multiple CPU ports, remove
> dst->master_netdev and ds->master_netdev and replace them with only one
> instance of the common object we have for a port: struct
> dsa_port::netdev. ds->master_netdev is currently write only and would be
> helpful in the case where we have two switches, both with CPU ports, and
> also connected within each other, which the multi-CPU port patch series
> would address.
>
> While at it, introduce a helper function used in net/dsa/slave.c to
> immediately get a reference on the master network device called
> dsa_master_netdev().
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Re: [PATCH net-next v2 5/5] net: dsa: Stop accessing ds->dst->cpu_dp in drivers

[Florian Fainelli]

2017-06-06 19:11 GMT-07:00 Vivien Didelot <vivien.didelot@savoirfairelinux.com>:
> Hi Florian,
>
> Florian Fainelli <f.fainelli@gmail.com> writes:
>
>> +static inline struct dsa_port *dsa_ds_get_cpu_dp(struct dsa_switch *ds)
>> +{
>> +     return &ds->ports[fls(ds->cpu_port_mask) - 1];
>> +}
>
> So as I said in v2, now that a driver is guaranteed that dp->cpu_dp is
> correctly assigned at setup time, isn't better (especially for future
> multi-CPU support) to provide an helper which returns the CPU port for a
> given port? i.e. dsa_get_cpu_port(struct dsa_switch *ds, int port).
>
> Or is there something blocking? I might be wrong.

mt7530.c needs access to the CPU port at ops->setup() time which is
why this is still here.

>
> Note that I'm suggesting s/dsa_ds_get_cpu_dp/dsa_get_cpu_port/ since
> public DSA API does not need to use variable shortcuts such as ds or dp,
> but that's a minor suggestion?

We also have a dsa_dst_get_cpu_dp() but sure dsa_get_cpu_port() works for me.
-- 
Florian

Re: [PATCH net-next v2 5/5] net: dsa: Stop accessing ds->dst->cpu_dp in drivers

[Vivien Didelot]

Hi Florian,

Florian Fainelli <f.fainelli@gmail.com> writes:

>> So as I said in v2, now that a driver is guaranteed that dp->cpu_dp is
>> correctly assigned at setup time, isn't better (especially for future
>> multi-CPU support) to provide an helper which returns the CPU port for a
>> given port? i.e. dsa_get_cpu_port(struct dsa_switch *ds, int port).
>>
>> Or is there something blocking? I might be wrong.
>
> mt7530.c needs access to the CPU port at ops->setup() time which is
> why this is still here.

Yes, mt7530 is the only one doing this and has an hardcoded CPU port. So
what I meant was, shouldn't we have this instead:

    struct dsa_port *dsa_get_cpu_port(struct dsa_switch *ds, int port)
    {
        return ds->ports[port].cpu_dp;
    }

And:

-       dn = ds->dst->cpu_dp->netdev->dev.of_node->parent;
+       cpu_dp = dsa_get_cpu_port(ds, MT7530_CPU_PORT);
+       dn = cpu_dp->netdev->dev.of_node->parent;


Thanks,

        Vivien

Re: [PATCH net-next v2 5/5] net: dsa: Stop accessing ds->dst->cpu_dp in drivers

[Florian Fainelli]

On 06/07/2017 10:15 AM, Vivien Didelot wrote:
> Hi Florian,
> 
> Florian Fainelli <f.fainelli@gmail.com> writes:
> 
>>> So as I said in v2, now that a driver is guaranteed that dp->cpu_dp is
>>> correctly assigned at setup time, isn't better (especially for future
>>> multi-CPU support) to provide an helper which returns the CPU port for a
>>> given port? i.e. dsa_get_cpu_port(struct dsa_switch *ds, int port).
>>>
>>> Or is there something blocking? I might be wrong.
>>
>> mt7530.c needs access to the CPU port at ops->setup() time which is
>> why this is still here.
> 
> Yes, mt7530 is the only one doing this and has an hardcoded CPU port. So
> what I meant was, shouldn't we have this instead:
> 
>     struct dsa_port *dsa_get_cpu_port(struct dsa_switch *ds, int port)
>     {
>         return ds->ports[port].cpu_dp;
>     }

We don't actually have a CPU port point to itself:

+
+		for (i = 0; i < ds->num_ports; i++) {
+			p = &ds->ports[i];
+			if (!dsa_port_is_valid(p) ||
+			    i == index) <=============
+				continue;
+
+			p->cpu_dp = port;
+		}
 	}

> 
> And:
> 
> -       dn = ds->dst->cpu_dp->netdev->dev.of_node->parent;
> +       cpu_dp = dsa_get_cpu_port(ds, MT7530_CPU_PORT);
> +       dn = cpu_dp->netdev->dev.of_node->parent;

If we are giving the port number to get its cpu_dp pointer back, that
seems a bit pointless.

I still think the helper with fls(ds->cpu_port_mask) - 1 is better in
that it will return what you have configured from Device Tree/platform
data. MT7530 does allow the CPU port being arbitrary, and it would
disable MTK tags in that case.

Thanks!
-- 
Florian

Re: [PATCH net-next v2 5/5] net: dsa: Stop accessing ds->dst->cpu_dp in drivers

[Vivien Didelot]

Florian Fainelli <f.fainelli@gmail.com> writes:

> On 06/07/2017 10:15 AM, Vivien Didelot wrote:
>> Hi Florian,
>> 
>> Florian Fainelli <f.fainelli@gmail.com> writes:
>> 
>>>> So as I said in v2, now that a driver is guaranteed that dp->cpu_dp is
>>>> correctly assigned at setup time, isn't better (especially for future
>>>> multi-CPU support) to provide an helper which returns the CPU port for a
>>>> given port? i.e. dsa_get_cpu_port(struct dsa_switch *ds, int port).
>>>>
>>>> Or is there something blocking? I might be wrong.
>>>
>>> mt7530.c needs access to the CPU port at ops->setup() time which is
>>> why this is still here.
>> 
>> Yes, mt7530 is the only one doing this and has an hardcoded CPU port. So
>> what I meant was, shouldn't we have this instead:
>> 
>>     struct dsa_port *dsa_get_cpu_port(struct dsa_switch *ds, int port)
>>     {
>>         return ds->ports[port].cpu_dp;
>>     }
>
> We don't actually have a CPU port point to itself:
>
> +
> +		for (i = 0; i < ds->num_ports; i++) {
> +			p = &ds->ports[i];
> +			if (!dsa_port_is_valid(p) ||
> +			    i == index) <=============
> +				continue;
> +
> +			p->cpu_dp = port;
> +		}
>  	}
>
>> 
>> And:
>> 
>> -       dn = ds->dst->cpu_dp->netdev->dev.of_node->parent;
>> +       cpu_dp = dsa_get_cpu_port(ds, MT7530_CPU_PORT);
>> +       dn = cpu_dp->netdev->dev.of_node->parent;
>
> If we are giving the port number to get its cpu_dp pointer back, that
> seems a bit pointless.
>
> I still think the helper with fls(ds->cpu_port_mask) - 1 is better in
> that it will return what you have configured from Device Tree/platform
> data. MT7530 does allow the CPU port being arbitrary, and it would
> disable MTK tags in that case.

OK looks good then!

Re: [PATCH net-next v2 5/5] net: dsa: Stop accessing ds->dst->cpu_dp in drivers

[Vivien Didelot]

Hi Florian,

Sorry to bother you again, I don't want to be annoying but I might not
get things right still.

Florian Fainelli <f.fainelli@gmail.com> writes:

>>>> So as I said in v2, now that a driver is guaranteed that dp->cpu_dp is
>>>> correctly assigned at setup time, isn't better (especially for future
>>>> multi-CPU support) to provide an helper which returns the CPU port for a
>>>> given port? i.e. dsa_get_cpu_port(struct dsa_switch *ds, int port).
>>>>
>>>> Or is there something blocking? I might be wrong.
>>>
>>> mt7530.c needs access to the CPU port at ops->setup() time which is
>>> why this is still here.
>> 
>> Yes, mt7530 is the only one doing this and has an hardcoded CPU port. So
>> what I meant was, shouldn't we have this instead:
>> 
>>     struct dsa_port *dsa_get_cpu_port(struct dsa_switch *ds, int port)
>>     {
>>         return ds->ports[port].cpu_dp;
>>     }
>
> We don't actually have a CPU port point to itself:
>
> +
> +		for (i = 0; i < ds->num_ports; i++) {
> +			p = &ds->ports[i];
> +			if (!dsa_port_is_valid(p) ||
> +			    i == index) <=============
> +				continue;
> +
> +			p->cpu_dp = port;
> +		}
>  	}
>
>> 
>> And:
>> 
>> -       dn = ds->dst->cpu_dp->netdev->dev.of_node->parent;
>> +       cpu_dp = dsa_get_cpu_port(ds, MT7530_CPU_PORT);
>> +       dn = cpu_dp->netdev->dev.of_node->parent;
>
> If we are giving the port number to get its cpu_dp pointer back, that
> seems a bit pointless.

What a driver may want is the master interface (e.g. eth0), actually
soldered to the dedicated switch CPU port, that will be used to
send/receive frames.

Also I do not think that it is a good thing that a DSA driver play much
in dsa_port structures (they are ideally DSA core only specific data).
They only seem to need the master interface, so what I see is:

    static inline struct net_device *dsa_get_master(struct dsa_switch *ds, int port)
    {
            struct dsa_port *dp = &ds->ports[port];

            if (!dsa_is_cpu_port(ds, port))
                    dp = dp->cpu_dp;

            return dp->netdev;
    }

> I still think the helper with fls(ds->cpu_port_mask) - 1 is better in
> that it will return what you have configured from Device Tree/platform
> data. MT7530 does allow the CPU port being arbitrary, and it would
> disable MTK tags in that case.

If MT7530 allows several CPU ports, what is MT7530_CPU_PORT then?
(Maybe Sean can give me some details here?)

Now let's think that you can have several CPU ports (as with mv88e6xxx).
I think it is the driver responsibility to iterate over CPU capable
ports and inspect the master devices if they need to, instead of having
DSA core return an arbitrary one (which might be the wrong one.)

This is a static data (describing to which SoC interface a switch CPU
port is wired) that should've been parsed by DSA core before setup.

Thanks,

        Vivien

Re: [PATCH net-next v2 5/5] net: dsa: Stop accessing ds->dst->cpu_dp in drivers

[Florian Fainelli]

On 06/07/2017 04:24 PM, Vivien Didelot wrote:
> Hi Florian,
> 
> Sorry to bother you again, I don't want to be annoying but I might not
> get things right still.
> 
> Florian Fainelli <f.fainelli@gmail.com> writes:
> 
>>>>> So as I said in v2, now that a driver is guaranteed that dp->cpu_dp is
>>>>> correctly assigned at setup time, isn't better (especially for future
>>>>> multi-CPU support) to provide an helper which returns the CPU port for a
>>>>> given port? i.e. dsa_get_cpu_port(struct dsa_switch *ds, int port).
>>>>>
>>>>> Or is there something blocking? I might be wrong.
>>>>
>>>> mt7530.c needs access to the CPU port at ops->setup() time which is
>>>> why this is still here.
>>>
>>> Yes, mt7530 is the only one doing this and has an hardcoded CPU port. So
>>> what I meant was, shouldn't we have this instead:
>>>
>>>     struct dsa_port *dsa_get_cpu_port(struct dsa_switch *ds, int port)
>>>     {
>>>         return ds->ports[port].cpu_dp;
>>>     }
>>
>> We don't actually have a CPU port point to itself:
>>
>> +
>> +		for (i = 0; i < ds->num_ports; i++) {
>> +			p = &ds->ports[i];
>> +			if (!dsa_port_is_valid(p) ||
>> +			    i == index) <=============
>> +				continue;
>> +
>> +			p->cpu_dp = port;
>> +		}
>>  	}
>>
>>>
>>> And:
>>>
>>> -       dn = ds->dst->cpu_dp->netdev->dev.of_node->parent;
>>> +       cpu_dp = dsa_get_cpu_port(ds, MT7530_CPU_PORT);
>>> +       dn = cpu_dp->netdev->dev.of_node->parent;
>>
>> If we are giving the port number to get its cpu_dp pointer back, that
>> seems a bit pointless.
> 
> What a driver may want is the master interface (e.g. eth0), actually
> soldered to the dedicated switch CPU port, that will be used to
> send/receive frames.>
> Also I do not think that it is a good thing that a DSA driver play much
> in dsa_port structures (they are ideally DSA core only specific data).
> They only seem to need the master interface, so what I see is:
> 
>     static inline struct net_device *dsa_get_master(struct dsa_switch *ds, int port)
>     {
>             struct dsa_port *dp = &ds->ports[port];
> 
>             if (!dsa_is_cpu_port(ds, port))
>                     dp = dp->cpu_dp;
> 
>             return dp->netdev;
>     }

The port parameter is kind of pointless, that is what I was trying to
say, see below.

> 
>> I still think the helper with fls(ds->cpu_port_mask) - 1 is better in
>> that it will return what you have configured from Device Tree/platform
>> data. MT7530 does allow the CPU port being arbitrary, and it would
>> disable MTK tags in that case.
> 
> If MT7530 allows several CPU ports, what is MT7530_CPU_PORT then?
> (Maybe Sean can give me some details here?)

MT7530_CPU_PORT = 6 and there is a define above for 7 ports, so it is
presumably the default CPU port that the switch uses. With Broadcom
switches you could have port 5, 7 or 8 as CPU ports but 8 is still the
default for most 9-ports capable switches.

> 
> Now let's think that you can have several CPU ports (as with mv88e6xxx).
> I think it is the driver responsibility to iterate over CPU capable
> ports and inspect the master devices if they need to, instead of having
> DSA core return an arbitrary one (which might be the wrong one.)

OK, then are you okay if I do this in mt7530.c instead:

diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 1e46418a3b74..d5b63958dd85 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -907,17 +907,26 @@ static int
 mt7530_setup(struct dsa_switch *ds)
 {
        struct mt7530_priv *priv = ds->priv;
+       struct dsa_port *port;
        int ret, i;
        u32 id, val;
        struct device_node *dn;
        struct mt7530_dummy_poll p;

-       /* The parent node of cpu_dp->netdev which holds the common system
-        * controller also is the container for two GMACs nodes representing
-        * as two netdev instances.
-        */
-       dn = ds->dst->cpu_dp->netdev->dev.of_node->parent;
-       priv->ethernet = syscon_node_to_regmap(dn);
+       for (i = 0; i < ds->num_ports; i++) {
+               port = &ds->ports[i];
+               /* The parent node of cpu_dp->netdev which holds the common
+                * system controller also is the container for two GMACs
nodes
+                * representing as two netdev instances.
+                */
+               if (dsa_is_cpu_port(ds, i)) {
+                       dn = port->netdev->dev.of_node->parent;
+                       if (priv->ethernet)
+                               return -EEXIST;
+                       priv->ethernet = syscon_node_to_regmap(dn);
+               }
+       }
+
        if (IS_ERR(priv->ethernet))
                return PTR_ERR(priv->ethernet);



> 
> This is a static data (describing to which SoC interface a switch CPU
> port is wired) that should've been parsed by DSA core before setup.

Yes, sure.
-- 
Florian

Re: [PATCH net-next v2 5/5] net: dsa: Stop accessing ds->dst->cpu_dp in drivers

[Vivien Didelot]

Hi Florian,

Florian Fainelli <f.fainelli@gmail.com> writes:

>> Also I do not think that it is a good thing that a DSA driver play much
>> in dsa_port structures (they are ideally DSA core only specific data).
>> They only seem to need the master interface, so what I see is:
>> 
>>     static inline struct net_device *dsa_get_master(struct dsa_switch *ds, int port)
>>     {
>>             struct dsa_port *dp = &ds->ports[port];
>> 
>>             if (!dsa_is_cpu_port(ds, port))
>>                     dp = dp->cpu_dp;
>> 
>>             return dp->netdev;
>>     }
>
> The port parameter is kind of pointless, that is what I was trying to
> say, see below.
>
> MT7530_CPU_PORT = 6 and there is a define above for 7 ports, so it is
> presumably the default CPU port that the switch uses. With Broadcom
> switches you could have port 5, 7 or 8 as CPU ports but 8 is still the
> default for most 9-ports capable switches.
>
>> Now let's think that you can have several CPU ports (as with mv88e6xxx).
>> I think it is the driver responsibility to iterate over CPU capable
>> ports and inspect the master devices if they need to, instead of having
>> DSA core return an arbitrary one (which might be the wrong one.)
>
> OK, then are you okay if I do this in mt7530.c instead:
>
> diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
> index 1e46418a3b74..d5b63958dd85 100644
> --- a/drivers/net/dsa/mt7530.c
> +++ b/drivers/net/dsa/mt7530.c
> @@ -907,17 +907,26 @@ static int
>  mt7530_setup(struct dsa_switch *ds)
>  {
>         struct mt7530_priv *priv = ds->priv;
> +       struct dsa_port *port;
>         int ret, i;
>         u32 id, val;
>         struct device_node *dn;
>         struct mt7530_dummy_poll p;
>
> -       /* The parent node of cpu_dp->netdev which holds the common system
> -        * controller also is the container for two GMACs nodes representing
> -        * as two netdev instances.
> -        */
> -       dn = ds->dst->cpu_dp->netdev->dev.of_node->parent;
> -       priv->ethernet = syscon_node_to_regmap(dn);
> +       for (i = 0; i < ds->num_ports; i++) {
> +               port = &ds->ports[i];
> +               /* The parent node of cpu_dp->netdev which holds the common
> +                * system controller also is the container for two GMACs
> nodes
> +                * representing as two netdev instances.
> +                */
> +               if (dsa_is_cpu_port(ds, i)) {
> +                       dn = port->netdev->dev.of_node->parent;
> +                       if (priv->ethernet)
> +                               return -EEXIST;
> +                       priv->ethernet = syscon_node_to_regmap(dn);
> +               }
> +       }
> +
>         if (IS_ERR(priv->ethernet))
>                 return PTR_ERR(priv->ethernet);

I think it is the correct way to do this.

This was the reason why I suggested a dsa_get_master() alternative, even
though it is not necessary per-se, it was just to reduce boilerplate and
avoid exposing too much the dsa_port structures.

dsa_ds_get_cpu_dp() doesn't make much sense anymore, right? If there are
2 CPU ports in use, it is better that the driver iterates on them itself
as you did, instead of having DSA return the first one it knows about.

Thanks,

        Vivien