diff mbox series

[v5,2/2] net: dsa: qca: ar9331: export stats64

Message ID 20201211105322.7818-3-o.rempel@pengutronix.de (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series net: dsa: add stats64 support | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning CHECK: Macro argument '_reg' may be better as '(_reg)' to avoid precedence issues CHECK: Macro argument '_stats' may be better as '(_stats)' to avoid precedence issues WARNING: networking block comments don't use an empty /* line, use /* Comment...
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Oleksij Rempel Dec. 11, 2020, 10:53 a.m. UTC
Add stats support for the ar9331 switch.

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

Comments

Vladimir Oltean Dec. 12, 2020, 1:48 p.m. UTC | #1
On Fri, Dec 11, 2020 at 11:53:22AM +0100, 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 | 256 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 255 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
> index 4d49c5f2b790..5baef0ec6410 100644
> --- a/drivers/net/dsa/qca/ar9331.c
> +++ b/drivers/net/dsa/qca/ar9331.c
> @@ -101,6 +101,9 @@
>  	 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))
> +
>  /* Phy bypass mode
>   * ------------------------------------------------------------------------
>   * Bit:   | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 |10 |11 |12 |13 |14 |15 |
> @@ -154,6 +157,111 @@
>  #define AR9331_SW_MDIO_POLL_SLEEP_US		1
>  #define AR9331_SW_MDIO_POLL_TIMEOUT_US		20
>  
> +/* The interval should be small enough to avoid overflow of 32bit MIBs */
> +/*
> + * FIXME: as long as we can't read MIBs from stats64 call directly, we should
> + * poll stats more frequently then it is actually needed. In normal case
> + * 100 sec interval should be OK.
> + */
> +#define STATS_INTERVAL_JIFFIES			(3 * HZ)
> +
> +struct ar9331_sw_stats_raw {
> +	u32 rxbroad;			/* 0x00 */

You should probably use all tabs, or all spaces.

> +	u32 rxpause;                    /* 0x04 */
> +	u32 rxmulti;                    /* 0x08 */
> +	u32 rxfcserr;                   /* 0x0c */
> +	u32 rxalignerr;                 /* 0x10 */
> +	u32 rxrunt;                     /* 0x14 */
> +	u32 rxfragment;                 /* 0x18 */
> +	u32 rx64byte;                   /* 0x1c */
> +	u32 rx128byte;                  /* 0x20 */
> +	u32 rx256byte;                  /* 0x24 */
> +	u32 rx512byte;                  /* 0x28 */
> +	u32 rx1024byte;                 /* 0x2c */
> +	u32 rx1518byte;                 /* 0x30 */
> +	u32 rxmaxbyte;                  /* 0x34 */
> +	u32 rxtoolong;                  /* 0x38 */
> +	u32 rxgoodbyte;                 /* 0x3c */
> +	u32 rxgoodbyte_hi;
> +	u32 rxbadbyte;                  /* 0x44 */
> +	u32 rxbadbyte_hi;
> +	u32 rxoverflow;                 /* 0x4c */
> +	u32 filtered;                   /* 0x50 */
> +	u32 txbroad;                    /* 0x54 */
> +	u32 txpause;                    /* 0x58 */
> +	u32 txmulti;                    /* 0x5c */
> +	u32 txunderrun;                 /* 0x60 */
> +	u32 tx64byte;                   /* 0x64 */
> +	u32 tx128byte;                  /* 0x68 */
> +	u32 tx256byte;                  /* 0x6c */
> +	u32 tx512byte;                  /* 0x70 */
> +	u32 tx1024byte;                 /* 0x74 */
> +	u32 tx1518byte;                 /* 0x78 */
> +	u32 txmaxbyte;                  /* 0x7c */
> +	u32 txoversize;                 /* 0x80 */
> +	u32 txbyte;                     /* 0x84 */
> +	u32 txbyte_hi;
> +	u32 txcollision;                /* 0x8c */
> +	u32 txabortcol;                 /* 0x90 */
> +	u32 txmulticol;                 /* 0x94 */
> +	u32 txsinglecol;                /* 0x98 */
> +	u32 txexcdefer;                 /* 0x9c */
> +	u32 txdefer;                    /* 0xa0 */
> +	u32 txlatecol;                  /* 0xa4 */
> +};
> +
> +struct ar9331_sw_stats {
> +	u64_stats_t rxbroad;
> +	u64_stats_t rxpause;
> +	u64_stats_t rxmulti;
> +	u64_stats_t rxfcserr;
> +	u64_stats_t rxalignerr;
> +	u64_stats_t rxrunt;
> +	u64_stats_t rxfragment;
> +	u64_stats_t rx64byte;
> +	u64_stats_t rx128byte;
> +	u64_stats_t rx256byte;
> +	u64_stats_t rx512byte;
> +	u64_stats_t rx1024byte;
> +	u64_stats_t rx1518byte;
> +	u64_stats_t rxmaxbyte;
> +	u64_stats_t rxtoolong;
> +	u64_stats_t rxgoodbyte;
> +	u64_stats_t rxbadbyte;
> +	u64_stats_t rxoverflow;
> +	u64_stats_t filtered;
> +	u64_stats_t txbroad;
> +	u64_stats_t txpause;
> +	u64_stats_t txmulti;
> +	u64_stats_t txunderrun;
> +	u64_stats_t tx64byte;
> +	u64_stats_t tx128byte;
> +	u64_stats_t tx256byte;
> +	u64_stats_t tx512byte;
> +	u64_stats_t tx1024byte;
> +	u64_stats_t tx1518byte;
> +	u64_stats_t txmaxbyte;
> +	u64_stats_t txoversize;
> +	u64_stats_t txbyte;
> +	u64_stats_t txcollision;
> +	u64_stats_t txabortcol;
> +	u64_stats_t txmulticol;
> +	u64_stats_t txsinglecol;
> +	u64_stats_t txexcdefer;
> +	u64_stats_t txdefer;
> +	u64_stats_t txlatecol;
> +
> +	struct u64_stats_sync syncp;
> +};
> +
> +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 ar9331_sw_priv {
>  	struct device *dev;
>  	struct dsa_switch ds;
> @@ -165,6 +273,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
> @@ -424,6 +533,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;
>  
> @@ -431,6 +541,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,
> @@ -441,10 +553,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:
> @@ -477,6 +592,128 @@ static void ar9331_sw_phylink_mac_link_up(struct dsa_switch *ds, int port,
>  		dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
>  }
>  
> +#define AR9331_STATS_ADD(_stats, _raw, _reg) \
> +{ \
> +	u64_stats_add(&_stats->_reg, _raw._reg); \
> +}
> +
> +static void ar9331_read_stats(struct ar9331_sw_port *port)
> +{
> +	struct ar9331_sw_stats *stats = &port->stats;
> +	struct ar9331_sw_priv *priv = port->priv;

You can probably avoid a backpointer to *priv as long as you already
have int idx.

#define ar9331_port_to_priv(p) \
	container_of((p) - (p)->idx, struct ar9331_sw_port, port)

	struct ar9331_sw_priv *priv = ar9331_port_to_priv(p);

> +	struct ar9331_sw_stats_raw raw;
> +	int ret;
> +
> +	/* Do the slowest part first, to avoid needles locking for long time */

Unless this is needles as in "needles and pins", it should probably be
spelled "needless".

> +	ret = regmap_bulk_read(priv->regmap, AR9331_MIB_COUNTER(port->idx),
> +			       &raw, sizeof(raw) / sizeof(u32));
> +	if (ret) {
> +		dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
> +		return;
> +	}
> +	/* All MIB counters are cleared automatically on read */
> +
> +	u64_stats_update_begin(&stats->syncp);
> +
> +	AR9331_STATS_ADD(stats, raw, rxgoodbyte);
> +	AR9331_STATS_ADD(stats, raw, rxbroad);
> +	AR9331_STATS_ADD(stats, raw, rxpause);
> +	AR9331_STATS_ADD(stats, raw, rxmulti);
> +	AR9331_STATS_ADD(stats, raw, rxfcserr);
> +	AR9331_STATS_ADD(stats, raw, rxalignerr);
> +	AR9331_STATS_ADD(stats, raw, rxrunt);
> +	AR9331_STATS_ADD(stats, raw, rxfragment);
> +	AR9331_STATS_ADD(stats, raw, rx64byte);
> +	AR9331_STATS_ADD(stats, raw, rx128byte);
> +	AR9331_STATS_ADD(stats, raw, rx256byte);
> +	AR9331_STATS_ADD(stats, raw, rx512byte);
> +	AR9331_STATS_ADD(stats, raw, rx1024byte);
> +	AR9331_STATS_ADD(stats, raw, rx1518byte);
> +	AR9331_STATS_ADD(stats, raw, rxmaxbyte);
> +	AR9331_STATS_ADD(stats, raw, rxtoolong);
> +	AR9331_STATS_ADD(stats, raw, rxbadbyte);
> +	AR9331_STATS_ADD(stats, raw, rxoverflow);
> +	AR9331_STATS_ADD(stats, raw, filtered);
> +	AR9331_STATS_ADD(stats, raw, txbroad);
> +	AR9331_STATS_ADD(stats, raw, txpause);
> +	AR9331_STATS_ADD(stats, raw, txmulti);
> +	AR9331_STATS_ADD(stats, raw, txunderrun);
> +	AR9331_STATS_ADD(stats, raw, tx64byte);
> +	AR9331_STATS_ADD(stats, raw, tx128byte);
> +	AR9331_STATS_ADD(stats, raw, tx256byte);
> +	AR9331_STATS_ADD(stats, raw, tx512byte);
> +	AR9331_STATS_ADD(stats, raw, tx1024byte);
> +	AR9331_STATS_ADD(stats, raw, tx1518byte);
> +	AR9331_STATS_ADD(stats, raw, txmaxbyte);
> +	AR9331_STATS_ADD(stats, raw, txoversize);
> +	AR9331_STATS_ADD(stats, raw, txbyte);
> +	AR9331_STATS_ADD(stats, raw, txcollision);
> +	AR9331_STATS_ADD(stats, raw, txabortcol);
> +	AR9331_STATS_ADD(stats, raw, txmulticol);
> +	AR9331_STATS_ADD(stats, raw, txsinglecol);
> +	AR9331_STATS_ADD(stats, raw, txexcdefer);
> +	AR9331_STATS_ADD(stats, raw, txdefer);
> +	AR9331_STATS_ADD(stats, raw, txlatecol);
> +
> +	u64_stats_update_end(&stats->syncp);
> +}
> +
> +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 = u64_stats_read(&s->rx64byte) +
> +		u64_stats_read(&s->rx128byte) + u64_stats_read(&s->rx256byte) +
> +		u64_stats_read(&s->rx512byte) + u64_stats_read(&s->rx1024byte) +
> +		u64_stats_read(&s->rx1518byte) + u64_stats_read(&s->rxmaxbyte);
> +	stats->tx_packets = u64_stats_read(&s->tx64byte) +
> +		u64_stats_read(&s->tx128byte) + u64_stats_read(&s->tx256byte) +
> +		u64_stats_read(&s->tx512byte) + u64_stats_read(&s->tx1024byte) +
> +		u64_stats_read(&s->tx1518byte) + u64_stats_read(&s->txmaxbyte);
> +	stats->rx_bytes = u64_stats_read(&s->rxgoodbyte);
> +	stats->tx_bytes = u64_stats_read(&s->txbyte);
> +	stats->rx_errors = u64_stats_read(&s->rxfcserr) +
> +		u64_stats_read(&s->rxalignerr) + u64_stats_read(&s->rxrunt) +
> +		u64_stats_read(&s->rxfragment) + u64_stats_read(&s->rxoverflow);
> +	stats->tx_errors = u64_stats_read(&s->txoversize);

Should tx_errors not also include tx_aborted_errors, tx_fifo_errors,
tx_window_errors?

> +	stats->multicast = u64_stats_read(&s->rxmulti);
> +	stats->collisions = u64_stats_read(&s->txcollision);
> +	stats->rx_length_errors = u64_stats_read(&s->rxrunt) +
> +		u64_stats_read(&s->rxfragment) + u64_stats_read(&s->rxtoolong);
> +	stats->rx_crc_errors = u64_stats_read(&s->rxfcserr) +
> +		u64_stats_read(&s->rxalignerr) + u64_stats_read(&s->rxfragment);
> +	stats->rx_frame_errors = u64_stats_read(&s->rxalignerr);
> +	stats->rx_missed_errors = u64_stats_read(&s->rxoverflow);
> +	stats->tx_aborted_errors = u64_stats_read(&s->txabortcol);
> +	stats->tx_fifo_errors = u64_stats_read(&s->txunderrun);
> +	stats->tx_window_errors = u64_stats_read(&s->txlatecol);
> +	stats->rx_nohandler = u64_stats_read(&s->filtered);

Should rx_nohandler not be also included in rx_errors?
You can probably avoid reading a few of these twice by assigning the
more specific ones first, then the rx_errors and tx_errors at the end.

> +}
> +
> +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];
> +	unsigned int start;
> +
> +	do {
> +		start = u64_stats_fetch_begin(&p->stats.syncp);
> +		ar9331_stats_update(p, s);
> +	} while (u64_stats_fetch_retry(&p->stats.syncp, start));
> +}
> +
>  static const struct dsa_switch_ops ar9331_sw_ops = {
>  	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
>  	.setup			= ar9331_sw_setup,
> @@ -485,6 +722,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)
> @@ -796,7 +1034,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)
> @@ -831,6 +1069,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;
> +		u64_stats_init(&port->stats.syncp);
> +		INIT_DELAYED_WORK(&port->mib_read, ar9331_do_stats_poll);
> +	}
> +
>  	ret = dsa_register_switch(ds);
>  	if (ret)
>  		goto err_remove_irq;
> @@ -846,6 +1093,13 @@ static int ar9331_sw_probe(struct mdio_device *mdiodev)
>  static void ar9331_sw_remove(struct mdio_device *mdiodev)
>  {
>  	struct ar9331_sw_priv *priv = dev_get_drvdata(&mdiodev->dev);
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(priv->port); i++) {
> +		struct ar9331_sw_port *port = &priv->port[i];
> +
> +		cancel_delayed_work_sync(&port->mib_read);
> +	}
>  
>  	irq_domain_remove(priv->irqdomain);
>  	mdiobus_unregister(priv->mbus);
> -- 
> 2.29.2
>
Jakub Kicinski Dec. 12, 2020, 5:34 p.m. UTC | #2
On Fri, 11 Dec 2020 11:53:22 +0100 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 | 256 ++++++++++++++++++++++++++++++++++-
>  1 file changed, 255 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
> index 4d49c5f2b790..5baef0ec6410 100644
> --- a/drivers/net/dsa/qca/ar9331.c
> +++ b/drivers/net/dsa/qca/ar9331.c
> @@ -101,6 +101,9 @@
>  	 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))
> +
>  /* Phy bypass mode
>   * ------------------------------------------------------------------------
>   * Bit:   | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 |10 |11 |12 |13 |14 |15 |
> @@ -154,6 +157,111 @@
>  #define AR9331_SW_MDIO_POLL_SLEEP_US		1
>  #define AR9331_SW_MDIO_POLL_TIMEOUT_US		20
>  
> +/* The interval should be small enough to avoid overflow of 32bit MIBs */
> +/*
> + * FIXME: as long as we can't read MIBs from stats64 call directly, we should
> + * poll stats more frequently then it is actually needed. In normal case
> + * 100 sec interval should be OK.

This comment is a little confusing, if you don't mind.

Should it says something like:

 FIXME: until we can read MIBs from stats64 call directly (i.e. sleep
 there), we have to poll stats more frequently then it is actually needed.
 For overflow protection, normally, 100 sec interval should have been OK.
Jakub Kicinski Dec. 12, 2020, 6 p.m. UTC | #3
On Sat, 12 Dec 2020 15:48:52 +0200 Vladimir Oltean wrote:
> > +	stats->rx_packets = u64_stats_read(&s->rx64byte) +
> > +		u64_stats_read(&s->rx128byte) + u64_stats_read(&s->rx256byte) +
> > +		u64_stats_read(&s->rx512byte) + u64_stats_read(&s->rx1024byte) +
> > +		u64_stats_read(&s->rx1518byte) + u64_stats_read(&s->rxmaxbyte);
> > +	stats->tx_packets = u64_stats_read(&s->tx64byte) +
> > +		u64_stats_read(&s->tx128byte) + u64_stats_read(&s->tx256byte) +
> > +		u64_stats_read(&s->tx512byte) + u64_stats_read(&s->tx1024byte) +
> > +		u64_stats_read(&s->tx1518byte) + u64_stats_read(&s->txmaxbyte);
> > +	stats->rx_bytes = u64_stats_read(&s->rxgoodbyte);
> > +	stats->tx_bytes = u64_stats_read(&s->txbyte);
> > +	stats->rx_errors = u64_stats_read(&s->rxfcserr) +
> > +		u64_stats_read(&s->rxalignerr) + u64_stats_read(&s->rxrunt) +
> > +		u64_stats_read(&s->rxfragment) + u64_stats_read(&s->rxoverflow);
> > +	stats->tx_errors = u64_stats_read(&s->txoversize);  
> 
> Should tx_errors not also include tx_aborted_errors, tx_fifo_errors,
> tx_window_errors?

Yes.

> > +	stats->multicast = u64_stats_read(&s->rxmulti);
> > +	stats->collisions = u64_stats_read(&s->txcollision);
> > +	stats->rx_length_errors = u64_stats_read(&s->rxrunt) +
> > +		u64_stats_read(&s->rxfragment) + u64_stats_read(&s->rxtoolong);
> > +	stats->rx_crc_errors = u64_stats_read(&s->rxfcserr) +
> > +		u64_stats_read(&s->rxalignerr) + u64_stats_read(&s->rxfragment);

Why would CRC errors include alignment errors and rxfragments?

Besides looks like rxfragment is already counted to length errors.

> > +	stats->rx_frame_errors = u64_stats_read(&s->rxalignerr);
> > +	stats->rx_missed_errors = u64_stats_read(&s->rxoverflow);
> > +	stats->tx_aborted_errors = u64_stats_read(&s->txabortcol);
> > +	stats->tx_fifo_errors = u64_stats_read(&s->txunderrun);
> > +	stats->tx_window_errors = u64_stats_read(&s->txlatecol);
> > +	stats->rx_nohandler = u64_stats_read(&s->filtered);  
> 
> Should rx_nohandler not be also included in rx_errors?

I don't think drivers should ever touch rx_nohandler, it's a pretty
specific SW stat. But you made me realize that we never specified where
to count frames discarded due to L2 address filtering. It appears that
high speed adapters I was looking at don't have such statistic?

I would go with rx_dropped, if that's what ->filtered is.

We should probably update the doc like this:

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 874cc12a34d9..82708c6db432 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -75,8 +75,9 @@ struct rtnl_link_stats {
  *
  * @rx_dropped: Number of packets received but not processed,
  *   e.g. due to lack of resources or unsupported protocol.
- *   For hardware interfaces this counter should not include packets
- *   dropped by the device which are counted separately in
+ *   For hardware interfaces this counter may include packets discarded
+ *   due to L2 address filtering but should not include packets dropped
+ *   by the device due to buffer exhaustion which are counted separately in
  *   @rx_missed_errors (since procfs folds those two counters together).
  *
  * @tx_dropped: Number of packets dropped on their way to transmission,


> You can probably avoid reading a few of these twice by assigning the
> more specific ones first, then the rx_errors and tx_errors at the end.
diff mbox series

Patch

diff --git a/drivers/net/dsa/qca/ar9331.c b/drivers/net/dsa/qca/ar9331.c
index 4d49c5f2b790..5baef0ec6410 100644
--- a/drivers/net/dsa/qca/ar9331.c
+++ b/drivers/net/dsa/qca/ar9331.c
@@ -101,6 +101,9 @@ 
 	 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))
+
 /* Phy bypass mode
  * ------------------------------------------------------------------------
  * Bit:   | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 |10 |11 |12 |13 |14 |15 |
@@ -154,6 +157,111 @@ 
 #define AR9331_SW_MDIO_POLL_SLEEP_US		1
 #define AR9331_SW_MDIO_POLL_TIMEOUT_US		20
 
+/* The interval should be small enough to avoid overflow of 32bit MIBs */
+/*
+ * FIXME: as long as we can't read MIBs from stats64 call directly, we should
+ * poll stats more frequently then it is actually needed. In normal case
+ * 100 sec interval should be OK.
+ */
+#define STATS_INTERVAL_JIFFIES			(3 * HZ)
+
+struct ar9331_sw_stats_raw {
+	u32 rxbroad;			/* 0x00 */
+	u32 rxpause;                    /* 0x04 */
+	u32 rxmulti;                    /* 0x08 */
+	u32 rxfcserr;                   /* 0x0c */
+	u32 rxalignerr;                 /* 0x10 */
+	u32 rxrunt;                     /* 0x14 */
+	u32 rxfragment;                 /* 0x18 */
+	u32 rx64byte;                   /* 0x1c */
+	u32 rx128byte;                  /* 0x20 */
+	u32 rx256byte;                  /* 0x24 */
+	u32 rx512byte;                  /* 0x28 */
+	u32 rx1024byte;                 /* 0x2c */
+	u32 rx1518byte;                 /* 0x30 */
+	u32 rxmaxbyte;                  /* 0x34 */
+	u32 rxtoolong;                  /* 0x38 */
+	u32 rxgoodbyte;                 /* 0x3c */
+	u32 rxgoodbyte_hi;
+	u32 rxbadbyte;                  /* 0x44 */
+	u32 rxbadbyte_hi;
+	u32 rxoverflow;                 /* 0x4c */
+	u32 filtered;                   /* 0x50 */
+	u32 txbroad;                    /* 0x54 */
+	u32 txpause;                    /* 0x58 */
+	u32 txmulti;                    /* 0x5c */
+	u32 txunderrun;                 /* 0x60 */
+	u32 tx64byte;                   /* 0x64 */
+	u32 tx128byte;                  /* 0x68 */
+	u32 tx256byte;                  /* 0x6c */
+	u32 tx512byte;                  /* 0x70 */
+	u32 tx1024byte;                 /* 0x74 */
+	u32 tx1518byte;                 /* 0x78 */
+	u32 txmaxbyte;                  /* 0x7c */
+	u32 txoversize;                 /* 0x80 */
+	u32 txbyte;                     /* 0x84 */
+	u32 txbyte_hi;
+	u32 txcollision;                /* 0x8c */
+	u32 txabortcol;                 /* 0x90 */
+	u32 txmulticol;                 /* 0x94 */
+	u32 txsinglecol;                /* 0x98 */
+	u32 txexcdefer;                 /* 0x9c */
+	u32 txdefer;                    /* 0xa0 */
+	u32 txlatecol;                  /* 0xa4 */
+};
+
+struct ar9331_sw_stats {
+	u64_stats_t rxbroad;
+	u64_stats_t rxpause;
+	u64_stats_t rxmulti;
+	u64_stats_t rxfcserr;
+	u64_stats_t rxalignerr;
+	u64_stats_t rxrunt;
+	u64_stats_t rxfragment;
+	u64_stats_t rx64byte;
+	u64_stats_t rx128byte;
+	u64_stats_t rx256byte;
+	u64_stats_t rx512byte;
+	u64_stats_t rx1024byte;
+	u64_stats_t rx1518byte;
+	u64_stats_t rxmaxbyte;
+	u64_stats_t rxtoolong;
+	u64_stats_t rxgoodbyte;
+	u64_stats_t rxbadbyte;
+	u64_stats_t rxoverflow;
+	u64_stats_t filtered;
+	u64_stats_t txbroad;
+	u64_stats_t txpause;
+	u64_stats_t txmulti;
+	u64_stats_t txunderrun;
+	u64_stats_t tx64byte;
+	u64_stats_t tx128byte;
+	u64_stats_t tx256byte;
+	u64_stats_t tx512byte;
+	u64_stats_t tx1024byte;
+	u64_stats_t tx1518byte;
+	u64_stats_t txmaxbyte;
+	u64_stats_t txoversize;
+	u64_stats_t txbyte;
+	u64_stats_t txcollision;
+	u64_stats_t txabortcol;
+	u64_stats_t txmulticol;
+	u64_stats_t txsinglecol;
+	u64_stats_t txexcdefer;
+	u64_stats_t txdefer;
+	u64_stats_t txlatecol;
+
+	struct u64_stats_sync syncp;
+};
+
+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 ar9331_sw_priv {
 	struct device *dev;
 	struct dsa_switch ds;
@@ -165,6 +273,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
@@ -424,6 +533,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;
 
@@ -431,6 +541,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,
@@ -441,10 +553,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:
@@ -477,6 +592,128 @@  static void ar9331_sw_phylink_mac_link_up(struct dsa_switch *ds, int port,
 		dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
 }
 
+#define AR9331_STATS_ADD(_stats, _raw, _reg) \
+{ \
+	u64_stats_add(&_stats->_reg, _raw._reg); \
+}
+
+static void ar9331_read_stats(struct ar9331_sw_port *port)
+{
+	struct ar9331_sw_stats *stats = &port->stats;
+	struct ar9331_sw_priv *priv = port->priv;
+	struct ar9331_sw_stats_raw raw;
+	int ret;
+
+	/* Do the slowest part first, to avoid needles locking for long time */
+	ret = regmap_bulk_read(priv->regmap, AR9331_MIB_COUNTER(port->idx),
+			       &raw, sizeof(raw) / sizeof(u32));
+	if (ret) {
+		dev_err_ratelimited(priv->dev, "%s: %i\n", __func__, ret);
+		return;
+	}
+	/* All MIB counters are cleared automatically on read */
+
+	u64_stats_update_begin(&stats->syncp);
+
+	AR9331_STATS_ADD(stats, raw, rxgoodbyte);
+	AR9331_STATS_ADD(stats, raw, rxbroad);
+	AR9331_STATS_ADD(stats, raw, rxpause);
+	AR9331_STATS_ADD(stats, raw, rxmulti);
+	AR9331_STATS_ADD(stats, raw, rxfcserr);
+	AR9331_STATS_ADD(stats, raw, rxalignerr);
+	AR9331_STATS_ADD(stats, raw, rxrunt);
+	AR9331_STATS_ADD(stats, raw, rxfragment);
+	AR9331_STATS_ADD(stats, raw, rx64byte);
+	AR9331_STATS_ADD(stats, raw, rx128byte);
+	AR9331_STATS_ADD(stats, raw, rx256byte);
+	AR9331_STATS_ADD(stats, raw, rx512byte);
+	AR9331_STATS_ADD(stats, raw, rx1024byte);
+	AR9331_STATS_ADD(stats, raw, rx1518byte);
+	AR9331_STATS_ADD(stats, raw, rxmaxbyte);
+	AR9331_STATS_ADD(stats, raw, rxtoolong);
+	AR9331_STATS_ADD(stats, raw, rxbadbyte);
+	AR9331_STATS_ADD(stats, raw, rxoverflow);
+	AR9331_STATS_ADD(stats, raw, filtered);
+	AR9331_STATS_ADD(stats, raw, txbroad);
+	AR9331_STATS_ADD(stats, raw, txpause);
+	AR9331_STATS_ADD(stats, raw, txmulti);
+	AR9331_STATS_ADD(stats, raw, txunderrun);
+	AR9331_STATS_ADD(stats, raw, tx64byte);
+	AR9331_STATS_ADD(stats, raw, tx128byte);
+	AR9331_STATS_ADD(stats, raw, tx256byte);
+	AR9331_STATS_ADD(stats, raw, tx512byte);
+	AR9331_STATS_ADD(stats, raw, tx1024byte);
+	AR9331_STATS_ADD(stats, raw, tx1518byte);
+	AR9331_STATS_ADD(stats, raw, txmaxbyte);
+	AR9331_STATS_ADD(stats, raw, txoversize);
+	AR9331_STATS_ADD(stats, raw, txbyte);
+	AR9331_STATS_ADD(stats, raw, txcollision);
+	AR9331_STATS_ADD(stats, raw, txabortcol);
+	AR9331_STATS_ADD(stats, raw, txmulticol);
+	AR9331_STATS_ADD(stats, raw, txsinglecol);
+	AR9331_STATS_ADD(stats, raw, txexcdefer);
+	AR9331_STATS_ADD(stats, raw, txdefer);
+	AR9331_STATS_ADD(stats, raw, txlatecol);
+
+	u64_stats_update_end(&stats->syncp);
+}
+
+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 = u64_stats_read(&s->rx64byte) +
+		u64_stats_read(&s->rx128byte) + u64_stats_read(&s->rx256byte) +
+		u64_stats_read(&s->rx512byte) + u64_stats_read(&s->rx1024byte) +
+		u64_stats_read(&s->rx1518byte) + u64_stats_read(&s->rxmaxbyte);
+	stats->tx_packets = u64_stats_read(&s->tx64byte) +
+		u64_stats_read(&s->tx128byte) + u64_stats_read(&s->tx256byte) +
+		u64_stats_read(&s->tx512byte) + u64_stats_read(&s->tx1024byte) +
+		u64_stats_read(&s->tx1518byte) + u64_stats_read(&s->txmaxbyte);
+	stats->rx_bytes = u64_stats_read(&s->rxgoodbyte);
+	stats->tx_bytes = u64_stats_read(&s->txbyte);
+	stats->rx_errors = u64_stats_read(&s->rxfcserr) +
+		u64_stats_read(&s->rxalignerr) + u64_stats_read(&s->rxrunt) +
+		u64_stats_read(&s->rxfragment) + u64_stats_read(&s->rxoverflow);
+	stats->tx_errors = u64_stats_read(&s->txoversize);
+	stats->multicast = u64_stats_read(&s->rxmulti);
+	stats->collisions = u64_stats_read(&s->txcollision);
+	stats->rx_length_errors = u64_stats_read(&s->rxrunt) +
+		u64_stats_read(&s->rxfragment) + u64_stats_read(&s->rxtoolong);
+	stats->rx_crc_errors = u64_stats_read(&s->rxfcserr) +
+		u64_stats_read(&s->rxalignerr) + u64_stats_read(&s->rxfragment);
+	stats->rx_frame_errors = u64_stats_read(&s->rxalignerr);
+	stats->rx_missed_errors = u64_stats_read(&s->rxoverflow);
+	stats->tx_aborted_errors = u64_stats_read(&s->txabortcol);
+	stats->tx_fifo_errors = u64_stats_read(&s->txunderrun);
+	stats->tx_window_errors = u64_stats_read(&s->txlatecol);
+	stats->rx_nohandler = u64_stats_read(&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];
+	unsigned int start;
+
+	do {
+		start = u64_stats_fetch_begin(&p->stats.syncp);
+		ar9331_stats_update(p, s);
+	} while (u64_stats_fetch_retry(&p->stats.syncp, start));
+}
+
 static const struct dsa_switch_ops ar9331_sw_ops = {
 	.get_tag_protocol	= ar9331_sw_get_tag_protocol,
 	.setup			= ar9331_sw_setup,
@@ -485,6 +722,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)
@@ -796,7 +1034,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)
@@ -831,6 +1069,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;
+		u64_stats_init(&port->stats.syncp);
+		INIT_DELAYED_WORK(&port->mib_read, ar9331_do_stats_poll);
+	}
+
 	ret = dsa_register_switch(ds);
 	if (ret)
 		goto err_remove_irq;
@@ -846,6 +1093,13 @@  static int ar9331_sw_probe(struct mdio_device *mdiodev)
 static void ar9331_sw_remove(struct mdio_device *mdiodev)
 {
 	struct ar9331_sw_priv *priv = dev_get_drvdata(&mdiodev->dev);
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(priv->port); i++) {
+		struct ar9331_sw_port *port = &priv->port[i];
+
+		cancel_delayed_work_sync(&port->mib_read);
+	}
 
 	irq_domain_remove(priv->irqdomain);
 	mdiobus_unregister(priv->mbus);