| Message ID | 20200120143206.710666-1-thierry.reding@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | pwm: sun4i: Initialize variables before use | expand |
Hello Thierry, On Mon, Jan 20, 2020 at 03:32:06PM +0100, Thierry Reding wrote: > GCC can't always determine that the duty, period and prescaler values > are initialized when returning from sun4i_pwm_calculate(), so help out a > little by initializing them to 0. Is it worth mentioning the gcc version you're using? > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > --- > drivers/pwm/pwm-sun4i.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > index 0decc7cde133..3e3efa6c768f 100644 I don't find this object (0decc7cde133) in my tree or next. Which version is this? > --- a/drivers/pwm/pwm-sun4i.c > +++ b/drivers/pwm/pwm-sun4i.c > @@ -234,9 +234,9 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > { > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip); > struct pwm_state cstate; > - u32 ctrl, duty, period, val; > + u32 ctrl, duty = 0, period = 0, val; + u32 ctrl, uninitialized_var(duty), uninitialized_var(period), val; should fix the warnings, too, and additionally explicitly documents that it's just the compiler that doesn't see there is no problem. Best regards Uwe
Hi Uwe, Thierry On Mon, 20 Jan 2020 at 21:09, Uwe Kleine-König <u.kleine-koenig@pengutronix.de> wrote: > > Hello Thierry, > > On Mon, Jan 20, 2020 at 03:32:06PM +0100, Thierry Reding wrote: > > GCC can't always determine that the duty, period and prescaler values > > are initialized when returning from sun4i_pwm_calculate(), so help out a > > little by initializing them to 0. > > Is it worth mentioning the gcc version you're using? This issue has been trig by kbuild test robot. I planned to submit a patch for it as it's due to my modification but forget to submit it... Original report [linux-next:master 6586/9861] drivers/pwm/pwm-sun4i.c:57:34: warning: 'prescaler' may be used uninitialized in this function tree: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git master head: de970dffa7d19eae1d703c3534825308ef8d5dec commit: 9f28e95b5286fce63a3d0d90dc7ca43eca8dda58 [6586/9861] pwm: sun4i: Add support to output source clock directly config: microblaze-randconfig-a001-20200118 (attached as .config) compiler: microblaze-linux-gcc (GCC) 7.5.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross git checkout 9f28e95b5286fce63a3d0d90dc7ca43eca8dda58 # save the attached .config to linux build tree GCC_VERSION=7.5.0 make.cross ARCH=microblaze If you fix the issue, kindly add following tag Reported-by: kbuild test robot <lkp@intel.com> Note: it may well be a FALSE warning. FWIW you are at least aware of it now. http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > --- > > drivers/pwm/pwm-sun4i.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > index 0decc7cde133..3e3efa6c768f 100644 > > I don't find this object (0decc7cde133) in my tree or next. Which > version is this? > > > --- a/drivers/pwm/pwm-sun4i.c > > +++ b/drivers/pwm/pwm-sun4i.c > > @@ -234,9 +234,9 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > { > > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip); > > struct pwm_state cstate; > > - u32 ctrl, duty, period, val; > > + u32 ctrl, duty = 0, period = 0, val; > > + u32 ctrl, uninitialized_var(duty), uninitialized_var(period), val; > > should fix the warnings, too, and additionally explicitly documents that > it's just the compiler that doesn't see there is no problem. > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | https://www.pengutronix.de/ |
On Mon, Jan 20, 2020 at 09:09:17PM +0100, Uwe Kleine-König wrote: > Hello Thierry, > > On Mon, Jan 20, 2020 at 03:32:06PM +0100, Thierry Reding wrote: > > GCC can't always determine that the duty, period and prescaler values > > are initialized when returning from sun4i_pwm_calculate(), so help out a > > little by initializing them to 0. > > Is it worth mentioning the gcc version you're using? I could, but what good is that going to be? I don't think this is something that's limited to one specific version but I don't know exactly which ones are impacted. Stating just one specific version isn't all that useful in that case. > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > --- > > drivers/pwm/pwm-sun4i.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > index 0decc7cde133..3e3efa6c768f 100644 > > I don't find this object (0decc7cde133) in my tree or next. Which > version is this? I made this on top of my local pwm/for-next when I was build-testing, which I usually do before pushing, so it's not surprising that you don't have this in your tree. > > > --- a/drivers/pwm/pwm-sun4i.c > > +++ b/drivers/pwm/pwm-sun4i.c > > @@ -234,9 +234,9 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > { > > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip); > > struct pwm_state cstate; > > - u32 ctrl, duty, period, val; > > + u32 ctrl, duty = 0, period = 0, val; > > + u32 ctrl, uninitialized_var(duty), uninitialized_var(period), val; > > should fix the warnings, too, and additionally explicitly documents that > it's just the compiler that doesn't see there is no problem. I haven't convinced myself fully yet that there really isn't a problem. I'm fairly sure it's safe, but always initializing to 0 doesn't hurt. Thierry
Hello Thierry, On Tue, Jan 21, 2020 at 02:50:11PM +0100, Thierry Reding wrote: > On Mon, Jan 20, 2020 at 09:09:17PM +0100, Uwe Kleine-König wrote: > > On Mon, Jan 20, 2020 at 03:32:06PM +0100, Thierry Reding wrote: > > > GCC can't always determine that the duty, period and prescaler values > > > are initialized when returning from sun4i_pwm_calculate(), so help out a > > > little by initializing them to 0. > > > > Is it worth mentioning the gcc version you're using? > > I could, but what good is that going to be? I don't think this is > something that's limited to one specific version but I don't know > exactly which ones are impacted. Stating just one specific version > isn't all that useful in that case. I think something like: gcc 4.6 reports [...], newer gccs are fine. is useful. If you read that in a few years, it helps you either to reproduce the problem or determine it is not important any more. > > > Signed-off-by: Thierry Reding <thierry.reding@gmail.com> > > > --- > > > drivers/pwm/pwm-sun4i.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c > > > index 0decc7cde133..3e3efa6c768f 100644 > > > > I don't find this object (0decc7cde133) in my tree or next. Which > > version is this? > > I made this on top of my local pwm/for-next when I was build-testing, > which I usually do before pushing, so it's not surprising that you > don't have this in your tree. The reason I asked this (and also the gcc version) is to reproduce the issue and work with it. With your reply I can only say that I expect that uninitialized_var fixes the problem in a better way. If I knew the exact circumstances, I could test them and claim that it indeed fixes it (or see it doesn't and don't take your time with non-sense reviews). > > > --- a/drivers/pwm/pwm-sun4i.c > > > +++ b/drivers/pwm/pwm-sun4i.c > > > @@ -234,9 +234,9 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, > > > { > > > struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip); > > > struct pwm_state cstate; > > > - u32 ctrl, duty, period, val; > > > + u32 ctrl, duty = 0, period = 0, val; > > > > + u32 ctrl, uninitialized_var(duty), uninitialized_var(period), val; > > > > should fix the warnings, too, and additionally explicitly documents that > > it's just the compiler that doesn't see there is no problem. > > I haven't convinced myself fully yet that there really isn't a problem. > I'm fairly sure it's safe, but always initializing to 0 doesn't hurt. Yes, I agree that initializing the variable fixes the warning. Still using uninitialized_var is the better way (assuming it indeed fixes the problem, see above). Best regards Uwe
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c index 0decc7cde133..3e3efa6c768f 100644 --- a/drivers/pwm/pwm-sun4i.c +++ b/drivers/pwm/pwm-sun4i.c @@ -234,9 +234,9 @@ static int sun4i_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm, { struct sun4i_pwm_chip *sun4i_pwm = to_sun4i_pwm_chip(chip); struct pwm_state cstate; - u32 ctrl, duty, period, val; + u32 ctrl, duty = 0, period = 0, val; int ret; - unsigned int delay_us, prescaler; + unsigned int delay_us, prescaler = 0; unsigned long now; bool bypass;
GCC can't always determine that the duty, period and prescaler values are initialized when returning from sun4i_pwm_calculate(), so help out a little by initializing them to 0. Signed-off-by: Thierry Reding <thierry.reding@gmail.com> --- drivers/pwm/pwm-sun4i.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)