linux-kernel - Re: [PATCH 1/6] clocksource: sh_cmt: compute rate before registration again

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 13 Feb 2017 20:35:26 -0800
From:   John Stultz <john.stultz@...aro.org>
To:     Nicolai Stange <nicstange@...il.com>,
        Magnus Damm <damm@...nsource.se>,
        Magnus Damm <magnus.damm@...il.com>
Cc:     Daniel Lezcano <daniel.lezcano@...aro.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Yoshinori Sato <ysato@...rs.sourceforge.jp>,
        lkml <linux-kernel@...r.kernel.org>,
        uclinux-h8-devel@...ts.sourceforge.jp
Subject: Re: [PATCH 1/6] clocksource: sh_cmt: compute rate before registration again

On Mon, Feb 6, 2017 at 1:11 PM, Nicolai Stange <nicstange@...il.com> wrote:
> With the upcoming NTP correction related rate adjustments to be implemented
> in the clockevents core, the latter needs to get informed about every rate
> change of a clockevent device made after its registration.
>
> Currently, sh_cmt violates this requirement in that it registers its
> clockevent device with a dummy rate and sets its final ->mult and ->shift
> values from its ->set_state_oneshot() and ->set_state_periodic() functions
> respectively.
>
> This patch moves the setting of the clockevent device's ->mult and ->shift
> values to before its registration.
>
> Note that there has been some back and forth regarding this question with
> respect to the clocksource also provided by this driver:
>   commit f4d7c3565c16 ("clocksource: sh_cmt: compute mult and shift before
>                         registration")
> moves the rate determination from the clocksource's ->enable() function to
> before its registration. OTOH, the later
>   commit 3593f5fe40a1 ("clocksource: sh_cmt: __clocksource_updatefreq_hz()
>                         update")
> basically reverts this, saying
>   "Without this patch the old code uses clocksource_register() together
>    with a hack that assumes a never changing clock rate."
>
> However, I checked all current sh_cmt users in arch/sh as well as in
> arch/arm/mach-shmobile carefully and right now, none of them changes any
> rate in any clock tree relevant to sh_cmt after their respective
> time_init(). Since all sh_cmt instances are created after time_init(), none
> of them should ever observe any clock rate changes.
>
> What's more, both, a clocksource as well as a clockevent device, can
> immediately get selected for use at their registration and thus, enabled
> at this point already. So it's probably safer to assume a "never changing
> clock rate" here.
>
> - Move the struct sh_cmt_channel's ->rate member to struct sh_cmt_device:
>   it's a property of the underlying clock which is in turn specific to
>   the sh_cmt_device.
> - Determine the ->rate value in sh_cmt_setup() at device probing rather
>   than at first usage.
> - Set the clockevent device's ->mult and ->shift values right before its
>   registration.
> - Although not strictly necessary for the upcoming clockevent core changes,
>   set the clocksource's rate at its registration for consistency.

Magnus: Do you have any objection to the above?  I know we reworked
this a few times in the past.

thanks
-john