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

Message ID 1417089760-26848-2-git-send-email-pang.xunlei@linaro.org
State New
Headers show

Commit Message

pang.xunlei Nov. 27, 2014, 12:02 p.m.
We want to convert y2038 unsafe rtc_class_ops.set_mmss() and all its
users to use time64_t. mxc_rtc_set_mmss() in "mxc" driver is one of
its users, but it uses get_alarm_or_time()/set_alarm_or_time() internal
interfaces which are also y2038 unsafe.

So here as a separate patch, it converts these two internal interfaces
of "mxc" to use safe time64_t first, so as to make some preparations
for the rtc_class_ops.set_mmss() conversion.

Currently, "mxc" is the only driver with such issue.

Cc: John Stultz <john.stultz@linaro.org>
Cc: Arnd Bergmann <arnd.bergmann@linaro.org>
Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
---
 drivers/rtc/rtc-mxc.c |   31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

Comments

pang.xunlei Nov. 28, 2014, 3:58 p.m. | #1
On 28 November 2014 at 07:47, Arnd Bergmann <arnd@arndb.de> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Patch

diff --git a/drivers/rtc/rtc-mxc.c b/drivers/rtc/rtc-mxc.c
index 419874f..07bff34 100644
--- a/drivers/rtc/rtc-mxc.c
+++ b/drivers/rtc/rtc-mxc.c
@@ -106,7 +106,7 @@  static inline int is_imx1_rtc(struct rtc_plat_data *data)
  * This function is used to obtain the RTC time or the alarm value in
  * second.
  */
-static u32 get_alarm_or_time(struct device *dev, int time_alarm)
+static time64_t get_alarm_or_time(struct device *dev, int time_alarm)
 {
 	struct platform_device *pdev = to_platform_device(dev);
 	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
@@ -129,29 +129,28 @@  static u32 get_alarm_or_time(struct device *dev, int time_alarm)
 	hr = hr_min >> 8;
 	min = hr_min & 0xff;
 
-	return (((day * 24 + hr) * 60) + min) * 60 + sec;
+	return ((((time64_t)day * 24 + hr) * 60) + min) * 60 + sec;
 }
 
 /*
  * This function sets the RTC alarm value or the time value.
  */
-static void set_alarm_or_time(struct device *dev, int time_alarm, u32 time)
+static void set_alarm_or_time(struct device *dev, int time_alarm, time64_t time)
 {
-	u32 day, hr, min, sec, temp;
+	u32 tod, day, hr, min, sec, temp;
 	struct platform_device *pdev = to_platform_device(dev);
 	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
 	void __iomem *ioaddr = pdata->ioaddr;
 
-	day = time / 86400;
-	time -= day * 86400;
+	day = div_s64_rem(time, 86400, &tod);
 
 	/* time is within a day now */
-	hr = time / 3600;
-	time -= hr * 3600;
+	hr = tod / 3600;
+	tod -= hr * 3600;
 
 	/* time is within an hour now */
-	min = time / 60;
-	sec = time - min * 60;
+	min = tod / 60;
+	sec = tod - min * 60;
 
 	temp = (hr << 8) + min;
 
@@ -176,20 +175,20 @@  static void set_alarm_or_time(struct device *dev, int time_alarm, u32 time)
 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);
 	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);
 
 	/* clear all the interrupt status bits */
 	writew(readw(ioaddr + RTC_RTCISR), ioaddr + RTC_RTCISR);
@@ -283,14 +282,14 @@  static int mxc_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled)
  */
 static int mxc_rtc_read_time(struct device *dev, struct rtc_time *tm)
 {
-	u32 val;
+	time64_t val;
 
 	/* Avoid roll-over from reading the different registers */
 	do {
 		val = get_alarm_or_time(dev, MXC_RTC_TIME);
 	} while (val != get_alarm_or_time(dev, MXC_RTC_TIME));
 
-	rtc_time_to_tm(val, tm);
+	rtc_time64_to_tm(val, tm);
 
 	return 0;
 }
@@ -333,7 +332,7 @@  static int mxc_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm)
 	struct rtc_plat_data *pdata = platform_get_drvdata(pdev);
 	void __iomem *ioaddr = pdata->ioaddr;
 
-	rtc_time_to_tm(get_alarm_or_time(dev, MXC_RTC_ALARM), &alrm->time);
+	rtc_time64_to_tm(get_alarm_or_time(dev, MXC_RTC_ALARM), &alrm->time);
 	alrm->pending = ((readw(ioaddr + RTC_RTCISR) & RTC_ALM_BIT)) ? 1 : 0;
 
 	return 0;