Offloading MACsec in PHYs requires inserting the SecTAG and the ICV in
the ethernet frame. This operation will increase the frame size with up
to 32 bytes. If the frames are sent at line rate, the PHY will not have
enough room to insert the SecTAG and the ICV.

Some PHYs use a hardware buffer to store a number of ethernet frames and,
if it fills up, a pause frame is sent to the MAC to control the flow.
This HW implementation does not need any modification in the stack.

Other PHYs might offer to use a specific ethertype with some padding
bytes present in the ethernet frame. This ethertype and its associated
bytes will be replaced by the SecTAG and ICV.

mdo_insert_tx_tag allows the PHY drivers to add any specific tag in the
skb.

Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
---
 drivers/net/macsec.c | 96 +++++++++++++++++++++++++++++++++++++++++++-
 include/net/macsec.h | 10 +++++
 2 files changed, 105 insertions(+), 1 deletion(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index ae60817ec5c2..5541aaced61f 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -93,6 +93,7 @@ struct pcpu_secy_stats {
  * @secys: linked list of SecY's on the underlying device
  * @gro_cells: pointer to the Generic Receive Offload cell
  * @offload: status of offloading on the MACsec device
+ * @insert_tx_tag: insert tx tag if true
  */
 struct macsec_dev {
 	struct macsec_secy secy;
@@ -102,6 +103,7 @@ struct macsec_dev {
 	struct list_head secys;
 	struct gro_cells gro_cells;
 	enum macsec_offload offload;
+	bool insert_tx_tag;
 };
 
 /**
@@ -2582,6 +2584,33 @@ static bool macsec_is_configured(struct macsec_dev *macsec)
 	return false;
 }
 
+static bool macsec_can_insert_tx_tag(struct macsec_dev *macsec,
+				     const struct macsec_ops *ops)
+{
+	return macsec->offload == MACSEC_OFFLOAD_PHY &&
+		ops->mdo_insert_tx_tag;
+}
+
+static void macsec_adjust_room(struct net_device *dev,
+			       const struct macsec_ops *ops)
+{
+	struct macsec_dev *macsec = macsec = macsec_priv(dev);
+
+	if (macsec_is_offloaded(macsec)) {
+		dev->needed_headroom -= MACSEC_NEEDED_HEADROOM;
+		dev->needed_headroom += ops->needed_headroom;
+		dev->needed_tailroom -= MACSEC_NEEDED_TAILROOM;
+		dev->needed_tailroom += ops->needed_tailroom;
+
+		return;
+	}
+
+	dev->needed_headroom -= ops->needed_headroom;
+	dev->needed_headroom += MACSEC_NEEDED_HEADROOM;
+	dev->needed_tailroom -= ops->needed_tailroom;
+	dev->needed_tailroom += MACSEC_NEEDED_TAILROOM;
+}
+
 static int macsec_update_offload(struct net_device *dev, enum macsec_offload offload)
 {
 	enum macsec_offload prev_offload;
@@ -2619,9 +2648,15 @@ static int macsec_update_offload(struct net_device *dev, enum macsec_offload off
 	ctx.secy = &macsec->secy;
 	ret = offload == MACSEC_OFFLOAD_OFF ? macsec_offload(ops->mdo_del_secy, &ctx)
 					    : macsec_offload(ops->mdo_add_secy, &ctx);
-	if (ret)
+	if (ret) {
 		macsec->offload = prev_offload;
+		goto out;
+	}
+
+	macsec_adjust_room(dev, ops);
+	macsec->insert_tx_tag = macsec_can_insert_tx_tag(macsec, ops);
 
+out:
 	return ret;
 }
 
@@ -3378,6 +3413,55 @@ static struct genl_family macsec_fam __ro_after_init = {
 	.resv_start_op	= MACSEC_CMD_UPD_OFFLOAD + 1,
 };
 
+static struct sk_buff *insert_tx_tag(struct sk_buff *skb,
+				     struct net_device *dev)
+{
+	struct macsec_dev *macsec = macsec_priv(dev);
+	const struct macsec_ops *ops;
+	struct phy_device *phydev;
+	struct macsec_context ctx;
+	int err;
+
+	if (!macsec->insert_tx_tag)
+		return skb;
+
+	ops = macsec_get_ops(macsec, &ctx);
+	phydev = macsec->real_dev->phydev;
+
+	if (unlikely(skb_headroom(skb) < ops->needed_headroom ||
+		     skb_tailroom(skb) < ops->needed_tailroom)) {
+		struct sk_buff *nskb = skb_copy_expand(skb,
+						       ops->needed_headroom,
+						       ops->needed_tailroom,
+						       GFP_ATOMIC);
+		if (likely(nskb)) {
+			consume_skb(skb);
+			skb = nskb;
+		} else {
+			err = -ENOMEM;
+			goto cleanup;
+		}
+	} else {
+		skb = skb_unshare(skb, GFP_ATOMIC);
+		if (!skb)
+			return ERR_PTR(-ENOMEM);
+	}
+
+	err = ops->mdo_insert_tx_tag(phydev, skb);
+	if (unlikely(err))
+		goto cleanup;
+
+	if (unlikely(skb->len - ETH_HLEN > macsec_priv(dev)->real_dev->mtu)) {
+		err = -EINVAL;
+		goto cleanup;
+	}
+
+	return skb;
+cleanup:
+	kfree_skb(skb);
+	return ERR_PTR(err);
+}
+
 static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
 				     struct net_device *dev)
 {
@@ -3392,6 +3476,13 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
 		skb_dst_drop(skb);
 		dst_hold(&md_dst->dst);
 		skb_dst_set(skb, &md_dst->dst);
+
+		skb = insert_tx_tag(skb, dev);
+		if (IS_ERR(skb)) {
+			DEV_STATS_INC(dev, tx_dropped);
+			return NETDEV_TX_OK;
+		}
+
 		skb->dev = macsec->real_dev;
 		return dev_queue_xmit(skb);
 	}
@@ -4125,6 +4216,9 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
 			err = macsec_offload(ops->mdo_add_secy, &ctx);
 			if (err)
 				goto del_dev;
+
+			macsec_adjust_room(dev, ops);
+			macsec->insert_tx_tag = macsec_can_insert_tx_tag(macsec, ops);
 		}
 	}
 
diff --git a/include/net/macsec.h b/include/net/macsec.h
index 76f024727bb4..9577921897f9 100644
--- a/include/net/macsec.h
+++ b/include/net/macsec.h
@@ -312,6 +312,11 @@ struct macsec_context {
  * @mdo_get_tx_sa_stats: called when TX SA stats are read
  * @mdo_get_rx_sc_stats: called when RX SC stats are read
  * @mdo_get_rx_sa_stats: called when RX SA stats are read
+ * @mdo_insert_tx_tag: called to insert the TX offload tag
+ * @needed_headroom: number of bytes reserved at the beginning of the sk_buff
+ *	for the TX Tag
+ * @needed_tailroom: number of bytes reserved at the end of the sk_buff for the
+ *	TX Tag
  */
 struct macsec_ops {
 	/* Device wide */
@@ -338,6 +343,11 @@ struct macsec_ops {
 	int (*mdo_get_tx_sa_stats)(struct macsec_context *ctx);
 	int (*mdo_get_rx_sc_stats)(struct macsec_context *ctx);
 	int (*mdo_get_rx_sa_stats)(struct macsec_context *ctx);
+	/* Offload tag */
+	int (*mdo_insert_tx_tag)(struct phy_device *phydev,
+				 struct sk_buff *skb);
+	unsigned int needed_headroom;
+	unsigned int needed_tailroom;
 };
 
 void macsec_pn_wrapped(struct macsec_secy *secy, struct macsec_tx_sa *tx_sa);
-- 
2.34.1

2023-08-24, 12:16:12 +0300, Radu Pirea (NXP OSS) wrote:
> Offloading MACsec in PHYs requires inserting the SecTAG and the ICV in
> the ethernet frame. This operation will increase the frame size with up
> to 32 bytes. If the frames are sent at line rate, the PHY will not have
> enough room to insert the SecTAG and the ICV.
> 
> Some PHYs use a hardware buffer to store a number of ethernet frames and,
> if it fills up, a pause frame is sent to the MAC to control the flow.
> This HW implementation does not need any modification in the stack.
> 
> Other PHYs might offer to use a specific ethertype with some padding
> bytes present in the ethernet frame. This ethertype and its associated
> bytes will be replaced by the SecTAG and ICV.
> 
> mdo_insert_tx_tag allows the PHY drivers to add any specific tag in the
> skb.

Please add a per-patch changelog between versions. For example:

v2:
 - add doc for the new fields in macsec_ops
 - add insert_tx_tag to macsec_dev
 - use unsigned int for macsec_ops.needed_{head,tail}room
[etc]

> Signed-off-by: Radu Pirea (NXP OSS) <radu-nicolae.pirea@oss.nxp.com>
> ---
>  drivers/net/macsec.c | 96 +++++++++++++++++++++++++++++++++++++++++++-
>  include/net/macsec.h | 10 +++++
>  2 files changed, 105 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index ae60817ec5c2..5541aaced61f 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -93,6 +93,7 @@ struct pcpu_secy_stats {
>   * @secys: linked list of SecY's on the underlying device
>   * @gro_cells: pointer to the Generic Receive Offload cell
>   * @offload: status of offloading on the MACsec device
> + * @insert_tx_tag: insert tx tag if true

(probably a bit nitpicky)
Maybe briefly mention something about offloading and why devices might
needed that tag? Otherwise this doc feels a bit like it's there just
to make a checker happy, it doesn't say anything that "bool
insert_tx_tag" doesn't already tell us.

Maybe something like:
"when offloading, device requires to insert an additional tag"

>   */
>  struct macsec_dev {
>  	struct macsec_secy secy;
> @@ -102,6 +103,7 @@ struct macsec_dev {
>  	struct list_head secys;
>  	struct gro_cells gro_cells;
>  	enum macsec_offload offload;
> +	bool insert_tx_tag;
>  };
>  
>  /**
> @@ -2582,6 +2584,33 @@ static bool macsec_is_configured(struct macsec_dev *macsec)
>  	return false;
>  }
>  
> +static bool macsec_can_insert_tx_tag(struct macsec_dev *macsec,

It feels more like a "needs" than a "can" situation to me. The device
needs this tag inserted in order to fully work.

> +				     const struct macsec_ops *ops)
> +{
> +	return macsec->offload == MACSEC_OFFLOAD_PHY &&
> +		ops->mdo_insert_tx_tag;
> +}
> +
> +static void macsec_adjust_room(struct net_device *dev,
> +			       const struct macsec_ops *ops)
> +{
> +	struct macsec_dev *macsec = macsec = macsec_priv(dev);

duplicate "macsec = macsec = ..."

> +
> +	if (macsec_is_offloaded(macsec)) {

Shouldn't that whole adjustment (in both directions) depend on
->insert_tx_tag?

> +		dev->needed_headroom -= MACSEC_NEEDED_HEADROOM;
> +		dev->needed_headroom += ops->needed_headroom;

I would compute "diff = ops->needed_headroom - MACSEC_NEEDED_HEADROOM"
at the start and then we can simply do "+= diff" or "-= diff" (and
same for tailroom).

> +		dev->needed_tailroom -= MACSEC_NEEDED_TAILROOM;
> +		dev->needed_tailroom += ops->needed_tailroom;
> +
> +		return;
> +	}

nit: else instead of the early return would make things more
symmetrical.

> +
> +	dev->needed_headroom -= ops->needed_headroom;
> +	dev->needed_headroom += MACSEC_NEEDED_HEADROOM;
> +	dev->needed_tailroom -= ops->needed_tailroom;
> +	dev->needed_tailroom += MACSEC_NEEDED_TAILROOM;
> +}
> +
>  static int macsec_update_offload(struct net_device *dev, enum macsec_offload offload)
>  {
>  	enum macsec_offload prev_offload;
> @@ -2619,9 +2648,15 @@ static int macsec_update_offload(struct net_device *dev, enum macsec_offload off
>  	ctx.secy = &macsec->secy;
>  	ret = offload == MACSEC_OFFLOAD_OFF ? macsec_offload(ops->mdo_del_secy, &ctx)
>  					    : macsec_offload(ops->mdo_add_secy, &ctx);
> -	if (ret)
> +	if (ret) {
>  		macsec->offload = prev_offload;
> +		goto out;

I would prefer a direct return right here instead of this goto.

> +	}
> +
> +	macsec_adjust_room(dev, ops);
> +	macsec->insert_tx_tag = macsec_can_insert_tx_tag(macsec, ops);
>  
> +out:
>  	return ret;
>  }
>  
> @@ -3378,6 +3413,55 @@ static struct genl_family macsec_fam __ro_after_init = {
>  	.resv_start_op	= MACSEC_CMD_UPD_OFFLOAD + 1,
>  };
>  
> +static struct sk_buff *insert_tx_tag(struct sk_buff *skb,
> +				     struct net_device *dev)
> +{
> +	struct macsec_dev *macsec = macsec_priv(dev);
> +	const struct macsec_ops *ops;
> +	struct phy_device *phydev;
> +	struct macsec_context ctx;
> +	int err;
> +
> +	if (!macsec->insert_tx_tag)
> +		return skb;

I think it would look a bit nicer if this test was moved out, before
calling insert_tx_tag(). Then if we call insert_tx_tag(), we know we
have to insert it.

> +	ops = macsec_get_ops(macsec, &ctx);
> +	phydev = macsec->real_dev->phydev;
> +

[...]
> @@ -4125,6 +4216,9 @@ static int macsec_newlink(struct net *net, struct net_device *dev,
>  			err = macsec_offload(ops->mdo_add_secy, &ctx);
>  			if (err)
>  				goto del_dev;
> +
> +			macsec_adjust_room(dev, ops);
> +			macsec->insert_tx_tag = macsec_can_insert_tx_tag(macsec, ops);
>  		}
>  	}
>  
> diff --git a/include/net/macsec.h b/include/net/macsec.h
> index 76f024727bb4..9577921897f9 100644
> --- a/include/net/macsec.h
> +++ b/include/net/macsec.h
> @@ -312,6 +312,11 @@ struct macsec_context {
>   * @mdo_get_tx_sa_stats: called when TX SA stats are read
>   * @mdo_get_rx_sc_stats: called when RX SC stats are read
>   * @mdo_get_rx_sa_stats: called when RX SA stats are read
> + * @mdo_insert_tx_tag: called to insert the TX offload tag
> + * @needed_headroom: number of bytes reserved at the beginning of the sk_buff
> + *	for the TX Tag
> + * @needed_tailroom: number of bytes reserved at the end of the sk_buff for the
> + *	TX Tag

It would be nice to use a consistent name (either "TX offload tag" or
"TX tag") and case in those 3 descriptions (slight preference for
"tag" over "Tag" on my side).

I'd also add ", to be filled by mdo_insert_tx_tag" (not sure whether
that needs to be @mdo_insert_tx_tag or just mdo_insert_tx_tag) to the
needed_headroom/needed_tailroom descriptions, just to be really clear.

-- 
Sabrina

On 24.08.2023 17:54, Sabrina Dubroca wrote:
> 2023-08-24, 12:16:12 +0300, Radu Pirea (NXP OSS) wrote:
> 
>> +
>> +	if (macsec_is_offloaded(macsec)) {
> 
> Shouldn't that whole adjustment (in both directions) depend on
> ->insert_tx_tag?

I asked myself the same thing and the answer was "It depends". This 
adjustment can be restricted only to the offloaded MACsec devs that 
require a special TX tag. However, the offloaded MACsec devs do not need 
additional headroom/tailroom.
>> +
>> +	if (!macsec->insert_tx_tag)
>> +		return skb;
> 
> I think it would look a bit nicer if this test was moved out, before
> calling insert_tx_tag(). Then if we call insert_tx_tag(), we know we
> have to insert it.
> 

I expected this suggestion :)

-- 
Radu P.