linux-kernel - Re: [patch 26/29] x86/hpet: Consolidate clockevent functions

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 26 Jun 2019 23:17:53 +0200
From:   Ingo Molnar <mingo@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     LKML <linux-kernel@...r.kernel.org>, x86@...nel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Ricardo Neri <ricardo.neri-calderon@...ux.intel.com>,
        Ashok Raj <ashok.raj@...el.com>,
        Andi Kleen <andi.kleen@...el.com>,
        Suravee Suthikulpanit <Suravee.Suthikulpanit@....com>,
        Stephane Eranian <eranian@...gle.com>,
        Ravi Shankar <ravi.v.shankar@...el.com>
Subject: Re: [patch 26/29] x86/hpet: Consolidate clockevent functions


* Thomas Gleixner <tglx@...utronix.de> wrote:

> @@ -440,11 +419,11 @@ static struct hpet_channel hpet_channel0
>  		.name			= "hpet",
>  		.features		= CLOCK_EVT_FEAT_PERIODIC |
>  					  CLOCK_EVT_FEAT_ONESHOT,
> -		.set_state_periodic	= hpet_legacy_set_periodic,
> -		.set_state_oneshot	= hpet_legacy_set_oneshot,
> -		.set_state_shutdown	= hpet_legacy_shutdown,
> -		.tick_resume		= hpet_legacy_resume,
> -		.set_next_event		= hpet_legacy_next_event,
> +		.set_state_periodic	= hpet_clkevt_set_periodic,
> +		.set_state_oneshot	= hpet_clkevt_set_oneshot,
> +		.set_state_shutdown	= hpet_clkevt_shutdown,
> +		.tick_resume		= hpet_clkevt_legacy_resume,
> +		.set_next_event		= hpet_clkevt_set_next_event,
>  		.irq			= 0,
>  		.rating			= 50,

> -	evt->set_state_shutdown = hpet_msi_shutdown;
> -	evt->set_state_oneshot = hpet_msi_set_oneshot;
> -	evt->tick_resume = hpet_msi_resume;
> -	evt->set_next_event = hpet_msi_next_event;
> +	evt->set_state_shutdown = hpet_clkevt_shutdown;
> +	evt->set_state_oneshot = hpet_clkevt_set_oneshot;
> +	evt->set_next_event = hpet_clkevt_set_next_event;
> +	evt->tick_resume = hpet_clkevt_msi_resume;
>  	evt->cpumask = cpumask_of(hc->cpu);

My compulsive-obsessive half really wants this to look like:

> +	evt->set_state_shutdown	= hpet_clkevt_shutdown;
> +	evt->set_state_oneshot	= hpet_clkevt_set_oneshot;
> +	evt->set_next_event	= hpet_clkevt_set_next_event;
> +	evt->tick_resume	= hpet_clkevt_msi_resume;
>  	evt->cpumask		= cpumask_of(hc->cpu);

:-)

Also, maybe harmonize the callback names with the local function names, 
like hpet_clkevt_set_next_event() already does and 
hpet_clkevt_set_oneshot() almost does:

 s/hpet_clkevt_shutdown
  /hpet_clkevt_set_state_shutdown

 s/hpet_clkevt_set_oneshot
  /hpet_clkevt_set_state_oneshot

 s/hpet_clkevt_msi_resume
  /hpet_clkevt_tick_resume

... unless the name variations have some hidden purpose and meaning?

With that fixed:

Reviewed-by: Ingo Molnar <mingo@...nel.org>

Thanks,

	Ingo