Re: [PATCH v2 net-next] net: dsa: microchip: Add MIB counter reading support

[Andrew Lunn <andrew@lunn.ch>]

On Thu, Nov 16, 2017 at 06:42:50PM -0800, Tristram.Ha@microchip.com wrote:
> From: Tristram Ha <Tristram.Ha@microchip.com>
> 
> Add MIB counter reading support.

Hi Tristram

Some more details would be good here.

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

dropped is unused here. Which seems to make dropped in general unused.

> +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_down)
> +			p->link_down = 0;

It is not obvious to me what this is doing.

> +		else if (!p->link_up)
> +			mib->cnt_ptr = dev->reg_mib_cnt;
> +		port_r_cnt(dev, i);
> +		mutex_unlock(&mib->cnt_mutex);
> +	}
> +}
> +
> +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));

Should the memcpy be made while holding the mutex? As soon as you
release it, the timer code can start updating the values in
mib->counters. That then makes the freeze pointless.

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

Is this race free?

   Andrew