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 <[email protected]> > Signed-off-by: Hangbin Liu <[email protected]>
No objection to this patch/series, just a nit and some discussion below, so: Reviewed-by: Sabrina Dubroca <[email protected]> [...] > +/** > + * 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
