[v2] backlight: pwm_bl: Fix uninitialized variable

Message ID 20180719161923.21510-1-daniel.thompson@linaro.org
State New
Headers show
Series
  • [v2] backlight: pwm_bl: Fix uninitialized variable
Related show

Commit Message

Daniel Thompson July 19, 2018, 4:19 p.m.
Currently, if the DT does not define num-interpolated-steps then
num_steps is undefined and the interpolation code will deploy randomly.
Fix this.

Additionally fix a small grammar error that was identified and
tighten up return code checking of DT properties, both of which came
up during review of this patch.

Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation between
brightness-levels")
Reported-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>
Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

---

Notes:
    v2:
     - Simplify SoB chain (with Marcel's permission)
     - Separate complex if statement and make other similar calls use same
       return code checking approach
     - Tidy up comment formatting and fix pre-existing grammar error

 drivers/video/backlight/pwm_bl.c | 25 ++++++++++++-------------
 1 file changed, 12 insertions(+), 13 deletions(-)

--
2.17.1

Comments

Lee Jones July 23, 2018, 7:23 a.m. | #1
On Thu, 19 Jul 2018, Daniel Thompson wrote:

> Currently, if the DT does not define num-interpolated-steps then

> num_steps is undefined and the interpolation code will deploy randomly.

> Fix this.

> 

> Additionally fix a small grammar error that was identified and

> tighten up return code checking of DT properties, both of which came

> up during review of this patch.

> 

> Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation between

> brightness-levels")

> Reported-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

> Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> ---

> 

> Notes:

>     v2:

>      - Simplify SoB chain (with Marcel's permission)

>      - Separate complex if statement and make other similar calls use same

>        return code checking approach

>      - Tidy up comment formatting and fix pre-existing grammar error

> 

>  drivers/video/backlight/pwm_bl.c | 25 ++++++++++++-------------

>  1 file changed, 12 insertions(+), 13 deletions(-)


I'm hesitant to provide feedback on this, as I feel as though I've
messed you around enough, however ... ;)

> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c

> index 9ee4c1b735b2..f7799f62fea0 100644

> --- a/drivers/video/backlight/pwm_bl.c

> +++ b/drivers/video/backlight/pwm_bl.c

> @@ -284,30 +284,29 @@ static int pwm_backlight_parse_dt(struct device *dev,

>  		ret = of_property_read_u32_array(node, "brightness-levels",

>  						 data->levels,

>  						 data->max_brightness);

> -		if (ret < 0)

> +		if (!ret)

>  			return ret;

> 

>  		ret = of_property_read_u32(node, "default-brightness-level",

>  					   &value);

> -		if (ret < 0)

> +		if (!ret)

>  			return ret;


Just FYI (it didn't even make it to 'nit' status), this should really
be done in a separate patch since it is unrelated to the rest of the
patch.

>  		data->dft_brightness = value;

> 

>  		/*

>  		 * This property is optional, if is set enables linear

> -		 * interpolation between each of the values of brightness levels

> -		 * and creates a new pre-computed table.

> +		 * interpolation between each of the values of brightness

> +		 * levels and creates a new pre-computed table.

>  		 */

> -		of_property_read_u32(node, "num-interpolated-steps",

> -				     &num_steps);

> -

> -		/*

> -		 * Make sure that there is at least two entries in the

> -		 * brightness-levels table, otherwise we can't interpolate

> -		 * between two points.

> -		 */

> -		if (num_steps) {

> +		ret = of_property_read_u32(node, "num-interpolated-steps",

> +					   &num_steps);

> +		if (!ret || num_steps) {


Not sure if it's even possible for of_property_read_u32() to fail AND
still populate num_steps, however this check makes it sound like that's
okay.  Is that correct?

I can't help but think that this all 'just goes away' if you
pre-initialise num_steps.  I wouldn't let the "do not initialise too
far away from the code using variable" affect this.  However, if
you're insistent, perhaps consider moving the declaration to just
below:

  if (data->max_brightness > 0) {

> +			/*

> +			 * Make sure that there are at least two entries in

> +			 * the brightness-levels table, otherwise we can't

> +			 * interpolate between two points.

> +			 */

>  			if (data->max_brightness < 2) {

>  				dev_err(dev, "can't interpolate\n");

>  				return -EINVAL;


-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
Daniel Thompson July 24, 2018, 6:48 a.m. | #2
On Mon, Jul 23, 2018 at 08:23:43AM +0100, Lee Jones wrote:
> On Thu, 19 Jul 2018, Daniel Thompson wrote:

> 

> > Currently, if the DT does not define num-interpolated-steps then

> > num_steps is undefined and the interpolation code will deploy randomly.

> > Fix this.

> > 

> > Additionally fix a small grammar error that was identified and

> > tighten up return code checking of DT properties, both of which came

> > up during review of this patch.

> > 

> > Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation between

> > brightness-levels")

> > Reported-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

> > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> > ---

> > 

> > Notes:

> >     v2:

> >      - Simplify SoB chain (with Marcel's permission)

> >      - Separate complex if statement and make other similar calls use same

> >        return code checking approach

> >      - Tidy up comment formatting and fix pre-existing grammar error

> > 

> >  drivers/video/backlight/pwm_bl.c | 25 ++++++++++++-------------

> >  1 file changed, 12 insertions(+), 13 deletions(-)

> 

> I'm hesitant to provide feedback on this, as I feel as though I've

> messed you around enough, however ... ;)

> 

> > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c

> > index 9ee4c1b735b2..f7799f62fea0 100644

> > --- a/drivers/video/backlight/pwm_bl.c

> > +++ b/drivers/video/backlight/pwm_bl.c

> > @@ -284,30 +284,29 @@ static int pwm_backlight_parse_dt(struct device *dev,

> >  		ret = of_property_read_u32_array(node, "brightness-levels",

> >  						 data->levels,

> >  						 data->max_brightness);

> > -		if (ret < 0)

> > +		if (!ret)

> >  			return ret;

> > 

> >  		ret = of_property_read_u32(node, "default-brightness-level",

> >  					   &value);

> > -		if (ret < 0)

> > +		if (!ret)

> >  			return ret;

> 

> Just FYI (it didn't even make it to 'nit' status), this should really

> be done in a separate patch since it is unrelated to the rest of the

> patch.

> 

> >  		data->dft_brightness = value;

> > 

> >  		/*

> >  		 * This property is optional, if is set enables linear

> > -		 * interpolation between each of the values of brightness levels

> > -		 * and creates a new pre-computed table.

> > +		 * interpolation between each of the values of brightness

> > +		 * levels and creates a new pre-computed table.

> >  		 */

> > -		of_property_read_u32(node, "num-interpolated-steps",

> > -				     &num_steps);

> > -

> > -		/*

> > -		 * Make sure that there is at least two entries in the

> > -		 * brightness-levels table, otherwise we can't interpolate

> > -		 * between two points.

> > -		 */

> > -		if (num_steps) {

> > +		ret = of_property_read_u32(node, "num-interpolated-steps",

> > +					   &num_steps);

> > +		if (!ret || num_steps) {

> 

> Not sure if it's even possible for of_property_read_u32() to fail AND

> still populate num_steps, however this check makes it sound like that's

> okay.  Is that correct?


No, it's bogus. Looks like when I broke the if statement into two
clauses I ended up flipping the && to an ||.


> I can't help but think that this all 'just goes away' if you

> pre-initialise num_steps.  I wouldn't let the "do not initialise too

> far away from the code using variable" affect this.  However, if

> you're insistent, perhaps consider moving the declaration to just

> below:

> 

>   if (data->max_brightness > 0) {

> 

> > +			/*

> > +			 * Make sure that there are at least two entries in

> > +			 * the brightness-levels table, otherwise we can't

> > +			 * interpolate between two points.

> > +			 */

> >  			if (data->max_brightness < 2) {

> >  				dev_err(dev, "can't interpolate\n");

> >  				return -EINVAL;

> 

> -- 

> Lee Jones [李琼斯]

> Linaro Services Technical Lead

> Linaro.org │ Open source software for ARM SoCs

> Follow Linaro: Facebook | Twitter | Blog
Daniel Thompson July 24, 2018, 7:01 a.m. | #3
On Mon, Jul 23, 2018 at 08:23:43AM +0100, Lee Jones wrote:
> On Thu, 19 Jul 2018, Daniel Thompson wrote:

> 

> > Currently, if the DT does not define num-interpolated-steps then

> > num_steps is undefined and the interpolation code will deploy randomly.

> > Fix this.

> > 

> > Additionally fix a small grammar error that was identified and

> > tighten up return code checking of DT properties, both of which came

> > up during review of this patch.

> > 

> > Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation between

> > brightness-levels")

> > Reported-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> > Signed-off-by: Daniel Thompson <daniel.thompson@linaro.org>

> > Tested-by: Marcel Ziswiler <marcel.ziswiler@toradex.com>

> > ---

> > 

> > Notes:

> >     v2:

> >      - Simplify SoB chain (with Marcel's permission)

> >      - Separate complex if statement and make other similar calls use same

> >        return code checking approach

> >      - Tidy up comment formatting and fix pre-existing grammar error

> > 

> >  drivers/video/backlight/pwm_bl.c | 25 ++++++++++++-------------

> >  1 file changed, 12 insertions(+), 13 deletions(-)

> 

> I'm hesitant to provide feedback on this, as I feel as though I've

> messed you around enough, however ... ;)

> 

> > diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c

> > index 9ee4c1b735b2..f7799f62fea0 100644

> > --- a/drivers/video/backlight/pwm_bl.c

> > +++ b/drivers/video/backlight/pwm_bl.c

> > @@ -284,30 +284,29 @@ static int pwm_backlight_parse_dt(struct device *dev,

> >  		ret = of_property_read_u32_array(node, "brightness-levels",

> >  						 data->levels,

> >  						 data->max_brightness);

> > -		if (ret < 0)

> > +		if (!ret)

> >  			return ret;

> > 

> >  		ret = of_property_read_u32(node, "default-brightness-level",

> >  					   &value);

> > -		if (ret < 0)

> > +		if (!ret)

> >  			return ret;

> 

> Just FYI (it didn't even make it to 'nit' status), this should really

> be done in a separate patch since it is unrelated to the rest of the

> patch.


Did wonder which way to go on this... I figured this close I'd accept
code either way so adopted fewest patches.

However I will split this out because I'm going to go back to the orignal
pre-v1 approach of just initializing the damn variable.


> >  		data->dft_brightness = value;

> > 

> >  		/*

> >  		 * This property is optional, if is set enables linear

> > -		 * interpolation between each of the values of brightness levels

> > -		 * and creates a new pre-computed table.

> > +		 * interpolation between each of the values of brightness

> > +		 * levels and creates a new pre-computed table.

> >  		 */

> > -		of_property_read_u32(node, "num-interpolated-steps",

> > -				     &num_steps);

> > -

> > -		/*

> > -		 * Make sure that there is at least two entries in the

> > -		 * brightness-levels table, otherwise we can't interpolate

> > -		 * between two points.

> > -		 */

> > -		if (num_steps) {

> > +		ret = of_property_read_u32(node, "num-interpolated-steps",

> > +					   &num_steps);

> > +		if (!ret || num_steps) {

> 

> Not sure if it's even possible for of_property_read_u32() to fail AND

> still populate num_steps, however this check makes it sound like that's

> okay.  Is that correct?

> 

> I can't help but think that this all 'just goes away' if you

> pre-initialise num_steps.  I wouldn't let the "do not initialise too

> far away from the code using variable" affect this.  However, if

> you're insistent, perhaps consider moving the declaration to just

> below:

> 

>   if (data->max_brightness > 0) {

> 

> > +			/*

> > +			 * Make sure that there are at least two entries in

> > +			 * the brightness-levels table, otherwise we can't

> > +			 * interpolate between two points.

> > +			 */

> >  			if (data->max_brightness < 2) {

> >  				dev_err(dev, "can't interpolate\n");

> >  				return -EINVAL;

> 

> -- 

> Lee Jones [李琼斯]

> Linaro Services Technical Lead

> Linaro.org │ Open source software for ARM SoCs

> Follow Linaro: Facebook | Twitter | Blog

Patch

diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 9ee4c1b735b2..f7799f62fea0 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -284,30 +284,29 @@  static int pwm_backlight_parse_dt(struct device *dev,
 		ret = of_property_read_u32_array(node, "brightness-levels",
 						 data->levels,
 						 data->max_brightness);
-		if (ret < 0)
+		if (!ret)
 			return ret;

 		ret = of_property_read_u32(node, "default-brightness-level",
 					   &value);
-		if (ret < 0)
+		if (!ret)
 			return ret;

 		data->dft_brightness = value;

 		/*
 		 * This property is optional, if is set enables linear
-		 * interpolation between each of the values of brightness levels
-		 * and creates a new pre-computed table.
+		 * interpolation between each of the values of brightness
+		 * levels and creates a new pre-computed table.
 		 */
-		of_property_read_u32(node, "num-interpolated-steps",
-				     &num_steps);
-
-		/*
-		 * Make sure that there is at least two entries in the
-		 * brightness-levels table, otherwise we can't interpolate
-		 * between two points.
-		 */
-		if (num_steps) {
+		ret = of_property_read_u32(node, "num-interpolated-steps",
+					   &num_steps);
+		if (!ret || num_steps) {
+			/*
+			 * Make sure that there are at least two entries in
+			 * the brightness-levels table, otherwise we can't
+			 * interpolate between two points.
+			 */
 			if (data->max_brightness < 2) {
 				dev_err(dev, "can't interpolate\n");
 				return -EINVAL;