On Thu, Apr 25, 2019 at 11:33:04AM -0700, Michael Chan wrote: > On Thu, Apr 25, 2019 at 11:14 AM Nathan Chancellor > <[email protected]> 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 <[email protected]> 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 > > > > <[email protected]> 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
