Re: [PATCH net-next v2 6/9] net: macsec: hardware offloading infrastructure

[Igor Russkikh <Igor.Russkikh@aquantia.com>]

On 08.08.2019 17:05, Antoine Tenart wrote:

> The Rx and TX handlers are modified to take in account the special case
> were the MACsec transformation happens in the hardware, whether in a PHY
> or in a MAC, as the packets seen by the networking stack on both the

Don't you think we may eventually may need xmit / handle_frame ops to be
a part of macsec_ops?

That way software macsec could be extract to just another type of offload.
The drawback of current code is it doesn't show explicitly the path of
offloaded packets. It is hidden in `handle_not_macsec` and in
`macsec_start_xmit` branch. This makes incorrect counters to tick (see my below
comment)

Another thing is that both xmit / macsec_handle_frame can't now be customized
by device driver. But this may be required.
We for example have usecases and HW features to allow specific flows to bypass
macsec encryption. This is normally used for macsec key control protocols,
identified by ethertype. Your phy is also capable on that as I see.


> @@ -2546,11 +2814,15 @@ static netdev_tx_t macsec_start_xmit(struct sk_buff *skb,
>  {
>  	struct macsec_dev *macsec = netdev_priv(dev);
>  	struct macsec_secy *secy = &macsec->secy;
> +	struct macsec_tx_sc *tx_sc = &secy->tx_sc;
>  	struct pcpu_secy_stats *secy_stats;
> +	struct macsec_tx_sa *tx_sa;
>  	int ret, len;
>  
> +	tx_sa = macsec_txsa_get(tx_sc->sa[tx_sc->encoding_sa]);

Declared, but not used?

>  	/* 10.5 */
> -	if (!secy->protect_frames) {
> +	if (!secy->protect_frames || macsec_get_ops(netdev_priv(dev), NULL)) {
>  		secy_stats = this_cpu_ptr(macsec->stats);
>  		u64_stats_update_begin(&secy_stats->syncp);
>  		secy_stats->stats.OutPktsUntagged++;

Here you use same `if` for sw and hw flows, this making `OutPktsUntagged`
counter invalid.

>  	struct macsec_dev *macsec = macsec_priv(dev);
> -	struct net_device *real_dev;
> +	struct net_device *real_dev, *loop_dev;
> +	struct macsec_context ctx;
> +	const struct macsec_ops *ops;
> +	struct net *loop_net;

Reverse Christmas tree is normally a formatting requirement where possible.

> +	for_each_net(loop_net) {
> +		for_each_netdev(loop_net, loop_dev) {
> +			struct macsec_dev *priv;
> +
> +			if (!netif_is_macsec(loop_dev))
> +				continue;
> +
> +			priv = macsec_priv(loop_dev);
> +
> +			/* A limitation of the MACsec h/w offloading is only a
> +			 * single MACsec interface can be created for a given
> +			 * real interface.
> +			 */
> +			if (macsec_get_ops(netdev_priv(dev), NULL) &&
> +			    priv->real_dev == real_dev)
> +				return -EBUSY;
> +		}
> +	}
> +

There is no need to do this search loop if `macsec_get_ops(..) == NULL` ?
So you can extract this check before `for_each_net` for SW macsec...

Regards,
  Igor