Re: [RFC PATCH 1/4] rtc/mxc: Convert get_alarm_or_time()/set_alarm_or_time() to use time64_t

[pang.xunlei]

On 28 November 2014 at 07:47, Arnd Bergmann <arnd@xxxxxxxx> wrote:
> On Friday 28 November 2014 00:02:47 Thomas Gleixner wrote:
>>
>> >  static int rtc_update_alarm(struct device *dev, struct rtc_time *alrm)
>> >  {
>> >       struct rtc_time alarm_tm, now_tm;
>> > -     unsigned long now, time;
>> > +     time64_t now, time;
>> >       struct platform_device *pdev = to_platform_device(dev);
>> >       struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
>> >       void __iomem *ioaddr = pdata->ioaddr;
>> >
>> >       now = get_alarm_or_time(dev, MXC_RTC_TIME);
>> > -     rtc_time_to_tm(now, &now_tm);
>> > +     rtc_time64_to_tm(now, &now_tm);
>>
>> So here you convert that to struct rtc_time.
>>
>> >       alarm_tm.tm_year = now_tm.tm_year;
>> >       alarm_tm.tm_mon = now_tm.tm_mon;
>> >       alarm_tm.tm_mday = now_tm.tm_mday;
>> >       alarm_tm.tm_hour = alrm->tm_hour;
>> >       alarm_tm.tm_min = alrm->tm_min;
>> >       alarm_tm.tm_sec = alrm->tm_sec;
>> > -     rtc_tm_to_time(&alarm_tm, &time);
>> > +     time = rtc_tm_to_time64(&alarm_tm);
>>
>> Just to convert it back and then do the reverse operation in
>> set_alarm_or_time()
>
> Apparently the code originally tried to make the alarm fire within
> the next 24 hours. What you and the author of c92182ee0b5a3
> ("drivers/rtc/rtc-mxc.c: make alarm work") apparently missed is that
> it is taking the year/mon/mday value of today and the hour/min/sec
> value from the function argument.
>
> I think the idea was to make it work with user space that sets only
> the last three fields (which would work on the typical x86 rtc),
> but after that other patch, the function no longer makes any sense,
> since it will set the alarm in the past half of the time.

In rtc_dev_ioctl() RTC_ALM_SET, there is some common code handling such issue:
/* alarm may need to wrap into tomorrow */
if (then < now) {
rtc_time_to_tm(now + 24 * 60 * 60, &tm);
alarm.time.tm_mday = tm.tm_mday;
alarm.time.tm_mon = tm.tm_mon;
alarm.time.tm_year = tm.tm_year;
}
}

return rtc_set_alarm(rtc, &alarm);

But it still has a small window may set the past alarm time, even in
rtc drivers' set_alarm().
I think the driver should behave as follows to completely avoid this issue:

disable local irq

make sure the alarm time is later than now by delta, this delta should
be long enough to finish regs operations.

set alarm regs

enable local irq

Sadly, lots of rtc drivers have problems.

Regards,
-Xunlei
>
>         Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/