From: "Neftin, Sasha" <sasha.neftin@intel.com> To: Nathan Chancellor <natechancellor@gmail.com> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com>, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, clang-built-linux@googlegroups.com, intel-wired-lan@lists.osuosl.org, Jakub Kicinski <kuba@kernel.org>, "David S. Miller" <davem@davemloft.net>, "Lifshits, Vitaly" <vitaly.lifshits@intel.com>, "Nguyen, Anthony L" <anthony.l.nguyen@intel.com>, "Neftin, Sasha" <sasha.neftin@intel.com> Subject: Re: [Intel-wired-lan] [PATCH] igc: Do not use link uninitialized in igc_check_for_copper_link Date: Sun, 19 Jul 2020 08:34:12 +0300 [thread overview] Message-ID: <96131050-57e4-934a-3d9a-a285f234e633@intel.com> (raw) In-Reply-To: <20200717021235.GA4098394@ubuntu-n2-xlarge-x86> On 7/17/2020 05:12, Nathan Chancellor wrote: > On Thu, Jul 16, 2020 at 07:29:03PM +0300, Neftin, Sasha wrote: >> On 7/16/2020 07:49, Nathan Chancellor wrote: >>> Clang warns: >>> >> Hello Nathan, >> Thanks for tracking our code.Please, look at https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20200709073416.14126-1-sasha.neftin@intel.com/ >> - I hope this patch already address this Clang warns - please, let me know. > > I have not explicitly tested it but it seems obvious that it will. Let's > go with that. > Good Nathan, let's go with my https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20200709073416.14126-1-sasha.neftin@intel.com/ and let me know if warning still generated. Thanks, Sasha > Cheers, > Nathan > >>> drivers/net/ethernet/intel/igc/igc_mac.c:374:6: warning: variable 'link' >>> is used uninitialized whenever 'if' condition is true >>> [-Wsometimes-uninitialized] >>> if (!mac->get_link_status) { >>> ^~~~~~~~~~~~~~~~~~~~~ >>> drivers/net/ethernet/intel/igc/igc_mac.c:424:33: note: uninitialized use >>> occurs here >>> ret_val = igc_set_ltr_i225(hw, link); >>> ^~~~ >>> drivers/net/ethernet/intel/igc/igc_mac.c:374:2: note: remove the 'if' if >>> its condition is always false >>> if (!mac->get_link_status) { >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> drivers/net/ethernet/intel/igc/igc_mac.c:367:11: note: initialize the >>> variable 'link' to silence this warning >>> bool link; >>> ^ >>> = 0 >>> 1 warning generated. >>> >>> It is not wrong, link is only uninitialized after this through >>> igc_phy_has_link. Presumably, if we skip the majority of this function >>> when get_link_status is false, we should skip calling igc_set_ltr_i225 >>> as well. Just directly return 0 in this case, rather than bothering with >>> adding another label or initializing link in the if statement. >>> >>> Fixes: 707abf069548 ("igc: Add initial LTR support") >>> Link: https://github.com/ClangBuiltLinux/linux/issues/1095 >>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> >>> --- >>> drivers/net/ethernet/intel/igc/igc_mac.c | 6 ++---- >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/intel/igc/igc_mac.c b/drivers/net/ethernet/intel/igc/igc_mac.c >>> index b47e7b0a6398..26e3c56a4a8b 100644 >>> --- a/drivers/net/ethernet/intel/igc/igc_mac.c >>> +++ b/drivers/net/ethernet/intel/igc/igc_mac.c >>> @@ -371,10 +371,8 @@ s32 igc_check_for_copper_link(struct igc_hw *hw) >>> * get_link_status flag is set upon receiving a Link Status >>> * Change or Rx Sequence Error interrupt. >>> */ >>> - if (!mac->get_link_status) { >>> - ret_val = 0; >>> - goto out; >>> - } >>> + if (!mac->get_link_status) >>> + return 0; >>> /* First we want to see if the MII Status Register reports >>> * link. If so, then we want to get the current speed/duplex >>> >>> base-commit: ca0e494af5edb59002665bf12871e94b4163a257 >>> >> Thanks, >> Sasha
WARNING: multiple messages have this Message-ID (diff)
From: Neftin, Sasha <sasha.neftin@intel.com> To: intel-wired-lan@osuosl.org Subject: [Intel-wired-lan] [PATCH] igc: Do not use link uninitialized in igc_check_for_copper_link Date: Sun, 19 Jul 2020 08:34:12 +0300 [thread overview] Message-ID: <96131050-57e4-934a-3d9a-a285f234e633@intel.com> (raw) In-Reply-To: <20200717021235.GA4098394@ubuntu-n2-xlarge-x86> On 7/17/2020 05:12, Nathan Chancellor wrote: > On Thu, Jul 16, 2020 at 07:29:03PM +0300, Neftin, Sasha wrote: >> On 7/16/2020 07:49, Nathan Chancellor wrote: >>> Clang warns: >>> >> Hello Nathan, >> Thanks for tracking our code.Please, look at https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20200709073416.14126-1-sasha.neftin at intel.com/ >> - I hope this patch already address this Clang warns - please, let me know. > > I have not explicitly tested it but it seems obvious that it will. Let's > go with that. > Good Nathan, let's go with my https://patchwork.ozlabs.org/project/intel-wired-lan/patch/20200709073416.14126-1-sasha.neftin at intel.com/ and let me know if warning still generated. Thanks, Sasha > Cheers, > Nathan > >>> drivers/net/ethernet/intel/igc/igc_mac.c:374:6: warning: variable 'link' >>> is used uninitialized whenever 'if' condition is true >>> [-Wsometimes-uninitialized] >>> if (!mac->get_link_status) { >>> ^~~~~~~~~~~~~~~~~~~~~ >>> drivers/net/ethernet/intel/igc/igc_mac.c:424:33: note: uninitialized use >>> occurs here >>> ret_val = igc_set_ltr_i225(hw, link); >>> ^~~~ >>> drivers/net/ethernet/intel/igc/igc_mac.c:374:2: note: remove the 'if' if >>> its condition is always false >>> if (!mac->get_link_status) { >>> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~ >>> drivers/net/ethernet/intel/igc/igc_mac.c:367:11: note: initialize the >>> variable 'link' to silence this warning >>> bool link; >>> ^ >>> = 0 >>> 1 warning generated. >>> >>> It is not wrong, link is only uninitialized after this through >>> igc_phy_has_link. Presumably, if we skip the majority of this function >>> when get_link_status is false, we should skip calling igc_set_ltr_i225 >>> as well. Just directly return 0 in this case, rather than bothering with >>> adding another label or initializing link in the if statement. >>> >>> Fixes: 707abf069548 ("igc: Add initial LTR support") >>> Link: https://github.com/ClangBuiltLinux/linux/issues/1095 >>> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> >>> --- >>> drivers/net/ethernet/intel/igc/igc_mac.c | 6 ++---- >>> 1 file changed, 2 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/net/ethernet/intel/igc/igc_mac.c b/drivers/net/ethernet/intel/igc/igc_mac.c >>> index b47e7b0a6398..26e3c56a4a8b 100644 >>> --- a/drivers/net/ethernet/intel/igc/igc_mac.c >>> +++ b/drivers/net/ethernet/intel/igc/igc_mac.c >>> @@ -371,10 +371,8 @@ s32 igc_check_for_copper_link(struct igc_hw *hw) >>> * get_link_status flag is set upon receiving a Link Status >>> * Change or Rx Sequence Error interrupt. >>> */ >>> - if (!mac->get_link_status) { >>> - ret_val = 0; >>> - goto out; >>> - } >>> + if (!mac->get_link_status) >>> + return 0; >>> /* First we want to see if the MII Status Register reports >>> * link. If so, then we want to get the current speed/duplex >>> >>> base-commit: ca0e494af5edb59002665bf12871e94b4163a257 >>> >> Thanks, >> Sasha
next prev parent reply other threads:[~2020-07-19 5:37 UTC|newest] Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-07-16 4:49 [PATCH] igc: Do not use link uninitialized in igc_check_for_copper_link Nathan Chancellor 2020-07-16 4:49 ` [Intel-wired-lan] " Nathan Chancellor 2020-07-16 16:29 ` Neftin, Sasha 2020-07-16 16:29 ` Neftin, Sasha 2020-07-17 2:12 ` Nathan Chancellor 2020-07-17 2:12 ` Nathan Chancellor 2020-07-19 5:34 ` Neftin, Sasha [this message] 2020-07-19 5:34 ` Neftin, Sasha
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=96131050-57e4-934a-3d9a-a285f234e633@intel.com \ --to=sasha.neftin@intel.com \ --cc=anthony.l.nguyen@intel.com \ --cc=clang-built-linux@googlegroups.com \ --cc=davem@davemloft.net \ --cc=intel-wired-lan@lists.osuosl.org \ --cc=jeffrey.t.kirsher@intel.com \ --cc=kuba@kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=natechancellor@gmail.com \ --cc=netdev@vger.kernel.org \ --cc=vitaly.lifshits@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.