kern: let drivers disable double-interrupt behavior #1948
Conversation
| The concept of a "pending" interrupt is inherently specific to a particular | ||
| architecture and interrupt controller. On an architecture without a concept of |
There was a problem hiding this comment.
this might just be one of my pet hobby-horses but i might cite some examples of such architectures here?
There was a problem hiding this comment.
I can't, sorry. I'd take suggestions, but, all the architectures where I'm deeply familiar with the interrupt controller have this bit. I think maybe AVRs don't, but they also don't have a Rust port.
There was a problem hiding this comment.
hm, honestly, maybe we should just say "such as all the architectures Hubris currently targets"...but we would have to go and change that if hubris gets ported to some weird thing someday...
sys/kern/src/syscalls.rs
Outdated
| @@ -753,15 +753,16 @@ fn irq_control( | |||
| ) -> Result<NextTask, UserError> { | |||
| let args = tasks[caller].save().as_irq_args(); | |||
|
|
|||
| let operation = match args.control { | |||
| let operation = match args.control & 1 { | |||
There was a problem hiding this comment.
feel free to ignore me, but i would personally prefer to write these masks as
| let operation = match args.control & 1 { | |
| let operation = match args.control & 0b01 { |
as it feels a bit more obvious which bit we are picking out
There was a problem hiding this comment.
Added a bitflags type and removed the constants.
sys/kern/src/syscalls.rs
Outdated
| let also_clear_pending = args.control & 2 != 0; | ||
| if args.control & !3 != 0 { |
There was a problem hiding this comment.
similarly, this is just a personal style preference and you're welcome to tell me i'm wrong, but i'd have done
| let also_clear_pending = args.control & 2 != 0; | |
| if args.control & !3 != 0 { | |
| let also_clear_pending = args.control & 0b10 != 0; | |
| if args.control & !0b11 != 0 { |
sys/userlib/src/lib.rs
Outdated
| #[inline(always)] | ||
| pub fn sys_irq_control_clear_pending(mask: u32, enable: bool) { | ||
| unsafe { | ||
| sys_irq_control_stub(mask, enable as u32 | 0b10); |
There was a problem hiding this comment.
it seems like it could be nicer if the bitflags for this argument were defined in sys/abi so that the kernel and userlib could depend on the same definition, instead of having magic numbers. this isn't a huge deal, and if it's too annoying, we're probably not going to go and mess with them any time soon, so it's fine-as is. but, it would make me a little happier.
You are too hard on yourself, these are good comments. :-) |
|
Alright, addressed, PTAL. |
| bitflags::bitflags! { | ||
| /// Bitflags that can be passed into the `IRQ_CONTROL` syscall. | ||
| #[derive(Copy, Clone, Debug, PartialEq, Eq)] | ||
| pub struct IrqControlArg: u32 { | ||
| /// Enables the interrupt if present, disables if not present. | ||
| const ENABLED = 1 << 0; | ||
| /// If present, requests that any pending instance of this interrupt be | ||
| // cleared. | ||
| const CLEAR_PENDING = 1 << 1; | ||
| } |
| UsageError::NoIrq, | ||
| ))); | ||
| } | ||
| let also_clear_pending = control.contains(IrqControlArg::CLEAR_PENDING); |
Currently drivers tend to get a spurious interrupt notification just after handling a real one. This is because the interrupt controller notices the kernel generic ISR exit while the interrupt condition is still asserted, because it doesn't expect us to be handling it in task code. So, it sets the pending bit so the 'new' interrupt condition isn't forgotten. This causes an interrupt to occur immediately when we re-enable. This adds an additional bit for IRQ_CONTROL that drivers can use to request any pending interrupt status be cleared. Fixes oxidecomputer#536.
cbiffle
force-pushed
the
cbiffle/clear-pending-irq
branch
from
74af38d to
3be9463
Compare
Currently drivers tend to get a spurious interrupt notification just after handling a real one. This is because the interrupt controller notices the kernel generic ISR exit while the interrupt condition is still asserted, because it doesn't expect us to be handling it in task code. So, it sets the pending bit so the 'new' interrupt condition isn't forgotten.
This causes an interrupt to occur immediately when we re-enable.
This adds an additional bit for IRQ_CONTROL that drivers can use to request any pending interrupt status be cleared.
Fixes #536.