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@xxxxxxxxxxxxxx> > > --- > > /* 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 |