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

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: <20241217003501.ar3nk6utdjllqjbk@skbuf>
Date: Tue, 17 Dec 2024 02:35:01 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: Faizal Rahim <faizal.abdul.rahim@...ux.intel.com>
Cc: Tony Nguyen <anthony.l.nguyen@...el.com>,
	Przemek Kitszel <przemyslaw.kitszel@...el.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	Jesper Dangaard Brouer <hawk@...nel.org>,
	John Fastabend <john.fastabend@...il.com>,
	Vinicius Costa Gomes <vinicius.gomes@...el.com>,
	intel-wired-lan@...ts.osuosl.org, netdev@...r.kernel.org,
	linux-kernel@...r.kernel.org, bpf@...r.kernel.org
Subject: Re: [PATCH iwl-next 8/9] igc: Add support to get MAC Merge data via
 ethtool

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@...ux.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;
> +}