netdev - Re: [PATCH] net: mvpp2: add ethtool GOP statistics

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20171103094014.387f2475@xps13>
Date:   Fri, 3 Nov 2017 09:40:14 +0100
From:   Miquel RAYNAL <miquel.raynal@...e-electrons.com>
To:     Andrew Lunn <andrew@...n.ch>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Thomas Petazzoni <thomas.petazzoni@...e-electrons.com>,
        Antoine Tenart <antoine.tenart@...e-electrons.com>,
        Gregory Clement <gregory.clement@...e-electrons.com>,
        Nadav Haklai <nadavh@...vell.com>, netdev@...r.kernel.org,
        Stefan Chulski <stefanc@...vell.com>
Subject: Re: [PATCH] net: mvpp2: add ethtool GOP statistics

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