[PATCH net-next 0/4] Support MACsec VLAN

[PATCH net-next 0/4] Support MACsec VLAN

[Emeel Hakim]

Dear maintainers,

This patch series introduces support for hardware (HW) offload MACsec
devices with VLAN configuration. The patches address both scenarios
where the VLAN header is both the inner and outer header for MACsec.

The changes include:

1. Adding MACsec offload operation for VLAN.
2. Considering VLAN when accessing MACsec net device.
3. Currently offloading MACsec when it's configured over VLAN with
current MACsec TX steering rules would wrongly insert the MACsec sec tag
after inserting the VLAN header. This resulted in an ETHERNET | SECTAG |
VLAN packet when ETHERNET | VLAN | SECTAG is configured. The patche
handles this issue when configuring steering rules.
4. Adding MACsec rx_handler change support in case of a marked skb and a
mismatch on the dst MAC address.

Please review these changes and let me know if you have any feedback or
concerns.

Thanks,
Emeel

Emeel Hakim (4):
  vlan: Add MACsec offload operations for VLAN interface
  net/mlx5: Support MACsec over VLAN
  net/mlx5: Consider VLAN interface in MACsec TX steering rules
  macsec: Add MACsec rx_handler change support

 .../mellanox/mlx5/core/en_accel/macsec.c      | 42 +++++++++------
 .../mellanox/mlx5/core/en_accel/macsec_fs.c   |  7 +++
 drivers/net/macsec.c                          |  9 ++++
 net/8021q/vlan_dev.c                          | 54 ++++++++++++++++++-
 4 files changed, 95 insertions(+), 17 deletions(-)

-- 
2.21.3

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

[Emeel Hakim]

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>
---
 net/8021q/vlan_dev.c | 54 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
index 5920544e93e8..7cf1f15340d7 100644
--- a/net/8021q/vlan_dev.c
+++ b/net/8021q/vlan_dev.c
@@ -26,6 +26,7 @@
 #include <linux/ethtool.h>
 #include <linux/phy.h>
 #include <net/arp.h>
+#include <net/macsec.h>
 
 #include "vlan.h"
 #include "vlanproc.h"
@@ -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;
 }
 
@@ -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);
+
+static const struct macsec_ops macsec_offload_ops = {
+	.mdo_add_txsa = VLAN_MACSEC_DECLARE_MDO(add_txsa),
+	.mdo_upd_txsa = VLAN_MACSEC_DECLARE_MDO(upd_txsa),
+	.mdo_del_txsa = VLAN_MACSEC_DECLARE_MDO(del_txsa),
+	.mdo_add_rxsc = VLAN_MACSEC_DECLARE_MDO(add_rxsc),
+	.mdo_upd_rxsc = VLAN_MACSEC_DECLARE_MDO(upd_rxsc),
+	.mdo_del_rxsc = VLAN_MACSEC_DECLARE_MDO(del_rxsc),
+	.mdo_add_rxsa = VLAN_MACSEC_DECLARE_MDO(add_rxsa),
+	.mdo_upd_rxsa = VLAN_MACSEC_DECLARE_MDO(upd_rxsa),
+	.mdo_del_rxsa = VLAN_MACSEC_DECLARE_MDO(del_rxsa),
+	.mdo_add_secy = VLAN_MACSEC_DECLARE_MDO(add_secy),
+	.mdo_upd_secy = VLAN_MACSEC_DECLARE_MDO(upd_secy),
+	.mdo_del_secy = VLAN_MACSEC_DECLARE_MDO(del_secy),
+};
+#endif
+
 static const struct ethtool_ops vlan_ethtool_ops = {
 	.get_link_ksettings	= vlan_ethtool_get_link_ksettings,
 	.get_drvinfo	        = vlan_ethtool_get_drvinfo,
@@ -868,7 +918,9 @@ void vlan_setup(struct net_device *dev)
 	dev->needs_free_netdev	= true;
 	dev->priv_destructor	= vlan_dev_free;
 	dev->ethtool_ops	= &vlan_ethtool_ops;
-
+#if IS_ENABLED(CONFIG_MACSEC)
+	dev->macsec_ops		= &macsec_offload_ops;
+#endif
 	dev->min_mtu		= 0;
 	dev->max_mtu		= ETH_MAX_MTU;
 
-- 
2.21.3

[PATCH net-next 2/4] net/mlx5: Support MACsec over VLAN

[Emeel Hakim]

MACsec device may have a VLAN device on top of it.
Detect MACsec state correctly under this condition,
and return the correct net device accordingly.

Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
---
 .../mellanox/mlx5/core/en_accel/macsec.c      | 42 ++++++++++++-------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
index 33b3620ea45c..f1646fa6737d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -4,6 +4,7 @@
 #include <linux/mlx5/device.h>
 #include <linux/mlx5/mlx5_ifc.h>
 #include <linux/xarray.h>
+#include <linux/if_vlan.h>
 
 #include "en.h"
 #include "lib/aso.h"
@@ -348,12 +349,21 @@ static void mlx5e_macsec_cleanup_sa(struct mlx5e_macsec *macsec,
 	sa->macsec_rule = NULL;
 }
 
+static inline struct mlx5e_priv *macsec_netdev_priv(const struct net_device *dev)
+{
+#if defined(CONFIG_VLAN_8021Q) || defined(CONFIG_VLAN_8021Q_MODULE)
+	if (is_vlan_dev(dev))
+		return netdev_priv(vlan_dev_priv(dev)->real_dev);
+#endif
+	return netdev_priv(dev);
+}
+
 static int mlx5e_macsec_init_sa(struct macsec_context *ctx,
 				struct mlx5e_macsec_sa *sa,
 				bool encrypt,
 				bool is_tx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec *macsec = priv->macsec;
 	struct mlx5_macsec_rule_attrs rule_attrs;
 	struct mlx5_core_dev *mdev = priv->mdev;
@@ -427,7 +437,7 @@ static int macsec_rx_sa_active_update(struct macsec_context *ctx,
 				      struct mlx5e_macsec_sa *rx_sa,
 				      bool active)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec *macsec = priv->macsec;
 	int err = 0;
 
@@ -510,7 +520,7 @@ static int mlx5e_macsec_add_txsa(struct macsec_context *ctx)
 {
 	const struct macsec_tx_sc *tx_sc = &ctx->secy->tx_sc;
 	const struct macsec_tx_sa *ctx_tx_sa = ctx->sa.tx_sa;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_secy *secy = ctx->secy;
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5_core_dev *mdev = priv->mdev;
@@ -585,7 +595,7 @@ static int mlx5e_macsec_upd_txsa(struct macsec_context *ctx)
 {
 	const struct macsec_tx_sc *tx_sc = &ctx->secy->tx_sc;
 	const struct macsec_tx_sa *ctx_tx_sa = ctx->sa.tx_sa;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	u8 assoc_num = ctx->sa.assoc_num;
 	struct mlx5e_macsec_sa *tx_sa;
@@ -645,7 +655,7 @@ static int mlx5e_macsec_upd_txsa(struct macsec_context *ctx)
 
 static int mlx5e_macsec_del_txsa(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	u8 assoc_num = ctx->sa.assoc_num;
 	struct mlx5e_macsec_sa *tx_sa;
@@ -696,7 +706,7 @@ static u32 mlx5e_macsec_get_sa_from_hashtable(struct rhashtable *sci_hash, sci_t
 static int mlx5e_macsec_add_rxsc(struct macsec_context *ctx)
 {
 	struct mlx5e_macsec_rx_sc_xarray_element *sc_xarray_element;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_rx_sc *ctx_rx_sc = ctx->rx_sc;
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_rx_sc *rx_sc;
@@ -776,7 +786,7 @@ static int mlx5e_macsec_add_rxsc(struct macsec_context *ctx)
 
 static int mlx5e_macsec_upd_rxsc(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_rx_sc *ctx_rx_sc = ctx->rx_sc;
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_rx_sc *rx_sc;
@@ -854,7 +864,7 @@ static void macsec_del_rxsc_ctx(struct mlx5e_macsec *macsec, struct mlx5e_macsec
 
 static int mlx5e_macsec_del_rxsc(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_rx_sc *rx_sc;
 	struct mlx5e_macsec *macsec;
@@ -890,8 +900,8 @@ static int mlx5e_macsec_del_rxsc(struct macsec_context *ctx)
 
 static int mlx5e_macsec_add_rxsa(struct macsec_context *ctx)
 {
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_rx_sa *ctx_rx_sa = ctx->sa.rx_sa;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5_core_dev *mdev = priv->mdev;
 	u8 assoc_num = ctx->sa.assoc_num;
@@ -976,8 +986,8 @@ static int mlx5e_macsec_add_rxsa(struct macsec_context *ctx)
 
 static int mlx5e_macsec_upd_rxsa(struct macsec_context *ctx)
 {
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_rx_sa *ctx_rx_sa = ctx->sa.rx_sa;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	u8 assoc_num = ctx->sa.assoc_num;
 	struct mlx5e_macsec_rx_sc *rx_sc;
@@ -1033,7 +1043,7 @@ static int mlx5e_macsec_upd_rxsa(struct macsec_context *ctx)
 
 static int mlx5e_macsec_del_rxsa(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	sci_t sci = ctx->sa.rx_sa->sc->sci;
 	struct mlx5e_macsec_rx_sc *rx_sc;
@@ -1085,7 +1095,7 @@ static int mlx5e_macsec_del_rxsa(struct macsec_context *ctx)
 
 static int mlx5e_macsec_add_secy(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct net_device *dev = ctx->secy->netdev;
 	const struct net_device *netdev = ctx->netdev;
 	struct mlx5e_macsec_device *macsec_device;
@@ -1137,7 +1147,7 @@ static int mlx5e_macsec_add_secy(struct macsec_context *ctx)
 static int macsec_upd_secy_hw_address(struct macsec_context *ctx,
 				      struct mlx5e_macsec_device *macsec_device)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct net_device *dev = ctx->secy->netdev;
 	struct mlx5e_macsec *macsec = priv->macsec;
 	struct mlx5e_macsec_rx_sc *rx_sc, *tmp;
@@ -1184,8 +1194,8 @@ static int macsec_upd_secy_hw_address(struct macsec_context *ctx,
  */
 static int mlx5e_macsec_upd_secy(struct macsec_context *ctx)
 {
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	const struct macsec_tx_sc *tx_sc = &ctx->secy->tx_sc;
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
 	const struct net_device *dev = ctx->secy->netdev;
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_sa *tx_sa;
@@ -1240,7 +1250,7 @@ static int mlx5e_macsec_upd_secy(struct macsec_context *ctx)
 
 static int mlx5e_macsec_del_secy(struct macsec_context *ctx)
 {
-	struct mlx5e_priv *priv = netdev_priv(ctx->netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(ctx->netdev);
 	struct mlx5e_macsec_device *macsec_device;
 	struct mlx5e_macsec_rx_sc *rx_sc, *tmp;
 	struct mlx5e_macsec_sa *tx_sa;
@@ -1741,7 +1751,7 @@ void mlx5e_macsec_offload_handle_rx_skb(struct net_device *netdev,
 {
 	struct mlx5e_macsec_rx_sc_xarray_element *sc_xarray_element;
 	u32 macsec_meta_data = be32_to_cpu(cqe->ft_metadata);
-	struct mlx5e_priv *priv = netdev_priv(netdev);
+	struct mlx5e_priv *priv = macsec_netdev_priv(netdev);
 	struct mlx5e_macsec_rx_sc *rx_sc;
 	struct mlx5e_macsec *macsec;
 	u32  fs_id;
-- 
2.21.3

[PATCH net-next 3/4] net/mlx5: Consider VLAN interface in MACsec TX steering rules

[Emeel Hakim]

Offloading MACsec when its configured over VLAN with current MACsec
TX steering rules will wrongly insert MACsec sec tag after inserting
the VLAN header leading to a ETHERNET | SECTAG | VLAN packet when
ETHERNET | VLAN | SECTAG is configured.

The above issue is due to adding the SECTAG by HW which is a later
stage compared to the VLAN header insertion stage.

Detect such a case and adjust TX steering rules to insert the
SECTAG in the correct place by using reformat_param_0 field in
the packet reformat to indicate the offset of SECTAG from end of
the MAC header to account for VLANs in granularity of 4Bytes.

Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
---
 .../net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c   | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
index 5b658a5588c6..daaaaf344f77 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec_fs.c
@@ -4,6 +4,7 @@
 #include <net/macsec.h>
 #include <linux/netdevice.h>
 #include <linux/mlx5/qp.h>
+#include <linux/if_vlan.h>
 #include "fs_core.h"
 #include "en/fs.h"
 #include "en_accel/macsec_fs.h"
@@ -510,6 +511,8 @@ static void macsec_fs_tx_del_rule(struct mlx5e_macsec_fs *macsec_fs,
 	macsec_fs_tx_ft_put(macsec_fs);
 }
 
+#define MLX5_REFORMAT_PARAM_ADD_MACSEC_OFFSET_4_BYTES 1
+
 static union mlx5e_macsec_rule *
 macsec_fs_tx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
 		      const struct macsec_context *macsec_ctx,
@@ -555,6 +558,10 @@ macsec_fs_tx_add_rule(struct mlx5e_macsec_fs *macsec_fs,
 	reformat_params.type = MLX5_REFORMAT_TYPE_ADD_MACSEC;
 	reformat_params.size = reformat_size;
 	reformat_params.data = reformatbf;
+
+	if (is_vlan_dev(macsec_ctx->netdev))
+		reformat_params.param_0 = MLX5_REFORMAT_PARAM_ADD_MACSEC_OFFSET_4_BYTES;
+
 	flow_act.pkt_reformat = mlx5_packet_reformat_alloc(macsec_fs->mdev,
 							   &reformat_params,
 							   MLX5_FLOW_NAMESPACE_EGRESS_MACSEC);
-- 
2.21.3

[PATCH net-next 4/4] macsec: Add MACsec rx_handler change support

[Emeel Hakim]

Offloading device drivers will mark offloaded MACsec SKBs with the
corresponding SCI in the skb_metadata_dst so the macsec rx handler will
know to which interface to divert those skbs, in case of a marked skb
and a mismatch on the dst MAC address, divert the skb to the macsec
net_device where the macsec rx_handler will be called.

Example of such a case is having a MACsec with VLAN as an inner header
ETHERNET | SECTAG | VLAN packet.

Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
---
 drivers/net/macsec.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 25616247d7a5..88b00ea4af68 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1048,6 +1048,15 @@ static enum rx_handler_result handle_not_macsec(struct sk_buff *skb)
 
 				__netif_rx(nskb);
 			}
+
+			if (md_dst && md_dst->type == METADATA_MACSEC &&
+			    (find_rx_sc(&macsec->secy, md_dst->u.macsec_info.sci))) {
+				skb->dev = ndev;
+				skb->pkt_type = PACKET_HOST;
+				ret = RX_HANDLER_ANOTHER;
+				goto out;
+			}
+
 			continue;
 		}
 
-- 
2.21.3

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

[Jakub Kicinski]

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?

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

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

[Emeel Hakim]

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

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

[Francois Romieu]

Emeel Hakim <ehakim@nvidia.com> :
> > -----Original Message-----
> > From: Jakub Kicinski <kuba@kernel.org>
[...]
> > > +#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.

Sometime it's also nice to be able to use such modern tools as tags and
grep.

While it still implies some duplication and it doesn't automagically
prevent wrongly mixing vlan_macsec_foo() and ops->mdo_bar(), the code
below may be considered as a trade-off:

#define _B(mdo) \
	const struct macsec_ops *ops; \
	ops = vlan_dev_priv(ctx->netdev)->real_dev->macsec_ops; \
	return ops ? ops->mdo_ ## mdo(ctx) : -EOPNOTSUPP; \


static int vlan_macsec_add_txsa(struct macsec_context *ctx) { _B(add_txsa) }
static int vlan_macsec_upd_txsa(struct macsec_context *ctx) { _B(upd_txsa) }
[...]
#undef _B

On a tangent topic, both codes expand 12 times the accessor

vlan_dev_priv(ctx->netdev)->real_dev->macsec_ops.

It imvho deserves an helper function so that the compiler can make some
choice.

As a final remark, VLAN_MACSEC_DECLARE_MDO above does not buy much.
Compare:

static const struct macsec_ops macsec_offload_ops = {
	.mdo_add_txsa = VLAN_MACSEC_DECLARE_MDO(add_txsa),
	.mdo_upd_txsa = VLAN_MACSEC_DECLARE_MDO(upd_txsa),
[...]

vs

static const struct macsec_ops macsec_offload_ops = {
	.mdo_add_txsa = vlan_macsec_add_txsa,
	.mdo_upd_txsa = vlan_macsec_upd_txsa,
[...]

This one could probably be:

#define _I(mdo) .mdo_ ## mdo = vlan_macsec_ ## mdo

static const struct macsec_ops macsec_offload_ops = {
	_I(add_txsa),
	_I(upd_txsa),
[...]
#undef _I

-- 
Ueimor

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

[Jakub Kicinski]

On Tue, 28 Mar 2023 06:54:11 +0000 Emeel Hakim wrote:
> > > +     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.

Decode this for me please:
 - what was you careful consideration
 - what do you think the task at hand is; and
 - what are vlan_features supposed to mean?

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

[Emeel Hakim]

> -----Original Message-----
> From: Francois Romieu <romieu@fr.zoreil.com>
> Sent: Tuesday, 28 March 2023 21:21
> To: Emeel Hakim <ehakim@nvidia.com>
> Cc: Jakub Kicinski <kuba@kernel.org>; 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
> 
> 
> Emeel Hakim <ehakim@nvidia.com> :
> > > -----Original Message-----
> > > From: Jakub Kicinski <kuba@kernel.org>
> [...]
> > > > +#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.
> 
> Sometime it's also nice to be able to use such modern tools as tags and grep.
> 
> While it still implies some duplication and it doesn't automagically prevent wrongly
> mixing vlan_macsec_foo() and ops->mdo_bar(), the code below may be considered
> as a trade-off:
> 
> #define _B(mdo) \
>         const struct macsec_ops *ops; \
>         ops = vlan_dev_priv(ctx->netdev)->real_dev->macsec_ops; \
>         return ops ? ops->mdo_ ## mdo(ctx) : -EOPNOTSUPP; \
> 
> 
> static int vlan_macsec_add_txsa(struct macsec_context *ctx) { _B(add_txsa) } static
> int vlan_macsec_upd_txsa(struct macsec_context *ctx) { _B(upd_txsa) } [...] #undef
> _B

I think that’s a reasonable compromise, I can do that and resend.

> On a tangent topic, both codes expand 12 times the accessor
> 
> vlan_dev_priv(ctx->netdev)->real_dev->macsec_ops.
> 
> It imvho deserves an helper function so that the compiler can make some choice.

Agreed.

> As a final remark, VLAN_MACSEC_DECLARE_MDO above does not buy much.
> Compare:
> 
> static const struct macsec_ops macsec_offload_ops = {
>         .mdo_add_txsa = VLAN_MACSEC_DECLARE_MDO(add_txsa),
>         .mdo_upd_txsa = VLAN_MACSEC_DECLARE_MDO(upd_txsa),
> [...]
> 
> vs
> 
> static const struct macsec_ops macsec_offload_ops = {
>         .mdo_add_txsa = vlan_macsec_add_txsa,
>         .mdo_upd_txsa = vlan_macsec_upd_txsa, [...]
> 
> This one could probably be:
> 
> #define _I(mdo) .mdo_ ## mdo = vlan_macsec_ ## mdo
> 
> static const struct macsec_ops macsec_offload_ops = {
>         _I(add_txsa),
>         _I(upd_txsa),
> [...]
> #undef _I

I agree I will do that.

> --
> Ueimor

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

[Emeel Hakim]

> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Wednesday, 29 March 2023 1:42
> 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 Tue, 28 Mar 2023 06:54:11 +0000 Emeel Hakim wrote:
> > > > +     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.
> 
> Decode this for me please:
>  - what was you careful consideration
>  - what do you think the task at hand is; and
>  - what are vlan_features supposed to mean?

I misunderstood your previous comment.
I took it internally with Leon Romanovsky and handled it,
I will send a v2.
Thanks for your comment.