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

[Jakub Kicinski <kuba@kernel.org>]

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.