Message ID | 966968bda15a7128a381b589329184dfea3e0548.1695471387.git.christophe.jaillet@wanadoo.fr |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:cae8:0:b0:403:3b70:6f57 with SMTP id r8csp211977vqu; Sat, 23 Sep 2023 07:34:38 -0700 (PDT) X-Google-Smtp-Source: AGHT+IH1A8KJ2UyW9ijtsw75JGE03K4InDTohT/fw8XxdVITP/9eFbLI+wy+vjlU0uiQjT9pFSBE X-Received: by 2002:a17:902:8f97:b0:1c5:e9a8:dbc0 with SMTP id z23-20020a1709028f9700b001c5e9a8dbc0mr2053068plo.51.1695479678141; Sat, 23 Sep 2023 07:34:38 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1695479678; cv=none; d=google.com; s=arc-20160816; b=ldh5H2q6xF1ATdRoxZCZx0sjyzmbkjO2dzqZFsYCyEDVXgrx2WO31SkitO9L9pKPHb 9Dhl3u63ZgeWjwv8wl/xWt9xUM7Itfd07qS3rywVnnRj0iV4w+xvO66OsWKRCfj0b8FT CcmHDKTgtVo2H4AI8uXg/LisdNxyzSzCD9sFWkdPXYRfFKVU6U4SE6pZ6bz/X44ze86Q uKeFALLWLMiNSsEYF3QSXsHL9BEigmoCKpDntdDkLINHkIeilX/n08XA0Hql2Csd9u// 6WtldEHBvibCFFtV7W5Pl9VN1vkN3amDnXjd140f3wWAP97Xfp+Am4T9ZZLbfyLHwKFp Fifg== 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=MZ+x3+dMxPzCGceJF8/93Shpse4vUvBYF6v0b3D2kfE=; fh=P2LlrCXt6+gJz4MxCF/oA6XHLArZCLypbNPcnF1eVUE=; b=CXbEcFANa1MYEmDOqQo1XHI9tPidm1IVw08NMIYpJLH7CRpz+mNqyZ7QyGSRR+0KZK CQYGOMl+JpPVQ4zvX0Z0RI6RA894rT3jgF5IHU/n0cm/W4Yhl0xlm/NpTLhCTQe40ixM fOA3MD1PHdnzBmP6MnIMLfS/f+aUP6nQsMP+oYhml6/kz7+L112EaIFm5BmHRdxkUfeK QkML3Z5BoMGLHhIec3+SP9l9yt02BLxnKmy+FZ+dSCNOzq9mjVDArdaHE3CYJd6vgpHk UOegsLphhkQCJX9+2C/63Stm3tbmHn1LR0A3c2ry8+ANAbscpYceIvfk8jfWxuesIbai NIXw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@wanadoo.fr header.s=t20230301 header.b=pWrkqoYW; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=wanadoo.fr Received: from fry.vger.email (fry.vger.email. [23.128.96.38]) by mx.google.com with ESMTPS id jh19-20020a170903329300b001bba90f8b73si5761351plb.78.2023.09.23.07.34.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Sat, 23 Sep 2023 07:34:37 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) client-ip=23.128.96.38; Authentication-Results: mx.google.com; dkim=pass header.i=@wanadoo.fr header.s=t20230301 header.b=pWrkqoYW; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.38 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=wanadoo.fr Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by fry.vger.email (Postfix) with ESMTP id 6F9C980FCC93; Sat, 23 Sep 2023 05:17:30 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at fry.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231343AbjIWMRU (ORCPT <rfc822;chrisfriedt@gmail.com> + 29 others); Sat, 23 Sep 2023 08:17:20 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33978 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S231405AbjIWMRT (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Sat, 23 Sep 2023 08:17:19 -0400 Received: from smtp.smtpout.orange.fr (smtp-24.smtpout.orange.fr [80.12.242.24]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2C804199 for <linux-kernel@vger.kernel.org>; Sat, 23 Sep 2023 05:17:11 -0700 (PDT) Received: from pop-os.home ([86.243.2.178]) by smtp.orange.fr with ESMTPA id k1ZLq0FE4ttXNk1ZLq9cSW; Sat, 23 Sep 2023 14:17:10 +0200 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=wanadoo.fr; s=t20230301; t=1695471430; bh=MZ+x3+dMxPzCGceJF8/93Shpse4vUvBYF6v0b3D2kfE=; h=From:To:Cc:Subject:Date; b=pWrkqoYWrs+z0eZtol7HHfGiyoEXhV/dOMobQxnDEhy6fd6vb/VVWl+xgiAj4FXnz 4PvjXGd+S2o3TMbt4Ut7bO62rSgDkrpmxKcfbTt9ZBgTg+WQ1X+cRAIwr8ZPvxx+TL unF0hAolnIG5mmLcu0WkE4eqgFOYmtiwp+867RTzbgveloEchATdFmfzfB359NaQHC y2nJVmMr/0cV50aDKXwfsgFzViVX8Rul99nI4gR5DjZMggg/qrfGY34L7oFPnBMrvX SvRULTOQBPjgKYToaahMrfT2APagS9YohRwfVWOAZ9IrPna8uYXnvjEX6NrQaznMMa +RLYynY5dib/w== X-ME-Helo: pop-os.home X-ME-Auth: Y2hyaXN0b3BoZS5qYWlsbGV0QHdhbmFkb28uZnI= X-ME-Date: Sat, 23 Sep 2023 14:17:10 +0200 X-ME-IP: 86.243.2.178 From: Christophe JAILLET <christophe.jaillet@wanadoo.fr> To: Jesse Brandeburg <jesse.brandeburg@intel.com>, Tony Nguyen <anthony.l.nguyen@intel.com>, "David S. Miller" <davem@davemloft.net>, Eric Dumazet <edumazet@google.com>, Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com> Cc: linux-kernel@vger.kernel.org, kernel-janitors@vger.kernel.org, Christophe JAILLET <christophe.jaillet@wanadoo.fr>, intel-wired-lan@lists.osuosl.org, netdev@vger.kernel.org Subject: [PATCH net-next] iavf: Avoid a memory allocation in iavf_print_link_message() Date: Sat, 23 Sep 2023 14:17:05 +0200 Message-Id: <966968bda15a7128a381b589329184dfea3e0548.1695471387.git.christophe.jaillet@wanadoo.fr> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=2.7 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, RCVD_IN_SBL_CSS,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on fry.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (fry.vger.email [0.0.0.0]); Sat, 23 Sep 2023 05:17:30 -0700 (PDT) X-Spam-Level: ** X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1777839299040806735 X-GMAIL-MSGID: 1777839299040806735 |
Series |
[net-next] iavf: Avoid a memory allocation in iavf_print_link_message()
|
|
Commit Message
Christophe JAILLET
Sept. 23, 2023, 12:17 p.m. UTC
IAVF_MAX_SPEED_STRLEN is only 13 and 'speed' is allocated and freed within
iavf_print_link_message().
'speed' is only used with some snprintf() and netdev_info() calls.
So there is no real use to kzalloc()/free() it. Use the stack instead.
This saves a memory allocation.
Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr>
---
drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
Comments
On 9/23/23 14:17, Christophe JAILLET wrote: > IAVF_MAX_SPEED_STRLEN is only 13 and 'speed' is allocated and freed within > iavf_print_link_message(). > > 'speed' is only used with some snprintf() and netdev_info() calls. > > So there is no real use to kzalloc()/free() it. Use the stack instead. > This saves a memory allocation. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > index 8ce6389b5815..980dc69d7fbe 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > @@ -1389,18 +1389,14 @@ void iavf_disable_vlan_insertion_v2(struct iavf_adapter *adapter, u16 tpid) > static void iavf_print_link_message(struct iavf_adapter *adapter) > { > struct net_device *netdev = adapter->netdev; > + char speed[IAVF_MAX_SPEED_STRLEN]; > int link_speed_mbps; > - char *speed; > > if (!adapter->link_up) { > netdev_info(netdev, "NIC Link is Down\n"); > return; > } > > - speed = kzalloc(IAVF_MAX_SPEED_STRLEN, GFP_KERNEL); > - if (!speed) > - return; > - > if (ADV_LINK_SUPPORT(adapter)) { > link_speed_mbps = adapter->link_speed_mbps; > goto print_link_msg; > @@ -1452,7 +1448,6 @@ static void iavf_print_link_message(struct iavf_adapter *adapter) > } > > netdev_info(netdev, "NIC Link is Up Speed is %s Full Duplex\n", speed); > - kfree(speed); > } > > /** Looks fine, thanks! Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com> I know that Jesse is fixing snprintf() calls currently, but I bet it's not conflicting.
On 9/23/2023 5:17 AM, Christophe JAILLET wrote: > IAVF_MAX_SPEED_STRLEN is only 13 and 'speed' is allocated and freed within > iavf_print_link_message(). > > 'speed' is only used with some snprintf() and netdev_info() calls. > > So there is no real use to kzalloc()/free() it. Use the stack instead. > This saves a memory allocation. > > Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> > --- > drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 7 +------ > 1 file changed, 1 insertion(+), 6 deletions(-) > > diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > index 8ce6389b5815..980dc69d7fbe 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > @@ -1389,18 +1389,14 @@ void iavf_disable_vlan_insertion_v2(struct iavf_adapter *adapter, u16 tpid) > static void iavf_print_link_message(struct iavf_adapter *adapter) > { > struct net_device *netdev = adapter->netdev; > + char speed[IAVF_MAX_SPEED_STRLEN]; > int link_speed_mbps; > - char *speed; > > if (!adapter->link_up) { > netdev_info(netdev, "NIC Link is Down\n"); > return; > } > > - speed = kzalloc(IAVF_MAX_SPEED_STRLEN, GFP_KERNEL); > - if (!speed) > - return; > - > if (ADV_LINK_SUPPORT(adapter)) { > link_speed_mbps = adapter->link_speed_mbps; > goto print_link_msg; > @@ -1452,7 +1448,6 @@ static void iavf_print_link_message(struct iavf_adapter *adapter) > } > > netdev_info(netdev, "NIC Link is Up Speed is %s Full Duplex\n", speed); > - kfree(speed); > } > > /** Hi Christophe! I had a slightly different proposal that gets rid of all the -Wformat=2 warnings in this code by using kasprintf to handle the varying string lengths. any thoughts about this instead and drop yours? I'm less worried about the "extra allocation" here in this function since it's slow path, and the same comment applies to your patch as well. your patch still shows these errors > drivers/net/ethernet/intel/iavf/iavf_virtchnl.c: In function ‘iavf_virtchnl_completion’: > drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1446:60: warning: ‘%s’ directive output may be truncated writing 4 bytes into a region of size between 1 and 11 [-Wformat-truncation=] > 1446 | snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s", > | ^~ > 1447 | link_speed_mbps, "Mbps"); > | ~~~~~~ > In function ‘iavf_print_link_message’, > inlined from ‘iavf_virtchnl_completion’ at drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1965:4: > drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1446:17: note: ‘snprintf’ output between 7 and 17 bytes into a destination of size 13 > 1446 | snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s", > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > 1447 | link_speed_mbps, "Mbps"); > | ~~~~~~~~~~~~~~~~~~~~~~~~ <my iavf patch pasted as a quote so my mail client won't wrap the lines...> > diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > index 8ce6389b5815..82b84a93bcc8 100644 > --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c > @@ -1378,8 +1378,6 @@ void iavf_disable_vlan_insertion_v2(struct iavf_adapter *adapter, u16 tpid) > VIRTCHNL_OP_DISABLE_VLAN_INSERTION_V2); > } > > -#define IAVF_MAX_SPEED_STRLEN 13 > - > /** > * iavf_print_link_message - print link up or down > * @adapter: adapter structure > @@ -1397,10 +1395,6 @@ static void iavf_print_link_message(struct iavf_adapter *adapter) > return; > } > > - speed = kzalloc(IAVF_MAX_SPEED_STRLEN, GFP_KERNEL); > - if (!speed) > - return; > - > if (ADV_LINK_SUPPORT(adapter)) { > link_speed_mbps = adapter->link_speed_mbps; > goto print_link_msg; > @@ -1438,17 +1432,17 @@ static void iavf_print_link_message(struct iavf_adapter *adapter) > > print_link_msg: > if (link_speed_mbps > SPEED_1000) { > - if (link_speed_mbps == SPEED_2500) > - snprintf(speed, IAVF_MAX_SPEED_STRLEN, "2.5 Gbps"); > - else > + if (link_speed_mbps == SPEED_2500) { > + speed = kasprintf(GFP_KERNEL, "%s", "2.5 Gbps"); > + } else { > /* convert to Gbps inline */ > - snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s", > - link_speed_mbps / 1000, "Gbps"); > + speed = kasprintf(GFP_KERNEL, "%d Gbps", > + link_speed_mbps / 1000); > + } > } else if (link_speed_mbps == SPEED_UNKNOWN) { > - snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%s", "Unknown Mbps"); > + speed = kasprintf(GFP_KERNEL, "%s", "Unknown Mbps"); > } else { > - snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s", > - link_speed_mbps, "Mbps"); > + speed = kasprintf(GFP_KERNEL, "%d Mbps", link_speed_mbps); > } > > netdev_info(netdev, "NIC Link is Up Speed is %s Full Duplex\n", speed);
Le 03/10/2023 à 19:14, Jesse Brandeburg a écrit : > On 9/23/2023 5:17 AM, Christophe JAILLET wrote: >> IAVF_MAX_SPEED_STRLEN is only 13 and 'speed' is allocated and freed within >> iavf_print_link_message(). >> >> 'speed' is only used with some snprintf() and netdev_info() calls. >> >> So there is no real use to kzalloc()/free() it. Use the stack instead. >> This saves a memory allocation. >> >> Signed-off-by: Christophe JAILLET <christophe.jaillet@wanadoo.fr> >> --- >> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c | 7 +------ >> 1 file changed, 1 insertion(+), 6 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c >> index 8ce6389b5815..980dc69d7fbe 100644 >> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c >> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c >> @@ -1389,18 +1389,14 @@ void iavf_disable_vlan_insertion_v2(struct iavf_adapter *adapter, u16 tpid) >> static void iavf_print_link_message(struct iavf_adapter *adapter) >> { >> struct net_device *netdev = adapter->netdev; >> + char speed[IAVF_MAX_SPEED_STRLEN]; >> int link_speed_mbps; >> - char *speed; >> >> if (!adapter->link_up) { >> netdev_info(netdev, "NIC Link is Down\n"); >> return; >> } >> >> - speed = kzalloc(IAVF_MAX_SPEED_STRLEN, GFP_KERNEL); >> - if (!speed) >> - return; >> - >> if (ADV_LINK_SUPPORT(adapter)) { >> link_speed_mbps = adapter->link_speed_mbps; >> goto print_link_msg; >> @@ -1452,7 +1448,6 @@ static void iavf_print_link_message(struct iavf_adapter *adapter) >> } >> >> netdev_info(netdev, "NIC Link is Up Speed is %s Full Duplex\n", speed); >> - kfree(speed); >> } >> >> /** > > Hi Christophe! > > I had a slightly different proposal that gets rid of all the -Wformat=2 > warnings in this code by using kasprintf to handle the varying string > lengths. > > any thoughts about this instead and drop yours? I'm less worried about > the "extra allocation" here in this function since it's slow path, and > the same comment applies to your patch as well. kasprintf() is much better. > > your patch still shows these errors I built-tested the patch before sending, so this is strange. However, I got a similar feedback from Greg KH and the "kernel test robot" for another similar patch. What version of gcc do you use? I use 12.3.0, and I suspect that the value range algorithm or how the diagnostic is done has been improved in recent gcc. The other report was from 11.3.0. CJ >> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c: In function ‘iavf_virtchnl_completion’: >> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1446:60: warning: ‘%s’ directive output may be truncated writing 4 bytes into a region of size between 1 and 11 [-Wformat-truncation=] >> 1446 | snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s", >> | ^~ >> 1447 | link_speed_mbps, "Mbps"); >> | ~~~~~~ >> In function ‘iavf_print_link_message’, >> inlined from ‘iavf_virtchnl_completion’ at drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1965:4: >> drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1446:17: note: ‘snprintf’ output between 7 and 17 bytes into a destination of size 13 >> 1446 | snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s", >> | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ >> 1447 | link_speed_mbps, "Mbps"); >> | ~~~~~~~~~~~~~~~~~~~~~~~~ > > > <my iavf patch pasted as a quote so my mail client won't wrap the lines...> > > >> diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c >> index 8ce6389b5815..82b84a93bcc8 100644 >> --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c >> +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c >> @@ -1378,8 +1378,6 @@ void iavf_disable_vlan_insertion_v2(struct iavf_adapter *adapter, u16 tpid) >> VIRTCHNL_OP_DISABLE_VLAN_INSERTION_V2); >> } >> >> -#define IAVF_MAX_SPEED_STRLEN 13 >> - >> /** >> * iavf_print_link_message - print link up or down >> * @adapter: adapter structure >> @@ -1397,10 +1395,6 @@ static void iavf_print_link_message(struct iavf_adapter *adapter) >> return; >> } >> >> - speed = kzalloc(IAVF_MAX_SPEED_STRLEN, GFP_KERNEL); >> - if (!speed) >> - return; >> - >> if (ADV_LINK_SUPPORT(adapter)) { >> link_speed_mbps = adapter->link_speed_mbps; >> goto print_link_msg; >> @@ -1438,17 +1432,17 @@ static void iavf_print_link_message(struct iavf_adapter *adapter) >> >> print_link_msg: >> if (link_speed_mbps > SPEED_1000) { >> - if (link_speed_mbps == SPEED_2500) >> - snprintf(speed, IAVF_MAX_SPEED_STRLEN, "2.5 Gbps"); >> - else >> + if (link_speed_mbps == SPEED_2500) { >> + speed = kasprintf(GFP_KERNEL, "%s", "2.5 Gbps"); >> + } else { >> /* convert to Gbps inline */ >> - snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s", >> - link_speed_mbps / 1000, "Gbps"); >> + speed = kasprintf(GFP_KERNEL, "%d Gbps", >> + link_speed_mbps / 1000); >> + } >> } else if (link_speed_mbps == SPEED_UNKNOWN) { >> - snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%s", "Unknown Mbps"); >> + speed = kasprintf(GFP_KERNEL, "%s", "Unknown Mbps"); >> } else { >> - snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s", >> - link_speed_mbps, "Mbps"); >> + speed = kasprintf(GFP_KERNEL, "%d Mbps", link_speed_mbps); >> } >> >> netdev_info(netdev, "NIC Link is Up Speed is %s Full Duplex\n", speed); > > >
On 10/3/2023 1:33 PM, Christophe JAILLET wrote: > kasprintf() is much better. cool! I just sent the patches and cc'd you earlier today. >> >> your patch still shows these errors > > I built-tested the patch before sending, so this is strange. > > However, I got a similar feedback from Greg KH and the "kernel test > robot" for another similar patch. > > What version of gcc do you use? > I use 12.3.0, and I suspect that the value range algorithm or how the > diagnostic is done has been improved in recent gcc. Fedora gcc 12.3.1, with W=1 flag gcc version 12.3.1 20230508 (Red Hat 12.3.1-1) (GCC) [linux]$ make W=1 M=drivers/net/ethernet/intel/iavf CC [M] drivers/net/ethernet/intel/iavf/iavf_main.o CC [M] drivers/net/ethernet/intel/iavf/iavf_ethtool.o CC [M] drivers/net/ethernet/intel/iavf/iavf_virtchnl.o CC [M] drivers/net/ethernet/intel/iavf/iavf_fdir.o CC [M] drivers/net/ethernet/intel/iavf/iavf_adv_rss.o CC [M] drivers/net/ethernet/intel/iavf/iavf_txrx.o CC [M] drivers/net/ethernet/intel/iavf/iavf_common.o CC [M] drivers/net/ethernet/intel/iavf/iavf_adminq.o CC [M] drivers/net/ethernet/intel/iavf/iavf_client.o drivers/net/ethernet/intel/iavf/iavf_virtchnl.c: In function ‘iavf_virtchnl_completion’: drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1446:60: warning: ‘%s’ directive output may be truncated writing 4 bytes into a region of size between 1 and 11 [-Wformat-truncation=] 1446 | snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s", | ^~ 1447 | link_speed_mbps, "Mbps"); | ~~~~~~ In function ‘iavf_print_link_message’, inlined from ‘iavf_virtchnl_completion’ at drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1965:4: drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1446:17: note: ‘snprintf’ output between 7 and 17 bytes into a destination of size 13 1446 | snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s", | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ 1447 | link_speed_mbps, "Mbps"); | ~~~~~~~~~~~~~~~~~~~~~~~~
On Tue, Oct 03, 2023 at 04:01:18PM -0700, Jesse Brandeburg wrote: > On 10/3/2023 1:33 PM, Christophe JAILLET wrote: > > kasprintf() is much better. > > cool! I just sent the patches and cc'd you earlier today. > > > > > > > your patch still shows these errors > > > > I built-tested the patch before sending, so this is strange. > > > > However, I got a similar feedback from Greg KH and the "kernel test > > robot" for another similar patch. > > > > What version of gcc do you use? > > I use 12.3.0, and I suspect that the value range algorithm or how the > > diagnostic is done has been improved in recent gcc. > > Fedora gcc 12.3.1, with W=1 flag > > gcc version 12.3.1 20230508 (Red Hat 12.3.1-1) (GCC) > > [linux]$ make W=1 M=drivers/net/ethernet/intel/iavf > CC [M] drivers/net/ethernet/intel/iavf/iavf_main.o > CC [M] drivers/net/ethernet/intel/iavf/iavf_ethtool.o > CC [M] drivers/net/ethernet/intel/iavf/iavf_virtchnl.o > CC [M] drivers/net/ethernet/intel/iavf/iavf_fdir.o > CC [M] drivers/net/ethernet/intel/iavf/iavf_adv_rss.o > CC [M] drivers/net/ethernet/intel/iavf/iavf_txrx.o > CC [M] drivers/net/ethernet/intel/iavf/iavf_common.o > CC [M] drivers/net/ethernet/intel/iavf/iavf_adminq.o > CC [M] drivers/net/ethernet/intel/iavf/iavf_client.o > drivers/net/ethernet/intel/iavf/iavf_virtchnl.c: In function > ‘iavf_virtchnl_completion’: > drivers/net/ethernet/intel/iavf/iavf_virtchnl.c:1446:60: warning: ‘%s’ > directive output may be truncated writing 4 bytes into a region of size > between 1 and 11 [-Wformat-truncation=] > 1446 | snprintf(speed, IAVF_MAX_SPEED_STRLEN, "%d %s", > | ^~ > 1447 | link_speed_mbps, "Mbps"); > | ~~~~~~ GCC is kind of crap at static analysis, right? Smatch would know that this at most 11 characters long. It's kind of laziness for GCC to print this warning. If you complained to me about a false positive like this in Smatch I would at least think about various ways to silence it. But I probably wouldn't write a check for this anyway because I don't view truncating strings as a note worthy bug... Smatch also gets stuff wrong, but in that case I just always encourage people to mark the warning as old news and move on. Only new warnings are interesting. I feel like as we incorporate more and more static analysis into our processes we're going to have to give up on trying to keep every static checker happy. regards, dan carpenter
diff --git a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c index 8ce6389b5815..980dc69d7fbe 100644 --- a/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c +++ b/drivers/net/ethernet/intel/iavf/iavf_virtchnl.c @@ -1389,18 +1389,14 @@ void iavf_disable_vlan_insertion_v2(struct iavf_adapter *adapter, u16 tpid) static void iavf_print_link_message(struct iavf_adapter *adapter) { struct net_device *netdev = adapter->netdev; + char speed[IAVF_MAX_SPEED_STRLEN]; int link_speed_mbps; - char *speed; if (!adapter->link_up) { netdev_info(netdev, "NIC Link is Down\n"); return; } - speed = kzalloc(IAVF_MAX_SPEED_STRLEN, GFP_KERNEL); - if (!speed) - return; - if (ADV_LINK_SUPPORT(adapter)) { link_speed_mbps = adapter->link_speed_mbps; goto print_link_msg; @@ -1452,7 +1448,6 @@ static void iavf_print_link_message(struct iavf_adapter *adapter) } netdev_info(netdev, "NIC Link is Up Speed is %s Full Duplex\n", speed); - kfree(speed); } /**