Message ID | 20221110094623.3395670-1-daniel.machon@microchip.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp45553wru; Thu, 10 Nov 2022 01:43:26 -0800 (PST) X-Google-Smtp-Source: AMsMyM51Lntbw8p/WAWIsMKd3fbParnoo0ur0LFSg6JF7oaXED8EXmZJTf1RKjIQSHWQubxmFToo X-Received: by 2002:aa7:cd61:0:b0:461:de5e:ba3b with SMTP id ca1-20020aa7cd61000000b00461de5eba3bmr60473270edb.74.1668073406696; Thu, 10 Nov 2022 01:43:26 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668073406; cv=none; d=google.com; s=arc-20160816; b=jAHuE+RVcmFJv6BseNj3u0A0C/J90dcKFo9GVpFUEv19oq+TJu43z199aRzKaiOA1M 3SAIfAGkZVP7imMmD0ox3iEueotP4w0reaRIzL8gSLsUAdP1UGXofHHgkQlVWM2Os0rO e3kRH9xCk3PSdn8H9IL0FoS5nM+4REMGY+x5YQdp7bSr8Fulu/9W/pzXvAyGoHZwmyRM R9jApRfslVjHYN54vJlCU7IAt794K85BHbcGBTw98RYXCqqaLy88lM0ybxBv+2zk9Vl5 cvNXgfGEp6PGH0BvGSRB/1xIcgoIJOzl7vCZLxpmlA+5NC66UxlIRYb1bEsQF0aX3dxh orlw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=bjCKfjzAwy8Wd4b5TXBsGJfIo4KPJDjc7Szrpk5EOHA=; b=ucup2AIxC55h0KeO1/BI/Rrod37TtkFTSDRYumJQfLwREDNjv4svr/vrcMwYO7P0X7 ZqybTO7rxCFH1MqCsCqfK2tWqsFCB63hqzTLyyBxaMMlBrG2O5kfZqqaXKaL5Ekw45Ec tuDjy91ktzUK/JdEtiicXOWpYu3RI1rphnIjqWanaQP5+hXBebgLZUb61MZPdVTKyyW9 wLLXZpMbgLRiqKJRByx+QIFJJUKQ+9p37OQPwLnsLdOP8kibjp+Le6eO4Y+zm9m8lcI2 Kj4iufXGURG5qQSafsYqspHMZzhneDGcgSx/oe69LkO2Tv7cGuVTaDa0mIOF1kQHxow6 4Bow== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@microchip.com header.s=mchp header.b=e09k7bYG; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=microchip.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id gb41-20020a170907962900b007ae2299a195si16231581ejc.815.2022.11.10.01.43.01; Thu, 10 Nov 2022 01:43:26 -0800 (PST) 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; Authentication-Results: mx.google.com; dkim=pass header.i=@microchip.com header.s=mchp header.b=e09k7bYG; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=microchip.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229968AbiKJJgq (ORCPT <rfc822;winker.wchi@gmail.com> + 99 others); Thu, 10 Nov 2022 04:36:46 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56268 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229628AbiKJJgp (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 10 Nov 2022 04:36:45 -0500 Received: from esa.microchip.iphmx.com (esa.microchip.iphmx.com [68.232.153.233]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A0FB267F60; Thu, 10 Nov 2022 01:36:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=microchip.com; i=@microchip.com; q=dns/txt; s=mchp; t=1668073004; x=1699609004; h=from:to:cc:subject:date:message-id:mime-version: content-transfer-encoding; bh=bZGinb1GlB+jVorOK6luSyiTkDtoV7cUYIIAN1FmgDQ=; b=e09k7bYG1C0/HJLdkFipjO67rgXaEyKiZfASTWz37A2yA7trbPyTkh7l FR2BnlwcAxMFf+QRgI7VkrRZxUrPQBHyVnuJ+T3zVUVv038cDuehAT+wf uzxqwdTI4lXdrFduj6bpWs2Rb3lgmgb/Gc+68DLo6933PkBm7YaKuvCzw yXsA9RSy0PG/h8isyrtuY5RlZH+Kyp0JpFrbM0PHlb4lW8PGxnu4fpRh6 UswcRjX97cjmc0t782CR/CK4DIH22uBD3RH4WxEgbIxv/aQHwP2U65We7 O/Mu/PJFpdFD3tGDoat6dJpPHNG80dZh6MHkhcPsqvGB0xzGDcu2Sv5UK A==; X-IronPort-AV: E=Sophos;i="5.96,153,1665471600"; d="scan'208";a="188462126" Received: from unknown (HELO email.microchip.com) ([170.129.1.10]) by esa3.microchip.iphmx.com with ESMTP/TLS/AES256-SHA256; 10 Nov 2022 02:36:43 -0700 Received: from chn-vm-ex04.mchp-main.com (10.10.85.152) by chn-vm-ex03.mchp-main.com (10.10.85.151) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.12; Thu, 10 Nov 2022 02:36:43 -0700 Received: from DEN-LT-70577.microchip.com (10.10.115.15) by chn-vm-ex04.mchp-main.com (10.10.85.152) with Microsoft SMTP Server id 15.1.2507.12 via Frontend Transport; Thu, 10 Nov 2022 02:36:41 -0700 From: Daniel Machon <daniel.machon@microchip.com> To: <netdev@vger.kernel.org> CC: <davem@davemloft.net>, <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>, <joe@perches.com>, <daniel.machon@microchip.com>, <vladimir.oltean@nxp.com>, <petrm@nvidia.com>, <linux-kernel@vger.kernel.org>, <UNGLinuxDriver@microchip.com>, "kernel test robot" <lkp@intel.com> Subject: [PATCH net-next] net: dcb: move getapptrust to separate function Date: Thu, 10 Nov 2022 10:46:23 +0100 Message-ID: <20221110094623.3395670-1-daniel.machon@microchip.com> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED, RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1749101740771554887?= X-GMAIL-MSGID: =?utf-8?q?1749101740771554887?= |
Series |
[net-next] net: dcb: move getapptrust to separate function
|
|
Commit Message
Daniel Machon
Nov. 10, 2022, 9:46 a.m. UTC
This patch fixes a frame size warning, reported by kernel test robot. >> net/dcb/dcbnl.c:1230:1: warning: the frame size of 1244 bytes is >> larger than 1024 bytes [-Wframe-larger-than=] The getapptrust part of dcbnl_ieee_fill is moved to a separate function, and the selector array is now dynamically allocated, instead of stack allocated. Tested on microchip sparx5 driver. Fixes: 6182d5875c33 ("net: dcb: add new apptrust attribute") Reported-by: kernel test robot <lkp@intel.com> Signed-off-by: Daniel Machon <daniel.machon@microchip.com> --- net/dcb/dcbnl.c | 67 +++++++++++++++++++++++++++++++++---------------- 1 file changed, 45 insertions(+), 22 deletions(-)
Comments
Daniel Machon <daniel.machon@microchip.com> writes: > diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c > index cec0632f96db..3f4d88c1ec78 100644 > --- a/net/dcb/dcbnl.c > +++ b/net/dcb/dcbnl.c > @@ -1060,11 +1060,52 @@ static int dcbnl_build_peer_app(struct net_device *netdev, struct sk_buff* skb, > return err; > } > > +static int dcbnl_getapptrust(struct net_device *netdev, struct sk_buff *skb) > +{ > + const struct dcbnl_rtnl_ops *ops = netdev->dcbnl_ops; > + int nselectors, err; > + u8 *selectors; > + > + selectors = kzalloc(IEEE_8021QAZ_APP_SEL_MAX + 1, GFP_KERNEL); > + if (!selectors) > + return -ENOMEM; > + > + err = ops->dcbnl_getapptrust(netdev, selectors, &nselectors); > + > + if (!err) { > + struct nlattr *apptrust; > + int i; (Maybe consider moving these up to the function scope. This scope business made sense in the generic function, IMHO is not as useful with a focused function like this one.) > + > + err = -EMSGSIZE; > + > + apptrust = nla_nest_start(skb, DCB_ATTR_DCB_APP_TRUST_TABLE); > + if (!apptrust) > + goto nla_put_failure; > + > + for (i = 0; i < nselectors; i++) { > + enum ieee_attrs_app type = > + dcbnl_app_attr_type_get(selectors[i]); Doesn't checkpatch warn about this? There should be a blank line after the variable declaration block. (Even if there wasn't one there in the original code either.) > + err = nla_put_u8(skb, type, selectors[i]); > + if (err) { > + nla_nest_cancel(skb, apptrust); > + goto nla_put_failure; > + } > + } > + nla_nest_end(skb, apptrust); > + } > + > + err = 0; > + > +nla_put_failure: > + kfree(selectors); > + return err; > +} > +
Den Thu, Nov 10, 2022 at 05:30:43PM +0100 skrev Petr Machata: > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > Daniel Machon <daniel.machon@microchip.com> writes: > > > diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c > > index cec0632f96db..3f4d88c1ec78 100644 > > --- a/net/dcb/dcbnl.c > > +++ b/net/dcb/dcbnl.c > > @@ -1060,11 +1060,52 @@ static int dcbnl_build_peer_app(struct net_device *netdev, struct sk_buff* skb, > > return err; > > } > > > > +static int dcbnl_getapptrust(struct net_device *netdev, struct sk_buff *skb) > > +{ > > + const struct dcbnl_rtnl_ops *ops = netdev->dcbnl_ops; > > + int nselectors, err; > > + u8 *selectors; > > + > > + selectors = kzalloc(IEEE_8021QAZ_APP_SEL_MAX + 1, GFP_KERNEL); > > + if (!selectors) > > + return -ENOMEM; > > + > > + err = ops->dcbnl_getapptrust(netdev, selectors, &nselectors); > > + > > + if (!err) { > > + struct nlattr *apptrust; > > + int i; > > (Maybe consider moving these up to the function scope. This scope > business made sense in the generic function, IMHO is not as useful with > a focused function like this one.) I dont mind doing that, however, this 'scope business' is just staying true to the rest of the dcbnl code :-) - that said, I think I agree with your point. > > > + > > + err = -EMSGSIZE; > > + > > + apptrust = nla_nest_start(skb, DCB_ATTR_DCB_APP_TRUST_TABLE); > > + if (!apptrust) > > + goto nla_put_failure; > > + > > + for (i = 0; i < nselectors; i++) { > > + enum ieee_attrs_app type = > > + dcbnl_app_attr_type_get(selectors[i]); > > Doesn't checkpatch warn about this? There should be a blank line after > the variable declaration block. (Even if there wasn't one there in the > original code either.) Nope, no warning. And I think it has something to do with the way the line is split. > > > + err = nla_put_u8(skb, type, selectors[i]); > > + if (err) { > > + nla_nest_cancel(skb, apptrust); > > + goto nla_put_failure; > > + } > > + } > > + nla_nest_end(skb, apptrust); > > + } > > + > > + err = 0; > > + > > +nla_put_failure: > > + kfree(selectors); > > + return err; > > +} > > +
<Daniel.Machon@microchip.com> writes: > Den Thu, Nov 10, 2022 at 05:30:43PM +0100 skrev Petr Machata: >> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe >> >> Daniel Machon <daniel.machon@microchip.com> writes: >> >> > diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c >> > index cec0632f96db..3f4d88c1ec78 100644 >> > --- a/net/dcb/dcbnl.c >> > +++ b/net/dcb/dcbnl.c >> > @@ -1060,11 +1060,52 @@ static int dcbnl_build_peer_app(struct net_device *netdev, struct sk_buff* skb, >> > return err; >> > } >> > >> > +static int dcbnl_getapptrust(struct net_device *netdev, struct sk_buff *skb) >> > +{ >> > + const struct dcbnl_rtnl_ops *ops = netdev->dcbnl_ops; >> > + int nselectors, err; >> > + u8 *selectors; >> > + >> > + selectors = kzalloc(IEEE_8021QAZ_APP_SEL_MAX + 1, GFP_KERNEL); >> > + if (!selectors) >> > + return -ENOMEM; >> > + >> > + err = ops->dcbnl_getapptrust(netdev, selectors, &nselectors); >> > + >> > + if (!err) { >> > + struct nlattr *apptrust; >> > + int i; >> >> (Maybe consider moving these up to the function scope. This scope >> business made sense in the generic function, IMHO is not as useful with >> a focused function like this one.) > > I dont mind doing that, however, this 'scope business' is just staying true > to the rest of the dcbnl code :-) - that said, I think I agree with your > point. > >> >> > + >> > + err = -EMSGSIZE; >> > + >> > + apptrust = nla_nest_start(skb, DCB_ATTR_DCB_APP_TRUST_TABLE); >> > + if (!apptrust) >> > + goto nla_put_failure; >> > + >> > + for (i = 0; i < nselectors; i++) { >> > + enum ieee_attrs_app type = >> > + dcbnl_app_attr_type_get(selectors[i]); >> >> Doesn't checkpatch warn about this? There should be a blank line after >> the variable declaration block. (Even if there wasn't one there in the >> original code either.) > > Nope, no warning. And I think it has something to do with the way the line > is split. OK. I find the code readable just fine, so I'm fine with it as it stands: Reviewed-by: Petr Machata <petrm@nvidia.com>
On Fri, 2022-11-11 at 06:45 +0000, Daniel.Machon@microchip.com wrote: > Den Thu, Nov 10, 2022 at 05:30:43PM +0100 skrev Petr Machata: > > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe > > > > Daniel Machon <daniel.machon@microchip.com> writes: > > > > > diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c > > > index cec0632f96db..3f4d88c1ec78 100644 > > > --- a/net/dcb/dcbnl.c > > > +++ b/net/dcb/dcbnl.c > > > @@ -1060,11 +1060,52 @@ static int dcbnl_build_peer_app(struct net_device *netdev, struct sk_buff* skb, > > > return err; > > > } > > > > > > +static int dcbnl_getapptrust(struct net_device *netdev, struct sk_buff *skb) > > > +{ > > > + const struct dcbnl_rtnl_ops *ops = netdev->dcbnl_ops; > > > + int nselectors, err; > > > + u8 *selectors; > > > + > > > + selectors = kzalloc(IEEE_8021QAZ_APP_SEL_MAX + 1, GFP_KERNEL); > > > + if (!selectors) > > > + return -ENOMEM; > > > + > > > + err = ops->dcbnl_getapptrust(netdev, selectors, &nselectors); > > > + > > > + if (!err) { > > > + struct nlattr *apptrust; > > > + int i; > > > > (Maybe consider moving these up to the function scope. This scope > > business made sense in the generic function, IMHO is not as useful with > > a focused function like this one.) > > I dont mind doing that, however, this 'scope business' is just staying true > to the rest of the dcbnl code :-) - that said, I think I agree with your > point. > > > > > > + > > > + err = -EMSGSIZE; > > > + > > > + apptrust = nla_nest_start(skb, DCB_ATTR_DCB_APP_TRUST_TABLE); > > > + if (!apptrust) > > > + goto nla_put_failure; > > > + > > > + for (i = 0; i < nselectors; i++) { > > > + enum ieee_attrs_app type = > > > + dcbnl_app_attr_type_get(selectors[i]); > > > > Doesn't checkpatch warn about this? There should be a blank line after > > the variable declaration block. (Even if there wasn't one there in the > > original code either.) > > Nope, no warning. And I think it has something to do with the way the line > is split. yup. And style trivia: I suggest adding error types after specific errors, reversing the test and unindenting the code too. Something like: err = ops->dcbnl_getapptrust(netdev, selectors, &nselectors); if (err) { err = 0; goto out; } apptrust = nla_nest_start(skb, DCB_ATTR_DCB_APP_TRUST_TABLE); if (!apptrust) { err = -EMSGSIZE; goto out; } for (i = 0; i < nselectors; i++) { enum ieee_attrs_app type = dcbnl_app_attr_type_get(selectors[i]); err = nla_put_u8(skb, type, selectors[i]); if (err) { nla_nest_cancel(skb, apptrust); goto out; } } nla_nest_end(skb, apptrust); err = 0; out: kfree(selectors); return err; }
> > > > + > > > > + err = -EMSGSIZE; > > > > + > > > > + apptrust = nla_nest_start(skb, DCB_ATTR_DCB_APP_TRUST_TABLE); > > > > + if (!apptrust) > > > > + goto nla_put_failure; > > > > + > > > > + for (i = 0; i < nselectors; i++) { > > > > + enum ieee_attrs_app type = > > > > + dcbnl_app_attr_type_get(selectors[i]); > > > > > > Doesn't checkpatch warn about this? There should be a blank line after > > > the variable declaration block. (Even if there wasn't one there in the > > > original code either.) > > > > Nope, no warning. And I think it has something to do with the way the line > > is split. > > yup. > > And style trivia: > > I suggest adding error types after specific errors, > reversing the test and unindenting the code too. > > Something like: > > err = ops->dcbnl_getapptrust(netdev, selectors, &nselectors); > if (err) { > err = 0; > goto out; > } > > apptrust = nla_nest_start(skb, DCB_ATTR_DCB_APP_TRUST_TABLE); > if (!apptrust) { > err = -EMSGSIZE; > goto out; > } > > for (i = 0; i < nselectors; i++) { > enum ieee_attrs_app type = dcbnl_app_attr_type_get(selectors[i]); > err = nla_put_u8(skb, type, selectors[i]); > if (err) { > nla_nest_cancel(skb, apptrust); > goto out; > } > } > nla_nest_end(skb, apptrust); > > err = 0; > > out: > kfree(selectors); > return err; > } > LGTM. The last err = 0 can even be removed, as I see it. Will submit a new version with the changes suggested. / Daniel
diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c index cec0632f96db..3f4d88c1ec78 100644 --- a/net/dcb/dcbnl.c +++ b/net/dcb/dcbnl.c @@ -1060,11 +1060,52 @@ static int dcbnl_build_peer_app(struct net_device *netdev, struct sk_buff* skb, return err; } +static int dcbnl_getapptrust(struct net_device *netdev, struct sk_buff *skb) +{ + const struct dcbnl_rtnl_ops *ops = netdev->dcbnl_ops; + int nselectors, err; + u8 *selectors; + + selectors = kzalloc(IEEE_8021QAZ_APP_SEL_MAX + 1, GFP_KERNEL); + if (!selectors) + return -ENOMEM; + + err = ops->dcbnl_getapptrust(netdev, selectors, &nselectors); + + if (!err) { + struct nlattr *apptrust; + int i; + + err = -EMSGSIZE; + + apptrust = nla_nest_start(skb, DCB_ATTR_DCB_APP_TRUST_TABLE); + if (!apptrust) + goto nla_put_failure; + + for (i = 0; i < nselectors; i++) { + enum ieee_attrs_app type = + dcbnl_app_attr_type_get(selectors[i]); + err = nla_put_u8(skb, type, selectors[i]); + if (err) { + nla_nest_cancel(skb, apptrust); + goto nla_put_failure; + } + } + nla_nest_end(skb, apptrust); + } + + err = 0; + +nla_put_failure: + kfree(selectors); + return err; +} + /* Handle IEEE 802.1Qaz/802.1Qau/802.1Qbb GET commands. */ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev) { const struct dcbnl_rtnl_ops *ops = netdev->dcbnl_ops; - struct nlattr *ieee, *app, *apptrust; + struct nlattr *ieee, *app; struct dcb_app_type *itr; int dcbx; int err; @@ -1168,27 +1209,9 @@ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev) nla_nest_end(skb, app); if (ops->dcbnl_getapptrust) { - u8 selectors[IEEE_8021QAZ_APP_SEL_MAX + 1] = {0}; - int nselectors, i; - - apptrust = nla_nest_start(skb, DCB_ATTR_DCB_APP_TRUST_TABLE); - if (!apptrust) - return -EMSGSIZE; - - err = ops->dcbnl_getapptrust(netdev, selectors, &nselectors); - if (!err) { - for (i = 0; i < nselectors; i++) { - enum ieee_attrs_app type = - dcbnl_app_attr_type_get(selectors[i]); - err = nla_put_u8(skb, type, selectors[i]); - if (err) { - nla_nest_cancel(skb, apptrust); - return err; - } - } - } - - nla_nest_end(skb, apptrust); + err = dcbnl_getapptrust(netdev, skb); + if (err) + return err; } /* get peer info if available */