* [PATCH] backlight: lm3639: Unconditionally call led_classdev_unregister @ 2018-09-21 20:21 Nathan Chancellor 2018-09-21 22:48 ` Nick Desaulniers 2018-10-09 9:10 ` Lee Jones 0 siblings, 2 replies; 6+ messages in thread From: Nathan Chancellor @ 2018-09-21 20:21 UTC (permalink / raw) To: Lee Jones, Daniel Thompson, Jingoo Han, Bartlomiej Zolnierkiewicz Cc: dri-devel, linux-fbdev, linux-kernel, Nick Desaulniers, Nathan Chancellor Clang warns that the address of a pointer will always evaluated as true in a boolean context. drivers/video/backlight/lm3639_bl.c:403:14: warning: address of 'pchip->cdev_torch' will always evaluate to 'true' [-Wpointer-bool-conversion] if (&pchip->cdev_torch) ~~ ~~~~~~~^~~~~~~~~~ drivers/video/backlight/lm3639_bl.c:405:14: warning: address of 'pchip->cdev_flash' will always evaluate to 'true' [-Wpointer-bool-conversion] if (&pchip->cdev_flash) ~~ ~~~~~~~^~~~~~~~~~ 2 warnings generated. These statements have been present since 2012, introduced by commit 0f59858d5119 ("backlight: add new lm3639 backlight driver"). Given that they have been called unconditionally since then presumably without any issues, removing the always true if statements to fix the warnings without any real world changes. Link: https://github.com/ClangBuiltLinux/linux/issues/119 Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> --- Alternatively, it's possible the address wasn't supposed to be taken or the dev in these structs should be checked instead. I don't have this hardware to make that call so I would appreciate some review and opinions on what was intended here. Thanks! drivers/video/backlight/lm3639_bl.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/drivers/video/backlight/lm3639_bl.c b/drivers/video/backlight/lm3639_bl.c index cd50df5807ea..086611c7bc03 100644 --- a/drivers/video/backlight/lm3639_bl.c +++ b/drivers/video/backlight/lm3639_bl.c @@ -400,10 +400,8 @@ static int lm3639_remove(struct i2c_client *client) regmap_write(pchip->regmap, REG_ENABLE, 0x00); - if (&pchip->cdev_torch) - led_classdev_unregister(&pchip->cdev_torch); - if (&pchip->cdev_flash) - led_classdev_unregister(&pchip->cdev_flash); + led_classdev_unregister(&pchip->cdev_torch); + led_classdev_unregister(&pchip->cdev_flash); if (pchip->bled) device_remove_file(&(pchip->bled->dev), &dev_attr_bled_mode); return 0; -- 2.19.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] backlight: lm3639: Unconditionally call led_classdev_unregister 2018-09-21 20:21 [PATCH] backlight: lm3639: Unconditionally call led_classdev_unregister Nathan Chancellor @ 2018-09-21 22:48 ` Nick Desaulniers 2018-09-21 23:10 ` Nathan Chancellor 2018-10-09 9:10 ` Lee Jones 1 sibling, 1 reply; 6+ messages in thread From: Nick Desaulniers @ 2018-09-21 22:48 UTC (permalink / raw) To: Nathan Chancellor Cc: lee.jones, daniel.thompson, jingoohan1, b.zolnierkie, dri-devel, linux-fbdev, LKML On Fri, Sep 21, 2018 at 1:23 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > Clang warns that the address of a pointer will always evaluated as true > in a boolean context. > > drivers/video/backlight/lm3639_bl.c:403:14: warning: address of > 'pchip->cdev_torch' will always evaluate to 'true' > [-Wpointer-bool-conversion] > if (&pchip->cdev_torch) > ~~ ~~~~~~~^~~~~~~~~~ > drivers/video/backlight/lm3639_bl.c:405:14: warning: address of > 'pchip->cdev_flash' will always evaluate to 'true' > [-Wpointer-bool-conversion] > if (&pchip->cdev_flash) > ~~ ~~~~~~~^~~~~~~~~~ > 2 warnings generated. > > These statements have been present since 2012, introduced by > commit 0f59858d5119 ("backlight: add new lm3639 backlight > driver"). Given that they have been called unconditionally since > then presumably without any issues, removing the always true if > statements to fix the warnings without any real world changes. > > Link: https://github.com/ClangBuiltLinux/linux/issues/119 > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > > Alternatively, it's possible the address wasn't supposed to be taken or > the dev in these structs should be checked instead. I don't have this > hardware to make that call so I would appreciate some review and > opinions on what was intended here. > > Thanks! > > drivers/video/backlight/lm3639_bl.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/video/backlight/lm3639_bl.c b/drivers/video/backlight/lm3639_bl.c > index cd50df5807ea..086611c7bc03 100644 > --- a/drivers/video/backlight/lm3639_bl.c > +++ b/drivers/video/backlight/lm3639_bl.c > @@ -400,10 +400,8 @@ static int lm3639_remove(struct i2c_client *client) > > regmap_write(pchip->regmap, REG_ENABLE, 0x00); > > - if (&pchip->cdev_torch) > - led_classdev_unregister(&pchip->cdev_torch); > - if (&pchip->cdev_flash) > - led_classdev_unregister(&pchip->cdev_flash); > + led_classdev_unregister(&pchip->cdev_torch); > + led_classdev_unregister(&pchip->cdev_flash); led_classdev_unregister() requires that its arg is non-null (as it dereferences it without any kind of check). It's not clear that i2c_get_clientdata() can never return a null pointer, so I think all references to pchip in this function should instead be guarded with a null check. Would you mind making that change and sending a v2? > if (pchip->bled) > device_remove_file(&(pchip->bled->dev), &dev_attr_bled_mode); > return 0; > -- > 2.19.0 > -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] backlight: lm3639: Unconditionally call led_classdev_unregister 2018-09-21 22:48 ` Nick Desaulniers @ 2018-09-21 23:10 ` Nathan Chancellor 2018-09-24 21:41 ` Nick Desaulniers 0 siblings, 1 reply; 6+ messages in thread From: Nathan Chancellor @ 2018-09-21 23:10 UTC (permalink / raw) To: Nick Desaulniers Cc: lee.jones, daniel.thompson, jingoohan1, b.zolnierkie, dri-devel, linux-fbdev, LKML On Fri, Sep 21, 2018 at 03:48:50PM -0700, Nick Desaulniers wrote: > On Fri, Sep 21, 2018 at 1:23 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > Clang warns that the address of a pointer will always evaluated as true > > in a boolean context. > > > > drivers/video/backlight/lm3639_bl.c:403:14: warning: address of > > 'pchip->cdev_torch' will always evaluate to 'true' > > [-Wpointer-bool-conversion] > > if (&pchip->cdev_torch) > > ~~ ~~~~~~~^~~~~~~~~~ > > drivers/video/backlight/lm3639_bl.c:405:14: warning: address of > > 'pchip->cdev_flash' will always evaluate to 'true' > > [-Wpointer-bool-conversion] > > if (&pchip->cdev_flash) > > ~~ ~~~~~~~^~~~~~~~~~ > > 2 warnings generated. > > > > These statements have been present since 2012, introduced by > > commit 0f59858d5119 ("backlight: add new lm3639 backlight > > driver"). Given that they have been called unconditionally since > > then presumably without any issues, removing the always true if > > statements to fix the warnings without any real world changes. > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/119 > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > --- > > > > Alternatively, it's possible the address wasn't supposed to be taken or > > the dev in these structs should be checked instead. I don't have this > > hardware to make that call so I would appreciate some review and > > opinions on what was intended here. > > > > Thanks! > > > > drivers/video/backlight/lm3639_bl.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/video/backlight/lm3639_bl.c b/drivers/video/backlight/lm3639_bl.c > > index cd50df5807ea..086611c7bc03 100644 > > --- a/drivers/video/backlight/lm3639_bl.c > > +++ b/drivers/video/backlight/lm3639_bl.c > > @@ -400,10 +400,8 @@ static int lm3639_remove(struct i2c_client *client) > > > > regmap_write(pchip->regmap, REG_ENABLE, 0x00); > > > > - if (&pchip->cdev_torch) > > - led_classdev_unregister(&pchip->cdev_torch); > > - if (&pchip->cdev_flash) > > - led_classdev_unregister(&pchip->cdev_flash); > > + led_classdev_unregister(&pchip->cdev_torch); > > + led_classdev_unregister(&pchip->cdev_flash); > > led_classdev_unregister() requires that its arg is non-null (as it > dereferences it without any kind of check). It's not clear that > i2c_get_clientdata() can never return a null pointer, so I think all > references to pchip in this function should instead be guarded with a > null check. Would you mind making that change and sending a v2? > Hi Nick, I did a quick grep throughout the tree and I didn't see any place where there were null checks for i2c_get_clientdata, leading me to believe that such a check isn't necessary although I am nowhere close to an expert into this stuff. I'm not sure I follow the rest of the request though, where should the check be? Before regmap_write? Furthermore, the probe function seems to make sure all of these get initialized properly, doesn't remove imply that probe was successful? Thank you for the comment and review! Nathan > > if (pchip->bled) > > device_remove_file(&(pchip->bled->dev), &dev_attr_bled_mode); > > return 0; > > -- > > 2.19.0 > > > > > -- > Thanks, > ~Nick Desaulniers ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] backlight: lm3639: Unconditionally call led_classdev_unregister 2018-09-21 23:10 ` Nathan Chancellor @ 2018-09-24 21:41 ` Nick Desaulniers 2018-09-26 8:59 ` Daniel Thompson 0 siblings, 1 reply; 6+ messages in thread From: Nick Desaulniers @ 2018-09-24 21:41 UTC (permalink / raw) To: Nathan Chancellor Cc: lee.jones, daniel.thompson, jingoohan1, b.zolnierkie, dri-devel, linux-fbdev, LKML On Fri, Sep 21, 2018 at 4:10 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > On Fri, Sep 21, 2018 at 03:48:50PM -0700, Nick Desaulniers wrote: > > On Fri, Sep 21, 2018 at 1:23 PM Nathan Chancellor > > <natechancellor@gmail.com> wrote: > > > > > > Clang warns that the address of a pointer will always evaluated as true > > > in a boolean context. > > > > > > drivers/video/backlight/lm3639_bl.c:403:14: warning: address of > > > 'pchip->cdev_torch' will always evaluate to 'true' > > > [-Wpointer-bool-conversion] > > > if (&pchip->cdev_torch) > > > ~~ ~~~~~~~^~~~~~~~~~ > > > drivers/video/backlight/lm3639_bl.c:405:14: warning: address of > > > 'pchip->cdev_flash' will always evaluate to 'true' > > > [-Wpointer-bool-conversion] > > > if (&pchip->cdev_flash) > > > ~~ ~~~~~~~^~~~~~~~~~ > > > 2 warnings generated. > > > > > > These statements have been present since 2012, introduced by > > > commit 0f59858d5119 ("backlight: add new lm3639 backlight > > > driver"). Given that they have been called unconditionally since > > > then presumably without any issues, removing the always true if > > > statements to fix the warnings without any real world changes. > > > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/119 > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > > --- > > > > > > Alternatively, it's possible the address wasn't supposed to be taken or > > > the dev in these structs should be checked instead. I don't have this > > > hardware to make that call so I would appreciate some review and > > > opinions on what was intended here. > > > > > > Thanks! > > > > > > drivers/video/backlight/lm3639_bl.c | 6 ++---- > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/video/backlight/lm3639_bl.c b/drivers/video/backlight/lm3639_bl.c > > > index cd50df5807ea..086611c7bc03 100644 > > > --- a/drivers/video/backlight/lm3639_bl.c > > > +++ b/drivers/video/backlight/lm3639_bl.c > > > @@ -400,10 +400,8 @@ static int lm3639_remove(struct i2c_client *client) > > > > > > regmap_write(pchip->regmap, REG_ENABLE, 0x00); > > > > > > - if (&pchip->cdev_torch) > > > - led_classdev_unregister(&pchip->cdev_torch); > > > - if (&pchip->cdev_flash) > > > - led_classdev_unregister(&pchip->cdev_flash); > > > + led_classdev_unregister(&pchip->cdev_torch); > > > + led_classdev_unregister(&pchip->cdev_flash); > > > > led_classdev_unregister() requires that its arg is non-null (as it > > dereferences it without any kind of check). It's not clear that > > i2c_get_clientdata() can never return a null pointer, so I think all > > references to pchip in this function should instead be guarded with a > > null check. Would you mind making that change and sending a v2? > > > > Hi Nick, > > I did a quick grep throughout the tree and I didn't see any place where > there were null checks for i2c_get_clientdata, leading me to believe > that such a check isn't necessary although I am nowhere close to an expert > into this stuff. This seems to be the case. We should start using __attribute__((returns_nonnull)) (gated on gcc 5+). I *think* that the device's driver_data is actually set in drivers/video/backlight/backlight.c. Looks like CONFIG_BACKLIGHT_LM3639 depends on CONFIG_BACKLIGHT_CLASS_DEVICE so I feel more confident in your patch. I would still prefer the maintainers to review though. > I'm not sure I follow the rest of the request though, > where should the check be? Before regmap_write? > > Furthermore, the probe function seems to make sure all of these get > initialized properly, doesn't remove imply that probe was successful? > > Thank you for the comment and review! > Nathan > > > > if (pchip->bled) > > > device_remove_file(&(pchip->bled->dev), &dev_attr_bled_mode); > > > return 0; > > > -- > > > 2.19.0 > > > > > > > > > -- > > Thanks, > > ~Nick Desaulniers -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] backlight: lm3639: Unconditionally call led_classdev_unregister 2018-09-24 21:41 ` Nick Desaulniers @ 2018-09-26 8:59 ` Daniel Thompson 0 siblings, 0 replies; 6+ messages in thread From: Daniel Thompson @ 2018-09-26 8:59 UTC (permalink / raw) To: Nick Desaulniers Cc: Nathan Chancellor, lee.jones, jingoohan1, b.zolnierkie, dri-devel, linux-fbdev, LKML On Mon, Sep 24, 2018 at 02:41:04PM -0700, Nick Desaulniers wrote: > On Fri, Sep 21, 2018 at 4:10 PM Nathan Chancellor > <natechancellor@gmail.com> wrote: > > > > On Fri, Sep 21, 2018 at 03:48:50PM -0700, Nick Desaulniers wrote: > > > On Fri, Sep 21, 2018 at 1:23 PM Nathan Chancellor > > > <natechancellor@gmail.com> wrote: > > > > > > > > Clang warns that the address of a pointer will always evaluated as true > > > > in a boolean context. > > > > > > > > drivers/video/backlight/lm3639_bl.c:403:14: warning: address of > > > > 'pchip->cdev_torch' will always evaluate to 'true' > > > > [-Wpointer-bool-conversion] > > > > if (&pchip->cdev_torch) > > > > ~~ ~~~~~~~^~~~~~~~~~ > > > > drivers/video/backlight/lm3639_bl.c:405:14: warning: address of > > > > 'pchip->cdev_flash' will always evaluate to 'true' > > > > [-Wpointer-bool-conversion] > > > > if (&pchip->cdev_flash) > > > > ~~ ~~~~~~~^~~~~~~~~~ > > > > 2 warnings generated. > > > > > > > > These statements have been present since 2012, introduced by > > > > commit 0f59858d5119 ("backlight: add new lm3639 backlight > > > > driver"). Given that they have been called unconditionally since > > > > then presumably without any issues, removing the always true if > > > > statements to fix the warnings without any real world changes. > > > > > > > > Link: https://github.com/ClangBuiltLinux/linux/issues/119 > > > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> Based on conversation below... Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org> > > > > --- > > > > > > > > Alternatively, it's possible the address wasn't supposed to be taken or > > > > the dev in these structs should be checked instead. I don't have this > > > > hardware to make that call so I would appreciate some review and > > > > opinions on what was intended here. > > > > > > > > Thanks! > > > > > > > > drivers/video/backlight/lm3639_bl.c | 6 ++---- > > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/video/backlight/lm3639_bl.c b/drivers/video/backlight/lm3639_bl.c > > > > index cd50df5807ea..086611c7bc03 100644 > > > > --- a/drivers/video/backlight/lm3639_bl.c > > > > +++ b/drivers/video/backlight/lm3639_bl.c > > > > @@ -400,10 +400,8 @@ static int lm3639_remove(struct i2c_client *client) > > > > > > > > regmap_write(pchip->regmap, REG_ENABLE, 0x00); > > > > > > > > - if (&pchip->cdev_torch) > > > > - led_classdev_unregister(&pchip->cdev_torch); > > > > - if (&pchip->cdev_flash) > > > > - led_classdev_unregister(&pchip->cdev_flash); > > > > + led_classdev_unregister(&pchip->cdev_torch); > > > > + led_classdev_unregister(&pchip->cdev_flash); > > > > > > led_classdev_unregister() requires that its arg is non-null (as it > > > dereferences it without any kind of check). It's not clear that > > > i2c_get_clientdata() can never return a null pointer, so I think all > > > references to pchip in this function should instead be guarded with a > > > null check. Would you mind making that change and sending a v2? > > > > > > > Hi Nick, > > > > I did a quick grep throughout the tree and I didn't see any place where > > there were null checks for i2c_get_clientdata, leading me to believe > > that such a check isn't necessary although I am nowhere close to an expert > > into this stuff. > > This seems to be the case. We should start using > __attribute__((returns_nonnull)) (gated on gcc 5+). > I *think* that the device's driver_data is actually set in > drivers/video/backlight/backlight.c. Looks like > CONFIG_BACKLIGHT_LM3639 depends on CONFIG_BACKLIGHT_CLASS_DEVICE so I > feel more confident in your patch. > I would still prefer the maintainers to review though. AFAICT it is impossible for the probe function to complete successfully without having called i2c_set_clientdata() and therefore it is impossible for pchip to be NULL in the remove function. Not sure it is possible to use return_nonnull though. It is not that i2c_get_clientdata() can *never* return non-NULL, it is that in *this* case (which is actually a fairly common one) it can never return non-NULL. Daniel. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] backlight: lm3639: Unconditionally call led_classdev_unregister 2018-09-21 20:21 [PATCH] backlight: lm3639: Unconditionally call led_classdev_unregister Nathan Chancellor 2018-09-21 22:48 ` Nick Desaulniers @ 2018-10-09 9:10 ` Lee Jones 1 sibling, 0 replies; 6+ messages in thread From: Lee Jones @ 2018-10-09 9:10 UTC (permalink / raw) To: Nathan Chancellor Cc: Daniel Thompson, Jingoo Han, Bartlomiej Zolnierkiewicz, dri-devel, linux-fbdev, linux-kernel, Nick Desaulniers On Fri, 21 Sep 2018, Nathan Chancellor wrote: > Clang warns that the address of a pointer will always evaluated as true > in a boolean context. > > drivers/video/backlight/lm3639_bl.c:403:14: warning: address of > 'pchip->cdev_torch' will always evaluate to 'true' > [-Wpointer-bool-conversion] > if (&pchip->cdev_torch) > ~~ ~~~~~~~^~~~~~~~~~ > drivers/video/backlight/lm3639_bl.c:405:14: warning: address of > 'pchip->cdev_flash' will always evaluate to 'true' > [-Wpointer-bool-conversion] > if (&pchip->cdev_flash) > ~~ ~~~~~~~^~~~~~~~~~ > 2 warnings generated. > > These statements have been present since 2012, introduced by > commit 0f59858d5119 ("backlight: add new lm3639 backlight > driver"). Given that they have been called unconditionally since > then presumably without any issues, removing the always true if > statements to fix the warnings without any real world changes. > > Link: https://github.com/ClangBuiltLinux/linux/issues/119 > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > > Alternatively, it's possible the address wasn't supposed to be taken or > the dev in these structs should be checked instead. I don't have this > hardware to make that call so I would appreciate some review and > opinions on what was intended here. > > Thanks! > > drivers/video/backlight/lm3639_bl.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) 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] 6+ messages in thread
end of thread, other threads:[~2018-10-09 9:10 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-21 20:21 [PATCH] backlight: lm3639: Unconditionally call led_classdev_unregister Nathan Chancellor 2018-09-21 22:48 ` Nick Desaulniers 2018-09-21 23:10 ` Nathan Chancellor 2018-09-24 21:41 ` Nick Desaulniers 2018-09-26 8:59 ` Daniel Thompson 2018-10-09 9:10 ` Lee Jones
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).