Re: [PATCH net-next v2 1/4] vlan: Add MACsec offload operations for VLAN interface

[Leon Romanovsky <leon@kernel.org>]

On Thu, Mar 30, 2023 at 07:19:21PM +0200, Sabrina Dubroca wrote:
> 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@nvidia.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). 

Sorry, I'm new to this netdev features zoo, but if I read correctly 
Documentation/networking/netdev-features.rst, the ->features is the list
of enabled ones:

   29  2. netdev->features set contains features which are currently enabled
   30     for a device.  This should be changed only by network core or in
   31     error paths of ndo_set_features callback.

And user will have a chance to disable it for VLAN because it was added
to ->hw_features:

   24  1. netdev->hw_features set contains features whose state may possibly
   25     be changed (enabled or disabled) for a particular device by user's
   26     request.  This set should be initialized in ndo_init callback and not
   27     changed later.

So how can VLAN be created with NETIF_F_HW_MACSEC while real_dev mcasec
offload is disabled?

> AFAIU, vlandev->hw_features should be based on realdev->vlan_features. 

Is this macsec offloaded VLAN can be called "child VLAN device"?

   33  3. netdev->vlan_features set contains features whose state is inherited
   34     by child VLAN devices (limits netdev->features set).  This is currently
   35     used for all VLAN devices whether tags are stripped or inserted in
   36     hardware or software.

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