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

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

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>


[...]

+/**
+ *	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).

+ *	@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)
+{

[...]

+	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]

?


Possible benefit: not forgetting to fix up the "extra" features in
some cases?  (ie calling netdev_change_features when we should have
called netdev_compute_master_upper_features)

+}

-- 
Sabrina