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

[Sabrina Dubroca <sd@queasysnail.net>]

2023-03-30, 21:56:56 +0300, Leon Romanovsky wrote:
> 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?

I'm proposing that be VLAN device be created with the capability
(->hw_features contains NETIF_F_HW_MACSEC) but disabled (->features
doesn't contain NETIF_F_HW_MACSEC). That way, if NETIF_F_HW_MACSEC is
re-enabled on the lower device, you don't need to destroy the VLAN
device to enable macsec offload on it as well. You still won't be able
to enable macsec offload on the VLAN device unless it's active on the
real NIC.

I think whether the lower device currently has NETIF_F_HW_MACSEC
should only affect whether you can enable the feature on the vlan
device right now. What feature is enabled at creation time should be
irrelevant.

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

Yes. In this patch, we're talking about this situation:

          eth0  --------------> vlan0 --------------> macsec0
       real NIC
     (capable of               (can also
   offloading MACsec)        offload MACsec)

And vlan0 would be a "child VLAN device" of eth0.

"limits netdev->features set" is the netdev_intersect_features you
quoted in your previous email.

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

-- 
Sabrina