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

[Thomas Gleixner]

On Tue, 10 Jul 2018, Sudeep Holla wrote:

> 
> 
> 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 ?

No, that's for 4.19

> 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.

The problem is you cannot trim it because cpu_all_mask is global and const.

> 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;

Yes, that makes sense. What we could do is warn, when cpu_all_mask is set
at registration time and replace the pointer with cpu_possible_mask.

Thanks,

	tglx