linux-kernel - RE: [PATCH v5 3/3] clocksource: Add clockevent support to NPS400 driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 14 Nov 2016 15:17:48 +0000
From:   Noam Camus <noamca@...lanox.com>
To:     Daniel Lezcano <daniel.lezcano@...aro.org>
CC:     "robh+dt@...nel.org" <robh+dt@...nel.org>,
        "mark.rutland@....com" <mark.rutland@....com>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v5 3/3] clocksource: Add clockevent support to NPS400
 driver

> From: Daniel Lezcano [mailto:daniel.lezcano@...aro.org] 
> 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?
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.

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

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