Re: [PATCH iwl-next v4 9/9] igc: Add support to get frame preemption statistics via ethtool

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Mon, Feb 10, 2025 at 02:02:07AM -0500, Faizal Rahim wrote:
> Implemented "ethtool --include-statistics --show-mm" callback for IGC.
> 
> Tested preemption scenario to check preemption statistics:
> 1) Trigger verification handshake on both boards:
>     $ sudo ethtool --set-mm enp1s0 pmac-enabled on
>     $ sudo ethtool --set-mm enp1s0 tx-enabled on
>     $ sudo ethtool --set-mm enp1s0 verify-enabled on

For the record, all these can be enabled at the same time through a
single ethtool command.

> 2) Set preemptible or express queue in taprio for tx board:
>     $ sudo tc qdisc replace dev enp1s0 parent root handle 100 taprio \
>       num_tc 4 map 3 2 1 0 3 3 3 3 3 3 3 3 3 3 3 3 \
>       queues 1@0 1@1 1@2 1@3 base-time 0 sched-entry S F 100000 \
>       fp E E P P
> 3) Send large size packets on preemptible queue
> 4) Send small size packets on express queue to preempt packets in
>    preemptible queue
> 5) Show preemption statistics on the receiving board:
>    $ ethtool --include-statistics --show-mm enp1s0
>      MAC Merge layer state for enp1s0:
>      pMAC enabled: on
>      TX enabled: on
>      TX active: on
>      TX minimum fragment size: 64
>      RX minimum fragment size: 60
>      Verify enabled: on
>      Verify time: 128
>      Max verify time: 128
>      Verification status: SUCCEEDED
>      Statistics:
>      	MACMergeFrameAssErrorCount: 0
> 	MACMergeFrameSmdErrorCount: 0
> 	MACMergeFrameAssOkCount: 511
> 	MACMergeFragCountRx: 764
> 	MACMergeFragCountTx: 0
> 	MACMergeHoldCount: 0

nitpick: mix of tabs and spaces.

> diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
> index f15ac7565fbd..cd5160315993 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -3076,6 +3076,7 @@ static bool igc_clean_tx_irq(struct igc_q_vector *q_vector, int napi_budget)
>  			break;
>  
>  		if (static_branch_unlikely(&igc_fpe_enabled) &&
> +		    adapter->fpe.mmsv.pmac_enabled &&

This bit is misplaced in this patch.
Also, both conditions, being repeated twice as they are, could be placed
in an igc_pmac_enabled() helper function or similar.

>  		    igc_fpe_transmitted_smd_v(tx_desc))
>  			ethtool_mmsv_event_handle(&adapter->fpe.mmsv,
>  						  ETHTOOL_MMSV_LD_SENT_VERIFY_MPACKET);