[PATCH v3 net-next 0/4] net: dsa: microchip: add MIB counters support

[PATCH v3 net-next 0/4] net: dsa: microchip: add MIB counters support

[Tristram.Ha]

From: Tristram Ha <Tristram.Ha@microchip.com>

This series of patches is to modify the KSZ9477 DSA driver to read MIB
counters periodically to avoid overflow.

The MIB counters should be read only when there is link.  Otherwise it is
a waste of time as hardware never increases the counters.

Functions are added to check the port link status so that MIB counters
read call is used efficiently.

v3
- Use netif_carrier_ok instead of checking the phy device pointer

v2
- Create macro similar to readx_poll_timeout to use with switch
- Create ksz_port_cleanup function so that variables like on_ports and
  live_ports can be updated inside it

v1
- Use readx_poll_timeout
- Do not clear MIB counters when port is enabled
- Do not advertise 1000 half-duplex mode when port is enabled
- Do not use freeze function as MIB counters may miss counts

Tristram Ha (4):
  net: dsa: microchip: prepare PHY for proper advertisement
  net: dsa: microchip: add MIB counter reading support
  net: dsa: microchip: get port link status
  net: dsa: microchip: remove unnecessary include headers

 drivers/net/dsa/microchip/ksz9477.c    | 137 +++++++++++++++++----------
 drivers/net/dsa/microchip/ksz_common.c | 163 ++++++++++++++++++++++++++++++++-
 drivers/net/dsa/microchip/ksz_common.h |  27 +++++-
 drivers/net/dsa/microchip/ksz_priv.h   |  14 +--
 4 files changed, 284 insertions(+), 57 deletions(-)

-- 
1.9.1

[PATCH v3 net-next 1/4] net: dsa: microchip: prepare PHY for proper advertisement

[Tristram.Ha]

From: Tristram Ha <Tristram.Ha@microchip.com>

Prepare PHY for proper advertisement as sometimes the PHY in the switch
has its own problems even though it may share the PHY id from regular PHY
but the fixes in the PHY driver do not apply.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c    | 13 ++++++++++++-
 drivers/net/dsa/microchip/ksz_common.c |  3 ++-
 drivers/net/dsa/microchip/ksz_priv.h   |  4 +++-
 3 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 674d77e..4573b6e 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -2,7 +2,7 @@
 /*
  * Microchip KSZ9477 switch driver main logic
  *
- * Copyright (C) 2017-2018 Microchip Technology Inc.
+ * Copyright (C) 2017-2019 Microchip Technology Inc.
  */
 
 #include <linux/delay.h>
@@ -966,6 +966,16 @@ static void ksz9477_port_mirror_del(struct dsa_switch *ds, int port,
 			     PORT_MIRROR_SNIFFER, false);
 }
 
+static void ksz9477_phy_setup(struct ksz_device *dev, int port,
+			      struct phy_device *phy)
+{
+	if (port < dev->phy_port_cnt) {
+		/* The MAC actually cannot run in 1000 half-duplex mode. */
+		phy_remove_link_mode(phy,
+				     ETHTOOL_LINK_MODE_1000baseT_Half_BIT);
+	}
+}
+
 static void ksz9477_port_setup(struct ksz_device *dev, int port, bool cpu_port)
 {
 	u8 data8;
@@ -1299,6 +1309,7 @@ static void ksz9477_switch_exit(struct ksz_device *dev)
 	.get_port_addr = ksz9477_get_port_addr,
 	.cfg_port_member = ksz9477_cfg_port_member,
 	.flush_dyn_mac_table = ksz9477_flush_dyn_mac_table,
+	.phy_setup = ksz9477_phy_setup,
 	.port_setup = ksz9477_port_setup,
 	.shutdown = ksz9477_reset_switch,
 	.detect = ksz9477_switch_detect,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 8a5111f..6f72842 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -2,7 +2,7 @@
 /*
  * Microchip switch driver main logic
  *
- * Copyright (C) 2017-2018 Microchip Technology Inc.
+ * Copyright (C) 2017-2019 Microchip Technology Inc.
  */
 
 #include <linux/delay.h>
@@ -238,6 +238,7 @@ int ksz_enable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
 
 	/* setup slave port */
 	dev->dev_ops->port_setup(dev, port, false);
+	dev->dev_ops->phy_setup(dev, port, phy);
 
 	/* port_stp_state_set() will be called after to enable the port so
 	 * there is no need to do anything.
diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h
index 60b4901..0fdc58b 100644
--- a/drivers/net/dsa/microchip/ksz_priv.h
+++ b/drivers/net/dsa/microchip/ksz_priv.h
@@ -2,7 +2,7 @@
  *
  * Microchip KSZ series switch common definitions
  *
- * Copyright (C) 2017-2018 Microchip Technology Inc.
+ * Copyright (C) 2017-2019 Microchip Technology Inc.
  */
 
 #ifndef __KSZ_PRIV_H
@@ -137,6 +137,8 @@ struct ksz_dev_ops {
 	u32 (*get_port_addr)(int port, int offset);
 	void (*cfg_port_member)(struct ksz_device *dev, int port, u8 member);
 	void (*flush_dyn_mac_table)(struct ksz_device *dev, int port);
+	void (*phy_setup)(struct ksz_device *dev, int port,
+			  struct phy_device *phy);
 	void (*port_setup)(struct ksz_device *dev, int port, bool cpu_port);
 	void (*r_phy)(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);
 	void (*w_phy)(struct ksz_device *dev, u16 phy, u16 reg, u16 val);
-- 
1.9.1

[PATCH v3 net-next 2/4] net: dsa: microchip: add MIB counter reading support

[Tristram.Ha]

From: Tristram Ha <Tristram.Ha@microchip.com>

Add background MIB counter reading support.

Port MIB counters should only be read when there is link.  Otherwise it is
a waste of time as hardware never increases those counters.  There are
exceptions as some switches keep track of dropped counts no matter what.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c    | 118 ++++++++++++++++++++------------
 drivers/net/dsa/microchip/ksz_common.c | 121 +++++++++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h |  24 ++++++-
 drivers/net/dsa/microchip/ksz_priv.h   |   9 ++-
 4 files changed, 224 insertions(+), 48 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 4573b6e..e2d74c7 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -18,8 +18,8 @@
 #include <net/switchdev.h>
 
 #include "ksz_priv.h"
-#include "ksz_common.h"
 #include "ksz9477_reg.h"
+#include "ksz_common.h"
 
 static const struct {
 	int index;
@@ -259,6 +259,71 @@ static int ksz9477_reset_switch(struct ksz_device *dev)
 	return 0;
 }
 
+static void ksz9477_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
+			      u64 *cnt)
+{
+	struct ksz_port *p = &dev->ports[port];
+	u32 data;
+	int ret;
+
+	/* retain the flush/freeze bit */
+	data = p->freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;
+	data |= MIB_COUNTER_READ;
+	data |= (addr << MIB_COUNTER_INDEX_S);
+	ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, data);
+
+	ret = ksz_pread_poll_timeout(ksz_pread32, dev, port,
+				     REG_PORT_MIB_CTRL_STAT__4, data,
+				     !(data & MIB_COUNTER_READ), 10, 1000);
+
+	/* failed to read MIB. get out of loop */
+	if (ret < 0) {
+		dev_dbg(dev->dev, "Failed to get MIB\n");
+		return;
+	}
+
+	/* count resets upon read */
+	ksz_pread32(dev, port, REG_PORT_MIB_DATA, &data);
+	*cnt += data;
+}
+
+static void ksz9477_r_mib_pkt(struct ksz_device *dev, int port, u16 addr,
+			      u64 *dropped, u64 *cnt)
+{
+	addr = ksz9477_mib_names[addr].index;
+	ksz9477_r_mib_cnt(dev, port, addr, cnt);
+}
+
+static void ksz9477_freeze_mib(struct ksz_device *dev, int port, bool freeze)
+{
+	u32 val = freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;
+	struct ksz_port *p = &dev->ports[port];
+
+	/* enable/disable the port for flush/freeze function */
+	mutex_lock(&p->mib.cnt_mutex);
+	ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, val);
+
+	/* used by MIB counter reading code to know freeze is enabled */
+	p->freeze = freeze;
+	mutex_unlock(&p->mib.cnt_mutex);
+}
+
+static void ksz9477_port_init_cnt(struct ksz_device *dev, int port)
+{
+	struct ksz_port_mib *mib = &dev->ports[port].mib;
+
+	/* flush all enabled port MIB counters */
+	mutex_lock(&mib->cnt_mutex);
+	ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4,
+		     MIB_COUNTER_FLUSH_FREEZE);
+	ksz_write8(dev, REG_SW_MAC_CTRL_6, SW_MIB_COUNTER_FLUSH);
+	ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, 0);
+	mutex_unlock(&mib->cnt_mutex);
+
+	mib->cnt_ptr = 0;
+	memset(mib->counters, 0, dev->mib_cnt * sizeof(u64));
+}
+
 static enum dsa_tag_protocol ksz9477_get_tag_protocol(struct dsa_switch *ds,
 						      int port)
 {
@@ -342,47 +407,6 @@ static void ksz9477_get_strings(struct dsa_switch *ds, int port,
 	}
 }
 
-static void ksz_get_ethtool_stats(struct dsa_switch *ds, int port,
-				  uint64_t *buf)
-{
-	struct ksz_device *dev = ds->priv;
-	int i;
-	u32 data;
-	int timeout;
-
-	mutex_lock(&dev->stats_mutex);
-
-	for (i = 0; i < TOTAL_SWITCH_COUNTER_NUM; i++) {
-		data = MIB_COUNTER_READ;
-		data |= ((ksz9477_mib_names[i].index & 0xFF) <<
-			MIB_COUNTER_INDEX_S);
-		ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, data);
-
-		timeout = 1000;
-		do {
-			ksz_pread32(dev, port, REG_PORT_MIB_CTRL_STAT__4,
-				    &data);
-			usleep_range(1, 10);
-			if (!(data & MIB_COUNTER_READ))
-				break;
-		} while (timeout-- > 0);
-
-		/* failed to read MIB. get out of loop */
-		if (!timeout) {
-			dev_dbg(dev->dev, "Failed to get MIB\n");
-			break;
-		}
-
-		/* count resets upon read */
-		ksz_pread32(dev, port, REG_PORT_MIB_DATA, &data);
-
-		dev->mib_value[i] += (uint64_t)data;
-		buf[i] = dev->mib_value[i];
-	}
-
-	mutex_unlock(&dev->stats_mutex);
-}
-
 static void ksz9477_cfg_port_member(struct ksz_device *dev, int port,
 				    u8 member)
 {
@@ -1151,9 +1175,14 @@ static int ksz9477_setup(struct dsa_switch *ds)
 	/* queue based egress rate limit */
 	ksz_cfg(dev, REG_SW_MAC_CTRL_5, SW_OUT_RATE_LIMIT_QUEUE_BASED, true);
 
+	/* enable global MIB counter freeze function */
+	ksz_cfg(dev, REG_SW_MAC_CTRL_6, SW_MIB_COUNTER_FREEZE, true);
+
 	/* start switch */
 	ksz_cfg(dev, REG_SW_OPERATION, SW_START, true);
 
+	ksz_init_mib_timer(dev);
+
 	return 0;
 }
 
@@ -1287,6 +1316,7 @@ static int ksz9477_switch_init(struct ksz_device *dev)
 	if (!dev->ports)
 		return -ENOMEM;
 	for (i = 0; i < dev->mib_port_cnt; i++) {
+		mutex_init(&dev->ports[i].mib.cnt_mutex);
 		dev->ports[i].mib.counters =
 			devm_kzalloc(dev->dev,
 				     sizeof(u64) *
@@ -1311,6 +1341,10 @@ static void ksz9477_switch_exit(struct ksz_device *dev)
 	.flush_dyn_mac_table = ksz9477_flush_dyn_mac_table,
 	.phy_setup = ksz9477_phy_setup,
 	.port_setup = ksz9477_port_setup,
+	.r_mib_cnt = ksz9477_r_mib_cnt,
+	.r_mib_pkt = ksz9477_r_mib_pkt,
+	.freeze_mib = ksz9477_freeze_mib,
+	.port_init_cnt = ksz9477_port_init_cnt,
 	.shutdown = ksz9477_reset_switch,
 	.detect = ksz9477_switch_detect,
 	.init = ksz9477_switch_init,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 6f72842..ff32ace 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -40,6 +40,101 @@ void ksz_update_port_member(struct ksz_device *dev, int port)
 }
 EXPORT_SYMBOL_GPL(ksz_update_port_member);
 
+static void port_r_cnt(struct ksz_device *dev, int port)
+{
+	struct ksz_port_mib *mib = &dev->ports[port].mib;
+	u64 *dropped;
+
+	/* Some ports may not have MIB counters before SWITCH_COUNTER_NUM. */
+	while (mib->cnt_ptr < dev->reg_mib_cnt) {
+		dev->dev_ops->r_mib_cnt(dev, port, mib->cnt_ptr,
+					&mib->counters[mib->cnt_ptr]);
+		++mib->cnt_ptr;
+	}
+
+	/* last one in storage */
+	dropped = &mib->counters[dev->mib_cnt];
+
+	/* Some ports may not have MIB counters after SWITCH_COUNTER_NUM. */
+	while (mib->cnt_ptr < dev->mib_cnt) {
+		dev->dev_ops->r_mib_pkt(dev, port, mib->cnt_ptr,
+					dropped, &mib->counters[mib->cnt_ptr]);
+		++mib->cnt_ptr;
+	}
+	mib->cnt_ptr = 0;
+}
+
+static void ksz_mib_read_work(struct work_struct *work)
+{
+	struct ksz_device *dev = container_of(work, struct ksz_device,
+					      mib_read);
+	struct ksz_port_mib *mib;
+	struct ksz_port *p;
+	int i;
+
+	for (i = 0; i < dev->mib_port_cnt; i++) {
+		p = &dev->ports[i];
+
+		/* Only read MIB counters when the port is told to do. */
+		if (!p->read)
+			continue;
+		mib = &p->mib;
+		mutex_lock(&mib->cnt_mutex);
+		port_r_cnt(dev, i);
+		mutex_unlock(&mib->cnt_mutex);
+	}
+}
+
+static void mib_monitor(struct timer_list *t)
+{
+	struct ksz_device *dev = from_timer(dev, t, mib_read_timer);
+	const struct dsa_port *dp;
+	struct net_device *netdev;
+	struct ksz_port_mib *mib;
+	struct ksz_port *p;
+	int i;
+
+	mod_timer(&dev->mib_read_timer, jiffies + dev->mib_read_interval);
+
+	/* Check which port needs to read MIB counters. */
+	for (i = 0; i < dev->mib_port_cnt; i++) {
+		p = &dev->ports[i];
+		if (!p->on)
+			continue;
+		dp = dsa_to_port(dev->ds, i);
+		netdev = dp->slave;
+
+		mib = &p->mib;
+		mutex_lock(&mib->cnt_mutex);
+
+		/* Read only dropped counters when link is not up. */
+		if (netdev && !netif_carrier_ok(netdev))
+			mib->cnt_ptr = dev->reg_mib_cnt;
+		mutex_unlock(&mib->cnt_mutex);
+		p->read = true;
+	}
+	schedule_work(&dev->mib_read);
+}
+
+void ksz_init_mib_timer(struct ksz_device *dev)
+{
+	int i;
+
+	/* Read MIB counters every 30 seconds to avoid overflow. */
+	dev->mib_read_interval = msecs_to_jiffies(30000);
+
+	INIT_WORK(&dev->mib_read, ksz_mib_read_work);
+	timer_setup(&dev->mib_read_timer, mib_monitor, 0);
+
+	for (i = 0; i < dev->mib_port_cnt; i++)
+		dev->dev_ops->port_init_cnt(dev, i);
+
+	/* Start the timer 2 seconds later. */
+	dev->mib_read_timer.expires = jiffies + msecs_to_jiffies(2000);
+	add_timer(&dev->mib_read_timer);
+}
+EXPORT_SYMBOL_GPL(ksz_init_mib_timer);
+
 int ksz_phy_read16(struct dsa_switch *ds, int addr, int reg)
 {
 	struct ksz_device *dev = ds->priv;
@@ -72,6 +167,26 @@ int ksz_sset_count(struct dsa_switch *ds, int port, int sset)
 }
 EXPORT_SYMBOL_GPL(ksz_sset_count);
 
+void ksz_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *buf)
+{
+	const struct dsa_port *dp = dsa_to_port(ds, port);
+	struct ksz_device *dev = ds->priv;
+	struct net_device *netdev;
+	struct ksz_port_mib *mib;
+
+	mib = &dev->ports[port].mib;
+	mutex_lock(&mib->cnt_mutex);
+
+	/* Only read dropped counters if no link. */
+	netdev = dp->slave;
+	if (netdev && !netif_carrier_ok(netdev))
+		mib->cnt_ptr = dev->reg_mib_cnt;
+	port_r_cnt(dev, port);
+	memcpy(buf, mib->counters, dev->mib_cnt * sizeof(u64));
+	mutex_unlock(&mib->cnt_mutex);
+}
+EXPORT_SYMBOL_GPL(ksz_get_ethtool_stats);
+
 int ksz_port_bridge_join(struct dsa_switch *ds, int port,
 			 struct net_device *br)
 {
@@ -339,6 +454,12 @@ int ksz_switch_register(struct ksz_device *dev,
 
 void ksz_switch_remove(struct ksz_device *dev)
 {
+	/* timer started */
+	if (dev->mib_read_timer.expires) {
+		del_timer_sync(&dev->mib_read_timer);
+		flush_work(&dev->mib_read);
+	}
+
 	dev->dev_ops->exit(dev);
 	dsa_unregister_switch(dev->ds);
 
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 2dd832d..0b0ed3d 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -1,19 +1,21 @@
 /* SPDX-License-Identifier: GPL-2.0
  * Microchip switch driver common header
  *
- * Copyright (C) 2017-2018 Microchip Technology Inc.
+ * Copyright (C) 2017-2019 Microchip Technology Inc.
  */
 
 #ifndef __KSZ_COMMON_H
 #define __KSZ_COMMON_H
 
 void ksz_update_port_member(struct ksz_device *dev, int port);
+void ksz_init_mib_timer(struct ksz_device *dev);
 
 /* Common DSA access functions */
 
 int ksz_phy_read16(struct dsa_switch *ds, int addr, int reg);
 int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val);
 int ksz_sset_count(struct dsa_switch *ds, int port, int sset);
+void ksz_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *buf);
 int ksz_port_bridge_join(struct dsa_switch *ds, int port,
 			 struct net_device *br);
 void ksz_port_bridge_leave(struct dsa_switch *ds, int port,
@@ -211,4 +213,24 @@ static void ksz_port_cfg(struct ksz_device *dev, int port, int offset, u8 bits,
 	ksz_write8(dev, addr, data);
 }
 
+/* Modify from readx_poll_timeout in iopoll.h. */
+#define ksz_pread_poll_timeout(op, dev, p, addr, val, cond, sleep_us, \
+			       timeout_us) \
+({ \
+	ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
+	might_sleep_if(sleep_us); \
+	for (;;) { \
+		op(dev, p, addr, &(val)); \
+		if (cond) \
+			break; \
+		if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
+			op(dev, p, addr, &(val)); \
+			break; \
+		} \
+		if (sleep_us) \
+			usleep_range((sleep_us >> 2) + 1, sleep_us); \
+	} \
+	(cond) ? 0 : -ETIMEDOUT; \
+})
+
 #endif
diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h
index 0fdc58b..1d2d98f 100644
--- a/drivers/net/dsa/microchip/ksz_priv.h
+++ b/drivers/net/dsa/microchip/ksz_priv.h
@@ -14,8 +14,6 @@
 #include <linux/etherdevice.h>
 #include <net/dsa.h>
 
-#include "ksz9477_reg.h"
-
 struct ksz_io_ops;
 
 struct vlan_table {
@@ -23,6 +21,7 @@ struct vlan_table {
 };
 
 struct ksz_port_mib {
+	struct mutex cnt_mutex;		/* structure access */
 	u8 cnt_ptr;
 	u64 *counters;
 };
@@ -38,7 +37,8 @@ struct ksz_port {
 	u32 fiber:1;			/* port is fiber */
 	u32 sgmii:1;			/* port is SGMII */
 	u32 force:1;
-	u32 link_just_down:1;		/* link just goes down */
+	u32 read:1;			/* read MIB counters in background */
+	u32 freeze:1;			/* MIB counter freeze is enabled */
 
 	struct ksz_port_mib mib;
 };
@@ -79,8 +79,6 @@ struct ksz_device {
 
 	struct vlan_table *vlan_cache;
 
-	u64 mib_value[TOTAL_SWITCH_COUNTER_NUM];
-
 	u8 *txbuf;
 
 	struct ksz_port *ports;
@@ -153,6 +151,7 @@ struct ksz_dev_ops {
 			  u64 *cnt);
 	void (*r_mib_pkt)(struct ksz_device *dev, int port, u16 addr,
 			  u64 *dropped, u64 *cnt);
+	void (*freeze_mib)(struct ksz_device *dev, int port, bool freeze);
 	void (*port_init_cnt)(struct ksz_device *dev, int port);
 	int (*shutdown)(struct ksz_device *dev);
 	int (*detect)(struct ksz_device *dev);
-- 
1.9.1

[PATCH v3 net-next 3/4] net: dsa: microchip: get port link status

[Tristram.Ha]

From: Tristram Ha <Tristram.Ha@microchip.com>

Get port link status to know whether to read MIB counters when the link
is going down.  Add port_cleanup function to read MIB counters the last
time as when the port is disabled the PHY is also powered down.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c    |  2 ++
 drivers/net/dsa/microchip/ksz_common.c | 39 ++++++++++++++++++++++++++++++++--
 drivers/net/dsa/microchip/ksz_common.h |  3 +++
 drivers/net/dsa/microchip/ksz_priv.h   |  1 +
 4 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index e2d74c7..6ad28e2 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -1191,6 +1191,7 @@ static int ksz9477_setup(struct dsa_switch *ds)
 	.setup			= ksz9477_setup,
 	.phy_read		= ksz9477_phy_read16,
 	.phy_write		= ksz9477_phy_write16,
+	.adjust_link		= ksz_adjust_link,
 	.port_enable		= ksz_enable_port,
 	.port_disable		= ksz_disable_port,
 	.get_strings		= ksz9477_get_strings,
@@ -1340,6 +1341,7 @@ static void ksz9477_switch_exit(struct ksz_device *dev)
 	.cfg_port_member = ksz9477_cfg_port_member,
 	.flush_dyn_mac_table = ksz9477_flush_dyn_mac_table,
 	.phy_setup = ksz9477_phy_setup,
+	.port_cleanup = ksz_port_cleanup,
 	.port_setup = ksz9477_port_setup,
 	.r_mib_cnt = ksz9477_r_mib_cnt,
 	.r_mib_pkt = ksz9477_r_mib_pkt,
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index ff32ace..dd66056 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -20,6 +20,22 @@
 
 #include "ksz_priv.h"
 
+void ksz_port_cleanup(struct ksz_device *dev, int port)
+{
+	/* Read all MIB counters when the link is going down. */
+	if (dev->live_ports & (1 << port)) {
+		struct ksz_port *p = &dev->ports[port];
+
+		p->read = true;
+		schedule_work(&dev->mib_read);
+	}
+
+	/* Common code for port cleanup. */
+	dev->on_ports &= ~(1 << port);
+	dev->live_ports &= ~(1 << port);
+}
+EXPORT_SYMBOL_GPL(ksz_port_cleanup);
+
 void ksz_update_port_member(struct ksz_device *dev, int port)
 {
 	struct ksz_port *p;
@@ -156,6 +172,26 @@ int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val)
 }
 EXPORT_SYMBOL_GPL(ksz_phy_write16);
 
+void ksz_adjust_link(struct dsa_switch *ds, int port,
+		     struct phy_device *phydev)
+{
+	struct ksz_device *dev = ds->priv;
+	struct ksz_port *p = &dev->ports[port];
+
+	if (!phydev->link) {
+		/* Read all MIB counters when the link is going down. */
+		if (dev->live_ports & (1 << port)) {
+			p->read = true;
+			schedule_work(&dev->mib_read);
+		}
+		dev->live_ports &= ~(1 << port);
+	} else {
+		/* Remember which port is connected and active. */
+		dev->live_ports |= (1 << port) & dev->on_ports;
+	}
+}
+EXPORT_SYMBOL_GPL(ksz_adjust_link);
+
 int ksz_sset_count(struct dsa_switch *ds, int port, int sset)
 {
 	struct ksz_device *dev = ds->priv;
@@ -367,8 +403,7 @@ void ksz_disable_port(struct dsa_switch *ds, int port, struct phy_device *phy)
 {
 	struct ksz_device *dev = ds->priv;
 
-	dev->on_ports &= ~(1 << port);
-	dev->live_ports &= ~(1 << port);
+	dev->dev_ops->port_cleanup(dev, port);
 
 	/* port_stp_state_set() will be called after to disable the port so
 	 * there is no need to do anything.
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index 0b0ed3d..ebe6f8e 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -7,6 +7,7 @@
 #ifndef __KSZ_COMMON_H
 #define __KSZ_COMMON_H
 
+void ksz_port_cleanup(struct ksz_device *dev, int port);
 void ksz_update_port_member(struct ksz_device *dev, int port);
 void ksz_init_mib_timer(struct ksz_device *dev);
 
@@ -14,6 +15,8 @@
 
 int ksz_phy_read16(struct dsa_switch *ds, int addr, int reg);
 int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val);
+void ksz_adjust_link(struct dsa_switch *ds, int port,
+		     struct phy_device *phydev);
 int ksz_sset_count(struct dsa_switch *ds, int port, int sset);
 void ksz_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *buf);
 int ksz_port_bridge_join(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/microchip/ksz_priv.h b/drivers/net/dsa/microchip/ksz_priv.h
index 1d2d98f..a1d84c1 100644
--- a/drivers/net/dsa/microchip/ksz_priv.h
+++ b/drivers/net/dsa/microchip/ksz_priv.h
@@ -137,6 +137,7 @@ struct ksz_dev_ops {
 	void (*flush_dyn_mac_table)(struct ksz_device *dev, int port);
 	void (*phy_setup)(struct ksz_device *dev, int port,
 			  struct phy_device *phy);
+	void (*port_cleanup)(struct ksz_device *dev, int port);
 	void (*port_setup)(struct ksz_device *dev, int port, bool cpu_port);
 	void (*r_phy)(struct ksz_device *dev, u16 phy, u16 reg, u16 *val);
 	void (*w_phy)(struct ksz_device *dev, u16 phy, u16 reg, u16 val);
-- 
1.9.1

[PATCH v3 net-next 4/4] net: dsa: microchip: remove unnecessary include headers

[Tristram.Ha]

From: Tristram Ha <Tristram.Ha@microchip.com>

Remove unnecessary header include.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/microchip/ksz9477.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 6ad28e2..a9465c1 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -5,14 +5,10 @@
  * Copyright (C) 2017-2019 Microchip Technology Inc.
  */
 
-#include <linux/delay.h>
-#include <linux/export.h>
-#include <linux/gpio.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/platform_data/microchip-ksz.h>
 #include <linux/phy.h>
-#include <linux/etherdevice.h>
 #include <linux/if_bridge.h>
 #include <net/dsa.h>
 #include <net/switchdev.h>
-- 
1.9.1

Re: [PATCH v3 net-next 1/4] net: dsa: microchip: prepare PHY for proper advertisement

[Andrew Lunn]

On Thu, Feb 21, 2019 at 02:03:14PM -0800, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Prepare PHY for proper advertisement as sometimes the PHY in the switch
> has its own problems even though it may share the PHY id from regular PHY
> but the fixes in the PHY driver do not apply.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 net-next 2/4] net: dsa: microchip: add MIB counter reading support

[Andrew Lunn]

On Thu, Feb 21, 2019 at 02:03:15PM -0800, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Add background MIB counter reading support.
> 
> Port MIB counters should only be read when there is link.  Otherwise it is
> a waste of time as hardware never increases those counters.  There are
> exceptions as some switches keep track of dropped counts no matter what.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 net-next 3/4] net: dsa: microchip: get port link status

[Andrew Lunn]

On Thu, Feb 21, 2019 at 02:03:16PM -0800, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Get port link status to know whether to read MIB counters when the link
> is going down.  Add port_cleanup function to read MIB counters the last
> time as when the port is disabled the PHY is also powered down.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v3 net-next 1/4] net: dsa: microchip: prepare PHY for proper advertisement

[Florian Fainelli]

On 2/21/19 2:03 PM, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Prepare PHY for proper advertisement as sometimes the PHY in the switch
> has its own problems even though it may share the PHY id from regular PHY
> but the fixes in the PHY driver do not apply.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v3 net-next 3/4] net: dsa: microchip: get port link status

[Florian Fainelli]

On 2/21/19 2:03 PM, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Get port link status to know whether to read MIB counters when the link
> is going down.  Add port_cleanup function to read MIB counters the last
> time as when the port is disabled the PHY is also powered down.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> ---

Some comments here and there again, not against a merge, but would be
nice to address in the future.

[snip]

> +void ksz_port_cleanup(struct ksz_device *dev, int port)
> +{
> +	/* Read all MIB counters when the link is going down. */
> +	if (dev->live_ports & (1 << port)) {
> +		struct ksz_port *p = &dev->ports[port];
> +
> +		p->read = true;
> +		schedule_work(&dev->mib_read);
> +	}
> +
> +	/* Common code for port cleanup. */
> +	dev->on_ports &= ~(1 << port);

It seems to be that dev->on_ports is a shorthand for a port is enabled
whether its link is up or down, which is equivalent to using:
!dsa_port_is_unused(ds, port)) unless I am mistaken about the intention
of the code.

In general you seem to be very prone to adding a lot of these
bookkeeping variables which just creates more opportunities for bugs to
creep in.

> +	dev->live_ports &= ~(1 << port);
> +}
> +EXPORT_SYMBOL_GPL(ksz_port_cleanup);
> +
>  void ksz_update_port_member(struct ksz_device *dev, int port)
>  {
>  	struct ksz_port *p;
> @@ -156,6 +172,26 @@ int ksz_phy_write16(struct dsa_switch *ds, int addr, int reg, u16 val)
>  }
>  EXPORT_SYMBOL_GPL(ksz_phy_write16);
>  
> +void ksz_adjust_link(struct dsa_switch *ds, int port,
> +		     struct phy_device *phydev)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_port *p = &dev->ports[port];
> +
> +	if (!phydev->link) {
> +		/* Read all MIB counters when the link is going down. */
> +		if (dev->live_ports & (1 << port)) {
> +			p->read = true;
> +			schedule_work(&dev->mib_read);
> +		}
> +		dev->live_ports &= ~(1 << port);
> +	} else {
> +		/* Remember which port is connected and active. */
> +		dev->live_ports |= (1 << port) & dev->on_ports;
> +	}

How about something simpler than this:

	if (phydev->link != p->old_link) {
		p->read = !!phydev->link;
		schedule_work(&dev->mib_read);
	}

This is also super racy because you schedule the workqueue without any
locking against the state machine which runs with the PHY device's mutex
held, so there is the possibility of the following happening:

Thread 0			Thread 1

ksz_adjust_link()
	phydev->link = 0
	schedule_work()
				ksz_adjust_link()
				p->read = true
	mib_read_workqueue()

and so we don't read counters anymore. It is probably fine since this is
clearly entirely opportunistic and in order to minimize register bandwidth.
-- 
Florian

Re: [PATCH v3 net-next 2/4] net: dsa: microchip: add MIB counter reading support

[Florian Fainelli]

On 2/21/19 2:03 PM, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Add background MIB counter reading support.
> 
> Port MIB counters should only be read when there is link.  Otherwise it is
> a waste of time as hardware never increases those counters.  There are
> exceptions as some switches keep track of dropped counts no matter what.

Some minor comments below, none of them need to be addressed right now,
but it might be a good idea to do it at a later time.

[snip]

> +
> +static void mib_monitor(struct timer_list *t)
> +{
> +	struct ksz_device *dev = from_timer(dev, t, mib_read_timer);
> +	const struct dsa_port *dp;
> +	struct net_device *netdev;
> +	struct ksz_port_mib *mib;
> +	struct ksz_port *p;
> +	int i;
> +
> +	mod_timer(&dev->mib_read_timer, jiffies + dev->mib_read_interval);
> +
> +	/* Check which port needs to read MIB counters. */
> +	for (i = 0; i < dev->mib_port_cnt; i++) {
> +		p = &dev->ports[i];
> +		if (!p->on)
> +			continue;

Would not using dsa_port_is_unused() accomplish the same thing here?
Your setup function should not have enabled non-existent/connected to
the outside world ports anyway.

> +		dp = dsa_to_port(dev->ds, i);
> +		netdev = dp->slave;
> +
> +		mib = &p->mib;
> +		mutex_lock(&mib->cnt_mutex);

Have you built this with CONFIG_DEBUG_SLEEP_ATOMIC? timer executes in BH
context (atomic), you cannot hold a mutex here which would be allowed to
sleep.

> +
> +		/* Read only dropped counters when link is not up. */
> +		if (netdev && !netif_carrier_ok(netdev))> +			mib->cnt_ptr = dev->reg_mib_cnt;
> +		mutex_unlock(&mib->cnt_mutex);
> +		p->read = true;
> +	}
> +	schedule_work(&dev->mib_read);
> +}

[snip]

> +
>  int ksz_phy_read16(struct dsa_switch *ds, int addr, int reg)
>  {
>  	struct ksz_device *dev = ds->priv;
> @@ -72,6 +167,26 @@ int ksz_sset_count(struct dsa_switch *ds, int port, int sset)
>  }
>  EXPORT_SYMBOL_GPL(ksz_sset_count);
>  
> +void ksz_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *buf)
> +{
> +	const struct dsa_port *dp = dsa_to_port(ds, port);
> +	struct ksz_device *dev = ds->priv;
> +	struct net_device *netdev;
> +	struct ksz_port_mib *mib;
> +
> +	mib = &dev->ports[port].mib;
> +	mutex_lock(&mib->cnt_mutex);
> +
> +	/* Only read dropped counters if no link. */
> +	netdev = dp->slave;
> +	if (netdev && !netif_carrier_ok(netdev))
> +		mib->cnt_ptr = dev->reg_mib_cnt;

The netdev is guaranteed to exist, otherwise you would not be in this
function, and we are executing with RTNL held, so the device can't
vanish under your feet.

[snip]
>  
> +/* Modify from readx_poll_timeout in iopoll.h. */
> +#define ksz_pread_poll_timeout(op, dev, p, addr, val, cond, sleep_us, \
> +			       timeout_us) \
> +({ \
> +	ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
> +	might_sleep_if(sleep_us); \
> +	for (;;) { \
> +		op(dev, p, addr, &(val)); \

I don't think you need to create your own custom macro, which you seem
to need such that you can  pass dev and p as arguments, instead you can
create a specific helper function, pass it down to op() as the "addr"
argument, since that is a macro that does not type checking at all. So
something like this:

struct ksz_read_ctx {
	struct ksz_device *dev;
	struct ksz_port *p;
	u16 reg;
};

static int ksz_pread32_poll(struct ksz_read_ctx *ctx)
{
	return ksz_pread32(ctx->dev, ctx->p, ctx->reg);
}

static void ksz9477_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
			      u64 *cnt)
{
	struct ksz_port *p = &dev->ports[port];
	struct ksz_read_ctx ctx = {
		.dev = dev,
		.p = &dev->ports[port],
		.reg = REG_PORT_MIB_CTRL_STAT__4
	};
	u32 data;
	int ret;

	/* retain the flush/freeze bit */
	data = p->freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;
	data |= MIB_COUNTER_READ;
	data |= (addr << MIB_COUNTER_INDEX_S);
	ksz_pwrite32(dev, port, REG_PORT_MIB_CTRL_STAT__4, data);

	ret = readx_poll_timeout(ksz_pread32_poll, &ctx, data, !(data &
MIB_COUNTER_READ), 10, 1000);

Completely not compile tested, but you get the idea.
-- 
Florian