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

["Abdul Rahim, Faizal" <faizal.abdul.rahim@linux.intel.com>]

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@localhost:~$ 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@localhost:~$ 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@localhost:~$ 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.