diff mbox series

[net-next,v3] net: stmmac: mmc_core: Drop interrupt registers from stats

Message ID 20240223-stmmac_stats-v3-1-5d483c2a071a@axis.com (mailing list archive)
State New, archived
Headers show
Series [net-next,v3] net: stmmac: mmc_core: Drop interrupt registers from stats | expand

Commit Message

Jesper Nilsson Feb. 23, 2024, 8:37 p.m. UTC 
The MMC IPC interrupt status and interrupt mask registers are
of little use as Ethernet statistics, but incrementing counters
based on the current interrupt and interrupt mask registers
makes them actively misleading.

For example, if the interrupt mask is set to 0x08420842,
the current code will increment by that amount each iteration,
leading to the following sequence of nonsense:

mmc_rx_ipc_intr_mask: 969816526
mmc_rx_ipc_intr_mask: 1108361744

These registers have been included in the Ethernet statistics
since the first version of MMC back in 2011 (commit 1c901a46d57).
That commit also mentions the MMC interrupts as
"something to add later (if actually useful)".

If the registers are actually useful, they should probably
be part of the Ethernet register dump instead of statistics,
but for now, drop the counters for mmc_rx_ipc_intr and
mmc_rx_ipc_intr_mask completely.

Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
Signed-off-by: Jesper Nilsson <jesper.nilsson@axis.com>
---
Changes in v3:
- Rebase to be against net-next
- Link to v2: https://lore.kernel.org/r/20240220-stmmac_stats-v2-1-0a78863bec70@axis.com

Changes in v2:
- Drop the misleading registers completely
- Link to v1: https://lore.kernel.org/r/20240216-stmmac_stats-v1-1-7065fa4613f8@axis.com
---
 drivers/net/ethernet/stmicro/stmmac/mmc.h            | 4 ----
 drivers/net/ethernet/stmicro/stmmac/mmc_core.c       | 3 ---
 drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 2 --
 3 files changed, 9 deletions(-)


---
base-commit: a08689109c5989acdc5c320de8e45324f6cfa791
change-id: 20240216-stmmac_stats-e3561d460d0e

Best regards,

Comments

Jiri Pirko Feb. 26, 2024, 9:22 a.m. UTC | #1 
Fri, Feb 23, 2024 at 09:37:01PM CET, jesper.nilsson@axis.com wrote:
>The MMC IPC interrupt status and interrupt mask registers are
>of little use as Ethernet statistics, but incrementing counters
>based on the current interrupt and interrupt mask registers
>makes them actively misleading.
>
>For example, if the interrupt mask is set to 0x08420842,
>the current code will increment by that amount each iteration,
>leading to the following sequence of nonsense:
>
>mmc_rx_ipc_intr_mask: 969816526
>mmc_rx_ipc_intr_mask: 1108361744
>
>These registers have been included in the Ethernet statistics
>since the first version of MMC back in 2011 (commit 1c901a46d57).
>That commit also mentions the MMC interrupts as
>"something to add later (if actually useful)".
>
>If the registers are actually useful, they should probably
>be part of the Ethernet register dump instead of statistics,
>but for now, drop the counters for mmc_rx_ipc_intr and
>mmc_rx_ipc_intr_mask completely.
>
>Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
>Signed-off-by: Jesper Nilsson <jesper.nilsson@axis.com>

Reviewed-by: Jiri Pirko <jiri@nvidia.com>

[...]

>---
>base-commit: a08689109c5989acdc5c320de8e45324f6cfa791
>change-id: 20240216-stmmac_stats-e3561d460d0e


Not sure what this is good for...
patchwork-bot+netdevbpf@kernel.org Feb. 27, 2024, 10:40 a.m. UTC | #2 
Hello:

This patch was applied to netdev/net-next.git (main)
by Paolo Abeni <pabeni@redhat.com>:

On Fri, 23 Feb 2024 21:37:01 +0100 you wrote:
> The MMC IPC interrupt status and interrupt mask registers are
> of little use as Ethernet statistics, but incrementing counters
> based on the current interrupt and interrupt mask registers
> makes them actively misleading.
> 
> For example, if the interrupt mask is set to 0x08420842,
> the current code will increment by that amount each iteration,
> leading to the following sequence of nonsense:
> 
> [...]

Here is the summary with links:
  - [net-next,v3] net: stmmac: mmc_core: Drop interrupt registers from stats
    https://git.kernel.org/netdev/net-next/c/d0dc1e42109d

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/mmc.h b/drivers/net/ethernet/stmicro/stmmac/mmc.h
index 14c9d2637dfe..dff02d75d519 100644
--- a/drivers/net/ethernet/stmicro/stmmac/mmc.h
+++ b/drivers/net/ethernet/stmicro/stmmac/mmc.h
@@ -86,10 +86,6 @@  struct stmmac_counters {
 	unsigned int mmc_rx_discard_octets_gb;
 	unsigned int mmc_rx_align_err_frames;
 
-	/* IPC */
-	unsigned int mmc_rx_ipc_intr_mask;
-	unsigned int mmc_rx_ipc_intr;
-
 	/* IPv4 */
 	unsigned int mmc_rx_ipv4_gd;
 	unsigned int mmc_rx_ipv4_hderr;
diff --git a/drivers/net/ethernet/stmicro/stmmac/mmc_core.c b/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
index 8597c6abae8d..7eb477faa75a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
@@ -316,9 +316,6 @@  static void dwmac_mmc_read(void __iomem *mmcaddr, struct stmmac_counters *mmc)
 	mmc->mmc_rx_fifo_overflow += readl(mmcaddr + MMC_RX_FIFO_OVERFLOW);
 	mmc->mmc_rx_vlan_frames_gb += readl(mmcaddr + MMC_RX_VLAN_FRAMES_GB);
 	mmc->mmc_rx_watchdog_error += readl(mmcaddr + MMC_RX_WATCHDOG_ERROR);
-	/* IPC */
-	mmc->mmc_rx_ipc_intr_mask += readl(mmcaddr + MMC_RX_IPC_INTR_MASK);
-	mmc->mmc_rx_ipc_intr += readl(mmcaddr + MMC_RX_IPC_INTR);
 	/* IPv4 */
 	mmc->mmc_rx_ipv4_gd += readl(mmcaddr + MMC_RX_IPV4_GD);
 	mmc->mmc_rx_ipv4_hderr += readl(mmcaddr + MMC_RX_IPV4_HDERR);
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 0e44b84fb7e7..e1537a57815f 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -243,8 +243,6 @@  static const struct stmmac_stats stmmac_mmc[] = {
 	STMMAC_MMC_STAT(mmc_rx_discard_frames_gb),
 	STMMAC_MMC_STAT(mmc_rx_discard_octets_gb),
 	STMMAC_MMC_STAT(mmc_rx_align_err_frames),
-	STMMAC_MMC_STAT(mmc_rx_ipc_intr_mask),
-	STMMAC_MMC_STAT(mmc_rx_ipc_intr),
 	STMMAC_MMC_STAT(mmc_rx_ipv4_gd),
 	STMMAC_MMC_STAT(mmc_rx_ipv4_hderr),
 	STMMAC_MMC_STAT(mmc_rx_ipv4_nopay),