From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Zyngier Subject: Re: [PATCH] clocksource: Fix arm_arch_timer clockmode when vDSO disabled Date: Fri, 21 Feb 2020 15:28:56 +0000 Message-ID: <6df28d31cf6d4dd6109415fbd73a9c48@kernel.org> References: <20200221130355.21373-1-vincenzo.frascino@arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mail.kernel.org ([198.145.29.99]:54058 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727053AbgBUP3A (ORCPT ); Fri, 21 Feb 2020 10:29:00 -0500 In-Reply-To: Sender: linux-arch-owner@vger.kernel.org List-ID: To: Vincenzo Frascino Cc: linux-arch@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, catalin.marinas@arm.com, will.deacon@arm.com, linux@armlinux.org.uk, luto@kernel.org, tglx@linutronix.de, m.szyprowski@samsung.com, mark.rutland@arm.com On 2020-02-21 14:48, Vincenzo Frascino wrote: > Hi Marc, > > On 21/02/2020 13:34, Marc Zyngier wrote: >> Vincenzo, >> >> Please include Mark and myself for anything that touches the arch >> timers >> (get_maintainer.pl will tell you who you need to cc). >> > > Sorry about that, I posted it too quickly without the proper Cc on the > patch. > >> On 2020-02-21 13:03, Vincenzo Frascino wrote: > > [...] > >> >> This feels pretty clunky. >> >> I'd extect VDSO_ARCH_CLOCKMODES (or some similar architecture-specific >> symbol) to be used for vdso_default, and that symbol to be defined as >> VDSO_CLOCKMODE_NONE when CONFIG_GENERIC_GETTIMEOFDAY isn't selected. >> > > My understanding is that currently VDSO_ARCH_CLOCKMODES depending on > the > architecture can identify one or more clocks. In the case of arm and > the > arm_arch_timer the arch specific symbol is VDSO_CLOCKMODE_ARCHTIMER > (used for > vdso_default), which as you are correctly stating has to be defined as > VDSO_CLOCKMODE_NONE when CONFIG_GENERIC_GETTIMEOFDAY isn't selected. This isn't what I'm saying. What I'm suggesting here is that there is possibly a missing indirection, which defaults to ARCH_TIMER when the VDSO is selected, and NONE when it isn't. Overloading a known symbol feels like papering over the issue. Ideally, this default symbol would be provided by asm/clocksource.h, but that may not even be the right thing to do. >> Otherwise, you'll end-up replicating the same pattern in every >> clock-source that gets used by the VDSO. > > Based on my investigation this fix should be replicated for all the > clocksources > used by architectures supported by Unified VDSO and of which VDSOs can > be > disabled (otherwise the current solution works). After a quick grep on > the > kernel tree: > > $ grep -nr "config VDSO" * > > arch/arm/mm/Kconfig:895:config VDSO > > Since the only clocksource that falls into these conditions seems to be > arm_arch_timer I modified its driver. Fair enough. But don't override the symbol locally. Create a new one: diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index ee2420d56f67..7eb3db75211d 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -69,7 +69,12 @@ static enum arch_timer_ppi_nr arch_timer_uses_ppi = ARCH_TIMER_VIRT_PPI; static bool arch_timer_c3stop; static bool arch_timer_mem_use_virtual; static bool arch_counter_suspend_stop; -static enum vdso_clock_mode vdso_default = VDSO_CLOCKMODE_ARCHTIMER; +#ifdef CONFIG_GENERIC_GETTIMEOFDAY +#define __VDSO_DEFAULT VDSO_CLOCKMODE_ARCHTIMER +#else +#define __VDSO_DEFAULT VDSO_CLOCKMODE_NONE +#endif +static enum vdso_clock_mode vdso_default = __VDSO_DEFAULT; static cpumask_t evtstrm_available = CPU_MASK_NONE; static bool evtstrm_enable = IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM); Or even this (no, I'm not suggesting this seriously): diff --git a/drivers/clocksource/arm_arch_timer.c b/drivers/clocksource/arm_arch_timer.c index ee2420d56f67..836b500d1bf1 100644 --- a/drivers/clocksource/arm_arch_timer.c +++ b/drivers/clocksource/arm_arch_timer.c @@ -69,7 +69,7 @@ static enum arch_timer_ppi_nr arch_timer_uses_ppi = ARCH_TIMER_VIRT_PPI; static bool arch_timer_c3stop; static bool arch_timer_mem_use_virtual; static bool arch_counter_suspend_stop; -static enum vdso_clock_mode vdso_default = VDSO_CLOCKMODE_ARCHTIMER; +static enum vdso_clock_mode vdso_default = VDSO_CLOCKMODE_MAX - 1; static cpumask_t evtstrm_available = CPU_MASK_NONE; static bool evtstrm_enable = IS_ENABLED(CONFIG_ARM_ARCH_TIMER_EVTSTREAM); M. -- Jazz is not dead. It just smells funny...