backlight: pwm_bl: Fix uninitialized variable

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

Commit Message

Daniel Thompson July 16, 2018, 9:02 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.

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>

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

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

---
 drivers/video/backlight/pwm_bl.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

--
2.17.1

Comments

Lee Jones July 18, 2018, 8:09 a.m. | #1
On Mon, 16 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.

> 

> 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>

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


This line is confusing.  Did you guys author this patch together?

My guess is that this line should be dropped and the RB and TB tags
should remain?  If it was reviewed too, perhaps an AB too?

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

> ---

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

>  1 file changed, 8 insertions(+), 9 deletions(-)

> 

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

> index 9ee4c1b735b2..e3c22b79fbcd 100644

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

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

> @@ -299,15 +299,14 @@ static int pwm_backlight_parse_dt(struct device *dev,

>  		 * 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) {

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

> +					  &num_steps) == 0) && num_steps) {


This is pretty ugly, and isn't it suffering from over-bracketing?  My
suggestion would be to break out the invocation of
of_property_read_u32() from the if and test only the result.

		of_property_read_u32(node, "num-interpolated-steps", &num_steps);
		if (!ret && num_steps) {

I haven't checked the underling code, but is it even feasible for
of_property_read_u32() to not succeed AND for num_steps to be set?

If not, the check for !ret if superfluous and you can drop it.

> +			/*

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


s/is/are/

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

> +			 * interpolate


Why break the line here?

> +			 * 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
Lee Jones July 18, 2018, 8:12 a.m. | #2
On Wed, 18 Jul 2018, Lee Jones wrote:

> On Mon, 16 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.

> > 

> > 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>

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

> 

> This line is confusing.  Did you guys author this patch together?

> 

> My guess is that this line should be dropped and the RB and TB tags

> should remain?  If it was reviewed too, perhaps an AB too?

> 

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

> > ---

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

> >  1 file changed, 8 insertions(+), 9 deletions(-)

> > 

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

> > index 9ee4c1b735b2..e3c22b79fbcd 100644

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

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

> > @@ -299,15 +299,14 @@ static int pwm_backlight_parse_dt(struct device *dev,

> >  		 * 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) {

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

> > +					  &num_steps) == 0) && num_steps) {

> 

> This is pretty ugly, and isn't it suffering from over-bracketing?  My

> suggestion would be to break out the invocation of

> of_property_read_u32() from the if and test only the result.

> 

> 		of_property_read_u32(node, "num-interpolated-steps", &num_steps);

> 		if (!ret && num_steps) {


Whoops!  I was playing around with the 80-char limit and forgot to
revert.  The lines should read:

 		ret = of_property_read_u32(node, "num-interpolated-steps",
					   &num_steps);
 		if (!ret && num_steps) {

> I haven't checked the underling code, but is it even feasible for

> of_property_read_u32() to not succeed AND for num_steps to be set?

> 

> If not, the check for !ret if superfluous and you can drop it.

> 

> > +			/*

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

> 

> s/is/are/

> 

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

> > +			 * interpolate

> 

> Why break the line here?

> 

> > +			 * 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 18, 2018, 8:26 a.m. | #3
On Wed, Jul 18, 2018 at 09:09:13AM +0100, Lee Jones wrote:
> On Mon, 16 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.

> > 

> > 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>

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

> 

> This line is confusing.  Did you guys author this patch together?


Yes (although the manipulations were fairly mechanical).

> 

> My guess is that this line should be dropped and the RB and TB tags

> should remain?  If it was reviewed too, perhaps an AB too?

> 

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

> > ---

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

> >  1 file changed, 8 insertions(+), 9 deletions(-)

> > 

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

> > index 9ee4c1b735b2..e3c22b79fbcd 100644

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

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

> > @@ -299,15 +299,14 @@ static int pwm_backlight_parse_dt(struct device *dev,

> >  		 * 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) {

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

> > +					  &num_steps) == 0) && num_steps) {

> 

> This is pretty ugly, and isn't it suffering from over-bracketing?  My

> suggestion would be to break out the invocation of

> of_property_read_u32() from the if and test only the result.

> 

> 		of_property_read_u32(node, "num-interpolated-steps", &num_steps);

> 		if (!ret && num_steps) {

> 

> I haven't checked the underling code, but is it even feasible for

> of_property_read_u32() to not succeed AND for num_steps to be set?

> 

> If not, the check for !ret if superfluous and you can drop it.

> 

> > +			/*

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

> 

> s/is/are/

> 

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

> > +			 * interpolate

> 

> Why break the line here?

> 

> > +			 * 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..e3c22b79fbcd 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -299,15 +299,14 @@  static int pwm_backlight_parse_dt(struct device *dev,
 		 * 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) {
+		if ((of_property_read_u32(node, "num-interpolated-steps",
+					  &num_steps) == 0) && 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 (data->max_brightness < 2) {
 				dev_err(dev, "can't interpolate\n");
 				return -EINVAL;