Messages in this thread | | | From | Baolin Wang <> | Date | Mon, 7 May 2018 15:57:47 +0800 | Subject | Re: [PATCH v2 2/2] MIPS: Convert update_persistent_clock() to update_persistent_clock64() |
| |
On 6 May 2018 at 11:04, Arnd Bergmann <arnd@arndb.de> wrote: > On Fri, May 4, 2018 at 3:07 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >> Since struct timespec is not y2038 safe on 32bit machines, this patch >> converts update_persistent_clock() to update_persistent_clock64() using >> struct timespec64. >> >> The rtc_mips_set_time() and rtc_mips_set_mmss() interfaces were using >> 'unsigned long' type that is not y2038 safe on 32bit machines, moreover >> there is only one platform implementing rtc_mips_set_time() and two >> platforms implementing rtc_mips_set_mmss(), so we can just make them each >> implement update_persistent_clock64() directly, to get that helper out >> of the common mips code by removing rtc_mips_set_time() and >> rtc_mips_set_mmss() interfaces. >> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > Looks good overall, but I still found a bug and one minor issue. With > those fixed, > > Acked-by: Arnd Bergmann <arnd@arndb.de> > >> diff --git a/arch/mips/dec/time.c b/arch/mips/dec/time.c >> index 9e992cf..934db6f 100644 >> --- a/arch/mips/dec/time.c >> +++ b/arch/mips/dec/time.c >> @@ -59,14 +59,15 @@ void read_persistent_clock64(struct timespec64 *ts) >> } >> >> /* >> - * In order to set the CMOS clock precisely, rtc_mips_set_mmss has to >> + * In order to set the CMOS clock precisely, update_persistent_clock64 has to >> * be called 500 ms after the second nowtime has started, because when >> * nowtime is written into the registers of the CMOS clock, it will >> * jump to the next second precisely 500 ms later. Check the Dallas >> * DS1287 data sheet for details. >> */ >> -int rtc_mips_set_mmss(unsigned long nowtime) >> +int update_persistent_clock64(struct timespec64 now) >> { >> + time64_t nowtime = now.tv_sec; >> int retval = 0; >> int real_seconds, real_minutes, cmos_minutes; >> unsigned char save_control, save_freq_select; > > > It looks like you now get an invalid 64-bit division in here, > you have to change it to either use div_u64_rem() or possibly > time64_to_tm() or rtc_time64_to_tm() (the latter requires > CONFIG_RTC_LIB).
I will use div_u64_rem() to calculate real_seconds and real_minutes.
> >> diff --git a/arch/mips/lasat/ds1603.c b/arch/mips/lasat/ds1603.c >> index d75c887..061815e 100644 >> --- a/arch/mips/lasat/ds1603.c >> +++ b/arch/mips/lasat/ds1603.c >> @@ -98,7 +98,7 @@ static void rtc_write_byte(unsigned int byte) >> } >> } >> >> -static void rtc_write_word(unsigned long word) >> +static void rtc_write_word(time64_t word) >> { >> int i; >> > > I would say this function should take a 'u32' argument (or keep the > unsigned long) to match the name and implementation, but then have a > type cast in the caller with a comment about the loss of range and overflow > in y2106.
OK.
> >> diff --git a/arch/mips/lasat/sysctl.c b/arch/mips/lasat/sysctl.c >> index 6f74224..76f7b62 100644 >> --- a/arch/mips/lasat/sysctl.c >> +++ b/arch/mips/lasat/sysctl.c >> @@ -73,8 +73,12 @@ int proc_dolasatrtc(struct ctl_table *table, int write, >> if (r) >> return r; >> >> - if (write) >> - rtc_mips_set_mmss(rtctmp); >> + if (write) { >> + ts.tv_sec = rtctmp; >> + ts.tv_nsec = 0; >> + >> + update_persistent_clock64(ts); >> + } >> > ... and probably also a comment here to explain that we can't actually use > the full 64-bit range because of HW limits. >
OK. Thanks for your comments.
-- Baolin.wang Best Regards
| |