* [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-16 21:02 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-16 21:02 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han
Cc: Thierry Reding, Bartlomiej Zolnierkiewicz, linux-pwm, dri-devel,
linux-fbdev, linux-kernel, patches, Marcel Ziswiler
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(-)
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;
--
2.17.1
^ permalink raw reply related [flat|nested] 58+ messages in thread* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
2018-07-16 21:02 ` Daniel Thompson
@ 2018-07-18 8:09 ` Lee Jones
-1 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-18 8:09 UTC (permalink / raw)
To: Daniel Thompson
Cc: Jingoo Han, Thierry Reding, Bartlomiej Zolnierkiewicz, linux-pwm,
dri-devel, linux-fbdev, linux-kernel, patches, Marcel Ziswiler
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
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 8:09 ` Lee Jones
0 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-18 8:09 UTC (permalink / raw)
To: Daniel Thompson
Cc: Jingoo Han, Thierry Reding, Bartlomiej Zolnierkiewicz, linux-pwm,
dri-devel, linux-fbdev, linux-kernel, patches, Marcel Ziswiler
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
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
2018-07-18 8:09 ` Lee Jones
(?)
@ 2018-07-18 8:12 ` Lee Jones
-1 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-18 8:12 UTC (permalink / raw)
To: Daniel Thompson
Cc: linux-pwm, linux-fbdev, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, Jingoo Han, patches, linux-kernel, dri-devel,
Thierry Reding
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
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 8:12 ` Lee Jones
0 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-18 8:12 UTC (permalink / raw)
To: Daniel Thompson
Cc: Jingoo Han, Thierry Reding, Bartlomiej Zolnierkiewicz, linux-pwm,
dri-devel, linux-fbdev, linux-kernel, patches, Marcel Ziswiler
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
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 8:12 ` Lee Jones
0 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-18 8:12 UTC (permalink / raw)
To: Daniel Thompson
Cc: linux-pwm, linux-fbdev, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, Jingoo Han, patches, linux-kernel, dri-devel,
Thierry Reding
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
2018-07-18 8:09 ` Lee Jones
@ 2018-07-18 8:22 ` Marcel Ziswiler
-1 siblings, 0 replies; 58+ messages in thread
From: Marcel Ziswiler @ 2018-07-18 8:22 UTC (permalink / raw)
To: daniel.thompson@linaro.org, lee.jones@linaro.org
Cc: linux-kernel@vger.kernel.org, jingoohan1@gmail.com,
linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org,
b.zolnierkie@samsung.com, thierry.reding@gmail.com,
dri-devel@lists.freedesktop.org, patches@linaro.org
T24gV2VkLCAyMDE4LTA3LTE4IGF0IDA5OjA5ICswMTAwLCBMZWUgSm9uZXMgd3JvdGU6DQo+IE9u
IE1vbiwgMTYgSnVsIDIwMTgsIERhbmllbCBUaG9tcHNvbiB3cm90ZToNCj4gDQo+ID4gQ3VycmVu
dGx5LCBpZiB0aGUgRFQgZG9lcyBub3QgZGVmaW5lIG51bS1pbnRlcnBvbGF0ZWQtc3RlcHMgdGhl
bg0KPiA+IG51bV9zdGVwcyBpcyB1bmRlZmluZWQgYW5kIHRoZSBpbnRlcnBvbGF0aW9uIGNvZGUg
d2lsbCBkZXBsb3kNCj4gPiByYW5kb21seS4NCj4gPiBGaXggdGhpcy4NCj4gPiANCj4gPiBGaXhl
czogNTczZmU2ZDFjMjVjICgiYmFja2xpZ2h0OiBwd21fYmw6IExpbmVhciBpbnRlcnBvbGF0aW9u
DQo+ID4gYmV0d2Vlbg0KPiA+IGJyaWdodG5lc3MtbGV2ZWxzIikNCj4gPiBSZXBvcnRlZC1ieTog
TWFyY2VsIFppc3dpbGVyIDxtYXJjZWwuemlzd2lsZXJAdG9yYWRleC5jb20+DQo+ID4gU2lnbmVk
LW9mZi1ieTogRGFuaWVsIFRob21wc29uIDxkYW5pZWwudGhvbXBzb25AbGluYXJvLm9yZz4NCj4g
PiBTaWduZWQtb2ZmLWJ5OiBNYXJjZWwgWmlzd2lsZXIgPG1hcmNlbC56aXN3aWxlckB0b3JhZGV4
LmNvbT4NCj4gDQo+IFRoaXMgbGluZSBpcyBjb25mdXNpbmcuICBEaWQgeW91IGd1eXMgYXV0aG9y
IHRoaXMgcGF0Y2ggdG9nZXRoZXI/DQoNClllcywgSSByZXBvcnRlZCBpdCBhbmQgd2UgY2FtZSB0
byBhIGNvbmNsdXNpb24gdG9nZXRoZXIuDQoNCj4gTXkgZ3Vlc3MgaXMgdGhhdCB0aGlzIGxpbmUg
c2hvdWxkIGJlIGRyb3BwZWQgYW5kIHRoZSBSQiBhbmQgVEIgdGFncw0KPiBzaG91bGQgcmVtYWlu
PyAgSWYgaXQgd2FzIHJldmlld2VkIHRvbywgcGVyaGFwcyBhbiBBQiB0b28/DQoNCkknbSBPSyBl
aXRoZXIgd2F5IGFuZCBkbyBub3QgbmVlZCBhbnkgZXhwbGljaXQgYXV0aG9yc2hpcCB0byBiZQ0K
ZXhwcmVzc2VkIGZvciBtZS4NCg0KPiA+IFRlc3RlZC1ieTogTWFyY2VsIFppc3dpbGVyIDxtYXJj
ZWwuemlzd2lsZXJAdG9yYWRleC5jb20+DQo+ID4gLS0tDQo+ID4gIGRyaXZlcnMvdmlkZW8vYmFj
a2xpZ2h0L3B3bV9ibC5jIHwgMTcgKysrKysrKystLS0tLS0tLS0NCj4gPiAgMSBmaWxlIGNoYW5n
ZWQsIDggaW5zZXJ0aW9ucygrKSwgOSBkZWxldGlvbnMoLSkNCj4gPiANCj4gPiBkaWZmIC0tZ2l0
IGEvZHJpdmVycy92aWRlby9iYWNrbGlnaHQvcHdtX2JsLmMNCj4gPiBiL2RyaXZlcnMvdmlkZW8v
YmFja2xpZ2h0L3B3bV9ibC5jDQo+ID4gaW5kZXggOWVlNGMxYjczNWIyLi5lM2MyMmI3OWZiY2Qg
MTAwNjQ0DQo+ID4gLS0tIGEvZHJpdmVycy92aWRlby9iYWNrbGlnaHQvcHdtX2JsLmMNCj4gPiAr
KysgYi9kcml2ZXJzL3ZpZGVvL2JhY2tsaWdodC9wd21fYmwuYw0KPiA+IEBAIC0yOTksMTUgKzI5
OSwxNCBAQCBzdGF0aWMgaW50IHB3bV9iYWNrbGlnaHRfcGFyc2VfZHQoc3RydWN0DQo+ID4gZGV2
aWNlICpkZXYsDQo+ID4gIAkJICogaW50ZXJwb2xhdGlvbiBiZXR3ZWVuIGVhY2ggb2YgdGhlIHZh
bHVlcyBvZg0KPiA+IGJyaWdodG5lc3MgbGV2ZWxzDQo+ID4gIAkJICogYW5kIGNyZWF0ZXMgYSBu
ZXcgcHJlLWNvbXB1dGVkIHRhYmxlLg0KPiA+ICAJCSAqLw0KPiA+IC0JCW9mX3Byb3BlcnR5X3Jl
YWRfdTMyKG5vZGUsICJudW0taW50ZXJwb2xhdGVkLQ0KPiA+IHN0ZXBzIiwNCj4gPiAtCQkJCSAg
ICAgJm51bV9zdGVwcyk7DQo+ID4gLQ0KPiA+IC0JCS8qDQo+ID4gLQkJICogTWFrZSBzdXJlIHRo
YXQgdGhlcmUgaXMgYXQgbGVhc3QgdHdvIGVudHJpZXMgaW4NCj4gPiB0aGUNCj4gPiAtCQkgKiBi
cmlnaHRuZXNzLWxldmVscyB0YWJsZSwgb3RoZXJ3aXNlIHdlIGNhbid0DQo+ID4gaW50ZXJwb2xh
dGUNCj4gPiAtCQkgKiBiZXR3ZWVuIHR3byBwb2ludHMuDQo+ID4gLQkJICovDQo+ID4gLQkJaWYg
KG51bV9zdGVwcykgew0KPiA+ICsJCWlmICgob2ZfcHJvcGVydHlfcmVhZF91MzIobm9kZSwgIm51
bS1pbnRlcnBvbGF0ZWQtDQo+ID4gc3RlcHMiLA0KPiA+ICsJCQkJCSAgJm51bV9zdGVwcykgPT0g
MCkgJiYNCj4gPiBudW1fc3RlcHMpIHsNCj4gDQo+IFRoaXMgaXMgcHJldHR5IHVnbHksIGFuZCBp
c24ndCBpdCBzdWZmZXJpbmcgZnJvbSBvdmVyLWJyYWNrZXRpbmc/ICBNeQ0KPiBzdWdnZXN0aW9u
IHdvdWxkIGJlIHRvIGJyZWFrIG91dCB0aGUgaW52b2NhdGlvbiBvZg0KPiBvZl9wcm9wZXJ0eV9y
ZWFkX3UzMigpIGZyb20gdGhlIGlmIGFuZCB0ZXN0IG9ubHkgdGhlIHJlc3VsdC4NCj4gDQo+IAkJ
b2ZfcHJvcGVydHlfcmVhZF91MzIobm9kZSwgIm51bS1pbnRlcnBvbGF0ZWQtc3RlcHMiLCANCj4g
Jm51bV9zdGVwcyk7DQoNCnlvdSBtZWFuOg0KDQoJCXJldCA9IG9mX3Byb3BlcnR5X3JlYWRfdTMy
KG5vZGUsICJudW0taW50ZXJwb2xhdGVkLQ0Kc3RlcHMiLCAmbnVtX3N0ZXBzKTsNCg0KPiAJCWlm
ICghcmV0ICYmIG51bV9zdGVwcykgew0KPiANCj4gSSBoYXZlbid0IGNoZWNrZWQgdGhlIHVuZGVy
bGluZyBjb2RlLCBidXQgaXMgaXQgZXZlbiBmZWFzaWJsZSBmb3INCj4gb2ZfcHJvcGVydHlfcmVh
ZF91MzIoKSB0byBub3Qgc3VjY2VlZCBBTkQgZm9yIG51bV9zdGVwcyB0byBiZSBzZXQ/DQo+IA0K
PiBJZiBub3QsIHRoZSBjaGVjayBmb3IgIXJldCBpZiBzdXBlcmZsdW91cyBhbmQgeW91IGNhbiBk
cm9wIGl0Lg0KDQpObywgdGhlbiB3ZSBhcmUgYmFjayB0byB0aGUgaW5pdGlhbCBpc3N1ZSBvZiBu
dW1fc3RlcHMgcG90ZW50aWFsbHkgbm90DQpiZWluZyBpbml0aWFsaXNlZC4gV2UgcmVhbGx5IHdh
bnQgYm90aCBvZl9wcm9wZXJ0eV9yZWFkX3UzMigpIHRvDQpzdWNjZWVkIEFORCBudW1fc3RlcHMg
dG8gYWN0dWFsbHkgYmUgc2V0Lg0KDQo+ID4gKwkJCS8qDQo+ID4gKwkJCSAqIE1ha2Ugc3VyZSB0
aGF0IHRoZXJlIGlzIGF0IGxlYXN0IHR3bw0KPiA+IGVudHJpZXMgaW4gdGhlDQo+IA0KPiBzL2lz
L2FyZS8NCj4gDQo+ID4gKwkJCSAqIGJyaWdodG5lc3MtbGV2ZWxzIHRhYmxlLCBvdGhlcndpc2Ug
d2UNCj4gPiBjYW4ndA0KPiA+ICsJCQkgKiBpbnRlcnBvbGF0ZQ0KPiANCj4gV2h5IGJyZWFrIHRo
ZSBsaW5lIGhlcmU/DQoNClRoYXQncyBwcm9iYWJseSBhIHJlbW5hbnQgb2YgZ29pbmcgYmFjayBh
bmQgZm9ydGggcGx1cyBxdW90aW5nIG9uIHRoZQ0KbWFpbGluZyBsaXN0Lg0KDQo+ID4gKwkJCSAq
IGJldHdlZW4gdHdvIHBvaW50cy4NCj4gPiArCQkJICovDQo+ID4gIAkJCWlmIChkYXRhLT5tYXhf
YnJpZ2h0bmVzcyA8IDIpIHsNCj4gPiAgCQkJCWRldl9lcnIoZGV2LCAiY2FuJ3QNCj4gPiBpbnRl
cnBvbGF0ZVxuIik7DQo+ID4gIAkJCQlyZXR1cm4gLUVJTlZBTDs
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 8:22 ` Marcel Ziswiler
0 siblings, 0 replies; 58+ messages in thread
From: Marcel Ziswiler @ 2018-07-18 8:22 UTC (permalink / raw)
To: daniel.thompson@linaro.org, lee.jones@linaro.org
Cc: linux-kernel@vger.kernel.org, jingoohan1@gmail.com,
linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org,
b.zolnierkie@samsung.com, thierry.reding@gmail.com,
dri-devel@lists.freedesktop.org, patches@linaro.org
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;
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
2018-07-18 8:22 ` Marcel Ziswiler
(?)
@ 2018-07-18 9:53 ` Lee Jones
-1 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-18 9:53 UTC (permalink / raw)
To: Marcel Ziswiler
Cc: linux-pwm@vger.kernel.org, daniel.thompson@linaro.org,
b.zolnierkie@samsung.com, jingoohan1@gmail.com,
patches@linaro.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, thierry.reding@gmail.com,
linux-fbdev@vger.kernel.org
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.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 9:53 ` Lee Jones
0 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-18 9:53 UTC (permalink / raw)
To: Marcel Ziswiler
Cc: daniel.thompson@linaro.org, linux-kernel@vger.kernel.org,
jingoohan1@gmail.com, linux-pwm@vger.kernel.org,
linux-fbdev@vger.kernel.org, b.zolnierkie@samsung.com,
thierry.reding@gmail.com, dri-devel@lists.freedesktop.org,
patches@linaro.org
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.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 9:53 ` Lee Jones
0 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-18 9:53 UTC (permalink / raw)
To: Marcel Ziswiler
Cc: linux-pwm@vger.kernel.org, daniel.thompson@linaro.org,
b.zolnierkie@samsung.com, jingoohan1@gmail.com,
patches@linaro.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, thierry.reding@gmail.com,
linux-fbdev@vger.kernel.org
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.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
2018-07-18 9:53 ` Lee Jones
@ 2018-07-18 10:12 ` Daniel Thompson
-1 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-18 10:12 UTC (permalink / raw)
To: Lee Jones
Cc: Marcel Ziswiler, linux-kernel@vger.kernel.org,
jingoohan1@gmail.com, linux-pwm@vger.kernel.org,
linux-fbdev@vger.kernel.org, b.zolnierkie@samsung.com,
thierry.reding@gmail.com, dri-devel@lists.freedesktop.org,
patches@linaro.org
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
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 10:12 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-18 10:12 UTC (permalink / raw)
To: Lee Jones
Cc: Marcel Ziswiler, linux-kernel@vger.kernel.org,
jingoohan1@gmail.com, linux-pwm@vger.kernel.org,
linux-fbdev@vger.kernel.org, b.zolnierkie@samsung.com,
thierry.reding@gmail.com, dri-devel@lists.freedesktop.org,
patches@linaro.org
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
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
2018-07-18 10:12 ` Daniel Thompson
@ 2018-07-18 12:57 ` Marcel Ziswiler
-1 siblings, 0 replies; 58+ messages in thread
From: Marcel Ziswiler @ 2018-07-18 12:57 UTC (permalink / raw)
To: daniel.thompson@linaro.org, lee.jones@linaro.org
Cc: linux-kernel@vger.kernel.org, jingoohan1@gmail.com,
linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org,
b.zolnierkie@samsung.com, thierry.reding@gmail.com,
dri-devel@lists.freedesktop.org, patches@linaro.org
T24gV2VkLCAyMDE4LTA3LTE4IGF0IDExOjEyICswMTAwLCBEYW5pZWwgVGhvbXBzb24gd3JvdGU6
DQo+IE9uIFdlZCwgSnVsIDE4LCAyMDE4IGF0IDEwOjUzOjM1QU0gKzAxMDAsIExlZSBKb25lcyB3
cm90ZToNCj4gPiBPbiBXZWQsIDE4IEp1bCAyMDE4LCBNYXJjZWwgWmlzd2lsZXIgd3JvdGU6DQo+
ID4gDQo+ID4gPiBPbiBXZWQsIDIwMTgtMDctMTggYXQgMDk6MDkgKzAxMDAsIExlZSBKb25lcyB3
cm90ZToNCj4gPiA+ID4gT24gTW9uLCAxNiBKdWwgMjAxOCwgRGFuaWVsIFRob21wc29uIHdyb3Rl
Og0KPiA+ID4gPiANCj4gPiA+ID4gPiBDdXJyZW50bHksIGlmIHRoZSBEVCBkb2VzIG5vdCBkZWZp
bmUgbnVtLWludGVycG9sYXRlZC1zdGVwcw0KPiA+ID4gPiA+IHRoZW4NCj4gPiA+ID4gPiBudW1f
c3RlcHMgaXMgdW5kZWZpbmVkIGFuZCB0aGUgaW50ZXJwb2xhdGlvbiBjb2RlIHdpbGwgZGVwbG95
DQo+ID4gPiA+ID4gcmFuZG9tbHkuDQo+ID4gPiA+ID4gRml4IHRoaXMuDQo+ID4gPiA+ID4gDQo+
ID4gPiA+ID4gRml4ZXM6IDU3M2ZlNmQxYzI1YyAoImJhY2tsaWdodDogcHdtX2JsOiBMaW5lYXIg
aW50ZXJwb2xhdGlvbg0KPiA+ID4gPiA+IGJldHdlZW4NCj4gPiA+ID4gPiBicmlnaHRuZXNzLWxl
dmVscyIpDQo+ID4gPiA+ID4gUmVwb3J0ZWQtYnk6IE1hcmNlbCBaaXN3aWxlciA8bWFyY2VsLnpp
c3dpbGVyQHRvcmFkZXguY29tPg0KPiA+ID4gPiA+IFNpZ25lZC1vZmYtYnk6IERhbmllbCBUaG9t
cHNvbiA8ZGFuaWVsLnRob21wc29uQGxpbmFyby5vcmc+DQo+ID4gPiA+ID4gU2lnbmVkLW9mZi1i
eTogTWFyY2VsIFppc3dpbGVyIDxtYXJjZWwuemlzd2lsZXJAdG9yYWRleC5jb20+DQo+ID4gPiA+
IA0KPiA+ID4gPiBUaGlzIGxpbmUgaXMgY29uZnVzaW5nLiAgRGlkIHlvdSBndXlzIGF1dGhvciB0
aGlzIHBhdGNoDQo+ID4gPiA+IHRvZ2V0aGVyPw0KPiA+ID4gDQo+ID4gPiBZZXMsIEkgcmVwb3J0
ZWQgaXQgYW5kIHdlIGNhbWUgdG8gYSBjb25jbHVzaW9uIHRvZ2V0aGVyLg0KPiA+IA0KPiA+IEl0
IHNvdW5kcyBsaWtlIHlvdSBuZWVkIHRvIGhhdmUgYWxsIG9mIHRoZSB0YWdzIChleGNlcHQgdGhp
cyBvbmUpLg0KPiA+IDopDQo+ID4gDQo+ID4gIFJlcG9ydGVkLWJ5OiAgZm9yIHJlcG9ydGluZyB0
aGUgaXNzdWUNCj4gPiAgU3VnZ2VzdGVkLWJ5OiBmb3Igc3VnZ2VzdGluZyBhIHJlc29sdXRpb24N
Cj4gPiAgQWNrZWQtYnk6ICAgICBmb3IgcmV2aWV3aW5nIGl0DQo+ID4gIFRlc3RlZC1ieTogICAg
Zm9yIHRlc3RpbmcgaXQNCj4gPiANCj4gPiBTaWduZWQtb2ZmLWJ5IHVzdWFsbHkgbWVhbnMgeW91
IGVpdGhlciB3cm90ZSBhIHNpZ25pZmljYW50IGFtb3VudA0KPiA+IG9mDQo+ID4gdGhlIGRpZmZz
dGF0IG9yIHlvdSB3ZXJlIHBhcnQgb2YgdGhlIHN1Ym1pc3Npb24gcGF0aC4NCj4gDQo+IEhlIGRp
ZCBbSSBkb24ndCBvYmplY3QgdG8gYnV0IHdvdWxkbid0IGhhdmUgdXNlZCB0aGUgZXh0cmEgYnJh
Y2tldHMNCj4geW91DQo+IGJyb3VnaHQgdXAgOy0pIF0uDQoNClllcywgSSB0YWtlIGFsbCB0aGUg
YmxhbWUgZm9yIHRoZSBleHRyYSBicmFja2V0cy4gUmVnYXJkbGVzcyBvZiBoYXZpbmcNCmEgbWFz
dGVycyBpbiBDUyBvciBub3QgSSBzdGlsbCBwcmVmZXIgdG9vIG1hbnkgdGhlbiB0b28gZmV3IG9m
IHRoZW0gKDstDQpwKS4NCg0KPiA+ID4gPiBNeSBndWVzcyBpcyB0aGF0IHRoaXMgbGluZSBzaG91
bGQgYmUgZHJvcHBlZCBhbmQgdGhlIFJCIGFuZCBUQg0KPiA+ID4gPiB0YWdzDQo+ID4gPiA+IHNo
b3VsZCByZW1haW4/ICBJZiBpdCB3YXMgcmV2aWV3ZWQgdG9vLCBwZXJoYXBzIGFuIEFCIHRvbz8N
Cj4gPiA+IA0KPiA+ID4gSSdtIE9LIGVpdGhlciB3YXkgYW5kIGRvIG5vdCBuZWVkIGFueSBleHBs
aWNpdCBhdXRob3JzaGlwIHRvIGJlDQo+ID4gPiBleHByZXNzZWQgZm9yIG1lLg0KPiA+IA0KPiA+
IEluIHRoaXMgaW5zdGFuY2UgSSBzdWdnZXN0IGtlZXBpbmcgUmVwb3J0ZWQtYnkgYW5kIFRlc3Rl
ZC1ieS4NCj4gPiANCj4gPiA+ID4gPiBUZXN0ZWQtYnk6IE1hcmNlbCBaaXN3aWxlciA8bWFyY2Vs
Lnppc3dpbGVyQHRvcmFkZXguY29tPg0KPiA+ID4gPiA+IC0tLQ0KPiA+ID4gPiA+ICBkcml2ZXJz
L3ZpZGVvL2JhY2tsaWdodC9wd21fYmwuYyB8IDE3ICsrKysrKysrLS0tLS0tLS0tDQo+ID4gPiA+
ID4gIDEgZmlsZSBjaGFuZ2VkLCA4IGluc2VydGlvbnMoKyksIDkgZGVsZXRpb25zKC0pDQo+ID4g
PiA+ID4gDQo+ID4gPiA+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvdmlkZW8vYmFja2xpZ2h0L3B3
bV9ibC5jDQo+ID4gPiA+ID4gYi9kcml2ZXJzL3ZpZGVvL2JhY2tsaWdodC9wd21fYmwuYw0KPiA+
ID4gPiA+IGluZGV4IDllZTRjMWI3MzViMi4uZTNjMjJiNzlmYmNkIDEwMDY0NA0KPiA+ID4gPiA+
IC0tLSBhL2RyaXZlcnMvdmlkZW8vYmFja2xpZ2h0L3B3bV9ibC5jDQo+ID4gPiA+ID4gKysrIGIv
ZHJpdmVycy92aWRlby9iYWNrbGlnaHQvcHdtX2JsLmMNCj4gPiA+ID4gPiBAQCAtMjk5LDE1ICsy
OTksMTQgQEAgc3RhdGljIGludA0KPiA+ID4gPiA+IHB3bV9iYWNrbGlnaHRfcGFyc2VfZHQoc3Ry
dWN0DQo+ID4gPiA+ID4gZGV2aWNlICpkZXYsDQo+ID4gPiA+ID4gIAkJICogaW50ZXJwb2xhdGlv
biBiZXR3ZWVuIGVhY2ggb2YgdGhlIHZhbHVlcw0KPiA+ID4gPiA+IG9mDQo+ID4gPiA+ID4gYnJp
Z2h0bmVzcyBsZXZlbHMNCj4gPiA+ID4gPiAgCQkgKiBhbmQgY3JlYXRlcyBhIG5ldyBwcmUtY29t
cHV0ZWQgdGFibGUuDQo+ID4gPiA+ID4gIAkJICovDQo+ID4gPiA+ID4gLQkJb2ZfcHJvcGVydHlf
cmVhZF91MzIobm9kZSwgIm51bS0NCj4gPiA+ID4gPiBpbnRlcnBvbGF0ZWQtDQo+ID4gPiA+ID4g
c3RlcHMiLA0KPiA+ID4gPiA+IC0JCQkJICAgICAmbnVtX3N0ZXBzKTsNCj4gPiA+ID4gPiAtDQo+
ID4gPiA+ID4gLQkJLyoNCj4gPiA+ID4gPiAtCQkgKiBNYWtlIHN1cmUgdGhhdCB0aGVyZSBpcyBh
dCBsZWFzdCB0d28NCj4gPiA+ID4gPiBlbnRyaWVzIGluDQo+ID4gPiA+ID4gdGhlDQo+ID4gPiA+
ID4gLQkJICogYnJpZ2h0bmVzcy1sZXZlbHMgdGFibGUsIG90aGVyd2lzZSB3ZQ0KPiA+ID4gPiA+
IGNhbid0DQo+ID4gPiA+ID4gaW50ZXJwb2xhdGUNCj4gPiA+ID4gPiAtCQkgKiBiZXR3ZWVuIHR3
byBwb2ludHMuDQo+ID4gPiA+ID4gLQkJICovDQo+ID4gPiA+ID4gLQkJaWYgKG51bV9zdGVwcykg
ew0KPiA+ID4gPiA+ICsJCWlmICgob2ZfcHJvcGVydHlfcmVhZF91MzIobm9kZSwgIm51bS0NCj4g
PiA+ID4gPiBpbnRlcnBvbGF0ZWQtDQo+ID4gPiA+ID4gc3RlcHMiLA0KPiA+ID4gPiA+ICsJCQkJ
CSAgJm51bV9zdGVwcykgPT0gMCkNCj4gPiA+ID4gPiAmJg0KPiA+ID4gPiA+IG51bV9zdGVwcykg
ew0KPiA+ID4gPiANCj4gPiA+ID4gVGhpcyBpcyBwcmV0dHkgdWdseSwgYW5kIGlzbid0IGl0IHN1
ZmZlcmluZyBmcm9tIG92ZXItDQo+ID4gPiA+IGJyYWNrZXRpbmc/ICBNeQ0KPiA+ID4gPiBzdWdn
ZXN0aW9uIHdvdWxkIGJlIHRvIGJyZWFrIG91dCB0aGUgaW52b2NhdGlvbiBvZg0KPiA+ID4gPiBv
Zl9wcm9wZXJ0eV9yZWFkX3UzMigpIGZyb20gdGhlIGlmIGFuZCB0ZXN0IG9ubHkgdGhlIHJlc3Vs
dC4NCj4gPiA+ID4gDQo+ID4gPiA+IAkJb2ZfcHJvcGVydHlfcmVhZF91MzIobm9kZSwgIm51bS1p
bnRlcnBvbGF0ZWQtDQo+ID4gPiA+IHN0ZXBzIiwgDQo+ID4gPiA+ICZudW1fc3RlcHMpOw0KPiA+
ID4gDQo+ID4gPiB5b3UgbWVhbjoNCj4gPiA+IA0KPiA+ID4gCQlyZXQgPSBvZl9wcm9wZXJ0eV9y
ZWFkX3UzMihub2RlLCAibnVtLWludGVycG9sYXRlZC0NCj4gPiA+IHN0ZXBzIiwgJm51bV9zdGVw
cyk7DQo+ID4gPiANCj4gPiA+ID4gCQlpZiAoIXJldCAmJiBudW1fc3RlcHMpIHsNCj4gPiA+ID4g
DQo+ID4gPiA+IEkgaGF2ZW4ndCBjaGVja2VkIHRoZSB1bmRlcmxpbmcgY29kZSwgYnV0IGlzIGl0
IGV2ZW4gZmVhc2libGUNCj4gPiA+ID4gZm9yDQo+ID4gPiA+IG9mX3Byb3BlcnR5X3JlYWRfdTMy
KCkgdG8gbm90IHN1Y2NlZWQgQU5EIGZvciBudW1fc3RlcHMgdG8gYmUNCj4gPiA+ID4gc2V0Pw0K
PiA+ID4gPiANCj4gPiA+ID4gSWYgbm90LCB0aGUgY2hlY2sgZm9yICFyZXQgaWYgc3VwZXJmbHVv
dXMgYW5kIHlvdSBjYW4gZHJvcCBpdC4NCj4gPiA+IA0KPiA+ID4gTm8sIHRoZW4gd2UgYXJlIGJh
Y2sgdG8gdGhlIGluaXRpYWwgaXNzdWUgb2YgbnVtX3N0ZXBzDQo+ID4gPiBwb3RlbnRpYWxseSBu
b3QNCj4gPiA+IGJlaW5nIGluaXRpYWxpc2VkLiBXZSByZWFsbHkgd2FudCBib3RoIG9mX3Byb3Bl
cnR5X3JlYWRfdTMyKCkgdG8NCj4gPiA+IHN1Y2NlZWQgQU5EIG51bV9zdGVwcyB0byBhY3R1YWxs
eSBiZSBzZXQuDQo+ID4gDQo+ID4gSSBhbHNvIHRoaW5rIG51bV9zdGVwcyBzaG91bGQgYmUgcHJl
LWluaXRpYWxpc2VkLg0KDQpZZXMsIEkgZ3Vlc3MgaXQgZGVmaW5pdGVseSBkb2VzIG5vdCBodXJ0
Lg0KDQo+ID4gVGhlbiBpdCB3aWxsIG9ubHkgYmUgc2V0IGlmIG9mX3Byb3BlcnR5X3JlYWRfdTMy
KCkgc3VjY2VlZHMuDQoNClllcywgYnV0IHdlIHN0aWxsIG5lZWQgdG8gY2hlY2sgZm9yIGJvdGgs
IHRoZSBmdW5jdGlvbiBub3QgZmFpbGluZyBhbmQNCm51bV9zdGVwcyB0byBhY3R1YWxseSBiZSBu
b24gemVyby4NCg0KPiA+IC0tIA0KPiA+IExlZSBKb25lcyBb5p2O55C85pavXQ0KPiA+IExpbmFy
byBTZXJ2aWNlcyBUZWNobmljYWwgTGVhZA0KPiA+IExpbmFyby5vcmcg4pSCIE9wZW4gc291cmNl
IHNvZnR3YXJlIGZvciBBUk0gU29Dcw0KPiA+IEZvbGxvdyBMaW5hcm86IEZhY2Vib29rIHwgVHdp
dHRlciB8IEJsb2c
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 12:57 ` Marcel Ziswiler
0 siblings, 0 replies; 58+ messages in thread
From: Marcel Ziswiler @ 2018-07-18 12:57 UTC (permalink / raw)
To: daniel.thompson@linaro.org, lee.jones@linaro.org
Cc: linux-kernel@vger.kernel.org, jingoohan1@gmail.com,
linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org,
b.zolnierkie@samsung.com, thierry.reding@gmail.com,
dri-devel@lists.freedesktop.org, patches@linaro.org
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
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
2018-07-18 12:57 ` Marcel Ziswiler
@ 2018-07-18 13:08 ` Lee Jones
-1 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-18 13:08 UTC (permalink / raw)
To: Marcel Ziswiler
Cc: daniel.thompson@linaro.org, linux-kernel@vger.kernel.org,
jingoohan1@gmail.com, linux-pwm@vger.kernel.org,
linux-fbdev@vger.kernel.org, b.zolnierkie@samsung.com,
thierry.reding@gmail.com, dri-devel@lists.freedesktop.org,
patches@linaro.org
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.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 13:08 ` Lee Jones
0 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-18 13:08 UTC (permalink / raw)
To: Marcel Ziswiler
Cc: daniel.thompson@linaro.org, linux-kernel@vger.kernel.org,
jingoohan1@gmail.com, linux-pwm@vger.kernel.org,
linux-fbdev@vger.kernel.org, b.zolnierkie@samsung.com,
thierry.reding@gmail.com, dri-devel@lists.freedesktop.org,
patches@linaro.org
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.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
2018-07-18 13:08 ` Lee Jones
@ 2018-07-18 13:26 ` Marcel Ziswiler
-1 siblings, 0 replies; 58+ messages in thread
From: Marcel Ziswiler @ 2018-07-18 13:26 UTC (permalink / raw)
To: lee.jones@linaro.org
Cc: linux-kernel@vger.kernel.org, jingoohan1@gmail.com,
linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org,
b.zolnierkie@samsung.com, thierry.reding@gmail.com,
daniel.thompson@linaro.org, dri-devel@lists.freedesktop.org,
patches@linaro.org
T24gV2VkLCAyMDE4LTA3LTE4IGF0IDE0OjA4ICswMTAwLCBMZWUgSm9uZXMgd3JvdGU6DQo+IE9u
IFdlZCwgMTggSnVsIDIwMTgsIE1hcmNlbCBaaXN3aWxlciB3cm90ZToNCj4gDQo+ID4gT24gV2Vk
LCAyMDE4LTA3LTE4IGF0IDExOjEyICswMTAwLCBEYW5pZWwgVGhvbXBzb24gd3JvdGU6DQo+ID4g
PiBPbiBXZWQsIEp1bCAxOCwgMjAxOCBhdCAxMDo1MzozNUFNICswMTAwLCBMZWUgSm9uZXMgd3Jv
dGU6DQo+ID4gPiA+IE9uIFdlZCwgMTggSnVsIDIwMTgsIE1hcmNlbCBaaXN3aWxlciB3cm90ZToN
Cj4gPiA+ID4gDQo+ID4gPiA+ID4gT24gV2VkLCAyMDE4LTA3LTE4IGF0IDA5OjA5ICswMTAwLCBM
ZWUgSm9uZXMgd3JvdGU6DQo+ID4gPiA+ID4gPiBPbiBNb24sIDE2IEp1bCAyMDE4LCBEYW5pZWwg
VGhvbXBzb24gd3JvdGU6DQo+ID4gPiA+ID4gPiANCj4gPiA+ID4gPiA+ID4gQ3VycmVudGx5LCBp
ZiB0aGUgRFQgZG9lcyBub3QgZGVmaW5lIG51bS1pbnRlcnBvbGF0ZWQtDQo+ID4gPiA+ID4gPiA+
IHN0ZXBzDQo+ID4gPiA+ID4gPiA+IHRoZW4NCj4gPiA+ID4gPiA+ID4gbnVtX3N0ZXBzIGlzIHVu
ZGVmaW5lZCBhbmQgdGhlIGludGVycG9sYXRpb24gY29kZSB3aWxsDQo+ID4gPiA+ID4gPiA+IGRl
cGxveQ0KPiA+ID4gPiA+ID4gPiByYW5kb21seS4NCj4gPiA+ID4gPiA+ID4gRml4IHRoaXMuDQo+
ID4gPiA+ID4gPiA+IA0KPiA+ID4gPiA+ID4gPiBGaXhlczogNTczZmU2ZDFjMjVjICgiYmFja2xp
Z2h0OiBwd21fYmw6IExpbmVhcg0KPiA+ID4gPiA+ID4gPiBpbnRlcnBvbGF0aW9uDQo+ID4gPiA+
ID4gPiA+IGJldHdlZW4NCj4gPiA+ID4gPiA+ID4gYnJpZ2h0bmVzcy1sZXZlbHMiKQ0KPiA+ID4g
PiA+ID4gPiBSZXBvcnRlZC1ieTogTWFyY2VsIFppc3dpbGVyIDxtYXJjZWwuemlzd2lsZXJAdG9y
YWRleC5jb20NCj4gPiA+ID4gPiA+ID4gPg0KPiA+ID4gPiA+ID4gPiBTaWduZWQtb2ZmLWJ5OiBE
YW5pZWwgVGhvbXBzb24gPGRhbmllbC50aG9tcHNvbkBsaW5hcm8ub3INCj4gPiA+ID4gPiA+ID4g
Zz4NCj4gPiA+ID4gPiA+ID4gU2lnbmVkLW9mZi1ieTogTWFyY2VsIFppc3dpbGVyIDxtYXJjZWwu
emlzd2lsZXJAdG9yYWRleC5jDQo+ID4gPiA+ID4gPiA+IG9tPg0KPiA+ID4gPiA+ID4gDQo+ID4g
PiA+ID4gPiBUaGlzIGxpbmUgaXMgY29uZnVzaW5nLiAgRGlkIHlvdSBndXlzIGF1dGhvciB0aGlz
IHBhdGNoDQo+ID4gPiA+ID4gPiB0b2dldGhlcj8NCj4gPiA+ID4gPiANCj4gPiA+ID4gPiBZZXMs
IEkgcmVwb3J0ZWQgaXQgYW5kIHdlIGNhbWUgdG8gYSBjb25jbHVzaW9uIHRvZ2V0aGVyLg0KPiA+
ID4gPiANCj4gPiA+ID4gSXQgc291bmRzIGxpa2UgeW91IG5lZWQgdG8gaGF2ZSBhbGwgb2YgdGhl
IHRhZ3MgKGV4Y2VwdCB0aGlzDQo+ID4gPiA+IG9uZSkuDQo+ID4gPiA+IDopDQo+ID4gPiA+IA0K
PiA+ID4gPiAgUmVwb3J0ZWQtYnk6ICBmb3IgcmVwb3J0aW5nIHRoZSBpc3N1ZQ0KPiA+ID4gPiAg
U3VnZ2VzdGVkLWJ5OiBmb3Igc3VnZ2VzdGluZyBhIHJlc29sdXRpb24NCj4gPiA+ID4gIEFja2Vk
LWJ5OiAgICAgZm9yIHJldmlld2luZyBpdA0KPiA+ID4gPiAgVGVzdGVkLWJ5OiAgICBmb3IgdGVz
dGluZyBpdA0KPiA+ID4gPiANCj4gPiA+ID4gU2lnbmVkLW9mZi1ieSB1c3VhbGx5IG1lYW5zIHlv
dSBlaXRoZXIgd3JvdGUgYSBzaWduaWZpY2FudA0KPiA+ID4gPiBhbW91bnQNCj4gPiA+ID4gb2YN
Cj4gPiA+ID4gdGhlIGRpZmZzdGF0IG9yIHlvdSB3ZXJlIHBhcnQgb2YgdGhlIHN1Ym1pc3Npb24g
cGF0aC4NCj4gPiA+IA0KPiA+ID4gSGUgZGlkIFtJIGRvbid0IG9iamVjdCB0byBidXQgd291bGRu
J3QgaGF2ZSB1c2VkIHRoZSBleHRyYQ0KPiA+ID4gYnJhY2tldHMNCj4gPiA+IHlvdQ0KPiA+ID4g
YnJvdWdodCB1cCA7LSkgXS4NCj4gPiANCj4gPiBZZXMsIEkgdGFrZSBhbGwgdGhlIGJsYW1lIGZv
ciB0aGUgZXh0cmEgYnJhY2tldHMuIFJlZ2FyZGxlc3Mgb2YNCj4gPiBoYXZpbmcNCj4gPiBhIG1h
c3RlcnMgaW4gQ1Mgb3Igbm90IEkgc3RpbGwgcHJlZmVyIHRvbyBtYW55IHRoZW4gdG9vIGZldyBv
ZiB0aGVtDQo+ID4gKDstDQo+ID4gcCkuDQo+ID4gDQo+ID4gPiA+ID4gPiBNeSBndWVzcyBpcyB0
aGF0IHRoaXMgbGluZSBzaG91bGQgYmUgZHJvcHBlZCBhbmQgdGhlIFJCIGFuZA0KPiA+ID4gPiA+
ID4gVEINCj4gPiA+ID4gPiA+IHRhZ3MNCj4gPiA+ID4gPiA+IHNob3VsZCByZW1haW4/ICBJZiBp
dCB3YXMgcmV2aWV3ZWQgdG9vLCBwZXJoYXBzIGFuIEFCIHRvbz8NCj4gPiA+ID4gPiANCj4gPiA+
ID4gPiBJJ20gT0sgZWl0aGVyIHdheSBhbmQgZG8gbm90IG5lZWQgYW55IGV4cGxpY2l0IGF1dGhv
cnNoaXAgdG8NCj4gPiA+ID4gPiBiZQ0KPiA+ID4gPiA+IGV4cHJlc3NlZCBmb3IgbWUuDQo+ID4g
PiA+IA0KPiA+ID4gPiBJbiB0aGlzIGluc3RhbmNlIEkgc3VnZ2VzdCBrZWVwaW5nIFJlcG9ydGVk
LWJ5IGFuZCBUZXN0ZWQtYnkuDQo+ID4gPiA+IA0KPiA+ID4gPiA+ID4gPiBUZXN0ZWQtYnk6IE1h
cmNlbCBaaXN3aWxlciA8bWFyY2VsLnppc3dpbGVyQHRvcmFkZXguY29tPg0KPiA+ID4gPiA+ID4g
PiAtLS0NCj4gPiA+ID4gPiA+ID4gIGRyaXZlcnMvdmlkZW8vYmFja2xpZ2h0L3B3bV9ibC5jIHwg
MTcgKysrKysrKystLS0tLS0tLS0NCj4gPiA+ID4gPiA+ID4gIDEgZmlsZSBjaGFuZ2VkLCA4IGlu
c2VydGlvbnMoKyksIDkgZGVsZXRpb25zKC0pDQo+ID4gPiA+ID4gPiA+IA0KPiA+ID4gPiA+ID4g
PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy92aWRlby9iYWNrbGlnaHQvcHdtX2JsLmMNCj4gPiA+ID4g
PiA+ID4gYi9kcml2ZXJzL3ZpZGVvL2JhY2tsaWdodC9wd21fYmwuYw0KPiA+ID4gPiA+ID4gPiBp
bmRleCA5ZWU0YzFiNzM1YjIuLmUzYzIyYjc5ZmJjZCAxMDA2NDQNCj4gPiA+ID4gPiA+ID4gLS0t
IGEvZHJpdmVycy92aWRlby9iYWNrbGlnaHQvcHdtX2JsLmMNCj4gPiA+ID4gPiA+ID4gKysrIGIv
ZHJpdmVycy92aWRlby9iYWNrbGlnaHQvcHdtX2JsLmMNCj4gPiA+ID4gPiA+ID4gQEAgLTI5OSwx
NSArMjk5LDE0IEBAIHN0YXRpYyBpbnQNCj4gPiA+ID4gPiA+ID4gcHdtX2JhY2tsaWdodF9wYXJz
ZV9kdChzdHJ1Y3QNCj4gPiA+ID4gPiA+ID4gZGV2aWNlICpkZXYsDQo+ID4gPiA+ID4gPiA+ICAJ
CSAqIGludGVycG9sYXRpb24gYmV0d2VlbiBlYWNoIG9mIHRoZQ0KPiA+ID4gPiA+ID4gPiB2YWx1
ZXMNCj4gPiA+ID4gPiA+ID4gb2YNCj4gPiA+ID4gPiA+ID4gYnJpZ2h0bmVzcyBsZXZlbHMNCj4g
PiA+ID4gPiA+ID4gIAkJICogYW5kIGNyZWF0ZXMgYSBuZXcgcHJlLWNvbXB1dGVkIHRhYmxlLg0K
PiA+ID4gPiA+ID4gPiAgCQkgKi8NCj4gPiA+ID4gPiA+ID4gLQkJb2ZfcHJvcGVydHlfcmVhZF91
MzIobm9kZSwgIm51bS0NCj4gPiA+ID4gPiA+ID4gaW50ZXJwb2xhdGVkLQ0KPiA+ID4gPiA+ID4g
PiBzdGVwcyIsDQo+ID4gPiA+ID4gPiA+IC0JCQkJICAgICAmbnVtX3N0ZXBzKTsNCj4gPiA+ID4g
PiA+ID4gLQ0KPiA+ID4gPiA+ID4gPiAtCQkvKg0KPiA+ID4gPiA+ID4gPiAtCQkgKiBNYWtlIHN1
cmUgdGhhdCB0aGVyZSBpcyBhdCBsZWFzdCB0d28NCj4gPiA+ID4gPiA+ID4gZW50cmllcyBpbg0K
PiA+ID4gPiA+ID4gPiB0aGUNCj4gPiA+ID4gPiA+ID4gLQkJICogYnJpZ2h0bmVzcy1sZXZlbHMg
dGFibGUsIG90aGVyd2lzZSB3ZQ0KPiA+ID4gPiA+ID4gPiBjYW4ndA0KPiA+ID4gPiA+ID4gPiBp
bnRlcnBvbGF0ZQ0KPiA+ID4gPiA+ID4gPiAtCQkgKiBiZXR3ZWVuIHR3byBwb2ludHMuDQo+ID4g
PiA+ID4gPiA+IC0JCSAqLw0KPiA+ID4gPiA+ID4gPiAtCQlpZiAobnVtX3N0ZXBzKSB7DQo+ID4g
PiA+ID4gPiA+ICsJCWlmICgob2ZfcHJvcGVydHlfcmVhZF91MzIobm9kZSwgIm51bS0NCj4gPiA+
ID4gPiA+ID4gaW50ZXJwb2xhdGVkLQ0KPiA+ID4gPiA+ID4gPiBzdGVwcyIsDQo+ID4gPiA+ID4g
PiA+ICsJCQkJCSAgJm51bV9zdGVwcykgPT0NCj4gPiA+ID4gPiA+ID4gMCkNCj4gPiA+ID4gPiA+
ID4gJiYNCj4gPiA+ID4gPiA+ID4gbnVtX3N0ZXBzKSB7DQo+ID4gPiA+ID4gPiANCj4gPiA+ID4g
PiA+IFRoaXMgaXMgcHJldHR5IHVnbHksIGFuZCBpc24ndCBpdCBzdWZmZXJpbmcgZnJvbSBvdmVy
LQ0KPiA+ID4gPiA+ID4gYnJhY2tldGluZz8gIE15DQo+ID4gPiA+ID4gPiBzdWdnZXN0aW9uIHdv
dWxkIGJlIHRvIGJyZWFrIG91dCB0aGUgaW52b2NhdGlvbiBvZg0KPiA+ID4gPiA+ID4gb2ZfcHJv
cGVydHlfcmVhZF91MzIoKSBmcm9tIHRoZSBpZiBhbmQgdGVzdCBvbmx5IHRoZQ0KPiA+ID4gPiA+
ID4gcmVzdWx0Lg0KPiA+ID4gPiA+ID4gDQo+ID4gPiA+ID4gPiAJCW9mX3Byb3BlcnR5X3JlYWRf
dTMyKG5vZGUsICJudW0taW50ZXJwb2xhdGVkLQ0KPiA+ID4gPiA+ID4gc3RlcHMiLCANCj4gPiA+
ID4gPiA+ICZudW1fc3RlcHMpOw0KPiA+ID4gPiA+IA0KPiA+ID4gPiA+IHlvdSBtZWFuOg0KPiA+
ID4gPiA+IA0KPiA+ID4gPiA+IAkJcmV0ID0gb2ZfcHJvcGVydHlfcmVhZF91MzIobm9kZSwgIm51
bS0NCj4gPiA+ID4gPiBpbnRlcnBvbGF0ZWQtDQo+ID4gPiA+ID4gc3RlcHMiLCAmbnVtX3N0ZXBz
KTsNCj4gPiA+ID4gPiANCj4gPiA+ID4gPiA+IAkJaWYgKCFyZXQgJiYgbnVtX3N0ZXBzKSB7DQo+
ID4gPiA+ID4gPiANCj4gPiA+ID4gPiA+IEkgaGF2ZW4ndCBjaGVja2VkIHRoZSB1bmRlcmxpbmcg
Y29kZSwgYnV0IGlzIGl0IGV2ZW4NCj4gPiA+ID4gPiA+IGZlYXNpYmxlDQo+ID4gPiA+ID4gPiBm
b3INCj4gPiA+ID4gPiA+IG9mX3Byb3BlcnR5X3JlYWRfdTMyKCkgdG8gbm90IHN1Y2NlZWQgQU5E
IGZvciBudW1fc3RlcHMgdG8NCj4gPiA+ID4gPiA+IGJlDQo+ID4gPiA+ID4gPiBzZXQ/DQo+ID4g
PiA+ID4gPiANCj4gPiA+ID4gPiA+IElmIG5vdCwgdGhlIGNoZWNrIGZvciAhcmV0IGlmIHN1cGVy
Zmx1b3VzIGFuZCB5b3UgY2FuIGRyb3ANCj4gPiA+ID4gPiA+IGl0Lg0KPiA+ID4gPiA+IA0KPiA+
ID4gPiA+IE5vLCB0aGVuIHdlIGFyZSBiYWNrIHRvIHRoZSBpbml0aWFsIGlzc3VlIG9mIG51bV9z
dGVwcw0KPiA+ID4gPiA+IHBvdGVudGlhbGx5IG5vdA0KPiA+ID4gPiA+IGJlaW5nIGluaXRpYWxp
c2VkLiBXZSByZWFsbHkgd2FudCBib3RoIG9mX3Byb3BlcnR5X3JlYWRfdTMyKCkNCj4gPiA+ID4g
PiB0bw0KPiA+ID4gPiA+IHN1Y2NlZWQgQU5EIG51bV9zdGVwcyB0byBhY3R1YWxseSBiZSBzZXQu
DQo+ID4gPiA+IA0KPiA+ID4gPiBJIGFsc28gdGhpbmsgbnVtX3N0ZXBzIHNob3VsZCBiZSBwcmUt
aW5pdGlhbGlzZWQuDQo+ID4gDQo+ID4gWWVzLCBJIGd1ZXNzIGl0IGRlZmluaXRlbHkgZG9lcyBu
b3QgaHVydC4NCj4gPiANCj4gPiA+ID4gVGhlbiBpdCB3aWxsIG9ubHkgYmUgc2V0IGlmIG9mX3By
b3BlcnR5X3JlYWRfdTMyKCkgc3VjY2VlZHMuDQo+ID4gDQo+ID4gWWVzLCBidXQgd2Ugc3RpbGwg
bmVlZCB0byBjaGVjayBmb3IgYm90aCwgdGhlIGZ1bmN0aW9uIG5vdCBmYWlsaW5nDQo+ID4gYW5k
DQo+ID4gbnVtX3N0ZXBzIHRvIGFjdHVhbGx5IGJlIG5vbiB6ZXJvLg0KPiANCj4gV2h5PyAgWW91
IGRvbid0IGRvIGFueXRoaW5nIGRpZmZlcmVudGx5IGlmIGl0IGZhaWxzLg0KDQpXZWxsLCBtYXli
ZSB3ZSBzaG91bGQgYnV0IGdpdmVuIHRoaXMgYmVpbmcgYW4gb3B0aW9uYWwgcHJvcGVydHkgbm9i
b2R5DQpjYXJlZC4
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 13:26 ` Marcel Ziswiler
0 siblings, 0 replies; 58+ messages in thread
From: Marcel Ziswiler @ 2018-07-18 13:26 UTC (permalink / raw)
To: lee.jones@linaro.org
Cc: linux-kernel@vger.kernel.org, jingoohan1@gmail.com,
linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org,
b.zolnierkie@samsung.com, thierry.reding@gmail.com,
daniel.thompson@linaro.org, dri-devel@lists.freedesktop.org,
patches@linaro.org
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.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
2018-07-18 13:08 ` Lee Jones
(?)
@ 2018-07-18 13:41 ` Daniel Thompson
-1 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-18 13:41 UTC (permalink / raw)
To: Lee Jones
Cc: linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org,
b.zolnierkie@samsung.com, Marcel Ziswiler, jingoohan1@gmail.com,
patches@linaro.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, thierry.reding@gmail.com
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.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 13:41 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-18 13:41 UTC (permalink / raw)
To: Lee Jones
Cc: Marcel Ziswiler, linux-kernel@vger.kernel.org,
jingoohan1@gmail.com, linux-pwm@vger.kernel.org,
linux-fbdev@vger.kernel.org, b.zolnierkie@samsung.com,
thierry.reding@gmail.com, dri-devel@lists.freedesktop.org,
patches@linaro.org
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.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 13:41 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-18 13:41 UTC (permalink / raw)
To: Lee Jones
Cc: linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org,
b.zolnierkie@samsung.com, Marcel Ziswiler, jingoohan1@gmail.com,
patches@linaro.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, thierry.reding@gmail.com
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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
2018-07-18 13:41 ` Daniel Thompson
(?)
@ 2018-07-18 15:55 ` Lee Jones
-1 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-18 15:55 UTC (permalink / raw)
To: Daniel Thompson
Cc: linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org,
b.zolnierkie@samsung.com, Marcel Ziswiler, jingoohan1@gmail.com,
patches@linaro.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, thierry.reding@gmail.com
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.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 15:55 ` Lee Jones
0 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-18 15:55 UTC (permalink / raw)
To: Daniel Thompson
Cc: Marcel Ziswiler, linux-kernel@vger.kernel.org,
jingoohan1@gmail.com, linux-pwm@vger.kernel.org,
linux-fbdev@vger.kernel.org, b.zolnierkie@samsung.com,
thierry.reding@gmail.com, dri-devel@lists.freedesktop.org,
patches@linaro.org
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.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 15:55 ` Lee Jones
0 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-18 15:55 UTC (permalink / raw)
To: Daniel Thompson
Cc: linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org,
b.zolnierkie@samsung.com, Marcel Ziswiler, jingoohan1@gmail.com,
patches@linaro.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, thierry.reding@gmail.com
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.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
2018-07-18 15:55 ` Lee Jones
(?)
@ 2018-07-18 16:34 ` Daniel Thompson
-1 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-18 16:34 UTC (permalink / raw)
To: Lee Jones
Cc: linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org,
b.zolnierkie@samsung.com, Marcel Ziswiler, jingoohan1@gmail.com,
patches@linaro.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, thierry.reding@gmail.com
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.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 16:34 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-18 16:34 UTC (permalink / raw)
To: Lee Jones
Cc: Marcel Ziswiler, linux-kernel@vger.kernel.org,
jingoohan1@gmail.com, linux-pwm@vger.kernel.org,
linux-fbdev@vger.kernel.org, b.zolnierkie@samsung.com,
thierry.reding@gmail.com, dri-devel@lists.freedesktop.org,
patches@linaro.org
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.
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 16:34 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-18 16:34 UTC (permalink / raw)
To: Lee Jones
Cc: linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org,
b.zolnierkie@samsung.com, Marcel Ziswiler, jingoohan1@gmail.com,
patches@linaro.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, thierry.reding@gmail.com
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.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
2018-07-18 16:34 ` Daniel Thompson
(?)
@ 2018-07-23 7:25 ` Lee Jones
-1 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-23 7:25 UTC (permalink / raw)
To: Daniel Thompson
Cc: linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org,
b.zolnierkie@samsung.com, Marcel Ziswiler, jingoohan1@gmail.com,
patches@linaro.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, thierry.reding@gmail.com
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?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-23 7:25 ` Lee Jones
0 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-23 7:25 UTC (permalink / raw)
To: Daniel Thompson
Cc: Marcel Ziswiler, linux-kernel@vger.kernel.org,
jingoohan1@gmail.com, linux-pwm@vger.kernel.org,
linux-fbdev@vger.kernel.org, b.zolnierkie@samsung.com,
thierry.reding@gmail.com, dri-devel@lists.freedesktop.org,
patches@linaro.org
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?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-23 7:25 ` Lee Jones
0 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-23 7:25 UTC (permalink / raw)
To: Daniel Thompson
Cc: linux-pwm@vger.kernel.org, linux-fbdev@vger.kernel.org,
b.zolnierkie@samsung.com, Marcel Ziswiler, jingoohan1@gmail.com,
patches@linaro.org, linux-kernel@vger.kernel.org,
dri-devel@lists.freedesktop.org, thierry.reding@gmail.com
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?
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
2018-07-18 8:09 ` Lee Jones
(?)
@ 2018-07-18 8:26 ` Daniel Thompson
-1 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-18 8:26 UTC (permalink / raw)
To: Lee Jones
Cc: linux-pwm, linux-fbdev, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, Jingoo Han, patches, linux-kernel, dri-devel,
Thierry Reding
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
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 8:26 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-18 8:26 UTC (permalink / raw)
To: Lee Jones
Cc: Jingoo Han, Thierry Reding, Bartlomiej Zolnierkiewicz, linux-pwm,
dri-devel, linux-fbdev, linux-kernel, patches, Marcel Ziswiler
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
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-18 8:26 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-18 8:26 UTC (permalink / raw)
To: Lee Jones
Cc: linux-pwm, linux-fbdev, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, Jingoo Han, patches, linux-kernel, dri-devel,
Thierry Reding
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v2] backlight: pwm_bl: Fix uninitialized variable
2018-07-16 21:02 ` Daniel Thompson
(?)
@ 2018-07-19 16:19 ` Daniel Thompson
-1 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-19 16:19 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han
Cc: linux-pwm, linux-fbdev, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, patches, linux-kernel, dri-devel, Thierry Reding
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(-)
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;
--
2.17.1
^ permalink raw reply related [flat|nested] 58+ messages in thread* [PATCH v2] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-19 16:19 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-19 16:19 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han
Cc: Thierry Reding, Bartlomiej Zolnierkiewicz, Marcel Ziswiler,
linux-pwm, dri-devel, linux-fbdev, linux-kernel, patches
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(-)
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;
--
2.17.1
^ permalink raw reply related [flat|nested] 58+ messages in thread* [PATCH v2] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-19 16:19 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-19 16:19 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han
Cc: linux-pwm, linux-fbdev, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, patches, linux-kernel, dri-devel, Thierry Reding
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(-)
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;
--
2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 58+ messages in thread* Re: [PATCH v2] backlight: pwm_bl: Fix uninitialized variable
2018-07-19 16:19 ` Daniel Thompson
@ 2018-07-23 7:23 ` Lee Jones
-1 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-23 7:23 UTC (permalink / raw)
To: Daniel Thompson
Cc: Jingoo Han, Thierry Reding, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, linux-pwm, dri-devel, linux-fbdev, linux-kernel,
patches
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
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v2] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-23 7:23 ` Lee Jones
0 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-23 7:23 UTC (permalink / raw)
To: Daniel Thompson
Cc: Jingoo Han, Thierry Reding, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, linux-pwm, dri-devel, linux-fbdev, linux-kernel,
patches
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
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v2] backlight: pwm_bl: Fix uninitialized variable
2018-07-23 7:23 ` Lee Jones
@ 2018-07-24 6:48 ` Daniel Thompson
-1 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-24 6:48 UTC (permalink / raw)
To: Lee Jones
Cc: Jingoo Han, Thierry Reding, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, linux-pwm, dri-devel, linux-fbdev, linux-kernel,
patches
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
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v2] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-24 6:48 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-24 6:48 UTC (permalink / raw)
To: Lee Jones
Cc: Jingoo Han, Thierry Reding, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, linux-pwm, dri-devel, linux-fbdev, linux-kernel,
patches
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
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v2] backlight: pwm_bl: Fix uninitialized variable
2018-07-23 7:23 ` Lee Jones
(?)
@ 2018-07-24 7:01 ` Daniel Thompson
-1 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-24 7:01 UTC (permalink / raw)
To: Lee Jones
Cc: linux-pwm, linux-fbdev, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, Jingoo Han, patches, linux-kernel, dri-devel,
Thierry Reding
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
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v2] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-24 7:01 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-24 7:01 UTC (permalink / raw)
To: Lee Jones
Cc: Jingoo Han, Thierry Reding, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, linux-pwm, dri-devel, linux-fbdev, linux-kernel,
patches
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
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v2] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-24 7:01 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-24 7:01 UTC (permalink / raw)
To: Lee Jones
Cc: linux-pwm, linux-fbdev, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, Jingoo Han, patches, linux-kernel, dri-devel,
Thierry Reding
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v3] backlight: pwm_bl: Fix uninitialized variable
2018-07-16 21:02 ` Daniel Thompson
(?)
@ 2018-07-24 7:12 ` Daniel Thompson
-1 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-24 7:12 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han
Cc: linux-pwm, linux-fbdev, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, patches, linux-kernel, dri-devel, Thierry Reding
Currently, if the DT does not define num-interpolated-steps then
num_steps is undefined and the interpolation code will deploy randomly.
Fix with a simple initialize to zero.
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:
v3:
- Switch to the simplest fix and zero the uninitialized variable. git
grep indicates that ~25% of calls to of_property_read_u32() use this
pattern (pre-initialize optional properties with sane values and
ignore the return code).
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 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 9ee4c1b735b2..bdfcc0a71db1 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
struct device_node *node = dev->of_node;
unsigned int num_levels = 0;
unsigned int levels_count;
- unsigned int num_steps;
+ unsigned int num_steps = 0;
struct property *prop;
unsigned int *table;
int length;
--
2.17.1
^ permalink raw reply related [flat|nested] 58+ messages in thread* [PATCH v3] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-24 7:12 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-24 7:12 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han
Cc: Thierry Reding, Bartlomiej Zolnierkiewicz, Marcel Ziswiler,
linux-pwm, dri-devel, linux-fbdev, linux-kernel, patches
Currently, if the DT does not define num-interpolated-steps then
num_steps is undefined and the interpolation code will deploy randomly.
Fix with a simple initialize to zero.
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:
v3:
- Switch to the simplest fix and zero the uninitialized variable. git
grep indicates that ~25% of calls to of_property_read_u32() use this
pattern (pre-initialize optional properties with sane values and
ignore the return code).
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 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 9ee4c1b735b2..bdfcc0a71db1 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
struct device_node *node = dev->of_node;
unsigned int num_levels = 0;
unsigned int levels_count;
- unsigned int num_steps;
+ unsigned int num_steps = 0;
struct property *prop;
unsigned int *table;
int length;
--
2.17.1
^ permalink raw reply related [flat|nested] 58+ messages in thread* [PATCH v3] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-24 7:12 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-24 7:12 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han
Cc: linux-pwm, linux-fbdev, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, patches, linux-kernel, dri-devel, Thierry Reding
Currently, if the DT does not define num-interpolated-steps then
num_steps is undefined and the interpolation code will deploy randomly.
Fix with a simple initialize to zero.
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:
v3:
- Switch to the simplest fix and zero the uninitialized variable. git
grep indicates that ~25% of calls to of_property_read_u32() use this
pattern (pre-initialize optional properties with sane values and
ignore the return code).
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 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 9ee4c1b735b2..bdfcc0a71db1 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
struct device_node *node = dev->of_node;
unsigned int num_levels = 0;
unsigned int levels_count;
- unsigned int num_steps;
+ unsigned int num_steps = 0;
struct property *prop;
unsigned int *table;
int length;
--
2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 58+ messages in thread* Re: [PATCH v3] backlight: pwm_bl: Fix uninitialized variable
2018-07-24 7:12 ` Daniel Thompson
@ 2018-07-24 23:56 ` Doug Anderson
-1 siblings, 0 replies; 58+ messages in thread
From: Doug Anderson @ 2018-07-24 23:56 UTC (permalink / raw)
To: Daniel Thompson
Cc: Lee Jones, Jingoo Han, Thierry Reding, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, linux-pwm, dri-devel, linux-fbdev, LKML,
Patch Tracking
Hi,
On Tue, Jul 24, 2018 at 12:12 AM, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
> Currently, if the DT does not define num-interpolated-steps then
> num_steps is undefined and the interpolation code will deploy randomly.
> Fix with a simple initialize to zero.
>
> Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation between
> brightness-levels")
I think it may be incorrect to word-wrap the Fixes line. A quick grep
through the source code looks like others agree.
> 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:
> v3:
> - Switch to the simplest fix and zero the uninitialized variable. git
> grep indicates that ~25% of calls to of_property_read_u32() use this
> pattern (pre-initialize optional properties with sane values and
> ignore the return code).
>
> 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Other than the nit on the commit message, this simple fix seems sane.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v3] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-24 23:56 ` Doug Anderson
0 siblings, 0 replies; 58+ messages in thread
From: Doug Anderson @ 2018-07-24 23:56 UTC (permalink / raw)
To: Daniel Thompson
Cc: Lee Jones, Jingoo Han, Thierry Reding, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, linux-pwm, dri-devel, linux-fbdev, LKML,
Patch Tracking
Hi,
On Tue, Jul 24, 2018 at 12:12 AM, Daniel Thompson
<daniel.thompson@linaro.org> wrote:
> Currently, if the DT does not define num-interpolated-steps then
> num_steps is undefined and the interpolation code will deploy randomly.
> Fix with a simple initialize to zero.
>
> Fixes: 573fe6d1c25c ("backlight: pwm_bl: Linear interpolation between
> brightness-levels")
I think it may be incorrect to word-wrap the Fixes line. A quick grep
through the source code looks like others agree.
> 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:
> v3:
> - Switch to the simplest fix and zero the uninitialized variable. git
> grep indicates that ~25% of calls to of_property_read_u32() use this
> pattern (pre-initialize optional properties with sane values and
> ignore the return code).
>
> 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Other than the nit on the commit message, this simple fix seems sane.
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply [flat|nested] 58+ messages in thread
* Re: [PATCH v3] backlight: pwm_bl: Fix uninitialized variable
2018-07-24 7:12 ` Daniel Thompson
(?)
@ 2018-07-25 5:22 ` Lee Jones
-1 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-25 5:22 UTC (permalink / raw)
To: Daniel Thompson
Cc: linux-pwm, linux-fbdev, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, Jingoo Han, patches, linux-kernel, dri-devel,
Thierry Reding
On Tue, 24 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 with a simple initialize to zero.
>
> 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:
> v3:
> - Switch to the simplest fix and zero the uninitialized variable. git
> grep indicates that ~25% of calls to of_property_read_u32() use this
> pattern (pre-initialize optional properties with sane values and
> ignore the return code).
>
> 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Once the 'Fixes:' line has been repaired:
Acked-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v3] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-25 5:22 ` Lee Jones
0 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-25 5:22 UTC (permalink / raw)
To: Daniel Thompson
Cc: Jingoo Han, Thierry Reding, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, linux-pwm, dri-devel, linux-fbdev, linux-kernel,
patches
On Tue, 24 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 with a simple initialize to zero.
>
> 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:
> v3:
> - Switch to the simplest fix and zero the uninitialized variable. git
> grep indicates that ~25% of calls to of_property_read_u32() use this
> pattern (pre-initialize optional properties with sane values and
> ignore the return code).
>
> 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Once the 'Fixes:' line has been repaired:
Acked-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v3] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-25 5:22 ` Lee Jones
0 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-25 5:22 UTC (permalink / raw)
To: Daniel Thompson
Cc: linux-pwm, linux-fbdev, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, Jingoo Han, patches, linux-kernel, dri-devel,
Thierry Reding
On Tue, 24 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 with a simple initialize to zero.
>
> 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:
> v3:
> - Switch to the simplest fix and zero the uninitialized variable. git
> grep indicates that ~25% of calls to of_property_read_u32() use this
> pattern (pre-initialize optional properties with sane values and
> ignore the return code).
>
> 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Once the 'Fixes:' line has been repaired:
Acked-by: Lee Jones <lee.jones@linaro.org>
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 58+ messages in thread
* [PATCH v4] backlight: pwm_bl: Fix uninitialized variable
2018-07-16 21:02 ` Daniel Thompson
(?)
@ 2018-07-25 7:38 ` Daniel Thompson
-1 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-25 7:38 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han
Cc: linux-pwm, linux-fbdev, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, patches, linux-kernel, dri-devel, Thierry Reding
Currently, if the DT does not define num-interpolated-steps then
num_steps is undefined and the interpolation code will deploy randomly.
Fix with a simple initialize to zero.
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>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
Notes:
v4:
- Remove line break from Fixes: and update the *-by:s
v3:
- Switch to the simplest fix and zero the uninitialized variable. git
grep indicates that ~25% of calls to of_property_read_u32() use this
pattern (pre-initialize optional properties with sane values and
ignore the return code).
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 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 9ee4c1b735b2..bdfcc0a71db1 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
struct device_node *node = dev->of_node;
unsigned int num_levels = 0;
unsigned int levels_count;
- unsigned int num_steps;
+ unsigned int num_steps = 0;
struct property *prop;
unsigned int *table;
int length;
--
2.17.1
^ permalink raw reply related [flat|nested] 58+ messages in thread* [PATCH v4] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-25 7:38 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-25 7:38 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han
Cc: Thierry Reding, Bartlomiej Zolnierkiewicz, Marcel Ziswiler,
linux-pwm, dri-devel, linux-fbdev, linux-kernel, patches
Currently, if the DT does not define num-interpolated-steps then
num_steps is undefined and the interpolation code will deploy randomly.
Fix with a simple initialize to zero.
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>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
Notes:
v4:
- Remove line break from Fixes: and update the *-by:s
v3:
- Switch to the simplest fix and zero the uninitialized variable. git
grep indicates that ~25% of calls to of_property_read_u32() use this
pattern (pre-initialize optional properties with sane values and
ignore the return code).
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 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 9ee4c1b735b2..bdfcc0a71db1 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
struct device_node *node = dev->of_node;
unsigned int num_levels = 0;
unsigned int levels_count;
- unsigned int num_steps;
+ unsigned int num_steps = 0;
struct property *prop;
unsigned int *table;
int length;
--
2.17.1
^ permalink raw reply related [flat|nested] 58+ messages in thread* [PATCH v4] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-25 7:38 ` Daniel Thompson
0 siblings, 0 replies; 58+ messages in thread
From: Daniel Thompson @ 2018-07-25 7:38 UTC (permalink / raw)
To: Lee Jones, Daniel Thompson, Jingoo Han
Cc: linux-pwm, linux-fbdev, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, patches, linux-kernel, dri-devel, Thierry Reding
Currently, if the DT does not define num-interpolated-steps then
num_steps is undefined and the interpolation code will deploy randomly.
Fix with a simple initialize to zero.
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>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Acked-by: Lee Jones <lee.jones@linaro.org>
---
Notes:
v4:
- Remove line break from Fixes: and update the *-by:s
v3:
- Switch to the simplest fix and zero the uninitialized variable. git
grep indicates that ~25% of calls to of_property_read_u32() use this
pattern (pre-initialize optional properties with sane values and
ignore the return code).
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 | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 9ee4c1b735b2..bdfcc0a71db1 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -250,7 +250,7 @@ static int pwm_backlight_parse_dt(struct device *dev,
struct device_node *node = dev->of_node;
unsigned int num_levels = 0;
unsigned int levels_count;
- unsigned int num_steps;
+ unsigned int num_steps = 0;
struct property *prop;
unsigned int *table;
int length;
--
2.17.1
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 58+ messages in thread* Re: [PATCH v4] backlight: pwm_bl: Fix uninitialized variable
2018-07-25 7:38 ` Daniel Thompson
@ 2018-07-25 8:03 ` Lee Jones
-1 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-25 8:03 UTC (permalink / raw)
To: Daniel Thompson
Cc: Jingoo Han, Thierry Reding, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, linux-pwm, dri-devel, linux-fbdev, linux-kernel,
patches
On Wed, 25 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 with a simple initialize to zero.
>
> 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>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> ---
>
> Notes:
> v4:
> - Remove line break from Fixes: and update the *-by:s
>
> v3:
> - Switch to the simplest fix and zero the uninitialized variable. git
> grep indicates that ~25% of calls to of_property_read_u32() use this
> pattern (pre-initialize optional properties with sane values and
> ignore the return code).
>
> 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied, thanks.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 58+ messages in thread* Re: [PATCH v4] backlight: pwm_bl: Fix uninitialized variable
@ 2018-07-25 8:03 ` Lee Jones
0 siblings, 0 replies; 58+ messages in thread
From: Lee Jones @ 2018-07-25 8:03 UTC (permalink / raw)
To: Daniel Thompson
Cc: Jingoo Han, Thierry Reding, Bartlomiej Zolnierkiewicz,
Marcel Ziswiler, linux-pwm, dri-devel, linux-fbdev, linux-kernel,
patches
On Wed, 25 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 with a simple initialize to zero.
>
> 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>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Acked-by: Lee Jones <lee.jones@linaro.org>
> ---
>
> Notes:
> v4:
> - Remove line break from Fixes: and update the *-by:s
>
> v3:
> - Switch to the simplest fix and zero the uninitialized variable. git
> grep indicates that ~25% of calls to of_property_read_u32() use this
> pattern (pre-initialize optional properties with sane values and
> ignore the return code).
>
> 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 | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Applied, thanks.
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply [flat|nested] 58+ messages in thread