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

[Sabrina Dubroca <sd@queasysnail.net>]

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