linux-kernel - Re: [PATCH net-next] 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <20250304162437.0160f687@kernel.org>
Date: Tue, 4 Mar 2025 16:24:37 -0800
From: Jakub Kicinski <kuba@...nel.org>
To: MD Danish Anwar <danishanwar@...com>
Cc: Meghana Malladi <m-malladi@...com>, Diogo Ivo <diogo.ivo@...mens.com>,
 Paolo Abeni <pabeni@...hat.com>, Eric Dumazet <edumazet@...gle.com>, "David
 S. Miller" <davem@...emloft.net>, Andrew Lunn <andrew+netdev@...n.ch>,
 <linux-kernel@...r.kernel.org>, <netdev@...r.kernel.org>,
 <linux-arm-kernel@...ts.infradead.org>, <srk@...com>, Vignesh Raghavendra
 <vigneshr@...com>, Roger Quadros <rogerq@...nel.org>
Subject: Re: [PATCH net-next] net: ti: icssg-prueth: Add ICSSG FW Stats

On Tue, 4 Mar 2025 13:46:39 +0530 MD Danish Anwar wrote:
> On 04/03/25 6:55 am, Jakub Kicinski wrote:
> > On Thu, 27 Feb 2025 15:07:12 +0530 MD Danish Anwar wrote:  
> >> +	ICSSG_PA_STATS(FW_PREEMPT_BAD_FRAG),
> >> +	ICSSG_PA_STATS(FW_PREEMPT_ASSEMBLY_ERR),
> >> +	ICSSG_PA_STATS(FW_PREEMPT_FRAG_CNT_TX),
> >> +	ICSSG_PA_STATS(FW_PREEMPT_ASSEMBLY_OK),
> >> +	ICSSG_PA_STATS(FW_PREEMPT_FRAG_CNT_RX),  
> > 
> > I presume frame preemption is implemented in silicon? If yes -
> > what makes these "FW statistics"? Does the FW collect them from   
> 
> The statistics are maintained / updated by firmware and thus the name.
> 
> Preemption is implemented partially in both the hardware and firmware.
> The STATE MACHINE for preemption is in the firmware. The decision to
> when to PREEMEPT / ASSEMBLE a packet is made in firmware.
> 
> These preemption statistics are updated by the firmware based on the
> action performed by the firmware. Driver can read these to know the
> statistics of preemption. These stats will be able used by
> ethtool_mm_stats once the support for Preemption is added in the driver.

That was going to be my next question. If the statistic is suitable 
for a standard interface it should not be reported via ethtool -S.

Please leave the stats for unimplemented features out.

> >> +/* Incremented if a packet is dropped at PRU because of a rule violation */
> >> +#define FW_DROPPED_PKT		0x00F8  
> > 
> > Instead of adding comments here please add a file under
> > Documentation/networking/device_drivers/ with the explanations.
> > That's far more likely to be discovered by users, no?  
> 
> Sure I will drop these MACRO comments and create a .rst file in
> Documentation/networking/device_drivers/
> 
> One question though, should I create a table for the stats and it's
> description or should I create a section for each stats?
> 
> Something like this,
> 
> FW_RTU_PKT_DROP
> ---------------

Let's document the user-visible names! The strings from ethtool -S

> Diagnostic error counter which increments when RTU drops a locally
> injected packet due to port being disabled or rule violation.
> 
> Please let me know what do you think.

Taking inspiration from:
  Documentation/networking/device_drivers/ethernet/meta/fbnic.rst
should be a safe choice, I hope.