From mboxrd@z Thu Jan  1 00:00:00 1970
From: Andrew Lunn <andrew@lunn.ch>
Subject: Re: [PATCH v2 net-next] net: dsa: microchip: Add MIB counter reading
 support
Date: Fri, 17 Nov 2017 05:18:56 +0100
Message-ID: <20171117041856.GE27450@lunn.ch>
References: <1510886570-5546-1-git-send-email-Tristram.Ha@microchip.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Cc: Florian Fainelli <f.fainelli@gmail.com>,
        Pavel Machek <pavel@ucw.cz>, muvarov@gmail.com,
        nathan.leigh.conrad@gmail.com, vivien.didelot@savoirfairelinux.com,
        UNGLinuxDriver@microchip.com, netdev@vger.kernel.org
To: Tristram.Ha@microchip.com
Return-path: <netdev-owner@vger.kernel.org>
Received: from vps0.lunn.ch ([185.16.172.187]:58171 "EHLO vps0.lunn.ch"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S932634AbdKQETB (ORCPT <rfc822;netdev@vger.kernel.org>);
        Thu, 16 Nov 2017 23:19:01 -0500
Content-Disposition: inline
In-Reply-To: <1510886570-5546-1-git-send-email-Tristram.Ha@microchip.com>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

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