On Mon, Nov 14, 2016 at 03:17:48PM +0000, Noam Camus wrote:

quoted

From: Daniel Lezcano [mailto:daniel.lezcano@linaro.org] 
Sent: Monday, November 14, 2016 4:35 PM

quoted

The function nps_clkevent_timer_event_setup() writes into the
NPS_REG_TIMER0_CTRL register but there is no critical section there. What
prevents another HW thread to write this register at the same time ?

Correct, during my last email to you I noticed that fact and already started
fixing it.

quoted

I do believe we have a framework to access shared registers, otherwise a
simple spinlock would be simpler and perhaps faster than disabling the
entire hardware scheduling for the system, no ?

When you are saying "we have a framework" do you mean to some generic
framework in the kernel? 

Yes, IIRC it is regmap but I'm not sure.

Anyway to my understanding I cannot guarantee this
atomics during my routines without preventing HW from changing the HW thread
this core executes.  As SW I am not aware to such HW scheduling, It is much
same as with interrupts that we disable them when we reach code that might be
shared by the interrupt handler.

I think there is something I am missing with this HW scheduling thing. Why are
these hw_schd_save/hw_schd_restore functions needed to be called from the
timer driver ? Regarding the explanation, the HW scheduling can happen everywhere
at any time, not only in the timer code but this one is the only one which need
the hw_schd_save/hw_schd_restore calls, why ?

Why,

spin_lock(&lock);
write_aux_reg(...)
spin_unlock(&lock);

can't work ?

IIUC, there can be more than 16 cpus/threads, so calling hw_schd_save /
hw_schd_restore will disable the HW scheduling for the entire system while one
cpu is processing something with these couple of registers, no ?

quoted

Regarding the comment I did above, it is possible the critical section is
reduced and moved into the shutdown function. Thus, the boolean wouldn't be
needed anymore, well that is conditional to the above comment. Discard the
comment for the moment, until the hw sched vs spinlock vs
NPS_REG_TIMER0_CTRL is sorted out.

OK, I will discard that in the meantime.

...

quoted

quoted

quoted

quoted

+	.set_state_shutdown		=
nps_clkevent_timer_shutdown,

quoted

Doesn't set_state_shutdown and set_state_oneshot_stopped need to remove
the HW thread from the TSI ?

You are correct, I will fix that.

quoted

And tick_resume. Perhaps, that is the reason why NO_HZ hangs.

What NO_HZ hang are you referring to in this case?  How calling
nps_clkevent_rm_thread() explain such hang?  Anyway I agree, and will add
nps_clkevent_rm_thread() to tick_resume.

Actually I meant NOHZ_FULL.
 

Appreciating your effort and will gladly provide any more information needed
about our SoC.  -Noam

-- 

 <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog