Messages in this thread | | | From | Michael Chan <> | Date | Thu, 25 Apr 2019 11:33:04 -0700 | Subject | Re: -Wsometimes-uninitialized Clang warning in drivers/net/ethernet/broadcom/bnxt/bnxt.c |
| |
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.
| |