bug report: signedness issue in ds3232_update_alarm()

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

Jack's email <[email protected]> is dead. B25806, could you take a look?

regards,
dan carpenter

Quoted 6 lines · 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?

Quoted 17 lines

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

Quoted 2 lines

regards,
dan carpenter

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.

Quoted 4 lines · 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;
}