linux-kernel - Re: [PATCH net-next 3/3] net: xilinx: axienet: Add statistics support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Tue, 11 Jun 2024 02:13:40 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Sean Anderson <sean.anderson@...ux.dev>
Cc: Radhey Shyam Pandey <radhey.shyam.pandey@....com>,
	netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	Michal Simek <michal.simek@....com>,
	Jakub Kicinski <kuba@...nel.org>,
	Russell King <linux@...linux.org.uk>,
	Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>,
	linux-kernel@...r.kernel.org,
	"David S . Miller" <davem@...emloft.net>
Subject: Re: [PATCH net-next 3/3] net: xilinx: axienet: Add statistics support

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