| Message ID | 1417089760-26848-2-git-send-email-pang.xunlei@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On Thu, 27 Nov 2014, Xunlei Pang wrote: > 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. Well. The driver has some more issues and again you are just blindly following your y2038 agenda instead of looking what this stuff is actually doing. > -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; So why does this convert a split representation, which could be easily represented in a struct rtc_time to time64t? Now looking at the usage sites: > 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() > 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); Ditto > 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); Ditto. Makes a lot of sense? NOT I did not have to look at the actual code to notice that nonsense. Looking at the patch was enough. Can you folks please stop to run shell scripts without looking at the actual code? This mechanical attempt to fix the 2038 issue w/o switching on braincells is just annoying as hell. Dammit, if you touch stuff then you better look at it and fix it proper instead of copying the same crap over and over. Thanks, tglx
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. Arnd
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
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;
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(-)