[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y5C1Hifsg3/lJJ8N@hog>
Date: Wed, 7 Dec 2022 16:45:34 +0100
From: Sabrina Dubroca <sd@...asysnail.net>
To: ehakim@...dia.com
Cc: linux-kernel@...r.kernel.org, raeds@...dia.com,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, netdev@...r.kernel.org, atenart@...nel.org,
jiri@...nulli.us
Subject: Re: [PATCH net-next v3 1/2] macsec: add support for
IFLA_MACSEC_OFFLOAD in macsec_changelink
2022-12-07, 12:10:16 +0200, ehakim@...dia.com wrote:
> From: Emeel Hakim <ehakim@...dia.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
Powered by blists - more mailing lists