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

[Vladimir Oltean <olteanv@gmail.com>]

Hi Faizal,

On Mon, Dec 23, 2024 at 05:23:27PM +0800, Abdul Rahim, Faizal wrote:
> To recap:
> 
> Standard range: 60, 124, 188, 252 (without mCRC).
> i226 range: 64, 128, 192, 256 (without mCRC).
> 
> The current IGC_TX_MIN_FRAG_SIZE is incorrectly set to 68 due to our
> misinterpretation of the i226 documentation:
> "The minimum size for non-final preempted fragments is 64 * (1 + MIN_FRAG) +
> 4 (mCRC)."
> 
> The calculation above is for the fragment size on the wire, including mCRC.
> For the TX preemption point and pure fragment size, mCRC should not be
> included, as confirmed by the hardware owner.
> 
> On RX, i226 can handle any size, even the standard minimum of 60 octets
> (without mCRC).
> 
> What would be ideal for i226:
> Min frag user set 60:64 → Multiplier = 0.
> Min frag user set 65:128 → Multiplier = 1.
> (And so on)
> 
> To make this work and reuse the existing code, we’d need to tweak these two
> functions:
> ethtool_mm_frag_size_add_to_min(val_min, xxx)
> ethtool_mm_frag_size_min_to_add(xx)
> 
> With the current code, if I pass 64 octets as val_min to
> ethtool_mm_frag_size_add_to_min(), it returns error.
> 
> Proposed modification:
> Add a new parameter to ethtool_mm_frag_size_min_to_add() - maybe let's call
> it dev_min_tx_frag_len.
> 
> Set dev_min_tx_frag_len = 64 for i226, 60 for other drivers.
> This field will be used to:
> (1) modify the calculation in ethtool_mm_frag_size_min_to_add()
> (2) as a warning prompt to user when the value is not standard, done in
> ethtool_mm_frag_size_add_to_min()
> 
> I was thinking (1) would modify the existing:
> u32 ethtool_mm_frag_size_add_to_min(u32 val_add)
> {
> 	return (ETH_ZLEN + ETH_FCS_LEN) * (1 + val_add) - ETH_FCS_LEN;
> }
> 
> To something like:
> u32 ethtool_mm_frag_size_add_to_min(u32 val_add, u32 dev_min_tx_frag_len)
> {
>     return dev_min_tx_frag_len + (val_add * 64);
> }
> 
> So this will yield:
> Standard range (dev_min_tx_frag_len = 60): 60, 124, 188, 252
> i226 range (dev_min_tx_frag_len = 64): 64, 128, 192, 256
> 
> But what's not so nice is, the rest of other drivers have to set this new
> param when calling ethtool_mm_frag_size_add_to_min().
> 
> Is something like this okay ? I'm open to better suggestion.

I'm taking a break probably for the rest of the year, and spending time
for the Christmas holidays mostly off lists.

I didn't look through all your replies. Just regarding the one quoted
above: just don't use the ethtool_mm_frag_size_add_to_min() and
ethtool_mm_frag_size_min_to_add() helpers if they aren't useful as-is.
They are designed for a standardized NIC implementation. They are opt-in
from driver code for a reason.