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

[Emeel Hakim <ehakim@nvidia.com>]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Monday, 27 March 2023 19:44
> To: Emeel Hakim <ehakim@nvidia.com>
> Cc: davem@davemloft.net; pabeni@redhat.com; edumazet@google.com;
> sd@queasysnail.net; netdev@vger.kernel.org
> Subject: Re: [PATCH net-next 1/4] vlan: Add MACsec offload operations for VLAN
> interface
> 
> External email: Use caution opening links or attachments
> 
> 
> On Sun, 26 Mar 2023 10:26:33 +0300 Emeel Hakim wrote:
> > @@ -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) @@ -660,6 +664,9 @@
> > static netdev_features_t vlan_dev_fix_features(struct net_device *dev,
> >       features |= old_features & (NETIF_F_SOFT_FEATURES |
> NETIF_F_GSO_SOFTWARE);
> >       features |= NETIF_F_LLTX;
> >
> > +     if (real_dev->features & NETIF_F_HW_MACSEC)
> > +             features |= NETIF_F_HW_MACSEC;
> > +
> >       return features;
> >  }
> 
> Shouldn't vlan_features be consulted somehow?

I did consider including the vlan_features, but after careful consideration, I couldn't see how they were relevant to the task at hand.
However, if you have any specific suggestions on how I could incorporate them to improve the code, I would be happy to hear them.

> > @@ -803,6 +810,49 @@ static int vlan_dev_fill_forward_path(struct
> net_device_path_ctx *ctx,
> >       return 0;
> >  }
> >
> > +#if IS_ENABLED(CONFIG_MACSEC)
> > +#define VLAN_MACSEC_MDO(mdo) \
> > +static int vlan_macsec_ ## mdo(struct macsec_context *ctx) \ { \
> > +     const struct macsec_ops *ops; \
> > +     ops =  vlan_dev_priv(ctx->netdev)->real_dev->macsec_ops; \
> > +     return ops ? ops->mdo_ ## mdo(ctx) : -EOPNOTSUPP; \ }
> > +
> > +#define VLAN_MACSEC_DECLARE_MDO(mdo) vlan_macsec_ ## mdo
> > +
> > +VLAN_MACSEC_MDO(add_txsa);
> > +VLAN_MACSEC_MDO(upd_txsa);
> > +VLAN_MACSEC_MDO(del_txsa);
> > +
> > +VLAN_MACSEC_MDO(add_rxsa);
> > +VLAN_MACSEC_MDO(upd_rxsa);
> > +VLAN_MACSEC_MDO(del_rxsa);
> > +
> > +VLAN_MACSEC_MDO(add_rxsc);
> > +VLAN_MACSEC_MDO(upd_rxsc);
> > +VLAN_MACSEC_MDO(del_rxsc);
> > +
> > +VLAN_MACSEC_MDO(add_secy);
> > +VLAN_MACSEC_MDO(upd_secy);
> > +VLAN_MACSEC_MDO(del_secy);
> 
> -1
> 
> impossible to grep for the functions :( but maybe others don't care

Thank you for bringing up the issue you noticed. However, I decided to go with this approach
because the functions are simple and look very similar, so there wasn't much to debug.
Using a macro allowed for cleaner code instead of having to resort to ugly code duplication.