On Thu, Jun 14, 2018 at 1:46 PM Arnd Bergmann <a...@arndb.de> wrote:
>
> On Wed, Jun 13, 2018 at 10:24 PM, Mathieu Malaterre <ma...@debian.org> wrote:
> > 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 <ma...@debian.org>
> 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).
I would submit the first patch and add the above as comment. I doubt
anyone would still have a running system by then.
Thanks
