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

[Andrew Lunn <andrew@lunn.ch>]

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