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

[Andrew Lunn <andrew@lunn.ch>]

> @@ -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