[PATCH v2 0/2] net: dsa: add stats64 support

[PATCH v2 0/2] net: dsa: add stats64 support

[Oleksij Rempel]

changes v2:
- use stats64 instead of get_ethtool_stats
- add worked to poll for the stats

Oleksij Rempel (2):
  net: dsa: add optional stats64 support
  net: dsa: qca: ar9331: export stats64

 drivers/net/dsa/qca/ar9331.c | 242 ++++++++++++++++++++++++++++++++++-
 include/net/dsa.h            |   3 +
 net/dsa/slave.c              |  14 +-
 3 files changed, 257 insertions(+), 2 deletions(-)

-- 
2.29.2

[PATCH v2 1/2] net: dsa: add optional stats64 support

[Oleksij Rempel]

Allow DSA drivers to export stats64

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 include/net/dsa.h |  3 +++
 net/dsa/slave.c   | 14 +++++++++++++-
 2 files changed, 16 insertions(+), 1 deletion(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 4e60d2610f20..457b89143875 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -655,6 +655,9 @@ struct dsa_switch_ops {
 	int	(*port_change_mtu)(struct dsa_switch *ds, int port,
 				   int new_mtu);
 	int	(*port_max_mtu)(struct dsa_switch *ds, int port);
+
+	void	(*get_stats64)(struct dsa_switch *ds, int port,
+				   struct rtnl_link_stats64 *s);
 };
 
 #define DSA_DEVLINK_PARAM_DRIVER(_id, _name, _type, _cmodes)		\
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index ff2266d2b998..6e1a4dc18a97 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1602,6 +1602,18 @@ static struct devlink_port *dsa_slave_get_devlink_port(struct net_device *dev)
 	return dp->ds->devlink ? &dp->devlink_port : NULL;
 }
 
+static void dsa_slave_get_stats64(struct net_device *dev,
+				  struct rtnl_link_stats64 *s)
+{
+	struct dsa_port *dp = dsa_slave_to_port(dev);
+	struct dsa_switch *ds = dp->ds;
+
+	if (!ds->ops->get_stats64)
+		return dev_get_tstats64(dev, s);
+
+	return ds->ops->get_stats64(ds, dp->index, s);
+}
+
 static const struct net_device_ops dsa_slave_netdev_ops = {
 	.ndo_open	 	= dsa_slave_open,
 	.ndo_stop		= dsa_slave_close,
@@ -1621,7 +1633,7 @@ static const struct net_device_ops dsa_slave_netdev_ops = {
 #endif
 	.ndo_get_phys_port_name	= dsa_slave_get_phys_port_name,
 	.ndo_setup_tc		= dsa_slave_setup_tc,
-	.ndo_get_stats64	= dev_get_tstats64,
+	.ndo_get_stats64	= dsa_slave_get_stats64,
 	.ndo_get_port_parent_id	= dsa_slave_get_port_parent_id,
 	.ndo_vlan_rx_add_vid	= dsa_slave_vlan_rx_add_vid,
 	.ndo_vlan_rx_kill_vid	= dsa_slave_vlan_rx_kill_vid,
-- 
2.29.2

[PATCH v2 2/2] net: dsa: qca: ar9331: export stats64

[Oleksij Rempel]

Add stats support for the ar9331 switch.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/qca/ar9331.c | 242 ++++++++++++++++++++++++++++++++++-
 1 file changed, 241 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index e24a99031b80..1a8027bc9561 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -101,6 +101,57 @@
 	 AR9331_SW_PORT_STATUS_RX_FLOW_EN | AR9331_SW_PORT_STATUS_TX_FLOW_EN | \
 	 AR9331_SW_PORT_STATUS_SPEED_M)
 
+/* MIB registers */
+#define AR9331_MIB_COUNTER(x)			(0x20000 + ((x) * 0x100))
+
+#define AR9331_PORT_MIB_rxbroad(_port)		(AR9331_MIB_COUNTER(_port) + 0x00)
+#define AR9331_PORT_MIB_rxpause(_port)		(AR9331_MIB_COUNTER(_port) + 0x04)
+#define AR9331_PORT_MIB_rxmulti(_port)		(AR9331_MIB_COUNTER(_port) + 0x08)
+#define AR9331_PORT_MIB_rxfcserr(_port)		(AR9331_MIB_COUNTER(_port) + 0x0c)
+#define AR9331_PORT_MIB_rxalignerr(_port)	(AR9331_MIB_COUNTER(_port) + 0x10)
+#define AR9331_PORT_MIB_rxrunt(_port)		(AR9331_MIB_COUNTER(_port) + 0x14)
+#define AR9331_PORT_MIB_rxfragment(_port)	(AR9331_MIB_COUNTER(_port) + 0x18)
+#define AR9331_PORT_MIB_rx64byte(_port)		(AR9331_MIB_COUNTER(_port) + 0x1c)
+#define AR9331_PORT_MIB_rx128byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x20)
+#define AR9331_PORT_MIB_rx256byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x24)
+#define AR9331_PORT_MIB_rx512byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x28)
+#define AR9331_PORT_MIB_rx1024byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x2c)
+#define AR9331_PORT_MIB_rx1518byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x30)
+#define AR9331_PORT_MIB_rxmaxbyte(_port)	(AR9331_MIB_COUNTER(_port) + 0x34)
+#define AR9331_PORT_MIB_rxtoolong(_port)	(AR9331_MIB_COUNTER(_port) + 0x38)
+
+/* 64 bit counter */
+#define AR9331_PORT_MIB_rxgoodbyte(_port)	(AR9331_MIB_COUNTER(_port) + 0x3c)
+
+/* 64 bit counter */
+#define AR9331_PORT_MIB_rxbadbyte(_port)	(AR9331_MIB_COUNTER(_port) + 0x44)
+
+#define AR9331_PORT_MIB_rxoverflow(_port)	(AR9331_MIB_COUNTER(_port) + 0x4c)
+#define AR9331_PORT_MIB_filtered(_port)		(AR9331_MIB_COUNTER(_port) + 0x50)
+#define AR9331_PORT_MIB_txbroad(_port)		(AR9331_MIB_COUNTER(_port) + 0x54)
+#define AR9331_PORT_MIB_txpause(_port)		(AR9331_MIB_COUNTER(_port) + 0x58)
+#define AR9331_PORT_MIB_txmulti(_port)		(AR9331_MIB_COUNTER(_port) + 0x5c)
+#define AR9331_PORT_MIB_txunderrun(_port)	(AR9331_MIB_COUNTER(_port) + 0x60)
+#define AR9331_PORT_MIB_tx64byte(_port)		(AR9331_MIB_COUNTER(_port) + 0x64)
+#define AR9331_PORT_MIB_tx128byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x68)
+#define AR9331_PORT_MIB_tx256byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x6c)
+#define AR9331_PORT_MIB_tx512byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x70)
+#define AR9331_PORT_MIB_tx1024byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x74)
+#define AR9331_PORT_MIB_tx1518byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x78)
+#define AR9331_PORT_MIB_txmaxbyte(_port)	(AR9331_MIB_COUNTER(_port) + 0x7c)
+#define AR9331_PORT_MIB_txoversize(_port)	(AR9331_MIB_COUNTER(_port) + 0x80)
+
+/* 64 bit counter */
+#define AR9331_PORT_MIB_txbyte(_port)		(AR9331_MIB_COUNTER(_port) + 0x84)
+
+#define AR9331_PORT_MIB_txcollision(_port)	(AR9331_MIB_COUNTER(_port) + 0x8c)
+#define AR9331_PORT_MIB_txabortcol(_port)	(AR9331_MIB_COUNTER(_port) + 0x90)
+#define AR9331_PORT_MIB_txmulticol(_port)	(AR9331_MIB_COUNTER(_port) + 0x94)
+#define AR9331_PORT_MIB_txsinglecol(_port)	(AR9331_MIB_COUNTER(_port) + 0x98)
+#define AR9331_PORT_MIB_txexcdefer(_port)	(AR9331_MIB_COUNTER(_port) + 0x9c)
+#define AR9331_PORT_MIB_txdefer(_port)		(AR9331_MIB_COUNTER(_port) + 0xa0)
+#define AR9331_PORT_MIB_txlatecol(_port)	(AR9331_MIB_COUNTER(_port) + 0xa4)
+
 /* Phy bypass mode
  * ------------------------------------------------------------------------
  * Bit:   | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 |10 |11 |12 |13 |14 |15 |
@@ -154,6 +205,59 @@
 #define AR9331_SW_MDIO_POLL_SLEEP_US		1
 #define AR9331_SW_MDIO_POLL_TIMEOUT_US		20
 
+#define STATS_INTERVAL_JIFFIES			(100 * HZ)
+
+struct ar9331_sw_stats {
+	u64 rxbroad;
+	u64 rxpause;
+	u64 rxmulti;
+	u64 rxfcserr;
+	u64 rxalignerr;
+	u64 rxrunt;
+	u64 rxfragment;
+	u64 rx64byte;
+	u64 rx128byte;
+	u64 rx256byte;
+	u64 rx512byte;
+	u64 rx1024byte;
+	u64 rx1518byte;
+	u64 rxmaxbyte;
+	u64 rxtoolong;
+	u64 rxgoodbyte;
+	u64 rxbadbyte;
+	u64 rxoverflow;
+	u64 filtered;
+	u64 txbroad;
+	u64 txpause;
+	u64 txmulti;
+	u64 txunderrun;
+	u64 tx64byte;
+	u64 tx128byte;
+	u64 tx256byte;
+	u64 tx512byte;
+	u64 tx1024byte;
+	u64 tx1518byte;
+	u64 txmaxbyte;
+	u64 txoversize;
+	u64 txbyte;
+	u64 txcollision;
+	u64 txabortcol;
+	u64 txmulticol;
+	u64 txsinglecol;
+	u64 txexcdefer;
+	u64 txdefer;
+	u64 txlatecol;
+};
+
+struct ar9331_sw_priv;
+struct ar9331_sw_port {
+	int idx;
+	struct ar9331_sw_priv *priv;
+	struct delayed_work mib_read;
+	struct ar9331_sw_stats stats;
+	struct mutex lock;		/* stats access */
+};
+
 struct ar9331_sw_priv {
 	struct device *dev;
 	struct dsa_switch ds;
@@ -163,6 +267,7 @@ struct ar9331_sw_priv {
 	struct mii_bus *sbus; /* mdio slave */
 	struct regmap *regmap;
 	struct reset_control *sw_reset;
+	struct ar9331_sw_port port[AR9331_SW_PORTS];
 };
 
 /* Warning: switch reset will reset last AR9331_SW_MDIO_PHY_MODE_PAGE request
@@ -422,6 +527,7 @@ static void ar9331_sw_phylink_mac_link_down(struct dsa_switch *ds, int port,
 					    phy_interface_t interface)
 {
 	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct ar9331_sw_port *p = &priv->port[port];
 	struct regmap *regmap = priv->regmap;
 	int ret;
 
@@ -429,6 +535,8 @@ static void ar9331_sw_phylink_mac_link_down(struct dsa_switch *ds, int port,
 				 AR9331_SW_PORT_STATUS_MAC_MASK, 0);
 	if (ret)
 		dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
+
+	cancel_delayed_work_sync(&p->mib_read);
 }
 
 static void ar9331_sw_phylink_mac_link_up(struct dsa_switch *ds, int port,
@@ -439,10 +547,13 @@ static void ar9331_sw_phylink_mac_link_up(struct dsa_switch *ds, int port,
 					  bool tx_pause, bool rx_pause)
 {
 	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct ar9331_sw_port *p = &priv->port[port];
 	struct regmap *regmap = priv->regmap;
 	u32 val;
 	int ret;
 
+	schedule_delayed_work(&p->mib_read, 0);
+
 	val = AR9331_SW_PORT_STATUS_MAC_MASK;
 	switch (speed) {
 	case SPEED_1000:
@@ -475,6 +586,125 @@ static void ar9331_sw_phylink_mac_link_up(struct dsa_switch *ds, int port,
 		dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
 }
 
+static u32 ar9331_stat_get_val(const struct ar9331_sw_priv *priv, u32 reg)
+{
+	u32 val;
+	int ret;
+
+	ret = regmap_read(priv->regmap, reg, &val);
+	if (ret) {
+		dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
+		return 0;
+	}
+
+	return val;
+}
+
+#define AR9331_READ_STATS(_port, _reg) \
+{ \
+	const struct ar9331_sw_priv *priv = _port->priv; \
+	struct ar9331_sw_stats *s = &_port->stats; \
+	int idx = _port->idx; \
+\
+	s->_reg += ar9331_stat_get_val(priv, AR9331_PORT_MIB_##_reg(idx)); \
+}
+
+static void ar9331_read_stats(struct ar9331_sw_port *port)
+{
+	mutex_lock(&port->lock);
+
+	AR9331_READ_STATS(port, rxbroad);
+	AR9331_READ_STATS(port, rxpause);
+	AR9331_READ_STATS(port, rxmulti);
+	AR9331_READ_STATS(port, rxfcserr);
+	AR9331_READ_STATS(port, rxalignerr);
+	AR9331_READ_STATS(port, rxrunt);
+	AR9331_READ_STATS(port, rxfragment);
+	AR9331_READ_STATS(port, rx64byte);
+	AR9331_READ_STATS(port, rx128byte);
+	AR9331_READ_STATS(port, rx256byte);
+	AR9331_READ_STATS(port, rx512byte);
+	AR9331_READ_STATS(port, rx1024byte);
+	AR9331_READ_STATS(port, rx1518byte);
+	AR9331_READ_STATS(port, rxmaxbyte);
+	AR9331_READ_STATS(port, rxtoolong);
+	AR9331_READ_STATS(port, rxgoodbyte);
+	AR9331_READ_STATS(port, rxbadbyte);
+	AR9331_READ_STATS(port, rxoverflow);
+	AR9331_READ_STATS(port, filtered);
+	AR9331_READ_STATS(port, txbroad);
+	AR9331_READ_STATS(port, txpause);
+	AR9331_READ_STATS(port, txmulti);
+	AR9331_READ_STATS(port, txunderrun);
+	AR9331_READ_STATS(port, tx64byte);
+	AR9331_READ_STATS(port, tx128byte);
+	AR9331_READ_STATS(port, tx256byte);
+	AR9331_READ_STATS(port, tx512byte);
+	AR9331_READ_STATS(port, tx1024byte);
+	AR9331_READ_STATS(port, tx1518byte);
+	AR9331_READ_STATS(port, txmaxbyte);
+	AR9331_READ_STATS(port, txoversize);
+	AR9331_READ_STATS(port, txbyte);
+	AR9331_READ_STATS(port, txcollision);
+	AR9331_READ_STATS(port, txabortcol);
+	AR9331_READ_STATS(port, txmulticol);
+	AR9331_READ_STATS(port, txsinglecol);
+	AR9331_READ_STATS(port, txexcdefer);
+	AR9331_READ_STATS(port, txdefer);
+	AR9331_READ_STATS(port, txlatecol);
+
+	mutex_unlock(&port->lock);
+}
+
+static void ar9331_stats_update(struct ar9331_sw_port *port,
+				struct rtnl_link_stats64 *stats)
+{
+	struct ar9331_sw_stats *s = &port->stats;
+
+	stats->rx_packets = s->rxbroad + s->rxmulti + s->rx64byte +
+		s->rx128byte + s->rx256byte + s->rx512byte + s->rx1024byte +
+		s->rx1518byte + s->rxmaxbyte;
+	stats->tx_packets = s->txbroad + s->txmulti + s->tx64byte +
+		s->tx128byte + s->tx256byte + s->tx512byte + s->tx1024byte +
+		s->tx1518byte + s->txmaxbyte;
+	stats->rx_bytes = s->rxgoodbyte;
+	stats->tx_bytes = s->txbyte;
+	stats->rx_errors = s->rxfcserr + s->rxalignerr + s->rxrunt +
+		s->rxfragment + s->rxoverflow;
+	stats->tx_errors = s->txoversize;
+	stats->multicast = s->rxmulti;
+	stats->collisions = s->txcollision;
+	stats->rx_length_errors = s->rxrunt * s->rxfragment + s->rxtoolong;
+	stats->rx_crc_errors = s->rxfcserr + s->rxalignerr + s->rxfragment;
+	stats->rx_frame_errors = s->rxalignerr;
+	stats->rx_missed_errors = s->rxoverflow;
+	stats->tx_aborted_errors = s->txabortcol;
+	stats->tx_fifo_errors = s->txunderrun;
+	stats->tx_window_errors = s->txlatecol;
+	stats->rx_nohandler = s->filtered;
+}
+
+static void ar9331_do_stats_poll(struct work_struct *work)
+{
+
+	struct ar9331_sw_port *port = container_of(work, struct ar9331_sw_port,
+					      mib_read.work);
+
+	ar9331_read_stats(port);
+
+	schedule_delayed_work(&port->mib_read, STATS_INTERVAL_JIFFIES);
+}
+
+static void ar9331_get_stats64(struct dsa_switch *ds, int port,
+			       struct rtnl_link_stats64 *s)
+{
+	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
+	struct ar9331_sw_port *p = &priv->port[port];
+
+	ar9331_read_stats(p);
+	ar9331_stats_update(p, s);
+}
+
 static const struct dsa_switch_ops ar9331_sw_ops = {
 	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
 	.setup			= ar9331_sw_setup,
@@ -483,6 +713,7 @@ static const struct dsa_switch_ops ar9331_sw_ops = {
 	.phylink_mac_config	= ar9331_sw_phylink_mac_config,
 	.phylink_mac_link_down	= ar9331_sw_phylink_mac_link_down,
 	.phylink_mac_link_up	= ar9331_sw_phylink_mac_link_up,
+	.get_stats64		= ar9331_get_stats64,
 };
 
 static irqreturn_t ar9331_sw_irq(int irq, void *data)
@@ -781,7 +1012,7 @@ static int ar9331_sw_probe(struct mdio_device *mdiodev)
 {
 	struct ar9331_sw_priv *priv;
 	struct dsa_switch *ds;
-	int ret;
+	int ret, i;
 
 	priv = devm_kzalloc(&mdiodev->dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
@@ -816,6 +1047,15 @@ static int ar9331_sw_probe(struct mdio_device *mdiodev)
 	ds->ops = &priv->ops;
 	dev_set_drvdata(&mdiodev->dev, priv);
 
+	for (i = 0; i < ARRAY_SIZE(priv->port); i++) {
+		struct ar9331_sw_port *port = &priv->port[i];
+
+		port->idx = i;
+		port->priv = priv;
+		mutex_init(&port->lock);
+		INIT_DELAYED_WORK(&port->mib_read, ar9331_do_stats_poll);
+	}
+
 	ret = dsa_register_switch(ds);
 	if (ret)
 		goto err_remove_irq;
-- 
2.29.2

Re: [PATCH v2 2/2] net: dsa: qca: ar9331: export stats64

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 5679 bytes --]

On 12/2/20 1:07 PM, Oleksij Rempel wrote:
> Add stats support for the ar9331 switch.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/qca/ar9331.c | 242 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 241 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
> index e24a99031b80..1a8027bc9561 100644
> --- a/drivers/net/dsa/qca/ar9331.c
> +++ b/drivers/net/dsa/qca/ar9331.c
> @@ -101,6 +101,57 @@
>  	 AR9331_SW_PORT_STATUS_RX_FLOW_EN | AR9331_SW_PORT_STATUS_TX_FLOW_EN | \
>  	 AR9331_SW_PORT_STATUS_SPEED_M)
>  
> +/* MIB registers */
> +#define AR9331_MIB_COUNTER(x)			(0x20000 + ((x) * 0x100))
> +
> +#define AR9331_PORT_MIB_rxbroad(_port)		(AR9331_MIB_COUNTER(_port) + 0x00)
> +#define AR9331_PORT_MIB_rxpause(_port)		(AR9331_MIB_COUNTER(_port) + 0x04)
> +#define AR9331_PORT_MIB_rxmulti(_port)		(AR9331_MIB_COUNTER(_port) + 0x08)
> +#define AR9331_PORT_MIB_rxfcserr(_port)		(AR9331_MIB_COUNTER(_port) + 0x0c)
> +#define AR9331_PORT_MIB_rxalignerr(_port)	(AR9331_MIB_COUNTER(_port) + 0x10)
> +#define AR9331_PORT_MIB_rxrunt(_port)		(AR9331_MIB_COUNTER(_port) + 0x14)
> +#define AR9331_PORT_MIB_rxfragment(_port)	(AR9331_MIB_COUNTER(_port) + 0x18)
> +#define AR9331_PORT_MIB_rx64byte(_port)		(AR9331_MIB_COUNTER(_port) + 0x1c)
> +#define AR9331_PORT_MIB_rx128byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x20)
> +#define AR9331_PORT_MIB_rx256byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x24)
> +#define AR9331_PORT_MIB_rx512byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x28)
> +#define AR9331_PORT_MIB_rx1024byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x2c)
> +#define AR9331_PORT_MIB_rx1518byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x30)
> +#define AR9331_PORT_MIB_rxmaxbyte(_port)	(AR9331_MIB_COUNTER(_port) + 0x34)
> +#define AR9331_PORT_MIB_rxtoolong(_port)	(AR9331_MIB_COUNTER(_port) + 0x38)
> +
> +/* 64 bit counter */
> +#define AR9331_PORT_MIB_rxgoodbyte(_port)	(AR9331_MIB_COUNTER(_port) + 0x3c)
> +
> +/* 64 bit counter */
> +#define AR9331_PORT_MIB_rxbadbyte(_port)	(AR9331_MIB_COUNTER(_port) + 0x44)
> +
> +#define AR9331_PORT_MIB_rxoverflow(_port)	(AR9331_MIB_COUNTER(_port) + 0x4c)
> +#define AR9331_PORT_MIB_filtered(_port)		(AR9331_MIB_COUNTER(_port) + 0x50)
> +#define AR9331_PORT_MIB_txbroad(_port)		(AR9331_MIB_COUNTER(_port) + 0x54)
> +#define AR9331_PORT_MIB_txpause(_port)		(AR9331_MIB_COUNTER(_port) + 0x58)
> +#define AR9331_PORT_MIB_txmulti(_port)		(AR9331_MIB_COUNTER(_port) + 0x5c)
> +#define AR9331_PORT_MIB_txunderrun(_port)	(AR9331_MIB_COUNTER(_port) + 0x60)
> +#define AR9331_PORT_MIB_tx64byte(_port)		(AR9331_MIB_COUNTER(_port) + 0x64)
> +#define AR9331_PORT_MIB_tx128byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x68)
> +#define AR9331_PORT_MIB_tx256byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x6c)
> +#define AR9331_PORT_MIB_tx512byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x70)
> +#define AR9331_PORT_MIB_tx1024byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x74)
> +#define AR9331_PORT_MIB_tx1518byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x78)
> +#define AR9331_PORT_MIB_txmaxbyte(_port)	(AR9331_MIB_COUNTER(_port) + 0x7c)
> +#define AR9331_PORT_MIB_txoversize(_port)	(AR9331_MIB_COUNTER(_port) + 0x80)
> +
> +/* 64 bit counter */
> +#define AR9331_PORT_MIB_txbyte(_port)		(AR9331_MIB_COUNTER(_port) + 0x84)
> +
> +#define AR9331_PORT_MIB_txcollision(_port)	(AR9331_MIB_COUNTER(_port) + 0x8c)
> +#define AR9331_PORT_MIB_txabortcol(_port)	(AR9331_MIB_COUNTER(_port) + 0x90)
> +#define AR9331_PORT_MIB_txmulticol(_port)	(AR9331_MIB_COUNTER(_port) + 0x94)
> +#define AR9331_PORT_MIB_txsinglecol(_port)	(AR9331_MIB_COUNTER(_port) + 0x98)
> +#define AR9331_PORT_MIB_txexcdefer(_port)	(AR9331_MIB_COUNTER(_port) + 0x9c)
> +#define AR9331_PORT_MIB_txdefer(_port)		(AR9331_MIB_COUNTER(_port) + 0xa0)
> +#define AR9331_PORT_MIB_txlatecol(_port)	(AR9331_MIB_COUNTER(_port) + 0xa4)
> +
>  /* Phy bypass mode
>   * ------------------------------------------------------------------------
>   * Bit:   | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 |10 |11 |12 |13 |14 |15 |
> @@ -154,6 +205,59 @@
>  #define AR9331_SW_MDIO_POLL_SLEEP_US		1
>  #define AR9331_SW_MDIO_POLL_TIMEOUT_US		20
>  
> +#define STATS_INTERVAL_JIFFIES			(100 * HZ)
> +
> +struct ar9331_sw_stats {
> +	u64 rxbroad;
> +	u64 rxpause;
> +	u64 rxmulti;
> +	u64 rxfcserr;
> +	u64 rxalignerr;
> +	u64 rxrunt;
> +	u64 rxfragment;
> +	u64 rx64byte;
> +	u64 rx128byte;
> +	u64 rx256byte;
> +	u64 rx512byte;
> +	u64 rx1024byte;
> +	u64 rx1518byte;
> +	u64 rxmaxbyte;
> +	u64 rxtoolong;
> +	u64 rxgoodbyte;
> +	u64 rxbadbyte;
> +	u64 rxoverflow;
> +	u64 filtered;
> +	u64 txbroad;
> +	u64 txpause;
> +	u64 txmulti;
> +	u64 txunderrun;
> +	u64 tx64byte;
> +	u64 tx128byte;
> +	u64 tx256byte;
> +	u64 tx512byte;
> +	u64 tx1024byte;
> +	u64 tx1518byte;
> +	u64 txmaxbyte;
> +	u64 txoversize;
> +	u64 txbyte;
> +	u64 txcollision;
> +	u64 txabortcol;
> +	u64 txmulticol;
> +	u64 txsinglecol;
> +	u64 txexcdefer;
> +	u64 txdefer;
> +	u64 txlatecol;
> +};
> +
> +struct ar9331_sw_priv;
> +struct ar9331_sw_port {
> +	int idx;
> +	struct ar9331_sw_priv *priv;
> +	struct delayed_work mib_read;
> +	struct ar9331_sw_stats stats;
> +	struct mutex lock;		/* stats access */

What does the lock protect? It's only used a single time.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 2/2] net: dsa: qca: ar9331: export stats64

[Oleksij Rempel]

On Wed, Dec 02, 2020 at 01:15:58PM +0100, Marc Kleine-Budde wrote:
> On 12/2/20 1:07 PM, Oleksij Rempel wrote:
> > Add stats support for the ar9331 switch.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/net/dsa/qca/ar9331.c | 242 ++++++++++++++++++++++++++++++++++-
> >  1 file changed, 241 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
> > index e24a99031b80..1a8027bc9561 100644
> > --- a/drivers/net/dsa/qca/ar9331.c
> > +++ b/drivers/net/dsa/qca/ar9331.c
> > @@ -101,6 +101,57 @@
> >  	 AR9331_SW_PORT_STATUS_RX_FLOW_EN | AR9331_SW_PORT_STATUS_TX_FLOW_EN | \
> >  	 AR9331_SW_PORT_STATUS_SPEED_M)
> >  
> > +/* MIB registers */
> > +#define AR9331_MIB_COUNTER(x)			(0x20000 + ((x) * 0x100))
> > +
> > +#define AR9331_PORT_MIB_rxbroad(_port)		(AR9331_MIB_COUNTER(_port) + 0x00)
> > +#define AR9331_PORT_MIB_rxpause(_port)		(AR9331_MIB_COUNTER(_port) + 0x04)
> > +#define AR9331_PORT_MIB_rxmulti(_port)		(AR9331_MIB_COUNTER(_port) + 0x08)
> > +#define AR9331_PORT_MIB_rxfcserr(_port)		(AR9331_MIB_COUNTER(_port) + 0x0c)
> > +#define AR9331_PORT_MIB_rxalignerr(_port)	(AR9331_MIB_COUNTER(_port) + 0x10)
> > +#define AR9331_PORT_MIB_rxrunt(_port)		(AR9331_MIB_COUNTER(_port) + 0x14)
> > +#define AR9331_PORT_MIB_rxfragment(_port)	(AR9331_MIB_COUNTER(_port) + 0x18)
> > +#define AR9331_PORT_MIB_rx64byte(_port)		(AR9331_MIB_COUNTER(_port) + 0x1c)
> > +#define AR9331_PORT_MIB_rx128byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x20)
> > +#define AR9331_PORT_MIB_rx256byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x24)
> > +#define AR9331_PORT_MIB_rx512byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x28)
> > +#define AR9331_PORT_MIB_rx1024byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x2c)
> > +#define AR9331_PORT_MIB_rx1518byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x30)
> > +#define AR9331_PORT_MIB_rxmaxbyte(_port)	(AR9331_MIB_COUNTER(_port) + 0x34)
> > +#define AR9331_PORT_MIB_rxtoolong(_port)	(AR9331_MIB_COUNTER(_port) + 0x38)
> > +
> > +/* 64 bit counter */
> > +#define AR9331_PORT_MIB_rxgoodbyte(_port)	(AR9331_MIB_COUNTER(_port) + 0x3c)
> > +
> > +/* 64 bit counter */
> > +#define AR9331_PORT_MIB_rxbadbyte(_port)	(AR9331_MIB_COUNTER(_port) + 0x44)
> > +
> > +#define AR9331_PORT_MIB_rxoverflow(_port)	(AR9331_MIB_COUNTER(_port) + 0x4c)
> > +#define AR9331_PORT_MIB_filtered(_port)		(AR9331_MIB_COUNTER(_port) + 0x50)
> > +#define AR9331_PORT_MIB_txbroad(_port)		(AR9331_MIB_COUNTER(_port) + 0x54)
> > +#define AR9331_PORT_MIB_txpause(_port)		(AR9331_MIB_COUNTER(_port) + 0x58)
> > +#define AR9331_PORT_MIB_txmulti(_port)		(AR9331_MIB_COUNTER(_port) + 0x5c)
> > +#define AR9331_PORT_MIB_txunderrun(_port)	(AR9331_MIB_COUNTER(_port) + 0x60)
> > +#define AR9331_PORT_MIB_tx64byte(_port)		(AR9331_MIB_COUNTER(_port) + 0x64)
> > +#define AR9331_PORT_MIB_tx128byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x68)
> > +#define AR9331_PORT_MIB_tx256byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x6c)
> > +#define AR9331_PORT_MIB_tx512byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x70)
> > +#define AR9331_PORT_MIB_tx1024byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x74)
> > +#define AR9331_PORT_MIB_tx1518byte(_port)	(AR9331_MIB_COUNTER(_port) + 0x78)
> > +#define AR9331_PORT_MIB_txmaxbyte(_port)	(AR9331_MIB_COUNTER(_port) + 0x7c)
> > +#define AR9331_PORT_MIB_txoversize(_port)	(AR9331_MIB_COUNTER(_port) + 0x80)
> > +
> > +/* 64 bit counter */
> > +#define AR9331_PORT_MIB_txbyte(_port)		(AR9331_MIB_COUNTER(_port) + 0x84)
> > +
> > +#define AR9331_PORT_MIB_txcollision(_port)	(AR9331_MIB_COUNTER(_port) + 0x8c)
> > +#define AR9331_PORT_MIB_txabortcol(_port)	(AR9331_MIB_COUNTER(_port) + 0x90)
> > +#define AR9331_PORT_MIB_txmulticol(_port)	(AR9331_MIB_COUNTER(_port) + 0x94)
> > +#define AR9331_PORT_MIB_txsinglecol(_port)	(AR9331_MIB_COUNTER(_port) + 0x98)
> > +#define AR9331_PORT_MIB_txexcdefer(_port)	(AR9331_MIB_COUNTER(_port) + 0x9c)
> > +#define AR9331_PORT_MIB_txdefer(_port)		(AR9331_MIB_COUNTER(_port) + 0xa0)
> > +#define AR9331_PORT_MIB_txlatecol(_port)	(AR9331_MIB_COUNTER(_port) + 0xa4)
> > +
> >  /* Phy bypass mode
> >   * ------------------------------------------------------------------------
> >   * Bit:   | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 |10 |11 |12 |13 |14 |15 |
> > @@ -154,6 +205,59 @@
> >  #define AR9331_SW_MDIO_POLL_SLEEP_US		1
> >  #define AR9331_SW_MDIO_POLL_TIMEOUT_US		20
> >  
> > +#define STATS_INTERVAL_JIFFIES			(100 * HZ)
> > +
> > +struct ar9331_sw_stats {
> > +	u64 rxbroad;
> > +	u64 rxpause;
> > +	u64 rxmulti;
> > +	u64 rxfcserr;
> > +	u64 rxalignerr;
> > +	u64 rxrunt;
> > +	u64 rxfragment;
> > +	u64 rx64byte;
> > +	u64 rx128byte;
> > +	u64 rx256byte;
> > +	u64 rx512byte;
> > +	u64 rx1024byte;
> > +	u64 rx1518byte;
> > +	u64 rxmaxbyte;
> > +	u64 rxtoolong;
> > +	u64 rxgoodbyte;
> > +	u64 rxbadbyte;
> > +	u64 rxoverflow;
> > +	u64 filtered;
> > +	u64 txbroad;
> > +	u64 txpause;
> > +	u64 txmulti;
> > +	u64 txunderrun;
> > +	u64 tx64byte;
> > +	u64 tx128byte;
> > +	u64 tx256byte;
> > +	u64 tx512byte;
> > +	u64 tx1024byte;
> > +	u64 tx1518byte;
> > +	u64 txmaxbyte;
> > +	u64 txoversize;
> > +	u64 txbyte;
> > +	u64 txcollision;
> > +	u64 txabortcol;
> > +	u64 txmulticol;
> > +	u64 txsinglecol;
> > +	u64 txexcdefer;
> > +	u64 txdefer;
> > +	u64 txlatecol;
> > +};
> > +
> > +struct ar9331_sw_priv;
> > +struct ar9331_sw_port {
> > +	int idx;
> > +	struct ar9331_sw_priv *priv;
> > +	struct delayed_work mib_read;
> > +	struct ar9331_sw_stats stats;
> > +	struct mutex lock;		/* stats access */
> 
> What does the lock protect? It's only used a single time.

The ar9331_read_stats() function is called from two different contests:
from worker over ar9331_do_stats_poll() and from user space over
ar9331_get_stats64().

The mutex lock should prevent a race in the read modify write operations
for in the stats->*

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 2/2] net: dsa: qca: ar9331: export stats64

[Marc Kleine-Budde]

[-- Attachment #1.1: Type: text/plain, Size: 912 bytes --]

On 12/2/20 1:43 PM, Oleksij Rempel wrote:
>>> +struct ar9331_sw_priv;
>>> +struct ar9331_sw_port {
>>> +	int idx;
>>> +	struct ar9331_sw_priv *priv;
>>> +	struct delayed_work mib_read;
>>> +	struct ar9331_sw_stats stats;
>>> +	struct mutex lock;		/* stats access */
>>
>> What does the lock protect? It's only used a single time.
> 
> The ar9331_read_stats() function is called from two different contests:
> from worker over ar9331_do_stats_poll() and from user space over
> ar9331_get_stats64().
> 
> The mutex lock should prevent a race in the read modify write operations
> for in the stats->*

Makes sense!

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] net: dsa: add optional stats64 support

[Vladimir Oltean]

On Wed, Dec 02, 2020 at 01:07:11PM +0100, Oleksij Rempel wrote:
> Allow DSA drivers to export stats64
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH v2 2/2] net: dsa: qca: ar9331: export stats64

[Vladimir Oltean]

On Wed, Dec 02, 2020 at 01:07:12PM +0100, Oleksij Rempel wrote:
> Add stats support for the ar9331 switch.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  /* Warning: switch reset will reset last AR9331_SW_MDIO_PHY_MODE_PAGE request
> @@ -422,6 +527,7 @@ static void ar9331_sw_phylink_mac_link_down(struct dsa_switch *ds, int port,
>  					    phy_interface_t interface)
>  {
>  	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> +	struct ar9331_sw_port *p = &priv->port[port];
>  	struct regmap *regmap = priv->regmap;
>  	int ret;
>  
> @@ -429,6 +535,8 @@ static void ar9331_sw_phylink_mac_link_down(struct dsa_switch *ds, int port,
>  				 AR9331_SW_PORT_STATUS_MAC_MASK, 0);
>  	if (ret)
>  		dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> +
> +	cancel_delayed_work_sync(&p->mib_read);

Is this sufficient? Do you get a guaranteed .phylink_mac_link_down event
on unbind? How do you ensure you don't race with the stats worker there?

> +static void ar9331_stats_update(struct ar9331_sw_port *port,
> +				struct rtnl_link_stats64 *stats)
> +{
> +	struct ar9331_sw_stats *s = &port->stats;
> +
> +	stats->rx_packets = s->rxbroad + s->rxmulti + s->rx64byte +
> +		s->rx128byte + s->rx256byte + s->rx512byte + s->rx1024byte +
> +		s->rx1518byte + s->rxmaxbyte;
> +	stats->tx_packets = s->txbroad + s->txmulti + s->tx64byte +
> +		s->tx128byte + s->tx256byte + s->tx512byte + s->tx1024byte +
> +		s->tx1518byte + s->txmaxbyte;
> +	stats->rx_bytes = s->rxgoodbyte;
> +	stats->tx_bytes = s->txbyte;
> +	stats->rx_errors = s->rxfcserr + s->rxalignerr + s->rxrunt +
> +		s->rxfragment + s->rxoverflow;
> +	stats->tx_errors = s->txoversize;
> +	stats->multicast = s->rxmulti;
> +	stats->collisions = s->txcollision;
> +	stats->rx_length_errors = s->rxrunt * s->rxfragment + s->rxtoolong;

Multiplication? Is this right?

> +	stats->rx_crc_errors = s->rxfcserr + s->rxalignerr + s->rxfragment;
> +	stats->rx_frame_errors = s->rxalignerr;
> +	stats->rx_missed_errors = s->rxoverflow;
> +	stats->tx_aborted_errors = s->txabortcol;
> +	stats->tx_fifo_errors = s->txunderrun;
> +	stats->tx_window_errors = s->txlatecol;
> +	stats->rx_nohandler = s->filtered;
> +}
> +
> +static void ar9331_do_stats_poll(struct work_struct *work)
> +{
> +

Could you remove this empty line.

Re: [PATCH v2 2/2] net: dsa: qca: ar9331: export stats64

[Oleksij Rempel]

On Wed, Dec 02, 2020 at 03:04:38PM +0200, Vladimir Oltean wrote:
> On Wed, Dec 02, 2020 at 01:07:12PM +0100, Oleksij Rempel wrote:
> > Add stats support for the ar9331 switch.
> > 
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  /* Warning: switch reset will reset last AR9331_SW_MDIO_PHY_MODE_PAGE request
> > @@ -422,6 +527,7 @@ static void ar9331_sw_phylink_mac_link_down(struct dsa_switch *ds, int port,
> >  					    phy_interface_t interface)
> >  {
> >  	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> > +	struct ar9331_sw_port *p = &priv->port[port];
> >  	struct regmap *regmap = priv->regmap;
> >  	int ret;
> >  
> > @@ -429,6 +535,8 @@ static void ar9331_sw_phylink_mac_link_down(struct dsa_switch *ds, int port,
> >  				 AR9331_SW_PORT_STATUS_MAC_MASK, 0);
> >  	if (ret)
> >  		dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> > +
> > +	cancel_delayed_work_sync(&p->mib_read);
> 
> Is this sufficient? Do you get a guaranteed .phylink_mac_link_down event
> on unbind? How do you ensure you don't race with the stats worker there?

Currently it works, but you are right, i'll better do this on remove as
well.

> > +static void ar9331_stats_update(struct ar9331_sw_port *port,
> > +				struct rtnl_link_stats64 *stats)
> > +{
> > +	struct ar9331_sw_stats *s = &port->stats;
> > +
> > +	stats->rx_packets = s->rxbroad + s->rxmulti + s->rx64byte +
> > +		s->rx128byte + s->rx256byte + s->rx512byte + s->rx1024byte +
> > +		s->rx1518byte + s->rxmaxbyte;
> > +	stats->tx_packets = s->txbroad + s->txmulti + s->tx64byte +
> > +		s->tx128byte + s->tx256byte + s->tx512byte + s->tx1024byte +
> > +		s->tx1518byte + s->txmaxbyte;
> > +	stats->rx_bytes = s->rxgoodbyte;
> > +	stats->tx_bytes = s->txbyte;
> > +	stats->rx_errors = s->rxfcserr + s->rxalignerr + s->rxrunt +
> > +		s->rxfragment + s->rxoverflow;
> > +	stats->tx_errors = s->txoversize;
> > +	stats->multicast = s->rxmulti;
> > +	stats->collisions = s->txcollision;
> > +	stats->rx_length_errors = s->rxrunt * s->rxfragment + s->rxtoolong;
> 
> Multiplication? Is this right?

no. fixed.

> > +	stats->rx_crc_errors = s->rxfcserr + s->rxalignerr + s->rxfragment;
> > +	stats->rx_frame_errors = s->rxalignerr;
> > +	stats->rx_missed_errors = s->rxoverflow;
> > +	stats->tx_aborted_errors = s->txabortcol;
> > +	stats->tx_fifo_errors = s->txunderrun;
> > +	stats->tx_window_errors = s->txlatecol;
> > +	stats->rx_nohandler = s->filtered;
> > +}
> > +
> > +static void ar9331_do_stats_poll(struct work_struct *work)
> > +{
> > +
> 
> Could you remove this empty line.

fixed

Thank you!

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH v2 1/2] net: dsa: add optional stats64 support

[Andrew Lunn]

On Wed, Dec 02, 2020 at 01:07:11PM +0100, Oleksij Rempel wrote:
> Allow DSA drivers to export stats64
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH v2 2/2] net: dsa: qca: ar9331: export stats64

[Andrew Lunn]

> @@ -422,6 +527,7 @@ static void ar9331_sw_phylink_mac_link_down(struct dsa_switch *ds, int port,
>  					    phy_interface_t interface)
>  {
>  	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> +	struct ar9331_sw_port *p = &priv->port[port];
>  	struct regmap *regmap = priv->regmap;
>  	int ret;
>  
> @@ -429,6 +535,8 @@ static void ar9331_sw_phylink_mac_link_down(struct dsa_switch *ds, int port,
>  				 AR9331_SW_PORT_STATUS_MAC_MASK, 0);
>  	if (ret)
>  		dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> +
> +	cancel_delayed_work_sync(&p->mib_read);
>  }

You could update the stats here, after the interface is down. You then
know the stats are actually up to date and correct!

     Andrew

Re: [PATCH v2 2/2] net: dsa: qca: ar9331: export stats64

[Vladimir Oltean]

On Wed, Dec 02, 2020 at 01:07:12PM +0100, Oleksij Rempel wrote:
> Add stats support for the ar9331 switch.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---

With Andrew's feedback applied:

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

Re: [PATCH v2 1/2] net: dsa: add optional stats64 support

[Florian Fainelli]

On 12/2/2020 4:07 AM, Oleksij Rempel wrote:
> Allow DSA drivers to export stats64
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v2 2/2] net: dsa: qca: ar9331: export stats64

[Jakub Kicinski]

On Wed,  2 Dec 2020 13:07:12 +0100 Oleksij Rempel wrote:
> +static void ar9331_read_stats(struct ar9331_sw_port *port)
> +{
> +	mutex_lock(&port->lock);

> +	mutex_unlock(&port->lock);
> +}

> +static void ar9331_get_stats64(struct dsa_switch *ds, int port,
> +			       struct rtnl_link_stats64 *s)
> +{
> +	struct ar9331_sw_priv *priv = (struct ar9331_sw_priv *)ds->priv;
> +	struct ar9331_sw_port *p = &priv->port[port];
> +
> +	ar9331_read_stats(p);

> +}
> +
>  static const struct dsa_switch_ops ar9331_sw_ops = {

> +	.get_stats64		= ar9331_get_stats64,
>  };

You can't take sleeping locks from .ndo_get_stats64.

Also regmap may sleep?

+	ret = regmap_read(priv->regmap, reg, &val);

Am I missing something?