Received-SPF: pass (google.com: domain of linux-kernel+bounces-209110-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 11 Jun 2024 02:13:40 +0200
From: Andrew Lunn <andrew@lunn.ch>
To: Sean Anderson <sean.anderson@linux.dev>
Cc: Radhey Shyam Pandey <radhey.shyam.pandey@amd.com>,
	netdev@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
	Michal Simek <michal.simek@amd.com>,
	Jakub Kicinski <kuba@kernel.org>,
	Russell King <linux@armlinux.org.uk>,
	Paolo Abeni <pabeni@redhat.com>, Eric Dumazet <edumazet@google.com>,
	linux-kernel@vger.kernel.org,
	"David S . Miller" <davem@davemloft.net>
Subject: Re: [PATCH net-next 3/3] net: xilinx: axienet: Add statistics support
Message-ID: <7c06c9d7-ad11-4acd-8c80-fbeb902da40d@lunn.ch>
References: <20240610231022.2460953-1-sean.anderson@linux.dev>
 <20240610231022.2460953-4-sean.anderson@linux.dev>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240610231022.2460953-4-sean.anderson@linux.dev>

On Mon, Jun 10, 2024 at 07:10:22PM -0400, Sean Anderson wrote:
> Add support for reading the statistics counters, if they are enabled.
> The counters may be 64-bit, but we can't detect this as there's no
> ability bit for it and the counters are read-only. Therefore, we assume
> the counters are 32-bits.

> +static void axienet_stats_update(struct axienet_local *lp)
> +{
> +	enum temac_stat stat;
> +
> +	lockdep_assert_held(&lp->stats_lock);
> +
> +	u64_stats_update_begin(&lp->hw_stat_sync);
> +	for (stat = 0; stat < STAT_COUNT; stat++) {
> +		u32 counter = axienet_ior(lp, XAE_STATS_OFFSET + stat * 8);

The * 8 here suggests the counters are spaced so that they could be 64
bit wide, even when only 32 bits are used. Does the documentation say
anything about the upper 32 bits when the counters are only 32 bits?
Are they guaranteed to read as zero? I'm just wondering if the code
should be forward looking and read all 64 bits? 

>  static int __axienet_device_reset(struct axienet_local *lp)
>  {
>  	u32 value;
>  	int ret;
>  
> +	/* Save statistics counters in case they will be reset */
> +	if (lp->features & XAE_FEATURE_STATS) {
> +		mutex_lock(&lp->stats_lock);
> +		axienet_stats_update(lp);
> +	}

It is a pretty unusual pattern to split a mutex lock/unlock like this
on an if statement. Maybe just unconditionally hold the mutex? This
does not appear to be anyway hot path, so the overhead should not
matter.

	Andrew