[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230330185656.GZ831478@unreal>
Date: Thu, 30 Mar 2023 21:56:56 +0300
From: Leon Romanovsky <leon@...nel.org>
To: Sabrina Dubroca <sd@...asysnail.net>
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
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@...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).
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
>
Powered by blists - more mailing lists