Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 11 Aug 2023 19:42:14 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     "Radu Pirea (NXP OSS)" <radu-nicolae.pirea@oss.nxp.com>
Cc:     hkallweit1@gmail.com, linux@armlinux.org.uk, davem@davemloft.net,
        edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
        richardcochran@gmail.com, sd@queasysnail.net,
        netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [RFC net-next v1 4/5] net: macsec: introduce mdo_insert_tx_tag
Message-ID: <ee545162-7695-4089-804d-64438e1de620@lunn.ch>
References: <20230811153249.283984-1-radu-nicolae.pirea@oss.nxp.com>
 <20230811153249.283984-5-radu-nicolae.pirea@oss.nxp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230811153249.283984-5-radu-nicolae.pirea@oss.nxp.com>
Precedence: bulk

On Fri, Aug 11, 2023 at 06:32:48PM +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 32
> bytes. If the frames are sent at line rate, the PHY will not have enough
> room to insert the SecTAG and the ICV.
> 
> To mitigate this scenario, the PHY offer to use require 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.

I think this could be worded better, to take into account different
implementations. As far as i understand, some PHYs include a MAC,
which reassembles the frame, and then places the frame into a queue
for processing. After processing, a second MAC does the actual send on
the wire. The queue allows for some number of back to back frames
without having problems. The PHY then uses flow control pause to slow
down the SoC MAC when there is a long burst of line rate frames which
would otherwise overflow the queue.

So:

> If the frames are sent at line rate, the PHY will not have enough
> room to insert the SecTAG and the ICV.

This probably want to clarify that a PHY which does not buffer....

> To mitigate this scenario, the PHY offer to use require a specific

and here you want to say some PHYs offer, since not all PHYs will do
this.

> +	if (macsec->offload == MACSEC_OFFLOAD_OFF) {
> +		dev->needed_headroom -= ops->needed_headroom;
> +		dev->needed_headroom += MACSEC_NEEDED_HEADROOM;
> +		dev->needed_tailroom -= ops->needed_tailroom;
> +		dev->needed_tailroom += MACSEC_NEEDED_TAILROOM;
> +	} else {
> +		dev->needed_headroom -= MACSEC_NEEDED_HEADROOM;
> +		dev->needed_headroom += ops->needed_headroom;
> +		dev->needed_tailroom -= MACSEC_NEEDED_TAILROOM;
> +		dev->needed_tailroom += ops->needed_tailroom;
> +	}

It is not obvious to me what this is doing. Should this actually be in
macsec_dev_init()? My main problem is why there is an else condition?

> +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 macsec_context ctx;
> +	int err;
> +
> +	if (!macsec_is_offloaded(macsec))
> +		return ERR_PTR(-EINVAL);

Hasn't this already been checked in macsec_start_xmit()?

> +
> +	ops = macsec_get_ops(macsec, &ctx);
> +	if (!ops)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!ops->mdo_insert_tx_tag)
> +		return skb;

You are in the hot path here. You don't expect this to change from
frame to frame. So could you evaluate this once and store it
somewhere? Maybe in macsec_dev ?

	Andrew