Received-SPF: pass (google.com: domain of linux-kernel+bounces-69053-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <61bdd802-abe4-4544-8e48-9493a6bb99c8@gmail.com>
Date: Fri, 16 Feb 2024 09:13:51 -0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] net: stmmac: mmc_core: Assign, don't add interrupt
 registers
Content-Language: en-US
To: Jesper Nilsson <jesper.nilsson@axis.com>,
 Alexandre Torgue <alexandre.torgue@foss.st.com>,
 Jose Abreu <joabreu@synopsys.com>, "David S. Miller" <davem@davemloft.net>,
 Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>,
 Paolo Abeni <pabeni@redhat.com>, Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: netdev@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com,
 linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
 kernel@axis.com
References: <20240216-stmmac_stats-v1-1-7065fa4613f8@axis.com>
From: Florian Fainelli <f.fainelli@gmail.com>
In-Reply-To: <20240216-stmmac_stats-v1-1-7065fa4613f8@axis.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 2/16/24 07:24, 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 worse than useless.
> 
> 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
> 
> Change the increment to a straight assignment to make the
> statistics at least nominally useful.
> 
> Signed-off-by: Jesper Nilsson <jesper.nilsson@axis.com>
> ---
>   drivers/net/ethernet/stmicro/stmmac/mmc_core.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/mmc_core.c b/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
> index 6a7c1d325c46..6051a22b3cec 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/mmc_core.c
> @@ -280,8 +280,8 @@ static void dwmac_mmc_read(void __iomem *mmcaddr, struct stmmac_counters *mmc)
>   	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);
> +	mmc->mmc_rx_ipc_intr_mask = readl(mmcaddr + MMC_RX_IPC_INTR_MASK);
> +	mmc->mmc_rx_ipc_intr = readl(mmcaddr + MMC_RX_IPC_INTR);

So in premise I agree with the patch, that incrementing those is not the 
right way to go about them. However these registers are currently 
provided as part of the statistics set, but they should instead be 
accessed via the register dumping method.

In either case you will get at best a snapshot of those two registers at 
any given time and I suppose this can help diagnose a stuck RX 
condition, but not much more than that.
-- 
Florian