arch: arm: Add z_arch_irq_current_prio function #17301

nordic-krch · 2019-07-03T10:20:22Z

Added function and extended test to cover new api.

ioannisg · 2019-07-03T12:09:39Z

What is the use case of this ARM-only aPI?

nordic-krch · 2019-07-03T12:16:40Z

What is the use case of this ARM-only aPI?

See #17293
https://github.com/zephyrproject-rtos/zephyr/blob/8de2f920eb9cdfae0c9e780873f5e08f61b209e3/drivers/clock_control/nrf_power_clock.c#L93

Function should not be called from interrupt priority higher than clock control because it may lead to deadlock since clock state will never be updated if status is read in a loop waiting for clock on and clock interrupt cannot be handled (since it has lower priority).

It maybe extended to other platforms as well if there is a use case.

ioannisg · 2019-07-03T12:31:52Z

Right, I need to think how this works with trustzone, there are some tricky parts there

andrewboie

I'd rather you called this z_arm_irq_current_prio() since unlike the other z_arch_* APIs there is no generic kernel API that calls it and this is only implemented on ARM.

andyross · 2019-07-03T18:07:47Z

Agree on the naming. Further, if the only use of this is an assert to catch priority inversion bugs, can't you do that via static analysis? For example, just make sure clock_control is at the highest available priority.

nordic-krch · 2019-07-04T05:34:04Z

I'd rather you called this z_arm_irq_current_prio()

Done

For example, just make sure clock_control is at the highest available priority.

I wouldn't like to force that to the user, especially that this does not help if context is on the same highest, priority level.

ioannisg

Just to contribute to the "api" discussion - would it be better to name this as bool irq_can_preempt(irq_line) or something like that? This looks more close to what you want to have for your use-case, right?

nordic-krch · 2019-07-04T07:56:10Z

@ioannisg you mean that function would check if current context can be preempted by given interrupt?

ioannisg · 2019-07-04T07:59:33Z

@ioannisg you mean that function would check if current context can be preempted by given interrupt?

Yes, that's what I meant.

nordic-krch · 2019-07-09T10:04:01Z

@ioannisg changed as you suggested to z_arm_irq_can_preempt.

zephyrbot · 2019-07-09T10:19:31Z

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

ioannisg · 2019-07-10T10:14:00Z

arch/arm/core/irq_manage.c

+
+bool z_arm_irq_can_preempt(u32_t irq_line)
+{
+	return z_arm_irq_current_prio() > NVIC_GetPriority((IRQn_Type)irq_line);


Is this really what we want, here? What if the irq_line is, in fact, not enabled, and the priority is a trash value? Can this actually happen?

current use case is using it in the driver which handles irq_line. I wouldn't involve enabling checking into that. What to return if interrupt is disabled? false would be correct because disabled interrupt cannot be preempt current context but that is not what we wanted to check.

By default priority in nvic is set to 0 (highest). I can add a note that only priority is checked and not the state of the interrupt.

This all depends on what API we want to provide, see the discussion below. So, if the irq_line is not enabled, this function should return false; that's my opinion. And of course, we document this nicely. Since we named it as _can_preempt(), we need to see if it can really preempt..

ioannisg · 2019-07-10T10:15:11Z

include/arch/arm/cortex_m/irq.h

@@ -153,8 +153,19 @@ extern void z_irq_spurious(void *unused);
 extern void _isr_wrapper(void);
 #endif

+/**


What we really need to decide for the API, is;

can be called by interrupts?

should it be thread-safe or don't care?

could it be generalized for all ARCHEs?

can be called by interrupts?

yes

should it be thread-safe or don't care?

I would say don't care. Are we afraid that interrupt priority level will change after that call? How likely is that?

could it be generalized for all ARCHEs?

when use case comes some will do that. I wouldn't like to see PR pending on other platforms implementation when current use case is for peripheral driver on arm cortex.

I agree that this should be called by interrupts. We shall document that.
Also, it is fine if this is an arm-only API: we call the function z_arm_xxx and we can use a z_arch_ static inline function in include/arch/arm.

I would say don't care. Are we afraid that interrupt priority level will change after that call? How likely is that?

Now, this is problematic. If we make an API that checks if we can be preempted, we need to make sure this works in a deterministic way. I thought about it, and I don't want this to be fuzzy. We need to make this thread-safe, right?

Added function for checking if current context can be preempted by given interrupt. Signed-off-by: Krzysztof Chruscinski <[email protected]>

Extended arm_irq_vector_table test to verify z_arm_irq_can_preempt function. Signed-off-by: Krzysztof Chruscinski <[email protected]>

Added a simulation replacement for NVIC_GetPriority() + Added a function (like for ARM) to check if current context can be preempted by given interrupt. Signed-off-by: Alberto Escolar Piedras <[email protected]>

carlescufi · 2019-08-26T14:35:54Z

@andyross could you please review this?

ioannisg · 2019-10-04T12:50:20Z

Will you continue working on this @nordic-krch or should we close the PR?

nordic-krch · 2019-10-04T13:01:44Z

will close it for now. Maybe feature need will come back.

nordic-krch added the area: ARM ARM (32-bit) Architecture label Jul 3, 2019

nordic-krch requested review from ioannisg and andrewboie July 3, 2019 10:20

nordic-krch requested review from andyross, galak, MaureenHelm and nashif as code owners July 3, 2019 10:20

nordic-krch mentioned this pull request Jul 3, 2019

drivers: clock_control: nrf driver reimplementation #17293

Merged

zephyrbot added area: API Changes to public APIs area: Tests Issues related to a particular existing or missing test area: Kernel labels Jul 3, 2019

aescolar force-pushed the irq_current_prio branch from 93883a8 to f6cc41a Compare July 3, 2019 11:45

nordic-krch requested a review from aescolar as a code owner July 3, 2019 11:45

aescolar force-pushed the irq_current_prio branch from f6cc41a to 8d560ab Compare July 3, 2019 11:49

zephyrbot added the area: Boards label Jul 3, 2019

aescolar force-pushed the irq_current_prio branch from 8d560ab to 9950efb Compare July 3, 2019 14:17

andrewboie suggested changes Jul 3, 2019

View reviewed changes

nordic-krch force-pushed the irq_current_prio branch from 9950efb to 553397b Compare July 4, 2019 05:32

aescolar requested a review from andrewboie July 4, 2019 06:40

ioannisg reviewed Jul 4, 2019

View reviewed changes

nordic-krch force-pushed the irq_current_prio branch from 553397b to 3cbfa04 Compare July 9, 2019 10:03

nordic-krch force-pushed the irq_current_prio branch from 3cbfa04 to aea435e Compare July 9, 2019 10:38

ioannisg reviewed Jul 10, 2019

View reviewed changes

nordic-krch and others added 3 commits July 11, 2019 12:41

arch: arm: Add z_arm_irq_can_preempt function

8408b63

Added function for checking if current context can be preempted by given interrupt. Signed-off-by: Krzysztof Chruscinski <[email protected]>

tests: kernel: arm_irq_vector_table: Added z_arm_irq_can_preempt test

0b28658

Extended arm_irq_vector_table test to verify z_arm_irq_can_preempt function. Signed-off-by: Krzysztof Chruscinski <[email protected]>

nrf52_bsim: irq handling: Add function z_arm_irq_can_preempt

7dedcee

Added a simulation replacement for NVIC_GetPriority() + Added a function (like for ARM) to check if current context can be preempted by given interrupt. Signed-off-by: Alberto Escolar Piedras <[email protected]>

nordic-krch force-pushed the irq_current_prio branch from aea435e to 7dedcee Compare July 11, 2019 10:41

pabigot mentioned this pull request Sep 6, 2019

Define and document expected behavior of API calls in various contexts #18970

Open

nordic-krch closed this Oct 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

arch: arm: Add z_arch_irq_current_prio function #17301

arch: arm: Add z_arch_irq_current_prio function #17301

nordic-krch commented Jul 3, 2019

ioannisg commented Jul 3, 2019

nordic-krch commented Jul 3, 2019

ioannisg commented Jul 3, 2019

andrewboie left a comment

andyross commented Jul 3, 2019

nordic-krch commented Jul 4, 2019

ioannisg left a comment

nordic-krch commented Jul 4, 2019

ioannisg commented Jul 4, 2019

nordic-krch commented Jul 9, 2019

zephyrbot commented Jul 9, 2019 •

edited

Loading

ioannisg Jul 10, 2019

nordic-krch Jul 11, 2019

ioannisg Jul 11, 2019 •

edited

Loading

ioannisg Jul 10, 2019 •

edited

Loading

nordic-krch Jul 11, 2019

ioannisg Jul 11, 2019

ioannisg Jul 11, 2019

carlescufi commented Aug 26, 2019

ioannisg commented Oct 4, 2019

nordic-krch commented Oct 4, 2019

arch: arm: Add z_arch_irq_current_prio function #17301

arch: arm: Add z_arch_irq_current_prio function #17301

Conversation

nordic-krch commented Jul 3, 2019

ioannisg commented Jul 3, 2019

nordic-krch commented Jul 3, 2019

ioannisg commented Jul 3, 2019

andrewboie left a comment

Choose a reason for hiding this comment

andyross commented Jul 3, 2019

nordic-krch commented Jul 4, 2019

ioannisg left a comment

Choose a reason for hiding this comment

nordic-krch commented Jul 4, 2019

ioannisg commented Jul 4, 2019

nordic-krch commented Jul 9, 2019

zephyrbot commented Jul 9, 2019 • edited Loading

ioannisg Jul 10, 2019

Choose a reason for hiding this comment

nordic-krch Jul 11, 2019

Choose a reason for hiding this comment

ioannisg Jul 11, 2019 • edited Loading

Choose a reason for hiding this comment

ioannisg Jul 10, 2019 • edited Loading

Choose a reason for hiding this comment

nordic-krch Jul 11, 2019

Choose a reason for hiding this comment

ioannisg Jul 11, 2019

Choose a reason for hiding this comment

ioannisg Jul 11, 2019

Choose a reason for hiding this comment

carlescufi commented Aug 26, 2019

ioannisg commented Oct 4, 2019

nordic-krch commented Oct 4, 2019

zephyrbot commented Jul 9, 2019 •

edited

Loading

ioannisg Jul 11, 2019 •

edited

Loading

ioannisg Jul 10, 2019 •

edited

Loading