Re: [PATCH net-next v5 2/7] net: ethtool: add tcp_data_split_mod member in kernel_ethtool_ringparam

[Jakub Kicinski <kuba@kernel.org>]

On Wed, 13 Nov 2024 17:32:16 +0000 Taehee Yoo wrote:
> When tcp-data-split is UNKNOWN mode, drivers arbitrarily handle it.
> For example, bnxt_en driver automatically enables if at least one of
> LRO/GRO/JUMBO is enabled.
> If tcp-data-split is UNKNOWN and LRO is enabled, a driver returns
> ENABLES of tcp-data-split, not UNKNOWN.
> So, `ethtool -g eth0` shows tcp-data-split is enabled.
> 
> The problem is in the setting situation.
> In the ethnl_set_rings(), it first calls get_ringparam() to get the
> current driver's config.
> At that moment, if driver's tcp-data-split config is UNKNOWN, it returns
> ENABLE if LRO/GRO/JUMBO is enabled.
> Then, it sets values from the user and driver's current config to
> kernel_ethtool_ringparam.
> Last it calls .set_ringparam().
> The driver, especially bnxt_en driver receives
> ETHTOOL_TCP_DATA_SPLIT_ENABLED.
> But it can't distinguish whether it is set by the user or just the
> current config.
> 
> The new tcp_data_split_mod member indicates the tcp-data-split value is
> explicitly set by the user.
> So the driver can handle ETHTOOL_TCP_DATA_SPLIT_ENABLED properly.

I think this can work, but it isn't exactly what I had in mind.

I was thinking we'd simply add u8 hds_config to 
struct ethtool_netdev_state (which is stored inside netdev).
And update it there if user request via ethnl_set_rings() succeeds.

That gives the driver and the core quick and easy access to checking if
the user forced the setting to ENABLED or DISABLED, or didn't (UNKNOWN).

As far as the parameter passed to ->set_ringparam() goes we could do
(assuming the new fields in ethtool_netdev state is called hds):

	kernel_ringparam.tcp_data_split = 
		nla_get_u32_default(tb[ETHTOOL_A_RINGS_TCP_DATA_SPLIT],
				    dev->ethtool->hds);

If the driver see UNKNOWN it means user doesn't care.
If the driver sees ENABLED/DISABLE it must comply, doesn't matter if
the user requested it in current netlink call, or previous and hasn't
reset it, yet.

Hope this makes sense...