| Message ID | 20190307225650.23001-1-natechancellor@gmail.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Commit | 89e28da82836530f1ac7a3a32fecc31f22d79b3e |
| Headers | show |
| Series | soc: mediatek: pwrap: Zero initialize rdata in pwrap_init_cipher | expand |
On Thu, Mar 7, 2019 at 2:57 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > When building with -Wsometimes-uninitialized, Clang warns: > > drivers/soc/mediatek/mtk-pmic-wrap.c:1358:6: error: variable 'rdata' is > used uninitialized whenever '||' condition is true > [-Werror,-Wsometimes-uninitialized] > > If pwrap_write returns non-zero, pwrap_read will not be called to > initialize rdata, meaning that we will use some random uninitialized > stack value in our print statement. Zero initialize rdata in case this > happens. > > Link: https://github.com/ClangBuiltLinux/linux/issues/401 > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > > I don't know if this is better or to just restructure the if statement > below (I'm not an expert in this code so I'll leave that up to the > maintainers to decide). No, I the way you have it here is most correct. That condition writes a value somewhere, reads it back, then compares it. The write or read could fail. Better to just initialize rdata in case the write fails. Thanks for the patch. Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> > > drivers/soc/mediatek/mtk-pmic-wrap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c > index 8236a6c87e19..2f632e8790f7 100644 > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c > @@ -1281,7 +1281,7 @@ static bool pwrap_is_pmic_cipher_ready(struct pmic_wrapper *wrp) > static int pwrap_init_cipher(struct pmic_wrapper *wrp) > { > int ret; > - u32 rdata; > + u32 rdata = 0; > > pwrap_writel(wrp, 0x1, PWRAP_CIPHER_SWRST); > pwrap_writel(wrp, 0x0, PWRAP_CIPHER_SWRST); > -- > 2.21.0 >
On Thu, Mar 07, 2019 at 03:56:51PM -0700, Nathan Chancellor wrote: > When building with -Wsometimes-uninitialized, Clang warns: > > drivers/soc/mediatek/mtk-pmic-wrap.c:1358:6: error: variable 'rdata' is > used uninitialized whenever '||' condition is true > [-Werror,-Wsometimes-uninitialized] > > If pwrap_write returns non-zero, pwrap_read will not be called to > initialize rdata, meaning that we will use some random uninitialized > stack value in our print statement. Zero initialize rdata in case this > happens. > > Link: https://github.com/ClangBuiltLinux/linux/issues/401 > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > > I don't know if this is better or to just restructure the if statement > below (I'm not an expert in this code so I'll leave that up to the > maintainers to decide). > > drivers/soc/mediatek/mtk-pmic-wrap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c > index 8236a6c87e19..2f632e8790f7 100644 > --- a/drivers/soc/mediatek/mtk-pmic-wrap.c > +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c > @@ -1281,7 +1281,7 @@ static bool pwrap_is_pmic_cipher_ready(struct pmic_wrapper *wrp) > static int pwrap_init_cipher(struct pmic_wrapper *wrp) > { > int ret; > - u32 rdata; > + u32 rdata = 0; > > pwrap_writel(wrp, 0x1, PWRAP_CIPHER_SWRST); > pwrap_writel(wrp, 0x0, PWRAP_CIPHER_SWRST); > -- > 2.21.0 > 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
On Fri, Mar 8, 2019 at 1:18 AM 'Nick Desaulniers' via Clang Built Linux <clang-built-linux@googlegroups.com> wrote: > > On Thu, Mar 7, 2019 at 2:57 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > When building with -Wsometimes-uninitialized, Clang warns: > > > > drivers/soc/mediatek/mtk-pmic-wrap.c:1358:6: error: variable 'rdata' is > > used uninitialized whenever '||' condition is true > > [-Werror,-Wsometimes-uninitialized] > > > > If pwrap_write returns non-zero, pwrap_read will not be called to > > initialize rdata, meaning that we will use some random uninitialized > > stack value in our print statement. Zero initialize rdata in case this > > happens. > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/401 > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > --- > > > > I don't know if this is better or to just restructure the if statement > > below (I'm not an expert in this code so I'll leave that up to the > > maintainers to decide). > > No, I the way you have it here is most correct. That condition writes > a value somewhere, reads it back, then compares it. The write or read > could fail. Better to just initialize rdata in case the write fails. > Thanks for the patch. > Reviewed-by: Nick Desaulniers <ndesaulniers@google.com> I also came up with a similar patch here, but I move the initialization to just before the first use of the variable: diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c index 8236a6c87e19..eb5035fd8ecd 100644 --- a/drivers/soc/mediatek/mtk-pmic-wrap.c +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c @@ -1355,6 +1355,7 @@ static int pwrap_init_cipher(struct pmic_wrapper *wrp) pwrap_writel(wrp, 1, PWRAP_CIPHER_MODE); /* Write Test */ + rdata = 0; if (pwrap_write(wrp, wrp->slave->dew_regs[PWRAP_DEW_WRITE_TEST], PWRAP_DEW_WRITE_TEST_VAL) || pwrap_read(wrp, wrp->slave->dew_regs[PWRAP_DEW_WRITE_TEST], Very little difference, so let's go with your patch Reviewed-by: Arnd Bergmann <arnd@arndb.de>
On 20/03/2019 20:11, Nathan Chancellor wrote: > On Thu, Mar 07, 2019 at 03:56:51PM -0700, Nathan Chancellor wrote: >> When building with -Wsometimes-uninitialized, Clang warns: >> >> drivers/soc/mediatek/mtk-pmic-wrap.c:1358:6: error: variable 'rdata' is >> used uninitialized whenever '||' condition is true >> [-Werror,-Wsometimes-uninitialized] >> >> If pwrap_write returns non-zero, pwrap_read will not be called to >> initialize rdata, meaning that we will use some random uninitialized >> stack value in our print statement. Zero initialize rdata in case this >> happens. >> >> Link: https://github.com/ClangBuiltLinux/linux/issues/401 >> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> >> --- >> >> I don't know if this is better or to just restructure the if statement >> below (I'm not an expert in this code so I'll leave that up to the >> maintainers to decide). >> >> drivers/soc/mediatek/mtk-pmic-wrap.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c >> index 8236a6c87e19..2f632e8790f7 100644 >> --- a/drivers/soc/mediatek/mtk-pmic-wrap.c >> +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c >> @@ -1281,7 +1281,7 @@ static bool pwrap_is_pmic_cipher_ready(struct pmic_wrapper *wrp) >> static int pwrap_init_cipher(struct pmic_wrapper *wrp) >> { >> int ret; >> - u32 rdata; >> + u32 rdata = 0; >> >> pwrap_writel(wrp, 0x1, PWRAP_CIPHER_SWRST); >> pwrap_writel(wrp, 0x0, PWRAP_CIPHER_SWRST); >> -- >> 2.21.0 >> > > 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. > applied now to v5.1-next/soc thanks
diff --git a/drivers/soc/mediatek/mtk-pmic-wrap.c b/drivers/soc/mediatek/mtk-pmic-wrap.c index 8236a6c87e19..2f632e8790f7 100644 --- a/drivers/soc/mediatek/mtk-pmic-wrap.c +++ b/drivers/soc/mediatek/mtk-pmic-wrap.c @@ -1281,7 +1281,7 @@ static bool pwrap_is_pmic_cipher_ready(struct pmic_wrapper *wrp) static int pwrap_init_cipher(struct pmic_wrapper *wrp) { int ret; - u32 rdata; + u32 rdata = 0; pwrap_writel(wrp, 0x1, PWRAP_CIPHER_SWRST); pwrap_writel(wrp, 0x0, PWRAP_CIPHER_SWRST);
When building with -Wsometimes-uninitialized, Clang warns: drivers/soc/mediatek/mtk-pmic-wrap.c:1358:6: error: variable 'rdata' is used uninitialized whenever '||' condition is true [-Werror,-Wsometimes-uninitialized] If pwrap_write returns non-zero, pwrap_read will not be called to initialize rdata, meaning that we will use some random uninitialized stack value in our print statement. Zero initialize rdata in case this happens. Link: https://github.com/ClangBuiltLinux/linux/issues/401 Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- I don't know if this is better or to just restructure the if statement below (I'm not an expert in this code so I'll leave that up to the maintainers to decide). drivers/soc/mediatek/mtk-pmic-wrap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)