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:   Tue, 6 Dec 2022 14:35:23 +0100
From:   Jiri Pirko <jiri@resnulli.us>
To:     Emeel Hakim <ehakim@nvidia.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Raed Salem <raeds@nvidia.com>,
        "davem@davemloft.net" <davem@davemloft.net>,
        "edumazet@google.com" <edumazet@google.com>,
        "kuba@kernel.org" <kuba@kernel.org>,
        "pabeni@redhat.com" <pabeni@redhat.com>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "sd@queasysnail.net" <sd@queasysnail.net>
Subject: Re: [PATCH net-next v2] macsec: Add support for IFLA_MACSEC_OFFLOAD
 in the netlink layer
Message-ID: <Y49FGzwBdyC/xHxH@nanopsycho>
References: <20221206085757.5816-1-ehakim@nvidia.com>
 <Y48IVReEUBmQza81@nanopsycho>
 <IA1PR12MB6353D358E112EE09C4DD770CAB1B9@IA1PR12MB6353.namprd12.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <IA1PR12MB6353D358E112EE09C4DD770CAB1B9@IA1PR12MB6353.namprd12.prod.outlook.com>
Precedence: bulk

Tue, Dec 06, 2022 at 01:31:54PM CET, ehakim@nvidia.com wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <jiri@resnulli.us>
>> Sent: Tuesday, 6 December 2022 11:16
>> To: Emeel Hakim <ehakim@nvidia.com>
>> Cc: linux-kernel@vger.kernel.org; Raed Salem <raeds@nvidia.com>;
>> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
>> pabeni@redhat.com; netdev@vger.kernel.org; sd@queasysnail.net
>> 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, ehakim@nvidia.com wrote:
>> >From: Emeel Hakim <ehakim@nvidia.com>
>> >
>> >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 <raeds@nvidia.com>
>> >Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
>> >---
>> >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
>> >