diff mbox series

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

Message ID 1550786597-591-3-git-send-email-Tristram.Ha@microchip.com
State Superseded
Delegated to: David Miller
Headers show
Series net: dsa: microchip: add MIB counters support | expand

Commit Message

Tristram.Ha@microchip.com Feb. 21, 2019, 10:03 p.m. UTC
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(-)

Comments

Andrew Lunn Feb. 21, 2019, 10:19 p.m. UTC | #1
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
Florian Fainelli Feb. 21, 2019, 11:01 p.m. UTC | #2
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.
diff mbox series

Patch

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);