Re: [PATCH iwl-next 8/9] igc: Add support to get MAC Merge data via ethtool

[Vladimir Oltean <olteanv@gmail.com>]

On Mon, Dec 16, 2024 at 01:47:19AM -0500, Faizal Rahim wrote:
> Implement "ethtool --show-mm" callback for IGC.
> 
> Tested with command:
> $ ethtool --show-mm enp1s0.
>   MAC Merge layer state for enp1s0:
>   pMAC enabled: on
>   TX enabled: on
>   TX active: on
>   TX minimum fragment size: 252
>   RX minimum fragment size: 252

I'm going to ask "why so high?" and then I'm going to answer that I
suspect this is a positive feedback loop created by openlldp, because of
the driver incorrectly reporting:

- 60 as 68, ..., 252 as 260, and openlldp always (correctly) rounding up
  these non-standard values to the closest upper multiple of an
  addFragSize, which is all that can be advertised over LLDP
- on RX what was configured on TX (see below), which in turn makes the
  link partner again want to readjust (increase) its TX, to satisfy the
  new RX requirement

But I'm open to hearing the correct answer, coming from you :)

>   Verify enabled: on
>   Verify time: 128
>   Max verify time: 128
>   Verification status: SUCCEEDED
> 
> Verified that the fields value are retrieved correctly.
> 
> Signed-off-by: Faizal Rahim <faizal.abdul.rahim@linux.intel.com>
> ---
> diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> index 7cde0e5a7320..16aa6e4e1727 100644
> --- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
> +++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
> @@ -1782,6 +1782,25 @@ static int igc_ethtool_set_eee(struct net_device *netdev,
>  	return 0;
>  }
>  
> +static int igc_ethtool_get_mm(struct net_device *netdev,
> +			      struct ethtool_mm_state *cmd)
> +{
> +	struct igc_adapter *adapter = netdev_priv(netdev);
> +	struct fpe_t *fpe = &adapter->fpe;
> +
> +	cmd->tx_min_frag_size = fpe->tx_min_frag_size;
> +	cmd->rx_min_frag_size = fpe->tx_min_frag_size;

This is most likely a mistake. rx_min_frag_size means what is the
smallest fragment size that the i225 can receive. Whereas tx_min_frag_size
means what minimum fragment size it is configured to transmit (based,
among others, on the link partner's minimum RX requirements).
To say that the i225's minimum RX fragment size depends on how small it
was configured to transmit seems wrong. I would expect a constant, or if
this is correct, an explanation. TI treats rx_min_frag_size != ETH_ZLEN
as errata.

> +	cmd->pmac_enabled = fpe->pmac_enabled;
> +	cmd->verify_enabled = fpe->verify_enabled;
> +	cmd->verify_time = fpe->verify_time;
> +	cmd->tx_active = igc_fpe_is_tx_preempt_allowed(&adapter->fpe);
> +	cmd->tx_enabled = fpe->tx_enabled;
> +	cmd->verify_status = igc_fpe_get_verify_status(&adapter->fpe);
> +	cmd->max_verify_time = MAX_VERIFY_TIME;
> +
> +	return 0;
> +}