Re: [PATCH v2 2/2] net: dsa: qca: ar9331: export stats64
On Wed, 2 Dec 2020 13:07:12 +0100 Oleksij Rempel wrote:
> +static void ar9331_read_stats(struct ar9331_sw_port *port)
> +{
> + mutex_lock(>lock);
> + mutex_unlock(>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 = >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, );
Am I missing something?
Re: [PATCH v2 2/2] net: dsa: qca: ar9331: export stats64
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 > --- With Andrew's feedback applied: Reviewed-by: Vladimir Oltean
Re: [PATCH v2 2/2] net: dsa: qca: ar9331: export stats64
> @@ -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 = >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(>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
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
> > ---
> > /* 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 = >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(>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 = >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- |
Re: [PATCH v2 2/2] net: dsa: qca: ar9331: export stats64
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
> ---
> /* 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 = >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(>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 = >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
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- |
signature.asc
Description: OpenPGP digital signature
Re: [PATCH v2 2/2] net: dsa: qca: ar9331: export stats64
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
> > ---
> > 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) (0x2 + ((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
Re: [PATCH v2 2/2] net: dsa: qca: ar9331: export stats64
On 12/2/20 1:07 PM, Oleksij Rempel wrote:
> Add stats support for the ar9331 switch.
>
> Signed-off-by: Oleksij Rempel
> ---
> 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)(0x2 + ((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;
> +
