From e5c500eb298a9f5ef9b80d16fcea9662c89467b7 Mon Sep 17 00:00:00 2001 From: Miquel Raynal Date: Wed, 8 Nov 2017 08:59:40 +0100 Subject: [PATCH] net: mvpp2: fix GOP statistics loop start and stop conditions GOP statistics from all ports of one instance of the driver are gathered with one work recalled in loop in a workqueue. The loop is started when a port is up, and stopped when a port is down. This last condition is obviously wrong. Fix this by having a work per port. This way, starting and stoping it when the port is up or down will be fine, while minimizing unnecessary CPU usage. Fixes: 118d6298f6f0 ("net: mvpp2: add ethtool GOP statistics") Reported-by: Stefan Chulski Signed-off-by: Miquel Raynal Signed-off-by: David S. Miller --- drivers/net/ethernet/marvell/mvpp2.c | 62 +++++++++++++++++------------------- 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/drivers/net/ethernet/marvell/mvpp2.c b/drivers/net/ethernet/marvell/mvpp2.c index a79d2ff4f86e..6c20e811f973 100644 --- a/drivers/net/ethernet/marvell/mvpp2.c +++ b/drivers/net/ethernet/marvell/mvpp2.c @@ -885,9 +885,7 @@ 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; + /* Workqueue to gather hardware statistics */ char queue_name[30]; struct workqueue_struct *stats_queue; }; @@ -955,6 +953,10 @@ struct mvpp2_port { struct mvpp2_pcpu_stats __percpu *stats; u64 *ethtool_stats; + /* Per-port work and its lock to gather hardware statistics */ + struct mutex gather_stats_lock; + struct delayed_work stats_work; + phy_interface_t phy_interface; struct device_node *phy_node; struct phy *comphy; @@ -4895,32 +4897,25 @@ static void mvpp2_ethtool_get_strings(struct net_device *netdev, u32 sset, static void mvpp2_gather_hw_statistics(struct work_struct *work) { struct delayed_work *del_work = to_delayed_work(work); - struct mvpp2 *priv = container_of(del_work, struct mvpp2, stats_work); - struct mvpp2_port *port; + struct mvpp2_port *port = container_of(del_work, struct mvpp2_port, + stats_work); u64 *pstats; - int i, j; - - mutex_lock(&priv->gather_stats_lock); + int i; - for (i = 0; i < priv->port_count; i++) { - if (!priv->port_list[i]) - continue; + mutex_lock(&port->gather_stats_lock); - port = priv->port_list[i]; - pstats = port->ethtool_stats; - for (j = 0; j < ARRAY_SIZE(mvpp2_ethtool_regs); j++) - *pstats++ += mvpp2_read_count(port, - &mvpp2_ethtool_regs[j]); - } + pstats = port->ethtool_stats; + for (i = 0; i < ARRAY_SIZE(mvpp2_ethtool_regs); i++) + *pstats++ += mvpp2_read_count(port, &mvpp2_ethtool_regs[i]); /* No need to read again the counters right after this function if it * was called asynchronously by the user (ie. use of ethtool). */ - cancel_delayed_work(&priv->stats_work); - queue_delayed_work(priv->stats_queue, &priv->stats_work, + cancel_delayed_work(&port->stats_work); + queue_delayed_work(port->priv->stats_queue, &port->stats_work, MVPP2_MIB_COUNTERS_STATS_DELAY); - mutex_unlock(&priv->gather_stats_lock); + mutex_unlock(&port->gather_stats_lock); } static void mvpp2_ethtool_get_stats(struct net_device *dev, @@ -4928,13 +4923,15 @@ static void mvpp2_ethtool_get_stats(struct net_device *dev, { 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); + /* Update statistics for the given port, then take the lock to avoid + * concurrent accesses on the ethtool_stats structure during its copy. + */ + mvpp2_gather_hw_statistics(&port->stats_work.work); - mutex_lock(&port->priv->gather_stats_lock); + mutex_lock(&port->gather_stats_lock); memcpy(data, port->ethtool_stats, sizeof(u64) * ARRAY_SIZE(mvpp2_ethtool_regs)); - mutex_unlock(&port->priv->gather_stats_lock); + mutex_unlock(&port->gather_stats_lock); } static int mvpp2_ethtool_get_sset_count(struct net_device *dev, int sset) @@ -7089,7 +7086,7 @@ static int mvpp2_open(struct net_device *dev) mvpp22_init_rss(port); /* Start hardware statistics gathering */ - queue_delayed_work(priv->stats_queue, &priv->stats_work, + queue_delayed_work(priv->stats_queue, &port->stats_work, MVPP2_MIB_COUNTERS_STATS_DELAY); return 0; @@ -7136,8 +7133,7 @@ static int mvpp2_stop(struct net_device *dev) mvpp2_cleanup_rxqs(port); mvpp2_cleanup_txqs(port); - cancel_delayed_work_sync(&priv->stats_work); - flush_workqueue(priv->stats_queue); + cancel_delayed_work_sync(&port->stats_work); return 0; } @@ -7889,6 +7885,9 @@ static int mvpp2_port_probe(struct platform_device *pdev, goto err_free_stats; } + mutex_init(&port->gather_stats_lock); + INIT_DELAYED_WORK(&port->stats_work, mvpp2_gather_hw_statistics); + mvpp2_port_copy_mac_addr(dev, priv, port_node, &mac_from); port->tx_ring_size = MVPP2_MAX_TXD; @@ -8356,7 +8355,6 @@ static int mvpp2_probe(struct platform_device *pdev) * smallest packets (64B) will overflow a 32-bit counter in less than * 30 seconds. Then, use a workqueue to fill 64-bit counters. */ - mutex_init(&priv->gather_stats_lock); snprintf(priv->queue_name, sizeof(priv->queue_name), "stats-wq-%s%s", netdev_name(priv->port_list[0]->dev), priv->port_count > 1 ? "+" : ""); @@ -8366,8 +8364,6 @@ static int mvpp2_probe(struct platform_device *pdev) goto err_mg_clk; } - INIT_DELAYED_WORK(&priv->stats_work, mvpp2_gather_hw_statistics); - platform_set_drvdata(pdev, priv); return 0; @@ -8389,12 +8385,14 @@ static int mvpp2_remove(struct platform_device *pdev) struct device_node *port_node; int i = 0; + flush_workqueue(priv->stats_queue); destroy_workqueue(priv->stats_queue); - mutex_destroy(&priv->gather_stats_lock); for_each_available_child_of_node(dn, port_node) { - if (priv->port_list[i]) + if (priv->port_list[i]) { + mutex_destroy(&priv->port_list[i]->gather_stats_lock); mvpp2_port_remove(priv->port_list[i]); + } i++; } -- 2.11.0