Re: [PATCHv6 net-next 1/4] net: add a common function to compute features for upper devices

On 10/20/25 11:10 AM, Sabrina Dubroca wrote:

2025-10-17, 03:41:52 +0000, Hangbin Liu wrote:

quoted

Some high level software drivers need to compute features from lower
devices. But each has their own implementations and may lost some
feature compute. Let's use one common function to compute features
for kinds of these devices.

The new helper uses the current bond implementation as the reference
one, as the latter already handles all the relevant aspects: netdev
features, TSO limits and dst retention.

Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Hangbin Liu <redacted>

No objection to this patch/series, just a nit and some discussion below, so:

Reviewed-by: Sabrina Dubroca <sd@queasysnail.net>


[...]

quoted

+/**
+ *	netdev_compute_master_upper_features - compute feature from lowers

nit: I'm slightly annoyed (that's not quite the right word, sorry)
that we're adding a new function to "compute features" that doesn't
touch netdev->features, but I can't come up with a better name
(the best I got was "compute extra features" and it doesn't help).

I'm not the right person to ask a good name, and I'm ok with the current
one, but since the question is pending... what about:

netdev_{compute,update}_offloads_from_lower()

?

As it actually updates (some of) the offloads available to the (upper)
device?

quoted

+ *	@dev: the upper device
+ *	@update_header: whether to update upper device's header_len/headroom/tailroom
+ *
+ *	Recompute the upper device's feature based on all lower devices.
+ */
+void netdev_compute_master_upper_features(struct net_device *dev, bool update_header)
+{

[...]

quoted

+	netif_set_tso_max_segs(dev, tso_max_segs);
+	netif_set_tso_max_size(dev, tso_max_size);
+
+	netdev_change_features(dev);

Maybe a dumb idea: I'm wondering if we're doing this from the wrong
side.

Right now we have:

[some device op] -> [this new function] -> netdev_change_features -> __netdev_update_features -> ndo_fix_features

Would it make more sense to go instead:

[some device op] -> netdev_change_features -> __netdev_update_features -> ndo_fix_features -> [this new function]

?

Uhmmm.... this function touches a few more things beyond dev->*features,
calling it from ndo_fix_features() looks a bit out-of-scope.

/P