On Mon, Nov 14, 2016 at 03:17:48PM +0000, Noam Camus wrote: > > From: Daniel Lezcano [mailto:daniel.lezcano@xxxxxxxxxx] > > Sent: Monday, November 14, 2016 4:35 PM > > > >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. > > >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 ? > >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. > > ... > >> >> + .set_state_shutdown = > >> >> nps_clkevent_timer_shutdown, > >> > >> >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. > > >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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
