Re: [PATCH 2/2] clocksource: arm_arch_timer: set arch_mem_timer cpumask to cpu_possible_mask

From: Sudeep Holla <sudeep.holla@arm.com>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: Sudeep Holla <sudeep.holla@arm.com>,
	linux-kernel@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	Marc Zyngier <marc.zyngier@arm.com>,
	Kevin Hilman <khilman@baylibre.com>,
	Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
	Mark Rutland <mark.rutland@arm.com>
Subject: Re: [PATCH 2/2] clocksource: arm_arch_timer: set arch_mem_timer cpumask to cpu_possible_mask
Date: Tue, 10 Jul 2018 11:29:05 +0100	[thread overview]
Message-ID: <b98e0d87-3d9a-9b0c-23c4-7b289cc79a97@arm.com> (raw)
In-Reply-To: <alpine.DEB.2.21.1807100008100.1590@nanos.tec.linutronix.de>

On 09/07/18 23:09, Thomas Gleixner wrote:
> On Mon, 9 Jul 2018, Sudeep Holla wrote:
> 
>> Currently, arch_mem_timer cpumask is set to cpu_all_mask which should be
>> fine. However, cpu_possible_mask is more accurate and if there are other
>> clockevent source in the system which are set to cpu_possible_mask, then
>> having cpu_all_mask may result in issue.
>>
>> E.g. on a platform with arm,sp804 timer with rating 300 and
>> cpu_possible_mask and this arch_mem_timer timer with rating 400 and
>> cpu_all_mask, tick_check_preferred may choose both preferred as the
>> cpumasks are not equal though they must be.
>>
>> This issue was root caused incorrectly initially and a fix was merged as
>> commit 1332a9055801 ("tick: Prefer a lower rating device only if it's CPU
>> local device").
> 
> To avoid that in the future we really should fix the decision logic to mask
> out the non possible CPUs from the supplied masks.

Sure, I can do that. But do you want that as a fix for v4.18 ?

I think we may need this check at another place in tick_setup_device

213         /*
214          * When the device is not per cpu, pin the interrupt to the
215          * current cpu:
216          */
217         if (!cpumask_equal(newdev->cpumask, cpumask))
218                 irq_set_affinity(newdev->irq, cpumask);

Does it make sense trim dev->cpumask when registering the clockevents
device itself instead of adding check at place where this cpumask
can be used ? So that any future user of those masks need not have to
take care of that.

Also only few ARM clocksource drivers use cpu_all_mask which could be
result of copy-paste, we can even fix them too.

arm_arch_timer.c:	clk->cpumask = cpu_all_mask;
tegra20_timer.c:	tegra_clockevent.cpumask = cpu_all_mask;
timer-atcpit100.c:	.cpumask = cpu_all_mask,
timer-keystone.c:	event_dev->cpumask = cpu_all_mask;
zevio-timer.c:		timer->clkevt.cpumask	= cpu_all_mask;

-- 
Regards,
Sudeep