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

[MD Danish Anwar <danishanwar@ti.com>]

Hi Jakub,

On 05/03/25 5:54 am, Jakub Kicinski wrote:
> 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.
> 

Sure. I will not report it via `ethtool -S`. This will only be reported
via ethtool_get_mm_stats. I will do something similar to
`icssg_miig_stats`. Have a boolean to indicate whether the stat is
getting reported by some standard interface or not.

> Please leave the stats for unimplemented features out.
> 

Sure. For now I will remove the FW_PREEMPT stats since the feature is
not implemented yet. Once the implementation is done, I will add the
stats to icssg_all_pa_stats() and read it via
emac_update_hardware_stats() and report it via ethtool_get_mm_stats.

>>>> +/* 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.

This looks to be a good choice. I will do the changes and send out a v2.
Thanks for the review.

-- 
Thanks and Regards,
Danish