linux-kernel - Re: [PATCH v7 03/15] clocksource: New RISC-V SBI timer driver

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, 16 Aug 2017 17:23:47 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Palmer Dabbelt <palmer@...belt.com>
cc:     peterz@...radead.org, jason@...edaemon.net, marc.zyngier@....com,
        Arnd Bergmann <arnd@...db.de>, yamada.masahiro@...ionext.com,
        mmarek@...e.com, albert@...ive.com, will.deacon@....com,
        boqun.feng@...il.com, oleg@...hat.com, mingo@...hat.com,
        daniel.lezcano@...aro.org, gregkh@...uxfoundation.org,
        jslaby@...e.com, davem@...emloft.net, mchehab@...nel.org,
        hverkuil@...all.nl, rdunlap@...radead.org, viro@...iv.linux.org.uk,
        mhiramat@...nel.org, fweisbec@...il.com, mcgrof@...nel.org,
        dledford@...hat.com, bart.vanassche@...disk.com,
        sstabellini@...nel.org, mpe@...erman.id.au,
        rmk+kernel@...linux.org.uk, paul.gortmaker@...driver.com,
        nicolas.dichtel@...nd.com, linux@...ck-us.net,
        heiko.carstens@...ibm.com, schwidefsky@...ibm.com,
        geert@...ux-m68k.org, akpm@...ux-foundation.org,
        andriy.shevchenko@...ux.intel.com, jiri@...lanox.com,
        vgupta@...opsys.com, airlied@...hat.com, jk@...abs.org,
        chris@...is-wilson.co.uk, Jason@...c4.com,
        paulmck@...ux.vnet.ibm.com, ncardwell@...gle.com,
        linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org,
        patches@...ups.riscv.org
Subject: Re: [PATCH v7 03/15] clocksource: New RISC-V SBI timer driver

On Mon, 31 Jul 2017, Palmer Dabbelt wrote:

> +void timer_riscv_init(int cpu_id,
> +		      unsigned long riscv_timebase,
> +		      unsigned long long (*rdtime)(struct clocksource *),
> +		      int (*next)(unsigned long, struct clock_event_device*))
> +{
> +	struct clocksource *cs = &per_cpu(clock_source, cpu_id);
> +	struct clock_event_device *ce = &per_cpu(clock_event, cpu_id);

per_cpu_ptr() please

> +	*cs = (struct clocksource) {
> +		.name = "riscv_clocksource",
> +		.rating = 300,
> +		.read = rdtime,
> +		.mask = CLOCKSOURCE_MASK(BITS_PER_LONG),
> +		.flags = CLOCK_SOURCE_IS_CONTINUOUS,
> +	};

Hmm, why do you try to register clocksources per cpu. The core code will
only use the first one registered and the others are just ballast. You
should just have that once at boot time and not on a per cpu basis.

> +	clocksource_register_hz(cs, riscv_timebase);

Clock events are per CPU though.

> +	*ce = (struct clock_event_device){
> +		.name = "riscv_timer_clockevent",
> +		.features = CLOCK_EVT_FEAT_ONESHOT,
> +		.rating = 300,
> +		.cpumask = cpumask_of(cpu_id),
> +		.set_next_event = next,
> +		.set_state_oneshot  = NULL,
> +		.set_state_shutdown = NULL,
> +	};

I'm not a big fan of that. What's wrong with just making it:

static DEFINE_PER_CPU(struct clock_event_device, clock_event) = {
	.name		= "riscv_timer_clockevent",
	.features	= CLOCK_EVT_FEAT_ONESHOT,
	.rating		= 300,
};

and here just do:

	ce->cpumask = cpumask_of(cpu_id);
	ce->set_next_event = next;

> +	clockevents_config_and_register(ce, riscv_timebase, 100, 0x7fffffff);

Hmm?

> +/*
> + * Looks up the clocksource or clock_even_device that cooresponds the given
> + * hart.
> + */
> +struct clocksource *timer_riscv_source(int cpuid);
> +struct clock_event_device *timer_riscv_device(int cpu_id);

What for? You register and forget them ....

Thanks,

	tglx