> @@ -817,6 +856,12 @@ struct mvpp2 {
>
> /* Maximum number of RXQs per port */
> unsigned int max_port_rxqs;
> +
> + /* Workqueue to gather hardware statistics with its lock */
> + struct mutex gather_stats_lock;
> + struct delayed_work stats_work;
> + char queue_name[20];
> + struct workqueue_struct *stats_queue;
> };
> +static u64 mvpp2_read_count(struct mvpp2_port *port,
> + const struct mvpp2_ethtool_counter *counter)
> +{
> + void __iomem *base;
> + u64 val;
> +
> + if (port->priv->hw_version == MVPP21)
> + base = port->priv->lms_base + MVPP21_MIB_COUNTERS_OFFSET +
> + port->gop_id * MVPP21_MIB_COUNTERS_PORT_SZ;
> + else
> + base = port->priv->iface_base + MVPP22_MIB_COUNTERS_OFFSET +
> + port->gop_id * MVPP22_MIB_COUNTERS_PORT_SZ;
This seems like something which could be calculated once and then
stored away, e.g. next to stats_queue.
> +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);
> +
> + memcpy(data, port->ethtool_stats,
> + sizeof(u64) * ARRAY_SIZE(mvpp2_ethtool_regs));
Thanks for adding the mutex in mvpp2_gather_hw_statistics(). However,
should we be holding the mutex while performing this copy? There is no
snapshot support, so the statistics are not guaranteed to be
consistent. However, since a statistics is a u64, it is possible the
copy will get the new lower 32 bits and the old 32 bits if the copy
happens while mvpp2_gather_hw_statistics() is running.
> + port->ethtool_stats = devm_kcalloc(&pdev->dev,
> + ARRAY_SIZE(mvpp2_ethtool_regs),
> + sizeof(u64), GFP_KERNEL);
> + if (!port->ethtool_stats) {
> + err = -ENOMEM;
> + goto err_free_stats;
> + }
> +
> mvpp2_port_copy_mac_addr(dev, priv, port_node, &mac_from);
>
> port->tx_ring_size = MVPP2_MAX_TXD;
> @@ -7707,6 +7904,7 @@ static void mvpp2_port_remove(struct mvpp2_port *port)
> of_node_put(port->phy_node);
> free_percpu(port->pcpu);
> free_percpu(port->stats);
> + kfree(port->ethtool_stats);
You allocate the memory using devm_. You should not use plain kfree()
on it. You might want to spend some time reading about devm_
> + mutex_init(&priv->gather_stats_lock);
> + index = ida_simple_get(&engine_index_ida, 0, 0, GFP_KERNEL);
> + if (index < 0)
> + goto err_mg_clk;
> +
> + snprintf(priv->queue_name, sizeof(priv->queue_name),
> + "mvpp2_stats_%d", index);
I know Florian asked for unique names, which IDA will give you. But
could you derive the name from device tree? It then becomes a name you
can actually map back to the hardware, rather than being semi-random.
Thanks
Andrew
