backlight: pwm_bl: Fix uninitialized variable

Message ID 20180716210241.9457-1-daniel.thompson@linaro.org
State Superseded
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

--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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 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;
>
Marcel Ziswiler July 18, 2018, 8:22 a.m. | #3
On Wed, 2018-07-18 at 09:09 +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, I reported it and we came to a conclusion 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?

I'm OK either way and do not need any explicit authorship to be
expressed for me.

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

you mean:

		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.

No, then we are back to the initial issue of num_steps potentially not
being initialised. We really want both of_property_read_u32() to
succeed AND num_steps to actually be set.

> > +			/*
> > +			 * 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?

That's probably a remnant of going back and forth plus quoting on the
mailing list.

> > +			 * between two points.
> > +			 */
> >  			if (data->max_brightness < 2) {
> >  				dev_err(dev, "can't
> > interpolate\n");
> >  				return -EINVAL;
Daniel Thompson July 18, 2018, 8:26 a.m. | #4
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
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones July 18, 2018, 9:53 a.m. | #5
On Wed, 18 Jul 2018, Marcel Ziswiler wrote:

> On Wed, 2018-07-18 at 09:09 +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, I reported it and we came to a conclusion together.

It sounds like you need to have all of the tags (except this one). :)

 Reported-by:  for reporting the issue
 Suggested-by: for suggesting a resolution
 Acked-by:     for reviewing it
 Tested-by:    for testing it

Signed-off-by usually means you either wrote a significant amount of
the diffstat or you were part of the submission path.

> > 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?
> 
> I'm OK either way and do not need any explicit authorship to be
> expressed for me.

In this instance I suggest keeping Reported-by and Tested-by.

> > > 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);
> 
> you mean:
> 
> 		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.
> 
> No, then we are back to the initial issue of num_steps potentially not
> being initialised. We really want both of_property_read_u32() to
> succeed AND num_steps to actually be set.

I also think num_steps should be pre-initialised.

Then it will only be set if of_property_read_u32() succeeds.
Daniel Thompson July 18, 2018, 10:12 a.m. | #6
On Wed, Jul 18, 2018 at 10:53:35AM +0100, Lee Jones wrote:
> On Wed, 18 Jul 2018, Marcel Ziswiler wrote:
> 
> > On Wed, 2018-07-18 at 09:09 +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, I reported it and we came to a conclusion together.
> 
> It sounds like you need to have all of the tags (except this one). :)
> 
>  Reported-by:  for reporting the issue
>  Suggested-by: for suggesting a resolution
>  Acked-by:     for reviewing it
>  Tested-by:    for testing it
> 
> Signed-off-by usually means you either wrote a significant amount of
> the diffstat or you were part of the submission path.

He did [I don't object to but wouldn't have used the extra brackets you
brought up ;-) ].

> 
> > > 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?
> > 
> > I'm OK either way and do not need any explicit authorship to be
> > expressed for me.
> 
> In this instance I suggest keeping Reported-by and Tested-by.
> 
> > > > 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);
> > 
> > you mean:
> > 
> > 		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.
> > 
> > No, then we are back to the initial issue of num_steps potentially not
> > being initialised. We really want both of_property_read_u32() to
> > succeed AND num_steps to actually be set.
> 
> I also think num_steps should be pre-initialised.
> 
> Then it will only be set if of_property_read_u32() succeeds.
> 
> -- 
> Lee Jones [李琼斯]
> Linaro Services Technical Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marcel Ziswiler July 18, 2018, 12:57 p.m. | #7
On Wed, 2018-07-18 at 11:12 +0100, Daniel Thompson wrote:
> On Wed, Jul 18, 2018 at 10:53:35AM +0100, Lee Jones wrote:
> > On Wed, 18 Jul 2018, Marcel Ziswiler wrote:
> > 
> > > On Wed, 2018-07-18 at 09:09 +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, I reported it and we came to a conclusion together.
> > 
> > It sounds like you need to have all of the tags (except this one).
> > :)
> > 
> >  Reported-by:  for reporting the issue
> >  Suggested-by: for suggesting a resolution
> >  Acked-by:     for reviewing it
> >  Tested-by:    for testing it
> > 
> > Signed-off-by usually means you either wrote a significant amount
> > of
> > the diffstat or you were part of the submission path.
> 
> He did [I don't object to but wouldn't have used the extra brackets
> you
> brought up ;-) ].

Yes, I take all the blame for the extra brackets. Regardless of having
a masters in CS or not I still prefer too many then too few of them (;-
p).

> > > > 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?
> > > 
> > > I'm OK either way and do not need any explicit authorship to be
> > > expressed for me.
> > 
> > In this instance I suggest keeping Reported-by and Tested-by.
> > 
> > > > > 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);
> > > 
> > > you mean:
> > > 
> > > 		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.
> > > 
> > > No, then we are back to the initial issue of num_steps
> > > potentially not
> > > being initialised. We really want both of_property_read_u32() to
> > > succeed AND num_steps to actually be set.
> > 
> > I also think num_steps should be pre-initialised.

Yes, I guess it definitely does not hurt.

> > Then it will only be set if of_property_read_u32() succeeds.

Yes, but we still need to check for both, the function not failing and
num_steps to actually be non zero.

> > -- 
> > Lee Jones [李琼斯]
> > Linaro Services Technical Lead
> > Linaro.org │ Open source software for ARM SoCs
> > Follow Linaro: Facebook | Twitter | Blog
Lee Jones July 18, 2018, 1:08 p.m. | #8
On Wed, 18 Jul 2018, Marcel Ziswiler wrote:

> On Wed, 2018-07-18 at 11:12 +0100, Daniel Thompson wrote:
> > On Wed, Jul 18, 2018 at 10:53:35AM +0100, Lee Jones wrote:
> > > On Wed, 18 Jul 2018, Marcel Ziswiler wrote:
> > > 
> > > > On Wed, 2018-07-18 at 09:09 +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, I reported it and we came to a conclusion together.
> > > 
> > > It sounds like you need to have all of the tags (except this one).
> > > :)
> > > 
> > >  Reported-by:  for reporting the issue
> > >  Suggested-by: for suggesting a resolution
> > >  Acked-by:     for reviewing it
> > >  Tested-by:    for testing it
> > > 
> > > Signed-off-by usually means you either wrote a significant amount
> > > of
> > > the diffstat or you were part of the submission path.
> > 
> > He did [I don't object to but wouldn't have used the extra brackets
> > you
> > brought up ;-) ].
> 
> Yes, I take all the blame for the extra brackets. Regardless of having
> a masters in CS or not I still prefer too many then too few of them (;-
> p).
> 
> > > > > 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?
> > > > 
> > > > I'm OK either way and do not need any explicit authorship to be
> > > > expressed for me.
> > > 
> > > In this instance I suggest keeping Reported-by and Tested-by.
> > > 
> > > > > > 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);
> > > > 
> > > > you mean:
> > > > 
> > > > 		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.
> > > > 
> > > > No, then we are back to the initial issue of num_steps
> > > > potentially not
> > > > being initialised. We really want both of_property_read_u32() to
> > > > succeed AND num_steps to actually be set.
> > > 
> > > I also think num_steps should be pre-initialised.
> 
> Yes, I guess it definitely does not hurt.
> 
> > > Then it will only be set if of_property_read_u32() succeeds.
> 
> Yes, but we still need to check for both, the function not failing and
> num_steps to actually be non zero.

Why?  You don't do anything differently if it fails.
Marcel Ziswiler July 18, 2018, 1:26 p.m. | #9
On Wed, 2018-07-18 at 14:08 +0100, Lee Jones wrote:
> On Wed, 18 Jul 2018, Marcel Ziswiler wrote:
> 
> > On Wed, 2018-07-18 at 11:12 +0100, Daniel Thompson wrote:
> > > On Wed, Jul 18, 2018 at 10:53:35AM +0100, Lee Jones wrote:
> > > > On Wed, 18 Jul 2018, Marcel Ziswiler wrote:
> > > > 
> > > > > On Wed, 2018-07-18 at 09:09 +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.or
> > > > > > > g>
> > > > > > > Signed-off-by: Marcel Ziswiler <marcel.ziswiler@toradex.c
> > > > > > > om>
> > > > > > 
> > > > > > This line is confusing.  Did you guys author this patch
> > > > > > together?
> > > > > 
> > > > > Yes, I reported it and we came to a conclusion together.
> > > > 
> > > > It sounds like you need to have all of the tags (except this
> > > > one).
> > > > :)
> > > > 
> > > >  Reported-by:  for reporting the issue
> > > >  Suggested-by: for suggesting a resolution
> > > >  Acked-by:     for reviewing it
> > > >  Tested-by:    for testing it
> > > > 
> > > > Signed-off-by usually means you either wrote a significant
> > > > amount
> > > > of
> > > > the diffstat or you were part of the submission path.
> > > 
> > > He did [I don't object to but wouldn't have used the extra
> > > brackets
> > > you
> > > brought up ;-) ].
> > 
> > Yes, I take all the blame for the extra brackets. Regardless of
> > having
> > a masters in CS or not I still prefer too many then too few of them
> > (;-
> > p).
> > 
> > > > > > 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?
> > > > > 
> > > > > I'm OK either way and do not need any explicit authorship to
> > > > > be
> > > > > expressed for me.
> > > > 
> > > > In this instance I suggest keeping Reported-by and Tested-by.
> > > > 
> > > > > > > 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);
> > > > > 
> > > > > you mean:
> > > > > 
> > > > > 		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.
> > > > > 
> > > > > No, then we are back to the initial issue of num_steps
> > > > > potentially not
> > > > > being initialised. We really want both of_property_read_u32()
> > > > > to
> > > > > succeed AND num_steps to actually be set.
> > > > 
> > > > I also think num_steps should be pre-initialised.
> > 
> > Yes, I guess it definitely does not hurt.
> > 
> > > > Then it will only be set if of_property_read_u32() succeeds.
> > 
> > Yes, but we still need to check for both, the function not failing
> > and
> > num_steps to actually be non zero.
> 
> Why?  You don't do anything differently if it fails.

Well, maybe we should but given this being an optional property nobody
cared.
Daniel Thompson July 18, 2018, 1:41 p.m. | #10
On Wed, Jul 18, 2018 at 02:08:53PM +0100, Lee Jones wrote:
> > > > > No, then we are back to the initial issue of num_steps
> > > > > potentially not
> > > > > being initialised. We really want both of_property_read_u32() to
> > > > > succeed AND num_steps to actually be set.
> > > > 
> > > > I also think num_steps should be pre-initialised.
> > 
> > Yes, I guess it definitely does not hurt.
> > 
> > > > Then it will only be set if of_property_read_u32() succeeds.
> > 
> > Yes, but we still need to check for both, the function not failing and
> > num_steps to actually be non zero.
> 
> Why?  You don't do anything differently if it fails.

Only if you initialize num_steps...

We should either initialize to zero and not worry about the return
code[1] or we check the return code and not worry about
initialization[2]. I don't think both are worthwhile.

Whilst initialization can fix this specific instance we generally avoid
overusing it since it messes up static analysis and, in this instance,
distance from declaration to use is >25 lines, hence current patchset.


Daniel.


[1] https://lkml.org/lkml/2018/7/16/399
[2] https://lkml.org/lkml/2018/7/16/1042

Or...

We check the return code and leave number

num_steps is uninitialized and stack allocated so it only has a valid
value if of_property_read_u32() succeeds.

We can (and I originally did) fix the bug by initializing num_steps to 0
but its quite some distance between declaration and use so I accepted
Marcel's counter proposal to check the return code instead.


Daniel.
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones July 18, 2018, 3:55 p.m. | #11
On Wed, 18 Jul 2018, Daniel Thompson wrote:

> On Wed, Jul 18, 2018 at 02:08:53PM +0100, Lee Jones wrote:
> > > > > > No, then we are back to the initial issue of num_steps
> > > > > > potentially not
> > > > > > being initialised. We really want both of_property_read_u32() to
> > > > > > succeed AND num_steps to actually be set.
> > > > > 
> > > > > I also think num_steps should be pre-initialised.
> > > 
> > > Yes, I guess it definitely does not hurt.
> > > 
> > > > > Then it will only be set if of_property_read_u32() succeeds.
> > > 
> > > Yes, but we still need to check for both, the function not failing and
> > > num_steps to actually be non zero.
> > 
> > Why?  You don't do anything differently if it fails.
> 
> Only if you initialize num_steps...
> 
> We should either initialize to zero and not worry about the return
> code[1] or we check the return code and not worry about
> initialization[2]. I don't think both are worthwhile.
> 
> Whilst initialization can fix this specific instance we generally avoid
> overusing it since it messes up static analysis and, in this instance,
> distance from declaration to use is >25 lines, hence current patchset.
> 
> 
> Daniel.
> 
> 
> [1] https://lkml.org/lkml/2018/7/16/399
> [2] https://lkml.org/lkml/2018/7/16/1042
> 
> Or...
> 
> We check the return code and leave number
> 
> num_steps is uninitialized and stack allocated so it only has a valid
> value if of_property_read_u32() succeeds.
> 
> We can (and I originally did) fix the bug by initializing num_steps to 0
> but its quite some distance between declaration and use so I accepted
> Marcel's counter proposal to check the return code instead.

Only checking the return value of of_property_read_u32() is also
suitable.
Daniel Thompson July 18, 2018, 4:34 p.m. | #12
On Wed, Jul 18, 2018 at 04:55:44PM +0100, Lee Jones wrote:
> On Wed, 18 Jul 2018, Daniel Thompson wrote:
> 
> > On Wed, Jul 18, 2018 at 02:08:53PM +0100, Lee Jones wrote:
> > > > > > > No, then we are back to the initial issue of num_steps
> > > > > > > potentially not
> > > > > > > being initialised. We really want both of_property_read_u32() to
> > > > > > > succeed AND num_steps to actually be set.
> > > > > > 
> > > > > > I also think num_steps should be pre-initialised.
> > > > 
> > > > Yes, I guess it definitely does not hurt.
> > > > 
> > > > > > Then it will only be set if of_property_read_u32() succeeds.
> > > > 
> > > > Yes, but we still need to check for both, the function not failing and
> > > > num_steps to actually be non zero.
> > > 
> > > Why?  You don't do anything differently if it fails.
> > 
> > Only if you initialize num_steps...
> > 
> > We should either initialize to zero and not worry about the return
> > code[1] or we check the return code and not worry about
> > initialization[2]. I don't think both are worthwhile.
> > 
> > Whilst initialization can fix this specific instance we generally avoid
> > overusing it since it messes up static analysis and, in this instance,
> > distance from declaration to use is >25 lines, hence current patchset.
> > 
> > 
> > Daniel.
> > 
> > 
> > [1] https://lkml.org/lkml/2018/7/16/399
> > [2] https://lkml.org/lkml/2018/7/16/1042
> > 
> > Or...
> > 
> > We check the return code and leave number
> > 
> > num_steps is uninitialized and stack allocated so it only has a valid
> > value if of_property_read_u32() succeeds.
> > 
> > We can (and I originally did) fix the bug by initializing num_steps to 0
> > but its quite some distance between declaration and use so I accepted
> > Marcel's counter proposal to check the return code instead.
> 
> Only checking the return value of of_property_read_u32() is also
> suitable.

I did think about that case... I concluded that it isn't wrong for a
DT to set to this property to 0 (effectively meaning "no interpolated
steps please").

If we take the branch when num_steps is zero we get a bunch of pointless
housekeeping that amounts to no more than an extremely elaborate
malloc/memcpy/free.


Daniel.
--
To unsubscribe from this list: send the line "unsubscribe linux-pwm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lee Jones July 23, 2018, 7:25 a.m. | #13
On Wed, 18 Jul 2018, Daniel Thompson wrote:

> On Wed, Jul 18, 2018 at 04:55:44PM +0100, Lee Jones wrote:
> > On Wed, 18 Jul 2018, Daniel Thompson wrote:
> > 
> > > On Wed, Jul 18, 2018 at 02:08:53PM +0100, Lee Jones wrote:
> > > > > > > > No, then we are back to the initial issue of num_steps
> > > > > > > > potentially not
> > > > > > > > being initialised. We really want both of_property_read_u32() to
> > > > > > > > succeed AND num_steps to actually be set.
> > > > > > > 
> > > > > > > I also think num_steps should be pre-initialised.
> > > > > 
> > > > > Yes, I guess it definitely does not hurt.
> > > > > 
> > > > > > > Then it will only be set if of_property_read_u32() succeeds.
> > > > > 
> > > > > Yes, but we still need to check for both, the function not failing and
> > > > > num_steps to actually be non zero.
> > > > 
> > > > Why?  You don't do anything differently if it fails.
> > > 
> > > Only if you initialize num_steps...
> > > 
> > > We should either initialize to zero and not worry about the return
> > > code[1] or we check the return code and not worry about
> > > initialization[2]. I don't think both are worthwhile.
> > > 
> > > Whilst initialization can fix this specific instance we generally avoid
> > > overusing it since it messes up static analysis and, in this instance,
> > > distance from declaration to use is >25 lines, hence current patchset.
> > > 
> > > 
> > > Daniel.
> > > 
> > > 
> > > [1] https://lkml.org/lkml/2018/7/16/399
> > > [2] https://lkml.org/lkml/2018/7/16/1042
> > > 
> > > Or...
> > > 
> > > We check the return code and leave number
> > > 
> > > num_steps is uninitialized and stack allocated so it only has a valid
> > > value if of_property_read_u32() succeeds.
> > > 
> > > We can (and I originally did) fix the bug by initializing num_steps to 0
> > > but its quite some distance between declaration and use so I accepted
> > > Marcel's counter proposal to check the return code instead.
> > 
> > Only checking the return value of of_property_read_u32() is also
> > suitable.
> 
> I did think about that case... I concluded that it isn't wrong for a
> DT to set to this property to 0 (effectively meaning "no interpolated
> steps please").
> 
> If we take the branch when num_steps is zero we get a bunch of pointless
> housekeeping that amounts to no more than an extremely elaborate
> malloc/memcpy/free.

Yet in the latest patch, you do it anyway?  Or have I misread it?

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;