[v1,net-next,2/4] net: dsa: microchip: add MIB counter reading support
diff mbox series

Message ID 1549598829-25970-3-git-send-email-Tristram.Ha@microchip.com
State Changes Requested
Delegated to: David Miller
Headers show
Series
  • net: dsa: microchip: add MIB counters support
Related show

Commit Message

Tristram.Ha@microchip.com Feb. 8, 2019, 4:07 a.m. UTC
From: Tristram Ha <Tristram.Ha@microchip.com>

Add MIB counter reading support.

Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
---
 drivers/net/dsa/microchip/ksz9477.c    | 139 +++++++++++++++++++++++----------
 drivers/net/dsa/microchip/ksz_common.c |  96 +++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h |   2 +
 drivers/net/dsa/microchip/ksz_priv.h   |   7 +-
 4 files changed, 198 insertions(+), 46 deletions(-)

Comments

Andrew Lunn Feb. 9, 2019, 5:22 p.m. UTC | #1
On Thu, Feb 07, 2019 at 08:07:07PM -0800, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Add MIB counter reading support.
> 
> Signed-off-by: Tristram Ha <Tristram.Ha@microchip.com>
> ---
>  drivers/net/dsa/microchip/ksz9477.c    | 139 +++++++++++++++++++++++----------
>  drivers/net/dsa/microchip/ksz_common.c |  96 +++++++++++++++++++++++
>  drivers/net/dsa/microchip/ksz_common.h |   2 +
>  drivers/net/dsa/microchip/ksz_priv.h   |   7 +-
>  4 files changed, 198 insertions(+), 46 deletions(-)
> 
> diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> index 0fdb22d..4502e13 100644
> --- a/drivers/net/dsa/microchip/ksz9477.c
> +++ b/drivers/net/dsa/microchip/ksz9477.c
> @@ -10,6 +10,7 @@
>  #include <linux/gpio.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> +#include <linux/iopoll.h>
>  #include <linux/platform_data/microchip-ksz.h>
>  #include <linux/phy.h>
>  #include <linux/etherdevice.h>
> @@ -18,8 +19,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;
> @@ -92,6 +93,27 @@ static void ksz9477_port_cfg32(struct ksz_device *dev, int port, int offset,
>  	ksz_write32(dev, addr, data);
>  }
>  
> +#define read8_op(addr)	\
> +({ \
> +	u8 data; \
> +	ksz_read8(dev, addr, &data); \
> +	data; \
> +})
> +
> +#define read32_op(addr)	\
> +({ \
> +	u32 data; \
> +	ksz_read32(dev, addr, &data); \
> +	data; \
> +})

These two are not used. Please remove them.

> +
> +#define pread32_op(addr)	\
> +({ \
> +	u32 data; \
> +	ksz_pread32(dev, port, addr, &data); \
> +	data; \
> +})


It works, but it is not nice, and it makes assumptions about how
readx_poll_timeout is implemented.

> +	ret = readx_poll_timeout(pread32_op, REG_PORT_MIB_CTRL_STAT__4, data,
> +				 !(data & MIB_COUNTER_READ), 10, 1000);

The macro is only used one, and addr is fixed,
REG_PORT_MIB_CTRL_STAT__4. So you can at least replace addr with port,
and rename the macro pread32_stat(port).


> +	/* 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)
> +{
> +	struct ksz_port *p = &dev->ports[port];
> +	u32 val = freeze ? MIB_COUNTER_FLUSH_FREEZE : 0;

Reverse Christmas tree.

> +
> +	/* 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 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;

This link_just_down stuff is not clear at all. Why can the drop
counters not be read when the link is up?

> +		port_r_cnt(dev, i);
> +		mutex_unlock(&mib->cnt_mutex);
> +	}
> +}
Tristram.Ha@microchip.com Feb. 13, 2019, 2:39 a.m. UTC | #2
> > +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;
> 
> Reverse Christmas tree.

There was a checkpatch.pl patch in 2016 that tried to check this, but it was never accepted?
 
> > +		/* 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;
> 
> This link_just_down stuff is not clear at all. Why can the drop
> counters not be read when the link is up?

All of the MIB counters, except some that may be marked by driver, do not get updated when the link is down, so it is a waste of time to read them.

My intention is the driver eventually reads the MIB counters at least every second or faster so that the ethtool API called to show MIB counters gets them from memory rather than starting a read operation.  For now that API is called from user space with the ethtool utility, so it may not be called too often and too fast.  But theoretically that API can be called from a program continually.

For simple switches that do not need to do anything special the MIB read operation does not cause any issue except CPU load, for more complicate switches that need to do some background operations too many read operation can affect some critical functions.
Andrew Lunn Feb. 13, 2019, 3:28 a.m. UTC | #3
> All of the MIB counters, except some that may be marked by driver,
> do not get updated when the link is down, so it is a waste of time
> to read them.

Hi Tristram

O.K, so make this clear in the code. Maybe rather than having this
link_just_down, have the adjust link callback update the cached
values for all counters? 

> My intention is the driver eventually reads the MIB counters at
> least every second or faster so that the ethtool API called to show
> MIB counters gets them from memory rather than starting a read
> operation.

The user expects to see the current counters, not some cached values.
For me it is O.K. to frequently read the counters to prevent wrap
around, but each ethtool call should update the counters before
returning them to user space.

> For simple switches that do not need to do anything special the MIB
> read operation does not cause any issue except CPU load, for more
> complicate switches that need to do some background operations too
> many read operation can affect some critical functions.

Sounds like a bad design of the switch, if reading statistics from it
can upset its operation. You might want to consider rate limiting the
ethtool call.

	Andrew
Florian Fainelli Feb. 13, 2019, 3:51 a.m. UTC | #4
On February 12, 2019 6:39:49 PM PST, Tristram.Ha@microchip.com wrote:
>> > +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;
>> 
>> Reverse Christmas tree.
>
>There was a checkpatch.pl patch in 2016 that tried to check this, but
>it was never accepted?
> 
>> > +		/* 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;
>> 
>> This link_just_down stuff is not clear at all. Why can the drop
>> counters not be read when the link is up?
>
>All of the MIB counters, except some that may be marked by driver, do
>not get updated when the link is down, so it is a waste of time to read
>them.

Can you use netif_running() to determine that condition? Maintaining your own set of variables when the PHY state machine should already determine the link state sounds redundant if not error prone.

>
>My intention is the driver eventually reads the MIB counters at least
>every second or faster so that the ethtool API called to show MIB
>counters gets them from memory rather than starting a read operation. 
>For now that API is called from user space with the ethtool utility, so
>it may not be called too often and too fast.  But theoretically that
>API can be called from a program continually.
>
>For simple switches that do not need to do anything special the MIB
>read operation does not cause any issue except CPU load, for more
>complicate switches that need to do some background operations too many
>read operation can affect some critical functions.

Some switches have a MIB autocast feature taking a snapshot which AFAIR is internally implemented as a fast read register with no contention on other registers internally, do you have something similar?
Tristram.Ha@microchip.com Feb. 14, 2019, 7:26 p.m. UTC | #5
> >> > +		/* 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;
> >>
> >> This link_just_down stuff is not clear at all. Why can the drop
> >> counters not be read when the link is up?
> >
> >All of the MIB counters, except some that may be marked by driver, do
> >not get updated when the link is down, so it is a waste of time to read
> >them.
> 
> Can you use netif_running() to determine that condition? Maintaining your
> own set of variables when the PHY state machine should already determine
> the link state sounds redundant if not error prone.
> 

The driver can store the PHY device pointer passed to it when the port is enabled.  But I am a little worried that pointer can be changed or completely gone as it is out of control of the driver.

> >My intention is the driver eventually reads the MIB counters at least
> >every second or faster so that the ethtool API called to show MIB
> >counters gets them from memory rather than starting a read operation.
> >For now that API is called from user space with the ethtool utility, so
> >it may not be called too often and too fast.  But theoretically that
> >API can be called from a program continually.
> >
> >For simple switches that do not need to do anything special the MIB
> >read operation does not cause any issue except CPU load, for more
> >complicate switches that need to do some background operations too many
> >read operation can affect some critical functions.
> 
> Some switches have a MIB autocast feature taking a snapshot which AFAIR is
> internally implemented as a fast read register with no contention on other
> registers internally, do you have something similar?

There is no such function in the switch.  Every MIB counter read has to go through a single SPI transfer using indirect access.  There are no table-like stored values that a single SPI transfer can retrieve all.

For i2C the access is even slower, but then I do not expect this access mechanism is used when the switch can do more complex things.
Florian Fainelli Feb. 14, 2019, 7:33 p.m. UTC | #6
On 2/14/19 11:26 AM, Tristram.Ha@microchip.com wrote:
>>>>> +		/* 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;
>>>>
>>>> This link_just_down stuff is not clear at all. Why can the drop
>>>> counters not be read when the link is up?
>>>
>>> All of the MIB counters, except some that may be marked by driver, do
>>> not get updated when the link is down, so it is a waste of time to read
>>> them.
>>
>> Can you use netif_running() to determine that condition? Maintaining your
>> own set of variables when the PHY state machine should already determine
>> the link state sounds redundant if not error prone.
>>
> 
> The driver can store the PHY device pointer passed to it when the port is enabled.  But I am a little worried that pointer can be changed or completely gone as it is out of control of the driver.

The per-port network device is accessible from dp->slave so you can do
netif_running(dp->slave) from your driver, what are you talking about here?

> 
>>> My intention is the driver eventually reads the MIB counters at least
>>> every second or faster so that the ethtool API called to show MIB
>>> counters gets them from memory rather than starting a read operation.
>>> For now that API is called from user space with the ethtool utility, so
>>> it may not be called too often and too fast.  But theoretically that
>>> API can be called from a program continually.
>>>
>>> For simple switches that do not need to do anything special the MIB
>>> read operation does not cause any issue except CPU load, for more
>>> complicate switches that need to do some background operations too many
>>> read operation can affect some critical functions.
>>
>> Some switches have a MIB autocast feature taking a snapshot which AFAIR is
>> internally implemented as a fast read register with no contention on other
>> registers internally, do you have something similar?
> 
> There is no such function in the switch.  Every MIB counter read has to go through a single SPI transfer using indirect access.  There are no table-like stored values that a single SPI transfer can retrieve all.
> 
> For i2C the access is even slower, but then I do not expect this access mechanism is used when the switch can do more complex things.
> 

Is that something you are considering to change for future designs?
Joe Perches Feb. 20, 2019, 11:49 p.m. UTC | #7
On Tue, 2019-02-12 at 19:51 -0800, Florian Fainelli wrote:
> On February 12, 2019 6:39:49 PM PST, Tristram.Ha@microchip.com wrote:
> > > > +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;
> > > 
> > > Reverse Christmas tree.
> > 
> > There was a checkpatch.pl patch in 2016 that tried to check this, but
> > it was never accepted?

https://lore.kernel.org/patchwork/patch/732076/

While I still think use of reverse christmas tree is misguided,
there were some disagreements about what form it really is:

Is this reverse christmas tree?

   void foo(void)
   {
   	int	aa;
   	int	a = 1;

or is

   void foo(void)
   {
   	int	a = 1;
   	int	aa;

IMHO: neither really helps to visually find or scan for
automatics so the whole concept isn't particularly useful.
Pavel Machek Feb. 25, 2019, 6:34 p.m. UTC | #8
On Wed 2019-02-20 15:49:25, Joe Perches wrote:
> On Tue, 2019-02-12 at 19:51 -0800, Florian Fainelli wrote:
> > On February 12, 2019 6:39:49 PM PST, Tristram.Ha@microchip.com wrote:
> > > > > +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;
> > > > 
> > > > Reverse Christmas tree.
> > > 
> > > There was a checkpatch.pl patch in 2016 that tried to check this, but
> > > it was never accepted?
> 
> https://lore.kernel.org/patchwork/patch/732076/
> 
> While I still think use of reverse christmas tree is misguided,
> there were some disagreements about what form it really is:

Agreed about "misguided".

Often there's more logical order, like "self" pointer first... which
this patch follows.
									Pavel

Patch
diff mbox series

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 0fdb22d..4502e13 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -10,6 +10,7 @@ 
 #include <linux/gpio.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/iopoll.h>
 #include <linux/platform_data/microchip-ksz.h>
 #include <linux/phy.h>
 #include <linux/etherdevice.h>
@@ -18,8 +19,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;
@@ -92,6 +93,27 @@  static void ksz9477_port_cfg32(struct ksz_device *dev, int port, int offset,
 	ksz_write32(dev, addr, data);
 }
 
+#define read8_op(addr)	\
+({ \
+	u8 data; \
+	ksz_read8(dev, addr, &data); \
+	data; \
+})
+
+#define read32_op(addr)	\
+({ \
+	u32 data; \
+	ksz_read32(dev, addr, &data); \
+	data; \
+})
+
+#define pread32_op(addr)	\
+({ \
+	u32 data; \
+	ksz_pread32(dev, port, addr, &data); \
+	data; \
+})
+
 static int ksz9477_wait_vlan_ctrl_ready(struct ksz_device *dev, u32 waiton,
 					int timeout)
 {
@@ -259,6 +281,70 @@  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 ret;
+	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);
+
+	ret = readx_poll_timeout(pread32_op, 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)
+{
+	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 +428,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)
 {
@@ -1153,9 +1198,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;
 }
 
@@ -1290,6 +1340,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) *
@@ -1314,6 +1365,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 a57bda7..62d4344 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,20 @@  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;
+
+	mutex_lock(&mib->cnt_mutex);
+	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)
 {
@@ -355,6 +445,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 6d49319..f5f5fd8 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 0fdc58b..a35bb34 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;
 };
@@ -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);