| Message ID | 0ca46228311ec615947e199def9fed62d70c1f07.1524118799.git.baolin.wang@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | m68k: Remove read_persistent_clock() | expand |
On Thu, Apr 19, 2018 at 8:22 AM, Baolin Wang <baolin.wang@linaro.org> wrote: > The read_persistent_clock() uses a timespec, which is not year 2038 safe > on 32bit systems. Moreover on m68k architecture, we have implemented generic > RTC drivers that can be used to compensate the system suspend time. So > we can remove the obsolete read_persistent_clock(). > > Signed-off-by: Baolin Wang <baolin.wang@linaro.org> I'm not sure about this one: it's still possible to turn off CONFIG_RTC_DRV_GENERIC on M68KCLASSIC, and in that case we still need a read_persistent_clock64() implementation. Or we use your patch but make CONFIG_RTC_DRV_GENERIC mandatory. See below for a version I did a while ago (but never submitted as I got distracted). Arnd commit 13ddf5a33a195e9b7a7a6ed10481363b5259c1d4 Author: Arnd Bergmann <arnd@arndb.de> Date: Thu Jan 25 15:30:50 2018 +0100 m68k: use read_persistent_clock64 consistently We have two ways of getting the current time from a platform at boot or during suspend: either using read_persistent_clock() or the rtc class operation. We never need both, so I'm hiding the read_persistent_clock variant when the generic RTC is enabled. Since read_persistent_clock() and mktime() are deprecated because of the y2038 overflow of time_t, we should use the time64_t based replacements here. Finally, the dependency on CONFIG_ARCH_USES_GETTIMEOFFSET looks completely bogus in this case, so let's remove that. It was added in commit b13b3f51ff7b ("m68k: fix inclusion of arch_gettimeoffset for non-MMU 68k classic CPU types") to deal with arch_gettimeoffset(), which has since been removed from this file and is unrelated to the RTC functions. The rtc accessors are only used by classic machines, while coldfire uses proper RTC drivers, so we can put the old ifdef back around both functions. Signed-off-by: Arnd Bergmann <arnd@arndb.de> diff --git a/arch/m68k/kernel/time.c b/arch/m68k/kernel/time.c index 97dd4e26f234..edcf3855d8b0 100644 --- a/arch/m68k/kernel/time.c +++ b/arch/m68k/kernel/time.c @@ -71,7 +71,9 @@ static irqreturn_t timer_interrupt(int irq, void *dummy) return IRQ_HANDLED; } -void read_persistent_clock(struct timespec *ts) +#ifdef CONFIG_M68KCLASSIC +#if !IS_BUILTIN(CONFIG_RTC_DRV_GENERIC) +void read_persistent_clock64(struct timespec64 *ts) { struct rtc_time time; ts->tv_sec = 0; @@ -82,12 +84,13 @@ void read_persistent_clock(struct timespec *ts) if ((time.tm_year += 1900) < 1970) time.tm_year += 100; - ts->tv_sec = mktime(time.tm_year, time.tm_mon, time.tm_mday, + ts->tv_sec = mktime64(time.tm_year, time.tm_mon, time.tm_mday, time.tm_hour, time.tm_min, time.tm_sec); } } +#endif -#if defined(CONFIG_ARCH_USES_GETTIMEOFFSET) && IS_ENABLED(CONFIG_RTC_DRV_GENERIC) +#if IS_ENABLED(CONFIG_RTC_DRV_GENERIC) static int rtc_generic_get_time(struct device *dev, struct rtc_time *tm) { mach_hwclk(0, tm); @@ -145,8 +148,8 @@ static int __init rtc_init(void) } module_init(rtc_init); - -#endif /* CONFIG_ARCH_USES_GETTIMEOFFSET */ +#endif /* CONFIG_RTC_DRV_GENERIC */ +#endif /* CONFIG M68KCLASSIC */ void __init time_init(void) {
On 20 April 2018 at 23:22, Arnd Bergmann <arnd@arndb.de> wrote: > On Thu, Apr 19, 2018 at 8:22 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >> The read_persistent_clock() uses a timespec, which is not year 2038 safe >> on 32bit systems. Moreover on m68k architecture, we have implemented generic >> RTC drivers that can be used to compensate the system suspend time. So >> we can remove the obsolete read_persistent_clock(). >> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > I'm not sure about this one: it's still possible to turn off > CONFIG_RTC_DRV_GENERIC > on M68KCLASSIC, and in that case we still need a read_persistent_clock64() > implementation. Or we use your patch but make CONFIG_RTC_DRV_GENERIC > mandatory. OK. Thanks for fixing the issue with your patch. > > See below for a version I did a while ago (but never submitted as I got > distracted). > > Arnd > > commit 13ddf5a33a195e9b7a7a6ed10481363b5259c1d4 > Author: Arnd Bergmann <arnd@arndb.de> > Date: Thu Jan 25 15:30:50 2018 +0100 > > m68k: use read_persistent_clock64 consistently > > We have two ways of getting the current time from a platform at boot > or during suspend: either using read_persistent_clock() or the rtc > class operation. We never need both, so I'm hiding the > read_persistent_clock variant when the generic RTC is enabled. > > Since read_persistent_clock() and mktime() are deprecated because of > the y2038 overflow of time_t, we should use the time64_t based > replacements here. > > Finally, the dependency on CONFIG_ARCH_USES_GETTIMEOFFSET looks > completely bogus in this case, so let's remove that. It was > added in commit b13b3f51ff7b ("m68k: fix inclusion of > arch_gettimeoffset for non-MMU 68k classic CPU types") to deal > with arch_gettimeoffset(), which has since been removed from > this file and is unrelated to the RTC functions. > > The rtc accessors are only used by classic machines, while > coldfire uses proper RTC drivers, so we can put the old > ifdef back around both functions. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > diff --git a/arch/m68k/kernel/time.c b/arch/m68k/kernel/time.c > index 97dd4e26f234..edcf3855d8b0 100644 > --- a/arch/m68k/kernel/time.c > +++ b/arch/m68k/kernel/time.c > @@ -71,7 +71,9 @@ static irqreturn_t timer_interrupt(int irq, void *dummy) > return IRQ_HANDLED; > } > > -void read_persistent_clock(struct timespec *ts) > +#ifdef CONFIG_M68KCLASSIC > +#if !IS_BUILTIN(CONFIG_RTC_DRV_GENERIC) > +void read_persistent_clock64(struct timespec64 *ts) > { > struct rtc_time time; > ts->tv_sec = 0; > @@ -82,12 +84,13 @@ void read_persistent_clock(struct timespec *ts) > > if ((time.tm_year += 1900) < 1970) > time.tm_year += 100; > - ts->tv_sec = mktime(time.tm_year, time.tm_mon, time.tm_mday, > + ts->tv_sec = mktime64(time.tm_year, time.tm_mon, time.tm_mday, > time.tm_hour, time.tm_min, time.tm_sec); > } > } > +#endif > > -#if defined(CONFIG_ARCH_USES_GETTIMEOFFSET) && > IS_ENABLED(CONFIG_RTC_DRV_GENERIC) > +#if IS_ENABLED(CONFIG_RTC_DRV_GENERIC) > static int rtc_generic_get_time(struct device *dev, struct rtc_time *tm) > { > mach_hwclk(0, tm); > @@ -145,8 +148,8 @@ static int __init rtc_init(void) > } > > module_init(rtc_init); > - > -#endif /* CONFIG_ARCH_USES_GETTIMEOFFSET */ > +#endif /* CONFIG_RTC_DRV_GENERIC */ > +#endif /* CONFIG M68KCLASSIC */ > > void __init time_init(void) > { -- Baolin.wang Best Regards
Hi Arnd, On Fri, Apr 20, 2018 at 5:22 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Thu, Apr 19, 2018 at 8:22 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >> The read_persistent_clock() uses a timespec, which is not year 2038 safe >> on 32bit systems. Moreover on m68k architecture, we have implemented generic >> RTC drivers that can be used to compensate the system suspend time. So >> we can remove the obsolete read_persistent_clock(). >> >> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> > > I'm not sure about this one: it's still possible to turn off > CONFIG_RTC_DRV_GENERIC > on M68KCLASSIC, and in that case we still need a read_persistent_clock64() > implementation. Or we use your patch but make CONFIG_RTC_DRV_GENERIC > mandatory. Typically (in the defconfigs) CONFIG_RTC_DRV_GENERIC is either "m", or not set. And in both cases, a platform-specific RTC class driver may or may not be builtin or loaded. We have them for some Amigas (RTC_DRV_MSM6242 or RTC_DRV_RP5C01). I've never been an expert of timekeeping code... Should we get rid of ARCH_USES_GETTIMEOFFSET? it seems m68k and two ARM platforms are the last users. What needs to be done? > See below for a version I did a while ago (but never submitted as I got > distracted). Thanks, I can apply it, if it is deemed correct ;-) > commit 13ddf5a33a195e9b7a7a6ed10481363b5259c1d4 > Author: Arnd Bergmann <arnd@arndb.de> > Date: Thu Jan 25 15:30:50 2018 +0100 > > m68k: use read_persistent_clock64 consistently > > We have two ways of getting the current time from a platform at boot > or during suspend: either using read_persistent_clock() or the rtc > class operation. We never need both, so I'm hiding the > read_persistent_clock variant when the generic RTC is enabled. > > Since read_persistent_clock() and mktime() are deprecated because of > the y2038 overflow of time_t, we should use the time64_t based > replacements here. > > Finally, the dependency on CONFIG_ARCH_USES_GETTIMEOFFSET looks > completely bogus in this case, so let's remove that. It was > added in commit b13b3f51ff7b ("m68k: fix inclusion of > arch_gettimeoffset for non-MMU 68k classic CPU types") to deal > with arch_gettimeoffset(), which has since been removed from > this file and is unrelated to the RTC functions. > > The rtc accessors are only used by classic machines, while > coldfire uses proper RTC drivers, so we can put the old > ifdef back around both functions. > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > diff --git a/arch/m68k/kernel/time.c b/arch/m68k/kernel/time.c > index 97dd4e26f234..edcf3855d8b0 100644 > --- a/arch/m68k/kernel/time.c > +++ b/arch/m68k/kernel/time.c > @@ -71,7 +71,9 @@ static irqreturn_t timer_interrupt(int irq, void *dummy) > return IRQ_HANDLED; > } > > -void read_persistent_clock(struct timespec *ts) > +#ifdef CONFIG_M68KCLASSIC > +#if !IS_BUILTIN(CONFIG_RTC_DRV_GENERIC) > +void read_persistent_clock64(struct timespec64 *ts) > { > struct rtc_time time; > ts->tv_sec = 0; > @@ -82,12 +84,13 @@ void read_persistent_clock(struct timespec *ts) > > if ((time.tm_year += 1900) < 1970) > time.tm_year += 100; > - ts->tv_sec = mktime(time.tm_year, time.tm_mon, time.tm_mday, > + ts->tv_sec = mktime64(time.tm_year, time.tm_mon, time.tm_mday, > time.tm_hour, time.tm_min, time.tm_sec); > } > } > +#endif > > -#if defined(CONFIG_ARCH_USES_GETTIMEOFFSET) && > IS_ENABLED(CONFIG_RTC_DRV_GENERIC) > +#if IS_ENABLED(CONFIG_RTC_DRV_GENERIC) > static int rtc_generic_get_time(struct device *dev, struct rtc_time *tm) > { > mach_hwclk(0, tm); > @@ -145,8 +148,8 @@ static int __init rtc_init(void) > } > > module_init(rtc_init); > - > -#endif /* CONFIG_ARCH_USES_GETTIMEOFFSET */ > +#endif /* CONFIG_RTC_DRV_GENERIC */ > +#endif /* CONFIG M68KCLASSIC */ > > void __init time_init(void) > { Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Baolin, On Mon, Apr 23, 2018 at 4:08 AM, Baolin Wang <baolin.wang@linaro.org> wrote: > On 20 April 2018 at 23:22, Arnd Bergmann <arnd@arndb.de> wrote: >> On Thu, Apr 19, 2018 at 8:22 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >>> The read_persistent_clock() uses a timespec, which is not year 2038 safe >>> on 32bit systems. Moreover on m68k architecture, we have implemented generic >>> RTC drivers that can be used to compensate the system suspend time. So >>> we can remove the obsolete read_persistent_clock(). >>> >>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >> >> I'm not sure about this one: it's still possible to turn off >> CONFIG_RTC_DRV_GENERIC >> on M68KCLASSIC, and in that case we still need a read_persistent_clock64() >> implementation. Or we use your patch but make CONFIG_RTC_DRV_GENERIC >> mandatory. > > OK. Thanks for fixing the issue with your patch. Can I consider that an Acked-by? Thanks! >> See below for a version I did a while ago (but never submitted as I got >> distracted). >> >> Arnd >> >> commit 13ddf5a33a195e9b7a7a6ed10481363b5259c1d4 >> Author: Arnd Bergmann <arnd@arndb.de> >> Date: Thu Jan 25 15:30:50 2018 +0100 >> >> m68k: use read_persistent_clock64 consistently Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, Apr 23, 2018 at 11:07 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Arnd, > > On Fri, Apr 20, 2018 at 5:22 PM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Thu, Apr 19, 2018 at 8:22 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >>> The read_persistent_clock() uses a timespec, which is not year 2038 safe >>> on 32bit systems. Moreover on m68k architecture, we have implemented generic >>> RTC drivers that can be used to compensate the system suspend time. So >>> we can remove the obsolete read_persistent_clock(). >>> >>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >> >> I'm not sure about this one: it's still possible to turn off >> CONFIG_RTC_DRV_GENERIC >> on M68KCLASSIC, and in that case we still need a read_persistent_clock64() >> implementation. Or we use your patch but make CONFIG_RTC_DRV_GENERIC >> mandatory. > > Typically (in the defconfigs) CONFIG_RTC_DRV_GENERIC is either "m", > or not set. > > And in both cases, a platform-specific RTC class driver may or may not be > builtin or loaded. We have them for some Amigas (RTC_DRV_MSM6242 or > RTC_DRV_RP5C01). > > I've never been an expert of timekeeping code... Some extra background on this: read_persistent_clock64/read_persistent_clock is primarily needed when you have a real time source that is better than the one exposed in the RTC class driver. For platforms doing suspend/resume, the timekeeping code will first try calling read_persistent_clock64() but fall back to the RTC if that fails. On only a few architectures like m68k, read_persistent_clock64() falls back to reading the RTC hardware, which means it will still work even if the RTC_DRV_GENERIC driver is not loaded. Removing that code will still work correctly as long as the generic RTC driver does get loaded before suspend. On platforms without suspend/resume, none of this matters. The other user of read_persistent_clock() is to set the initial time at boot. This again can be done by the RTC subsystem if the correct RTC driver is built-in and CONFIG_RTC_HCTOSYS is set. Alexandre is planning to remove that option and instead force early user space to load the RTC driver and then sync it manually using the sysfs knob for it. If you have ancient user space that doesn't do this, you might still rely on read_persistent_clock() to set the boot time. > Should we get rid of ARCH_USES_GETTIMEOFFSET? > it seems m68k and two ARM platforms are the last users. > What needs to be done? This is entirely unrelated, but generally speaking it would be great to convert m68k away from ARCH_USES_GETTIMEOFFSET, yes. The first step would be to get rid of all the dummy gettimeoffset() functions that return a constant (like dn_gettimeoffset()). The larger part of that effort would be to turn each clock implementation in m68k into a real clocksource driver. This is probably straightforward, but I don't know the details (adding LinusW to Cc, he's done this on some ARM hardware in the past). >> See below for a version I did a while ago (but never submitted as I got >> distracted). > > Thanks, I can apply it, if it is deemed correct ;-) Please apply Finn's bugfix for the month-off bug first, that one is more urgent. I've rebased my patch on top of his and resent that one. Arnd
Hi Arnd, On Mon, Apr 23, 2018 at 11:28 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Mon, Apr 23, 2018 at 11:07 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> On Fri, Apr 20, 2018 at 5:22 PM, Arnd Bergmann <arnd@arndb.de> wrote: >>> On Thu, Apr 19, 2018 at 8:22 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >>>> The read_persistent_clock() uses a timespec, which is not year 2038 safe >>>> on 32bit systems. Moreover on m68k architecture, we have implemented generic >>>> RTC drivers that can be used to compensate the system suspend time. So >>>> we can remove the obsolete read_persistent_clock(). >>>> >>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >>> >>> I'm not sure about this one: it's still possible to turn off >>> CONFIG_RTC_DRV_GENERIC >>> on M68KCLASSIC, and in that case we still need a read_persistent_clock64() >>> implementation. Or we use your patch but make CONFIG_RTC_DRV_GENERIC >>> mandatory. >> >> Typically (in the defconfigs) CONFIG_RTC_DRV_GENERIC is either "m", >> or not set. >> >> And in both cases, a platform-specific RTC class driver may or may not be >> builtin or loaded. We have them for some Amigas (RTC_DRV_MSM6242 or >> RTC_DRV_RP5C01). >> >> I've never been an expert of timekeeping code... > > Some extra background on this: > > read_persistent_clock64/read_persistent_clock is primarily needed when you > have a real time source that is better than the one exposed in the RTC class > driver. For platforms doing suspend/resume, the timekeeping code will > first try calling read_persistent_clock64() but fall back to the RTC > if that fails. > On only a few architectures like m68k, read_persistent_clock64() falls > back to reading the RTC hardware, which means it will still work even if > the RTC_DRV_GENERIC driver is not loaded. Removing that code will > still work correctly as long as the generic RTC driver does get loaded > before suspend. On platforms without suspend/resume, none of this matters. M68k-with-MMU[*] does not support suspend/resume. [*] Probably the PM dependency should be updated for Coldfire with MMU? > The other user of read_persistent_clock() is to set the initial time at boot. > This again can be done by the RTC subsystem if the correct RTC driver > is built-in and CONFIG_RTC_HCTOSYS is set. Alexandre is planning > to remove that option and instead force early user space to load the > RTC driver and then sync it manually using the sysfs knob for it. > > If you have ancient user space that doesn't do this, you might still > rely on read_persistent_clock() to set the boot time. Yeah, we have some ancient userspace (old ramdisk from just after the a.out to ELF switch ;-), which worked fine last time I tried ;-) But this should not be considered a dependency, as most people will run e.g. Debian/ports, which I assume is a modern userspace. >> Should we get rid of ARCH_USES_GETTIMEOFFSET? >> it seems m68k and two ARM platforms are the last users. >> What needs to be done? > > This is entirely unrelated, but generally speaking it would be > great to convert m68k away from ARCH_USES_GETTIMEOFFSET, > yes. > > The first step would be to get rid of all the dummy gettimeoffset() functions > that return a constant (like dn_gettimeoffset()). > > The larger part of that effort would be to turn each clock implementation > in m68k into a real clocksource driver. This is probably straightforward, > but I don't know the details (adding LinusW to Cc, he's done this on > some ARM hardware in the past). OK, more work to do... >>> See below for a version I did a while ago (but never submitted as I got >>> distracted). >> >> Thanks, I can apply it, if it is deemed correct ;-) > > Please apply Finn's bugfix for the month-off bug first, that one is more > urgent. I've rebased my patch on top of his and resent that one. Sure. Will do. Thanks! Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 23/04/2018 11:28:38+0200, Arnd Bergmann wrote: > Some extra background on this: > > read_persistent_clock64/read_persistent_clock is primarily needed when you > have a real time source that is better than the one exposed in the RTC class > driver. For platforms doing suspend/resume, the timekeeping code will > first try calling read_persistent_clock64() but fall back to the RTC > if that fails. > On only a few architectures like m68k, read_persistent_clock64() falls > back to reading the RTC hardware, which means it will still work even if > the RTC_DRV_GENERIC driver is not loaded. Removing that code will > still work correctly as long as the generic RTC driver does get loaded > before suspend. On platforms without suspend/resume, none of this matters. > > The other user of read_persistent_clock() is to set the initial time at boot. > This again can be done by the RTC subsystem if the correct RTC driver > is built-in and CONFIG_RTC_HCTOSYS is set. Alexandre is planning > to remove that option and instead force early user space to load the > RTC driver and then sync it manually using the sysfs knob for it. > There is still one use case that could require CONFIG_RTC_HCTOSYS, that's when the rootfs is on a network fs that requires the time to be roughly set before being accessed. This could be solved with an initramfs though (and that initramfs would be required if the RTC driver is a module anyway). I still need to check the 'avoid unnecessary fsck runs at boot time' claim but having the kernel modules and fsck on separate FS seems to be a very very specific use case to me. But yes, my plan is at least to get distros to stop enabling it by default on all their kernels because this led to multiple reports of systemd entering a loop and this not so nice workaround (still way better than having each driver have its own version though): https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/rtc/hctosys.c?id=b3a5ac42ab18b7d1a8f2f072ca0ee76a3b754a43 > If you have ancient user space that doesn't do this, you might still > rely on read_persistent_clock() to set the boot time. > Last time I checked a default debian install on x86 would set the system time 2 times from the kernel and then 2 times from userspace. I'm pretty sure we could do without read_persistent_clock and CONFIG_RTC_HCTOSYS and let userspace handle system time. For example, on ARM, only omap and tegra are defining a read_persistent_clock callback. All the other platforms are just working fine without it. As you pointed out on IRC a while ago, s390 seems to be the only user where this is actually useful. -- Alexandre Belloni, Bootlin (formerly Free Electrons) Embedded Linux and Kernel engineering https://bootlin.com
Hi Geert, On 23 April 2018 at 17:07, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Baolin, > > On Mon, Apr 23, 2018 at 4:08 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >> On 20 April 2018 at 23:22, Arnd Bergmann <arnd@arndb.de> wrote: >>> On Thu, Apr 19, 2018 at 8:22 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >>>> The read_persistent_clock() uses a timespec, which is not year 2038 safe >>>> on 32bit systems. Moreover on m68k architecture, we have implemented generic >>>> RTC drivers that can be used to compensate the system suspend time. So >>>> we can remove the obsolete read_persistent_clock(). >>>> >>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >>> >>> I'm not sure about this one: it's still possible to turn off >>> CONFIG_RTC_DRV_GENERIC >>> on M68KCLASSIC, and in that case we still need a read_persistent_clock64() >>> implementation. Or we use your patch but make CONFIG_RTC_DRV_GENERIC >>> mandatory. >> >> OK. Thanks for fixing the issue with your patch. > > Can I consider that an Acked-by? I will add one reviewed-by tag in Arnd's new patch. -- Baolin.wang Best Regards
On Mon, Apr 23, 2018 at 11:47 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Hi Arnd, > > On Mon, Apr 23, 2018 at 11:28 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Mon, Apr 23, 2018 at 11:07 AM, Geert Uytterhoeven >> <geert@linux-m68k.org> wrote: >>> On Fri, Apr 20, 2018 at 5:22 PM, Arnd Bergmann <arnd@arndb.de> wrote: >>>> On Thu, Apr 19, 2018 at 8:22 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >>>>> The read_persistent_clock() uses a timespec, which is not year 2038 safe >>>>> on 32bit systems. Moreover on m68k architecture, we have implemented generic >>>>> RTC drivers that can be used to compensate the system suspend time. So >>>>> we can remove the obsolete read_persistent_clock(). >>>>> >>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >>>> >>>> I'm not sure about this one: it's still possible to turn off >>>> CONFIG_RTC_DRV_GENERIC >>>> on M68KCLASSIC, and in that case we still need a read_persistent_clock64() >>>> implementation. Or we use your patch but make CONFIG_RTC_DRV_GENERIC >>>> mandatory. >>> >>> Typically (in the defconfigs) CONFIG_RTC_DRV_GENERIC is either "m", >>> or not set. >>> >>> And in both cases, a platform-specific RTC class driver may or may not be >>> builtin or loaded. We have them for some Amigas (RTC_DRV_MSM6242 or >>> RTC_DRV_RP5C01). >>> >>> I've never been an expert of timekeeping code... >> >> Some extra background on this: >> >> read_persistent_clock64/read_persistent_clock is primarily needed when you >> have a real time source that is better than the one exposed in the RTC class >> driver. For platforms doing suspend/resume, the timekeeping code will >> first try calling read_persistent_clock64() but fall back to the RTC >> if that fails. >> On only a few architectures like m68k, read_persistent_clock64() falls >> back to reading the RTC hardware, which means it will still work even if >> the RTC_DRV_GENERIC driver is not loaded. Removing that code will >> still work correctly as long as the generic RTC driver does get loaded >> before suspend. On platforms without suspend/resume, none of this matters. > > M68k-with-MMU[*] does not support suspend/resume. > > [*] Probably the PM dependency should be updated for Coldfire with MMU? Ok, so for the suspend case on m68k, can can totally ignore read_persistent_clock64(): classic doesn't have suspend and coldfire doesn't implement mach_hwclock() callbacks, right? For reference, this is the set of machines implementing mach_hwclk: arch/m68k/68000/m68328.c: mach_hwclk = m68328_hwclk; arch/m68k/68000/m68EZ328.c: mach_hwclk = m68328_hwclk; arch/m68k/68000/m68VZ328.c: mach_hwclk = m68328_hwclk; arch/m68k/apollo/config.c: mach_hwclk = dn_dummy_hwclk; /* */ arch/m68k/atari/config.c: mach_hwclk = atari_tt_hwclk; arch/m68k/atari/config.c: mach_hwclk = atari_mste_hwclk; arch/m68k/bvme6000/config.c: mach_hwclk = bvme6000_hwclk; arch/m68k/hp300/config.c: mach_hwclk = hp300_hwclk; arch/m68k/mac/config.c: mach_hwclk = mac_hwclk; arch/m68k/mvme147/config.c: mach_hwclk = mvme147_hwclk; arch/m68k/mvme16x/config.c: mach_hwclk = mvme16x_hwclk; arch/m68k/q40/config.c: mach_hwclk = q40_hwclk; arch/m68k/sun3/config.c: mach_hwclk = sun3_hwclk; arch/m68k/sun3x/config.c: mach_hwclk = sun3x_hwclk; >> The other user of read_persistent_clock() is to set the initial time at boot. >> This again can be done by the RTC subsystem if the correct RTC driver >> is built-in and CONFIG_RTC_HCTOSYS is set. Alexandre is planning >> to remove that option and instead force early user space to load the >> RTC driver and then sync it manually using the sysfs knob for it. >> >> If you have ancient user space that doesn't do this, you might still >> rely on read_persistent_clock() to set the boot time. > > Yeah, we have some ancient userspace (old ramdisk from just after the > a.out to ELF switch ;-), which worked fine last time I tried ;-) > > But this should not be considered a dependency, as most people will > run e.g. Debian/ports, which I assume is a modern userspace. Ok. In that case, a possible cleanup for the old time handling could be to move each of the hwclk implementations over to use their own RTC driver instead of a mach_hwclk function with generic_rtc. It seems that the mach_set_ss and nach_set_mmss callbacks can simply get removed already, they are never called. Similarly, the mach_get_rtc_pll() and mach_get_rtc_pll() callbacks are only used on q40, and could be removed after changing the q40 over to using its own RTC driver, or registering the ioctl operation itself. Arnd
On 23/04/18 21:47, Arnd Bergmann wrote: > On Mon, Apr 23, 2018 at 11:47 AM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: >> Hi Arnd, >> >> On Mon, Apr 23, 2018 at 11:28 AM, Arnd Bergmann <arnd@arndb.de> wrote: >>> On Mon, Apr 23, 2018 at 11:07 AM, Geert Uytterhoeven >>> <geert@linux-m68k.org> wrote: >>>> On Fri, Apr 20, 2018 at 5:22 PM, Arnd Bergmann <arnd@arndb.de> wrote: >>>>> On Thu, Apr 19, 2018 at 8:22 AM, Baolin Wang <baolin.wang@linaro.org> wrote: >>>>>> The read_persistent_clock() uses a timespec, which is not year 2038 safe >>>>>> on 32bit systems. Moreover on m68k architecture, we have implemented generic >>>>>> RTC drivers that can be used to compensate the system suspend time. So >>>>>> we can remove the obsolete read_persistent_clock(). >>>>>> >>>>>> Signed-off-by: Baolin Wang <baolin.wang@linaro.org> >>>>> >>>>> I'm not sure about this one: it's still possible to turn off >>>>> CONFIG_RTC_DRV_GENERIC >>>>> on M68KCLASSIC, and in that case we still need a read_persistent_clock64() >>>>> implementation. Or we use your patch but make CONFIG_RTC_DRV_GENERIC >>>>> mandatory. >>>> >>>> Typically (in the defconfigs) CONFIG_RTC_DRV_GENERIC is either "m", >>>> or not set. >>>> >>>> And in both cases, a platform-specific RTC class driver may or may not be >>>> builtin or loaded. We have them for some Amigas (RTC_DRV_MSM6242 or >>>> RTC_DRV_RP5C01). >>>> >>>> I've never been an expert of timekeeping code... >>> >>> Some extra background on this: >>> >>> read_persistent_clock64/read_persistent_clock is primarily needed when you >>> have a real time source that is better than the one exposed in the RTC class >>> driver. For platforms doing suspend/resume, the timekeeping code will >>> first try calling read_persistent_clock64() but fall back to the RTC >>> if that fails. >>> On only a few architectures like m68k, read_persistent_clock64() falls >>> back to reading the RTC hardware, which means it will still work even if >>> the RTC_DRV_GENERIC driver is not loaded. Removing that code will >>> still work correctly as long as the generic RTC driver does get loaded >>> before suspend. On platforms without suspend/resume, none of this matters. >> >> M68k-with-MMU[*] does not support suspend/resume. >> >> [*] Probably the PM dependency should be updated for Coldfire with MMU? > > Ok, so for the suspend case on m68k, can can totally ignore > read_persistent_clock64(): classic doesn't have suspend and > coldfire doesn't implement mach_hwclock() callbacks, right? Yep, that is right, no ColdFire platforms use it. Regards Greg > For reference, this is the set of machines implementing mach_hwclk: > > arch/m68k/68000/m68328.c: mach_hwclk = m68328_hwclk; > arch/m68k/68000/m68EZ328.c: mach_hwclk = m68328_hwclk; > arch/m68k/68000/m68VZ328.c: mach_hwclk = m68328_hwclk; > arch/m68k/apollo/config.c: mach_hwclk = dn_dummy_hwclk; /* */ > arch/m68k/atari/config.c: mach_hwclk = atari_tt_hwclk; > arch/m68k/atari/config.c: mach_hwclk = atari_mste_hwclk; > arch/m68k/bvme6000/config.c: mach_hwclk = bvme6000_hwclk; > arch/m68k/hp300/config.c: mach_hwclk = hp300_hwclk; > arch/m68k/mac/config.c: mach_hwclk = mac_hwclk; > arch/m68k/mvme147/config.c: mach_hwclk = mvme147_hwclk; > arch/m68k/mvme16x/config.c: mach_hwclk = mvme16x_hwclk; > arch/m68k/q40/config.c: mach_hwclk = q40_hwclk; > arch/m68k/sun3/config.c: mach_hwclk = sun3_hwclk; > arch/m68k/sun3x/config.c: mach_hwclk = sun3x_hwclk; > >>> The other user of read_persistent_clock() is to set the initial time at boot. >>> This again can be done by the RTC subsystem if the correct RTC driver >>> is built-in and CONFIG_RTC_HCTOSYS is set. Alexandre is planning >>> to remove that option and instead force early user space to load the >>> RTC driver and then sync it manually using the sysfs knob for it. >>> >>> If you have ancient user space that doesn't do this, you might still >>> rely on read_persistent_clock() to set the boot time. >> >> Yeah, we have some ancient userspace (old ramdisk from just after the >> a.out to ELF switch ;-), which worked fine last time I tried ;-) >> >> But this should not be considered a dependency, as most people will >> run e.g. Debian/ports, which I assume is a modern userspace. > > Ok. In that case, a possible cleanup for the old time handling > could be to move each of the hwclk implementations over to use > their own RTC driver instead of a mach_hwclk function with > generic_rtc. > > It seems that the mach_set_ss and nach_set_mmss > callbacks can simply get removed already, they are > never called. Similarly, the mach_get_rtc_pll() and > mach_get_rtc_pll() callbacks are only used on q40, and > could be removed after changing the q40 over to using > its own RTC driver, or registering the ioctl operation itself. > > Arnd > -- > To unsubscribe from this list: send the line "unsubscribe linux-m68k" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
diff --git a/arch/m68k/kernel/time.c b/arch/m68k/kernel/time.c index 97dd4e2..cb386d8 100644 --- a/arch/m68k/kernel/time.c +++ b/arch/m68k/kernel/time.c @@ -71,22 +71,6 @@ static irqreturn_t timer_interrupt(int irq, void *dummy) return IRQ_HANDLED; } -void read_persistent_clock(struct timespec *ts) -{ - struct rtc_time time; - ts->tv_sec = 0; - ts->tv_nsec = 0; - - if (mach_hwclk) { - mach_hwclk(0, &time); - - if ((time.tm_year += 1900) < 1970) - time.tm_year += 100; - ts->tv_sec = mktime(time.tm_year, time.tm_mon, time.tm_mday, - time.tm_hour, time.tm_min, time.tm_sec); - } -} - #if defined(CONFIG_ARCH_USES_GETTIMEOFFSET) && IS_ENABLED(CONFIG_RTC_DRV_GENERIC) static int rtc_generic_get_time(struct device *dev, struct rtc_time *tm) {
The read_persistent_clock() uses a timespec, which is not year 2038 safe on 32bit systems. Moreover on m68k architecture, we have implemented generic RTC drivers that can be used to compensate the system suspend time. So we can remove the obsolete read_persistent_clock(). Signed-off-by: Baolin Wang <baolin.wang@linaro.org> --- arch/m68k/kernel/time.c | 16 ---------------- 1 file changed, 16 deletions(-) -- 1.7.9.5