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;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Jason Gunthorpe <jgg@ziepe.ca>
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
In-Reply-To: <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> <20201202162723.GJ5487@ziepe.ca>
Date:   Wed, 02 Dec 2020 20:21:00 +0100
Message-ID: <87a6uwdnfn.fsf@nanos.tec.linutronix.de>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Wed, Dec 02 2020 at 12:27, Jason Gunthorpe wrote:
> On Wed, Dec 02, 2020 at 02:44:53PM +0100, Thomas Gleixner wrote:
>>  	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.

This has two purposes:

     1) Initiating the update on boot once ntp is synced.

     2) Reinitiating the sync after ntp lost sync and the work did not
        reschedule itself because it observed !ntp_synced().

In both cases it's highly unlikely that the write actually happens when
the work is queued because do_adjtimex() would have to be exactly around
the valid update window. So it will not write immediately. It will run
through at least one retry.

I don't think the timer should be canceled if the ntp_synced() state did
not change. Otherwise every do_adtimex() call will cancel/restart
it, which does not make sense. Lemme stare at it some more.

> 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

Because nobody had the stomach and/or cycles to touch it :)

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

Unfortunately that code path is not that dead on x86. You need to fix
all the (ab)users first. :)

Thanks,

        tglx