netdev - Re: [PATCH iwl-next 5/9] igc: Add support to set 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] [day] [month] [year] [list]

Message-ID: <20241223214343.dbmhbj7cr7pfbeub@skbuf>
Date: Mon, 23 Dec 2024 23:43:43 +0200
From: Vladimir Oltean <olteanv@...il.com>
To: "Abdul Rahim, Faizal" <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 5/9] igc: Add support to set MAC Merge data via
 ethtool

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.