From: Emeel Hakim <[email protected]>
This adds support for configuring Macsec offload through the
netlink layer by:
- Considering IFLA_MACSEC_OFFLOAD in macsec_fill_info.
- Handling IFLA_MACSEC_OFFLOAD in macsec_changelink.
- Adding IFLA_MACSEC_OFFLOAD to the netlink policy.
- Adjusting macsec_get_size.
The handling in macsec_changlink is similar to
macsec_upd_offload.
Update macsec_upd_offload to use a common helper function
to avoid duplication.
Example for setting offload for a macsec device
ip link set macsec0 type macsec offload mac
Reviewed-by: Raed Salem <[email protected]>
Signed-off-by: Emeel Hakim <[email protected]>
---
V1 -> V2: Add common helper to avoid duplicating code
drivers/net/macsec.c | 114 ++++++++++++++++++++++++++++---------------
1 file changed, 74 insertions(+), 40 deletions(-)
diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index d73b9d535b7a..afd6ff47ba56 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -2583,16 +2583,45 @@ static bool macsec_is_configured(struct macsec_dev *macsec)
return false;
}
+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);
+
+ if (ret)
+ macsec->offload = prev_offload;
+
+ return ret;
+}
+
static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info)
{
struct nlattr *tb_offload[MACSEC_OFFLOAD_ATTR_MAX + 1];
- enum macsec_offload offload, prev_offload;
- int (*func)(struct macsec_context *ctx);
struct nlattr **attrs = info->attrs;
- struct net_device *dev;
- const struct macsec_ops *ops;
- struct macsec_context ctx;
+ enum macsec_offload offload;
struct macsec_dev *macsec;
+ struct net_device *dev;
int ret;
if (!attrs[MACSEC_ATTR_IFINDEX])
@@ -2629,39 +2658,7 @@ static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info)
rtnl_lock();
- prev_offload = macsec->offload;
- macsec->offload = offload;
-
- /* Check if the device already has rules configured: we do not support
- * rules migration.
- */
- if (macsec_is_configured(macsec)) {
- ret = -EBUSY;
- goto rollback;
- }
-
- ops = __macsec_get_ops(offload == MACSEC_OFFLOAD_OFF ? prev_offload : offload,
- macsec, &ctx);
- if (!ops) {
- ret = -EOPNOTSUPP;
- goto rollback;
- }
-
- if (prev_offload == MACSEC_OFFLOAD_OFF)
- func = ops->mdo_add_secy;
- else
- func = ops->mdo_del_secy;
-
- ctx.secy = &macsec->secy;
- ret = macsec_offload(func, &ctx);
- if (ret)
- goto rollback;
-
- rtnl_unlock();
- return 0;
-
-rollback:
- macsec->offload = prev_offload;
+ ret = macsec_update_offload(macsec, offload);
rtnl_unlock();
return ret;
@@ -3698,6 +3695,7 @@ static const struct nla_policy macsec_rtnl_policy[IFLA_MACSEC_MAX + 1] = {
[IFLA_MACSEC_SCB] = { .type = NLA_U8 },
[IFLA_MACSEC_REPLAY_PROTECT] = { .type = NLA_U8 },
[IFLA_MACSEC_VALIDATION] = { .type = NLA_U8 },
+ [IFLA_MACSEC_OFFLOAD] = { .type = NLA_U8 },
};
static void macsec_free_netdev(struct net_device *dev)
@@ -3803,6 +3801,29 @@ static int macsec_changelink_common(struct net_device *dev,
return 0;
}
+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]);
+
+ 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);
+}
+
static int macsec_changelink(struct net_device *dev, struct nlattr *tb[],
struct nlattr *data[],
struct netlink_ext_ack *extack)
@@ -3831,6 +3852,12 @@ static int macsec_changelink(struct net_device *dev, struct nlattr *tb[],
if (ret)
goto cleanup;
+ if (data[IFLA_MACSEC_OFFLOAD]) {
+ ret = macsec_changelink_upd_offload(dev, data);
+ if (ret)
+ goto cleanup;
+ }
+
/* If h/w offloading is available, propagate to the device */
if (macsec_is_offloaded(macsec)) {
const struct macsec_ops *ops;
@@ -4231,16 +4258,22 @@ static size_t macsec_get_size(const struct net_device *dev)
nla_total_size(1) + /* IFLA_MACSEC_SCB */
nla_total_size(1) + /* IFLA_MACSEC_REPLAY_PROTECT */
nla_total_size(1) + /* IFLA_MACSEC_VALIDATION */
+ nla_total_size(1) + /* IFLA_MACSEC_OFFLOAD */
0;
}
static int macsec_fill_info(struct sk_buff *skb,
const struct net_device *dev)
{
- struct macsec_secy *secy = &macsec_priv(dev)->secy;
- struct macsec_tx_sc *tx_sc = &secy->tx_sc;
+ struct macsec_tx_sc *tx_sc;
+ struct macsec_dev *macsec;
+ struct macsec_secy *secy;
u64 csid;
+ macsec = macsec_priv(dev);
+ secy = &macsec->secy;
+ tx_sc = &secy->tx_sc;
+
switch (secy->key_len) {
case MACSEC_GCM_AES_128_SAK_LEN:
csid = secy->xpn ? MACSEC_CIPHER_ID_GCM_AES_XPN_128 : MACSEC_DEFAULT_CIPHER_ID;
@@ -4265,6 +4298,7 @@ static int macsec_fill_info(struct sk_buff *skb,
nla_put_u8(skb, IFLA_MACSEC_SCB, tx_sc->scb) ||
nla_put_u8(skb, IFLA_MACSEC_REPLAY_PROTECT, secy->replay_protect) ||
nla_put_u8(skb, IFLA_MACSEC_VALIDATION, secy->validate_frames) ||
+ nla_put_u8(skb, IFLA_MACSEC_OFFLOAD, macsec->offload) ||
0)
goto nla_put_failure;
--
2.21.3
Tue, Dec 06, 2022 at 09:57:57AM CET, [email protected] wrote:
>From: Emeel Hakim <[email protected]>
>
>This adds support for configuring Macsec offload through the
Tell the codebase what to do. Be imperative in your patch descriptions
so it is clear what are the intensions of the patch.
>netlink layer by:
>- Considering IFLA_MACSEC_OFFLOAD in macsec_fill_info.
>- Handling IFLA_MACSEC_OFFLOAD in macsec_changelink.
>- Adding IFLA_MACSEC_OFFLOAD to the netlink policy.
>- Adjusting macsec_get_size.
4 patches then?
I mean really, not a macsec person, but I should be able to follow what
are your intensions looking and description&code right away.
>
>The handling in macsec_changlink is similar to
s/macsec_changlink/macsec_changelink/
>macsec_upd_offload.
>Update macsec_upd_offload to use a common helper function
>to avoid duplication.
>
>Example for setting offload for a macsec device
> ip link set macsec0 type macsec offload mac
>
>Reviewed-by: Raed Salem <[email protected]>
>Signed-off-by: Emeel Hakim <[email protected]>
>---
>V1 -> V2: Add common helper to avoid duplicating code
> drivers/net/macsec.c | 114 ++++++++++++++++++++++++++++---------------
> 1 file changed, 74 insertions(+), 40 deletions(-)
>
>diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
>index d73b9d535b7a..afd6ff47ba56 100644
>--- a/drivers/net/macsec.c
>+++ b/drivers/net/macsec.c
>@@ -2583,16 +2583,45 @@ static bool macsec_is_configured(struct macsec_dev *macsec)
> return false;
> }
>
>+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);
>+
>+ if (ret)
>+ macsec->offload = prev_offload;
>+
>+ return ret;
>+}
>+
> static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info)
> {
> struct nlattr *tb_offload[MACSEC_OFFLOAD_ATTR_MAX + 1];
>- enum macsec_offload offload, prev_offload;
>- int (*func)(struct macsec_context *ctx);
> struct nlattr **attrs = info->attrs;
>- struct net_device *dev;
>- const struct macsec_ops *ops;
>- struct macsec_context ctx;
>+ enum macsec_offload offload;
> struct macsec_dev *macsec;
>+ struct net_device *dev;
> int ret;
>
> if (!attrs[MACSEC_ATTR_IFINDEX])
>@@ -2629,39 +2658,7 @@ static int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info)
>
> rtnl_lock();
>
>- prev_offload = macsec->offload;
>- macsec->offload = offload;
>-
>- /* Check if the device already has rules configured: we do not support
>- * rules migration.
>- */
>- if (macsec_is_configured(macsec)) {
>- ret = -EBUSY;
>- goto rollback;
>- }
>-
>- ops = __macsec_get_ops(offload == MACSEC_OFFLOAD_OFF ? prev_offload : offload,
>- macsec, &ctx);
>- if (!ops) {
>- ret = -EOPNOTSUPP;
>- goto rollback;
>- }
>-
>- if (prev_offload == MACSEC_OFFLOAD_OFF)
>- func = ops->mdo_add_secy;
>- else
>- func = ops->mdo_del_secy;
>-
>- ctx.secy = &macsec->secy;
>- ret = macsec_offload(func, &ctx);
>- if (ret)
>- goto rollback;
>-
>- rtnl_unlock();
>- return 0;
>-
>-rollback:
>- macsec->offload = prev_offload;
>+ ret = macsec_update_offload(macsec, offload);
>
> rtnl_unlock();
> return ret;
>@@ -3698,6 +3695,7 @@ static const struct nla_policy macsec_rtnl_policy[IFLA_MACSEC_MAX + 1] = {
> [IFLA_MACSEC_SCB] = { .type = NLA_U8 },
> [IFLA_MACSEC_REPLAY_PROTECT] = { .type = NLA_U8 },
> [IFLA_MACSEC_VALIDATION] = { .type = NLA_U8 },
>+ [IFLA_MACSEC_OFFLOAD] = { .type = NLA_U8 },
> };
>
> static void macsec_free_netdev(struct net_device *dev)
>@@ -3803,6 +3801,29 @@ static int macsec_changelink_common(struct net_device *dev,
> return 0;
> }
>
>+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]);
>+
>+ 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);
>+}
>+
> static int macsec_changelink(struct net_device *dev, struct nlattr *tb[],
> struct nlattr *data[],
> struct netlink_ext_ack *extack)
>@@ -3831,6 +3852,12 @@ static int macsec_changelink(struct net_device *dev, struct nlattr *tb[],
> if (ret)
> goto cleanup;
>
>+ if (data[IFLA_MACSEC_OFFLOAD]) {
>+ ret = macsec_changelink_upd_offload(dev, data);
>+ if (ret)
>+ goto cleanup;
>+ }
>+
> /* If h/w offloading is available, propagate to the device */
> if (macsec_is_offloaded(macsec)) {
> const struct macsec_ops *ops;
>@@ -4231,16 +4258,22 @@ static size_t macsec_get_size(const struct net_device *dev)
> nla_total_size(1) + /* IFLA_MACSEC_SCB */
> nla_total_size(1) + /* IFLA_MACSEC_REPLAY_PROTECT */
> nla_total_size(1) + /* IFLA_MACSEC_VALIDATION */
>+ nla_total_size(1) + /* IFLA_MACSEC_OFFLOAD */
> 0;
> }
>
> static int macsec_fill_info(struct sk_buff *skb,
> const struct net_device *dev)
> {
>- struct macsec_secy *secy = &macsec_priv(dev)->secy;
>- struct macsec_tx_sc *tx_sc = &secy->tx_sc;
>+ struct macsec_tx_sc *tx_sc;
>+ struct macsec_dev *macsec;
>+ struct macsec_secy *secy;
> u64 csid;
>
>+ macsec = macsec_priv(dev);
>+ secy = &macsec->secy;
>+ tx_sc = &secy->tx_sc;
>+
> switch (secy->key_len) {
> case MACSEC_GCM_AES_128_SAK_LEN:
> csid = secy->xpn ? MACSEC_CIPHER_ID_GCM_AES_XPN_128 : MACSEC_DEFAULT_CIPHER_ID;
>@@ -4265,6 +4298,7 @@ static int macsec_fill_info(struct sk_buff *skb,
> nla_put_u8(skb, IFLA_MACSEC_SCB, tx_sc->scb) ||
> nla_put_u8(skb, IFLA_MACSEC_REPLAY_PROTECT, secy->replay_protect) ||
> nla_put_u8(skb, IFLA_MACSEC_VALIDATION, secy->validate_frames) ||
>+ nla_put_u8(skb, IFLA_MACSEC_OFFLOAD, macsec->offload) ||
> 0)
> goto nla_put_failure;
>
>--
>2.21.3
>
> -----Original Message-----
> From: Jiri Pirko <[email protected]>
> Sent: Tuesday, 6 December 2022 11:16
> To: Emeel Hakim <[email protected]>
> Cc: [email protected]; Raed Salem <[email protected]>;
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH net-next v2] macsec: Add support for IFLA_MACSEC_OFFLOAD
> in the netlink layer
>
> External email: Use caution opening links or attachments
>
>
> Tue, Dec 06, 2022 at 09:57:57AM CET, [email protected] wrote:
> >From: Emeel Hakim <[email protected]>
> >
> >This adds support for configuring Macsec offload through the
>
> Tell the codebase what to do. Be imperative in your patch descriptions so it is clear
> what are the intensions of the patch.
Ack
>
>
> >netlink layer by:
> >- Considering IFLA_MACSEC_OFFLOAD in macsec_fill_info.
> >- Handling IFLA_MACSEC_OFFLOAD in macsec_changelink.
> >- Adding IFLA_MACSEC_OFFLOAD to the netlink policy.
> >- Adjusting macsec_get_size.
>
> 4 patches then?
Ack, I will change the commit message to be imperative and will replace the list with a good description.
I still believe it should be a one patch since splitting this could break a bisect process.
> I mean really, not a macsec person, but I should be able to follow what are your
> intensions looking and description&code right away.
>
> >
> >The handling in macsec_changlink is similar to
>
> s/macsec_changlink/macsec_changelink/
Ack
> >macsec_upd_offload.
> >Update macsec_upd_offload to use a common helper function to avoid
> >duplication.
> >
> >Example for setting offload for a macsec device
> > ip link set macsec0 type macsec offload mac
> >
> >Reviewed-by: Raed Salem <[email protected]>
> >Signed-off-by: Emeel Hakim <[email protected]>
> >---
> >V1 -> V2: Add common helper to avoid duplicating code
> >drivers/net/macsec.c | 114 ++++++++++++++++++++++++++++---------------
> > 1 file changed, 74 insertions(+), 40 deletions(-)
> >
> >diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index
> >d73b9d535b7a..afd6ff47ba56 100644
> >--- a/drivers/net/macsec.c
> >+++ b/drivers/net/macsec.c
> >@@ -2583,16 +2583,45 @@ static bool macsec_is_configured(struct macsec_dev
> *macsec)
> > return false;
> > }
> >
> >+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);
> >+
> >+ if (ret)
> >+ macsec->offload = prev_offload;
> >+
> >+ return ret;
> >+}
> >+
> > static int macsec_upd_offload(struct sk_buff *skb, struct genl_info
> >*info) {
> > struct nlattr *tb_offload[MACSEC_OFFLOAD_ATTR_MAX + 1];
> >- enum macsec_offload offload, prev_offload;
> >- int (*func)(struct macsec_context *ctx);
> > struct nlattr **attrs = info->attrs;
> >- struct net_device *dev;
> >- const struct macsec_ops *ops;
> >- struct macsec_context ctx;
> >+ enum macsec_offload offload;
> > struct macsec_dev *macsec;
> >+ struct net_device *dev;
> > int ret;
> >
> > if (!attrs[MACSEC_ATTR_IFINDEX]) @@ -2629,39 +2658,7 @@ static
> >int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info)
> >
> > rtnl_lock();
> >
> >- prev_offload = macsec->offload;
> >- macsec->offload = offload;
> >-
> >- /* Check if the device already has rules configured: we do not support
> >- * rules migration.
> >- */
> >- if (macsec_is_configured(macsec)) {
> >- ret = -EBUSY;
> >- goto rollback;
> >- }
> >-
> >- ops = __macsec_get_ops(offload == MACSEC_OFFLOAD_OFF ? prev_offload :
> offload,
> >- macsec, &ctx);
> >- if (!ops) {
> >- ret = -EOPNOTSUPP;
> >- goto rollback;
> >- }
> >-
> >- if (prev_offload == MACSEC_OFFLOAD_OFF)
> >- func = ops->mdo_add_secy;
> >- else
> >- func = ops->mdo_del_secy;
> >-
> >- ctx.secy = &macsec->secy;
> >- ret = macsec_offload(func, &ctx);
> >- if (ret)
> >- goto rollback;
> >-
> >- rtnl_unlock();
> >- return 0;
> >-
> >-rollback:
> >- macsec->offload = prev_offload;
> >+ ret = macsec_update_offload(macsec, offload);
> >
> > rtnl_unlock();
> > return ret;
> >@@ -3698,6 +3695,7 @@ static const struct nla_policy
> macsec_rtnl_policy[IFLA_MACSEC_MAX + 1] = {
> > [IFLA_MACSEC_SCB] = { .type = NLA_U8 },
> > [IFLA_MACSEC_REPLAY_PROTECT] = { .type = NLA_U8 },
> > [IFLA_MACSEC_VALIDATION] = { .type = NLA_U8 },
> >+ [IFLA_MACSEC_OFFLOAD] = { .type = NLA_U8 },
> > };
> >
> > static void macsec_free_netdev(struct net_device *dev) @@ -3803,6
> >+3801,29 @@ static int macsec_changelink_common(struct net_device *dev,
> > return 0;
> > }
> >
> >+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]);
> >+
> >+ 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); }
> >+
> > static int macsec_changelink(struct net_device *dev, struct nlattr *tb[],
> > struct nlattr *data[],
> > struct netlink_ext_ack *extack) @@ -3831,6
> >+3852,12 @@ static int macsec_changelink(struct net_device *dev, struct nlattr
> *tb[],
> > if (ret)
> > goto cleanup;
> >
> >+ if (data[IFLA_MACSEC_OFFLOAD]) {
> >+ ret = macsec_changelink_upd_offload(dev, data);
> >+ if (ret)
> >+ goto cleanup;
> >+ }
> >+
> > /* If h/w offloading is available, propagate to the device */
> > if (macsec_is_offloaded(macsec)) {
> > const struct macsec_ops *ops; @@ -4231,16 +4258,22 @@
> >static size_t macsec_get_size(const struct net_device *dev)
> > nla_total_size(1) + /* IFLA_MACSEC_SCB */
> > nla_total_size(1) + /* IFLA_MACSEC_REPLAY_PROTECT */
> > nla_total_size(1) + /* IFLA_MACSEC_VALIDATION */
> >+ nla_total_size(1) + /* IFLA_MACSEC_OFFLOAD */
> > 0;
> > }
> >
> > static int macsec_fill_info(struct sk_buff *skb,
> > const struct net_device *dev) {
> >- struct macsec_secy *secy = &macsec_priv(dev)->secy;
> >- struct macsec_tx_sc *tx_sc = &secy->tx_sc;
> >+ struct macsec_tx_sc *tx_sc;
> >+ struct macsec_dev *macsec;
> >+ struct macsec_secy *secy;
> > u64 csid;
> >
> >+ macsec = macsec_priv(dev);
> >+ secy = &macsec->secy;
> >+ tx_sc = &secy->tx_sc;
> >+
> > switch (secy->key_len) {
> > case MACSEC_GCM_AES_128_SAK_LEN:
> > csid = secy->xpn ? MACSEC_CIPHER_ID_GCM_AES_XPN_128 :
> >MACSEC_DEFAULT_CIPHER_ID; @@ -4265,6 +4298,7 @@ static int
> macsec_fill_info(struct sk_buff *skb,
> > nla_put_u8(skb, IFLA_MACSEC_SCB, tx_sc->scb) ||
> > nla_put_u8(skb, IFLA_MACSEC_REPLAY_PROTECT, secy->replay_protect) ||
> > nla_put_u8(skb, IFLA_MACSEC_VALIDATION,
> >secy->validate_frames) ||
> >+ nla_put_u8(skb, IFLA_MACSEC_OFFLOAD, macsec->offload) ||
> > 0)
> > goto nla_put_failure;
> >
> >--
> >2.21.3
> >
Tue, Dec 06, 2022 at 01:31:54PM CET, [email protected] wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <[email protected]>
>> Sent: Tuesday, 6 December 2022 11:16
>> To: Emeel Hakim <[email protected]>
>> Cc: [email protected]; Raed Salem <[email protected]>;
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected]
>> Subject: Re: [PATCH net-next v2] macsec: Add support for IFLA_MACSEC_OFFLOAD
>> in the netlink layer
>>
>> External email: Use caution opening links or attachments
>>
>>
>> Tue, Dec 06, 2022 at 09:57:57AM CET, [email protected] wrote:
>> >From: Emeel Hakim <[email protected]>
>> >
>> >This adds support for configuring Macsec offload through the
>>
>> Tell the codebase what to do. Be imperative in your patch descriptions so it is clear
>> what are the intensions of the patch.
>
>Ack
>
>>
>>
>> >netlink layer by:
>> >- Considering IFLA_MACSEC_OFFLOAD in macsec_fill_info.
>> >- Handling IFLA_MACSEC_OFFLOAD in macsec_changelink.
>> >- Adding IFLA_MACSEC_OFFLOAD to the netlink policy.
>> >- Adjusting macsec_get_size.
>>
>> 4 patches then?
>
>Ack, I will change the commit message to be imperative and will replace the list with a good description.
>I still believe it should be a one patch since splitting this could break a bisect process.
Well, when you split, you have to make sure you don't break bisection,
always. Please try to figure that out.
>
>> I mean really, not a macsec person, but I should be able to follow what are your
>> intensions looking and description&code right away.
>>
>> >
>> >The handling in macsec_changlink is similar to
>>
>> s/macsec_changlink/macsec_changelink/
>
>Ack
>
>> >macsec_upd_offload.
>> >Update macsec_upd_offload to use a common helper function to avoid
>> >duplication.
>> >
>> >Example for setting offload for a macsec device
>> > ip link set macsec0 type macsec offload mac
>> >
>> >Reviewed-by: Raed Salem <[email protected]>
>> >Signed-off-by: Emeel Hakim <[email protected]>
>> >---
>> >V1 -> V2: Add common helper to avoid duplicating code
>> >drivers/net/macsec.c | 114 ++++++++++++++++++++++++++++---------------
>> > 1 file changed, 74 insertions(+), 40 deletions(-)
>> >
>> >diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c index
>> >d73b9d535b7a..afd6ff47ba56 100644
>> >--- a/drivers/net/macsec.c
>> >+++ b/drivers/net/macsec.c
>> >@@ -2583,16 +2583,45 @@ static bool macsec_is_configured(struct macsec_dev
>> *macsec)
>> > return false;
>> > }
>> >
>> >+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);
>> >+
>> >+ if (ret)
>> >+ macsec->offload = prev_offload;
>> >+
>> >+ return ret;
>> >+}
>> >+
>> > static int macsec_upd_offload(struct sk_buff *skb, struct genl_info
>> >*info) {
>> > struct nlattr *tb_offload[MACSEC_OFFLOAD_ATTR_MAX + 1];
>> >- enum macsec_offload offload, prev_offload;
>> >- int (*func)(struct macsec_context *ctx);
>> > struct nlattr **attrs = info->attrs;
>> >- struct net_device *dev;
>> >- const struct macsec_ops *ops;
>> >- struct macsec_context ctx;
>> >+ enum macsec_offload offload;
>> > struct macsec_dev *macsec;
>> >+ struct net_device *dev;
>> > int ret;
>> >
>> > if (!attrs[MACSEC_ATTR_IFINDEX]) @@ -2629,39 +2658,7 @@ static
>> >int macsec_upd_offload(struct sk_buff *skb, struct genl_info *info)
>> >
>> > rtnl_lock();
>> >
>> >- prev_offload = macsec->offload;
>> >- macsec->offload = offload;
>> >-
>> >- /* Check if the device already has rules configured: we do not support
>> >- * rules migration.
>> >- */
>> >- if (macsec_is_configured(macsec)) {
>> >- ret = -EBUSY;
>> >- goto rollback;
>> >- }
>> >-
>> >- ops = __macsec_get_ops(offload == MACSEC_OFFLOAD_OFF ? prev_offload :
>> offload,
>> >- macsec, &ctx);
>> >- if (!ops) {
>> >- ret = -EOPNOTSUPP;
>> >- goto rollback;
>> >- }
>> >-
>> >- if (prev_offload == MACSEC_OFFLOAD_OFF)
>> >- func = ops->mdo_add_secy;
>> >- else
>> >- func = ops->mdo_del_secy;
>> >-
>> >- ctx.secy = &macsec->secy;
>> >- ret = macsec_offload(func, &ctx);
>> >- if (ret)
>> >- goto rollback;
>> >-
>> >- rtnl_unlock();
>> >- return 0;
>> >-
>> >-rollback:
>> >- macsec->offload = prev_offload;
>> >+ ret = macsec_update_offload(macsec, offload);
>> >
>> > rtnl_unlock();
>> > return ret;
>> >@@ -3698,6 +3695,7 @@ static const struct nla_policy
>> macsec_rtnl_policy[IFLA_MACSEC_MAX + 1] = {
>> > [IFLA_MACSEC_SCB] = { .type = NLA_U8 },
>> > [IFLA_MACSEC_REPLAY_PROTECT] = { .type = NLA_U8 },
>> > [IFLA_MACSEC_VALIDATION] = { .type = NLA_U8 },
>> >+ [IFLA_MACSEC_OFFLOAD] = { .type = NLA_U8 },
>> > };
>> >
>> > static void macsec_free_netdev(struct net_device *dev) @@ -3803,6
>> >+3801,29 @@ static int macsec_changelink_common(struct net_device *dev,
>> > return 0;
>> > }
>> >
>> >+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]);
>> >+
>> >+ 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); }
>> >+
>> > static int macsec_changelink(struct net_device *dev, struct nlattr *tb[],
>> > struct nlattr *data[],
>> > struct netlink_ext_ack *extack) @@ -3831,6
>> >+3852,12 @@ static int macsec_changelink(struct net_device *dev, struct nlattr
>> *tb[],
>> > if (ret)
>> > goto cleanup;
>> >
>> >+ if (data[IFLA_MACSEC_OFFLOAD]) {
>> >+ ret = macsec_changelink_upd_offload(dev, data);
>> >+ if (ret)
>> >+ goto cleanup;
>> >+ }
>> >+
>> > /* If h/w offloading is available, propagate to the device */
>> > if (macsec_is_offloaded(macsec)) {
>> > const struct macsec_ops *ops; @@ -4231,16 +4258,22 @@
>> >static size_t macsec_get_size(const struct net_device *dev)
>> > nla_total_size(1) + /* IFLA_MACSEC_SCB */
>> > nla_total_size(1) + /* IFLA_MACSEC_REPLAY_PROTECT */
>> > nla_total_size(1) + /* IFLA_MACSEC_VALIDATION */
>> >+ nla_total_size(1) + /* IFLA_MACSEC_OFFLOAD */
>> > 0;
>> > }
>> >
>> > static int macsec_fill_info(struct sk_buff *skb,
>> > const struct net_device *dev) {
>> >- struct macsec_secy *secy = &macsec_priv(dev)->secy;
>> >- struct macsec_tx_sc *tx_sc = &secy->tx_sc;
>> >+ struct macsec_tx_sc *tx_sc;
>> >+ struct macsec_dev *macsec;
>> >+ struct macsec_secy *secy;
>> > u64 csid;
>> >
>> >+ macsec = macsec_priv(dev);
>> >+ secy = &macsec->secy;
>> >+ tx_sc = &secy->tx_sc;
>> >+
>> > switch (secy->key_len) {
>> > case MACSEC_GCM_AES_128_SAK_LEN:
>> > csid = secy->xpn ? MACSEC_CIPHER_ID_GCM_AES_XPN_128 :
>> >MACSEC_DEFAULT_CIPHER_ID; @@ -4265,6 +4298,7 @@ static int
>> macsec_fill_info(struct sk_buff *skb,
>> > nla_put_u8(skb, IFLA_MACSEC_SCB, tx_sc->scb) ||
>> > nla_put_u8(skb, IFLA_MACSEC_REPLAY_PROTECT, secy->replay_protect) ||
>> > nla_put_u8(skb, IFLA_MACSEC_VALIDATION,
>> >secy->validate_frames) ||
>> >+ nla_put_u8(skb, IFLA_MACSEC_OFFLOAD, macsec->offload) ||
>> > 0)
>> > goto nla_put_failure;
>> >
>> >--
>> >2.21.3
>> >
2022-12-06, 14:35:23 +0100, Jiri Pirko wrote:
> Tue, Dec 06, 2022 at 01:31:54PM CET, [email protected] wrote:
> >> Tue, Dec 06, 2022 at 09:57:57AM CET, [email protected] wrote:
> >> >From: Emeel Hakim <[email protected]>
> >> >
> >> >This adds support for configuring Macsec offload through the
> >>
> >> Tell the codebase what to do. Be imperative in your patch descriptions so it is clear
> >> what are the intensions of the patch.
> >
> >Ack
> >
> >>
> >>
> >> >netlink layer by:
> >> >- Considering IFLA_MACSEC_OFFLOAD in macsec_fill_info.
> >> >- Handling IFLA_MACSEC_OFFLOAD in macsec_changelink.
> >> >- Adding IFLA_MACSEC_OFFLOAD to the netlink policy.
> >> >- Adjusting macsec_get_size.
> >>
> >> 4 patches then?
> >
> >Ack, I will change the commit message to be imperative and will replace the list with a good description.
> >I still believe it should be a one patch since splitting this could break a bisect process.
>
> Well, when you split, you have to make sure you don't break bisection,
> always. Please try to figure that out.
I think this can be split pretty nicely into 3 patches:
- add IFLA_MACSEC_OFFLOAD to macsec_rtnl_policy (probably for net
with a Fixes tag on the commit that introduced IFLA_MACSEC_OFFLOAD)
- add offload to macsec_fill_info/macsec_get_size
- add IFLA_MACSEC_OFFLOAD support to changelink
The subject of the last patch should also make it clear that it's only
adding IFLA_MACSEC_OFFLOAD to changelink. As it's written, someone
could assume there's no support at all via rtnl ops and wonder why
this patch isn't doing anything to newlink, and whether/why this
IFLA_MACSEC_OFFLOAD already exists.
--
Sabrina
> -----Original Message-----
> From: Sabrina Dubroca <[email protected]>
> Sent: Tuesday, 6 December 2022 18:11
> To: Jiri Pirko <[email protected]>
> Cc: Emeel Hakim <[email protected]>; [email protected]; Raed
> Salem <[email protected]>; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]
> Subject: Re: [PATCH net-next v2] macsec: Add support for IFLA_MACSEC_OFFLOAD
> in the netlink layer
>
> External email: Use caution opening links or attachments
>
>
> 2022-12-06, 14:35:23 +0100, Jiri Pirko wrote:
> > Tue, Dec 06, 2022 at 01:31:54PM CET, [email protected] wrote:
> > >> Tue, Dec 06, 2022 at 09:57:57AM CET, [email protected] wrote:
> > >> >From: Emeel Hakim <[email protected]>
> > >> >
> > >> >This adds support for configuring Macsec offload through the
> > >>
> > >> Tell the codebase what to do. Be imperative in your patch
> > >> descriptions so it is clear what are the intensions of the patch.
> > >
> > >Ack
> > >
> > >>
> > >>
> > >> >netlink layer by:
> > >> >- Considering IFLA_MACSEC_OFFLOAD in macsec_fill_info.
> > >> >- Handling IFLA_MACSEC_OFFLOAD in macsec_changelink.
> > >> >- Adding IFLA_MACSEC_OFFLOAD to the netlink policy.
> > >> >- Adjusting macsec_get_size.
> > >>
> > >> 4 patches then?
> > >
> > >Ack, I will change the commit message to be imperative and will replace the list
> with a good description.
> > >I still believe it should be a one patch since splitting this could break a bisect
> process.
> >
> > Well, when you split, you have to make sure you don't break bisection,
> > always. Please try to figure that out.
>
> I think this can be split pretty nicely into 3 patches:
> - add IFLA_MACSEC_OFFLOAD to macsec_rtnl_policy (probably for net
> with a Fixes tag on the commit that introduced IFLA_MACSEC_OFFLOAD)
> - add offload to macsec_fill_info/macsec_get_size
> - add IFLA_MACSEC_OFFLOAD support to changelink
>
> The subject of the last patch should also make it clear that it's only adding
> IFLA_MACSEC_OFFLOAD to changelink. As it's written, someone could assume
> there's no support at all via rtnl ops and wonder why this patch isn't doing anything
> to newlink, and whether/why this IFLA_MACSEC_OFFLOAD already exists.
Ack , I will split the patch an send the new patches.
> --
> Sabrina