* [PATCH net-next v3 0/6] net: dsa: motorcomm: MIB fixup
@ 2026-09-06 13:14 David Yang
2026-09-06 13:14 ` [PATCH net-next v3 1/6] net: dsa: motorcomm: Rename MIB stuffs David Yang
` (5 more replies)
0 siblings, 6 replies; 15+ messages in thread
From: David Yang @ 2026-09-06 13:14 UTC (permalink / raw)
To: netdev
Cc: David Yang, Andrew Lunn, Vladimir Oltean, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Russell King,
linux-kernel
Fix (theoretical) MIB inconsistency and prepare for new devices. No new
features are introduced.
v3:
- fix build errors
- fix logic error on MIB buffer creation
v2: https://lore.kernel.org/r/20260904162952.709368-1-mmyangfl@gmail.com
- split patches
v1: https://lore.kernel.org/r/20260903143514.532023-1-mmyangfl@gmail.com
David Yang (6):
net: dsa: motorcomm: Rename MIB stuffs
net: dsa: motorcomm: Split MIB buffers
net: dsa: motorcomm: Split MIB module
net: dsa: motorcomm: Use u64_stats_t for MIB stats
net: dsa: motorcomm: Fix MIB synchronization
net: dsa: motorcomm: Use safe 64-bit counter reader
drivers/net/dsa/motorcomm/Makefile | 1 +
drivers/net/dsa/motorcomm/chip.c | 408 +++------------------------
drivers/net/dsa/motorcomm/chip.h | 122 +-------
drivers/net/dsa/motorcomm/mib.c | 429 +++++++++++++++++++++++++++++
drivers/net/dsa/motorcomm/mib.h | 167 +++++++++++
drivers/net/dsa/motorcomm/smi.c | 30 ++
drivers/net/dsa/motorcomm/smi.h | 2 +
7 files changed, 667 insertions(+), 492 deletions(-)
create mode 100644 drivers/net/dsa/motorcomm/mib.c
create mode 100644 drivers/net/dsa/motorcomm/mib.h
--
2.53.0
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH net-next v3 1/6] net: dsa: motorcomm: Rename MIB stuffs 2026-09-06 13:14 [PATCH net-next v3 0/6] net: dsa: motorcomm: MIB fixup David Yang @ 2026-09-06 13:14 ` David Yang 2026-09-09 15:16 ` netdev-bot+sashiko 2026-09-06 13:14 ` [PATCH net-next v3 2/6] net: dsa: motorcomm: Split MIB buffers David Yang ` (4 subsequent siblings) 5 siblings, 1 reply; 15+ messages in thread From: David Yang @ 2026-09-06 13:14 UTC (permalink / raw) To: netdev Cc: David Yang, Andrew Lunn, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Russell King, linux-kernel Rename them in preparation of major MIB refactor. Signed-off-by: David Yang <mmyangfl@gmail.com> --- drivers/net/dsa/motorcomm/chip.c | 32 ++++++++++++++++---------------- drivers/net/dsa/motorcomm/chip.h | 4 ++-- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c index d663af010f43..ad5fe1ccb1d7 100644 --- a/drivers/net/dsa/motorcomm/chip.c +++ b/drivers/net/dsa/motorcomm/chip.c @@ -545,11 +545,11 @@ yt921x_mbus_ext_init(struct yt921x_priv *priv, struct device_node *mnp) } /* Read and handle overflow of 32bit MIBs. MIB buffer must be zeroed before. */ -static int yt921x_read_mib(struct yt921x_priv *priv, int port) +static int yt921x_mib_read(struct yt921x_priv *priv, int port) { struct yt921x_port *pp = &priv->ports[port]; struct device *dev = to_device(priv); - struct yt921x_mib *mib = &pp->mib; + struct yt921x_mib_stats *mib = &pp->mib; int res = 0; /* Reading of yt921x_port::mib is not protected by a lock and it's vain @@ -604,7 +604,7 @@ static int yt921x_read_mib(struct yt921x_priv *priv, int port) return res; } -static void yt921x_poll_mib(struct work_struct *work) +static void yt921x_mib_poll(struct work_struct *work) { struct yt921x_port *pp = container_of_const(work, struct yt921x_port, mib_read.work); @@ -615,7 +615,7 @@ static void yt921x_poll_mib(struct work_struct *work) int res; mutex_lock(&priv->reg_lock); - res = yt921x_read_mib(priv, port); + res = yt921x_mib_read(priv, port); mutex_unlock(&priv->reg_lock); if (res) delay *= 4; @@ -643,11 +643,11 @@ yt921x_dsa_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data) { struct yt921x_priv *priv = to_yt921x_priv(ds); struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib *mib = &pp->mib; + struct yt921x_mib_stats *mib = &pp->mib; size_t j; mutex_lock(&priv->reg_lock); - yt921x_read_mib(priv, port); + yt921x_mib_read(priv, port); mutex_unlock(&priv->reg_lock); j = 0; @@ -685,10 +685,10 @@ yt921x_dsa_get_eth_mac_stats(struct dsa_switch *ds, int port, { struct yt921x_priv *priv = to_yt921x_priv(ds); struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib *mib = &pp->mib; + struct yt921x_mib_stats *mib = &pp->mib; mutex_lock(&priv->reg_lock); - yt921x_read_mib(priv, port); + yt921x_mib_read(priv, port); mutex_unlock(&priv->reg_lock); mac_stats->FramesTransmittedOK = pp->tx_frames; @@ -721,10 +721,10 @@ yt921x_dsa_get_eth_ctrl_stats(struct dsa_switch *ds, int port, { struct yt921x_priv *priv = to_yt921x_priv(ds); struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib *mib = &pp->mib; + struct yt921x_mib_stats *mib = &pp->mib; mutex_lock(&priv->reg_lock); - yt921x_read_mib(priv, port); + yt921x_mib_read(priv, port); mutex_unlock(&priv->reg_lock); ctrl_stats->MACControlFramesTransmitted = mib->tx_pause; @@ -750,10 +750,10 @@ yt921x_dsa_get_rmon_stats(struct dsa_switch *ds, int port, { struct yt921x_priv *priv = to_yt921x_priv(ds); struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib *mib = &pp->mib; + struct yt921x_mib_stats *mib = &pp->mib; mutex_lock(&priv->reg_lock); - yt921x_read_mib(priv, port); + yt921x_mib_read(priv, port); mutex_unlock(&priv->reg_lock); *ranges = yt921x_rmon_ranges; @@ -786,7 +786,7 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port, { struct yt921x_priv *priv = to_yt921x_priv(ds); struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib *mib = &pp->mib; + struct yt921x_mib_stats *mib = &pp->mib; stats->rx_length_errors = mib->rx_undersize_errors + mib->rx_fragment_errors; @@ -822,10 +822,10 @@ yt921x_dsa_get_pause_stats(struct dsa_switch *ds, int port, { struct yt921x_priv *priv = to_yt921x_priv(ds); struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib *mib = &pp->mib; + struct yt921x_mib_stats *mib = &pp->mib; mutex_lock(&priv->reg_lock); - yt921x_read_mib(priv, port); + yt921x_mib_read(priv, port); mutex_unlock(&priv->reg_lock); pause_stats->tx_pause_frames = mib->tx_pause; @@ -4749,7 +4749,7 @@ static int yt921x_mdio_probe(struct mdio_device *mdiodev) struct yt921x_port *pp = &priv->ports[i]; pp->index = i; - INIT_DELAYED_WORK(&pp->mib_read, yt921x_poll_mib); + INIT_DELAYED_WORK(&pp->mib_read, yt921x_mib_poll); } ds = &priv->ds; diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h index 83cd454955dd..805febb48683 100644 --- a/drivers/net/dsa/motorcomm/chip.h +++ b/drivers/net/dsa/motorcomm/chip.h @@ -861,7 +861,7 @@ enum yt921x_fdb_entry_status { #define yt921x_port_is_internal(port) ((port) < 8) #define yt921x_port_is_external(port) ((port) == 8 || (port) == 9) -struct yt921x_mib { +struct yt921x_mib_stats { u64 rx_broadcast; u64 rx_pause; u64 rx_multicast; @@ -940,7 +940,7 @@ struct yt921x_port { bool isolated; struct delayed_work mib_read; - struct yt921x_mib mib; + struct yt921x_mib_stats mib; u64 rx_frames; u64 tx_frames; -- 2.53.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v3 1/6] net: dsa: motorcomm: Rename MIB stuffs 2026-09-06 13:14 ` [PATCH net-next v3 1/6] net: dsa: motorcomm: Rename MIB stuffs David Yang @ 2026-09-09 15:16 ` netdev-bot+sashiko 2026-09-09 17:32 ` David Yang 0 siblings, 1 reply; 15+ messages in thread From: netdev-bot+sashiko @ 2026-09-09 15:16 UTC (permalink / raw) To: mmyangfl Cc: netdev, andrew, olteanv, davem, edumazet, kuba, pabeni, linux, linux-kernel Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 0 · Low: 1 - [Low] Stale comment after the rename: `drivers/net/dsa/motorcomm/chip.c:41` still reads "/* Must agree with… -- net: dsa: motorcomm: Rename MIB stuffs This renames yt921x_read_mib() to yt921x_mib_read(), yt921x_poll_mib() to yt921x_mib_poll(), and struct yt921x_mib to struct yt921x_mib_stats, in preparation for a larger MIB refactor. No functional change is intended. > diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c > index d663af010f431..ad5fe1ccb1d71 100644 > --- a/drivers/net/dsa/motorcomm/chip.c > +++ b/drivers/net/dsa/motorcomm/chip.c > @@ -545,11 +545,11 @@ yt921x_mbus_ext_init(struct yt921x_priv *priv, struct device_node *mnp) > } > > /* Read and handle overflow of 32bit MIBs. MIB buffer must be zeroed before. */ > -static int yt921x_read_mib(struct yt921x_priv *priv, int port) > +static int yt921x_mib_read(struct yt921x_priv *priv, int port) > { > struct yt921x_port *pp = &priv->ports[port]; > struct device *dev = to_device(priv); > - struct yt921x_mib *mib = &pp->mib; > + struct yt921x_mib_stats *mib = &pp->mib; > int res = 0; [ ... ] > diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h > index 83cd454955ddb..805febb486838 100644 > --- a/drivers/net/dsa/motorcomm/chip.h > +++ b/drivers/net/dsa/motorcomm/chip.h > @@ -861,7 +861,7 @@ enum yt921x_fdb_entry_status { > #define yt921x_port_is_internal(port) ((port) < 8) > #define yt921x_port_is_external(port) ((port) == 8 || (port) == 9) > > -struct yt921x_mib { > +struct yt921x_mib_stats { > u64 rx_broadcast; > u64 rx_pause; > u64 rx_multicast; [Severity: Low] Should the comment above yt921x_mib_descs[] in chip.c be renamed too? It still points at the old type name: drivers/net/dsa/motorcomm/chip.c /* Must agree with yt921x_mib * * Unstructured fields (name != NULL) will appear in get_ethtool_stats(), * ... */ static const struct yt921x_mib_desc yt921x_mib_descs[] = { After this commit there is no struct yt921x_mib left in the tree, so the comment names a type that no longer exists. This comment is also the only thing documenting the invariant that yt921x_mib_read() relies on: for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) { const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i]; u32 reg = YT921X_MIBn_DATA0(port) + desc->offset; u64 *valp = &((u64 *)mib)[i]; The descriptor table order and count must match the u64 field order and count of the (now renamed) struct, and there is no static_assert() or BUILD_BUG_ON() tying ARRAY_SIZE(yt921x_mib_descs) to sizeof(struct yt921x_mib_stats) / sizeof(u64). The invariant itself still holds here (39 descriptors, 39 u64 fields, same order), so this is only a comment-versus-definition mismatch. Worth noting for the rest of the series: by the end of the series the same comment survives in drivers/net/dsa/motorcomm/mib.c, but struct yt921x_mib has been reintroduced there as a different container type (work, syncp, stats, data[]). At that point the comment points at a struct that the table must not agree with, since the table has to match struct yt921x_mib_stats. Would it make sense to update the comment in this patch, and possibly add a compile-time assertion on the entry count? -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906131414.2683362-1-mmyangfl%40gmail.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v3 1/6] net: dsa: motorcomm: Rename MIB stuffs 2026-09-09 15:16 ` netdev-bot+sashiko @ 2026-09-09 17:32 ` David Yang 0 siblings, 0 replies; 15+ messages in thread From: David Yang @ 2026-09-09 17:32 UTC (permalink / raw) To: netdev-bot+sashiko Cc: netdev, andrew, olteanv, davem, edumazet, kuba, pabeni, linux, linux-kernel On Wed, Sep 9, 2026 at 11:16 PM <netdev-bot+sashiko@kernel.org> wrote: > [Severity: Low] > Should the comment above yt921x_mib_descs[] in chip.c be renamed too? It > still points at the old type name: > > drivers/net/dsa/motorcomm/chip.c > /* Must agree with yt921x_mib > * > * Unstructured fields (name != NULL) will appear in get_ethtool_stats(), > * ... > */ > static const struct yt921x_mib_desc yt921x_mib_descs[] = { > > After this commit there is no struct yt921x_mib left in the tree, so the > comment names a type that no longer exists. Will fix them in the next version, please do not merge it. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net-next v3 2/6] net: dsa: motorcomm: Split MIB buffers 2026-09-06 13:14 [PATCH net-next v3 0/6] net: dsa: motorcomm: MIB fixup David Yang 2026-09-06 13:14 ` [PATCH net-next v3 1/6] net: dsa: motorcomm: Rename MIB stuffs David Yang @ 2026-09-06 13:14 ` David Yang 2026-09-09 15:16 ` netdev-bot+sashiko 2026-09-06 13:14 ` [PATCH net-next v3 3/6] net: dsa: motorcomm: Split MIB module David Yang ` (3 subsequent siblings) 5 siblings, 1 reply; 15+ messages in thread From: David Yang @ 2026-09-06 13:14 UTC (permalink / raw) To: netdev Cc: David Yang, Andrew Lunn, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Russell King, linux-kernel With MIB improvement introduced later, struct yt921x_priv will be over 20k which is not very good for a single kmalloc(). Dynamically allocate the MIB structures to reduce the size of struct yt921x_priv, and do not waste memory on dummy ports. Signed-off-by: David Yang <mmyangfl@gmail.com> --- drivers/net/dsa/motorcomm/chip.c | 112 +++++++++++++++++++++++-------- drivers/net/dsa/motorcomm/chip.h | 16 +++-- 2 files changed, 96 insertions(+), 32 deletions(-) diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c index ad5fe1ccb1d7..95041a2fb0ec 100644 --- a/drivers/net/dsa/motorcomm/chip.c +++ b/drivers/net/dsa/motorcomm/chip.c @@ -549,9 +549,12 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) { struct yt921x_port *pp = &priv->ports[port]; struct device *dev = to_device(priv); - struct yt921x_mib_stats *mib = &pp->mib; + struct yt921x_mib *pm = pp->mib; + struct yt921x_mib_stats *mib; int res = 0; + mib = &pm->stats; + /* Reading of yt921x_port::mib is not protected by a lock and it's vain * to keep its consistency, since we have to read registers one by one * and there is no way to make a snapshot of MIB stats. @@ -589,11 +592,11 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) WRITE_ONCE(*valp, val); } - pp->rx_frames = mib->rx_64byte + mib->rx_65_127byte + + pm->rx_frames = mib->rx_64byte + mib->rx_65_127byte + mib->rx_128_255byte + mib->rx_256_511byte + mib->rx_512_1023byte + mib->rx_1024_1518byte + mib->rx_jumbo; - pp->tx_frames = mib->tx_64byte + mib->tx_65_127byte + + pm->tx_frames = mib->tx_64byte + mib->tx_65_127byte + mib->tx_128_255byte + mib->tx_256_511byte + mib->tx_512_1023byte + mib->tx_1024_1518byte + mib->tx_jumbo; @@ -606,10 +609,11 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) static void yt921x_mib_poll(struct work_struct *work) { - struct yt921x_port *pp = container_of_const(work, struct yt921x_port, - mib_read.work); - struct yt921x_priv *priv = (void *)(pp - pp->index) - - offsetof(struct yt921x_priv, ports); + struct yt921x_mib *pm = container_of_const(work, struct yt921x_mib, + work.work); + struct yt921x_port *pp = pm->port; + struct yt921x_priv *priv = container_of_const(pp, struct yt921x_priv, + ports[pp->index]); unsigned long delay = YT921X_STATS_INTERVAL_JIFFIES; int port = pp->index; int res; @@ -620,7 +624,7 @@ static void yt921x_mib_poll(struct work_struct *work) if (res) delay *= 4; - schedule_delayed_work(&pp->mib_read, delay); + schedule_delayed_work(&pm->work, delay); } static void @@ -643,9 +647,14 @@ yt921x_dsa_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data) { struct yt921x_priv *priv = to_yt921x_priv(ds); struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib_stats *mib = &pp->mib; + struct yt921x_mib *pm = pp->mib; + struct yt921x_mib_stats *mib; size_t j; + if (!pm) + return; + mib = &pm->stats; + mutex_lock(&priv->reg_lock); yt921x_mib_read(priv, port); mutex_unlock(&priv->reg_lock); @@ -685,16 +694,21 @@ yt921x_dsa_get_eth_mac_stats(struct dsa_switch *ds, int port, { struct yt921x_priv *priv = to_yt921x_priv(ds); struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib_stats *mib = &pp->mib; + struct yt921x_mib *pm = pp->mib; + struct yt921x_mib_stats *mib; + + if (!pm) + return; + mib = &pm->stats; mutex_lock(&priv->reg_lock); yt921x_mib_read(priv, port); mutex_unlock(&priv->reg_lock); - mac_stats->FramesTransmittedOK = pp->tx_frames; + mac_stats->FramesTransmittedOK = pm->tx_frames; mac_stats->SingleCollisionFrames = mib->tx_single_collisions; mac_stats->MultipleCollisionFrames = mib->tx_multiple_collisions; - mac_stats->FramesReceivedOK = pp->rx_frames; + mac_stats->FramesReceivedOK = pm->rx_frames; mac_stats->FrameCheckSequenceErrors = mib->rx_crc_errors; mac_stats->AlignmentErrors = mib->rx_alignment_errors; mac_stats->OctetsTransmittedOK = mib->tx_good_bytes; @@ -721,7 +735,12 @@ yt921x_dsa_get_eth_ctrl_stats(struct dsa_switch *ds, int port, { struct yt921x_priv *priv = to_yt921x_priv(ds); struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib_stats *mib = &pp->mib; + struct yt921x_mib *pm = pp->mib; + struct yt921x_mib_stats *mib; + + if (!pm) + return; + mib = &pm->stats; mutex_lock(&priv->reg_lock); yt921x_mib_read(priv, port); @@ -750,7 +769,12 @@ yt921x_dsa_get_rmon_stats(struct dsa_switch *ds, int port, { struct yt921x_priv *priv = to_yt921x_priv(ds); struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib_stats *mib = &pp->mib; + struct yt921x_mib *pm = pp->mib; + struct yt921x_mib_stats *mib; + + if (!pm) + return; + mib = &pm->stats; mutex_lock(&priv->reg_lock); yt921x_mib_read(priv, port); @@ -786,7 +810,12 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port, { struct yt921x_priv *priv = to_yt921x_priv(ds); struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib_stats *mib = &pp->mib; + struct yt921x_mib *pm = pp->mib; + struct yt921x_mib_stats *mib; + + if (!pm) + return; + mib = &pm->stats; stats->rx_length_errors = mib->rx_undersize_errors + mib->rx_fragment_errors; @@ -802,8 +831,8 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port, /* stats->tx_heartbeat_errors */ stats->tx_window_errors = mib->tx_late_collisions; - stats->rx_packets = pp->rx_frames; - stats->tx_packets = pp->tx_frames; + stats->rx_packets = pm->rx_frames; + stats->tx_packets = pm->tx_frames; stats->rx_bytes = mib->rx_good_bytes - ETH_FCS_LEN * stats->rx_packets; stats->tx_bytes = mib->tx_good_bytes - ETH_FCS_LEN * stats->tx_packets; stats->rx_errors = stats->rx_length_errors + stats->rx_over_errors + @@ -822,7 +851,12 @@ yt921x_dsa_get_pause_stats(struct dsa_switch *ds, int port, { struct yt921x_priv *priv = to_yt921x_priv(ds); struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib_stats *mib = &pp->mib; + struct yt921x_mib *pm = pp->mib; + struct yt921x_mib_stats *mib; + + if (!pm) + return; + mib = &pm->stats; mutex_lock(&priv->reg_lock); yt921x_mib_read(priv, port); @@ -3953,11 +3987,14 @@ yt921x_phylink_mac_link_down(struct phylink_config *config, unsigned int mode, { struct dsa_port *dp = dsa_phylink_to_port(config); struct yt921x_priv *priv = to_yt921x_priv(dp->ds); + struct yt921x_mib *pm; int port = dp->index; int res; /* No need to sync; port control block is hold until device remove */ - cancel_delayed_work(&priv->ports[port].mib_read); + pm = priv->ports[port].mib; + if (pm) + cancel_delayed_work(&pm->work); mutex_lock(&priv->reg_lock); res = yt921x_port_down(priv, port); @@ -3976,6 +4013,7 @@ yt921x_phylink_mac_link_up(struct phylink_config *config, { struct dsa_port *dp = dsa_phylink_to_port(config); struct yt921x_priv *priv = to_yt921x_priv(dp->ds); + struct yt921x_mib *pm; int port = dp->index; int res; @@ -3988,7 +4026,9 @@ yt921x_phylink_mac_link_up(struct phylink_config *config, dev_err(dp->ds->dev, "Failed to %s port %d: %i\n", "bring up", port, res); - schedule_delayed_work(&priv->ports[port].mib_read, 0); + pm = priv->ports[port].mib; + if (pm) + schedule_delayed_work(&pm->work, 0); } static void @@ -4108,11 +4148,35 @@ yt921x_dsa_get_tag_protocol(struct dsa_switch *ds, int port, return DSA_TAG_PROTO_YT921X; } +static void yt921x_dsa_port_teardown(struct dsa_switch *ds, int port) +{ + struct yt921x_priv *priv = to_yt921x_priv(ds); + struct yt921x_port *pp = &priv->ports[port]; + struct yt921x_mib *pm = pp->mib; + + if (pm) + disable_delayed_work_sync(&pm->work); +} + static int yt921x_dsa_port_setup(struct dsa_switch *ds, int port) { struct yt921x_priv *priv = to_yt921x_priv(ds); + struct yt921x_port *pp = &priv->ports[port]; + struct device *dev = to_device(priv); + struct yt921x_mib *pm = pp->mib; int res; + if (!pm && (BIT(port) & (priv->info->internal_mask | + priv->info->external_mask))) { + pm = devm_kzalloc(dev, sizeof(*pm), GFP_KERNEL); + if (!pm) + return -ENOMEM; + pp->mib = pm; + + pm->port = pp; + INIT_DELAYED_WORK(&pm->work, yt921x_mib_poll); + } + mutex_lock(&priv->reg_lock); res = yt921x_port_setup(priv, port); mutex_unlock(&priv->reg_lock); @@ -4667,6 +4731,7 @@ static const struct dsa_switch_ops yt921x_dsa_switch_ops = { /* port */ .get_tag_protocol = yt921x_dsa_get_tag_protocol, .phylink_get_caps = yt921x_dsa_phylink_get_caps, + .port_teardown = yt921x_dsa_port_teardown, .port_setup = yt921x_dsa_port_setup, #if IS_ENABLED(CONFIG_DCB) /* dscp */ @@ -4696,12 +4761,6 @@ static void yt921x_mdio_remove(struct mdio_device *mdiodev) if (!priv) return; - for (size_t i = ARRAY_SIZE(priv->ports); i-- > 0; ) { - struct yt921x_port *pp = &priv->ports[i]; - - disable_delayed_work_sync(&pp->mib_read); - } - dsa_unregister_switch(&priv->ds); for (unsigned int i = 0; i < ARRAY_SIZE(priv->acl_blks); i++) { @@ -4749,7 +4808,6 @@ static int yt921x_mdio_probe(struct mdio_device *mdiodev) struct yt921x_port *pp = &priv->ports[i]; pp->index = i; - INIT_DELAYED_WORK(&pp->mib_read, yt921x_mib_poll); } ds = &priv->ds; diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h index 805febb48683..5bf2d3409f4b 100644 --- a/drivers/net/dsa/motorcomm/chip.h +++ b/drivers/net/dsa/motorcomm/chip.h @@ -913,6 +913,15 @@ struct yt921x_mib_stats { u64 tx_oam; }; +struct yt921x_mib { + struct yt921x_port *port; + + struct delayed_work work; + struct yt921x_mib_stats stats; + u64 rx_frames; + u64 tx_frames; +}; + struct yt921x_acl_entry { u32 key[2]; u32 mask[2]; @@ -939,11 +948,6 @@ struct yt921x_port { bool hairpin; bool isolated; - struct delayed_work mib_read; - struct yt921x_mib_stats mib; - u64 rx_frames; - u64 tx_frames; - #if IS_ENABLED(CONFIG_NET_DSA_YT921X_LEDS) unsigned char led_duty; unsigned short led_cycle; @@ -953,6 +957,8 @@ struct yt921x_port { struct yt921x_led *leds[YT921X_LED_GROUP_NUM]; #endif + + struct yt921x_mib *mib; }; struct yt921x_reg_ops { -- 2.53.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v3 2/6] net: dsa: motorcomm: Split MIB buffers 2026-09-06 13:14 ` [PATCH net-next v3 2/6] net: dsa: motorcomm: Split MIB buffers David Yang @ 2026-09-09 15:16 ` netdev-bot+sashiko 0 siblings, 0 replies; 15+ messages in thread From: netdev-bot+sashiko @ 2026-09-09 15:16 UTC (permalink / raw) To: mmyangfl Cc: netdev, andrew, olteanv, davem, edumazet, kuba, pabeni, linux, linux-kernel Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 1 (1) · Low: 1 (1) - [Medium] Asymmetric use of the counted workqueue disable API: the new .port_teardown callback yt921x_dsa_port_teardown()… - [Low] The descriptor-table comment above yt921x_mib_descs[] says 'Must agree with yt921x_mib', but after this patch `struct yt921x_mib`… Pre-existing issues: - [Medium] The software MIB shadow is not re-zeroed when the hardware MIB counters are cleared: yt921x_chip_setup() issues YT921X_MIB_CTRL_CLEAN… - [Low] Unsynchronized 64-bit statistics: the MIB counters are written by yt921x_mib_read() from the poll worker under priv->reg_lock (plain… -- net: dsa: motorcomm: Split MIB buffers This moves the per-port MIB statistics, the derived rx/tx frame counters and the MIB poll delayed_work out of struct yt921x_port into a new struct yt921x_mib, allocated with devm_kzalloc() from .port_setup() only for internal/external ports. A new .port_teardown() callback replaces the disable loop that used to live in yt921x_mdio_remove(). > diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c > index ad5fe1ccb1d71..95041a2fb0ecc 100644 > --- a/drivers/net/dsa/motorcomm/chip.c > +++ b/drivers/net/dsa/motorcomm/chip.c > @@ -549,9 +549,12 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) > { > struct yt921x_port *pp = &priv->ports[port]; > struct device *dev = to_device(priv); > - struct yt921x_mib_stats *mib = &pp->mib; > + struct yt921x_mib *pm = pp->mib; > + struct yt921x_mib_stats *mib; > int res = 0; > > + mib = &pm->stats; > + > /* Reading of yt921x_port::mib is not protected by a lock and it's vain > * to keep its consistency, since we have to read registers one by one > * and there is no way to make a snapshot of MIB stats. [Severity: Low] Two comments look stale after the split. The header comment above yt921x_mib_descs[] says "Must agree with yt921x_mib", but struct yt921x_mib is now the wrapper holding the port back pointer, the delayed work and the two derived counters. The descriptor offsets are applied to the flat counter layout instead: mib = &pm->stats; ... u64 *valp = &((u64 *)mib)[i]; Should that comment name yt921x_mib_stats now? The comment kept here still says "Reading of yt921x_port::mib is not protected by a lock", although that member is only a pointer after this patch and the lockless data lives in yt921x_mib::stats, rx_frames and tx_frames. This second half is removed later in the series by "net: dsa: motorcomm: Fix MIB synchronization", but the yt921x_mib_descs comment still reads "Must agree with yt921x_mib" at the end of the series. [ ... ] > @@ -786,7 +810,12 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port, > { > struct yt921x_priv *priv = to_yt921x_priv(ds); > struct yt921x_port *pp = &priv->ports[port]; > - struct yt921x_mib_stats *mib = &pp->mib; > + struct yt921x_mib *pm = pp->mib; > + struct yt921x_mib_stats *mib; > + > + if (!pm) > + return; > + mib = &pm->stats; > > stats->rx_length_errors = mib->rx_undersize_errors + > mib->rx_fragment_errors; > @@ -802,8 +831,8 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port, > /* stats->tx_heartbeat_errors */ > stats->tx_window_errors = mib->tx_late_collisions; > > - stats->rx_packets = pp->rx_frames; > - stats->tx_packets = pp->tx_frames; > + stats->rx_packets = pm->rx_frames; > + stats->tx_packets = pm->tx_frames; > stats->rx_bytes = mib->rx_good_bytes - ETH_FCS_LEN * stats->rx_packets; > stats->tx_bytes = mib->tx_good_bytes - ETH_FCS_LEN * stats->tx_packets; [Severity: Low] This isn't a bug introduced by this patch, but can these u64 reads tear on 32-bit? yt921x_mib_read() stores pm->rx_frames and pm->tx_frames with plain assignments from the poll worker, and yt921x_dsa_get_stats64() reads them with neither priv->reg_lock nor a u64_stats_sync retry loop, so a reader can observe halves from two different updates. The subtraction stats->rx_bytes = mib->rx_good_bytes - ETH_FCS_LEN * stats->rx_packets; can then underflow. The baseline had the same lockless read of pp->rx_frames/pp->tx_frames, and later patches in this series ("net: dsa: motorcomm: Use u64_stats_t for MIB stats" and "net: dsa: motorcomm: Fix MIB synchronization") wrap the writer in u64_stats_update_begin()/end() and the reader in u64_stats_fetch_begin()/u64_stats_fetch_retry(), so this is only noted for completeness. [ ... ] > @@ -3988,7 +4026,9 @@ yt921x_phylink_mac_link_up(struct phylink_config *config, > dev_err(dp->ds->dev, "Failed to %s port %d: %i\n", "bring up", > port, res); > > - schedule_delayed_work(&priv->ports[port].mib_read, 0); > + pm = priv->ports[port].mib; > + if (pm) > + schedule_delayed_work(&pm->work, 0); > } > > static void > @@ -4108,11 +4148,35 @@ yt921x_dsa_get_tag_protocol(struct dsa_switch *ds, int port, > return DSA_TAG_PROTO_YT921X; > } > > +static void yt921x_dsa_port_teardown(struct dsa_switch *ds, int port) > +{ > + struct yt921x_priv *priv = to_yt921x_priv(ds); > + struct yt921x_port *pp = &priv->ports[port]; > + struct yt921x_mib *pm = pp->mib; > + > + if (pm) > + disable_delayed_work_sync(&pm->work); > +} > + > static int yt921x_dsa_port_setup(struct dsa_switch *ds, int port) > { > struct yt921x_priv *priv = to_yt921x_priv(ds); > + struct yt921x_port *pp = &priv->ports[port]; > + struct device *dev = to_device(priv); > + struct yt921x_mib *pm = pp->mib; > int res; > > + if (!pm && (BIT(port) & (priv->info->internal_mask | > + priv->info->external_mask))) { > + pm = devm_kzalloc(dev, sizeof(*pm), GFP_KERNEL); > + if (!pm) > + return -ENOMEM; > + pp->mib = pm; > + > + pm->port = pp; > + INIT_DELAYED_WORK(&pm->work, yt921x_mib_poll); > + } > + [Severity: Medium] Is the disable/enable pair here asymmetric? yt921x_dsa_port_teardown() calls disable_delayed_work_sync(&pm->work), but yt921x_dsa_port_setup() has no matching enable_delayed_work(), and the "if (!pm && ...)" gate means an already allocated pm (and its work item, including the disable counter) is reused untouched. The disable count is a plain counter in the work data, documented in kernel/workqueue.c as: * Disable @work by incrementing its disable count and cancel it if currently * pending. As long as the disable count is non-zero, any attempt to queue @work * will fail and return %false. with work_offqd_disable()/work_offqd_enable() incrementing/decrementing it and enable_delayed_work() being the only decrement. So after a single teardown/setup cycle on a still-live switch: yt921x_phylink_mac_link_up() schedule_delayed_work(&pm->work, 0); /* returns false, forever */ yt921x_mib_poll() never runs again and the cached counters read by yt921x_dsa_get_stats64() and the ethtool callbacks stay frozen, including the 32-bit wrap accumulation done in yt921x_mib_read(). Two paths where DSA runs port_setup again for the same priv: net/dsa/dsa.c:dsa_tree_setup_ports() { ... err = dsa_port_setup(dp); if (err) { err = dsa_port_setup_as_unused(dp); ... } dsa_port_setup() already ran the driver's port_teardown on its error path (dsa_port_devlink_setup() calls dsa_port_devlink_teardown() when devlink_port_register() fails), and then port_setup is called again. In a multi-switch tree, dsa_switch_remove() -> dsa_tree_teardown() tears down the ports of the surviving switches too, and a later re-probe runs dsa_tree_setup() again, calling port_setup() for a port whose pp->mib is already non-NULL. Should port_setup() call enable_delayed_work(&pm->work) when it reuses an existing pm? As written, each cycle also bumps the disable depth monotonically until it hits WORK_OFFQ_DISABLE_BITS and WARN_ONCE fires. [Severity: Medium] This is a pre-existing issue, but should the shadow counters be re-zeroed here rather than only on first allocation? yt921x_chip_setup() clears the hardware MIB on every .setup(): ctrl = YT921X_MIB_CTRL_CLEAN | YT921X_MIB_CTRL_ALL_PORT; res = yt921x_reg_write(priv, YT921X_MIB_CTRL, ctrl); while pm->stats is zeroed only by the devm_kzalloc() inside the "if (!pm && ...)" branch. dsa_switch_teardown() clears ds->setup, so a second dsa_tree_setup() can call ds->ops->setup again for the same priv (for example when a sibling switch in the tree is removed and re-probed). The retained shadow then holds pre-clean values while the hardware restarts at zero, and the first read applies the wrap correction: drivers/net/dsa/motorcomm/chip.c:yt921x_mib_read() { ... val = (old_val & ~(u64)U32_MAX) | val0; if (val < old_val) val += 1ull << 32; ... } adding a spurious 2^32 to every affected counter. The baseline had the same behaviour with the shadow embedded in priv. [ ... ] > diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h > index 805febb486838..5bf2d3409f4bb 100644 > --- a/drivers/net/dsa/motorcomm/chip.h > +++ b/drivers/net/dsa/motorcomm/chip.h > @@ -913,6 +913,15 @@ struct yt921x_mib_stats { > u64 tx_oam; > }; > > +struct yt921x_mib { > + struct yt921x_port *port; > + > + struct delayed_work work; > + struct yt921x_mib_stats stats; > + u64 rx_frames; > + u64 tx_frames; > +}; > + [ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906131414.2683362-1-mmyangfl%40gmail.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net-next v3 3/6] net: dsa: motorcomm: Split MIB module 2026-09-06 13:14 [PATCH net-next v3 0/6] net: dsa: motorcomm: MIB fixup David Yang 2026-09-06 13:14 ` [PATCH net-next v3 1/6] net: dsa: motorcomm: Rename MIB stuffs David Yang 2026-09-06 13:14 ` [PATCH net-next v3 2/6] net: dsa: motorcomm: Split MIB buffers David Yang @ 2026-09-06 13:14 ` David Yang 2026-09-09 15:16 ` netdev-bot+sashiko 2026-09-06 13:14 ` [PATCH net-next v3 4/6] net: dsa: motorcomm: Use u64_stats_t for MIB stats David Yang ` (2 subsequent siblings) 5 siblings, 1 reply; 15+ messages in thread From: David Yang @ 2026-09-06 13:14 UTC (permalink / raw) To: netdev Cc: David Yang, Andrew Lunn, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Russell King, linux-kernel yt922x is going to use a different MIB scheme, split it to reduce the length of chip.c. Signed-off-by: David Yang <mmyangfl@gmail.com> --- drivers/net/dsa/motorcomm/Makefile | 1 + drivers/net/dsa/motorcomm/chip.c | 398 +--------------------------- drivers/net/dsa/motorcomm/chip.h | 124 --------- drivers/net/dsa/motorcomm/mib.c | 408 +++++++++++++++++++++++++++++ drivers/net/dsa/motorcomm/mib.h | 159 +++++++++++ 5 files changed, 569 insertions(+), 521 deletions(-) create mode 100644 drivers/net/dsa/motorcomm/mib.c create mode 100644 drivers/net/dsa/motorcomm/mib.h diff --git a/drivers/net/dsa/motorcomm/Makefile b/drivers/net/dsa/motorcomm/Makefile index aeb12cb91f93..5c2d34ec05fc 100644 --- a/drivers/net/dsa/motorcomm/Makefile +++ b/drivers/net/dsa/motorcomm/Makefile @@ -2,4 +2,5 @@ obj-$(CONFIG_NET_DSA_YT921X) += yt921x.o yt921x-objs := chip.o yt921x-$(CONFIG_NET_DSA_YT921X_LEDS) += leds.o +yt921x-objs += mib.o yt921x-objs += smi.o diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c index 95041a2fb0ec..e9730b9f8c62 100644 --- a/drivers/net/dsa/motorcomm/chip.c +++ b/drivers/net/dsa/motorcomm/chip.c @@ -27,75 +27,9 @@ #include "chip.h" #include "leds.h" +#include "mib.h" #include "smi.h" -struct yt921x_mib_desc { - unsigned int size; - unsigned int offset; - const char *name; -}; - -#define MIB_DESC(_size, _offset, _name) \ - {_size, _offset, _name} - -/* Must agree with yt921x_mib - * - * Unstructured fields (name != NULL) will appear in get_ethtool_stats(), - * structured go to their *_stats() methods, but we need their sizes and offsets - * to perform 32bit MIB overflow wraparound. - */ -static const struct yt921x_mib_desc yt921x_mib_descs[] = { - MIB_DESC(1, YT921X_MIB_DATA_RX_BROADCAST, NULL), - MIB_DESC(1, YT921X_MIB_DATA_RX_PAUSE, NULL), - MIB_DESC(1, YT921X_MIB_DATA_RX_MULTICAST, NULL), - MIB_DESC(1, YT921X_MIB_DATA_RX_CRC_ERR, NULL), - - MIB_DESC(1, YT921X_MIB_DATA_RX_ALIGN_ERR, NULL), - MIB_DESC(1, YT921X_MIB_DATA_RX_UNDERSIZE_ERR, NULL), - MIB_DESC(1, YT921X_MIB_DATA_RX_FRAG_ERR, NULL), - MIB_DESC(1, YT921X_MIB_DATA_RX_PKT_SZ_64, NULL), - - MIB_DESC(1, YT921X_MIB_DATA_RX_PKT_SZ_65_TO_127, NULL), - MIB_DESC(1, YT921X_MIB_DATA_RX_PKT_SZ_128_TO_255, NULL), - MIB_DESC(1, YT921X_MIB_DATA_RX_PKT_SZ_256_TO_511, NULL), - MIB_DESC(1, YT921X_MIB_DATA_RX_PKT_SZ_512_TO_1023, NULL), - - MIB_DESC(1, YT921X_MIB_DATA_RX_PKT_SZ_1024_TO_1518, NULL), - MIB_DESC(1, YT921X_MIB_DATA_RX_PKT_SZ_1519_TO_MAX, NULL), - MIB_DESC(2, YT921X_MIB_DATA_RX_GOOD_BYTES, NULL), - - MIB_DESC(2, YT921X_MIB_DATA_RX_BAD_BYTES, "RxBadBytes"), - MIB_DESC(1, YT921X_MIB_DATA_RX_OVERSIZE_ERR, NULL), - - MIB_DESC(1, YT921X_MIB_DATA_RX_DROPPED, NULL), - MIB_DESC(1, YT921X_MIB_DATA_TX_BROADCAST, NULL), - MIB_DESC(1, YT921X_MIB_DATA_TX_PAUSE, NULL), - MIB_DESC(1, YT921X_MIB_DATA_TX_MULTICAST, NULL), - - MIB_DESC(1, YT921X_MIB_DATA_TX_UNDERSIZE_ERR, NULL), - MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_64, NULL), - MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_65_TO_127, NULL), - MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_128_TO_255, NULL), - - MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_256_TO_511, NULL), - MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_512_TO_1023, NULL), - MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_1024_TO_1518, NULL), - MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_1519_TO_MAX, NULL), - - MIB_DESC(2, YT921X_MIB_DATA_TX_GOOD_BYTES, NULL), - MIB_DESC(1, YT921X_MIB_DATA_TX_COLLISION, NULL), - - MIB_DESC(1, YT921X_MIB_DATA_TX_EXCESSIVE_COLLISION, NULL), - MIB_DESC(1, YT921X_MIB_DATA_TX_MULTIPLE_COLLISION, NULL), - MIB_DESC(1, YT921X_MIB_DATA_TX_SINGLE_COLLISION, NULL), - MIB_DESC(1, YT921X_MIB_DATA_TX_PKT, NULL), - - MIB_DESC(1, YT921X_MIB_DATA_TX_DEFERRED, NULL), - MIB_DESC(1, YT921X_MIB_DATA_TX_LATE_COLLISION, NULL), - MIB_DESC(1, YT921X_MIB_DATA_RX_OAM, "RxOAM"), - MIB_DESC(1, YT921X_MIB_DATA_TX_OAM, "TxOAM"), -}; - struct yt921x_info { const char *name; u16 major; @@ -154,14 +88,6 @@ static const struct yt921x_info yt921x_infos[] = { #define YT921X_VID_UNWARE 4095 -/* The interval should be small enough to avoid overflow of 32bit MIBs. - * - * 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. - */ -#define YT921X_STATS_INTERVAL_JIFFIES (3 * HZ) - struct yt921x_reg_mdio { struct mii_bus *bus; int addr; @@ -544,328 +470,6 @@ yt921x_mbus_ext_init(struct yt921x_priv *priv, struct device_node *mnp) return 0; } -/* Read and handle overflow of 32bit MIBs. MIB buffer must be zeroed before. */ -static int yt921x_mib_read(struct yt921x_priv *priv, int port) -{ - struct yt921x_port *pp = &priv->ports[port]; - struct device *dev = to_device(priv); - struct yt921x_mib *pm = pp->mib; - struct yt921x_mib_stats *mib; - int res = 0; - - mib = &pm->stats; - - /* Reading of yt921x_port::mib is not protected by a lock and it's vain - * to keep its consistency, since we have to read registers one by one - * and there is no way to make a snapshot of MIB stats. - * - * Writing (by this function only) is and should be protected by - * reg_lock. - */ - - for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) { - const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i]; - u32 reg = YT921X_MIBn_DATA0(port) + desc->offset; - u64 *valp = &((u64 *)mib)[i]; - u32 val0; - u64 val; - - res = yt921x_reg_read(priv, reg, &val0); - if (res) - break; - - if (desc->size <= 1) { - u64 old_val = *valp; - - val = (old_val & ~(u64)U32_MAX) | val0; - if (val < old_val) - val += 1ull << 32; - } else { - u32 val1; - - res = yt921x_reg_read(priv, reg + 4, &val1); - if (res) - break; - val = ((u64)val1 << 32) | val0; - } - - WRITE_ONCE(*valp, val); - } - - pm->rx_frames = mib->rx_64byte + mib->rx_65_127byte + - mib->rx_128_255byte + mib->rx_256_511byte + - mib->rx_512_1023byte + mib->rx_1024_1518byte + - mib->rx_jumbo; - pm->tx_frames = mib->tx_64byte + mib->tx_65_127byte + - mib->tx_128_255byte + mib->tx_256_511byte + - mib->tx_512_1023byte + mib->tx_1024_1518byte + - mib->tx_jumbo; - - if (res) - dev_err(dev, "Failed to %s port %d: %i\n", "read stats for", - port, res); - return res; -} - -static void yt921x_mib_poll(struct work_struct *work) -{ - struct yt921x_mib *pm = container_of_const(work, struct yt921x_mib, - work.work); - struct yt921x_port *pp = pm->port; - struct yt921x_priv *priv = container_of_const(pp, struct yt921x_priv, - ports[pp->index]); - unsigned long delay = YT921X_STATS_INTERVAL_JIFFIES; - int port = pp->index; - int res; - - mutex_lock(&priv->reg_lock); - res = yt921x_mib_read(priv, port); - mutex_unlock(&priv->reg_lock); - if (res) - delay *= 4; - - schedule_delayed_work(&pm->work, delay); -} - -static void -yt921x_dsa_get_strings(struct dsa_switch *ds, int port, u32 stringset, - uint8_t *data) -{ - if (stringset != ETH_SS_STATS) - return; - - for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) { - const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i]; - - if (desc->name) - ethtool_puts(&data, desc->name); - } -} - -static void -yt921x_dsa_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data) -{ - struct yt921x_priv *priv = to_yt921x_priv(ds); - struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib *pm = pp->mib; - struct yt921x_mib_stats *mib; - size_t j; - - if (!pm) - return; - mib = &pm->stats; - - mutex_lock(&priv->reg_lock); - yt921x_mib_read(priv, port); - mutex_unlock(&priv->reg_lock); - - j = 0; - for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) { - const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i]; - - if (!desc->name) - continue; - - data[j] = ((u64 *)mib)[i]; - j++; - } -} - -static int yt921x_dsa_get_sset_count(struct dsa_switch *ds, int port, int sset) -{ - int cnt = 0; - - if (sset != ETH_SS_STATS) - return 0; - - for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) { - const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i]; - - if (desc->name) - cnt++; - } - - return cnt; -} - -static void -yt921x_dsa_get_eth_mac_stats(struct dsa_switch *ds, int port, - struct ethtool_eth_mac_stats *mac_stats) -{ - struct yt921x_priv *priv = to_yt921x_priv(ds); - struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib *pm = pp->mib; - struct yt921x_mib_stats *mib; - - if (!pm) - return; - mib = &pm->stats; - - mutex_lock(&priv->reg_lock); - yt921x_mib_read(priv, port); - mutex_unlock(&priv->reg_lock); - - mac_stats->FramesTransmittedOK = pm->tx_frames; - mac_stats->SingleCollisionFrames = mib->tx_single_collisions; - mac_stats->MultipleCollisionFrames = mib->tx_multiple_collisions; - mac_stats->FramesReceivedOK = pm->rx_frames; - mac_stats->FrameCheckSequenceErrors = mib->rx_crc_errors; - mac_stats->AlignmentErrors = mib->rx_alignment_errors; - mac_stats->OctetsTransmittedOK = mib->tx_good_bytes; - mac_stats->FramesWithDeferredXmissions = mib->tx_deferred; - mac_stats->LateCollisions = mib->tx_late_collisions; - mac_stats->FramesAbortedDueToXSColls = mib->tx_aborted_errors; - /* mac_stats->FramesLostDueToIntMACXmitError */ - /* mac_stats->CarrierSenseErrors */ - mac_stats->OctetsReceivedOK = mib->rx_good_bytes; - /* mac_stats->FramesLostDueToIntMACRcvError */ - mac_stats->MulticastFramesXmittedOK = mib->tx_multicast; - mac_stats->BroadcastFramesXmittedOK = mib->tx_broadcast; - /* mac_stats->FramesWithExcessiveDeferral */ - mac_stats->MulticastFramesReceivedOK = mib->rx_multicast; - mac_stats->BroadcastFramesReceivedOK = mib->rx_broadcast; - /* mac_stats->InRangeLengthErrors */ - /* mac_stats->OutOfRangeLengthField */ - mac_stats->FrameTooLongErrors = mib->rx_oversize_errors; -} - -static void -yt921x_dsa_get_eth_ctrl_stats(struct dsa_switch *ds, int port, - struct ethtool_eth_ctrl_stats *ctrl_stats) -{ - struct yt921x_priv *priv = to_yt921x_priv(ds); - struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib *pm = pp->mib; - struct yt921x_mib_stats *mib; - - if (!pm) - return; - mib = &pm->stats; - - mutex_lock(&priv->reg_lock); - yt921x_mib_read(priv, port); - mutex_unlock(&priv->reg_lock); - - ctrl_stats->MACControlFramesTransmitted = mib->tx_pause; - ctrl_stats->MACControlFramesReceived = mib->rx_pause; - /* ctrl_stats->UnsupportedOpcodesReceived */ -} - -static const struct ethtool_rmon_hist_range yt921x_rmon_ranges[] = { - { 0, 64 }, - { 65, 127 }, - { 128, 255 }, - { 256, 511 }, - { 512, 1023 }, - { 1024, 1518 }, - { 1519, YT921X_FRAME_SIZE_MAX }, - {} -}; - -static void -yt921x_dsa_get_rmon_stats(struct dsa_switch *ds, int port, - struct ethtool_rmon_stats *rmon_stats, - const struct ethtool_rmon_hist_range **ranges) -{ - struct yt921x_priv *priv = to_yt921x_priv(ds); - struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib *pm = pp->mib; - struct yt921x_mib_stats *mib; - - if (!pm) - return; - mib = &pm->stats; - - mutex_lock(&priv->reg_lock); - yt921x_mib_read(priv, port); - mutex_unlock(&priv->reg_lock); - - *ranges = yt921x_rmon_ranges; - - rmon_stats->undersize_pkts = mib->rx_undersize_errors; - rmon_stats->oversize_pkts = mib->rx_oversize_errors; - rmon_stats->fragments = mib->rx_alignment_errors; - /* rmon_stats->jabbers */ - - rmon_stats->hist[0] = mib->rx_64byte; - rmon_stats->hist[1] = mib->rx_65_127byte; - rmon_stats->hist[2] = mib->rx_128_255byte; - rmon_stats->hist[3] = mib->rx_256_511byte; - rmon_stats->hist[4] = mib->rx_512_1023byte; - rmon_stats->hist[5] = mib->rx_1024_1518byte; - rmon_stats->hist[6] = mib->rx_jumbo; - - rmon_stats->hist_tx[0] = mib->tx_64byte; - rmon_stats->hist_tx[1] = mib->tx_65_127byte; - rmon_stats->hist_tx[2] = mib->tx_128_255byte; - rmon_stats->hist_tx[3] = mib->tx_256_511byte; - rmon_stats->hist_tx[4] = mib->tx_512_1023byte; - rmon_stats->hist_tx[5] = mib->tx_1024_1518byte; - rmon_stats->hist_tx[6] = mib->tx_jumbo; -} - -static void -yt921x_dsa_get_stats64(struct dsa_switch *ds, int port, - struct rtnl_link_stats64 *stats) -{ - struct yt921x_priv *priv = to_yt921x_priv(ds); - struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib *pm = pp->mib; - struct yt921x_mib_stats *mib; - - if (!pm) - return; - mib = &pm->stats; - - stats->rx_length_errors = mib->rx_undersize_errors + - mib->rx_fragment_errors; - stats->rx_over_errors = mib->rx_oversize_errors; - stats->rx_crc_errors = mib->rx_crc_errors; - stats->rx_frame_errors = mib->rx_alignment_errors; - /* stats->rx_fifo_errors */ - /* stats->rx_missed_errors */ - - stats->tx_aborted_errors = mib->tx_aborted_errors; - /* stats->tx_carrier_errors */ - stats->tx_fifo_errors = mib->tx_undersize_errors; - /* stats->tx_heartbeat_errors */ - stats->tx_window_errors = mib->tx_late_collisions; - - stats->rx_packets = pm->rx_frames; - stats->tx_packets = pm->tx_frames; - stats->rx_bytes = mib->rx_good_bytes - ETH_FCS_LEN * stats->rx_packets; - stats->tx_bytes = mib->tx_good_bytes - ETH_FCS_LEN * stats->tx_packets; - stats->rx_errors = stats->rx_length_errors + stats->rx_over_errors + - stats->rx_crc_errors + stats->rx_frame_errors; - stats->tx_errors = stats->tx_aborted_errors + stats->tx_fifo_errors + - stats->tx_window_errors; - stats->rx_dropped = mib->rx_dropped; - /* stats->tx_dropped */ - stats->multicast = mib->rx_multicast; - stats->collisions = mib->tx_collisions; -} - -static void -yt921x_dsa_get_pause_stats(struct dsa_switch *ds, int port, - struct ethtool_pause_stats *pause_stats) -{ - struct yt921x_priv *priv = to_yt921x_priv(ds); - struct yt921x_port *pp = &priv->ports[port]; - struct yt921x_mib *pm = pp->mib; - struct yt921x_mib_stats *mib; - - if (!pm) - return; - mib = &pm->stats; - - mutex_lock(&priv->reg_lock); - yt921x_mib_read(priv, port); - mutex_unlock(&priv->reg_lock); - - pause_stats->tx_pause_frames = mib->tx_pause; - pause_stats->rx_pause_frames = mib->rx_pause; -} - static int yt921x_set_eee(struct yt921x_priv *priv, int port, struct ethtool_keee *e) { diff --git a/drivers/net/dsa/motorcomm/chip.h b/drivers/net/dsa/motorcomm/chip.h index 5bf2d3409f4b..ce63865bb905 100644 --- a/drivers/net/dsa/motorcomm/chip.h +++ b/drivers/net/dsa/motorcomm/chip.h @@ -167,69 +167,6 @@ #define YT921X_EEE_CTRL 0xb0000 #define YT921X_EEE_CTRL_ENn(port) BIT(port) -#define YT921X_MIB_CTRL 0xc0004 -#define YT921X_MIB_CTRL_CLEAN BIT(30) -#define YT921X_MIB_CTRL_PORT_M GENMASK(6, 3) -#define YT921X_MIB_CTRL_PORT(x) FIELD_PREP(YT921X_MIB_CTRL_PORT_M, (x)) -#define YT921X_MIB_CTRL_ONE_PORT BIT(1) -#define YT921X_MIB_CTRL_ALL_PORT BIT(0) -#define YT921X_MIBn_DATA0(port) (0xc0100 + 0x100 * (port)) -#define YT921X_MIBn_DATAm(port, x) (YT921X_MIBn_DATA0(port) + 4 * (x)) -#define YT921X_MIB_DATA_RX_BROADCAST 0x00 -#define YT921X_MIB_DATA_RX_PAUSE 0x04 -#define YT921X_MIB_DATA_RX_MULTICAST 0x08 -#define YT921X_MIB_DATA_RX_CRC_ERR 0x0c - -#define YT921X_MIB_DATA_RX_ALIGN_ERR 0x10 -#define YT921X_MIB_DATA_RX_UNDERSIZE_ERR 0x14 -#define YT921X_MIB_DATA_RX_FRAG_ERR 0x18 -#define YT921X_MIB_DATA_RX_PKT_SZ_64 0x1c - -#define YT921X_MIB_DATA_RX_PKT_SZ_65_TO_127 0x20 -#define YT921X_MIB_DATA_RX_PKT_SZ_128_TO_255 0x24 -#define YT921X_MIB_DATA_RX_PKT_SZ_256_TO_511 0x28 -#define YT921X_MIB_DATA_RX_PKT_SZ_512_TO_1023 0x2c - -#define YT921X_MIB_DATA_RX_PKT_SZ_1024_TO_1518 0x30 -#define YT921X_MIB_DATA_RX_PKT_SZ_1519_TO_MAX 0x34 -/* 0x38: unused */ -#define YT921X_MIB_DATA_RX_GOOD_BYTES 0x3c - -/* 0x40: 64 bytes */ -#define YT921X_MIB_DATA_RX_BAD_BYTES 0x44 -/* 0x48: 64 bytes */ -#define YT921X_MIB_DATA_RX_OVERSIZE_ERR 0x4c - -#define YT921X_MIB_DATA_RX_DROPPED 0x50 -#define YT921X_MIB_DATA_TX_BROADCAST 0x54 -#define YT921X_MIB_DATA_TX_PAUSE 0x58 -#define YT921X_MIB_DATA_TX_MULTICAST 0x5c - -#define YT921X_MIB_DATA_TX_UNDERSIZE_ERR 0x60 -#define YT921X_MIB_DATA_TX_PKT_SZ_64 0x64 -#define YT921X_MIB_DATA_TX_PKT_SZ_65_TO_127 0x68 -#define YT921X_MIB_DATA_TX_PKT_SZ_128_TO_255 0x6c - -#define YT921X_MIB_DATA_TX_PKT_SZ_256_TO_511 0x70 -#define YT921X_MIB_DATA_TX_PKT_SZ_512_TO_1023 0x74 -#define YT921X_MIB_DATA_TX_PKT_SZ_1024_TO_1518 0x78 -#define YT921X_MIB_DATA_TX_PKT_SZ_1519_TO_MAX 0x7c - -/* 0x80: unused */ -#define YT921X_MIB_DATA_TX_GOOD_BYTES 0x84 -/* 0x88: 64 bytes */ -#define YT921X_MIB_DATA_TX_COLLISION 0x8c - -#define YT921X_MIB_DATA_TX_EXCESSIVE_COLLISION 0x90 -#define YT921X_MIB_DATA_TX_MULTIPLE_COLLISION 0x94 -#define YT921X_MIB_DATA_TX_SINGLE_COLLISION 0x98 -#define YT921X_MIB_DATA_TX_PKT 0x9c - -#define YT921X_MIB_DATA_TX_DEFERRED 0xa0 -#define YT921X_MIB_DATA_TX_LATE_COLLISION 0xa4 -#define YT921X_MIB_DATA_RX_OAM 0xa8 -#define YT921X_MIB_DATA_TX_OAM 0xac - #define YT921X_EDATA_CTRL 0xe0000 #define YT921X_EDATA_CTRL_ADDR_M GENMASK(15, 8) #define YT921X_EDATA_CTRL_ADDR(x) FIELD_PREP(YT921X_EDATA_CTRL_ADDR_M, (x)) @@ -861,67 +798,6 @@ enum yt921x_fdb_entry_status { #define yt921x_port_is_internal(port) ((port) < 8) #define yt921x_port_is_external(port) ((port) == 8 || (port) == 9) -struct yt921x_mib_stats { - u64 rx_broadcast; - u64 rx_pause; - u64 rx_multicast; - u64 rx_crc_errors; - - u64 rx_alignment_errors; - u64 rx_undersize_errors; - u64 rx_fragment_errors; - u64 rx_64byte; - - u64 rx_65_127byte; - u64 rx_128_255byte; - u64 rx_256_511byte; - u64 rx_512_1023byte; - - u64 rx_1024_1518byte; - u64 rx_jumbo; - u64 rx_good_bytes; - - u64 rx_bad_bytes; - u64 rx_oversize_errors; - - u64 rx_dropped; - u64 tx_broadcast; - u64 tx_pause; - u64 tx_multicast; - - u64 tx_undersize_errors; - u64 tx_64byte; - u64 tx_65_127byte; - u64 tx_128_255byte; - - u64 tx_256_511byte; - u64 tx_512_1023byte; - u64 tx_1024_1518byte; - u64 tx_jumbo; - - u64 tx_good_bytes; - u64 tx_collisions; - - u64 tx_aborted_errors; - u64 tx_multiple_collisions; - u64 tx_single_collisions; - u64 tx_good; - - u64 tx_deferred; - u64 tx_late_collisions; - u64 rx_oam; - u64 tx_oam; -}; - -struct yt921x_mib { - struct yt921x_port *port; - - struct delayed_work work; - struct yt921x_mib_stats stats; - u64 rx_frames; - u64 tx_frames; -}; - struct yt921x_acl_entry { u32 key[2]; u32 mask[2]; diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c new file mode 100644 index 000000000000..b1296f224cf6 --- /dev/null +++ b/drivers/net/dsa/motorcomm/mib.c @@ -0,0 +1,408 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (c) 2026 David Yang + */ + +#include "chip.h" +#include "mib.h" +#include "smi.h" + +struct yt921x_mib_desc { + unsigned int size; + unsigned int offset; + const char *name; +}; + +#define MIB_DESC(_size, _offset, _name) \ + {_size, _offset, _name} + +/* Must agree with yt921x_mib + * + * Unstructured fields (name != NULL) will appear in get_ethtool_stats(), + * structured go to their *_stats() methods, but we need their sizes and offsets + * to perform 32bit MIB overflow wraparound. + */ +static const struct yt921x_mib_desc yt921x_mib_descs[] = { + MIB_DESC(1, YT921X_MIB_DATA_RX_BROADCAST, NULL), + MIB_DESC(1, YT921X_MIB_DATA_RX_PAUSE, NULL), + MIB_DESC(1, YT921X_MIB_DATA_RX_MULTICAST, NULL), + MIB_DESC(1, YT921X_MIB_DATA_RX_CRC_ERR, NULL), + + MIB_DESC(1, YT921X_MIB_DATA_RX_ALIGN_ERR, NULL), + MIB_DESC(1, YT921X_MIB_DATA_RX_UNDERSIZE_ERR, NULL), + MIB_DESC(1, YT921X_MIB_DATA_RX_FRAG_ERR, NULL), + MIB_DESC(1, YT921X_MIB_DATA_RX_PKT_SZ_64, NULL), + + MIB_DESC(1, YT921X_MIB_DATA_RX_PKT_SZ_65_TO_127, NULL), + MIB_DESC(1, YT921X_MIB_DATA_RX_PKT_SZ_128_TO_255, NULL), + MIB_DESC(1, YT921X_MIB_DATA_RX_PKT_SZ_256_TO_511, NULL), + MIB_DESC(1, YT921X_MIB_DATA_RX_PKT_SZ_512_TO_1023, NULL), + + MIB_DESC(1, YT921X_MIB_DATA_RX_PKT_SZ_1024_TO_1518, NULL), + MIB_DESC(1, YT921X_MIB_DATA_RX_PKT_SZ_1519_TO_MAX, NULL), + MIB_DESC(2, YT921X_MIB_DATA_RX_GOOD_BYTES, NULL), + + MIB_DESC(2, YT921X_MIB_DATA_RX_BAD_BYTES, "RxBadBytes"), + MIB_DESC(1, YT921X_MIB_DATA_RX_OVERSIZE_ERR, NULL), + + MIB_DESC(1, YT921X_MIB_DATA_RX_DROPPED, NULL), + MIB_DESC(1, YT921X_MIB_DATA_TX_BROADCAST, NULL), + MIB_DESC(1, YT921X_MIB_DATA_TX_PAUSE, NULL), + MIB_DESC(1, YT921X_MIB_DATA_TX_MULTICAST, NULL), + + MIB_DESC(1, YT921X_MIB_DATA_TX_UNDERSIZE_ERR, NULL), + MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_64, NULL), + MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_65_TO_127, NULL), + MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_128_TO_255, NULL), + + MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_256_TO_511, NULL), + MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_512_TO_1023, NULL), + MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_1024_TO_1518, NULL), + MIB_DESC(1, YT921X_MIB_DATA_TX_PKT_SZ_1519_TO_MAX, NULL), + + MIB_DESC(2, YT921X_MIB_DATA_TX_GOOD_BYTES, NULL), + MIB_DESC(1, YT921X_MIB_DATA_TX_COLLISION, NULL), + + MIB_DESC(1, YT921X_MIB_DATA_TX_EXCESSIVE_COLLISION, NULL), + MIB_DESC(1, YT921X_MIB_DATA_TX_MULTIPLE_COLLISION, NULL), + MIB_DESC(1, YT921X_MIB_DATA_TX_SINGLE_COLLISION, NULL), + MIB_DESC(1, YT921X_MIB_DATA_TX_PKT, NULL), + + MIB_DESC(1, YT921X_MIB_DATA_TX_DEFERRED, NULL), + MIB_DESC(1, YT921X_MIB_DATA_TX_LATE_COLLISION, NULL), + MIB_DESC(1, YT921X_MIB_DATA_RX_OAM, "RxOAM"), + MIB_DESC(1, YT921X_MIB_DATA_TX_OAM, "TxOAM"), +}; + +/* The interval should be small enough to avoid overflow of 32bit MIBs. + * + * 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. + */ +#define YT921X_STATS_INTERVAL_JIFFIES (3 * HZ) + +#define to_yt921x_priv(_ds) container_of_const(_ds, struct yt921x_priv, ds) +#define to_device(priv) ((priv)->ds.dev) + +/* Read and handle overflow of 32bit MIBs. MIB buffer must be zeroed before. */ +static int yt921x_mib_read(struct yt921x_priv *priv, int port) +{ + struct yt921x_port *pp = &priv->ports[port]; + struct device *dev = to_device(priv); + struct yt921x_mib *pm = pp->mib; + struct yt921x_mib_stats *mib; + int res = 0; + + mib = &pm->stats; + + /* Reading of yt921x_port::mib is not protected by a lock and it's vain + * to keep its consistency, since we have to read registers one by one + * and there is no way to make a snapshot of MIB stats. + * + * Writing (by this function only) is and should be protected by + * reg_lock. + */ + + for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) { + const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i]; + u32 reg = YT921X_MIBn_DATA0(port) + desc->offset; + u64 *valp = &((u64 *)mib)[i]; + u32 val0; + u64 val; + + res = yt921x_reg_read(priv, reg, &val0); + if (res) + break; + + if (desc->size <= 1) { + u64 old_val = *valp; + + val = (old_val & ~(u64)U32_MAX) | val0; + if (val < old_val) + val += 1ull << 32; + } else { + u32 val1; + + res = yt921x_reg_read(priv, reg + 4, &val1); + if (res) + break; + val = ((u64)val1 << 32) | val0; + } + + WRITE_ONCE(*valp, val); + } + + pm->rx_frames = mib->rx_64byte + mib->rx_65_127byte + + mib->rx_128_255byte + mib->rx_256_511byte + + mib->rx_512_1023byte + mib->rx_1024_1518byte + + mib->rx_jumbo; + pm->tx_frames = mib->tx_64byte + mib->tx_65_127byte + + mib->tx_128_255byte + mib->tx_256_511byte + + mib->tx_512_1023byte + mib->tx_1024_1518byte + + mib->tx_jumbo; + + if (res) + dev_err(dev, "Failed to %s port %d: %i\n", "read stats for", + port, res); + return res; +} + +void yt921x_mib_poll(struct work_struct *work) +{ + struct yt921x_mib *pm = container_of_const(work, struct yt921x_mib, + work.work); + struct yt921x_port *pp = pm->port; + struct yt921x_priv *priv = container_of_const(pp, struct yt921x_priv, + ports[pp->index]); + unsigned long delay = YT921X_STATS_INTERVAL_JIFFIES; + int port = pp->index; + int res; + + mutex_lock(&priv->reg_lock); + res = yt921x_mib_read(priv, port); + mutex_unlock(&priv->reg_lock); + if (res) + delay *= 4; + + schedule_delayed_work(&pm->work, delay); +} + +void +yt921x_dsa_get_strings(struct dsa_switch *ds, int port, u32 stringset, + uint8_t *data) +{ + if (stringset != ETH_SS_STATS) + return; + + for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) { + const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i]; + + if (desc->name) + ethtool_puts(&data, desc->name); + } +} + +void +yt921x_dsa_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data) +{ + struct yt921x_priv *priv = to_yt921x_priv(ds); + struct yt921x_port *pp = &priv->ports[port]; + struct yt921x_mib *pm = pp->mib; + struct yt921x_mib_stats *mib; + size_t j; + + if (!pm) + return; + mib = &pm->stats; + + mutex_lock(&priv->reg_lock); + yt921x_mib_read(priv, port); + mutex_unlock(&priv->reg_lock); + + j = 0; + for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) { + const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i]; + + if (!desc->name) + continue; + + data[j] = ((u64 *)mib)[i]; + j++; + } +} + +int yt921x_dsa_get_sset_count(struct dsa_switch *ds, int port, int sset) +{ + int cnt = 0; + + if (sset != ETH_SS_STATS) + return 0; + + for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) { + const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i]; + + if (desc->name) + cnt++; + } + + return cnt; +} + +void +yt921x_dsa_get_eth_mac_stats(struct dsa_switch *ds, int port, + struct ethtool_eth_mac_stats *mac_stats) +{ + struct yt921x_priv *priv = to_yt921x_priv(ds); + struct yt921x_port *pp = &priv->ports[port]; + struct yt921x_mib *pm = pp->mib; + struct yt921x_mib_stats *mib; + + if (!pm) + return; + mib = &pm->stats; + + mutex_lock(&priv->reg_lock); + yt921x_mib_read(priv, port); + mutex_unlock(&priv->reg_lock); + + mac_stats->FramesTransmittedOK = pm->tx_frames; + mac_stats->SingleCollisionFrames = mib->tx_single_collisions; + mac_stats->MultipleCollisionFrames = mib->tx_multiple_collisions; + mac_stats->FramesReceivedOK = pm->rx_frames; + mac_stats->FrameCheckSequenceErrors = mib->rx_crc_errors; + mac_stats->AlignmentErrors = mib->rx_alignment_errors; + mac_stats->OctetsTransmittedOK = mib->tx_good_bytes; + mac_stats->FramesWithDeferredXmissions = mib->tx_deferred; + mac_stats->LateCollisions = mib->tx_late_collisions; + mac_stats->FramesAbortedDueToXSColls = mib->tx_aborted_errors; + /* mac_stats->FramesLostDueToIntMACXmitError */ + /* mac_stats->CarrierSenseErrors */ + mac_stats->OctetsReceivedOK = mib->rx_good_bytes; + /* mac_stats->FramesLostDueToIntMACRcvError */ + mac_stats->MulticastFramesXmittedOK = mib->tx_multicast; + mac_stats->BroadcastFramesXmittedOK = mib->tx_broadcast; + /* mac_stats->FramesWithExcessiveDeferral */ + mac_stats->MulticastFramesReceivedOK = mib->rx_multicast; + mac_stats->BroadcastFramesReceivedOK = mib->rx_broadcast; + /* mac_stats->InRangeLengthErrors */ + /* mac_stats->OutOfRangeLengthField */ + mac_stats->FrameTooLongErrors = mib->rx_oversize_errors; +} + +void +yt921x_dsa_get_eth_ctrl_stats(struct dsa_switch *ds, int port, + struct ethtool_eth_ctrl_stats *ctrl_stats) +{ + struct yt921x_priv *priv = to_yt921x_priv(ds); + struct yt921x_port *pp = &priv->ports[port]; + struct yt921x_mib *pm = pp->mib; + struct yt921x_mib_stats *mib; + + if (!pm) + return; + mib = &pm->stats; + + mutex_lock(&priv->reg_lock); + yt921x_mib_read(priv, port); + mutex_unlock(&priv->reg_lock); + + ctrl_stats->MACControlFramesTransmitted = mib->tx_pause; + ctrl_stats->MACControlFramesReceived = mib->rx_pause; + /* ctrl_stats->UnsupportedOpcodesReceived */ +} + +static const struct ethtool_rmon_hist_range yt921x_rmon_ranges[] = { + { 0, 64 }, + { 65, 127 }, + { 128, 255 }, + { 256, 511 }, + { 512, 1023 }, + { 1024, 1518 }, + { 1519, YT921X_FRAME_SIZE_MAX }, + {} +}; + +void +yt921x_dsa_get_rmon_stats(struct dsa_switch *ds, int port, + struct ethtool_rmon_stats *rmon_stats, + const struct ethtool_rmon_hist_range **ranges) +{ + struct yt921x_priv *priv = to_yt921x_priv(ds); + struct yt921x_port *pp = &priv->ports[port]; + struct yt921x_mib *pm = pp->mib; + struct yt921x_mib_stats *mib; + + if (!pm) + return; + mib = &pm->stats; + + mutex_lock(&priv->reg_lock); + yt921x_mib_read(priv, port); + mutex_unlock(&priv->reg_lock); + + *ranges = yt921x_rmon_ranges; + + rmon_stats->undersize_pkts = mib->rx_undersize_errors; + rmon_stats->oversize_pkts = mib->rx_oversize_errors; + rmon_stats->fragments = mib->rx_alignment_errors; + /* rmon_stats->jabbers */ + + rmon_stats->hist[0] = mib->rx_64byte; + rmon_stats->hist[1] = mib->rx_65_127byte; + rmon_stats->hist[2] = mib->rx_128_255byte; + rmon_stats->hist[3] = mib->rx_256_511byte; + rmon_stats->hist[4] = mib->rx_512_1023byte; + rmon_stats->hist[5] = mib->rx_1024_1518byte; + rmon_stats->hist[6] = mib->rx_jumbo; + + rmon_stats->hist_tx[0] = mib->tx_64byte; + rmon_stats->hist_tx[1] = mib->tx_65_127byte; + rmon_stats->hist_tx[2] = mib->tx_128_255byte; + rmon_stats->hist_tx[3] = mib->tx_256_511byte; + rmon_stats->hist_tx[4] = mib->tx_512_1023byte; + rmon_stats->hist_tx[5] = mib->tx_1024_1518byte; + rmon_stats->hist_tx[6] = mib->tx_jumbo; +} + +void +yt921x_dsa_get_stats64(struct dsa_switch *ds, int port, + struct rtnl_link_stats64 *stats) +{ + struct yt921x_priv *priv = to_yt921x_priv(ds); + struct yt921x_port *pp = &priv->ports[port]; + struct yt921x_mib *pm = pp->mib; + struct yt921x_mib_stats *mib; + + if (!pm) + return; + mib = &pm->stats; + + stats->rx_length_errors = mib->rx_undersize_errors + + mib->rx_fragment_errors; + stats->rx_over_errors = mib->rx_oversize_errors; + stats->rx_crc_errors = mib->rx_crc_errors; + stats->rx_frame_errors = mib->rx_alignment_errors; + /* stats->rx_fifo_errors */ + /* stats->rx_missed_errors */ + + stats->tx_aborted_errors = mib->tx_aborted_errors; + /* stats->tx_carrier_errors */ + stats->tx_fifo_errors = mib->tx_undersize_errors; + /* stats->tx_heartbeat_errors */ + stats->tx_window_errors = mib->tx_late_collisions; + + stats->rx_packets = pm->rx_frames; + stats->tx_packets = pm->tx_frames; + stats->rx_bytes = mib->rx_good_bytes - ETH_FCS_LEN * stats->rx_packets; + stats->tx_bytes = mib->tx_good_bytes - ETH_FCS_LEN * stats->tx_packets; + stats->rx_errors = stats->rx_length_errors + stats->rx_over_errors + + stats->rx_crc_errors + stats->rx_frame_errors; + stats->tx_errors = stats->tx_aborted_errors + stats->tx_fifo_errors + + stats->tx_window_errors; + stats->rx_dropped = mib->rx_dropped; + /* stats->tx_dropped */ + stats->multicast = mib->rx_multicast; + stats->collisions = mib->tx_collisions; +} + +void +yt921x_dsa_get_pause_stats(struct dsa_switch *ds, int port, + struct ethtool_pause_stats *pause_stats) +{ + struct yt921x_priv *priv = to_yt921x_priv(ds); + struct yt921x_port *pp = &priv->ports[port]; + struct yt921x_mib *pm = pp->mib; + struct yt921x_mib_stats *mib; + + if (!pm) + return; + mib = &pm->stats; + + mutex_lock(&priv->reg_lock); + yt921x_mib_read(priv, port); + mutex_unlock(&priv->reg_lock); + + pause_stats->tx_pause_frames = mib->tx_pause; + pause_stats->rx_pause_frames = mib->rx_pause; +} diff --git a/drivers/net/dsa/motorcomm/mib.h b/drivers/net/dsa/motorcomm/mib.h new file mode 100644 index 000000000000..cfad0665baae --- /dev/null +++ b/drivers/net/dsa/motorcomm/mib.h @@ -0,0 +1,159 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* + * Copyright (c) 2026 David Yang + */ + +#ifndef _YT_MIB_H +#define _YT_MIB_H + +#include <net/dsa.h> + +#define YT921X_MIB_CTRL 0xc0004 +#define YT921X_MIB_CTRL_CLEAN BIT(30) +#define YT921X_MIB_CTRL_PORT_M GENMASK(6, 3) +#define YT921X_MIB_CTRL_PORT(x) FIELD_PREP(YT921X_MIB_CTRL_PORT_M, (x)) +#define YT921X_MIB_CTRL_ONE_PORT BIT(1) +#define YT921X_MIB_CTRL_ALL_PORT BIT(0) +#define YT921X_MIBn_DATA0(port) (0xc0100 + 0x100 * (port)) +#define YT921X_MIBn_DATAm(port, x) (YT921X_MIBn_DATA0(port) + 4 * (x)) +#define YT921X_MIB_DATA_RX_BROADCAST 0x00 +#define YT921X_MIB_DATA_RX_PAUSE 0x04 +#define YT921X_MIB_DATA_RX_MULTICAST 0x08 +#define YT921X_MIB_DATA_RX_CRC_ERR 0x0c + +#define YT921X_MIB_DATA_RX_ALIGN_ERR 0x10 +#define YT921X_MIB_DATA_RX_UNDERSIZE_ERR 0x14 +#define YT921X_MIB_DATA_RX_FRAG_ERR 0x18 +#define YT921X_MIB_DATA_RX_PKT_SZ_64 0x1c + +#define YT921X_MIB_DATA_RX_PKT_SZ_65_TO_127 0x20 +#define YT921X_MIB_DATA_RX_PKT_SZ_128_TO_255 0x24 +#define YT921X_MIB_DATA_RX_PKT_SZ_256_TO_511 0x28 +#define YT921X_MIB_DATA_RX_PKT_SZ_512_TO_1023 0x2c + +#define YT921X_MIB_DATA_RX_PKT_SZ_1024_TO_1518 0x30 +#define YT921X_MIB_DATA_RX_PKT_SZ_1519_TO_MAX 0x34 +/* 0x38: unused */ +#define YT921X_MIB_DATA_RX_GOOD_BYTES 0x3c + +/* 0x40: 64 bytes */ +#define YT921X_MIB_DATA_RX_BAD_BYTES 0x44 +/* 0x48: 64 bytes */ +#define YT921X_MIB_DATA_RX_OVERSIZE_ERR 0x4c + +#define YT921X_MIB_DATA_RX_DROPPED 0x50 +#define YT921X_MIB_DATA_TX_BROADCAST 0x54 +#define YT921X_MIB_DATA_TX_PAUSE 0x58 +#define YT921X_MIB_DATA_TX_MULTICAST 0x5c + +#define YT921X_MIB_DATA_TX_UNDERSIZE_ERR 0x60 +#define YT921X_MIB_DATA_TX_PKT_SZ_64 0x64 +#define YT921X_MIB_DATA_TX_PKT_SZ_65_TO_127 0x68 +#define YT921X_MIB_DATA_TX_PKT_SZ_128_TO_255 0x6c + +#define YT921X_MIB_DATA_TX_PKT_SZ_256_TO_511 0x70 +#define YT921X_MIB_DATA_TX_PKT_SZ_512_TO_1023 0x74 +#define YT921X_MIB_DATA_TX_PKT_SZ_1024_TO_1518 0x78 +#define YT921X_MIB_DATA_TX_PKT_SZ_1519_TO_MAX 0x7c + +/* 0x80: unused */ +#define YT921X_MIB_DATA_TX_GOOD_BYTES 0x84 +/* 0x88: 64 bytes */ +#define YT921X_MIB_DATA_TX_COLLISION 0x8c + +#define YT921X_MIB_DATA_TX_EXCESSIVE_COLLISION 0x90 +#define YT921X_MIB_DATA_TX_MULTIPLE_COLLISION 0x94 +#define YT921X_MIB_DATA_TX_SINGLE_COLLISION 0x98 +#define YT921X_MIB_DATA_TX_PKT 0x9c + +#define YT921X_MIB_DATA_TX_DEFERRED 0xa0 +#define YT921X_MIB_DATA_TX_LATE_COLLISION 0xa4 +#define YT921X_MIB_DATA_RX_OAM 0xa8 +#define YT921X_MIB_DATA_TX_OAM 0xac + +struct yt921x_mib_stats { + u64 rx_broadcast; + u64 rx_pause; + u64 rx_multicast; + u64 rx_crc_errors; + + u64 rx_alignment_errors; + u64 rx_undersize_errors; + u64 rx_fragment_errors; + u64 rx_64byte; + + u64 rx_65_127byte; + u64 rx_128_255byte; + u64 rx_256_511byte; + u64 rx_512_1023byte; + + u64 rx_1024_1518byte; + u64 rx_jumbo; + u64 rx_good_bytes; + + u64 rx_bad_bytes; + u64 rx_oversize_errors; + + u64 rx_dropped; + u64 tx_broadcast; + u64 tx_pause; + u64 tx_multicast; + + u64 tx_undersize_errors; + u64 tx_64byte; + u64 tx_65_127byte; + u64 tx_128_255byte; + + u64 tx_256_511byte; + u64 tx_512_1023byte; + u64 tx_1024_1518byte; + u64 tx_jumbo; + + u64 tx_good_bytes; + u64 tx_collisions; + + u64 tx_aborted_errors; + u64 tx_multiple_collisions; + u64 tx_single_collisions; + u64 tx_good; + + u64 tx_deferred; + u64 tx_late_collisions; + u64 rx_oam; + u64 tx_oam; +}; + +struct yt921x_mib { + struct yt921x_port *port; + + struct delayed_work work; + struct yt921x_mib_stats stats; + u64 rx_frames; + u64 tx_frames; +}; + +void yt921x_mib_poll(struct work_struct *work); +void +yt921x_dsa_get_strings(struct dsa_switch *ds, int port, u32 stringset, + uint8_t *data); +void +yt921x_dsa_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data); +int yt921x_dsa_get_sset_count(struct dsa_switch *ds, int port, int sset); +void +yt921x_dsa_get_eth_mac_stats(struct dsa_switch *ds, int port, + struct ethtool_eth_mac_stats *mac_stats); +void +yt921x_dsa_get_eth_ctrl_stats(struct dsa_switch *ds, int port, + struct ethtool_eth_ctrl_stats *ctrl_stats); +void +yt921x_dsa_get_rmon_stats(struct dsa_switch *ds, int port, + struct ethtool_rmon_stats *rmon_stats, + const struct ethtool_rmon_hist_range **ranges); +void +yt921x_dsa_get_stats64(struct dsa_switch *ds, int port, + struct rtnl_link_stats64 *stats); +void +yt921x_dsa_get_pause_stats(struct dsa_switch *ds, int port, + struct ethtool_pause_stats *pause_stats); + +#endif -- 2.53.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v3 3/6] net: dsa: motorcomm: Split MIB module 2026-09-06 13:14 ` [PATCH net-next v3 3/6] net: dsa: motorcomm: Split MIB module David Yang @ 2026-09-09 15:16 ` netdev-bot+sashiko 0 siblings, 0 replies; 15+ messages in thread From: netdev-bot+sashiko @ 2026-09-09 15:16 UTC (permalink / raw) To: mmyangfl Cc: netdev, andrew, olteanv, davem, edumazet, kuba, pabeni, linux, linux-kernel Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 1 (2) · Low: 0 - [Medium] MIB polling work is permanently disabled by `yt921x_dsa_port_teardown()`… Pre-existing issues: - [Medium] Unsynchronized access to the cached 64-bit MIB counters: at this commit `yt921x_mib_read()` (drivers/net/dsa/motorcomm/mib.c)… - [Medium] Non-atomic read of the 64-bit MIB counter register pairs: for descriptors with `size == 2` (RX_GOOD_BYTES 0x3c/0x40, RX_BAD_BYTES… -- net: dsa: motorcomm: Split MIB module This moves the yt921x MIB descriptor table, register definitions, stats structures and all of the ethtool/stats64 callbacks out of chip.c and chip.h into new mib.c and mib.h files, in preparation for yt922x using a different MIB scheme. The change is described as pure code motion. > diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c > new file mode 100644 > index 0000000000000..b1296f224cf69 > --- /dev/null > +++ b/drivers/net/dsa/motorcomm/mib.c > @@ -0,0 +1,408 @@ [ ... ] > +/* Read and handle overflow of 32bit MIBs. MIB buffer must be zeroed before. */ > +static int yt921x_mib_read(struct yt921x_priv *priv, int port) > +{ [ ... ] > + for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) { > + const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i]; > + u32 reg = YT921X_MIBn_DATA0(port) + desc->offset; > + u64 *valp = &((u64 *)mib)[i]; > + u32 val0; > + u64 val; > + > + res = yt921x_reg_read(priv, reg, &val0); > + if (res) > + break; > + > + if (desc->size <= 1) { > + u64 old_val = *valp; > + > + val = (old_val & ~(u64)U32_MAX) | val0; > + if (val < old_val) > + val += 1ull << 32; > + } else { > + u32 val1; > + > + res = yt921x_reg_read(priv, reg + 4, &val1); > + if (res) > + break; > + val = ((u64)val1 << 32) | val0; > + } [Severity: Medium] This is a pre-existing issue carried over by the move, not something this patch introduces, but can the size == 2 path assemble a torn value? The two halves come from two independent SMI/MDIO transactions: res = yt921x_reg_read(priv, reg, &val0); ... res = yt921x_reg_read(priv, reg + 4, &val1); ... val = ((u64)val1 << 32) | val0; If the low word wraps between the two reads, the combined value is off by 2^32. Unlike the size <= 1 branch there is no wraparound correction here, so the affected counters (RX_GOOD_BYTES, RX_BAD_BYTES, TX_GOOD_BYTES) can step backwards by about 4 GiB, and stats64 rx_bytes/tx_bytes can underflow once ETH_FCS_LEN * packets is subtracted. At 1 Gbps the low word of a byte counter wraps roughly every 34 seconds, while each register access over MDIO takes tens of microseconds, so the window is reachable in practice. For traceability: a later commit in this series, "net: dsa: motorcomm: Use safe 64-bit counter reader", replaces this open-coded pair read with yt921x_counter_read() in smi.c, which reads lo, then re-reads hi/lo and accepts the value only when lo >= old_lo (bounded to 16 attempts, -ETIMEDOUT otherwise). > + > + WRITE_ONCE(*valp, val); > + } > + > + pm->rx_frames = mib->rx_64byte + mib->rx_65_127byte + > + mib->rx_128_255byte + mib->rx_256_511byte + > + mib->rx_512_1023byte + mib->rx_1024_1518byte + > + mib->rx_jumbo; > + pm->tx_frames = mib->tx_64byte + mib->tx_65_127byte + > + mib->tx_128_255byte + mib->tx_256_511byte + > + mib->tx_512_1023byte + mib->tx_1024_1518byte + > + mib->tx_jumbo; [ ... ] > +void yt921x_mib_poll(struct work_struct *work) > +{ > + struct yt921x_mib *pm = container_of_const(work, struct yt921x_mib, > + work.work); [ ... ] > + schedule_delayed_work(&pm->work, delay); > +} [Severity: Medium] Now that yt921x_mib_poll() lives here, is the work item's disable count ever balanced for a re-used struct yt921x_mib? In chip.c, yt921x_dsa_port_teardown() does: if (pm) disable_delayed_work_sync(&pm->work); which is __cancel_work_sync(&dwork->work, WORK_CANCEL_DELAYED | WORK_CANCEL_DISABLE), so the disable count stays in work->data until a matching enable_delayed_work(). On a later setup, yt921x_dsa_port_setup() only re-initialises the work when the allocation is fresh: if (!pm && (BIT(port) & (priv->info->internal_mask | priv->info->external_mask))) { pm = devm_kzalloc(dev, sizeof(*pm), GFP_KERNEL); ... INIT_DELAYED_WORK(&pm->work, yt921x_mib_poll); } pp->mib is a devm allocation that survives a DSA tree teardown/setup cycle, so pm is non-NULL the second time and neither INIT_DELAYED_WORK() nor enable_delayed_work() runs. The teardown callback is reached on ordinary port teardown, not just device removal: dsa_port_devlink_teardown() calls ds->ops->port_teardown, and dsa_switch_remove() tears down the whole tree, so a surviving switch's ports get torn down while its pp->mib stays alive. A later dsa_tree_setup() then calls port_setup again on that switch. After that, the schedule_delayed_work(&pm->work, 0) in yt921x_phylink_mac_link_up() is silently dropped by clear_pending_if_disabled(), so MIB polling never resumes for the port and the 32-bit wraparound accumulation in yt921x_mib_read() stops happening. Would cancel_delayed_work_sync() in port_teardown, or a matching enable_delayed_work() in yt921x_dsa_port_setup(), be more appropriate for a re-usable object? [ ... ] > +void > +yt921x_dsa_get_stats64(struct dsa_switch *ds, int port, > + struct rtnl_link_stats64 *stats) > +{ > + struct yt921x_priv *priv = to_yt921x_priv(ds); > + struct yt921x_port *pp = &priv->ports[port]; > + struct yt921x_mib *pm = pp->mib; > + struct yt921x_mib_stats *mib; > + > + if (!pm) > + return; > + mib = &pm->stats; [ ... ] > + stats->rx_packets = pm->rx_frames; > + stats->tx_packets = pm->tx_frames; > + stats->rx_bytes = mib->rx_good_bytes - ETH_FCS_LEN * stats->rx_packets; > + stats->tx_bytes = mib->tx_good_bytes - ETH_FCS_LEN * stats->tx_packets; [Severity: Medium] This isn't a bug introduced by this patch, but how are these cached 64-bit counters synchronized against the poll worker? yt921x_mib_read() writes each counter with WRITE_ONCE() on a plain u64 and stores pm->rx_frames / pm->tx_frames with plain assignments while holding priv->reg_lock. yt921x_dsa_get_stats64() reads all of them with no lock at all, and the other callbacks release the lock before copying out, e.g.: mutex_lock(&priv->reg_lock); yt921x_mib_read(priv, port); mutex_unlock(&priv->reg_lock); mac_stats->FramesTransmittedOK = pm->tx_frames; and in yt921x_dsa_get_pause_stats(): mutex_unlock(&priv->reg_lock); pause_stats->tx_pause_frames = mib->tx_pause; On 32-bit builds a u64 store is two stores, so a reader can observe mismatched halves, and derived values can mix fields from different snapshots. Can rx_bytes underflow when rx_good_bytes comes from an older snapshot than rx_frames? For traceability: later commits in this series, "net: dsa: motorcomm: Use u64_stats_t for MIB stats" and "net: dsa: motorcomm: Fix MIB synchronization", convert the counters to u64_stats_t, wrap the write-back in u64_stats_update_begin()/u64_stats_update_end(), add a u64_stats_fetch_begin()/u64_stats_fetch_retry() loop in yt921x_dsa_get_stats64(), and move the remaining field copies inside the reg_lock section. > diff --git a/drivers/net/dsa/motorcomm/mib.h b/drivers/net/dsa/motorcomm/mib.h > new file mode 100644 > index 0000000000000..cfad0665baaea > --- /dev/null > +++ b/drivers/net/dsa/motorcomm/mib.h > @@ -0,0 +1,159 @@ [ ... ] > +struct yt921x_mib_stats { > + u64 rx_broadcast; > + u64 rx_pause; [ ... ] > +struct yt921x_mib { > + struct yt921x_port *port; > + > + struct delayed_work work; > + struct yt921x_mib_stats stats; > + u64 rx_frames; > + u64 tx_frames; > +}; [ ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906131414.2683362-1-mmyangfl%40gmail.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net-next v3 4/6] net: dsa: motorcomm: Use u64_stats_t for MIB stats 2026-09-06 13:14 [PATCH net-next v3 0/6] net: dsa: motorcomm: MIB fixup David Yang ` (2 preceding siblings ...) 2026-09-06 13:14 ` [PATCH net-next v3 3/6] net: dsa: motorcomm: Split MIB module David Yang @ 2026-09-06 13:14 ` David Yang 2026-09-09 15:16 ` netdev-bot+sashiko 2026-09-06 13:14 ` [PATCH net-next v3 5/6] net: dsa: motorcomm: Fix MIB synchronization David Yang 2026-09-06 13:14 ` [PATCH net-next v3 6/6] net: dsa: motorcomm: Use safe 64-bit counter reader David Yang 5 siblings, 1 reply; 15+ messages in thread From: David Yang @ 2026-09-06 13:14 UTC (permalink / raw) To: netdev Cc: David Yang, Andrew Lunn, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Russell King, linux-kernel 64-bit variables might not be atomic on 32-bit architectures, and could lead to load/store tearing. This is a verbatim change that replace every u64 statistics into u64_stats_t. u64_stats_sync will be equipped in the next commit. Signed-off-by: David Yang <mmyangfl@gmail.com> --- drivers/net/dsa/motorcomm/mib.c | 152 ++++++++++++++++---------------- drivers/net/dsa/motorcomm/mib.h | 104 +++++++++++----------- 2 files changed, 131 insertions(+), 125 deletions(-) diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c index b1296f224cf6..f5d39d22215d 100644 --- a/drivers/net/dsa/motorcomm/mib.c +++ b/drivers/net/dsa/motorcomm/mib.c @@ -92,22 +92,16 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) struct device *dev = to_device(priv); struct yt921x_mib *pm = pp->mib; struct yt921x_mib_stats *mib; + u64 rx_frames; + u64 tx_frames; int res = 0; mib = &pm->stats; - /* Reading of yt921x_port::mib is not protected by a lock and it's vain - * to keep its consistency, since we have to read registers one by one - * and there is no way to make a snapshot of MIB stats. - * - * Writing (by this function only) is and should be protected by - * reg_lock. - */ - for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) { const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i]; u32 reg = YT921X_MIBn_DATA0(port) + desc->offset; - u64 *valp = &((u64 *)mib)[i]; + u64_stats_t *valp = &((u64_stats_t *)mib)[i]; u32 val0; u64 val; @@ -116,7 +110,7 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) break; if (desc->size <= 1) { - u64 old_val = *valp; + u64 old_val = u64_stats_read(valp); val = (old_val & ~(u64)U32_MAX) | val0; if (val < old_val) @@ -130,17 +124,25 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) val = ((u64)val1 << 32) | val0; } - WRITE_ONCE(*valp, val); + u64_stats_set(valp, val); } - pm->rx_frames = mib->rx_64byte + mib->rx_65_127byte + - mib->rx_128_255byte + mib->rx_256_511byte + - mib->rx_512_1023byte + mib->rx_1024_1518byte + - mib->rx_jumbo; - pm->tx_frames = mib->tx_64byte + mib->tx_65_127byte + - mib->tx_128_255byte + mib->tx_256_511byte + - mib->tx_512_1023byte + mib->tx_1024_1518byte + - mib->tx_jumbo; + rx_frames = u64_stats_read(&mib->rx_64byte) + + u64_stats_read(&mib->rx_65_127byte) + + u64_stats_read(&mib->rx_128_255byte) + + u64_stats_read(&mib->rx_256_511byte) + + u64_stats_read(&mib->rx_512_1023byte) + + u64_stats_read(&mib->rx_1024_1518byte) + + u64_stats_read(&mib->rx_jumbo); + tx_frames = u64_stats_read(&mib->tx_64byte) + + u64_stats_read(&mib->tx_65_127byte) + + u64_stats_read(&mib->tx_128_255byte) + + u64_stats_read(&mib->tx_256_511byte) + + u64_stats_read(&mib->tx_512_1023byte) + + u64_stats_read(&mib->tx_1024_1518byte) + + u64_stats_read(&mib->tx_jumbo); + u64_stats_set(&pm->rx_frames, rx_frames); + u64_stats_set(&pm->tx_frames, tx_frames); if (res) dev_err(dev, "Failed to %s port %d: %i\n", "read stats for", @@ -207,7 +209,7 @@ yt921x_dsa_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data) if (!desc->name) continue; - data[j] = ((u64 *)mib)[i]; + data[j] = u64_stats_read(&((u64_stats_t *)mib)[i]); j++; } } @@ -246,28 +248,28 @@ yt921x_dsa_get_eth_mac_stats(struct dsa_switch *ds, int port, yt921x_mib_read(priv, port); mutex_unlock(&priv->reg_lock); - mac_stats->FramesTransmittedOK = pm->tx_frames; - mac_stats->SingleCollisionFrames = mib->tx_single_collisions; - mac_stats->MultipleCollisionFrames = mib->tx_multiple_collisions; - mac_stats->FramesReceivedOK = pm->rx_frames; - mac_stats->FrameCheckSequenceErrors = mib->rx_crc_errors; - mac_stats->AlignmentErrors = mib->rx_alignment_errors; - mac_stats->OctetsTransmittedOK = mib->tx_good_bytes; - mac_stats->FramesWithDeferredXmissions = mib->tx_deferred; - mac_stats->LateCollisions = mib->tx_late_collisions; - mac_stats->FramesAbortedDueToXSColls = mib->tx_aborted_errors; + mac_stats->FramesTransmittedOK = u64_stats_read(&pm->tx_frames); + mac_stats->SingleCollisionFrames = u64_stats_read(&mib->tx_single_collisions); + mac_stats->MultipleCollisionFrames = u64_stats_read(&mib->tx_multiple_collisions); + mac_stats->FramesReceivedOK = u64_stats_read(&pm->rx_frames); + mac_stats->FrameCheckSequenceErrors = u64_stats_read(&mib->rx_crc_errors); + mac_stats->AlignmentErrors = u64_stats_read(&mib->rx_alignment_errors); + mac_stats->OctetsTransmittedOK = u64_stats_read(&mib->tx_good_bytes); + mac_stats->FramesWithDeferredXmissions = u64_stats_read(&mib->tx_deferred); + mac_stats->LateCollisions = u64_stats_read(&mib->tx_late_collisions); + mac_stats->FramesAbortedDueToXSColls = u64_stats_read(&mib->tx_aborted_errors); /* mac_stats->FramesLostDueToIntMACXmitError */ /* mac_stats->CarrierSenseErrors */ - mac_stats->OctetsReceivedOK = mib->rx_good_bytes; + mac_stats->OctetsReceivedOK = u64_stats_read(&mib->rx_good_bytes); /* mac_stats->FramesLostDueToIntMACRcvError */ - mac_stats->MulticastFramesXmittedOK = mib->tx_multicast; - mac_stats->BroadcastFramesXmittedOK = mib->tx_broadcast; + mac_stats->MulticastFramesXmittedOK = u64_stats_read(&mib->tx_multicast); + mac_stats->BroadcastFramesXmittedOK = u64_stats_read(&mib->tx_broadcast); /* mac_stats->FramesWithExcessiveDeferral */ - mac_stats->MulticastFramesReceivedOK = mib->rx_multicast; - mac_stats->BroadcastFramesReceivedOK = mib->rx_broadcast; + mac_stats->MulticastFramesReceivedOK = u64_stats_read(&mib->rx_multicast); + mac_stats->BroadcastFramesReceivedOK = u64_stats_read(&mib->rx_broadcast); /* mac_stats->InRangeLengthErrors */ /* mac_stats->OutOfRangeLengthField */ - mac_stats->FrameTooLongErrors = mib->rx_oversize_errors; + mac_stats->FrameTooLongErrors = u64_stats_read(&mib->rx_oversize_errors); } void @@ -287,8 +289,8 @@ yt921x_dsa_get_eth_ctrl_stats(struct dsa_switch *ds, int port, yt921x_mib_read(priv, port); mutex_unlock(&priv->reg_lock); - ctrl_stats->MACControlFramesTransmitted = mib->tx_pause; - ctrl_stats->MACControlFramesReceived = mib->rx_pause; + ctrl_stats->MACControlFramesTransmitted = u64_stats_read(&mib->tx_pause); + ctrl_stats->MACControlFramesReceived = u64_stats_read(&mib->rx_pause); /* ctrl_stats->UnsupportedOpcodesReceived */ } @@ -323,26 +325,26 @@ yt921x_dsa_get_rmon_stats(struct dsa_switch *ds, int port, *ranges = yt921x_rmon_ranges; - rmon_stats->undersize_pkts = mib->rx_undersize_errors; - rmon_stats->oversize_pkts = mib->rx_oversize_errors; - rmon_stats->fragments = mib->rx_alignment_errors; + rmon_stats->undersize_pkts = u64_stats_read(&mib->rx_undersize_errors); + rmon_stats->oversize_pkts = u64_stats_read(&mib->rx_oversize_errors); + rmon_stats->fragments = u64_stats_read(&mib->rx_alignment_errors); /* rmon_stats->jabbers */ - rmon_stats->hist[0] = mib->rx_64byte; - rmon_stats->hist[1] = mib->rx_65_127byte; - rmon_stats->hist[2] = mib->rx_128_255byte; - rmon_stats->hist[3] = mib->rx_256_511byte; - rmon_stats->hist[4] = mib->rx_512_1023byte; - rmon_stats->hist[5] = mib->rx_1024_1518byte; - rmon_stats->hist[6] = mib->rx_jumbo; - - rmon_stats->hist_tx[0] = mib->tx_64byte; - rmon_stats->hist_tx[1] = mib->tx_65_127byte; - rmon_stats->hist_tx[2] = mib->tx_128_255byte; - rmon_stats->hist_tx[3] = mib->tx_256_511byte; - rmon_stats->hist_tx[4] = mib->tx_512_1023byte; - rmon_stats->hist_tx[5] = mib->tx_1024_1518byte; - rmon_stats->hist_tx[6] = mib->tx_jumbo; + rmon_stats->hist[0] = u64_stats_read(&mib->rx_64byte); + rmon_stats->hist[1] = u64_stats_read(&mib->rx_65_127byte); + rmon_stats->hist[2] = u64_stats_read(&mib->rx_128_255byte); + rmon_stats->hist[3] = u64_stats_read(&mib->rx_256_511byte); + rmon_stats->hist[4] = u64_stats_read(&mib->rx_512_1023byte); + rmon_stats->hist[5] = u64_stats_read(&mib->rx_1024_1518byte); + rmon_stats->hist[6] = u64_stats_read(&mib->rx_jumbo); + + rmon_stats->hist_tx[0] = u64_stats_read(&mib->tx_64byte); + rmon_stats->hist_tx[1] = u64_stats_read(&mib->tx_65_127byte); + rmon_stats->hist_tx[2] = u64_stats_read(&mib->tx_128_255byte); + rmon_stats->hist_tx[3] = u64_stats_read(&mib->tx_256_511byte); + rmon_stats->hist_tx[4] = u64_stats_read(&mib->tx_512_1023byte); + rmon_stats->hist_tx[5] = u64_stats_read(&mib->tx_1024_1518byte); + rmon_stats->hist_tx[6] = u64_stats_read(&mib->tx_jumbo); } void @@ -358,32 +360,34 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port, return; mib = &pm->stats; - stats->rx_length_errors = mib->rx_undersize_errors + - mib->rx_fragment_errors; - stats->rx_over_errors = mib->rx_oversize_errors; - stats->rx_crc_errors = mib->rx_crc_errors; - stats->rx_frame_errors = mib->rx_alignment_errors; + stats->rx_length_errors = u64_stats_read(&mib->rx_undersize_errors) + + u64_stats_read(&mib->rx_fragment_errors); + stats->rx_over_errors = u64_stats_read(&mib->rx_oversize_errors); + stats->rx_crc_errors = u64_stats_read(&mib->rx_crc_errors); + stats->rx_frame_errors = u64_stats_read(&mib->rx_alignment_errors); /* stats->rx_fifo_errors */ /* stats->rx_missed_errors */ - stats->tx_aborted_errors = mib->tx_aborted_errors; + stats->tx_aborted_errors = u64_stats_read(&mib->tx_aborted_errors); /* stats->tx_carrier_errors */ - stats->tx_fifo_errors = mib->tx_undersize_errors; + stats->tx_fifo_errors = u64_stats_read(&mib->tx_undersize_errors); /* stats->tx_heartbeat_errors */ - stats->tx_window_errors = mib->tx_late_collisions; - - stats->rx_packets = pm->rx_frames; - stats->tx_packets = pm->tx_frames; - stats->rx_bytes = mib->rx_good_bytes - ETH_FCS_LEN * stats->rx_packets; - stats->tx_bytes = mib->tx_good_bytes - ETH_FCS_LEN * stats->tx_packets; + stats->tx_window_errors = u64_stats_read(&mib->tx_late_collisions); + + stats->rx_packets = u64_stats_read(&pm->rx_frames); + stats->tx_packets = u64_stats_read(&pm->tx_frames); + stats->rx_bytes = u64_stats_read(&mib->rx_good_bytes) - + ETH_FCS_LEN * stats->rx_packets; + stats->tx_bytes = u64_stats_read(&mib->tx_good_bytes) - + ETH_FCS_LEN * stats->tx_packets; stats->rx_errors = stats->rx_length_errors + stats->rx_over_errors + stats->rx_crc_errors + stats->rx_frame_errors; stats->tx_errors = stats->tx_aborted_errors + stats->tx_fifo_errors + stats->tx_window_errors; - stats->rx_dropped = mib->rx_dropped; + stats->rx_dropped = u64_stats_read(&mib->rx_dropped); /* stats->tx_dropped */ - stats->multicast = mib->rx_multicast; - stats->collisions = mib->tx_collisions; + stats->multicast = u64_stats_read(&mib->rx_multicast); + stats->collisions = u64_stats_read(&mib->tx_collisions); } void @@ -403,6 +407,6 @@ yt921x_dsa_get_pause_stats(struct dsa_switch *ds, int port, yt921x_mib_read(priv, port); mutex_unlock(&priv->reg_lock); - pause_stats->tx_pause_frames = mib->tx_pause; - pause_stats->rx_pause_frames = mib->rx_pause; + pause_stats->tx_pause_frames = u64_stats_read(&mib->tx_pause); + pause_stats->rx_pause_frames = u64_stats_read(&mib->rx_pause); } diff --git a/drivers/net/dsa/motorcomm/mib.h b/drivers/net/dsa/motorcomm/mib.h index cfad0665baae..d84ae2f449ac 100644 --- a/drivers/net/dsa/motorcomm/mib.h +++ b/drivers/net/dsa/motorcomm/mib.h @@ -6,6 +6,8 @@ #ifndef _YT_MIB_H #define _YT_MIB_H +#include <linux/u64_stats_sync.h> + #include <net/dsa.h> #define YT921X_MIB_CTRL 0xc0004 @@ -72,55 +74,55 @@ #define YT921X_MIB_DATA_TX_OAM 0xac struct yt921x_mib_stats { - u64 rx_broadcast; - u64 rx_pause; - u64 rx_multicast; - u64 rx_crc_errors; - - u64 rx_alignment_errors; - u64 rx_undersize_errors; - u64 rx_fragment_errors; - u64 rx_64byte; - - u64 rx_65_127byte; - u64 rx_128_255byte; - u64 rx_256_511byte; - u64 rx_512_1023byte; - - u64 rx_1024_1518byte; - u64 rx_jumbo; - u64 rx_good_bytes; - - u64 rx_bad_bytes; - u64 rx_oversize_errors; - - u64 rx_dropped; - u64 tx_broadcast; - u64 tx_pause; - u64 tx_multicast; - - u64 tx_undersize_errors; - u64 tx_64byte; - u64 tx_65_127byte; - u64 tx_128_255byte; - - u64 tx_256_511byte; - u64 tx_512_1023byte; - u64 tx_1024_1518byte; - u64 tx_jumbo; - - u64 tx_good_bytes; - u64 tx_collisions; - - u64 tx_aborted_errors; - u64 tx_multiple_collisions; - u64 tx_single_collisions; - u64 tx_good; - - u64 tx_deferred; - u64 tx_late_collisions; - u64 rx_oam; - u64 tx_oam; + u64_stats_t rx_broadcast; + u64_stats_t rx_pause; + u64_stats_t rx_multicast; + u64_stats_t rx_crc_errors; + + u64_stats_t rx_alignment_errors; + u64_stats_t rx_undersize_errors; + u64_stats_t rx_fragment_errors; + u64_stats_t rx_64byte; + + u64_stats_t rx_65_127byte; + u64_stats_t rx_128_255byte; + u64_stats_t rx_256_511byte; + u64_stats_t rx_512_1023byte; + + u64_stats_t rx_1024_1518byte; + u64_stats_t rx_jumbo; + u64_stats_t rx_good_bytes; + + u64_stats_t rx_bad_bytes; + u64_stats_t rx_oversize_errors; + + u64_stats_t rx_dropped; + u64_stats_t tx_broadcast; + u64_stats_t tx_pause; + u64_stats_t tx_multicast; + + u64_stats_t tx_undersize_errors; + u64_stats_t tx_64byte; + u64_stats_t tx_65_127byte; + u64_stats_t tx_128_255byte; + + u64_stats_t tx_256_511byte; + u64_stats_t tx_512_1023byte; + u64_stats_t tx_1024_1518byte; + u64_stats_t tx_jumbo; + + u64_stats_t tx_good_bytes; + u64_stats_t tx_collisions; + + u64_stats_t tx_aborted_errors; + u64_stats_t tx_multiple_collisions; + u64_stats_t tx_single_collisions; + u64_stats_t tx_good; + + u64_stats_t tx_deferred; + u64_stats_t tx_late_collisions; + u64_stats_t rx_oam; + u64_stats_t tx_oam; }; struct yt921x_mib { @@ -128,8 +130,8 @@ struct yt921x_mib { struct delayed_work work; struct yt921x_mib_stats stats; - u64 rx_frames; - u64 tx_frames; + u64_stats_t rx_frames; + u64_stats_t tx_frames; }; void yt921x_mib_poll(struct work_struct *work); -- 2.53.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v3 4/6] net: dsa: motorcomm: Use u64_stats_t for MIB stats 2026-09-06 13:14 ` [PATCH net-next v3 4/6] net: dsa: motorcomm: Use u64_stats_t for MIB stats David Yang @ 2026-09-09 15:16 ` netdev-bot+sashiko 0 siblings, 0 replies; 15+ messages in thread From: netdev-bot+sashiko @ 2026-09-09 15:16 UTC (permalink / raw) To: mmyangfl Cc: netdev, andrew, olteanv, davem, edumazet, kuba, pabeni, linux, linux-kernel Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 0 (1) · Low: 1 (1) - [Low] In "net: dsa: motorcomm: Use u64_stats_t for MIB stats" the MIB counters are converted from u64 to u64_stats_t while no `struct… Pre-existing issues: - [Medium] For the 64-bit MIB counters (desc->size == 2: RX_GOOD_BYTES, RX_BAD_BYTES, TX_GOOD_BYTES) yt921x_mib_read() performs two… - [Low] yt921x_dsa_get_stats64() (drivers/net/dsa/motorcomm/mib.c) reads the shared MIB counters with no lock and, at the reviewed commit, no… -- net: dsa: motorcomm: Use u64_stats_t for MIB stats This converts every MIB counter in struct yt921x_mib_stats and the derived rx_frames/tx_frames fields from plain u64 to u64_stats_t, and updates all readers in mib.c to use u64_stats_read(). The changelog notes that u64_stats_sync is added in the next commit. > diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c > index b1296f224cf69..f5d39d22215d4 100644 > --- a/drivers/net/dsa/motorcomm/mib.c > +++ b/drivers/net/dsa/motorcomm/mib.c > @@ -92,22 +92,16 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) > struct device *dev = to_device(priv); > struct yt921x_mib *pm = pp->mib; > struct yt921x_mib_stats *mib; > + u64 rx_frames; > + u64 tx_frames; > int res = 0; > > mib = &pm->stats; > > - /* Reading of yt921x_port::mib is not protected by a lock and it's vain > - * to keep its consistency, since we have to read registers one by one > - * and there is no way to make a snapshot of MIB stats. > - * > - * Writing (by this function only) is and should be protected by > - * reg_lock. > - */ > - > for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) { > const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i]; > u32 reg = YT921X_MIBn_DATA0(port) + desc->offset; > - u64 *valp = &((u64 *)mib)[i]; > + u64_stats_t *valp = &((u64_stats_t *)mib)[i]; > u32 val0; > u64 val; > > @@ -116,7 +110,7 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) > break; > > if (desc->size <= 1) { > - u64 old_val = *valp; > + u64 old_val = u64_stats_read(valp); > > val = (old_val & ~(u64)U32_MAX) | val0; > if (val < old_val) [Severity: Low] The changelog says the change addresses the case where "64-bit variables might not be atomic on 32-bit architectures, and could lead to load/store tearing". Does this commit on its own change anything on 32-bit? With BITS_PER_LONG == 32, u64_stats_read() and u64_stats_set() are plain accesses: static inline u64 u64_stats_read(const u64_stats_t *p) { return p->v; } static inline void u64_stats_set(u64_stats_t *p, u64 val) { p->v = val; } struct yt921x_mib gains no struct u64_stats_sync here, yt921x_mib_read() has no u64_stats_update_begin()/u64_stats_update_end() region, and yt921x_dsa_get_stats64() has no u64_stats_fetch_begin()/ u64_stats_fetch_retry() loop, so the tearing named in the changelog is still there after this patch. Also, the writer store goes from WRITE_ONCE(*valp, val) to u64_stats_set(valp, val), which on 32-bit is a plain store, and the comment documenting the "written only here under reg_lock, read locklessly" rule is deleted with nothing replacing it. Would it be preferable to fold this patch together with the follow-up, or at least keep the WRITE_ONCE() equivalent and the locking comment until the follow-up lands? For the record, the next patch in this series, "net: dsa: motorcomm: Fix MIB synchronization", does add struct u64_stats_sync syncp to struct yt921x_mib, wraps the write-back in u64_stats_update_begin()/u64_stats_update_end(), turns yt921x_dsa_get_stats64() into a fetch_begin()/fetch_retry() loop, moves mutex_unlock(&priv->reg_lock) to after the counter reads in yt921x_dsa_get_eth_mac_stats(), yt921x_dsa_get_eth_ctrl_stats(), yt921x_dsa_get_rmon_stats(), yt921x_dsa_get_pause_stats() and yt921x_dsa_get_ethtool_stats(), and restores the access documentation as /* protected by syncp OR priv->reg_lock */ in mib.h. > @@ -130,17 +124,25 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) > val = ((u64)val1 << 32) | val0; ^^^^ [Severity: Medium] This is a pre-existing issue and not introduced by this patch, but while yt921x_mib_read() is being touched: for the desc->size == 2 counters (RX_GOOD_BYTES, RX_BAD_BYTES, TX_GOOD_BYTES) the low word at YT921X_MIBn_DATA0(port) + desc->offset and the high word at + 4 are fetched in two separate SMI transactions with nothing latching the pair. If the low word wraps between the two reads, does the composed value end up about 4 GiB too high, and does that then get cached as the base for the 32-bit wraparound arithmetic above and exported as a non-monotonic rx_bytes/ tx_bytes? The last patch of this series, "net: dsa: motorcomm: Use safe 64-bit counter reader", replaces this with yt921x_counter_read(), which re-reads hi and lo and accepts the pair only when lo >= old_lo, so the window is closed there. > } > > - WRITE_ONCE(*valp, val); > + u64_stats_set(valp, val); > } > > - pm->rx_frames = mib->rx_64byte + mib->rx_65_127byte + > - mib->rx_128_255byte + mib->rx_256_511byte + > - mib->rx_512_1023byte + mib->rx_1024_1518byte + > - mib->rx_jumbo; > - pm->tx_frames = mib->tx_64byte + mib->tx_65_127byte + > - mib->tx_128_255byte + mib->tx_256_511byte + > - mib->tx_512_1023byte + mib->tx_1024_1518byte + > - mib->tx_jumbo; > + rx_frames = u64_stats_read(&mib->rx_64byte) + > + u64_stats_read(&mib->rx_65_127byte) + > + u64_stats_read(&mib->rx_128_255byte) + > + u64_stats_read(&mib->rx_256_511byte) + > + u64_stats_read(&mib->rx_512_1023byte) + > + u64_stats_read(&mib->rx_1024_1518byte) + > + u64_stats_read(&mib->rx_jumbo); > + tx_frames = u64_stats_read(&mib->tx_64byte) + > + u64_stats_read(&mib->tx_65_127byte) + > + u64_stats_read(&mib->tx_128_255byte) + > + u64_stats_read(&mib->tx_256_511byte) + > + u64_stats_read(&mib->tx_512_1023byte) + > + u64_stats_read(&mib->tx_1024_1518byte) + > + u64_stats_read(&mib->tx_jumbo); > + u64_stats_set(&pm->rx_frames, rx_frames); > + u64_stats_set(&pm->tx_frames, tx_frames); > > if (res) > dev_err(dev, "Failed to %s port %d: %i\n", "read stats for", [ ... ] > @@ -246,28 +248,28 @@ yt921x_dsa_get_eth_mac_stats(struct dsa_switch *ds, int port, > yt921x_mib_read(priv, port); > mutex_unlock(&priv->reg_lock); > > - mac_stats->FramesTransmittedOK = pm->tx_frames; > + mac_stats->FramesTransmittedOK = u64_stats_read(&pm->tx_frames); [ ... ] > @@ -358,32 +360,34 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port, > return; > mib = &pm->stats; > [ ... ] > + stats->rx_packets = u64_stats_read(&pm->rx_frames); > + stats->tx_packets = u64_stats_read(&pm->tx_frames); > + stats->rx_bytes = u64_stats_read(&mib->rx_good_bytes) - > + ETH_FCS_LEN * stats->rx_packets; > + stats->tx_bytes = u64_stats_read(&mib->tx_good_bytes) - > + ETH_FCS_LEN * stats->tx_packets; [Severity: Low] This is a pre-existing issue rather than something this patch introduces, but the derivation here mixes two independently loaded counters while yt921x_dsa_get_stats64() holds no lock: CPU0 yt921x_dsa_get_stats64() stats->rx_packets = u64_stats_read(&pm->rx_frames); /* CPU1: yt921x_mib_poll() -> yt921x_mib_read() rewrites * all counters under priv->reg_lock */ stats->rx_bytes = u64_stats_read(&mib->rx_good_bytes) - ETH_FCS_LEN * stats->rx_packets; If rx_packets is observed fresh and rx_good_bytes stale, can this unsigned subtraction wrap and report an rx_bytes value close to 2^64 through rtnl_link_stats64? The follow-up patch "net: dsa: motorcomm: Fix MIB synchronization" wraps this whole block in a u64_stats_fetch_begin()/u64_stats_fetch_retry() loop and publishes all counters plus rx_frames/tx_frames in one writer section, which gives a consistent snapshot on 32-bit. > diff --git a/drivers/net/dsa/motorcomm/mib.h b/drivers/net/dsa/motorcomm/mib.h > index cfad0665baaea..d84ae2f449ac8 100644 > --- a/drivers/net/dsa/motorcomm/mib.h > +++ b/drivers/net/dsa/motorcomm/mib.h [ ... ] > @@ -128,8 +130,8 @@ struct yt921x_mib { > > struct delayed_work work; > struct yt921x_mib_stats stats; > - u64 rx_frames; > - u64 tx_frames; > + u64_stats_t rx_frames; > + u64_stats_t tx_frames; > }; > > void yt921x_mib_poll(struct work_struct *work); Would it make sense to add struct u64_stats_sync syncp to struct yt921x_mib in this same patch, so the u64_stats_t fields are never present without their companion seqcount? -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906131414.2683362-1-mmyangfl%40gmail.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net-next v3 5/6] net: dsa: motorcomm: Fix MIB synchronization 2026-09-06 13:14 [PATCH net-next v3 0/6] net: dsa: motorcomm: MIB fixup David Yang ` (3 preceding siblings ...) 2026-09-06 13:14 ` [PATCH net-next v3 4/6] net: dsa: motorcomm: Use u64_stats_t for MIB stats David Yang @ 2026-09-06 13:14 ` David Yang 2026-09-09 15:16 ` netdev-bot+sashiko 2026-09-06 13:14 ` [PATCH net-next v3 6/6] net: dsa: motorcomm: Use safe 64-bit counter reader David Yang 5 siblings, 1 reply; 15+ messages in thread From: David Yang @ 2026-09-06 13:14 UTC (permalink / raw) To: netdev Cc: David Yang, Andrew Lunn, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Russell King, linux-kernel The previous MIB sync was based on the false assumption that 64-bit variables are always atomic. To fix that: * .get_stats64() uses u64_stats_sync, together with u64_stats_t; * others take the free mutex lock if possible. To minimize time of the write side of u64_stats_sync, a per-port buffer is used so that all the stat can be written back altogether after IO operations. Signed-off-by: David Yang <mmyangfl@gmail.com> --- drivers/net/dsa/motorcomm/chip.c | 2 + drivers/net/dsa/motorcomm/mib.c | 90 +++++++++++++++++++------------- drivers/net/dsa/motorcomm/mib.h | 6 +++ 3 files changed, 63 insertions(+), 35 deletions(-) diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c index e9730b9f8c62..392bf44d409e 100644 --- a/drivers/net/dsa/motorcomm/chip.c +++ b/drivers/net/dsa/motorcomm/chip.c @@ -19,6 +19,7 @@ #include <linux/of_mdio.h> #include <linux/of_net.h> #include <linux/sort.h> +#include <linux/u64_stats_sync.h> #include <net/dsa.h> #include <net/dscp.h> @@ -3778,6 +3779,7 @@ static int yt921x_dsa_port_setup(struct dsa_switch *ds, int port) pp->mib = pm; pm->port = pp; + u64_stats_init(&pm->syncp); INIT_DELAYED_WORK(&pm->work, yt921x_mib_poll); } diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c index f5d39d22215d..8601d25fe988 100644 --- a/drivers/net/dsa/motorcomm/mib.c +++ b/drivers/net/dsa/motorcomm/mib.c @@ -92,6 +92,7 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) struct device *dev = to_device(priv); struct yt921x_mib *pm = pp->mib; struct yt921x_mib_stats *mib; + u64 *buf = pm->data; u64 rx_frames; u64 tx_frames; int res = 0; @@ -101,7 +102,6 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) { const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i]; u32 reg = YT921X_MIBn_DATA0(port) + desc->offset; - u64_stats_t *valp = &((u64_stats_t *)mib)[i]; u32 val0; u64 val; @@ -110,7 +110,7 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) break; if (desc->size <= 1) { - u64 old_val = u64_stats_read(valp); + u64 old_val = buf[i]; val = (old_val & ~(u64)U32_MAX) | val0; if (val < old_val) @@ -124,9 +124,12 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) val = ((u64)val1 << 32) | val0; } - u64_stats_set(valp, val); + buf[i] = val; } + u64_stats_update_begin(&pm->syncp); + for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) + u64_stats_set(&((u64_stats_t *)mib)[i], buf[i]); rx_frames = u64_stats_read(&mib->rx_64byte) + u64_stats_read(&mib->rx_65_127byte) + u64_stats_read(&mib->rx_128_255byte) + @@ -143,6 +146,7 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) u64_stats_read(&mib->tx_jumbo); u64_stats_set(&pm->rx_frames, rx_frames); u64_stats_set(&pm->tx_frames, tx_frames); + u64_stats_update_end(&pm->syncp); if (res) dev_err(dev, "Failed to %s port %d: %i\n", "read stats for", @@ -191,16 +195,16 @@ yt921x_dsa_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data) struct yt921x_priv *priv = to_yt921x_priv(ds); struct yt921x_port *pp = &priv->ports[port]; struct yt921x_mib *pm = pp->mib; - struct yt921x_mib_stats *mib; + u64 *buf; size_t j; if (!pm) return; - mib = &pm->stats; + buf = pm->data; mutex_lock(&priv->reg_lock); + yt921x_mib_read(priv, port); - mutex_unlock(&priv->reg_lock); j = 0; for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) { @@ -209,9 +213,11 @@ yt921x_dsa_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data) if (!desc->name) continue; - data[j] = u64_stats_read(&((u64_stats_t *)mib)[i]); + data[j] = buf[i]; j++; } + + mutex_unlock(&priv->reg_lock); } int yt921x_dsa_get_sset_count(struct dsa_switch *ds, int port, int sset) @@ -245,8 +251,8 @@ yt921x_dsa_get_eth_mac_stats(struct dsa_switch *ds, int port, mib = &pm->stats; mutex_lock(&priv->reg_lock); + yt921x_mib_read(priv, port); - mutex_unlock(&priv->reg_lock); mac_stats->FramesTransmittedOK = u64_stats_read(&pm->tx_frames); mac_stats->SingleCollisionFrames = u64_stats_read(&mib->tx_single_collisions); @@ -270,6 +276,8 @@ yt921x_dsa_get_eth_mac_stats(struct dsa_switch *ds, int port, /* mac_stats->InRangeLengthErrors */ /* mac_stats->OutOfRangeLengthField */ mac_stats->FrameTooLongErrors = u64_stats_read(&mib->rx_oversize_errors); + + mutex_unlock(&priv->reg_lock); } void @@ -286,12 +294,14 @@ yt921x_dsa_get_eth_ctrl_stats(struct dsa_switch *ds, int port, mib = &pm->stats; mutex_lock(&priv->reg_lock); + yt921x_mib_read(priv, port); - mutex_unlock(&priv->reg_lock); ctrl_stats->MACControlFramesTransmitted = u64_stats_read(&mib->tx_pause); ctrl_stats->MACControlFramesReceived = u64_stats_read(&mib->rx_pause); /* ctrl_stats->UnsupportedOpcodesReceived */ + + mutex_unlock(&priv->reg_lock); } static const struct ethtool_rmon_hist_range yt921x_rmon_ranges[] = { @@ -320,8 +330,8 @@ yt921x_dsa_get_rmon_stats(struct dsa_switch *ds, int port, mib = &pm->stats; mutex_lock(&priv->reg_lock); + yt921x_mib_read(priv, port); - mutex_unlock(&priv->reg_lock); *ranges = yt921x_rmon_ranges; @@ -345,6 +355,8 @@ yt921x_dsa_get_rmon_stats(struct dsa_switch *ds, int port, rmon_stats->hist_tx[4] = u64_stats_read(&mib->tx_512_1023byte); rmon_stats->hist_tx[5] = u64_stats_read(&mib->tx_1024_1518byte); rmon_stats->hist_tx[6] = u64_stats_read(&mib->tx_jumbo); + + mutex_unlock(&priv->reg_lock); } void @@ -355,39 +367,45 @@ yt921x_dsa_get_stats64(struct dsa_switch *ds, int port, struct yt921x_port *pp = &priv->ports[port]; struct yt921x_mib *pm = pp->mib; struct yt921x_mib_stats *mib; + unsigned int start; if (!pm) return; mib = &pm->stats; - stats->rx_length_errors = u64_stats_read(&mib->rx_undersize_errors) + - u64_stats_read(&mib->rx_fragment_errors); - stats->rx_over_errors = u64_stats_read(&mib->rx_oversize_errors); - stats->rx_crc_errors = u64_stats_read(&mib->rx_crc_errors); - stats->rx_frame_errors = u64_stats_read(&mib->rx_alignment_errors); - /* stats->rx_fifo_errors */ - /* stats->rx_missed_errors */ - - stats->tx_aborted_errors = u64_stats_read(&mib->tx_aborted_errors); - /* stats->tx_carrier_errors */ - stats->tx_fifo_errors = u64_stats_read(&mib->tx_undersize_errors); - /* stats->tx_heartbeat_errors */ - stats->tx_window_errors = u64_stats_read(&mib->tx_late_collisions); - - stats->rx_packets = u64_stats_read(&pm->rx_frames); - stats->tx_packets = u64_stats_read(&pm->tx_frames); - stats->rx_bytes = u64_stats_read(&mib->rx_good_bytes) - - ETH_FCS_LEN * stats->rx_packets; - stats->tx_bytes = u64_stats_read(&mib->tx_good_bytes) - - ETH_FCS_LEN * stats->tx_packets; + do { + start = u64_stats_fetch_begin(&pm->syncp); + + stats->rx_length_errors = u64_stats_read(&mib->rx_undersize_errors) + + u64_stats_read(&mib->rx_fragment_errors); + stats->rx_over_errors = u64_stats_read(&mib->rx_oversize_errors); + stats->rx_crc_errors = u64_stats_read(&mib->rx_crc_errors); + stats->rx_frame_errors = u64_stats_read(&mib->rx_alignment_errors); + /* stats->rx_fifo_errors */ + /* stats->rx_missed_errors */ + + stats->tx_aborted_errors = u64_stats_read(&mib->tx_aborted_errors); + /* stats->tx_carrier_errors */ + stats->tx_fifo_errors = u64_stats_read(&mib->tx_undersize_errors); + /* stats->tx_heartbeat_errors */ + stats->tx_window_errors = u64_stats_read(&mib->tx_late_collisions); + + stats->rx_packets = u64_stats_read(&pm->rx_frames); + stats->tx_packets = u64_stats_read(&pm->tx_frames); + stats->rx_bytes = u64_stats_read(&mib->rx_good_bytes) - + ETH_FCS_LEN * stats->rx_packets; + stats->tx_bytes = u64_stats_read(&mib->tx_good_bytes) - + ETH_FCS_LEN * stats->tx_packets; + stats->rx_dropped = u64_stats_read(&mib->rx_dropped); + /* stats->tx_dropped */ + stats->multicast = u64_stats_read(&mib->rx_multicast); + stats->collisions = u64_stats_read(&mib->tx_collisions); + } while (u64_stats_fetch_retry(&pm->syncp, start)); + stats->rx_errors = stats->rx_length_errors + stats->rx_over_errors + stats->rx_crc_errors + stats->rx_frame_errors; stats->tx_errors = stats->tx_aborted_errors + stats->tx_fifo_errors + stats->tx_window_errors; - stats->rx_dropped = u64_stats_read(&mib->rx_dropped); - /* stats->tx_dropped */ - stats->multicast = u64_stats_read(&mib->rx_multicast); - stats->collisions = u64_stats_read(&mib->tx_collisions); } void @@ -404,9 +422,11 @@ yt921x_dsa_get_pause_stats(struct dsa_switch *ds, int port, mib = &pm->stats; mutex_lock(&priv->reg_lock); + yt921x_mib_read(priv, port); - mutex_unlock(&priv->reg_lock); pause_stats->tx_pause_frames = u64_stats_read(&mib->tx_pause); pause_stats->rx_pause_frames = u64_stats_read(&mib->rx_pause); + + mutex_unlock(&priv->reg_lock); } diff --git a/drivers/net/dsa/motorcomm/mib.h b/drivers/net/dsa/motorcomm/mib.h index d84ae2f449ac..33992a5f9bdc 100644 --- a/drivers/net/dsa/motorcomm/mib.h +++ b/drivers/net/dsa/motorcomm/mib.h @@ -125,13 +125,19 @@ struct yt921x_mib_stats { u64_stats_t tx_oam; }; +#define YT921X_MIB_NUM (sizeof(struct yt921x_mib_stats) / sizeof(u64_stats_t)) + struct yt921x_mib { struct yt921x_port *port; struct delayed_work work; + struct u64_stats_sync syncp; + /* protected by syncp OR priv->reg_lock */ struct yt921x_mib_stats stats; u64_stats_t rx_frames; u64_stats_t tx_frames; + /* protected by priv->reg_lock */ + u64 data[YT921X_MIB_NUM]; }; void yt921x_mib_poll(struct work_struct *work); -- 2.53.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v3 5/6] net: dsa: motorcomm: Fix MIB synchronization 2026-09-06 13:14 ` [PATCH net-next v3 5/6] net: dsa: motorcomm: Fix MIB synchronization David Yang @ 2026-09-09 15:16 ` netdev-bot+sashiko 2026-09-09 17:45 ` David Yang 0 siblings, 1 reply; 15+ messages in thread From: netdev-bot+sashiko @ 2026-09-09 15:16 UTC (permalink / raw) To: mmyangfl Cc: netdev, andrew, olteanv, davem, edumazet, kuba, pabeni, linux, linux-kernel Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: Critical: 0 · High: 1 · Medium: 2 (2) · Low: 1 (1) - [High] u64_stats_sync write side in yt921x_mib_read() runs with preemption enabled. - [Medium] Per-port MIB delayed work is permanently disabled after a port teardown/setup cycle. - [Medium] The commit fixes a real data-race/tearing defect in already- merged driver code (the unsynchronized 64-bit MIB accessors and… - [Low] The layout invariant tying yt921x_mib_descs[] to the u64_stats_t fields of struct yt921x_mib_stats is documented against the wrong… Pre-existing issues: - [Medium] In yt921x_dsa_get_rmon_stats() the ethtool RMON field `fragments` is filled from the alignment-error counter instead of the… - [Medium] yt921x_chip_setup() unconditionally zeroes all hardware MIB counters (drivers/net/dsa/motorcomm/chip.c:4203, YT921X_MIB_CTRL_CLEAN… - [Low] At the reviewed commit, the 64-bit MIB counters (RX_GOOD_BYTES 0x3c/0x40, RX_BAD_BYTES 0x44/0x48, TX_GOOD_BYTES 0x84/0x88 — the… -- net: dsa: motorcomm: Fix MIB synchronization This converts the yt921x MIB accounting to u64_stats_sync for the .get_stats64() path and extends priv->reg_lock coverage over the other statistics callbacks. A new per-port u64 data[] buffer collects register reads so the counters can be published in one short write section. (no Link: tags in the commit header) > net: dsa: motorcomm: Fix MIB synchronization > > The previous MIB sync was based on the false assumption that 64-bit > variables are always atomic. To fix that: [Severity: Medium] Should this carry a Fixes: tag? The unsynchronized 64-bit MIB accessors and .get_stats64() came in with 186623f4aa724c ("net: dsa: yt921x: Add support for Motorcomm YT921x"), so this is a fix to already-merged driver code. Related to that, the fix does not look self-contained: the counter type conversion lives in the preceding patch "net: dsa: motorcomm: Use u64_stats_t for MIB stats", whose changelog says "u64_stats_sync will be equipped in the next commit", and this commit also builds on "Rename MIB stuffs", "Split MIB buffers" and "Split MIB module". Since neither commit alone closes the tearing window, how would a stable backport of this fix be done without pulling in the whole refactor series? > diff --git a/drivers/net/dsa/motorcomm/chip.c b/drivers/net/dsa/motorcomm/chip.c > index e9730b9f8c629..392bf44d409e0 100644 > --- a/drivers/net/dsa/motorcomm/chip.c > +++ b/drivers/net/dsa/motorcomm/chip.c [ ... ] > @@ -3778,6 +3779,7 @@ static int yt921x_dsa_port_setup(struct dsa_switch *ds, int port) > pp->mib = pm; > > pm->port = pp; > + u64_stats_init(&pm->syncp); > INIT_DELAYED_WORK(&pm->work, yt921x_mib_poll); > } > [Severity: Medium] Does the MIB delayed work ever become schedulable again after a port teardown? yt921x_dsa_port_teardown() does: if (pm) disable_delayed_work_sync(&pm->work); which increments the work item's disable count, and enable_work() documents that "@work can only be queued if its disable count is 0". There is no enable_delayed_work() call anywhere in drivers/net/dsa/motorcomm/. The initialization above (now including u64_stats_init()) sits inside the "if (!pm && ...)" guard, and pm is devm_kzalloc()'d, so it survives a DSA tree teardown/re-setup while the switch stays bound: dsa_switch_remove() -> dsa_tree_teardown() -> yt921x_dsa_port_teardown() dsa_register_switch() -> dsa_tree_setup() -> yt921x_dsa_port_setup() On the second setup pp->mib is non-NULL, so the guarded block is skipped and the disable count stays at 1. Would schedule_delayed_work(&pm->work, 0) in yt921x_phylink_mac_link_up() then silently do nothing, leaving the 32-bit wraparound accumulation in yt921x_mib_read() stopped and .get_stats64() returning frozen values? The asymmetry was introduced by the earlier patch in this series, "net: dsa: motorcomm: Split MIB buffers", which moved disable_delayed_work_sync() out of yt921x_mdio_remove() into the new .port_teardown callback, and it is still there at the series tip. [Severity: Medium] This isn't a bug introduced by this patch, but on the same teardown/re-setup path, is the software shadow ever resynchronized with the hardware counters? yt921x_chip_setup() unconditionally clears them: /* Clear MIB */ ctrl = YT921X_MIB_CTRL_CLEAN | YT921X_MIB_CTRL_ALL_PORT; res = yt921x_reg_write(priv, YT921X_MIB_CTRL, ctrl); while pm (and now pm->data as well as pm->stats) is only zeroed at devm_kzalloc() time above. After the hardware counters go back to zero, the wrap heuristic in yt921x_mib_read(): u64 old_val = buf[i]; val = (old_val & ~(u64)U32_MAX) | val0; if (val < old_val) val += 1ull << 32; would see a small val0 against a large old_val and add 2^32, so every 32-bit counter would over-report by up to ~4.29e9 for the rest of the driver's lifetime. The comment "MIB buffer must be zeroed before" in mib.c only holds for the first allocation. > diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c > index f5d39d22215d4..8601d25fe9887 100644 > --- a/drivers/net/dsa/motorcomm/mib.c > +++ b/drivers/net/dsa/motorcomm/mib.c [ ... ] > @@ -110,7 +110,7 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) > break; > > if (desc->size <= 1) { > - u64 old_val = u64_stats_read(valp); > + u64 old_val = buf[i]; > > val = (old_val & ~(u64)U32_MAX) | val0; > if (val < old_val) [Severity: Low] This isn't a bug introduced by this patch, but the MIB_DESC(2, ...) counters (RX_GOOD_BYTES 0x3c/0x40, RX_BAD_BYTES 0x44/0x48, TX_GOOD_BYTES 0x84/0x88) are still composed from two independent SMI reads in the else branch just below: } else { u32 val1; res = yt921x_reg_read(priv, reg + 4, &val1); if (res) break; val = ((u64)val1 << 32) | val0; } If the counter crosses a 2^32 boundary between the two transactions, does the stale low word get combined with the already incremented high word, giving a ~4 GiB jump in rx_bytes/tx_bytes and Octets*OK? The later patch in this series, "net: dsa: motorcomm: Use safe 64-bit counter reader", replaces this with yt921x_counter_read(), which re-reads the low word and retries until lo >= old_lo, so the concern does not survive to the end of the series. > @@ -124,9 +124,12 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) > val = ((u64)val1 << 32) | val0; > } > > - u64_stats_set(valp, val); > + buf[i] = val; > } > > + u64_stats_update_begin(&pm->syncp); > + for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) > + u64_stats_set(&((u64_stats_t *)mib)[i], buf[i]); [Severity: High] Should this write section run with preemption disabled? yt921x_mib_read() is only ever called with priv->reg_lock held, which is a sleepable mutex, and all callers are fully preemptible process context: yt921x_mib_poll() mutex_lock(&priv->reg_lock); yt921x_mib_read() u64_stats_update_begin(&pm->syncp); include/linux/u64_stats_sync.h states as constraint (2) that the "Write side must disable preemption, or a seqcount reader can preempt the writer and also spin forever". On BITS_PER_LONG==32, __u64_stats_update_begin() uses preempt_disable_nested(), which does not disable preemption on non-PREEMPT_RT builds: if (IS_ENABLED(CONFIG_PREEMPT_RT)) \ preempt_disable(); \ else \ lockdep_assert_preemption_disabled(); So on 32-bit with CONFIG_PROVE_LOCKING, would this warn on every poll (every YT921X_STATS_INTERVAL_JIFFIES, 3 s)? And without lockdep, the sequence stays odd across the ~40 u64_stats_set() stores plus the two histogram summations while preemption is enabled. The reader takes no mutex and can run with preemption off, for example: bond_get_stats() spin_lock_nested(&bond->stats_lock) dev_get_stats() -> dsa_user_get_stats64() -> yt921x_dsa_get_stats64() u64_stats_fetch_begin(&pm->syncp) If that reader lands on the same CPU as a preempted, CPU-bound kworker writer, does it spin indefinitely in __read_seqcount_begin()? Would u64_stats_update_begin_irqsave()/u64_stats_update_end_irqrestore(), or an explicit preempt_disable() around the write section, be the right form here? > rx_frames = u64_stats_read(&mib->rx_64byte) + > u64_stats_read(&mib->rx_65_127byte) + > u64_stats_read(&mib->rx_128_255byte) + > @@ -143,6 +146,7 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) > u64_stats_read(&mib->tx_jumbo); > u64_stats_set(&pm->rx_frames, rx_frames); > u64_stats_set(&pm->tx_frames, tx_frames); > + u64_stats_update_end(&pm->syncp); > > if (res) > dev_err(dev, "Failed to %s port %d: %i\n", "read stats for", [ ... ] > @@ -320,8 +330,8 @@ yt921x_dsa_get_rmon_stats(struct dsa_switch *ds, int port, > mib = &pm->stats; > > mutex_lock(&priv->reg_lock); > + > yt921x_mib_read(priv, port); > - mutex_unlock(&priv->reg_lock); > > *ranges = yt921x_rmon_ranges; > [Severity: Medium] This is a pre-existing issue and not something this patch introduced, but a few lines below in yt921x_dsa_get_rmon_stats() the RMON fragments field is fed from the alignment error counter: rmon_stats->undersize_pkts = u64_stats_read(&mib->rx_undersize_errors); rmon_stats->oversize_pkts = u64_stats_read(&mib->rx_oversize_errors); rmon_stats->fragments = u64_stats_read(&mib->rx_alignment_errors); /* rmon_stats->jabbers */ struct ethtool_rmon_stats defines fragments as etherStatsFragments, and the driver has a dedicated rx_fragment_errors counter (fed from YT921X_MIB_DATA_RX_FRAG_ERR) which is only used in yt921x_dsa_get_stats64(): stats->rx_length_errors = u64_stats_read(&mib->rx_undersize_errors) + u64_stats_read(&mib->rx_fragment_errors); ... stats->rx_frame_errors = u64_stats_read(&mib->rx_alignment_errors); Should rmon_stats->fragments read rx_fragment_errors instead? > @@ -345,6 +355,8 @@ yt921x_dsa_get_rmon_stats(struct dsa_switch *ds, int port, > rmon_stats->hist_tx[4] = u64_stats_read(&mib->tx_512_1023byte); > rmon_stats->hist_tx[5] = u64_stats_read(&mib->tx_1024_1518byte); > rmon_stats->hist_tx[6] = u64_stats_read(&mib->tx_jumbo); > + > + mutex_unlock(&priv->reg_lock); > } [ ... ] > diff --git a/drivers/net/dsa/motorcomm/mib.h b/drivers/net/dsa/motorcomm/mib.h > index d84ae2f449ac8..33992a5f9bdc1 100644 > --- a/drivers/net/dsa/motorcomm/mib.h > +++ b/drivers/net/dsa/motorcomm/mib.h > @@ -125,13 +125,19 @@ struct yt921x_mib_stats { > u64_stats_t tx_oam; > }; > > +#define YT921X_MIB_NUM (sizeof(struct yt921x_mib_stats) / sizeof(u64_stats_t)) > + > struct yt921x_mib { > struct yt921x_port *port; > > struct delayed_work work; > + struct u64_stats_sync syncp; > + /* protected by syncp OR priv->reg_lock */ > struct yt921x_mib_stats stats; > u64_stats_t rx_frames; > u64_stats_t tx_frames; > + /* protected by priv->reg_lock */ > + u64 data[YT921X_MIB_NUM]; > }; [Severity: Low] Could a static_assert() be added here to tie YT921X_MIB_NUM to ARRAY_SIZE(yt921x_mib_descs)? Both loops in yt921x_mib_read() are bounded by ARRAY_SIZE(yt921x_mib_descs) while data[] is now sized from sizeof(struct yt921x_mib_stats), and data[] is the last member of the devm_kzalloc()'d struct yt921x_mib. Both counts are 39 today, so nothing overruns at this commit, and no such assertion appears later in the series. Adding a descriptor without a matching u64_stats_t field would turn what used to be an in-struct overrun into a write past the end of the allocation. Also, the invariant comment above yt921x_mib_descs[] in mib.c says: /* Must agree with yt921x_mib * Should that name yt921x_mib_stats, which is the struct YT921X_MIB_NUM measures and the one the descriptor table actually indexes? -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906131414.2683362-1-mmyangfl%40gmail.com ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v3 5/6] net: dsa: motorcomm: Fix MIB synchronization 2026-09-09 15:16 ` netdev-bot+sashiko @ 2026-09-09 17:45 ` David Yang 0 siblings, 0 replies; 15+ messages in thread From: David Yang @ 2026-09-09 17:45 UTC (permalink / raw) To: netdev-bot+sashiko Cc: netdev, andrew, olteanv, davem, edumazet, kuba, pabeni, linux, linux-kernel On Wed, Sep 9, 2026 at 11:16 PM <netdev-bot+sashiko@kernel.org> wrote: > [Severity: Medium] > Should this carry a Fixes: tag? The unsynchronized 64-bit MIB accessors > and .get_stats64() came in with 186623f4aa724c ("net: dsa: yt921x: Add > support for Motorcomm YT921x"), so this is a fix to already-merged driver > code. > > Related to that, the fix does not look self-contained: the counter type > conversion lives in the preceding patch "net: dsa: motorcomm: Use > u64_stats_t for MIB stats", whose changelog says "u64_stats_sync will be > equipped in the next commit", and this commit also builds on "Rename MIB > stuffs", "Split MIB buffers" and "Split MIB module". > > Since neither commit alone closes the tearing window, how would a stable > backport of this fix be done without pulling in the whole refactor series? This is a low-risk issue and does not affect control paths. Also it would be quite impossible to backport it since the whole series is to fix this and it would require major refactor on the struct. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net-next v3 6/6] net: dsa: motorcomm: Use safe 64-bit counter reader 2026-09-06 13:14 [PATCH net-next v3 0/6] net: dsa: motorcomm: MIB fixup David Yang ` (4 preceding siblings ...) 2026-09-06 13:14 ` [PATCH net-next v3 5/6] net: dsa: motorcomm: Fix MIB synchronization David Yang @ 2026-09-06 13:14 ` David Yang 2026-09-09 15:16 ` netdev-bot+sashiko 5 siblings, 1 reply; 15+ messages in thread From: David Yang @ 2026-09-06 13:14 UTC (permalink / raw) To: netdev Cc: David Yang, Andrew Lunn, Vladimir Oltean, David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni, Russell King, linux-kernel 64-bit counter registers are read as two separate 32-bit transactions. If the low word wraps from about 0xffffffff to a small value while the high word increments between the two transactions, the recombined value becomes ((new_high << 32) | old_low), producing a spurious ~4 GiB jump. Use a safe counter reader for this. Signed-off-by: David Yang <mmyangfl@gmail.com> --- drivers/net/dsa/motorcomm/mib.c | 21 +++++++++------------ drivers/net/dsa/motorcomm/smi.c | 30 ++++++++++++++++++++++++++++++ drivers/net/dsa/motorcomm/smi.h | 2 ++ 3 files changed, 41 insertions(+), 12 deletions(-) diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c index 8601d25fe988..cfae2977a77b 100644 --- a/drivers/net/dsa/motorcomm/mib.c +++ b/drivers/net/dsa/motorcomm/mib.c @@ -102,29 +102,26 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) { const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i]; u32 reg = YT921X_MIBn_DATA0(port) + desc->offset; - u32 val0; - u64 val; - - res = yt921x_reg_read(priv, reg, &val0); - if (res) - break; if (desc->size <= 1) { u64 old_val = buf[i]; + u32 val0; + u64 val; + + res = yt921x_reg_read(priv, reg, &val0); + if (res) + break; val = (old_val & ~(u64)U32_MAX) | val0; if (val < old_val) val += 1ull << 32; - } else { - u32 val1; - res = yt921x_reg_read(priv, reg + 4, &val1); + buf[i] = val; + } else { + res = yt921x_counter_read(priv, reg, &buf[i]); if (res) break; - val = ((u64)val1 << 32) | val0; } - - buf[i] = val; } u64_stats_update_begin(&pm->syncp); diff --git a/drivers/net/dsa/motorcomm/smi.c b/drivers/net/dsa/motorcomm/smi.c index bf3adfd64165..27086602997b 100644 --- a/drivers/net/dsa/motorcomm/smi.c +++ b/drivers/net/dsa/motorcomm/smi.c @@ -63,6 +63,36 @@ int yt921x_reg_update_bits(struct yt921x_priv *priv, u32 reg, u32 mask, u32 val) return yt921x_reg_write(priv, reg, u); } +/* Reliably read a 64bit counter */ +int yt921x_counter_read(struct yt921x_priv *priv, u32 reg, u64 *valp) +{ + u32 old_lo; + int res; + u32 hi; + u32 lo; + + res = yt921x_reg_read(priv, reg, &old_lo); + if (res) + return res; + + for (int i = 0; i < 16; i++) { + res = yt921x_reg_read(priv, reg + 4, &hi); + if (res) + return res; + res = yt921x_reg_read(priv, reg, &lo); + if (res) + return res; + + if (lo >= old_lo) { + *valp = ((u64)hi << 32) | lo; + return 0; + } + old_lo = lo; + } + + return -ETIMEDOUT; +} + static int yt921x_regs_read(struct yt921x_priv *priv, u32 reg, u32 *vals, unsigned int num_regs) diff --git a/drivers/net/dsa/motorcomm/smi.h b/drivers/net/dsa/motorcomm/smi.h index 212e20f71d80..d34240548af7 100644 --- a/drivers/net/dsa/motorcomm/smi.h +++ b/drivers/net/dsa/motorcomm/smi.h @@ -35,6 +35,8 @@ yt921x_reg_toggle_bits(struct yt921x_priv *priv, u32 reg, u32 mask, bool set) return yt921x_reg_update_bits(priv, reg, mask, !set ? 0 : mask); } +int yt921x_counter_read(struct yt921x_priv *priv, u32 reg, u64 *valp); + /* Some multi-word registers, like VLANn_CTRL, should be treated as a single * long register. More specifically, writes to parts of its words won't become * visible, until the last word is written. -- 2.53.0 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net-next v3 6/6] net: dsa: motorcomm: Use safe 64-bit counter reader 2026-09-06 13:14 ` [PATCH net-next v3 6/6] net: dsa: motorcomm: Use safe 64-bit counter reader David Yang @ 2026-09-09 15:16 ` netdev-bot+sashiko 0 siblings, 0 replies; 15+ messages in thread From: netdev-bot+sashiko @ 2026-09-09 15:16 UTC (permalink / raw) To: mmyangfl Cc: netdev, andrew, olteanv, davem, edumazet, kuba, pabeni, linux, linux-kernel Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: Critical: 0 · High: 0 · Medium: 0 · Low: 1 - [Low] The patch is a functional bug fix (it removes spurious ~4 GiB jumps in the 64-bit MIB byte counters that are exported to userspace)… -- net: dsa: motorcomm: Use safe 64-bit counter reader This switches the multi-word MIB counters in yt921x_mib_read() over to a new helper, yt921x_counter_read(), which re-reads the low word after the high word and retries until the pair is consistent. The single-word path keeps its existing software wrap extension. > diff --git a/drivers/net/dsa/motorcomm/mib.c b/drivers/net/dsa/motorcomm/mib.c > index 8601d25fe9887..cfae2977a77b9 100644 > --- a/drivers/net/dsa/motorcomm/mib.c > +++ b/drivers/net/dsa/motorcomm/mib.c > @@ -102,29 +102,26 @@ static int yt921x_mib_read(struct yt921x_priv *priv, int port) > for (size_t i = 0; i < ARRAY_SIZE(yt921x_mib_descs); i++) { > const struct yt921x_mib_desc *desc = &yt921x_mib_descs[i]; > u32 reg = YT921X_MIBn_DATA0(port) + desc->offset; > - u32 val0; > - u64 val; > - > - res = yt921x_reg_read(priv, reg, &val0); > - if (res) > - break; > > if (desc->size <= 1) { > u64 old_val = buf[i]; > + u32 val0; > + u64 val; > + > + res = yt921x_reg_read(priv, reg, &val0); > + if (res) > + break; > > val = (old_val & ~(u64)U32_MAX) | val0; > if (val < old_val) > val += 1ull << 32; > - } else { > - u32 val1; > > - res = yt921x_reg_read(priv, reg + 4, &val1); > + buf[i] = val; > + } else { > + res = yt921x_counter_read(priv, reg, &buf[i]); > if (res) > break; > - val = ((u64)val1 << 32) | val0; > } > - > - buf[i] = val; > } [Severity: Low] Should this hunk carry a Fixes: tag and a stable Cc? The commit message describes a concrete user-visible misbehaviour ("producing a spurious ~4 GiB jump"), and the racy sequence it replaces is already in a released kernel. At the baseline the same code lives in drivers/net/dsa/motorcomm/chip.c:yt921x_mib_read(): res = yt921x_reg_read(priv, reg, &val0); ... res = yt921x_reg_read(priv, reg + 4, &val1); if (res) break; val = ((u64)val1 << 32) | val0; git blame attributes that two-transaction read to 186623f4aa724c ("net: dsa: Add Motorcomm YT921x DSA switch support"), and the earlier patches in this series only moved it from chip.c into mib.c:yt921x_mib_read(). The affected entries are the desc->size > 1 counters, i.e. rx_good_bytes, rx_bad_bytes and tx_good_bytes in struct yt921x_mib_stats, which are reported through the periodic MIB poll and the DSA get_stats64()/ethtool paths. Without a Fixes: tag pointing at 186623f4aa724c, would this hunk be distinguishable from the surrounding refactor for backporting purposes? [ ... smi.c and smi.h hunks snipped, no comments ... ] -- Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260906131414.2683362-1-mmyangfl%40gmail.com ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2026-09-09 17:46 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-09-06 13:14 [PATCH net-next v3 0/6] net: dsa: motorcomm: MIB fixup David Yang 2026-09-06 13:14 ` [PATCH net-next v3 1/6] net: dsa: motorcomm: Rename MIB stuffs David Yang 2026-09-09 15:16 ` netdev-bot+sashiko 2026-09-09 17:32 ` David Yang 2026-09-06 13:14 ` [PATCH net-next v3 2/6] net: dsa: motorcomm: Split MIB buffers David Yang 2026-09-09 15:16 ` netdev-bot+sashiko 2026-09-06 13:14 ` [PATCH net-next v3 3/6] net: dsa: motorcomm: Split MIB module David Yang 2026-09-09 15:16 ` netdev-bot+sashiko 2026-09-06 13:14 ` [PATCH net-next v3 4/6] net: dsa: motorcomm: Use u64_stats_t for MIB stats David Yang 2026-09-09 15:16 ` netdev-bot+sashiko 2026-09-06 13:14 ` [PATCH net-next v3 5/6] net: dsa: motorcomm: Fix MIB synchronization David Yang 2026-09-09 15:16 ` netdev-bot+sashiko 2026-09-09 17:45 ` David Yang 2026-09-06 13:14 ` [PATCH net-next v3 6/6] net: dsa: motorcomm: Use safe 64-bit counter reader David Yang 2026-09-09 15:16 ` netdev-bot+sashiko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox