Skip to content

arch: arm: Add z_arch_irq_current_prio function #17301

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

arch: arm: Add z_arch_irq_current_prio function #17301

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8408b63
arch: arm: Add z_arm_irq_can_preempt function
nordic-krch Jul 3, 2019
0b28658
tests: kernel: arm_irq_vector_table: Added z_arm_irq_can_preempt test
nordic-krch Jul 3, 2019
7dedcee
nrf52_bsim: irq handling: Add function z_arm_irq_can_preempt
aescolar Jul 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions arch/arm/core/irq_manage.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,25 @@ void z_irq_spurious(void *unused)
__reserved();
}

static int z_arm_irq_current_prio(void)
{
u32_t isr_vector_num = __get_IPSR() & IPSR_ISR_Msk;
s32_t irq_type;

if (isr_vector_num == 0) {
return INT_MAX;
}

irq_type = ((s32_t)isr_vector_num - 16);

return (NVIC_GetPriority((IRQn_Type)irq_type) & 0xFF);
}

bool z_arm_irq_can_preempt(u32_t irq_line)
{
return z_arm_irq_current_prio() > NVIC_GetPriority((IRQn_Type)irq_line);
Copy link
Member

@ioannisg ioannisg Jul 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@nordic-krch nordic-krch Jul 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ioannisg ioannisg Jul 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/* FIXME: IRQ direct inline functions have to be placed here and not in
* arch/cpu.h as inline functions due to nasty circular dependency between
* arch/cpu.h and kernel_structs.h; the inline functions typically need to
Expand Down
3 changes: 3 additions & 0 deletions boards/posix/nrf52_bsim/board_irq.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@

#include "sw_isr_table.h"
#include "zephyr/types.h"
#include "NRF_regs.h"

#ifdef __cplusplus
extern "C" {
Expand All @@ -18,6 +19,8 @@ extern "C" {
void z_isr_declare(unsigned int irq_p, int flags, void isr_p(void *),
void *isr_param_p);
void z_irq_priority_set(unsigned int irq, unsigned int prio, u32_t flags);
bool z_arm_irq_can_preempt(u32_t irq_line);
unsigned int NVIC_GetPriority(IRQn_Type IRQn);

/**
* Configure a static interrupt.
Expand Down
28 changes: 28 additions & 0 deletions boards/posix/nrf52_bsim/irq_handler.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,6 +327,25 @@ void z_irq_priority_set(unsigned int irq, unsigned int prio, uint32_t flags)
hw_irq_ctrl_prio_set(irq, prio);
}

/**
* @brief Return current context priority.
*
* @return Interrupt priority or INT_MAX if in thread mode.
*/
static int z_arm_irq_current_prio(void)
{
if (_kernel.nested == 0) {
return INT_MAX;
} else {
return hw_irq_ctrl_get_cur_prio();
}
}

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

/**
* Similar to ARM's NVIC_SetPendingIRQ
* set a pending IRQ from SW
Expand Down Expand Up @@ -402,6 +421,15 @@ void NVIC_ClearPendingIRQ(IRQn_Type IRQn)
hw_irq_ctrl_clear_irq(IRQn);
}

/*
* Replacement for ARMs NVIC_GetPriority()
* Return the programmed priority of IRQn
*/
unsigned int NVIC_GetPriority(IRQn_Type IRQn)
{
return hw_irq_ctrl_get_prio(IRQn);
}

/*
* Very simple model of the WFE and SEV ARM instructions
* which seems good enough for the Nordic controller
Expand Down
13 changes: 13 additions & 0 deletions include/arch/arm/cortex_m/irq.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,8 +153,21 @@ extern void z_irq_spurious(void *unused);
extern void _isr_wrapper(void);
#endif

/**
Copy link
Member

@ioannisg ioannisg Jul 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@nordic-krch nordic-krch Jul 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ioannisg ioannisg Jul 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@ioannisg ioannisg Jul 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

* @brief Check if current context can be preempted by given interrupt.
*
* @note Function does not check if interrupt is enabled.
*
* @param irq_line Interrupt ID.
*
* @return true if current context can be preempted by given interrupt, false
* otherwise.
*/
extern bool z_arm_irq_can_preempt(u32_t irq_line);

#endif /* _ASMLANGUAGE */


#ifdef __cplusplus
}
#endif
Expand Down
25 changes: 23 additions & 2 deletions tests/kernel/arm_irq_vector_table/src/arm_irq_vector_table.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,10 @@ struct k_sem sem[3];
void isr0(void)
{
printk("%s ran!\n", __func__);
/* Test which interrupt can preempt current context */
zassert_false(z_arm_irq_can_preempt(0 + _ISR_OFFSET), "");
zassert_false(z_arm_irq_can_preempt(1 + _ISR_OFFSET), "");
zassert_false(z_arm_irq_can_preempt(2 + _ISR_OFFSET), "");
k_sem_give(&sem[0]);
_IntExit();
}
Expand All @@ -82,6 +86,10 @@ void isr0(void)
void isr1(void)
{
printk("%s ran!\n", __func__);
/* Test which interrupt can preempt current context */
zassert_true(z_arm_irq_can_preempt(0 + _ISR_OFFSET), "");
zassert_false(z_arm_irq_can_preempt(1 + _ISR_OFFSET), "");
zassert_false(z_arm_irq_can_preempt(2 + _ISR_OFFSET), "");
k_sem_give(&sem[1]);
_IntExit();
}
Expand All @@ -96,6 +104,9 @@ void isr1(void)
void isr2(void)
{
printk("%s ran!\n", __func__);
zassert_true(z_arm_irq_can_preempt(0 + _ISR_OFFSET), "");
zassert_true(z_arm_irq_can_preempt(1 + _ISR_OFFSET), "");
zassert_false(z_arm_irq_can_preempt(2 + _ISR_OFFSET), "");
k_sem_give(&sem[2]);
_IntExit();
}
Expand All @@ -116,19 +127,29 @@ void isr2(void)
* NVIC_SetPendingIRQ(), to trigger the pending interrupt. And we check
* that the corresponding interrupt handler is getting called or not.
*
* @see irq_enable(), z_irq_priority_set(), NVIC_SetPendingIRQ()
* Addtionally, z_arm_irq_can_preempt() is tested in the thread mode and
* interrupt context.
*
* @see irq_enable(), z_irq_priority_set(), NVIC_SetPendingIRQ(),
* z_arm_irq_can_preempt()
*
*/
void test_arm_irq_vector_table(void)
{
printk("Test Cortex-M IRQs installed directly in the vector table\n");


for (int ii = 0; ii < 3; ii++) {
irq_enable(_ISR_OFFSET + ii);
z_irq_priority_set(_ISR_OFFSET + ii, 0, 0);
z_irq_priority_set(_ISR_OFFSET + ii, ii, 0);
k_sem_init(&sem[ii], 0, UINT_MAX);
}

/* Test that context can be preempted by each interrupt */
zassert_true(z_arm_irq_can_preempt(0 + _ISR_OFFSET), "");
zassert_true(z_arm_irq_can_preempt(1 + _ISR_OFFSET), "");
zassert_true(z_arm_irq_can_preempt(2 + _ISR_OFFSET), "");

zassert_true((k_sem_take(&sem[0], K_NO_WAIT) ||
k_sem_take(&sem[1], K_NO_WAIT) ||
k_sem_take(&sem[2], K_NO_WAIT)), NULL);
Expand Down