powerpc: use time64_t in read_persistent_clock

Arnd,

In 5bfd643583b2e I can see that you changed:

$ git show 5bfd643583b2e -- arch/powerpc/platforms/powermac/time.c
[...]
 #ifdef CONFIG_ADB_PMU
-static unsigned long pmu_get_time(void)
+static time64_t pmu_get_time(void)
 {
        struct adb_request req;
-       unsigned int now;
+       time64_t now;

        if (pmu_request(&req, NULL, 1, PMU_READ_RTC) < 0)
                return 0;

@@ -160,10 +151,10 @@ static unsigned long pmu_get_time(void)
                       req.reply_len);
        now = (req.reply[0] << 24) + (req.reply[1] << 16)
                + (req.reply[2] << 8) + req.reply[3];
-       return ((unsigned long)now) - RTC_OFFSET;
+       return now - RTC_OFFSET;
 }

[...]

As far as I can tell the old function would never return a negative
value and rely on unsigned long roll over. Now I am seeing negative
value being returned and it seems to break later on in the rtc
library.

Could you comment why you removed the cast to (unsigned long) in your commit ?

Thanks

On Wed, Jun 13, 2018 at 10:24 PM, Mathieu Malaterre [off-list ref] wrote:

I'm sorry my patch caused a regression. Even with your bug report
it took me a while to see what exactly went wrong, and how I got
mixed up the integer conversion rules.

I mistakenly assume that fiven

       long long now;
       unsigned char reply[4];
       now = reply[0] << 24;

we would always end up with a positive number, but since reply[0] is
promoted to a signed integer, the right-hand side of the assignment
ends up as a negative number for the usual contents of the RTC.

Can you confirm that this patch addresses your problem?

diff --git a/arch/powerpc/platforms/powermac/time.c

b/arch/powerpc/platforms/powermac/time.c
index 7c968e46736f..a7fdcd3051f9 100644

--- a/arch/powerpc/platforms/powermac/time.c
+++ b/arch/powerpc/platforms/powermac/time.c

@@ -97,7 +97,7 @@ static time64_t cuda_get_time(void)
        if (req.reply_len != 7)
                printk(KERN_ERR "cuda_get_time: got %d byte reply\n",
                       req.reply_len);
-       now = (req.reply[3] << 24) + (req.reply[4] << 16)
+       now = (u32)(req.reply[3] << 24) + (req.reply[4] << 16)
                + (req.reply[5] << 8) + req.reply[6];
        return now - RTC_OFFSET;
 }

@@ -140,7 +140,7 @@ static time64_t pmu_get_time(void)
        if (req.reply_len != 4)
                printk(KERN_ERR "pmu_get_time: got %d byte reply from PMU\n",
                       req.reply_len);
-       now = (req.reply[0] << 24) + (req.reply[1] << 16)
+       now = (u32)(req.reply[0] << 24) + (req.reply[1] << 16)
                + (req.reply[2] << 8) + req.reply[3];
        return now - RTC_OFFSET;
 }

On a related note, we do have some freedom in how we want to
interpret values beyond year 2038/2040 when the RTC does wrap
around.

- With the original code, we converted the time into a signed
  32-bit integer based on the 1970 Unix epoch, converting RTC
  years 2038 through 2040 into Linux years 1902 through 1904,
  which doesn't seem very helpful.

- With my fix above, we would interpret the numbers as documented,
  with 2040 wrapping around to 1904.

- If we want, we could reinterpret the 1904-1970 range as later
  times between 2040 and 2106, like this:

--- a/arch/powerpc/platforms/powermac/time.c
+++ b/arch/powerpc/platforms/powermac/time.c

@@ -88,7 +88,7 @@ long __init pmac_time_init(void)
 static time64_t cuda_get_time(void)
 {
        struct adb_request req;
-       time64_t now;
+       u32 now;

        if (cuda_request(&req, NULL, 2, CUDA_PACKET, CUDA_GET_TIME) < 0)
                return 0;

@@ -132,7 +132,7 @@ static int cuda_set_rtc_time(struct rtc_time *tm)
 static time64_t pmu_get_time(void)
 {
        struct adb_request req;
-       time64_t now;
+       u32 now;

        if (pmu_request(&req, NULL, 1, PMU_READ_RTC) < 0)
                return 0;

On the upside, this would possibly extend the life of some machines
by another 66 years, on the downside it might cause issues when
an empty RTC battery causes the initial system time to be
above the 32-bit TIME_T_MAX (instead of going back to 1904).

      Arnd

On Thu, Jun 14, 2018 at 1:46 PM Arnd Bergmann [off-list ref] wrote:

On Wed, Jun 13, 2018 at 10:24 PM, Mathieu Malaterre [off-list ref] wrote:

quoted

Arnd,

In 5bfd643583b2e I can see that you changed:

$ git show 5bfd643583b2e -- arch/powerpc/platforms/powermac/time.c
[...]
 #ifdef CONFIG_ADB_PMU
-static unsigned long pmu_get_time(void)
+static time64_t pmu_get_time(void)
 {
        struct adb_request req;
-       unsigned int now;
+       time64_t now;

        if (pmu_request(&req, NULL, 1, PMU_READ_RTC) < 0)
                return 0;

@@ -160,10 +151,10 @@ static unsigned long pmu_get_time(void)
                       req.reply_len);
        now = (req.reply[0] << 24) + (req.reply[1] << 16)
                + (req.reply[2] << 8) + req.reply[3];
-       return ((unsigned long)now) - RTC_OFFSET;
+       return now - RTC_OFFSET;
 }

[...]

As far as I can tell the old function would never return a negative
value and rely on unsigned long roll over. Now I am seeing negative
value being returned and it seems to break later on in the rtc
library.

Could you comment why you removed the cast to (unsigned long) in your commit ?

I'm sorry my patch caused a regression. Even with your bug report
it took me a while to see what exactly went wrong, and how I got
mixed up the integer conversion rules.

I mistakenly assume that fiven

       long long now;
       unsigned char reply[4];
       now = reply[0] << 24;

we would always end up with a positive number, but since reply[0] is
promoted to a signed integer, the right-hand side of the assignment
ends up as a negative number for the usual contents of the RTC.

Ah right. My diagnosis was invalid.

Can you confirm that this patch addresses your problem?

Yes !

Before:
[    5.986710] rtc-generic rtc-generic: hctosys: unable to read the
hardware clock

After:
[    5.579611] rtc-generic rtc-generic: setting system clock to
2018-06-14 18:57:00 UTC (1529002620)

So for the above:

Tested-by: Mathieu Malaterre <redacted>

I would submit the first patch and add the above as comment. I doubt
anyone would still have a running system by then.

Thanks

On Thu, Jun 14, 2018 at 9:03 PM, Mathieu Malaterre [off-list ref] wrote:

On Thu, Jun 14, 2018 at 1:46 PM Arnd Bergmann [off-list ref] wrote:

quoted

On Wed, Jun 13, 2018 at 10:24 PM, Mathieu Malaterre [off-list ref] wrote:

quoted

Can you confirm that this patch addresses your problem?

Yes !

Before:
[    5.986710] rtc-generic rtc-generic: hctosys: unable to read the
hardware clock

After:
[    5.579611] rtc-generic rtc-generic: setting system clock to
2018-06-14 18:57:00 UTC (1529002620)

So for the above:

Tested-by: Mathieu Malaterre <redacted>

Thanks a lot!

quoted

--- a/arch/powerpc/platforms/powermac/time.c
+++ b/arch/powerpc/platforms/powermac/time.c

@@ -88,7 +88,7 @@ long __init pmac_time_init(void)
 static time64_t cuda_get_time(void)
 {
        struct adb_request req;
-       time64_t now;
+       u32 now;

        if (cuda_request(&req, NULL, 2, CUDA_PACKET, CUDA_GET_TIME) < 0)
                return 0;

@@ -132,7 +132,7 @@ static int cuda_set_rtc_time(struct rtc_time *tm)
 static time64_t pmu_get_time(void)
 {
        struct adb_request req;
-       time64_t now;
+       u32 now;

        if (pmu_request(&req, NULL, 1, PMU_READ_RTC) < 0)
                return 0;

On the upside, this would possibly extend the life of some machines
by another 66 years, on the downside it might cause issues when
an empty RTC battery causes the initial system time to be
above the 32-bit TIME_T_MAX (instead of going back to 1904).

I would submit the first patch and add the above as comment. I doubt
anyone would still have a running system by then.

I was about to send a similar patch to the first one for m68k macs, which have
basically the same code. My patch had the same bug, so I'm glad I had
not sent it yet, but I also noticed that I made the opposite decision on m68k
as that code had already been using 'unsigned long' in the conversion (but
then of course would wrap it during the conversion to 'time_t). I've picked
the same approach for both now, using the 1904..2040 range.

       Arnd