-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Monday, 27 March 2023 19:44
To: Emeel Hakim <redacted>
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:

quoted

@@ -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);

quoted

      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.

quoted

@@ -803,6 +810,49 @@ static int vlan_dev_fill_forward_path(struct

net_device_path_ctx *ctx,

quoted

      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.