Re: [PATCH v2] net: stmmac: mmc_core: Drop interrupt registers from stats

[Serge Semin]

Hi Jesper

On Tue, Feb 20, 2024 at 01:00:22PM +0100, Jesper Nilsson 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.
> 
> Signed-off-by: Jesper Nilsson <jesper.nilsson@xxxxxxxx>

Thank you very much! Definitely:
Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx>

One more statistics-related clean-up you may find useful:

net: stmmac: mmc: Discard double Rx CRC errors counter read
    
DW XGMAC MMC Rx CRC errors counter is read twice in a row. It's redundant
to do so, because unlikely the counter has changed since the first read
much while up to date statistics can be retrieved on the next MMC-read
operation. In that matter Rx CRC errors counter is no different from the
other counters, which are read just once in the same callback. So most
likely the second Rx CRC errors counter read has been added by mistake.
Let's discard it then.
    
Fixes: b6cdf09f51c2 ("net: stmmac: xgmac: Implement MMC counters")

diff --git a/drivers/net/ethernet/stmicro/stmmac/mmc_core.c b/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
index 8597c6abae8d..759a83100f11 100644
--- a/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
+++ b/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
@@ -467,8 +467,6 @@ static void dwxgmac_mmc_read(void __iomem *mmcaddr, struct stmmac_counters *mmc)
                             &mmc->mmc_rx_multicastframe_g);
        dwxgmac_read_mmc_reg(mmcaddr, MMC_XGMAC_RX_CRC_ERR,
                             &mmc->mmc_rx_crc_error);
-       dwxgmac_read_mmc_reg(mmcaddr, MMC_XGMAC_RX_CRC_ERR,
-                            &mmc->mmc_rx_crc_error);
        mmc->mmc_rx_run_error += readl(mmcaddr + MMC_XGMAC_RX_RUNT_ERR);
        mmc->mmc_rx_jabber_error += readl(mmcaddr + MMC_XGMAC_RX_JABBER_ERR);
        mmc->mmc_rx_undersize_g += readl(mmcaddr + MMC_XGMAC_RX_UNDER);

-Serge(y)

> ---
> Changes in v2:
> - Drop the misleading registers completely
> - Link to v1: https://lore.kernel.org/r/20240216-stmmac_stats-v1-1-7065fa4613f8@xxxxxxxx
> ---
>  drivers/net/ethernet/stmicro/stmmac/mmc.h            | 3 ---
>  drivers/net/ethernet/stmicro/stmmac/mmc_core.c       | 3 ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c | 2 --
>  3 files changed, 8 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/mmc.h b/drivers/net/ethernet/stmicro/stmmac/mmc.h
> index a0c05925883e..8cfba817491b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/mmc.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/mmc.h
> @@ -78,9 +78,6 @@ struct stmmac_counters {
>  	unsigned int mmc_rx_fifo_overflow;
>  	unsigned int mmc_rx_vlan_frames_gb;
>  	unsigned int mmc_rx_watchdog_error;
> -	/* 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 6a7c1d325c46..ab3b7770f62d 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
> @@ -279,9 +279,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 f628411ae4ae..28accdc98282 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
> @@ -236,8 +236,6 @@ static const struct stmmac_stats stmmac_mmc[] = {
>  	STMMAC_MMC_STAT(mmc_rx_fifo_overflow),
>  	STMMAC_MMC_STAT(mmc_rx_vlan_frames_gb),
>  	STMMAC_MMC_STAT(mmc_rx_watchdog_error),
> -	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),
> 
> ---
> base-commit: 0dd3ee31125508cd67f7e7172247f05b7fd1753a
> change-id: 20240216-stmmac_stats-e3561d460d0e
> 
> Best regards,
> -- 
> 
> /^JN - Jesper Nilsson
> -- 
>                Jesper Nilsson -- jesper.nilsson@xxxxxxxx
> 
>