[bug report] leds: Add support for Philips PCA955x I2C LED drivers

[bug report] leds: Add support for Philips PCA955x I2C LED drivers

[Dan Carpenter]

Hello Nate Case,

The patch f46e9203d9a1: "leds: Add support for Philips PCA955x I2C
LED drivers" from Jul 16, 2008, leads to the following static checker
warning:

	drivers/leds/leds-pca955x.c:476 pca955x_probe()
	warn: this array is probably non-NULL. 'pdata->leds + i->name'

drivers/leds/leds-pca955x.c
   465                  switch (pca955x_led->type) {
   466                  case PCA955X_TYPE_NONE:
   467                          break;
   468                  case PCA955X_TYPE_GPIO:
   469                          ngpios++;
   470                          break;
   471                  case PCA955X_TYPE_LED:
   472                          /*
   473                           * Platform data can specify LED names and
   474                           * default triggers
   475                           */
   476                          if (pdata->leds[i].name)
                                    ^^^^^^^^^^^^^^^^^^^
The comment implies that we should be testing pdata->leds[i].name[0] to
see if any string has been set?

   477                                  snprintf(pca955x_led->name,
   478                                          sizeof(pca955x_led->name), "pca955x:%s",
   479                                          pdata->leds[i].name);
   480                          if (pdata->leds[i].default_trigger)
   481                                  pca955x_led->led_cdev.default_trigger =
   482                                          pdata->leds[i].default_trigger;
   483  
   484                          pca955x_led->led_cdev.name = pca955x_led->name;
   485                          pca955x_led->led_cdev.brightness_set_blocking =
   486                                  pca955x_led_set;
   487  
   488                          err = devm_led_classdev_register(&client->dev,
   489                                                          &pca955x_led->led_cdev);
   490                          if (err)
   491                                  return err;
   492  
   493                          /* Turn off LED */
   494                          pca955x_led_set(&pca955x_led->led_cdev, LED_OFF);

regards,
dan carpenter

Re: [bug report] leds: Add support for Philips PCA955x I2C LED drivers

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]

Hi!

> Hello Nate Case,
> 
> The patch f46e9203d9a1: "leds: Add support for Philips PCA955x I2C
> LED drivers" from Jul 16, 2008, leads to the following static checker
> warning:
> 
> 	drivers/leds/leds-pca955x.c:476 pca955x_probe()
> 	warn: this array is probably non-NULL. 'pdata->leds + i->name'
> 
> drivers/leds/leds-pca955x.c
>    465                  switch (pca955x_led->type) {
>    466                  case PCA955X_TYPE_NONE:
>    467                          break;
>    468                  case PCA955X_TYPE_GPIO:
>    469                          ngpios++;
>    470                          break;
>    471                  case PCA955X_TYPE_LED:
>    472                          /*
>    473                           * Platform data can specify LED names and
>    474                           * default triggers
>    475                           */
>    476                          if (pdata->leds[i].name)
>                                     ^^^^^^^^^^^^^^^^^^^
> The comment implies that we should be testing pdata->leds[i].name[0] to
> see if any string has been set?

Someone was already submitting patch from this one, no?

And please don't mark this one as a "bug report". There's no bug. Your
code analysis tool found a way to make kernel code shorter... well, so
what?


									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [bug report] leds: Add support for Philips PCA955x I2C LED drivers

[Dan Carpenter]

On Thu, Aug 17, 2017 at 01:27:38PM +0200, Pavel Machek wrote:
> Hi!
> 
> > Hello Nate Case,
> > 
> > The patch f46e9203d9a1: "leds: Add support for Philips PCA955x I2C
> > LED drivers" from Jul 16, 2008, leads to the following static checker
> > warning:
> > 
> > 	drivers/leds/leds-pca955x.c:476 pca955x_probe()
> > 	warn: this array is probably non-NULL. 'pdata->leds + i->name'
> > 
> > drivers/leds/leds-pca955x.c
> >    465                  switch (pca955x_led->type) {
> >    466                  case PCA955X_TYPE_NONE:
> >    467                          break;
> >    468                  case PCA955X_TYPE_GPIO:
> >    469                          ngpios++;
> >    470                          break;
> >    471                  case PCA955X_TYPE_LED:
> >    472                          /*
> >    473                           * Platform data can specify LED names and
> >    474                           * default triggers
> >    475                           */
> >    476                          if (pdata->leds[i].name)
> >                                     ^^^^^^^^^^^^^^^^^^^
> > The comment implies that we should be testing pdata->leds[i].name[0] to
> > see if any string has been set?
> 
> Someone was already submitting patch from this one, no?
> 
> And please don't mark this one as a "bug report". There's no bug. Your
> code analysis tool found a way to make kernel code shorter... well, so
> what?
> 

You didn't read my email.  It was only one sentence long...  :/

>From the comments, it looked like maybe a different test was intended.
Anyway, that's the point of these warnings is because many times the
bogus NULL test should be replaced with a correct test.  I don't forward
the warning if it's obvious that the test should just be deleted.

I probably could review the warnings even more but I am pretty busy and
the code is obviously bogus and it's easier for the author.

regards,
dan carpenter

Re: [bug report] leds: Add support for Philips PCA955x I2C LED drivers

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 1890 bytes --]

On Thu 2017-08-17 14:51:48, Dan Carpenter wrote:
> On Thu, Aug 17, 2017 at 01:27:38PM +0200, Pavel Machek wrote:
> > Hi!
> > 
> > > Hello Nate Case,
> > > 
> > > The patch f46e9203d9a1: "leds: Add support for Philips PCA955x I2C
> > > LED drivers" from Jul 16, 2008, leads to the following static checker
> > > warning:

> > >    472                          /*
> > >    473                           * Platform data can specify LED names and
> > >    474                           * default triggers
> > >    475                           */
> > >    476                          if (pdata->leds[i].name)
> > >                                     ^^^^^^^^^^^^^^^^^^^
> > > The comment implies that we should be testing pdata->leds[i].name[0] to
> > > see if any string has been set?
> > 
> > Someone was already submitting patch from this one, no?
> > 
> > And please don't mark this one as a "bug report". There's no bug. Your
> > code analysis tool found a way to make kernel code shorter... well, so
> > what?
> > 
> 
> You didn't read my email.  It was only one sentence long...  :/

I read your subject, and it claimed bug.

Bug normally means: Hey, there's problem with the driver. It oopses my
kernel here.

> >From the comments, it looked like maybe a different test was intended.
> Anyway, that's the point of these warnings is because many times the
> bogus NULL test should be replaced with a correct test.  I don't forward
> the warning if it's obvious that the test should just be deleted.
> 
> I probably could review the warnings even more but I am pretty busy and
> the code is obviously bogus and it's easier for the author.

Anyway, there was patch on the lists, already...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [bug report] leds: Add support for Philips PCA955x I2C LED drivers

[Dan Carpenter]

On Thu, Aug 17, 2017 at 03:27:40PM +0200, Pavel Machek wrote:
> On Thu 2017-08-17 14:51:48, Dan Carpenter wrote:
> > On Thu, Aug 17, 2017 at 01:27:38PM +0200, Pavel Machek wrote:
> > > Hi!
> > > 
> > > > Hello Nate Case,
> > > > 
> > > > The patch f46e9203d9a1: "leds: Add support for Philips PCA955x I2C
> > > > LED drivers" from Jul 16, 2008, leads to the following static checker
> > > > warning:
> 
> > > >    472                          /*
> > > >    473                           * Platform data can specify LED names and
> > > >    474                           * default triggers
> > > >    475                           */
> > > >    476                          if (pdata->leds[i].name)
> > > >                                     ^^^^^^^^^^^^^^^^^^^
> > > > The comment implies that we should be testing pdata->leds[i].name[0] to
> > > > see if any string has been set?
> > > 
> > > Someone was already submitting patch from this one, no?
> > > 
> > > And please don't mark this one as a "bug report". There's no bug. Your
> > > code analysis tool found a way to make kernel code shorter... well, so
> > > what?
> > > 
> > 
> > You didn't read my email.  It was only one sentence long...  :/
> 
> I read your subject, and it claimed bug.
> 
> Bug normally means: Hey, there's problem with the driver. It oopses my
> kernel here.
> 

I thought it probably was a bug.  One hour ago Colin "fixed" one of
these by removing the NULL check and it turned out that it was a real
bug.  It's legit to suspect that these are often bugs.

Just set up a procmail filter to change the subject to whatever you
prefer.  I can't find it in me to care at all.

regards,
dan carpenter

Re: [bug report] leds: Add support for Philips PCA955x I2C LED drivers

[Dan Carpenter]

On Thu, Aug 17, 2017 at 04:37:22PM +0300, Dan Carpenter wrote:
> On Thu, Aug 17, 2017 at 03:27:40PM +0200, Pavel Machek wrote:
> > On Thu 2017-08-17 14:51:48, Dan Carpenter wrote:
> > > On Thu, Aug 17, 2017 at 01:27:38PM +0200, Pavel Machek wrote:
> > > > Hi!
> > > > 
> > > > > Hello Nate Case,
> > > > > 
> > > > > The patch f46e9203d9a1: "leds: Add support for Philips PCA955x I2C
> > > > > LED drivers" from Jul 16, 2008, leads to the following static checker
> > > > > warning:
> > 
> > > > >    472                          /*
> > > > >    473                           * Platform data can specify LED names and
> > > > >    474                           * default triggers
> > > > >    475                           */
> > > > >    476                          if (pdata->leds[i].name)
> > > > >                                     ^^^^^^^^^^^^^^^^^^^
> > > > > The comment implies that we should be testing pdata->leds[i].name[0] to
> > > > > see if any string has been set?
> > > > 
> > > > Someone was already submitting patch from this one, no?
> > > > 
> > > > And please don't mark this one as a "bug report". There's no bug. Your
> > > > code analysis tool found a way to make kernel code shorter... well, so
> > > > what?
> > > > 
> > > 
> > > You didn't read my email.  It was only one sentence long...  :/
> > 
> > I read your subject, and it claimed bug.
> > 
> > Bug normally means: Hey, there's problem with the driver. It oopses my
> > kernel here.
> > 
> 
> I thought it probably was a bug.  One hour ago Colin "fixed" one of
> these by removing the NULL check and it turned out that it was a real
> bug.  It's legit to suspect that these are often bugs.
> 

It's especially legit to suspect that the check might be important if
there is a comment explaining why the check is there.  If you don't want
people to question your code stop writing nonsense code in the first
place.  Gar...

regards,
dan carpenter

Re: [bug report] leds: Add support for Philips PCA955x I2C LED drivers

[Dan Carpenter]

Never mind, Pavel didn't write this code.  Sorry, Nate.  This code is
nice enough.  Pavel is just making me annoyed with his constant waste of
time responses to what are essentially automated emails.

regards,
dan carpenter

Re: [bug report] leds: Add support for Philips PCA955x I2C LED drivers

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 848 bytes --]

On Thu 2017-08-17 16:49:37, Dan Carpenter wrote:
> Never mind, Pavel didn't write this code.  Sorry, Nate.  This code is
> nice enough.  Pavel is just making me annoyed with his constant waste of
> time responses to what are essentially automated emails.

It is you that is wasting people's time with automated emails and
lying subjects... then claiming that people can fix that in their
procmailrc.

If your favourite toy maybe flagged something in the code, mark it as
such. That toy has rather lot of false-positives. Its sibling toy
broke boot for me not-too recently.

If you have to do "essentially automated" emails, at least take care
to make it clear in the subject.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]

Re: [bug report] leds: Add support for Philips PCA955x I2C LED drivers

[Nate Case]

On Aug 17, 2017, at 6:51 AM, Dan Carpenter dan.carpenter@oracle.com wrote:
> > > Hello Nate Case,

> > > The patch f46e9203d9a1: "leds: Add support for Philips PCA955x I2C
> > > LED drivers" from Jul 16, 2008, leads to the following static checker
> > > warning:
---[snip]---
> From the comments, it looked like maybe a different test was intended.
> Anyway, that's the point of these warnings is because many times the
> bogus NULL test should be replaced with a correct test. I don't forward
> the warning if it's obvious that the test should just be deleted.

> I probably could review the warnings even more but I am pretty busy and
> the code is obviously bogus and it's easier for the author.

> regards,
> dan carpenter

Hi Dan,

My memory is actually fuzzy on this since I wrote this code in 2008.

But, looking back, I can agree that Colin King's patch is harmless.  I
think my true original intention was to use an LED name of
"pca955x:<bit number>" as a fallback if a name was not specified in
the device tree.

So, yes, a better way to implement that would be to rework it to
something like this:

    /* Default LED name */
    snprintf(pca955x_led->name, sizeof(pca955x_led->name), "pca955x:%d", i);

    if (pdata) {
        if (pdata->leds[i].name[0] != '\0')
            /* Override LED name from pdata */
            snprintf(pca955x_led->name, ....);
        if (pdata->leds[i].default_trigger)
            ...
    }
  
Otherwise, it looks like it'd be possible to end up with LED names
of "pca955x:" in some corner cases.

Thanks,

Nate Case <ncase@xes-inc.com>

Re: [bug report] leds: Add support for Philips PCA955x I2C LED drivers

[Dan Carpenter]

HA ha ha ha ha ha ha....

Pavel, ya idjit.  You were wrong.  The code was buggy.

regards,
dan carpenter

Re: [bug report] leds: Add support for Philips PCA955x I2C LED drivers

[Pavel Machek]

[-- Attachment #1: Type: text/plain, Size: 2404 bytes --]

On Thu 2017-08-17 13:28:47, Dan Carpenter wrote:
> Hello Nate Case,
> 
> The patch f46e9203d9a1: "leds: Add support for Philips PCA955x I2C
> LED drivers" from Jul 16, 2008, leads to the following static checker
> warning:
> 
> 	drivers/leds/leds-pca955x.c:476 pca955x_probe()
> 	warn: this array is probably non-NULL. 'pdata->leds + i->name'
> 
> drivers/leds/leds-pca955x.c
>    465                  switch (pca955x_led->type) {
>    466                  case PCA955X_TYPE_NONE:
>    467                          break;
>    468                  case PCA955X_TYPE_GPIO:
>    469                          ngpios++;
>    470                          break;
>    471                  case PCA955X_TYPE_LED:
>    472                          /*
>    473                           * Platform data can specify LED names and
>    474                           * default triggers
>    475                           */
>    476                          if (pdata->leds[i].name)
>                                     ^^^^^^^^^^^^^^^^^^^
> The comment implies that we should be testing pdata->leds[i].name[0] to
> see if any string has been set?
> 

From: Colin King <colin.king@canonical.com>
To: Richard Purdie <rpurdie@rpsys.net>, Jacek Anaszewski
<jacek.anaszewski@gmail.com>, Pavel
        Machek <pavel@ucw.cz>, linux-leds@vger.kernel.org
	Cc: kernel-janitors@vger.kernel.org,
	linux-kernel@vger.kernel.org
	Subject: [PATCH][leds-next] leds: pca955x: remove redundant
	null check on array name
	X-Mailer: git-send-email 2.11.0

From: Colin Ian King <colin.king@canonical.com>

The check to see if pdata-leds[i].name is redundant as name is
a 32 byte char array and hence can never be null.  Remove the
redundant check.

Detected by CoverityScan, CID#1454218 ("Array compared against 0")

Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
 drivers/leds/leds-pca955x.c | 7 +++----
  1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/leds/leds-pca955x.c b/drivers/leds/leds-pca955x.c
index f062d1e7640f..b12b7ea51867 100644
--- a/drivers/leds/leds-pca955x.c
+++ b/drivers/leds/leds-pca955x.c
@@ -473,10 +473,9 @@ static int pca955x_probe(struct i2c_client
*client,

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]