From mboxrd@z Thu Jan  1 00:00:00 1970
From: Miquel RAYNAL <miquel.raynal@free-electrons.com>
Subject: Re: [PATCH] net: mvpp2: add ethtool GOP statistics
Date: Fri, 3 Nov 2017 09:40:14 +0100
Message-ID: <20171103094014.387f2475@xps13>
References: <20171102185239.22093-1-miquel.raynal@free-electrons.com>
        <20171102195827.GM24320@lunn.ch>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8BIT
Cc: "David S . Miller" <davem@davemloft.net>,
        Thomas Petazzoni <thomas.petazzoni@free-electrons.com>,
        Antoine Tenart <antoine.tenart@free-electrons.com>,
        Gregory Clement <gregory.clement@free-electrons.com>,
        Nadav Haklai <nadavh@marvell.com>, netdev@vger.kernel.org,
        Stefan Chulski <stefanc@marvell.com>
To: Andrew Lunn <andrew@lunn.ch>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail.free-electrons.com ([62.4.15.54]:54502 "EHLO
        mail.free-electrons.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1752465AbdKCIk0 (ORCPT
        <rfc822;netdev@vger.kernel.org>); Fri, 3 Nov 2017 04:40:26 -0400
In-Reply-To: <20171102195827.GM24320@lunn.ch>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

Hi Andrew,

Thanks for the review, I forgot to mention this is for net-next, I'll
fix the subject line when sending the v2.

> > +static struct mvpp2_ethtool_statistics mvpp2_ethtool_stats[] = {  
> 
> This can probably be const, and save a few bytes of RAM.

Absolutely.

> 
> > +	{ MVPP2_MIB_GOOD_OCTETS_RCVD_LOW, "good_octets_received" },
> > +	{ MVPP2_MIB_BAD_OCTETS_RCVD, "bad_octets_received" },
> > +	{ MVPP2_MIB_CRC_ERRORS_SENT, "crc_errors_sent" },
> > +	{ MVPP2_MIB_UNICAST_FRAMES_RCVD,
> > "unicast_frames_received" },
> > +	{ MVPP2_MIB_BROADCAST_FRAMES_RCVD,
> > "broadcast_frames_received" },
> > +	{ MVPP2_MIB_MULTICAST_FRAMES_RCVD,
> > "multicast_frames_received" },
> > +	{ MVPP2_MIB_FRAMES_64_OCTETS, "frames_64_octets" },
> > +	{ MVPP2_MIB_FRAMES_65_TO_127_OCTETS,
> > "frames_65_to_127_octet" },
> > +	{ MVPP2_MIB_FRAMES_128_TO_255_OCTETS,

...

> > +static void mvpp2_ethtool_get_stats(struct net_device *dev,
> > +				    struct ethtool_stats *stats,
> > u64 *data) +{
> > +	struct mvpp2_port *port = netdev_priv(dev);
> > +
> > +	/* Update statistics for all ports, copy only those
> > actually needed */
> > +	mvpp2_gather_hw_statistics(&port->priv->stats_work.work);  
> 
> Shouldn't there be some locking here? What if
> mvpp2_gather_hw_statistic is already running?

You are right, locking is needed when accessing the registers. I added
mutexes, please have a look in the v2 regarding their implementation as
I am not very familiar with them.

> 
> > @@ -7613,13 +7788,19 @@ static int mvpp2_port_probe(struct
> > platform_device *pdev, port->base = priv->iface_base +
> > MVPP22_GMAC_BASE(port->gop_id); }
> >  
> > -	/* Alloc per-cpu stats */
> > +	/* Alloc per-cpu and ethtool stats */
> >  	port->stats = netdev_alloc_pcpu_stats(struct
> > mvpp2_pcpu_stats); if (!port->stats) {
> >  		err = -ENOMEM;
> >  		goto err_free_irq;
> >  	}
> >  
> > +	port->ethtool_stats = kzalloc(sizeof(mvpp2_ethtool_stats),
> > GFP_KERNEL);  
> 
> devm_ to make the cleanup simpler?

Ok.

> 
> > +	/* This work recall himself within a delay. If the
> > cancellation returned
> > +	 * a non-zero value, it means a work is still running. In
> > that case, use
> > +	 * use the flush (returns when the running work will be
> > done) and cancel  
> 
> One use is enough.
> 
> > +	 * the new work that was just submitted to the queue but
> > not started yet
> > +	 * due to the delay.
> > +	 */
> > +	if (!cancel_delayed_work(&priv->stats_work)) {
> > +		flush_workqueue(priv->stats_queue);
> > +		cancel_delayed_work(&priv->stats_work);
> > +	}  
> 
> Why is cancel_delayed_work_sync() not enough?

I did not knew about the *_sync() version, thanks for pointing it.


Thank you,
Miquèl