linux-kernel - Re: [PATCH net-next v2] net: ti: icssg-prueth: Add ICSSG FW Stats

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] [day] [month] [year] [list]

Message-ID: <20250307083959.33098949@kernel.org>
Date: Fri, 7 Mar 2025 08:39:59 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: MD Danish Anwar <danishanwar@...com>
Cc: Vignesh Raghavendra <vigneshr@...com>, Meghana Malladi
 <m-malladi@...com>, Diogo Ivo <diogo.ivo@...mens.com>, Lee Trager
 <lee@...ger.us>, Andrew Lunn <andrew+netdev@...n.ch>, Roger Quadros
 <rogerq@...nel.org>, Jonathan Corbet <corbet@....net>, Simon Horman
 <horms@...nel.org>, Paolo Abeni <pabeni@...hat.com>, Eric Dumazet
 <edumazet@...gle.com>, "David S. Miller" <davem@...emloft.net>,
 <linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
 <linux-doc@...r.kernel.org>, <netdev@...r.kernel.org>, <srk@...com>
Subject: Re: [PATCH net-next v2] net: ti: icssg-prueth: Add ICSSG FW Stats

On Fri, 7 Mar 2025 16:00:40 +0530 MD Danish Anwar wrote:
> > Thanks for the docs, it looks good. Now, do all of these get included
> > in the standard stats returned by icssg_ndo_get_stats64 ?
> > That's the primary source of information for the user regarding packet
> > loss.  
> 
> No, these are not reported via icssg_ndo_get_stats64.
> 
> .ndo_get_stats64 populates stats that are part of `struct
> rtnl_link_stats64`. icssg_ndo_get_stats64 populates those stats wherever
> applicable. These firmware stats are not same as the ones defined in
> `icssg_ndo_get_stats64` hence they are not populated. They are not
> standard stats, they will be dumped by `ethtool -S`. Wherever there is a
> standard stats, I will make sure it gets dumped from the standard
> interface instead of `ethtool -S`
> 
> Only below stats are included in the standard stats returned by
> icssg_ndo_get_stats64
> - rx_packets
> - rx_bytes
> - tx_packets
> - tx_bytes
> - rx_crc_errors
> - rx_over_errors
> - rx_multicast_frames

Yes, but if the stats you're adding here relate to packets sent /
destined to the host which were lost you should include them
in the aggregate rx_errors / rx_dropped / tx_errors / tx_dropped.
I understand that there's unlikely to be a 1:1 match with specific
stats.

> > This gets called by icssg_ndo_get_stats64() which is under RCU   
> 
> Yes, this does get called by icssg_ndo_get_stats64(). Apart from that
> there is a workqueue (`icssg_stats_work_handler`) that calls this API
> periodically and updates the emac->stats and emac->pa_stats arrays.
>
> > protection and nothing else. I don't see any locking here, and  
> 
> There is no locking here. I don't think this is related to the patch.
> The API emac_update_hardware_stats() updates all the stats supported by
> ICSSG not just standard stats.

Yes, I'm saying you should send a separate fix, not really related or
blocking this patch (unless they conflict)

> > I hope the regmap doesn't sleep. cat /proc/net/dev to test.
> > You probably need to send some fixes to net.  
> 
> I checked cat /proc/net/dev and the stats shown there are not related to
> the stats I am introducing in this series.

You misunderstood. I pointed that you so you can check on a debug
kernel if there are any warnings (e.g. something tries to sleep
since /proc/net/dev read is under RCU lock).

> The fix could be to add a lock in this function, but we have close to 90
> stats and this function is called not only from icssg_ndo_get_stats64()
> but from emac_get_ethtool_stats(). The function also gets called
> periodically (Every 25 Seconds for 1G Link). I think every time locking
> 90 regmap_reads() could result in performance degradation.

Correctness comes first.