diff mbox series

[RFC,2/6] net: dsa: microchip: Add MIB counter reading support

Message ID 1543880097-7106-3-git-send-email-Tristram.Ha@microchip.com
State RFC, archived
Delegated to: David Miller
Headers show
Series net: dsa: microchip: Modify KSZ9477 DSA driver to support different tail tag formats | expand

Commit Message

Tristram.Ha@microchip.com Dec. 3, 2018, 11:34 p.m. UTC 
From: Tristram Ha <Tristram.Ha@microchip.com>

Add MIB counter reading support.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
Reviewed-by: Woojung Huh <Woojung.Huh@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c    | 121 ++++++++++++++++++++++-----------
 drivers/net/dsa/microchip/ksz_common.c | 101 +++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h |   2 +
 drivers/net/dsa/microchip/ksz_priv.h   |   7 +-
 4 files changed, 186 insertions(+), 45 deletions(-)

Comments

Andrew Lunn Dec. 5, 2018, 5:53 p.m. UTC | #1 
Hi Tristan

> +static void ksz9477_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
> +			      u64 *cnt)
> +{
> +	u32 data;
> +	int timeout;
> +	struct ksz_port *p = &dev->ports[port];
> +
> +	/* 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);
> +
> +	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);

Could you use readx_poll_timeout() here?

> +void ksz_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *buf)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_port_mib *mib;
> +
> +	mib = &dev->ports[port].mib;
> +
> +	/* freeze MIB counters if supported */
> +	if (dev->dev_ops->freeze_mib)
> +		dev->dev_ops->freeze_mib(dev, port, true);
> +	mutex_lock(&mib->cnt_mutex);
> +	port_r_cnt(dev, port);
> +	mutex_unlock(&mib->cnt_mutex);
> +	if (dev->dev_ops->freeze_mib)
> +		dev->dev_ops->freeze_mib(dev, port, false);

Should the freeze be protected by the mutex as well?

> +	memcpy(buf, mib->counters, dev->mib_cnt * sizeof(u64));

I wonder if this memcpy should also be protected by the mutex. As soon
as the mutex is dropped, the scheduled work could start updating
mib->counters in non-atomic ways?

> +}
> +
>  int ksz_port_bridge_join(struct dsa_switch *ds, int port,
>  			 struct net_device *br)
>  {
> @@ -255,6 +349,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);
> +	dev->dev_ops->port_init_cnt(dev, port);

This is probably not the correct place to do this. MIB counters should
not be cleared by an ifdown/ifup cycle. They should only be cleared
when the driver is probed.
Tristram.Ha@microchip.com Dec. 6, 2018, 8:16 p.m. UTC | #2 
> > +static void ksz9477_r_mib_cnt(struct ksz_device *dev, int port, u16 addr,
> > +			      u64 *cnt)
> > +{
> > +	u32 data;
> > +	int timeout;
> > +	struct ksz_port *p = &dev->ports[port];
> > +
> > +	/* 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);
> > +
> > +	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);
> 
> Could you use readx_poll_timeout() here?
> 
> > +void ksz_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *buf)
> > +{
> > +	struct ksz_device *dev = ds->priv;
> > +	struct ksz_port_mib *mib;
> > +
> > +	mib = &dev->ports[port].mib;
> > +
> > +	/* freeze MIB counters if supported */
> > +	if (dev->dev_ops->freeze_mib)
> > +		dev->dev_ops->freeze_mib(dev, port, true);
> > +	mutex_lock(&mib->cnt_mutex);
> > +	port_r_cnt(dev, port);
> > +	mutex_unlock(&mib->cnt_mutex);
> > +	if (dev->dev_ops->freeze_mib)
> > +		dev->dev_ops->freeze_mib(dev, port, false);
> 
> Should the freeze be protected by the mutex as well?
> 
> > +	memcpy(buf, mib->counters, dev->mib_cnt * sizeof(u64));
> 
> I wonder if this memcpy should also be protected by the mutex. As soon
> as the mutex is dropped, the scheduled work could start updating
> mib->counters in non-atomic ways?
>

I will update as suggested.
 
> > +}
> > +
> >  int ksz_port_bridge_join(struct dsa_switch *ds, int port,
> >  			 struct net_device *br)
> >  {
> > @@ -255,6 +349,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);
> > +	dev->dev_ops->port_init_cnt(dev, port);
> 
> This is probably not the correct place to do this. MIB counters should
> not be cleared by an ifdown/ifup cycle. They should only be cleared
> when the driver is probed.

I wonder what is the official way to clear the counters.  For network debugging
It is good to clear the counters to start fresh to see which frame is not
being sent or received.  Typically the device is reset when it is shutdown as there
are hardware problems.  I would think it is the job of applications like SNMP Manager
to keep track of MIB counters throughout the life of a running system.
Andrew Lunn Dec. 6, 2018, 8:42 p.m. UTC | #3 
> I wonder what is the official way to clear the counters.

I don't think there is one, other than unloading the driver and
loading it again.

	Andrew
diff mbox series

Patch

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 98aa25e..bd1ca33 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -259,6 +259,76 @@  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)
+{
+	u32 data;
+	int timeout;
+	struct ksz_port *p = &dev->ports[port];
+
+	/* 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);
+
+	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");
+		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)
+{
+	struct ksz_port *p = &dev->ports[port];
+	u32 val = freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;
+
+	/* 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 +412,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)
 {
@@ -1150,9 +1179,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 +1321,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 +1346,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 39adc57..7b8f57b 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -40,6 +40,82 @@  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 *p;
+	struct ksz_port_mib *mib;
+	int i;
+
+	for (i = 0; i < dev->mib_port_cnt; i++) {
+		p = &dev->ports[i];
+		if (!p->on)
+			continue;
+		mib = &p->mib;
+		mutex_lock(&mib->cnt_mutex);
+
+		/* read only dropped counters when link is not up */
+		if (p->link_just_down)
+			p->link_just_down = 0;
+		else if (!p->phydev.link)
+			mib->cnt_ptr = dev->reg_mib_cnt;
+		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);
+
+	mod_timer(&dev->mib_read_timer, jiffies + dev->mib_read_interval);
+	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;
@@ -88,6 +164,24 @@  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)
+{
+	struct ksz_device *dev = ds->priv;
+	struct ksz_port_mib *mib;
+
+	mib = &dev->ports[port].mib;
+
+	/* freeze MIB counters if supported */
+	if (dev->dev_ops->freeze_mib)
+		dev->dev_ops->freeze_mib(dev, port, true);
+	mutex_lock(&mib->cnt_mutex);
+	port_r_cnt(dev, port);
+	mutex_unlock(&mib->cnt_mutex);
+	if (dev->dev_ops->freeze_mib)
+		dev->dev_ops->freeze_mib(dev, port, false);
+	memcpy(buf, mib->counters, dev->mib_cnt * sizeof(u64));
+}
+
 int ksz_port_bridge_join(struct dsa_switch *ds, int port,
 			 struct net_device *br)
 {
@@ -255,6 +349,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);
+	dev->dev_ops->port_init_cnt(dev, port);
 
 	/* port_stp_state_set() will be called after to enable the port so
 	 * there is no need to do anything.
@@ -344,6 +439,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 206b313..547859e 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -8,6 +8,7 @@ 
 #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 */
 
@@ -16,6 +17,7 @@ 
 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,
 			 struct net_device *br);
 void ksz_port_bridge_leave(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 fcb75a8..1955ea6 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;
 };
@@ -39,6 +38,7 @@  struct ksz_port {
 	u32 sgmii:1;			/* port is SGMII */
 	u32 force:1;
 	u32 link_just_down:1;		/* link just goes down */
+	u32 freeze:1;			/* MIB counter freeze is enabled */
 
 	struct ksz_port_mib mib;
 };
@@ -77,8 +77,6 @@  struct ksz_device {
 
 	struct vlan_table *vlan_cache;
 
-	u64 mib_value[TOTAL_SWITCH_COUNTER_NUM];
-
 	u8 *txbuf;
 
 	struct ksz_port *ports;
@@ -151,6 +149,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);