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: <c9a5a458-6015-442f-988d-c4b830dabd01@linux.intel.com>
Date: Mon, 23 Dec 2024 17:39:55 +0800
From: "Abdul Rahim, Faizal" <faizal.abdul.rahim@...ux.intel.com>
To: Vladimir Oltean <olteanv@...il.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 17/12/2024 8:35 am, Vladimir Oltean wrote:
> 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 :)
> 

Actually ... it was so high 252 ... because I mistakenly copied the result 
from my past openlldp test that did:			
sudo lldptool -T -i enp1s0 -V addEthCaps addFragSize=3
Which sets is to 252 ..sorry causing confusion

Without OpenLLDP, with just ethtool and with default tx min frag size, it 
will look like:			
user@...alhost:~$ sudo ethtool --show-mm enp1s0
MAC Merge layer state for enp1s0:
pMAC enabled: off
TX enabled: off
TX active: off
TX minimum fragment size: 68
RX minimum fragment size: 68
Verify enabled: off
Verify time: 10
Max verify time: 128
Verification status: DISABLED

When verify handshake is done with OpenLLDP, it will look like:
user@...alhost:~$ sudo lldptool -t -i enp1s0 -V addEthCaps
Additional Ethernet Capabilities TLV
         Preemption capability supported
         Preemption capability enabled
         Preemption capability active
         Additional fragment size: 1 (124 octets)

user@...alhost:~$ sudo ethtool --show-mm enp1s0
MAC Merge layer state for enp1s0:
pMAC enabled: on
TX enabled: on
TX active: on
TX minimum fragment size: 124
RX minimum fragment size: 124
Verify enabled: on
Verify time: 128
Max verify time: 128
Verification status: SUCCEEDED

Which makes sense, due to the rounding up 68 to the closest upper multiple 
of addFragSize which is 124 octet in OpenLLDP, as what you mentioned.

>> 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.
> 

My bad.
I got your point, it's clearly explained, thanks :).
Just got to know i226 is able to handle any frag size for RX.
Since standard for min TX is 60, I'll use 60 then.