[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZCXEmUQgswOBoRqR@hog>
Date: Thu, 30 Mar 2023 19:19:21 +0200
From: Sabrina Dubroca <sd@...asysnail.net>
To: Leon Romanovsky <leon@...nel.org>
Cc: Emeel Hakim <ehakim@...dia.com>, davem@...emloft.net,
kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com,
netdev@...r.kernel.org
Subject: Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for
VLAN interface
2023-03-29, 21:42:01 +0300, Leon Romanovsky wrote:
> On Wed, Mar 29, 2023 at 04:43:59PM +0200, Sabrina Dubroca wrote:
> > 2023-03-29, 15:21:04 +0300, Emeel Hakim wrote:
> > > Add support for MACsec offload operations for VLAN driver
> > > to allow offloading MACsec when VLAN's real device supports
> > > Macsec offload by forwarding the offload request to it.
> > >
> > > Signed-off-by: Emeel Hakim <ehakim@...dia.com>
> > > ---
> > > V1 -> V2: - Consult vlan_features when adding NETIF_F_HW_MACSEC.
> >
> > Uh? You're not actually doing that? You also dropped the
> > changes to vlan_dev_fix_features without explaining why.
>
> vlan_dev_fix_features() relies on real_dev->vlan_features which was set
> in mlx5 part of this patch.
>
> 643 static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
> 644 netdev_features_t features)
> 645 {
> ...
> 649
> 650 lower_features = netdev_intersect_features((real_dev->vlan_features |
> 651 NETIF_F_RXCSUM),
> 652 real_dev->features);
>
> This part ensure that once real_dev->vlan_features and real_dev->features have NETIF_F_HW_MACSEC,
> the returned features will include NETIF_F_HW_MACSEC too.
Ok, thanks.
But back to the issue of vlan_features, in vlan_dev_init: I'm not
convinced NETIF_F_HW_MACSEC should be added to hw_features based on
->features. That would result in a new vlan device that can't offload
macsec at all if it was created at the wrong time (while the lower
device's macsec offload was temporarily disabled). AFAIU,
vlandev->hw_features should be based on realdev->vlan_features. I
don't see a reason to advertise a feature in the vlan device if we
won't ever be able to turn it on because it's not in ->vlan_features
("grmbl why can't I enable it, ethtool says it's here?!").
Emeel, I'm not a maintainer, but I don't think you should be reposting
until the existing discussion has settled down.
> >
> > [...]
> > > @@ -572,6 +573,9 @@ static int vlan_dev_init(struct net_device *dev)
> > > NETIF_F_HIGHDMA | NETIF_F_SCTP_CRC |
> > > NETIF_F_ALL_FCOE;
> > >
> > > + if (real_dev->features & NETIF_F_HW_MACSEC)
> > > + dev->hw_features |= NETIF_F_HW_MACSEC;
> > > +
> > > dev->features |= dev->hw_features | NETIF_F_LLTX;
> > > netif_inherit_tso_max(dev, real_dev);
> > > if (dev->features & NETIF_F_VLAN_FEATURES)
--
Sabrina
Powered by blists - more mailing lists