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

[Jakub Kicinski <kuba@kernel.org>]

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.