Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 2 Dec 2020 12:27:23 -0400
From:   Jason Gunthorpe <jgg@ziepe.ca>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Miroslav Lichvar <mlichvar@redhat.com>,
        linux-kernel@vger.kernel.org, John Stultz <john.stultz@linaro.org>,
        Prarit Bhargava <prarit@redhat.com>
Subject: Re: [PATCH] rtc: adapt allowed RTC update error
Message-ID: <20201202162723.GJ5487@ziepe.ca>
References: <20201201143835.2054508-1-mlichvar@redhat.com>
 <20201201161224.GF5487@ziepe.ca>
 <20201201171420.GN1900232@localhost>
 <20201201173540.GH5487@ziepe.ca>
 <87mtywe2zu.fsf@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87mtywe2zu.fsf@nanos.tec.linutronix.de>
Precedence: bulk

On Wed, Dec 02, 2020 at 02:44:53PM +0100, Thomas Gleixner wrote:

> So if this ends up in level #1 then again the chance is pretty low that
> the expiry time is aligned to the the level period. If this works then
> it only works by chance.

Yes, I always thought it was timer wheel related, but at least in my
testing long ago it seemed reliable at the short timeout. Maybe I had
a lucky hz value.
 
> > This seems like a more fundamental problem someplace else in the
> > kernel.
> 
> I don't think so. :)
> 
> Something like the completely untested below should make this reliable
> and only needs to retry when the work is running late (busy machine),
> but the wakeup will be on time or at max 1 jiffie off when high
> resolution timers are not available or disabled.

This seems like the right approach

> @@ -629,7 +618,7 @@ void ntp_notify_cmos_timer(void)
>  
>  	if (IS_ENABLED(CONFIG_GENERIC_CMOS_UPDATE) ||
>  	    IS_ENABLED(CONFIG_RTC_SYSTOHC))
> -		queue_delayed_work(system_power_efficient_wq, &sync_work, 0);
> +		queue_work(system_power_efficient_wq, &sync_work);

As Miroslav noted, probably the right thing to do here is to reset the
hrtimer and remove the sync_work? I think this code was to expedite an
RTC sync when NTP fixes the clock on boot.

IIRC this was made somewhat messy due to the dual path with rtclib and
old cmos.

It would be very nice to kill the cmos path, things are better
now.. But PPC still has a long way to go:

arch/powerpc/platforms/52xx/efika.c:    .set_rtc_time           = rtas_set_rtc_time,
arch/powerpc/platforms/8xx/mpc86xads_setup.c:   .set_rtc_time           = mpc8xx_set_rtc_time,
arch/powerpc/platforms/8xx/tqm8xx_setup.c:      .set_rtc_time           = mpc8xx_set_rtc_time,
arch/powerpc/platforms/cell/setup.c:    .set_rtc_time           = rtas_set_rtc_time,
arch/powerpc/platforms/chrp/setup.c:            ppc_md.set_rtc_time     = rtas_set_rtc_time;
arch/powerpc/platforms/chrp/setup.c:    .set_rtc_time           = chrp_set_rtc_time,
arch/powerpc/platforms/maple/setup.c:   .set_rtc_time           = maple_set_rtc_time,
arch/powerpc/platforms/powermac/setup.c:        .set_rtc_time           = pmac_set_rtc_time,
arch/powerpc/platforms/pseries/setup.c: .set_rtc_time           = rtas_set_rtc_time,

Also x86 needs a touch, it already has RTC lib, no idea why it also
provides this old path too

I wonder if the cmos path could be killed off under the dead HW
principle?

Jason