diff mbox

Re: [PATCH] rtc: test before subtraction on unsigned

Message ID 20090302145145.ee3fafc1.akpm@linux-foundation.org
State Superseded, archived
Headers show

Commit Message

Andrew Morton March 2, 2009, 10:51 p.m. UTC 
On Mon, 02 Mar 2009 15:41:36 +0100
Roel Kluin <roel.kluin@gmail.com> wrote:

> please review.
> ------------------------------>8-------------8<---------------------------------
> new_alarm is unsigned so test before the subtraction.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> diff --git a/drivers/rtc/rtc-ds1374.c b/drivers/rtc/rtc-ds1374.c
> index a5b0fc0..39129cf 100644
> --- a/drivers/rtc/rtc-ds1374.c
> +++ b/drivers/rtc/rtc-ds1374.c
> @@ -222,16 +222,16 @@ static int ds1374_set_alarm(struct device *dev, struct rtc_wkalrm *alarm)
>  	rtc_tm_to_time(&alarm->time, &new_alarm);
>  	rtc_tm_to_time(&now, &itime);
>  
> -	new_alarm -= itime;
> -
>  	/* This can happen due to races, in addition to dates that are
>  	 * truly in the past.  To avoid requiring the caller to check for
>  	 * races, dates in the past are assumed to be in the recent past
>  	 * (i.e. not something that we'd rather the caller know about via
>  	 * an error), and the alarm is set to go off as soon as possible.
>  	 */
> -	if (new_alarm <= 0)
> +	if (new_alarm <= itime)
>  		new_alarm = 1;
> +	else
> +		new_alarm -= itime;
>  
>  	mutex_lock(&ds1374->mutex);
>  

The code still fails to correctly handle wrapping.

It's possible that these quantities can never wrap, dunno.  But still,
it would be better to implement in a wrapping-safe fashion and remove
all doubt.

Something like this, perhaps:
diff mbox

Patch

--- a/drivers/rtc/rtc-ds1374.c~rtc-test-before-subtraction-on-unsigned-fix
+++ a/drivers/rtc/rtc-ds1374.c
@@ -228,7 +228,7 @@  static int ds1374_set_alarm(struct devic
 	 * (i.e. not something that we'd rather the caller know about via
 	 * an error), and the alarm is set to go off as soon as possible.
 	 */
-	if (new_alarm <= itime)
+	if (time_before_eq(new_alarm, itime))
 		new_alarm = 1;
 	else
 		new_alarm -= itime;