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
