Re: [PATCH 2/3] [v2] m68k: mac: use time64_t in RTC handling

[Arnd Bergmann]

On Wed, Jul 18, 2018 at 2:02 PM, Finn Thain <fthain@xxxxxxxxxxxxxxxxxxx> wrote:
> On Wed, 18 Jul 2018, Geert Uytterhoeven wrote:
>
>>
>> Thanks for your patch!
>>
>> Applied and queued for v4.19, with the WARN_ON() dropped.
>>
>
> The patch you've committed to your for-v4.19 branch has this hunk:
>
> @@ -269,8 +275,12 @@ static long via_read_time(void)
>                 via_pram_command(0x89, &result.cdata[1]);
>                 via_pram_command(0x8D, &result.cdata[0]);
>
> -               if (result.idata == last_result.idata)
> +               if (result.idata == last_result.idata) {
> +                       if (result.idata < RTC_OFFSET)
> +                               result.idata += 0x100000000ull;
> +
>                         return result.idata - RTC_OFFSET;
> +               }
>
>                 if (++count > 10)
>                         break;
>
> That looks bogus to me, since result.idata is a long.



> Also, the following hunk seems a bit pointless (?)
>
> @@ -291,11 +301,11 @@ static long via_read_time(void)
>   * is basically any machine with Mac II-style ADB.
>   */
>
> -static void via_write_time(long time)
> +static void via_write_time(time64_t time)
>  {
>         union {
>                 __u8 cdata[4];
> -               long idata;
> +               __u32 idata;
>         } data;
>         __u8 temp;
>
>
> But if data.idata needs to be changed to __u32 here, why not change the
> same struct member in via_read_time() also?

Hmm, apparently I forgot to update via_read_time(), that one
is indeed bogus and now inconsistent with the other functions.

The change in via_write_time() seems at least consistent wtih
what we do elsewhere, and using __u32 makes this code
more portable. (yes, I realize that 64-bit powermac doesn't
use the VIA RTC, but it feels better to write code portably
anyway).

I'd suggest we do it like below to make it consistent with the
rest again, using the 1904..2040 range of dates and no warning
for invalid dates.

If you agree, I'll send that as a proper patch.

       Arnd

diff --git a/arch/m68k/mac/misc.c b/arch/m68k/mac/misc.c
index bf8df47a6d09..8335509969f1 100644
--- a/arch/m68k/mac/misc.c
+++ b/arch/m68k/mac/misc.c
@@ -255,12 +255,13 @@ static void via_write_pram(int offset, __u8 data)
  * is basically any machine with Mac II-style ADB.
  */

-static long via_read_time(void)
+static time64_t via_read_time(void)
 {
        union {
                __u8 cdata[4];
-               long idata;
+               __u32 idata;
        } result, last_result;
+       time64_t ret;
        int count = 1;

        via_pram_command(0x81, &last_result.cdata[3]);
@@ -279,12 +280,8 @@ static long via_read_time(void)
                via_pram_command(0x89, &result.cdata[1]);
                via_pram_command(0x8D, &result.cdata[0]);

-               if (result.idata == last_result.idata) {
-                       if (result.idata < RTC_OFFSET)
-                               result.idata += 0x100000000ull;
-
-                       return result.idata - RTC_OFFSET;
-               }
+               if (result.idata == last_result.idata)
+                       return (time64_t(result.idata) - RTC_OFFSET);

                if (++count > 10)
                        break;