Thanks to Jakub, Michal and Jarod for the time and consideration. I
have been caught up with other stuff, and not spent much time on this
since.
I am not that familiar with the upper/lower sync logic and
implications. But I agree with Michal, it may not be necessary to
have a blanket exclusion of certain type of devices in those sync
functions.
I was thinking something maybe in vlan_dev.c:
static int vlan_dev_init(struct net_device *dev)
{
struct net_device *real_dev = vlan_dev_priv(dev)->real_dev;
netif_carrier_off(dev);
/* IFF_BROADCAST|IFF_MULTICAST; ??? */
dev->flags = real_dev->flags & ~(IFF_UP | IFF_PROMISC | IFF_ALLMULTI |
IFF_MASTER | IFF_SLAVE);
dev->state = (real_dev->state & ((1<<__LINK_STATE_NOCARRIER) |
(1<<__LINK_STATE_DORMANT))) |
(1<<__LINK_STATE_PRESENT);
dev->hw_features = NETIF_F_HW_CSUM | NETIF_F_SG |
NETIF_F_FRAGLIST | NETIF_F_GSO_SOFTWARE |
NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
NETIF_F_ALL_FCOE;
here, maybe check if real_dev's NETIF_F_UPPER_DISABLES features bits
are set, and add them to vlan_dev?
On Sun, Dec 6, 2020 at 11:49 AM Michal Kubecek <mkube...@suse.cz> wrote:
>
> On Sat, Dec 05, 2020 at 07:04:06PM -0500, Jarod Wilson wrote:
> > On Mon, Nov 23, 2020 at 7:27 PM Jakub Kicinski <k...@kernel.org> wrote:
> > >
> > > On Thu, 19 Nov 2020 20:37:27 -0500 Limin Wang wrote:
> > > > Under relatively recent kernels (v4.4+), creating a vlan subport on a
> > > > LRO supported parent NIC may turn LRO off on the parent port and
> > > > further render its LRO feature practically unchangeable.
> > >
> > > That does sound like an oversight in commit fd867d51f889 ("net/core:
> > > generic support for disabling netdev features down stack").
> > >
> > > Are you able to create a patch to fix this?
> >
> > Something like this, perhaps? Completely untested copy-pasta'd
> > theoretical patch:
> >
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index 8588ade790cb..a5ce372e02ba 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -9605,8 +9605,10 @@ int __netdev_update_features(struct net_device *dev)
> > features = netdev_fix_features(dev, features);
> >
> > /* some features can't be enabled if they're off on an upper device
> > */
> > - netdev_for_each_upper_dev_rcu(dev, upper, iter)
> > - features = netdev_sync_upper_features(dev, upper, features);
> > + netdev_for_each_upper_dev_rcu(dev, upper, iter) {
> > + if (netif_is_lag_master(upper) ||
> > netif_is_bridge_master(upper))
> > + features = netdev_sync_upper_features(dev,
> > upper, features);
> > + }
> >
> > if (dev->features == features)
> > goto sync_lower;
> > @@ -9633,8 +9635,10 @@ int __netdev_update_features(struct net_device *dev)
> > /* some features must be disabled on lower devices when disabled
> > * on an upper device (think: bonding master or bridge)
> > */
> > - netdev_for_each_lower_dev(dev, lower, iter)
> > - netdev_sync_lower_features(dev, lower, features);
> > + if (netif_is_lag_master(dev) || netif_is_bridge_master(dev)) {
> > + netdev_for_each_lower_dev(dev, lower, iter)
> > + netdev_sync_lower_features(dev, lower, features);
> > + }
> >
> > if (!err) {
> > netdev_features_t diff = features ^ dev->features;
> >
> > I'm not sure what all other upper devices this excludes besides just
> > vlan ports though, so perhaps safer add upper device types to not do
> > feature sync on than to choose which ones to do them on?
>
> I'm not sure excluding devices from feature sync is the right way,
> whether it's an explicit list types or default. The logic still makes
> sense to me. Couldn't we address the issue by either setting features in
> NETIF_F_UPPER_DISABLES) by default for a new vlan (and probably macvlan)
> device? Or perhaps inheriting their values from the lower device.
>
> Michal
