From: Jens Emil Schulz Ostergaard <jensemil.schulzostergaard@microchip.com>
To: Vladimir Oltean <olteanv@gmail.com>
Cc: <UNGLinuxDriver@microchip.com>, Andrew Lunn <andrew@lunn.ch>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
"Jakub Kicinski" <kuba@kernel.org>,
Paolo Abeni <pabeni@redhat.com>, Simon Horman <horms@kernel.org>,
Rob Herring <robh@kernel.org>,
Krzysztof Kozlowski <krzk+dt@kernel.org>,
Conor Dooley <conor+dt@kernel.org>,
Woojung Huh <woojung.huh@microchip.com>,
Russell King <linux@armlinux.org.uk>,
"Steen Hegelund" <Steen.Hegelund@microchip.com>,
Daniel Machon <daniel.machon@microchip.com>,
<linux-kernel@vger.kernel.org>, <netdev@vger.kernel.org>,
<devicetree@vger.kernel.org>
Subject: Re: [PATCH net-next 8/8] net: dsa: lan9645x: add port statistics
Date: Wed, 4 Mar 2026 16:50:41 +0100 [thread overview]
Message-ID: <35131cd06fc5f8b5b37f315bc5ab2b283dbe29fb.camel@microchip.com> (raw)
In-Reply-To: <20260303160159.efjtqzpa2w4awxht@skbuf>
On Tue, 2026-03-03 at 18:01 +0200, Vladimir Oltean wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Tue, Mar 03, 2026 at 01:22:34PM +0100, Jens Emil Schulz Østergaard wrote:
> > Add statistics support for the port counters. Chip registers are 32 bit,
> > so this unit is responsible maintaining a 64bit software cache, and
> > updating it frequently to handle overflows in hardware.
> >
> > Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
> > Signed-off-by: Jens Emil Schulz Østergaard <jensemil.schulzostergaard@microchip.com>
> > ---
> > drivers/net/dsa/microchip/lan9645x/Makefile | 1 +
> > drivers/net/dsa/microchip/lan9645x/lan9645x_main.c | 82 ++
> > drivers/net/dsa/microchip/lan9645x/lan9645x_main.h | 3 +
> > .../net/dsa/microchip/lan9645x/lan9645x_stats.c | 825 +++++++++++++++++++++
> > .../net/dsa/microchip/lan9645x/lan9645x_stats.h | 288 +++++++
> > 5 files changed, 1199 insertions(+)
> >
> > diff --git a/drivers/net/dsa/microchip/lan9645x/Makefile b/drivers/net/dsa/microchip/lan9645x/Makefile
> > index a90a46f81c72..486b005cf740 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/Makefile
> > +++ b/drivers/net/dsa/microchip/lan9645x/Makefile
> > @@ -7,3 +7,4 @@ mchp-lan9645x-objs := lan9645x_main.o \
> > lan9645x_phylink.o \
> > lan9645x_vlan.o \
> > lan9645x_mac.o \
> > + lan9645x_stats.o \
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > index ba76279b4414..8a1de2588ab8 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.c
> > @@ -7,6 +7,7 @@
> > #include <linux/phy/phy.h>
> >
> > #include "lan9645x_main.h"
> > +#include "lan9645x_stats.h"
> >
> > static const char *lan9645x_resource_names[NUM_TARGETS + 1] = {
> > [TARGET_GCB] = "gcb",
> > @@ -79,6 +80,7 @@ static void lan9645x_teardown(struct dsa_switch *ds)
> > debugfs_remove_recursive(lan9645x->debugfs_root);
> > lan9645x_npi_port_deinit(lan9645x, lan9645x->npi);
> > lan9645x_mac_deinit(lan9645x);
> > + lan9645x_stats_deinit(lan9645x);
> > }
> >
> > static int lan9645x_change_mtu(struct dsa_switch *ds, int port, int new_mtu)
> > @@ -274,6 +276,12 @@ static int lan9645x_setup(struct dsa_switch *ds)
> >
> > lan9645x_port_set_tail_drop_wm(lan9645x);
> >
> > + err = lan9645x_stats_init(lan9645x);
> > + if (err) {
> > + dev_err(dev, "Lan9645x setup: failed to init stats.");
>
> Missing \n.
> Also "Lan9645x" probably not required, it will be somewhere in the
> driver name.
>
I will remote the lan9645x and add newline.
> > + return err;
> > + }
> > +
> > ds->mtu_enforcement_ingress = true;
> > ds->assisted_learning_on_cpu_port = true;
> > ds->fdb_isolation = true;
> > @@ -636,6 +644,68 @@ static int lan9645x_fdb_del(struct dsa_switch *ds, int port,
> > return __lan9645x_fdb_del(lan9645x, port, addr, vid, br);
> > }
> >
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.h b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.h
> > index 4c7111375918..fe801d0ed39a 100644
> > --- a/drivers/net/dsa/microchip/lan9645x/lan9645x_main.h
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_main.h
> > @@ -217,6 +217,9 @@ struct lan9645x {
> > u8 vlan_flags[VLAN_N_VID];
> > DECLARE_BITMAP(cpu_vlan_mask, VLAN_N_VID); /* CPU VLAN membership */
> >
> > + /* Statistics */
> > + struct lan9645x_stats *stats;
> > +
> > int num_port_dis;
> > bool dd_dis;
> > bool tsn_dis;
> > diff --git a/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c
> > new file mode 100644
> > index 000000000000..43078e441e55
> > --- /dev/null
> > +++ b/drivers/net/dsa/microchip/lan9645x/lan9645x_stats.c
> > @@ -0,0 +1,825 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/* Copyright (C) 2026 Microchip Technology Inc.
> > + */
> > +
> > +#include <linux/workqueue.h>
> > +#include <linux/debugfs.h>
> > +
> > +#include "lan9645x_main.h"
> > +#include "lan9645x_stats.h"
> > +
> > +#define LAN9645X_STATS_CHECK_DELAY (3 * HZ)
> > +
> > +static const struct lan9645x_stat_region lan9645x_port_stat_regions[] = {
> > + /* RX region */
> > + { .base_offset = 0x0, .cnt = 67, .cnts_base_idx = 0 },
> > + /* TX region */
> > + { .base_offset = 0x80, .cnt = 48, .cnts_base_idx = 67 },
> > + /* DR region */
> > + { .base_offset = 0x100, .cnt = 18, .cnts_base_idx = 115 },
> > +};
>
> Did you find any bug in ocelot_prepare_stats_regions(), which is
> supposed to automatically build regions of contiguous stats registers
> from just their declarations? Because I find this super error-prone and
> hard to maintain in the long term, especially with cherry-picks on
> stable branches and so on.
>
No I did not find a bug in ocelot_prepare_stats_regions. I will try to take
a look and borrow it if I can. I agree this would be terrible to maintain by
hand.
I got a machine generated description of the stats registers and names from the
designers and used that to generate the tables. In the out-of-tree driver
I added all the stats at once, so it was a one-off thing.
> > +
> > +static const struct lan9645x_stat_layout lan9645x_port_stats_layout[] = {
> > + { .name = "rx_oct", .offset = 0x0 },
>
> New drivers which report a counter through standard ethtool API aren't
> supposed to need a driver-specific string for it.
>
Ok, I was not aware, I will try to do this right.
> > + { .name = "rx_uc", .offset = 0x1 },
> > + { .name = "rx_mc", .offset = 0x2 },
> > + { .name = "rx_bc", .offset = 0x3 },
> > + { .name = "rx_short", .offset = 0x4 },
> > + { .name = "rx_frag", .offset = 0x5 },
> > + { .name = "rx_jabber", .offset = 0x6 },
> > + { .name = "rx_crc", .offset = 0x7 },
> > + { .name = "rx_symbol_err", .offset = 0x8 },
> > + { .name = "rx_sz_64", .offset = 0x9 },
> > + { .name = "rx_sz_65_127", .offset = 0xa },
> > + { .name = "rx_sz_128_255", .offset = 0xb },
> > + { .name = "rx_sz_256_511", .offset = 0xc },
> > + { .name = "rx_sz_512_1023", .offset = 0xd },
> > + { .name = "rx_sz_1024_1526", .offset = 0xe },
> > + { .name = "rx_sz_jumbo", .offset = 0xf },
> > + { .name = "rx_pause", .offset = 0x10 },
> > + { .name = "rx_control", .offset = 0x11 },
> > + { .name = "rx_long", .offset = 0x12 },
> > + { .name = "rx_cat_drop", .offset = 0x13 },
> > + { .name = "rx_red_prio_0", .offset = 0x14 },
> > + { .name = "rx_red_prio_1", .offset = 0x15 },
> > + { .name = "rx_red_prio_2", .offset = 0x16 },
> > + { .name = "rx_red_prio_3", .offset = 0x17 },
> > + { .name = "rx_red_prio_4", .offset = 0x18 },
> > + { .name = "rx_red_prio_5", .offset = 0x19 },
> > + { .name = "rx_red_prio_6", .offset = 0x1a },
> > + { .name = "rx_red_prio_7", .offset = 0x1b },
> > + { .name = "rx_yellow_prio_0", .offset = 0x1c },
> > + { .name = "rx_yellow_prio_1", .offset = 0x1d },
> > + { .name = "rx_yellow_prio_2", .offset = 0x1e },
> > + { .name = "rx_yellow_prio_3", .offset = 0x1f },
> > + { .name = "rx_yellow_prio_4", .offset = 0x20 },
> > + { .name = "rx_yellow_prio_5", .offset = 0x21 },
> > + { .name = "rx_yellow_prio_6", .offset = 0x22 },
> > + { .name = "rx_yellow_prio_7", .offset = 0x23 },
> > + { .name = "rx_green_prio_0", .offset = 0x24 },
> > + { .name = "rx_green_prio_1", .offset = 0x25 },
> > + { .name = "rx_green_prio_2", .offset = 0x26 },
> > + { .name = "rx_green_prio_3", .offset = 0x27 },
> > + { .name = "rx_green_prio_4", .offset = 0x28 },
> > + { .name = "rx_green_prio_5", .offset = 0x29 },
> > + { .name = "rx_green_prio_6", .offset = 0x2a },
> > + { .name = "rx_green_prio_7", .offset = 0x2b },
> > + { .name = "rx_assembly_err", .offset = 0x2c },
> > + { .name = "rx_smd_err", .offset = 0x2d },
> > + { .name = "rx_assembly_ok", .offset = 0x2e },
> > + { .name = "rx_merge_frag", .offset = 0x2f },
> > + { .name = "rx_pmac_oct", .offset = 0x30 },
> > + { .name = "rx_pmac_uc", .offset = 0x31 },
> > + { .name = "rx_pmac_mc", .offset = 0x32 },
> > + { .name = "rx_pmac_bc", .offset = 0x33 },
> > + { .name = "rx_pmac_short", .offset = 0x34 },
> > + { .name = "rx_pmac_frag", .offset = 0x35 },
> > + { .name = "rx_pmac_jabber", .offset = 0x36 },
> > + { .name = "rx_pmac_crc", .offset = 0x37 },
> > + { .name = "rx_pmac_symbol_err", .offset = 0x38 },
> > + { .name = "rx_pmac_sz_64", .offset = 0x39 },
> > + { .name = "rx_pmac_sz_65_127", .offset = 0x3a },
> > + { .name = "rx_pmac_sz_128_255", .offset = 0x3b },
> > + { .name = "rx_pmac_sz_256_511", .offset = 0x3c },
> > + { .name = "rx_pmac_sz_512_1023", .offset = 0x3d },
> > + { .name = "rx_pmac_sz_1024_1526", .offset = 0x3e },
> > + { .name = "rx_pmac_sz_jumbo", .offset = 0x3f },
> > + { .name = "rx_pmac_pause", .offset = 0x40 },
> > + { .name = "rx_pmac_control", .offset = 0x41 },
> > + { .name = "rx_pmac_long", .offset = 0x42 },
> > + { .name = "tx_oct", .offset = 0x80 },
> > + { .name = "tx_uc", .offset = 0x81 },
> > + { .name = "tx_mc", .offset = 0x82 },
> > + { .name = "tx_bc", .offset = 0x83 },
> > + { .name = "tx_col", .offset = 0x84 },
> > + { .name = "tx_drop", .offset = 0x85 },
> > + { .name = "tx_pause", .offset = 0x86 },
> > + { .name = "tx_sz_64", .offset = 0x87 },
> > + { .name = "tx_sz_65_127", .offset = 0x88 },
> > + { .name = "tx_sz_128_255", .offset = 0x89 },
> > + { .name = "tx_sz_256_511", .offset = 0x8a },
> > + { .name = "tx_sz_512_1023", .offset = 0x8b },
> > + { .name = "tx_sz_1024_1526", .offset = 0x8c },
> > + { .name = "tx_sz_jumbo", .offset = 0x8d },
> > + { .name = "tx_yellow_prio_0", .offset = 0x8e },
> > + { .name = "tx_yellow_prio_1", .offset = 0x8f },
> > + { .name = "tx_yellow_prio_2", .offset = 0x90 },
> > + { .name = "tx_yellow_prio_3", .offset = 0x91 },
> > + { .name = "tx_yellow_prio_4", .offset = 0x92 },
> > + { .name = "tx_yellow_prio_5", .offset = 0x93 },
> > + { .name = "tx_yellow_prio_6", .offset = 0x94 },
> > + { .name = "tx_yellow_prio_7", .offset = 0x95 },
> > + { .name = "tx_green_prio_0", .offset = 0x96 },
> > + { .name = "tx_green_prio_1", .offset = 0x97 },
> > + { .name = "tx_green_prio_2", .offset = 0x98 },
> > + { .name = "tx_green_prio_3", .offset = 0x99 },
> > + { .name = "tx_green_prio_4", .offset = 0x9a },
> > + { .name = "tx_green_prio_5", .offset = 0x9b },
> > + { .name = "tx_green_prio_6", .offset = 0x9c },
> > + { .name = "tx_green_prio_7", .offset = 0x9d },
> > + { .name = "tx_aged", .offset = 0x9e },
> > + { .name = "tx_llct", .offset = 0x9f },
> > + { .name = "tx_ct", .offset = 0xa0 },
> > + { .name = "tx_bufdrop", .offset = 0xa1 },
> > + { .name = "tx_mm_hold", .offset = 0xa2 },
> > + { .name = "tx_merge_frag", .offset = 0xa3 },
> > + { .name = "tx_pmac_oct", .offset = 0xa4 },
> > + { .name = "tx_pmac_uc", .offset = 0xa5 },
> > + { .name = "tx_pmac_mc", .offset = 0xa6 },
> > + { .name = "tx_pmac_bc", .offset = 0xa7 },
> > + { .name = "tx_pmac_pause", .offset = 0xa8 },
> > + { .name = "tx_pmac_sz_64", .offset = 0xa9 },
> > + { .name = "tx_pmac_sz_65_127", .offset = 0xaa },
> > + { .name = "tx_pmac_sz_128_255", .offset = 0xab },
> > + { .name = "tx_pmac_sz_256_511", .offset = 0xac },
> > + { .name = "tx_pmac_sz_512_1023", .offset = 0xad },
> > + { .name = "tx_pmac_sz_1024_1526", .offset = 0xae },
> > + { .name = "tx_pmac_sz_jumbo", .offset = 0xaf },
> > + { .name = "dr_local", .offset = 0x100 },
> > + { .name = "dr_tail", .offset = 0x101 },
> > + { .name = "dr_yellow_prio_0", .offset = 0x102 },
> > + { .name = "dr_yellow_prio_1", .offset = 0x103 },
> > + { .name = "dr_yellow_prio_2", .offset = 0x104 },
> > + { .name = "dr_yellow_prio_3", .offset = 0x105 },
> > + { .name = "dr_yellow_prio_4", .offset = 0x106 },
> > + { .name = "dr_yellow_prio_5", .offset = 0x107 },
> > + { .name = "dr_yellow_prio_6", .offset = 0x108 },
> > + { .name = "dr_yellow_prio_7", .offset = 0x109 },
> > + { .name = "dr_green_prio_0", .offset = 0x10a },
> > + { .name = "dr_green_prio_1", .offset = 0x10b },
> > + { .name = "dr_green_prio_2", .offset = 0x10c },
> > + { .name = "dr_green_prio_3", .offset = 0x10d },
> > + { .name = "dr_green_prio_4", .offset = 0x10e },
> > + { .name = "dr_green_prio_5", .offset = 0x10f },
> > + { .name = "dr_green_prio_6", .offset = 0x110 },
> > + { .name = "dr_green_prio_7", .offset = 0x111 },
> > +};
> > +
> > +static const struct lan9645x_view_stats lan9645x_view_stat_cfgs[] = {
> > + [LAN9645X_STAT_PORTS] = {
> > + .name = "ports",
> > + .type = LAN9645X_STAT_PORTS,
> > + .layout = lan9645x_port_stats_layout,
> > + .num_cnts = ARRAY_SIZE(lan9645x_port_stats_layout),
> > + .num_indexes = NUM_PHYS_PORTS,
> > + .regions = lan9645x_port_stat_regions,
> > + .num_regions = ARRAY_SIZE(lan9645x_port_stat_regions),
>
> Odd to mix spaces with tabs.
>
Ah yes I will fix this.
> > + },
> > +};
> > +void
> > +lan9645x_stats_get_rmon_stats(struct lan9645x *lan9645x, int port,
> > + struct ethtool_rmon_stats *rmon_stats,
> > + const struct ethtool_rmon_hist_range **ranges)
> > +{
> > + u64 *port_cnt;
> > +
> > + mutex_lock(&lan9645x->stats->hw_lock);
> > +
> > + __lan9645x_stats_view_idx_update(lan9645x, LAN9645X_STAT_PORTS, port);
> > +
> > + port_cnt = STAT_COUNTERS(lan9645x, LAN9645X_STAT_PORTS, port);
> > +
> > + rmon_stats->undersize_pkts =
> > + port_cnt[SCNT_RX_SHORT] +
> > + port_cnt[SCNT_RX_PMAC_SHORT];
>
> You aren't supposed to sum up eMAC + pMAC stats except when
> rmon_stats->src == ETHTOOL_MAC_STATS_SRC_AGGREGATE. Otherwise report
> them individually.
>
> Same thing for lan9645x_stats_get_eth_mac_stats().
Ok thank you I will fix this.
>
> > + rmon_stats->oversize_pkts =
> > + port_cnt[SCNT_RX_LONG] +
> > + port_cnt[SCNT_RX_PMAC_LONG];
> > + rmon_stats->fragments =
> > + port_cnt[SCNT_RX_FRAG] +
> > + port_cnt[SCNT_RX_PMAC_FRAG];
> > + rmon_stats->jabbers =
> > + port_cnt[SCNT_RX_JABBER] +
> > + port_cnt[SCNT_RX_PMAC_JABBER];
> > +
> > + rmon_stats->hist[0] =
> > + port_cnt[SCNT_RX_SZ_64] +
> > + port_cnt[SCNT_RX_PMAC_SZ_64];
> > + rmon_stats->hist[1] =
> > + port_cnt[SCNT_RX_SZ_65_127] +
> > + port_cnt[SCNT_RX_PMAC_SZ_65_127];
> > + rmon_stats->hist[2] =
> > + port_cnt[SCNT_RX_SZ_128_255] +
> > + port_cnt[SCNT_RX_PMAC_SZ_128_255];
> > + rmon_stats->hist[3] =
> > + port_cnt[SCNT_RX_SZ_256_511] +
> > + port_cnt[SCNT_RX_PMAC_SZ_256_511];
> > + rmon_stats->hist[4] =
> > + port_cnt[SCNT_RX_SZ_512_1023] +
> > + port_cnt[SCNT_RX_PMAC_SZ_512_1023];
> > + rmon_stats->hist[5] =
> > + port_cnt[SCNT_RX_SZ_1024_1526] +
> > + port_cnt[SCNT_RX_PMAC_SZ_1024_1526];
> > + rmon_stats->hist[6] =
> > + port_cnt[SCNT_RX_SZ_JUMBO] +
> > + port_cnt[SCNT_RX_PMAC_SZ_JUMBO];
> > +
> > + rmon_stats->hist_tx[0] =
> > + port_cnt[SCNT_TX_SZ_64] +
> > + port_cnt[SCNT_TX_PMAC_SZ_64];
> > + rmon_stats->hist_tx[1] =
> > + port_cnt[SCNT_TX_SZ_65_127] +
> > + port_cnt[SCNT_TX_PMAC_SZ_65_127];
> > + rmon_stats->hist_tx[2] =
> > + port_cnt[SCNT_TX_SZ_128_255] +
> > + port_cnt[SCNT_TX_PMAC_SZ_128_255];
> > + rmon_stats->hist_tx[3] =
> > + port_cnt[SCNT_TX_SZ_256_511] +
> > + port_cnt[SCNT_TX_PMAC_SZ_256_511];
> > + rmon_stats->hist_tx[4] =
> > + port_cnt[SCNT_TX_SZ_512_1023] +
> > + port_cnt[SCNT_TX_PMAC_SZ_512_1023];
> > + rmon_stats->hist_tx[5] =
> > + port_cnt[SCNT_TX_SZ_1024_1526] +
> > + port_cnt[SCNT_TX_PMAC_SZ_1024_1526];
> > + rmon_stats->hist_tx[6] =
> > + port_cnt[SCNT_TX_SZ_JUMBO] +
> > + port_cnt[SCNT_TX_PMAC_SZ_JUMBO];
> > +
> > + mutex_unlock(&lan9645x->stats->hw_lock);
> > +
> > + *ranges = lan9645x_rmon_ranges;
> > +}
> > +
> > +void lan9645x_stats_get_stats64(struct lan9645x *lan9645x, int port,
> > + struct rtnl_link_stats64 *stats)
> > +{
> > + u64 *port_cnt;
> > +
> > + /* Avoid stats update, as this is called very often by DSA. */
> > + mutex_lock(&lan9645x->stats->hw_lock);
>
> This is atomic context, you can't acquire a mutex which may be held by a
> process which sleeps, which also puts you to sleep for it.
>
> This is why ocelot->stats_lock is a spinlock, different from
> ocelot->stat_view_lock (your lan9645x->stats->hw_lock).
>
> No problem with not updating the stats upon ndo_get_stats64() call,
> given the fact that the context is atomic.
>
Good point. I will look into adding a spinlock, because I believe I need
some locking here to avoid torn reads if the host cpu is 32bit.
And I will add a comment about atomic context.
> > +
> > + port_cnt = STAT_COUNTERS(lan9645x, LAN9645X_STAT_PORTS, port);
> > +
> > + stats->rx_bytes = port_cnt[SCNT_RX_OCT] +
> > + port_cnt[SCNT_RX_PMAC_OCT];
> > +
> > + stats->rx_packets = port_cnt[SCNT_RX_SHORT] +
> > + port_cnt[SCNT_RX_FRAG] +
> > + port_cnt[SCNT_RX_JABBER] +
> > + port_cnt[SCNT_RX_CRC] +
> > + port_cnt[SCNT_RX_SYMBOL_ERR] +
> > + port_cnt[SCNT_RX_SZ_64] +
> > + port_cnt[SCNT_RX_SZ_65_127] +
> > + port_cnt[SCNT_RX_SZ_128_255] +
> > + port_cnt[SCNT_RX_SZ_256_511] +
> > + port_cnt[SCNT_RX_SZ_512_1023] +
> > + port_cnt[SCNT_RX_SZ_1024_1526] +
> > + port_cnt[SCNT_RX_SZ_JUMBO] +
> > + port_cnt[SCNT_RX_LONG] +
> > + port_cnt[SCNT_RX_PMAC_SHORT] +
> > + port_cnt[SCNT_RX_PMAC_FRAG] +
> > + port_cnt[SCNT_RX_PMAC_JABBER] +
> > + port_cnt[SCNT_RX_PMAC_SZ_64] +
> > + port_cnt[SCNT_RX_PMAC_SZ_65_127] +
> > + port_cnt[SCNT_RX_PMAC_SZ_128_255] +
> > + port_cnt[SCNT_RX_PMAC_SZ_256_511] +
> > + port_cnt[SCNT_RX_PMAC_SZ_512_1023] +
> > + port_cnt[SCNT_RX_PMAC_SZ_1024_1526] +
> > + port_cnt[SCNT_RX_PMAC_SZ_JUMBO];
> > +
> > + stats->multicast = port_cnt[SCNT_RX_MC] +
> > + port_cnt[SCNT_RX_PMAC_MC];
> > +
> > + stats->rx_errors = port_cnt[SCNT_RX_SHORT] +
> > + port_cnt[SCNT_RX_FRAG] +
> > + port_cnt[SCNT_RX_JABBER] +
> > + port_cnt[SCNT_RX_CRC] +
> > + port_cnt[SCNT_RX_SYMBOL_ERR] +
> > + port_cnt[SCNT_RX_LONG] +
> > + port_cnt[SCNT_RX_PMAC_SHORT] +
> > + port_cnt[SCNT_RX_PMAC_FRAG] +
> > + port_cnt[SCNT_RX_PMAC_JABBER] +
> > + port_cnt[SCNT_RX_PMAC_CRC] +
> > + port_cnt[SCNT_RX_PMAC_SYMBOL_ERR] +
> > + port_cnt[SCNT_RX_PMAC_LONG];
> > +
> > + stats->rx_dropped = port_cnt[SCNT_RX_LONG] +
> > + port_cnt[SCNT_DR_LOCAL] +
> > + port_cnt[SCNT_DR_TAIL] +
> > + port_cnt[SCNT_RX_CAT_DROP] +
> > + port_cnt[SCNT_RX_RED_PRIO_0] +
> > + port_cnt[SCNT_RX_RED_PRIO_1] +
> > + port_cnt[SCNT_RX_RED_PRIO_2] +
> > + port_cnt[SCNT_RX_RED_PRIO_3] +
> > + port_cnt[SCNT_RX_RED_PRIO_4] +
> > + port_cnt[SCNT_RX_RED_PRIO_5] +
> > + port_cnt[SCNT_RX_RED_PRIO_6] +
> > + port_cnt[SCNT_RX_RED_PRIO_7];
> > +
> > + for (int i = 0; i < LAN9645X_NUM_TC; i++) {
> > + stats->rx_dropped += port_cnt[SCNT_DR_YELLOW_PRIO_0 + i] +
> > + port_cnt[SCNT_DR_GREEN_PRIO_0 + i];
> > + }
> > +
> > + stats->tx_bytes = port_cnt[SCNT_TX_OCT] +
> > + port_cnt[SCNT_TX_PMAC_OCT];
> > +
> > + stats->tx_packets = port_cnt[SCNT_TX_SZ_64] +
> > + port_cnt[SCNT_TX_SZ_65_127] +
> > + port_cnt[SCNT_TX_SZ_128_255] +
> > + port_cnt[SCNT_TX_SZ_256_511] +
> > + port_cnt[SCNT_TX_SZ_512_1023] +
> > + port_cnt[SCNT_TX_SZ_1024_1526] +
> > + port_cnt[SCNT_TX_SZ_JUMBO] +
> > + port_cnt[SCNT_TX_PMAC_SZ_64] +
> > + port_cnt[SCNT_TX_PMAC_SZ_65_127] +
> > + port_cnt[SCNT_TX_PMAC_SZ_128_255] +
> > + port_cnt[SCNT_TX_PMAC_SZ_256_511] +
> > + port_cnt[SCNT_TX_PMAC_SZ_512_1023] +
> > + port_cnt[SCNT_TX_PMAC_SZ_1024_1526] +
> > + port_cnt[SCNT_TX_PMAC_SZ_JUMBO];
> > +
> > + stats->tx_dropped = port_cnt[SCNT_TX_DROP] +
> > + port_cnt[SCNT_TX_AGED];
> > +
> > + stats->collisions = port_cnt[SCNT_TX_COL];
> > +
> > + mutex_unlock(&lan9645x->stats->hw_lock);
> > +}
> > +
> > +void lan9645x_stats_get_eth_phy_stats(struct lan9645x *lan9645x, int port,
> > + struct ethtool_eth_phy_stats *phy_stats)
> > +{
> > + u64 *port_cnt;
> > +
> > + mutex_lock(&lan9645x->stats->hw_lock);
> > +
> > + __lan9645x_stats_view_idx_update(lan9645x, LAN9645X_STAT_PORTS, port);
> > +
> > + port_cnt = STAT_COUNTERS(lan9645x, LAN9645X_STAT_PORTS, port);
> > +
> > + switch (phy_stats->src) {
> > + case ETHTOOL_MAC_STATS_SRC_EMAC:
> > + phy_stats->SymbolErrorDuringCarrier =
> > + port_cnt[SCNT_RX_SYMBOL_ERR];
> > + break;
> > + case ETHTOOL_MAC_STATS_SRC_PMAC:
> > + phy_stats->SymbolErrorDuringCarrier =
> > + port_cnt[SCNT_RX_PMAC_SYMBOL_ERR];
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + mutex_unlock(&lan9645x->stats->hw_lock);
> > +}
> > +
> > +void
> > +lan9645x_stats_get_eth_ctrl_stats(struct lan9645x *lan9645x, int port,
> > + struct ethtool_eth_ctrl_stats *ctrl_stats)
> > +{
> > + u64 *port_cnt;
> > +
> > + mutex_lock(&lan9645x->stats->hw_lock);
> > +
> > + __lan9645x_stats_view_idx_update(lan9645x, LAN9645X_STAT_PORTS, port);
> > +
> > + port_cnt = STAT_COUNTERS(lan9645x, LAN9645X_STAT_PORTS, port);
> > +
> > + switch (ctrl_stats->src) {
> > + case ETHTOOL_MAC_STATS_SRC_EMAC:
> > + ctrl_stats->MACControlFramesReceived =
> > + port_cnt[SCNT_RX_CONTROL];
> > + break;
> > + case ETHTOOL_MAC_STATS_SRC_PMAC:
> > + ctrl_stats->MACControlFramesReceived =
> > + port_cnt[SCNT_RX_PMAC_CONTROL];
> > + break;
> > + default:
> > + break;
> > + }
> > +
> > + mutex_unlock(&lan9645x->stats->hw_lock);
> > +}
> > +
> > +void lan9645x_stats_get_pause_stats(struct lan9645x *lan9645x, int port,
> > + struct ethtool_pause_stats *ps)
> > +{
> > + u64 *port_cnt;
> > +
> > + mutex_lock(&lan9645x->stats->hw_lock);
> > +
> > + __lan9645x_stats_view_idx_update(lan9645x, LAN9645X_STAT_PORTS, port);
> > +
> > + port_cnt = STAT_COUNTERS(lan9645x, LAN9645X_STAT_PORTS, port);
> > +
> > + switch (ps->src) {
> > + case ETHTOOL_MAC_STATS_SRC_EMAC:
> > + ps->tx_pause_frames = port_cnt[SCNT_TX_PAUSE];
> > + ps->rx_pause_frames = port_cnt[SCNT_RX_PAUSE];
> > + break;
> > + case ETHTOOL_MAC_STATS_SRC_PMAC:
> > + ps->tx_pause_frames = port_cnt[SCNT_TX_PMAC_PAUSE];
> > + ps->rx_pause_frames = port_cnt[SCNT_RX_PMAC_PAUSE];
> > + break;
>
> ...no aggregate? I guess this will just return zeroes under normal
> operation, and not anything useful? Similar thing everywhere you look
> for the source of statistics.
>
> BTW, Ioana is working on a selftest that makes sure standard counters
> are implemeted correctly.
> https://lore.kernel.org/netdev/20260225150648.1542206-6-ioana.ciornei@nxp.com/
>
> The test packaging still needs to be improved, but it's functional.
> Maybe you could give it a run? I'm thinking it would have caught this.
>
I will add the aggregate handling and try out the selftest patch.
> > + default:
> > + break;
> > + }
> > +
> > + mutex_unlock(&lan9645x->stats->hw_lock);
> > +}
> > +
> > +void lan9645x_stats_get_mm_stats(struct lan9645x *lan9645x, int port,
> > + struct ethtool_mm_stats *stats)
> > +{
> > + u64 *port_cnt;
> > +
> > + mutex_lock(&lan9645x->stats->hw_lock);
> > +
> > + __lan9645x_stats_view_idx_update(lan9645x, LAN9645X_STAT_PORTS, port);
> > +
> > + port_cnt = STAT_COUNTERS(lan9645x, LAN9645X_STAT_PORTS, port);
> > +
> > + stats->MACMergeFrameAssErrorCount = port_cnt[SCNT_RX_ASSEMBLY_ERR];
> > + stats->MACMergeFrameSmdErrorCount = port_cnt[SCNT_RX_SMD_ERR];
> > + stats->MACMergeFrameAssOkCount = port_cnt[SCNT_RX_ASSEMBLY_OK];
> > + stats->MACMergeFragCountRx = port_cnt[SCNT_RX_MERGE_FRAG];
> > + stats->MACMergeFragCountTx = port_cnt[SCNT_TX_MERGE_FRAG];
> > + stats->MACMergeHoldCount = port_cnt[SCNT_TX_MM_HOLD];
> > +
> > + mutex_unlock(&lan9645x->stats->hw_lock);
> > +}
> > +
> > +void lan9645x_stats_clear_counters(struct lan9645x *lan9645x,
> > + enum lan9645x_view_stat_type type, int idx)
>
> Never called, please remove. Don't leave dead code in the driver.
>
I will remove it.
> > +{
> > + struct lan9645x_view_stats *vstats =
> > + lan9645x_get_vstats(lan9645x, type);
> > + u64 *idx_grp;
> > + int cntr;
> > + u32 sel;
> > +
> > + switch (type) {
> > + case LAN9645X_STAT_PORTS:
> > + /* Drop, TX and RX counters */
> > + sel = BIT(2) | BIT(1) | BIT(0);
> > + break;
> > + case LAN9645X_STAT_ISDX:
> > + /* ISDX and FRER seq gen */
> > + sel = BIT(5) | BIT(3);
> > + break;
> > + case LAN9645X_STAT_ESDX:
> > + /* ESDX */
> > + sel = BIT(6);
> > + break;
> > + case LAN9645X_STAT_SFID:
> > + /* Stream filter */
> > + sel = BIT(4);
> > + break;
> > + default:
> > + return;
> > + }
> > +
> > + mutex_lock(&lan9645x->stats->hw_lock);
> > +
> > + lan_wr(SYS_STAT_CFG_STAT_CLEAR_SHOT_SET(sel) |
> > + SYS_STAT_CFG_STAT_VIEW_SET(idx),
> > + lan9645x, SYS_STAT_CFG);
> > +
> > + idx_grp = STATS_INDEX(vstats, idx);
> > + for (cntr = 0; cntr < vstats->num_cnts; cntr++)
> > + idx_grp[cntr] = 0;
> > +
> > + mutex_unlock(&lan9645x->stats->hw_lock);
> > +}
> > +
> > +static void lan9645x_check_stats_work(struct work_struct *work)
> > +{
> > + struct delayed_work *del_work = to_delayed_work(work);
> > + struct lan9645x_stats *stats;
> > +
> > + stats = container_of(del_work, struct lan9645x_stats, work);
> > +
> > + lan9645x_stats_update(stats->lan9645x);
> > +
> > + queue_delayed_work(stats->queue, &stats->work,
> > + LAN9645X_STATS_CHECK_DELAY);
> > +}
> > +
> > +static int lan9645x_stats_debugfs_show(struct seq_file *m, void *unused)
> > +{
> > + struct lan9645x_view_stats *vstats = m->private;
> > + int idx, cntr;
> > + size_t total;
> > + u64 *snap;
> > +
> > + total = vstats->num_cnts * vstats->num_indexes;
> > +
> > + /* Snapshot counters under lock to avoid holding hw_lock during
> > + * slow seq_printf output.
> > + */
> > + snap = kmalloc_array(total, sizeof(u64), GFP_KERNEL);
> > + if (!snap)
> > + return -ENOMEM;
> > +
> > + mutex_lock(&vstats->stats->hw_lock);
> > + memcpy(snap, vstats->cnts, total * sizeof(u64));
> > + mutex_unlock(&vstats->stats->hw_lock);
> > +
> > + for (idx = 0; idx < vstats->num_indexes; idx++) {
> > + for (cntr = 0; cntr < vstats->num_cnts; cntr++) {
> > + seq_printf(m, "%s_%d_%-*s %llu\n", vstats->name, idx,
> > + 30, vstats->layout[cntr].name,
> > + snap[vstats->num_cnts * idx + cntr]);
> > + }
> > + }
> > +
> > + kfree(snap);
> > + return 0;
> > +}
> > +DEFINE_SHOW_ATTRIBUTE(lan9645x_stats_debugfs);
>
> What is the purpose of a 'stats' debugfs?
>
> If this is about standard ethtool stats on the CPU port, we need a
> larger discussion about this, since it is a common problem for multiple
> drivers.
>
> I had a prototype patch to permit "ethtool" to be run on devlink ports
> (we do have one of those on the CPU port). But I never had time to start
> a discussion about it.
>
Yes I used it to expose _all_ hardware counters for all views and indices. In
particular, the cpu port. I used it for debugging, but I will remove it
and the parent debugfs.
Is devlink a valid use for something like this?
> > +
> > +static void lan9645x_stats_debugfs(struct lan9645x *lan9645x,
> > + struct dentry *parent)
> > +{
> > + struct lan9645x_stats *stats = lan9645x->stats;
> > + struct dentry *dir;
> > + int i;
> > +
> > + dir = debugfs_create_dir("stats", parent);
> > + if (PTR_ERR_OR_ZERO(dir))
> > + return;
> > +
> > + for (i = 0; i < ARRAY_SIZE(stats->view); i++)
> > + debugfs_create_file(stats->view[i].name, 0444, dir,
> > + &stats->view[i],
> > + &lan9645x_stats_debugfs_fops);
> > +}
> > +
> > +static int lan9645x_view_stat_init(struct lan9645x *lan9645x,
> > + struct lan9645x_view_stats *vstat,
> > + const struct lan9645x_view_stats *cfg)
> > +{
> > + size_t total = cfg->num_cnts * cfg->num_indexes;
> > +
> > + memcpy(vstat, cfg, sizeof(*cfg));
> > +
> > + vstat->cnts = devm_kcalloc(lan9645x->dev, total, sizeof(u64),
> > + GFP_KERNEL);
> > + if (!vstat->cnts)
> > + return -ENOMEM;
> > +
> > + vstat->buf = devm_kcalloc(lan9645x->dev, total, sizeof(u32),
> > + GFP_KERNEL);
> > + if (!vstat->buf)
> > + return -ENOMEM;
> > +
> > + vstat->stats = lan9645x->stats;
> > +
> > + return 0;
> > +}
> > +
> > +int lan9645x_stats_init(struct lan9645x *lan9645x)
> > +{
> > + const struct lan9645x_view_stats *vs;
> > + struct lan9645x_stats *stats;
> > + int err, i;
> > +
> > + lan9645x->stats = devm_kzalloc(lan9645x->dev, sizeof(*stats),
> > + GFP_KERNEL);
> > + if (!lan9645x->stats)
> > + return -ENOMEM;
> > +
> > + stats = lan9645x->stats;
> > + stats->lan9645x = lan9645x;
> > +
> > + mutex_init(&stats->hw_lock);
> > + stats->queue = alloc_ordered_workqueue("lan9645x-stats", 0);
> > + if (!stats->queue)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < ARRAY_SIZE(lan9645x_view_stat_cfgs); i++) {
> > + vs = &lan9645x_view_stat_cfgs[i];
> > +
> > + if (!vs->num_cnts)
> > + continue;
> > +
> > + err = lan9645x_view_stat_init(lan9645x, &stats->view[vs->type],
> > + vs);
> > + if (err)
>
> stats->queue is leaked.
>
Ah yes, thanks.
> > + return err;
> > + }
> > +
> > + INIT_DELAYED_WORK(&stats->work, lan9645x_check_stats_work);
> > + queue_delayed_work(stats->queue, &stats->work,
> > + LAN9645X_STATS_CHECK_DELAY);
> > +
> > + lan9645x_stats_debugfs(lan9645x, lan9645x->debugfs_root);
> > +
> > + return 0;
> > +}
> >
> >
>
Thank you for the comments,
Emil
prev parent reply other threads:[~2026-03-04 15:51 UTC|newest]
Thread overview: 69+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-03-03 12:22 [PATCH net-next 0/8] net: dsa: add DSA support for the LAN9645x switch chip family Jens Emil Schulz Østergaard
2026-03-03 12:22 ` [PATCH net-next 1/8] net: dsa: add tag driver for LAN9645X Jens Emil Schulz Østergaard
2026-03-03 14:13 ` Andrew Lunn
2026-03-03 15:58 ` Jens Emil Schulz Ostergaard
2026-03-04 15:14 ` Andrew Lunn
2026-03-05 12:59 ` Jens Emil Schulz Ostergaard
2026-03-03 16:11 ` Vladimir Oltean
2026-03-05 13:53 ` Jens Emil Schulz Ostergaard
2026-03-04 15:23 ` Andrew Lunn
2026-03-05 13:01 ` Jens Emil Schulz Ostergaard
2026-03-03 12:22 ` [PATCH net-next 2/8] dt-bindings: net: lan9645x: add LAN9645X switch bindings Jens Emil Schulz Østergaard
2026-03-03 13:22 ` Vladimir Oltean
2026-03-03 16:00 ` Jens Emil Schulz Ostergaard
2026-03-03 14:18 ` Andrew Lunn
2026-03-03 19:04 ` Conor Dooley
2026-03-04 15:57 ` Jens Emil Schulz Ostergaard
2026-03-05 12:57 ` Jens Emil Schulz Ostergaard
2026-03-05 18:31 ` Conor Dooley
2026-03-06 15:08 ` Jens Emil Schulz Ostergaard
2026-03-06 15:20 ` Conor Dooley
2026-03-18 14:19 ` Jens Emil Schulz Ostergaard
2026-03-18 17:18 ` Conor Dooley
2026-03-18 17:20 ` Conor Dooley
2026-03-18 17:26 ` Christian Marangi
2026-03-24 10:31 ` Jens Emil Schulz Ostergaard
2026-03-04 15:55 ` Jens Emil Schulz Ostergaard
2026-03-03 18:49 ` Conor Dooley
2026-03-04 15:58 ` Jens Emil Schulz Ostergaard
2026-03-03 18:56 ` Conor Dooley
2026-03-04 16:10 ` Jens Emil Schulz Ostergaard
2026-03-04 16:14 ` Vladimir Oltean
2026-03-04 19:06 ` Conor Dooley
2026-03-05 13:08 ` Jens Emil Schulz Ostergaard
2026-03-03 12:22 ` [PATCH net-next 3/8] net: dsa: lan9645x: add autogenerated register macros Jens Emil Schulz Østergaard
2026-03-03 12:22 ` [PATCH net-next 4/8] net: dsa: lan9645x: add basic dsa driver for LAN9645X Jens Emil Schulz Østergaard
2026-03-03 14:15 ` Vladimir Oltean
2026-03-04 14:37 ` Jens Emil Schulz Ostergaard
2026-03-04 15:58 ` Russell King (Oracle)
2026-03-05 14:24 ` Jens Emil Schulz Ostergaard
2026-03-05 14:58 ` Andrew Lunn
2026-03-05 15:10 ` Vladimir Oltean
2026-03-05 16:54 ` Alexander Stein
2026-03-05 17:37 ` Andrew Lunn
2026-03-06 15:03 ` Jens Emil Schulz Ostergaard
2026-03-06 16:33 ` Andrew Lunn
2026-03-09 12:01 ` Jens Emil Schulz Ostergaard
2026-03-06 14:22 ` Russell King (Oracle)
2026-03-06 21:03 ` Jakub Kicinski
2026-03-03 12:22 ` [PATCH net-next 5/8] net: dsa: lan9645x: add bridge support Jens Emil Schulz Østergaard
2026-03-03 14:20 ` Vladimir Oltean
2026-03-03 16:08 ` Jens Emil Schulz Ostergaard
2026-03-03 16:17 ` Vladimir Oltean
2026-03-05 13:14 ` Jens Emil Schulz Ostergaard
2026-03-03 14:51 ` Vladimir Oltean
2026-03-09 12:09 ` Jens Emil Schulz Ostergaard
2026-03-03 12:22 ` [PATCH net-next 6/8] net: dsa: lan9645x: add vlan support Jens Emil Schulz Østergaard
2026-03-03 14:59 ` Vladimir Oltean
2026-03-04 14:40 ` Jens Emil Schulz Ostergaard
2026-03-04 14:52 ` Vladimir Oltean
2026-03-03 12:22 ` [PATCH net-next 7/8] net: dsa: lan9645x: add mac table integration Jens Emil Schulz Østergaard
2026-03-03 15:27 ` Vladimir Oltean
2026-03-04 15:23 ` Jens Emil Schulz Ostergaard
2026-03-04 15:34 ` Andrew Lunn
2026-03-05 13:17 ` Jens Emil Schulz Ostergaard
2026-03-03 12:22 ` [PATCH net-next 8/8] net: dsa: lan9645x: add port statistics Jens Emil Schulz Østergaard
2026-03-03 16:01 ` Vladimir Oltean
2026-03-03 20:21 ` Andrew Lunn
2026-03-04 15:51 ` Jens Emil Schulz Ostergaard
2026-03-04 15:50 ` Jens Emil Schulz Ostergaard [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=35131cd06fc5f8b5b37f315bc5ab2b283dbe29fb.camel@microchip.com \
--to=jensemil.schulzostergaard@microchip.com \
--cc=Steen.Hegelund@microchip.com \
--cc=UNGLinuxDriver@microchip.com \
--cc=andrew@lunn.ch \
--cc=conor+dt@kernel.org \
--cc=daniel.machon@microchip.com \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=edumazet@google.com \
--cc=horms@kernel.org \
--cc=krzk+dt@kernel.org \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@armlinux.org.uk \
--cc=netdev@vger.kernel.org \
--cc=olteanv@gmail.com \
--cc=pabeni@redhat.com \
--cc=robh@kernel.org \
--cc=woojung.huh@microchip.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox