Message-ID: <20160921232903.lb43z3vzx33zenwq@piout.net>
Date:   Thu, 22 Sep 2016 01:29:03 +0200
From:   Alexandre Belloni <alexandre.belloni@...e-electrons.com>
To:     Gabriele Mazzotta <gabriele.mzt@...il.com>
Cc:     a.zummo@...ertech.it, rtc-linux@...glegroups.com,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] rtc-cmos: Reject unsupported alarm values

On 02/09/2016 at 21:55:16 +0200, Gabriele Mazzotta wrote :
> Some platforms allows to specify the month and day of the month in
> which an alarm should go off, some others the day of the month and
> some others just the time.
> 
> Currently any given value is accepted by the driver and only the
> supported fields are used to program the hardware. As consequence,
> alarms are potentially programmed to go off in the wrong moment.
> 
> Fix this by rejecting any value not supported by the hardware.
> 
> Signed-off-by: Gabriele Mazzotta <gabriele.mzt@...il.com>
> ---
> 
> I revisited the naive implementation of v1. I tested the new
> algorithm using some dates that and verified that it behaved as
> expected, but I might have missed some corner cases.
> 
> I made some assumptions that maybe should be dropped, at least
> two of them. They are commented in the code, but I didn't mention
> that they are assumptions:
> 
>  - If the day can't be specified, the alarm can only be set to go
>    off 24 hours minus 1 second in the future. I'm worried things
>    would go wrong if the current time is used to set an alarm that
>    should go off the next day.
>  - If the mday can be specified and the next month has more days
>    than the current month, the alarm can be set to go off in the
>    extra days of the next month.

Hum, you are actually not allowing them in the code below (which I think
is the right thing to do).

>  - If the month can be specified, it's the 28th of February and the
>    next year is a leap year, the alarm can be set for the 29th of
>    February of the next year.

Is that really true? I would expect the opposite. If it is currently
28/02, the max date you can actually go to is 27/02. If you allow 29/02,
at some time the RTC will have 28/02 and the alarm will fire.

> 
> Basically I'm assuming that the hardware decides when an alarm should
> go off comparing the current date with the one programmed. If they
> match, the alarm goes off. This seemed reasonable to me, but it's
> not easy to verify.
> 
>  drivers/rtc/rtc-cmos.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 104 insertions(+)
> 
> diff --git a/drivers/rtc/rtc-cmos.c b/drivers/rtc/rtc-cmos.c
> index 4cdb335..37cb7c1 100644
> --- a/drivers/rtc/rtc-cmos.c
> +++ b/drivers/rtc/rtc-cmos.c
> @@ -328,14 +328,118 @@ static void cmos_irq_disable(struct cmos_rtc *cmos, unsigned char mask)
>  	cmos_checkintr(cmos, rtc_control);
>  }
>  
> +static int cmos_validate_alarm(struct device *dev, struct rtc_wkalrm *t)
> +{
> +	struct cmos_rtc *cmos = dev_get_drvdata(dev);
> +	struct rtc_time now;
> +
> +	cmos_read_time(dev, &now);
> +
> +	if (!cmos->day_alrm) {
> +		time64_t t_max_date;
> +		time64_t t_alrm;
> +
> +		t_alrm = rtc_tm_to_time64(&t->time);
> +		t_max_date = rtc_tm_to_time64(&now);
> +		/*
> +		 * Subtract 1 second to ensure that the alarm time is
> +		 * different from the current time.
> +		 */
> +		t_max_date += 24 * 60 * 60 - 1;
> +		if (t_alrm > t_max_date) {
> +			dev_err(dev,
> +				"Alarms can be up to one day in the future\n");
> +			return -EINVAL;
> +		}
> +	} else if (!cmos->mon_alrm) {
> +		struct rtc_time max_date = now;
> +		time64_t t_max_date;
> +		time64_t t_alrm;
> +		int max_mday;
> +		bool is_max_mday = false;
> +
> +		/*
> +		 * If the next month has more days than the current month
> +		 * and we are at the max mday of this month, we can program
> +		 * the alarm to go off the max mday of the next month without
> +		 * it going off sooner than expected.
> +		 */
> +		max_mday = rtc_month_days(now.tm_mon, now.tm_year);
> +		if (now.tm_mday == max_mday)
> +			is_max_mday = true;
> +
> +		if (max_date.tm_mon == 11) {
> +			max_date.tm_mon = 0;
> +			max_date.tm_year += 1;
> +		} else {
> +			max_date.tm_mon += 1;
> +		}
> +		max_mday = rtc_month_days(max_date.tm_mon, max_date.tm_year);
> +		if (max_date.tm_mday > max_mday || is_max_mday)
> +			max_date.tm_mday = max_mday;
> +
> +		max_date.tm_hour = 23;
> +		max_date.tm_min = 59;
> +		max_date.tm_sec = 59;
> +

Actually, this is wrong.

If it is currently 1:23:45 on 22/09, you can go up to 1:23:44 on 22/10.
trying to set a time after 1:23:45 will actually match on the same day
instead of a month later.

> +		t_max_date = rtc_tm_to_time64(&max_date);
> +		t_alrm = rtc_tm_to_time64(&t->time);
> +		if (t_alrm > t_max_date) {
> +			dev_err(dev,
> +				"Alarms can be up to one month in the future\n");
> +			return -EINVAL;
> +		}
> +	} else {
> +		struct rtc_time max_date = now;
> +		time64_t t_max_date;
> +		time64_t t_alrm;
> +		int max_mday;
> +		bool allow_leap_day = false;
> +
> +		/*
> +		 * If it's the 28th of February and the next year is a leap
> +		 * year, allow to set alarms for the 29th of February.
> +		 */
> +		if (now.tm_mon == 1) {
> +			max_mday = rtc_month_days(now.tm_mon, now.tm_year);
> +			if (now.tm_mday == max_mday)
> +				allow_leap_day = true;
> +		}
> +
> +		max_date.tm_year += 1;
> +		max_mday = rtc_month_days(max_date.tm_mon, max_date.tm_year);
> +		if (max_date.tm_mday > max_mday || allow_leap_day)
> +			max_date.tm_mday = max_mday;
> +
> +		max_date.tm_hour = 23;
> +		max_date.tm_min = 59;
> +		max_date.tm_sec = 59;
> +

Ditto, 1:23:45 on 22/09/2016 can go up to 1:23:44 on 22/09/2017.

Regards,

-- 
Alexandre Belloni, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Powered by blists - more mailing lists