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:   Wed, 7 Dec 2022 16:45:34 +0100
From:   Sabrina Dubroca <sd@queasysnail.net>
To:     ehakim@nvidia.com
Cc:     linux-kernel@vger.kernel.org, raeds@nvidia.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, netdev@vger.kernel.org, atenart@kernel.org,
        jiri@resnulli.us
Subject: Re: [PATCH net-next v3 1/2] macsec: add support for
 IFLA_MACSEC_OFFLOAD in macsec_changelink
Message-ID: <Y5C1Hifsg3/lJJ8N@hog>
References: <20221207101017.533-1-ehakim@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20221207101017.533-1-ehakim@nvidia.com>
Precedence: bulk

2022-12-07, 12:10:16 +0200, ehakim@nvidia.com wrote:
> From: Emeel Hakim <ehakim@nvidia.com>
> 
> Add support for changing Macsec offload selection through the
> netlink layer by implementing the relevant changes in
> macsec_change link.

nit: macsec_changelink

[...]
> +static int macsec_update_offload(struct macsec_dev *macsec, enum macsec_offload offload)
> +{
> +	enum macsec_offload prev_offload;
> +	const struct macsec_ops *ops;
> +	struct macsec_context ctx;
> +	int ret = 0;
> +
> +	prev_offload = macsec->offload;
> +
> +	/* Check if the device already has rules configured: we do not support
> +	 * rules migration.
> +	 */
> +	if (macsec_is_configured(macsec))
> +		return -EBUSY;
> +
> +	ops = __macsec_get_ops(offload == MACSEC_OFFLOAD_OFF ? prev_offload : offload,
> +			       macsec, &ctx);
> +	if (!ops)
> +		return -EOPNOTSUPP;
> +
> +	macsec->offload = offload;
> +
> +	ctx.secy = &macsec->secy;
> +	ret = (offload == MACSEC_OFFLOAD_OFF) ? macsec_offload(ops->mdo_del_secy, &ctx) :
> +		      macsec_offload(ops->mdo_add_secy, &ctx);

I think aligning the two macsec_offload(...) calls would make this a
bit easier to read:

	ret = offload == MACSEC_OFFLOAD_OFF ? macsec_offload(ops->mdo_del_secy, &ctx)
					    : macsec_offload(ops->mdo_add_secy, &ctx);

(and remove the unnecessary ())

> +
> +	if (ret)
> +		macsec->offload = prev_offload;
> +
> +	return ret;
> +}
> +

[...]
> +static int macsec_changelink_upd_offload(struct net_device *dev, struct nlattr *data[])
> +{
> +	enum macsec_offload offload;
> +	struct macsec_dev *macsec;
> +
> +	macsec = macsec_priv(dev);
> +	offload = nla_get_u8(data[IFLA_MACSEC_OFFLOAD]);

All those checks are also present in macsec_upd_offload, why not move
them into macsec_update_offload as well? (and then you don't really
need macsec_changelink_upd_offload anymore)

> +	if (macsec->offload == offload)
> +		return 0;
> +
> +	/* Check if the offloading mode is supported by the underlying layers */
> +	if (offload != MACSEC_OFFLOAD_OFF &&
> +	    !macsec_check_offload(offload, macsec))
> +		return -EOPNOTSUPP;
> +
> +	/* Check if the net device is busy. */
> +	if (netif_running(dev))
> +		return -EBUSY;
> +
> +	return macsec_update_offload(macsec, offload);
> +}
> +

-- 
Sabrina