| Message ID | 20190308005735.GA4122@archlinux-ryzen |
|---|---|
| State | RFC |
| Delegated to: | David Miller |
| Headers | show |
| Series | -Wsometimes-uninitialized Clang warning in drivers/net/ethernet/broadcom/bnxt/bnxt.c | expand |
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
+ 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?
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
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
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.
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
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;
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