[PATCH net-next v1 1/1] net: dsa: microchip: add stats64 support for ksz8 series of switches

[PATCH net-next v1 1/1] net: dsa: microchip: add stats64 support for ksz8 series of switches

[Oleksij Rempel]

Add stats64 support for ksz8xxx series of switches.

Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
---
 drivers/net/dsa/microchip/ksz_common.c | 87 ++++++++++++++++++++++++++
 drivers/net/dsa/microchip/ksz_common.h |  1 +
 2 files changed, 88 insertions(+)

diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index f39b041765fb..423f944cc34c 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -70,6 +70,43 @@ struct ksz_stats_raw {
 	u64 tx_discards;
 };
 
+struct ksz88xx_stats_raw {
+	u64 rx;
+	u64 rx_hi;
+	u64 rx_undersize;
+	u64 rx_fragments;
+	u64 rx_oversize;
+	u64 rx_jabbers;
+	u64 rx_symbol_err;
+	u64 rx_crc_err;
+	u64 rx_align_err;
+	u64 rx_mac_ctrl;
+	u64 rx_pause;
+	u64 rx_bcast;
+	u64 rx_mcast;
+	u64 rx_ucast;
+	u64 rx_64_or_less;
+	u64 rx_65_127;
+	u64 rx_128_255;
+	u64 rx_256_511;
+	u64 rx_512_1023;
+	u64 rx_1024_1522;
+	u64 tx;
+	u64 tx_hi;
+	u64 tx_late_col;
+	u64 tx_pause;
+	u64 tx_bcast;
+	u64 tx_mcast;
+	u64 tx_ucast;
+	u64 tx_deferred;
+	u64 tx_total_col;
+	u64 tx_exc_col;
+	u64 tx_single_col;
+	u64 tx_mult_col;
+	u64 rx_discards;
+	u64 tx_discards;
+};
+
 static const struct ksz_mib_names ksz88xx_mib_names[] = {
 	{ 0x00, "rx" },
 	{ 0x01, "rx_hi" },
@@ -156,6 +193,7 @@ static const struct ksz_dev_ops ksz8_dev_ops = {
 	.w_phy = ksz8_w_phy,
 	.r_mib_cnt = ksz8_r_mib_cnt,
 	.r_mib_pkt = ksz8_r_mib_pkt,
+	.r_mib_stat64 = ksz88xx_r_mib_stats64,
 	.freeze_mib = ksz8_freeze_mib,
 	.port_init_cnt = ksz8_port_init_cnt,
 	.fdb_dump = ksz8_fdb_dump,
@@ -1583,6 +1621,55 @@ void ksz_r_mib_stats64(struct ksz_device *dev, int port)
 	spin_unlock(&mib->stats64_lock);
 }
 
+void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port)
+{
+	struct ethtool_pause_stats *pstats;
+	struct rtnl_link_stats64 *stats;
+	struct ksz88xx_stats_raw *raw;
+	struct ksz_port_mib *mib;
+
+	mib = &dev->ports[port].mib;
+	stats = &mib->stats64;
+	pstats = &mib->pause_stats;
+	raw = (struct ksz88xx_stats_raw *)mib->counters;
+
+	spin_lock(&mib->stats64_lock);
+
+	stats->rx_packets = raw->rx_bcast + raw->rx_mcast + raw->rx_ucast +
+		raw->rx_pause;
+	stats->tx_packets = raw->tx_bcast + raw->tx_mcast + raw->tx_ucast +
+		raw->tx_pause;
+
+	/* HW counters are counting bytes + FCS which is not acceptable
+	 * for rtnl_link_stats64 interface
+	 */
+	stats->rx_bytes = raw->rx + raw->rx_hi - stats->rx_packets * ETH_FCS_LEN;
+	stats->tx_bytes = raw->tx + raw->tx_hi - stats->tx_packets * ETH_FCS_LEN;
+
+	stats->rx_length_errors = raw->rx_undersize + raw->rx_fragments +
+		raw->rx_oversize;
+
+	stats->rx_crc_errors = raw->rx_crc_err;
+	stats->rx_frame_errors = raw->rx_align_err;
+	stats->rx_dropped = raw->rx_discards;
+	stats->rx_errors = stats->rx_length_errors + stats->rx_crc_errors +
+		stats->rx_frame_errors  + stats->rx_dropped;
+
+	stats->tx_window_errors = raw->tx_late_col;
+	stats->tx_fifo_errors = raw->tx_discards;
+	stats->tx_aborted_errors = raw->tx_exc_col;
+	stats->tx_errors = stats->tx_window_errors + stats->tx_fifo_errors +
+		stats->tx_aborted_errors;
+
+	stats->multicast = raw->rx_mcast;
+	stats->collisions = raw->tx_total_col;
+
+	pstats->tx_pause_frames = raw->tx_pause;
+	pstats->rx_pause_frames = raw->rx_pause;
+
+	spin_unlock(&mib->stats64_lock);
+}
+
 static void ksz_get_stats64(struct dsa_switch *ds, int port,
 			    struct rtnl_link_stats64 *s)
 {
diff --git a/drivers/net/dsa/microchip/ksz_common.h b/drivers/net/dsa/microchip/ksz_common.h
index cb27f5a180c7..055d61ff3fb8 100644
--- a/drivers/net/dsa/microchip/ksz_common.h
+++ b/drivers/net/dsa/microchip/ksz_common.h
@@ -345,6 +345,7 @@ void ksz_switch_remove(struct ksz_device *dev);
 
 void ksz_init_mib_timer(struct ksz_device *dev);
 void ksz_r_mib_stats64(struct ksz_device *dev, int port);
+void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port);
 void ksz_port_stp_state_set(struct dsa_switch *ds, int port, u8 state);
 bool ksz_get_gbit(struct ksz_device *dev, int port);
 phy_interface_t ksz_get_xmii(struct ksz_device *dev, int port, bool gbit);
-- 
2.30.2

Re: [PATCH net-next v1 1/1] net: dsa: microchip: add stats64 support for ksz8 series of switches

[Vladimir Oltean]

On Mon, Dec 05, 2022 at 06:29:04AM +0100, Oleksij Rempel wrote:
> +void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port)
> +{
> +	struct ethtool_pause_stats *pstats;
> +	struct rtnl_link_stats64 *stats;
> +	struct ksz88xx_stats_raw *raw;
> +	struct ksz_port_mib *mib;
> +
> +	mib = &dev->ports[port].mib;
> +	stats = &mib->stats64;
> +	pstats = &mib->pause_stats;
> +	raw = (struct ksz88xx_stats_raw *)mib->counters;
> +
> +	spin_lock(&mib->stats64_lock);
> +
> +	stats->rx_packets = raw->rx_bcast + raw->rx_mcast + raw->rx_ucast +
> +		raw->rx_pause;
> +	stats->tx_packets = raw->tx_bcast + raw->tx_mcast + raw->tx_ucast +
> +		raw->tx_pause;
> +
> +	/* HW counters are counting bytes + FCS which is not acceptable
> +	 * for rtnl_link_stats64 interface
> +	 */
> +	stats->rx_bytes = raw->rx + raw->rx_hi - stats->rx_packets * ETH_FCS_LEN;
> +	stats->tx_bytes = raw->tx + raw->tx_hi - stats->tx_packets * ETH_FCS_LEN;

What are rx_hi, tx_hi compared to rx, tx?

> +
> +	stats->rx_length_errors = raw->rx_undersize + raw->rx_fragments +
> +		raw->rx_oversize;
> +
> +	stats->rx_crc_errors = raw->rx_crc_err;
> +	stats->rx_frame_errors = raw->rx_align_err;
> +	stats->rx_dropped = raw->rx_discards;
> +	stats->rx_errors = stats->rx_length_errors + stats->rx_crc_errors +
> +		stats->rx_frame_errors  + stats->rx_dropped;
> +
> +	stats->tx_window_errors = raw->tx_late_col;
> +	stats->tx_fifo_errors = raw->tx_discards;
> +	stats->tx_aborted_errors = raw->tx_exc_col;
> +	stats->tx_errors = stats->tx_window_errors + stats->tx_fifo_errors +
> +		stats->tx_aborted_errors;
> +
> +	stats->multicast = raw->rx_mcast;
> +	stats->collisions = raw->tx_total_col;
> +
> +	pstats->tx_pause_frames = raw->tx_pause;
> +	pstats->rx_pause_frames = raw->rx_pause;

FWIW, ksz_get_pause_stats() can sleep, just ksz_get_stats64() can't. So
the pause stats don't need to be periodically read (unless you want to
do that to prevent 32-bit overflows).

> +
> +	spin_unlock(&mib->stats64_lock);
> +}

Re: [PATCH net-next v1 1/1] net: dsa: microchip: add stats64 support for ksz8 series of switches

[Jakub Kicinski]

On Mon,  5 Dec 2022 06:29:04 +0100 Oleksij Rempel wrote:
> +	stats->rx_packets = raw->rx_bcast + raw->rx_mcast + raw->rx_ucast +
> +		raw->rx_pause;
> +	stats->tx_packets = raw->tx_bcast + raw->tx_mcast + raw->tx_ucast +
> +		raw->tx_pause;

FWIW for normal netdevs / NICs the rtnl_link_stat pkts do not include
pause frames, normally. Otherwise one can't maintain those stats in SW
(and per-ring stats, if any, don't add up to the full link stats).
But if you have a good reason to do this - I won't nack..

Re: [PATCH net-next v1 1/1] net: dsa: microchip: add stats64 support for ksz8 series of switches

[Oleksij Rempel]

On Tue, Dec 06, 2022 at 07:08:01PM +0200, Vladimir Oltean wrote:
> On Mon, Dec 05, 2022 at 06:29:04AM +0100, Oleksij Rempel wrote:
> > +void ksz88xx_r_mib_stats64(struct ksz_device *dev, int port)
> > +{
> > +	struct ethtool_pause_stats *pstats;
> > +	struct rtnl_link_stats64 *stats;
> > +	struct ksz88xx_stats_raw *raw;
> > +	struct ksz_port_mib *mib;
> > +
> > +	mib = &dev->ports[port].mib;
> > +	stats = &mib->stats64;
> > +	pstats = &mib->pause_stats;
> > +	raw = (struct ksz88xx_stats_raw *)mib->counters;
> > +
> > +	spin_lock(&mib->stats64_lock);
> > +
> > +	stats->rx_packets = raw->rx_bcast + raw->rx_mcast + raw->rx_ucast +
> > +		raw->rx_pause;
> > +	stats->tx_packets = raw->tx_bcast + raw->tx_mcast + raw->tx_ucast +
> > +		raw->tx_pause;
> > +
> > +	/* HW counters are counting bytes + FCS which is not acceptable
> > +	 * for rtnl_link_stats64 interface
> > +	 */
> > +	stats->rx_bytes = raw->rx + raw->rx_hi - stats->rx_packets * ETH_FCS_LEN;
> > +	stats->tx_bytes = raw->tx + raw->tx_hi - stats->tx_packets * ETH_FCS_LEN;
> 
> What are rx_hi, tx_hi compared to rx, tx?

rx, tx are packets with normal priority and rx_hi, tx_hi are packets
with high prio.

> > +
> > +	stats->rx_length_errors = raw->rx_undersize + raw->rx_fragments +
> > +		raw->rx_oversize;
> > +
> > +	stats->rx_crc_errors = raw->rx_crc_err;
> > +	stats->rx_frame_errors = raw->rx_align_err;
> > +	stats->rx_dropped = raw->rx_discards;
> > +	stats->rx_errors = stats->rx_length_errors + stats->rx_crc_errors +
> > +		stats->rx_frame_errors  + stats->rx_dropped;
> > +
> > +	stats->tx_window_errors = raw->tx_late_col;
> > +	stats->tx_fifo_errors = raw->tx_discards;
> > +	stats->tx_aborted_errors = raw->tx_exc_col;
> > +	stats->tx_errors = stats->tx_window_errors + stats->tx_fifo_errors +
> > +		stats->tx_aborted_errors;
> > +
> > +	stats->multicast = raw->rx_mcast;
> > +	stats->collisions = raw->tx_total_col;
> > +
> > +	pstats->tx_pause_frames = raw->tx_pause;
> > +	pstats->rx_pause_frames = raw->rx_pause;
> 
> FWIW, ksz_get_pause_stats() can sleep, just ksz_get_stats64() can't. So
> the pause stats don't need to be periodically read (unless you want to
> do that to prevent 32-bit overflows).

KSZ driver is using worker to read stats periodically. Since all needed
locks are already taken, I copy pause stats as well.

Otherwise it will need some different locking, which will make things
look different but do not reduce CPU load. 

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 net-next v1 1/1] net: dsa: microchip: add stats64 support for ksz8 series of switches

[Oleksij Rempel]

On Tue, Dec 06, 2022 at 11:41:33AM -0800, Jakub Kicinski wrote:
> On Mon,  5 Dec 2022 06:29:04 +0100 Oleksij Rempel wrote:
> > +	stats->rx_packets = raw->rx_bcast + raw->rx_mcast + raw->rx_ucast +
> > +		raw->rx_pause;
> > +	stats->tx_packets = raw->tx_bcast + raw->tx_mcast + raw->tx_ucast +
> > +		raw->tx_pause;
> 
> FWIW for normal netdevs / NICs the rtnl_link_stat pkts do not include
> pause frames, normally. Otherwise one can't maintain those stats in SW
> (and per-ring stats, if any, don't add up to the full link stats).
> But if you have a good reason to do this - I won't nack..

Pause frames are accounted by rx/tx_bytes by HW. Since pause frames may
have different size, it is not possible to correct byte counters, so I
need to add them to the packet counters.

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 net-next v1 1/1] net: dsa: microchip: add stats64 support for ksz8 series of switches

[Jakub Kicinski]

On Wed, 7 Dec 2022 07:16:30 +0100 Oleksij Rempel wrote:
> > FWIW for normal netdevs / NICs the rtnl_link_stat pkts do not include
> > pause frames, normally. Otherwise one can't maintain those stats in SW
> > (and per-ring stats, if any, don't add up to the full link stats).
> > But if you have a good reason to do this - I won't nack..  
> 
> Pause frames are accounted by rx/tx_bytes by HW. Since pause frames may
> have different size, it is not possible to correct byte counters, so I
> need to add them to the packet counters.

I have embarrassed myself with my lack of understanding of pause frames
before but nonetheless - are you sure?  I thought they are always 64B.
Quick look at the standard seems to agree:

 31C.3.1 Receive state diagram (INITIATE MAC CONTROL FUNCTION) for
         EXTENSION operation

shows a 64 octet frame.

Sending long pause frames seems self-defeating as we presumably want
the receiver to react ASAP.

Re: [PATCH net-next v1 1/1] net: dsa: microchip: add stats64 support for ksz8 series of switches

[Oleksij Rempel]

On Wed, Dec 07, 2022 at 03:48:26PM -0800, Jakub Kicinski wrote:
> On Wed, 7 Dec 2022 07:16:30 +0100 Oleksij Rempel wrote:
> > > FWIW for normal netdevs / NICs the rtnl_link_stat pkts do not include
> > > pause frames, normally. Otherwise one can't maintain those stats in SW
> > > (and per-ring stats, if any, don't add up to the full link stats).
> > > But if you have a good reason to do this - I won't nack..  
> > 
> > Pause frames are accounted by rx/tx_bytes by HW. Since pause frames may
> > have different size, it is not possible to correct byte counters, so I
> > need to add them to the packet counters.
> 
> I have embarrassed myself with my lack of understanding of pause frames
> before but nonetheless - are you sure?  I thought they are always 64B.
> Quick look at the standard seems to agree:
> 
>  31C.3.1 Receive state diagram (INITIATE MAC CONTROL FUNCTION) for
>          EXTENSION operation
> 
> shows a 64 octet frame.
> 
> Sending long pause frames seems self-defeating as we presumably want
> the receiver to react ASAP.

I tested it by sending correct and malformed pause frames manually with
mausezahn. Since it is possible to send and receive pause frames
manually, it is good to count all bytes in use, otherwise we may have
bogus or malicious stealth traffic without possibility to measure it.

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 net-next v1 1/1] net: dsa: microchip: add stats64 support for ksz8 series of switches

[Jakub Kicinski]

On Thu, 8 Dec 2022 06:55:12 +0100 Oleksij Rempel wrote:
> I tested it by sending correct and malformed pause frames manually with
> mausezahn. Since it is possible to send and receive pause frames
> manually, it is good to count all bytes in use, otherwise we may have
> bogus or malicious stealth traffic without possibility to measure it.

🙃️🙃️

Re: [PATCH net-next v1 1/1] net: dsa: microchip: add stats64 support for ksz8 series of switches

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (master)
by Jakub Kicinski <kuba@kernel.org>:

On Mon,  5 Dec 2022 06:29:04 +0100 you wrote:
> Add stats64 support for ksz8xxx series of switches.
> 
> Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> ---
>  drivers/net/dsa/microchip/ksz_common.c | 87 ++++++++++++++++++++++++++
>  drivers/net/dsa/microchip/ksz_common.h |  1 +
>  2 files changed, 88 insertions(+)

Here is the summary with links:
  - [net-next,v1,1/1] net: dsa: microchip: add stats64 support for ksz8 series of switches
    https://git.kernel.org/netdev/net-next/c/bde55dd9ccda

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html