bug report: signedness issue in ds3232_update_alarm()

bug report: signedness issue in ds3232_update_alarm()

[Dan Carpenter]

Hi Jack,

This code was added in f46418c5cadf "drivers/rtc/rtc-ds3232.c: add alarm
function".  Smatch complains because bcd2bin() returns an unsigned so
it's never less than zero and the whole block is a no-op.  I'm not sure
what was intended here, could you take a look?

drivers/rtc/rtc-ds3232.c +292 ds3232_update_alarm(13)
	warn: unsigned 'bcd2bin(buf[0])' is never less than zero.
   291
   292          buf[0] = bcd2bin(buf[0]) < 0 || (ds3232->rtc->irq_data & RTC_UF) ?
   293									0x80 : buf[0];
   294          buf[1] = bcd2bin(buf[1]) < 0 || (ds3232->rtc->irq_data & RTC_UF) ?
   295									0x80 : buf[1];
   296          buf[2] = bcd2bin(buf[2]) < 0 || (ds3232->rtc->irq_data & RTC_UF) ?
   297									0x80 : buf[2];
   298          buf[3] = bcd2bin(buf[3]) < 0 || (ds3232->rtc->irq_data & RTC_UF) ?
   299									0x80 : buf[3];
   300

regards,
dan carpenter

Re: bug report: signedness issue in ds3232_update_alarm()

[Dan Carpenter]

Jack's email <jack.lan@freescale.com> is dead.  B25806, could you take
a look?

regards,
dan carpenter

On Thu, Oct 28, 2010 at 10:35:03AM +0200, Dan Carpenter wrote:
> Hi Jack,
> 
> This code was added in f46418c5cadf "drivers/rtc/rtc-ds3232.c: add alarm
> function".  Smatch complains because bcd2bin() returns an unsigned so
> it's never less than zero and the whole block is a no-op.  I'm not sure
> what was intended here, could you take a look?
> 
> drivers/rtc/rtc-ds3232.c +292 ds3232_update_alarm(13)
> 	warn: unsigned 'bcd2bin(buf[0])' is never less than zero.
>    291
>    292          buf[0] = bcd2bin(buf[0]) < 0 || (ds3232->rtc->irq_data & RTC_UF) ?
>    293									0x80 : buf[0];
>    294          buf[1] = bcd2bin(buf[1]) < 0 || (ds3232->rtc->irq_data & RTC_UF) ?
>    295									0x80 : buf[1];
>    296          buf[2] = bcd2bin(buf[2]) < 0 || (ds3232->rtc->irq_data & RTC_UF) ?
>    297									0x80 : buf[2];
>    298          buf[3] = bcd2bin(buf[3]) < 0 || (ds3232->rtc->irq_data & RTC_UF) ?
>    299									0x80 : buf[3];
>    300
> 
> regards,
> dan carpenter

Re: bug report: signedness issue in ds3232_update_alarm()

[Lan Chunhe-B25806]

On Thu, 28 Oct 2010 16:55:22 +0800, Dan Carpenter <error27@gmail.com>  
wrote:

> Jack's email <jack.lan@freescale.com> is dead.  B25806, could you take
> a look?

    Sorry, you can use b25806@freescale.com OR chunhe.lan@gmail.com.

> regards,
> dan carpenter
>
> On Thu, Oct 28, 2010 at 10:35:03AM +0200, Dan Carpenter wrote:
>> Hi Jack,
>>
>> This code was added in f46418c5cadf "drivers/rtc/rtc-ds3232.c: add alarm
>> function".  Smatch complains because bcd2bin() returns an unsigned so
>> it's never less than zero and the whole block is a no-op.  I'm not sure
>> what was intended here, could you take a look?

       Of course, bcd2bin()/Time is never less than zero. But it is fault  
tolerant.
       When time has exceptional data, it set time to the default value.

>> drivers/rtc/rtc-ds3232.c +292 ds3232_update_alarm(13)
>> 	warn: unsigned 'bcd2bin(buf[0])' is never less than zero.
>>    291
>>    292          buf[0] = bcd2bin(buf[0]) < 0 || (ds3232->rtc->irq_data  
>> & RTC_UF) ?
>>    293									0x80 : buf[0];
>>    294          buf[1] = bcd2bin(buf[1]) < 0 || (ds3232->rtc->irq_data  
>> & RTC_UF) ?
>>    295									0x80 : buf[1];
>>    296          buf[2] = bcd2bin(buf[2]) < 0 || (ds3232->rtc->irq_data  
>> & RTC_UF) ?
>>    297									0x80 : buf[2];
>>    298          buf[3] = bcd2bin(buf[3]) < 0 || (ds3232->rtc->irq_data  
>> & RTC_UF) ?
>>    299									0x80 : buf[3];
>>    300
>>
>> regards,
>> dan carpenter

      Thanks.

      Jack Lan

Re: bug report: signedness issue in ds3232_update_alarm()

[Dan Carpenter]

On Thu, Oct 28, 2010 at 06:15:46PM +0800, Lan Chunhe-B25806 wrote:
>       Of course, bcd2bin()/Time is never less than zero. But it is fault  
> tolerant.
>       When time has exceptional data, it set time to the default value.
>

Sorry, I still don't understand what you're trying to do.  This showed
up on my bug scanner, and I've studied it but as far as I can see it
doesn't do anything.  I've created a sample program to demonstrate what
I mean.

It passes every possible non-zero value to bcd2bin() and counts how many
of the returns are negative.

regards,
dan carpenter

#include <stdio.h>

unsigned bcd2bin(unsigned char val)
{
        return (val & 0x0f) + (val >> 4) * 10;
}

int main(int argc, char **argv)
{
	unsigned char x;
	int useful = 0;

	for (x = 1; x; x++) {
		if (bcd2bin(x) < 0)
			useful++;
	}

	printf("useful = %d\n", useful);
	return 0;
}

Re: bug report: signedness issue in ds3232_update_alarm()

[Lan Chunhe-B25806]

On Thu, 28 Oct 2010 18:58:40 +0800, Dan Carpenter <error27@gmail.com>  
wrote:

> On Thu, Oct 28, 2010 at 06:15:46PM +0800, Lan Chunhe-B25806 wrote:
>>       Of course, bcd2bin()/Time is never less than zero. But it is fault
>> tolerant.
>>       When time has exceptional data, it set time to the default value.
>>
>
> Sorry, I still don't understand what you're trying to do.  This showed
> up on my bug scanner, and I've studied it but as far as I can see it
> doesn't do anything.  I've created a sample program to demonstrate what
> I mean.

    If you must not display these message on your bug scanner, you can
    modify code from

    buf[0] = bcd2bin(buf[0]) < 0 || (ds3232->rtc->irq_data & RTC_UF) ?
						0x80 : buf[0];
    to

    buf[0] = (ds3232->rtc->irq_data & RTC_UF) ?
						0x80 : buf[0];

> It passes every possible non-zero value to bcd2bin() and counts how many
> of the returns are negative.
>
> regards,
> dan carpenter
>
> #include <stdio.h>
>
> unsigned bcd2bin(unsigned char val)
> {
>         return (val & 0x0f) + (val >> 4) * 10;
> }
>
> int main(int argc, char **argv)
> {
> 	unsigned char x;
> 	int useful = 0;
>
> 	for (x = 1; x; x++) {
> 		if (bcd2bin(x) < 0)
> 			useful++;
> 	}
>
> 	printf("useful = %d\n", useful);
> 	return 0;
> }
>
    Thanks.

    Jack Lan