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

[Miquel RAYNAL <miquel.raynal@free-electrons.com>]

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