[PATCH] m68k: Remove read_persistent_clock()

[PATCH] m68k: Remove read_persistent_clock()

[Baolin Wang]

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(-)

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)
 {
-- 
1.7.9.5

Re: [PATCH] m68k: Remove read_persistent_clock()

[Arnd Bergmann]

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)
 {

Re: [PATCH] m68k: Remove read_persistent_clock()

[Baolin Wang]

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

Re: [PATCH] m68k: Remove read_persistent_clock()

[Geert Uytterhoeven]

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

Re: [PATCH] m68k: Remove read_persistent_clock()

[Baolin Wang]

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

Re: [PATCH] m68k: Remove read_persistent_clock()

[Geert Uytterhoeven]

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

Re: [PATCH] m68k: Remove read_persistent_clock()

[Arnd Bergmann]

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

Re: [PATCH] m68k: Remove read_persistent_clock()

[Geert Uytterhoeven]

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

Re: [PATCH] m68k: Remove read_persistent_clock()

[Arnd Bergmann]

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

Re: [PATCH] m68k: Remove read_persistent_clock()

[Greg Ungerer]

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
> 

Re: [PATCH] m68k: Remove read_persistent_clock()

[Alexandre Belloni]

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