* -Wsometimes-uninitialized Clang warning in drivers/net/ethernet/broadcom/bnxt/bnxt.c @ 2019-03-08 0:57 Nathan Chancellor 2019-03-20 19:08 ` Nathan Chancellor 0 siblings, 1 reply; 7+ messages in thread From: Nathan Chancellor @ 2019-03-08 0:57 UTC (permalink / raw) To: Michael Chan, David S. Miller Cc: netdev, linux-kernel, Nick Desaulniers, clang-built-linux Hi all, We are trying to get Clang's -Wsometimes-uninitialized turned on for the kernel as it can catch some bugs that GCC can't. This warning came up: drivers/net/ethernet/broadcom/bnxt/bnxt.c:1612:6: warning: variable 'len' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] if (rxcmp1->rx_cmp_cfa_code_errors_v2 & RX_CMP_L2_ERRORS) { ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/ethernet/broadcom/bnxt/bnxt.c:1703:19: note: uninitialized use occurs here cpr->rx_bytes += len; ^~~ drivers/net/ethernet/broadcom/bnxt/bnxt.c:1612:2: note: remove the 'if' if its condition is always false if (rxcmp1->rx_cmp_cfa_code_errors_v2 & RX_CMP_L2_ERRORS) { ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ drivers/net/ethernet/broadcom/bnxt/bnxt.c:1540:18: note: initialize the variable 'len' to silence this warning unsigned int len; ^ = 0 1 warning generated. It seems like the logical change to make is this; however, I am not sure if this has any other unintended consequences since this is a rather dense function. I would much appreciate your input, especially if there is a better way to fix it. Cheers, Nathan diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index 0bb9d7b3a2b6..7d3d206c2e86 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -1615,7 +1615,7 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr, bnxt_reuse_rx_agg_bufs(cpr, cp_cons, agg_bufs); rc = -EIO; - goto next_rx; + goto next_rx_no_prod_no_len; } len = le32_to_cpu(rxcmp->rx_cmp_len_flags_type) >> RX_CMP_LEN_SHIFT; ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: -Wsometimes-uninitialized Clang warning in drivers/net/ethernet/broadcom/bnxt/bnxt.c 2019-03-08 0:57 -Wsometimes-uninitialized Clang warning in drivers/net/ethernet/broadcom/bnxt/bnxt.c Nathan Chancellor @ 2019-03-20 19:08 ` Nathan Chancellor 2019-03-20 20:41 ` Nick Desaulniers 0 siblings, 1 reply; 7+ messages in thread From: Nathan Chancellor @ 2019-03-20 19:08 UTC (permalink / raw) To: Michael Chan, David S. Miller Cc: netdev, linux-kernel, Nick Desaulniers, clang-built-linux On Thu, Mar 07, 2019 at 05:57:35PM -0700, Nathan Chancellor wrote: > Hi all, > > We are trying to get Clang's -Wsometimes-uninitialized turned on for the > kernel as it can catch some bugs that GCC can't. This warning came up: > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1612:6: warning: variable 'len' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] > if (rxcmp1->rx_cmp_cfa_code_errors_v2 & RX_CMP_L2_ERRORS) { > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1703:19: note: uninitialized use occurs here > cpr->rx_bytes += len; > ^~~ > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1612:2: note: remove the 'if' if its condition is always false > if (rxcmp1->rx_cmp_cfa_code_errors_v2 & RX_CMP_L2_ERRORS) { > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1540:18: note: initialize the variable 'len' to silence this warning > unsigned int len; > ^ > = 0 > 1 warning generated. > > It seems like the logical change to make is this; however, I am not sure > if this has any other unintended consequences since this is a rather > dense function. I would much appreciate your input, especially if there > is a better way to fix it. > > Cheers, > Nathan > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > index 0bb9d7b3a2b6..7d3d206c2e86 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > @@ -1615,7 +1615,7 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr, > bnxt_reuse_rx_agg_bufs(cpr, cp_cons, agg_bufs); > > rc = -EIO; > - goto next_rx; > + goto next_rx_no_prod_no_len; > } > > len = le32_to_cpu(rxcmp->rx_cmp_len_flags_type) >> RX_CMP_LEN_SHIFT; Gentle ping (if there was a response to this, I didn't receive it). I know I sent it in the middle of a merge window so I get if it slipped through the cracks. Thanks, Nathan ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: -Wsometimes-uninitialized Clang warning in drivers/net/ethernet/broadcom/bnxt/bnxt.c 2019-03-20 19:08 ` Nathan Chancellor @ 2019-03-20 20:41 ` Nick Desaulniers 2019-03-22 14:32 ` Arnd Bergmann 0 siblings, 1 reply; 7+ messages in thread From: Nick Desaulniers @ 2019-03-20 20:41 UTC (permalink / raw) To: Nathan Chancellor Cc: Michael Chan, David S. Miller, netdev, LKML, clang-built-linux, eddie.wai, huangjw, prashant, mchan, vasundhara-v.volam + Broadcom folks from commit c0c050c58d84 ("bnxt_en: New Broadcom ethernet driver."). Looks like Michael wrote and is still maintaining the driver. On Wed, Mar 20, 2019 at 12:08 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Thu, Mar 07, 2019 at 05:57:35PM -0700, Nathan Chancellor wrote: > > Hi all, > > > > We are trying to get Clang's -Wsometimes-uninitialized turned on for the > > kernel as it can catch some bugs that GCC can't. This warning came up: > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1612:6: warning: variable 'len' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] > > if (rxcmp1->rx_cmp_cfa_code_errors_v2 & RX_CMP_L2_ERRORS) { > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1703:19: note: uninitialized use occurs here > > cpr->rx_bytes += len; > > ^~~ > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1612:2: note: remove the 'if' if its condition is always false > > if (rxcmp1->rx_cmp_cfa_code_errors_v2 & RX_CMP_L2_ERRORS) { > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1540:18: note: initialize the variable 'len' to silence this warning > > unsigned int len; > > ^ > > = 0 > > 1 warning generated. > > > > It seems like the logical change to make is this; however, I am not sure > > if this has any other unintended consequences since this is a rather > > dense function. I would much appreciate your input, especially if there > > is a better way to fix it. I agree that `goto next_rx_no_prod_no_len` appears to be most correct; though I don't understand why this function is a mix of early return codes, vs setting rc then updating *raw_cons. The alternative is probably zero initializing len, but I'm not sure whether *raw_cons should be updated in that case or not. Thanks for bringing this up and the patch. Sorry for the delay in review. Can folks at Broadcom please clarify? -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: -Wsometimes-uninitialized Clang warning in drivers/net/ethernet/broadcom/bnxt/bnxt.c 2019-03-20 20:41 ` Nick Desaulniers @ 2019-03-22 14:32 ` Arnd Bergmann 2019-04-25 18:14 ` Nathan Chancellor 0 siblings, 1 reply; 7+ messages in thread From: Arnd Bergmann @ 2019-03-22 14:32 UTC (permalink / raw) To: Nick Desaulniers Cc: Nathan Chancellor, Michael Chan, David S. Miller, Networking, LKML, clang-built-linux, Eddie Wai, huangjw, prashant, mchan, vasundhara-v.volam On Wed, Mar 20, 2019 at 9:41 PM 'Nick Desaulniers' via Clang Built Linux <clang-built-linux@googlegroups.com> wrote: > > + Broadcom folks from commit c0c050c58d84 ("bnxt_en: New Broadcom > ethernet driver."). Looks like Michael wrote and is still maintaining > the driver. > > On Wed, Mar 20, 2019 at 12:08 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > On Thu, Mar 07, 2019 at 05:57:35PM -0700, Nathan Chancellor wrote: > > > Hi all, > > > > > > We are trying to get Clang's -Wsometimes-uninitialized turned on for the > > > kernel as it can catch some bugs that GCC can't. This warning came up: > > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1612:6: warning: variable 'len' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] > > > if (rxcmp1->rx_cmp_cfa_code_errors_v2 & RX_CMP_L2_ERRORS) { > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1703:19: note: uninitialized use occurs here > > > cpr->rx_bytes += len; > > > ^~~ > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1612:2: note: remove the 'if' if its condition is always false > > > if (rxcmp1->rx_cmp_cfa_code_errors_v2 & RX_CMP_L2_ERRORS) { > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1540:18: note: initialize the variable 'len' to silence this warning > > > unsigned int len; > > > ^ > > > = 0 > > > 1 warning generated. > > > > > > It seems like the logical change to make is this; however, I am not sure > > > if this has any other unintended consequences since this is a rather > > > dense function. I would much appreciate your input, especially if there > > > is a better way to fix it. > > I agree that `goto next_rx_no_prod_no_len` appears to be most correct; > though I don't understand why this function is a mix of early return > codes, vs setting rc then updating *raw_cons. The alternative is > probably zero initializing len, but I'm not sure whether *raw_cons > should be updated in that case or not. Thanks for bringing this up > and the patch. Sorry for the delay in review. Can folks at Broadcom > please clarify? I also came up with a workaround for this, but did it the other way round: diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c index 0bb9d7b3a2b6..48bdb87574c3 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c @@ -1608,6 +1608,7 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr, } *event |= BNXT_RX_EVENT; + len = le32_to_cpu(rxcmp->rx_cmp_len_flags_type) >> RX_CMP_LEN_SHIFT; rx_buf->data = NULL; if (rxcmp1->rx_cmp_cfa_code_errors_v2 & RX_CMP_L2_ERRORS) { bnxt_reuse_rx_data(rxr, cons, data); @@ -1618,7 +1619,6 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct bnxt_cp_ring_info *cpr, goto next_rx; } - len = le32_to_cpu(rxcmp->rx_cmp_len_flags_type) >> RX_CMP_LEN_SHIFT; dma_addr = rx_buf->mapping; if (bnxt_rx_xdp(bp, rxr, cons, data, &data_ptr, &len, event)) { Presumably one of the two is correct here, but I don't know which one ;-) Arnd ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: -Wsometimes-uninitialized Clang warning in drivers/net/ethernet/broadcom/bnxt/bnxt.c 2019-03-22 14:32 ` Arnd Bergmann @ 2019-04-25 18:14 ` Nathan Chancellor 2019-04-25 18:33 ` Michael Chan 0 siblings, 1 reply; 7+ messages in thread From: Nathan Chancellor @ 2019-04-25 18:14 UTC (permalink / raw) To: Arnd Bergmann Cc: Nick Desaulniers, Michael Chan, David S. Miller, Networking, LKML, clang-built-linux, Eddie Wai, huangjw, prashant, mchan, vasundhara-v.volam Hi Arnd, On Fri, Mar 22, 2019 at 03:32:50PM +0100, Arnd Bergmann wrote: > On Wed, Mar 20, 2019 at 9:41 PM 'Nick Desaulniers' via Clang Built > Linux <clang-built-linux@googlegroups.com> wrote: > > > > + Broadcom folks from commit c0c050c58d84 ("bnxt_en: New Broadcom > > ethernet driver."). Looks like Michael wrote and is still maintaining > > the driver. > > > > On Wed, Mar 20, 2019 at 12:08 PM Nathan Chancellor > > <natechancellor@gmail.com> wrote: > > > > > > On Thu, Mar 07, 2019 at 05:57:35PM -0700, Nathan Chancellor wrote: > > > > Hi all, > > > > > > > > We are trying to get Clang's -Wsometimes-uninitialized turned on for the > > > > kernel as it can catch some bugs that GCC can't. This warning came up: > > > > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1612:6: warning: variable 'len' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] > > > > if (rxcmp1->rx_cmp_cfa_code_errors_v2 & RX_CMP_L2_ERRORS) { > > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1703:19: note: uninitialized use occurs here > > > > cpr->rx_bytes += len; > > > > ^~~ > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1612:2: note: remove the 'if' if its condition is always false > > > > if (rxcmp1->rx_cmp_cfa_code_errors_v2 & RX_CMP_L2_ERRORS) { > > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1540:18: note: initialize the variable 'len' to silence this warning > > > > unsigned int len; > > > > ^ > > > > = 0 > > > > 1 warning generated. > > > > > > > > It seems like the logical change to make is this; however, I am not sure > > > > if this has any other unintended consequences since this is a rather > > > > dense function. I would much appreciate your input, especially if there > > > > is a better way to fix it. > > > > I agree that `goto next_rx_no_prod_no_len` appears to be most correct; > > though I don't understand why this function is a mix of early return > > codes, vs setting rc then updating *raw_cons. The alternative is > > probably zero initializing len, but I'm not sure whether *raw_cons > > should be updated in that case or not. Thanks for bringing this up > > and the patch. Sorry for the delay in review. Can folks at Broadcom > > please clarify? > > I also came up with a workaround for this, but did it the other way round: > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > index 0bb9d7b3a2b6..48bdb87574c3 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c > @@ -1608,6 +1608,7 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct > bnxt_cp_ring_info *cpr, > } > *event |= BNXT_RX_EVENT; > > + len = le32_to_cpu(rxcmp->rx_cmp_len_flags_type) >> RX_CMP_LEN_SHIFT; > rx_buf->data = NULL; > if (rxcmp1->rx_cmp_cfa_code_errors_v2 & RX_CMP_L2_ERRORS) { > bnxt_reuse_rx_data(rxr, cons, data); > @@ -1618,7 +1619,6 @@ static int bnxt_rx_pkt(struct bnxt *bp, struct > bnxt_cp_ring_info *cpr, > goto next_rx; > } > > - len = le32_to_cpu(rxcmp->rx_cmp_len_flags_type) >> RX_CMP_LEN_SHIFT; > dma_addr = rx_buf->mapping; > > if (bnxt_rx_xdp(bp, rxr, cons, data, &data_ptr, &len, event)) { > > Presumably one of the two is correct here, but I don't know which one ;-) > > Arnd Did you want to submit this a formal patch? Since no one from Broadcom has chipped in yet, it'd probably be better to submit this for review and get comments that way. I cannot imagine that this is any worse than what is currently happening (adding uninitialized stack memory to rx_bytes when jumping to next_rx) and I like it better than modifying the goto statement. This is the only other warning I run into in arm, arm64 and x86_64 all{yes,mod}config so I'd like to get it resolved soon so we can turn on this warning for the whole kernel. Cheers, Nathan ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: -Wsometimes-uninitialized Clang warning in drivers/net/ethernet/broadcom/bnxt/bnxt.c 2019-04-25 18:14 ` Nathan Chancellor @ 2019-04-25 18:33 ` Michael Chan 2019-04-25 18:35 ` Nathan Chancellor 0 siblings, 1 reply; 7+ messages in thread From: Michael Chan @ 2019-04-25 18:33 UTC (permalink / raw) To: Nathan Chancellor Cc: Arnd Bergmann, Nick Desaulniers, David S. Miller, Networking, LKML, clang-built-linux, Eddie Wai, jeffrey.huang@broadcom.com, prashant.sreedharan@broadcom.com, michael.chan@broadcom.com, Vasundhara Volam On Thu, Apr 25, 2019 at 11:14 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > > Hi Arnd, > > On Fri, Mar 22, 2019 at 03:32:50PM +0100, Arnd Bergmann wrote: > > On Wed, Mar 20, 2019 at 9:41 PM 'Nick Desaulniers' via Clang Built > > Linux <clang-built-linux@googlegroups.com> wrote: > > > > > > + Broadcom folks from commit c0c050c58d84 ("bnxt_en: New Broadcom > > > ethernet driver."). Looks like Michael wrote and is still maintaining > > > the driver. > > > > > > On Wed, Mar 20, 2019 at 12:08 PM Nathan Chancellor > > > <natechancellor@gmail.com> wrote: > > > > > > > > On Thu, Mar 07, 2019 at 05:57:35PM -0700, Nathan Chancellor wrote: > > > > > Hi all, > > > > > > > > > > We are trying to get Clang's -Wsometimes-uninitialized turned on for the > > > > > kernel as it can catch some bugs that GCC can't. This warning came up: > > > > > > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1612:6: warning: variable 'len' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] > > > > > if (rxcmp1->rx_cmp_cfa_code_errors_v2 & RX_CMP_L2_ERRORS) { > > > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1703:19: note: uninitialized use occurs here > > > > > cpr->rx_bytes += len; > > > > > ^~~ > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1612:2: note: remove the 'if' if its condition is always false > > > > > if (rxcmp1->rx_cmp_cfa_code_errors_v2 & RX_CMP_L2_ERRORS) { > > > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1540:18: note: initialize the variable 'len' to silence this warning > > > > > unsigned int len; > > > > > ^ > > > > > = 0 > > > > > 1 warning generated. > > > > > > > > > > It seems like the logical change to make is this; however, I am not sure > > > > > if this has any other unintended consequences since this is a rather > > > > > dense function. I would much appreciate your input, especially if there > > > > > is a better way to fix it. > > > > > > I agree that `goto next_rx_no_prod_no_len` appears to be most correct; > > > though I don't understand why this function is a mix of early return > > > codes, vs setting rc then updating *raw_cons. The alternative is > > > probably zero initializing len, but I'm not sure whether *raw_cons > > > should be updated in that case or not. Thanks for bringing this up > > > and the patch. Sorry for the delay in review. Can folks at Broadcom > > > please clarify? Sorry, I somehow missed this email earlier. "goto next_rx_no_prod_no_len" is the most correct fix. The "cpr->rx_bytes += len" logic was added about a year ago when we added the DIM (Dynamic Interrupt Moderation) support and we missed fixing this goto. I can prepare and send the patch later today. Thanks. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: -Wsometimes-uninitialized Clang warning in drivers/net/ethernet/broadcom/bnxt/bnxt.c 2019-04-25 18:33 ` Michael Chan @ 2019-04-25 18:35 ` Nathan Chancellor 0 siblings, 0 replies; 7+ messages in thread From: Nathan Chancellor @ 2019-04-25 18:35 UTC (permalink / raw) To: Michael Chan Cc: Arnd Bergmann, Nick Desaulniers, David S. Miller, Networking, LKML, clang-built-linux, Eddie Wai, jeffrey.huang@broadcom.com, prashant.sreedharan@broadcom.com, michael.chan@broadcom.com, Vasundhara Volam On Thu, Apr 25, 2019 at 11:33:04AM -0700, Michael Chan wrote: > On Thu, Apr 25, 2019 at 11:14 AM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > Hi Arnd, > > > > On Fri, Mar 22, 2019 at 03:32:50PM +0100, Arnd Bergmann wrote: > > > On Wed, Mar 20, 2019 at 9:41 PM 'Nick Desaulniers' via Clang Built > > > Linux <clang-built-linux@googlegroups.com> wrote: > > > > > > > > + Broadcom folks from commit c0c050c58d84 ("bnxt_en: New Broadcom > > > > ethernet driver."). Looks like Michael wrote and is still maintaining > > > > the driver. > > > > > > > > On Wed, Mar 20, 2019 at 12:08 PM Nathan Chancellor > > > > <natechancellor@gmail.com> wrote: > > > > > > > > > > On Thu, Mar 07, 2019 at 05:57:35PM -0700, Nathan Chancellor wrote: > > > > > > Hi all, > > > > > > > > > > > > We are trying to get Clang's -Wsometimes-uninitialized turned on for the > > > > > > kernel as it can catch some bugs that GCC can't. This warning came up: > > > > > > > > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1612:6: warning: variable 'len' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized] > > > > > > if (rxcmp1->rx_cmp_cfa_code_errors_v2 & RX_CMP_L2_ERRORS) { > > > > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1703:19: note: uninitialized use occurs here > > > > > > cpr->rx_bytes += len; > > > > > > ^~~ > > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1612:2: note: remove the 'if' if its condition is always false > > > > > > if (rxcmp1->rx_cmp_cfa_code_errors_v2 & RX_CMP_L2_ERRORS) { > > > > > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > > > > drivers/net/ethernet/broadcom/bnxt/bnxt.c:1540:18: note: initialize the variable 'len' to silence this warning > > > > > > unsigned int len; > > > > > > ^ > > > > > > = 0 > > > > > > 1 warning generated. > > > > > > > > > > > > It seems like the logical change to make is this; however, I am not sure > > > > > > if this has any other unintended consequences since this is a rather > > > > > > dense function. I would much appreciate your input, especially if there > > > > > > is a better way to fix it. > > > > > > > > I agree that `goto next_rx_no_prod_no_len` appears to be most correct; > > > > though I don't understand why this function is a mix of early return > > > > codes, vs setting rc then updating *raw_cons. The alternative is > > > > probably zero initializing len, but I'm not sure whether *raw_cons > > > > should be updated in that case or not. Thanks for bringing this up > > > > and the patch. Sorry for the delay in review. Can folks at Broadcom > > > > please clarify? > > Sorry, I somehow missed this email earlier. "goto > next_rx_no_prod_no_len" is the most correct fix. The "cpr->rx_bytes > += len" logic was added about a year ago when we added the DIM > (Dynamic Interrupt Moderation) support and we missed fixing this goto. > > I can prepare and send the patch later today. Thanks. Thank you for the reply, I appreciate it! I'll be on the lookout for the patch. Cheers, Nathan ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-04-25 18:35 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-03-08 0:57 -Wsometimes-uninitialized Clang warning in drivers/net/ethernet/broadcom/bnxt/bnxt.c Nathan Chancellor 2019-03-20 19:08 ` Nathan Chancellor 2019-03-20 20:41 ` Nick Desaulniers 2019-03-22 14:32 ` Arnd Bergmann 2019-04-25 18:14 ` Nathan Chancellor 2019-04-25 18:33 ` Michael Chan 2019-04-25 18:35 ` Nathan Chancellor
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.