-
-
Notifications
You must be signed in to change notification settings - Fork 790
Introduce the PanicWriter trait, a standard mechanism for implementing synchronous writers for panic messages
#4684
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.
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
base: master
Are you sure you want to change the base?
Conversation
The `PanicWriter` trait allows creation of a synchronous, reinitialized Uart-like peripheral for use in panic handlers.
lschuermann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I agree with this approach. Given the implementation of PanicWriter will be chip-specific, and should carefully reason about whether an implementation of PanicWriter is actually safe for each UART driver, I think it might make sense to split these up over multiple PRs.
| /// Create a new synchronous writer capable of sending panic messages. | ||
| /// | ||
| /// The constructed writer must be created on the stack. Because panic | ||
| /// will never return this is effectively a static allocation. | ||
| /// | ||
| /// The writer must implement [`IoWrite`]. | ||
| unsafe fn create_panic_writer(config: Self::Config) -> impl IoWrite; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't really enforce that the return value will be stored on the stack, and I don't think it actually matters.
What does matter are the other guarantees we ought to require callers of this unsafe function to uphold:
- the function must only be called once: that ensures that we will never have two instances of the resulting
impl IoWritearound that may be trampling over each other, and - this function must only be called in the panic handler. In particular, after this function is called, no other code will access the hardware backing this writer. In a sense, this function will forcefully "take over" the underlying hardware, whichever state it will be in. A full system reset is required before any other code is allowed to access the underlying hardware again.
These promises by the caller are, I think, required to build reliable and sound panic writer implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other two guarantees make sense and I will add.
Re: where the writer is stored, aren't the three options statically, on the heap, or on the stack? We don't have a heap. Is there a way for the implementation to declare it statically that we would find acceptable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bradjc I think you're right, but I'm not even sure what the reader is supposed to do with this:
/// The constructed writer must be created on the stack. Because panic
/// will never return this is effectively a static allocation.
I don't necessarily even think it's the case that it must have be static. If this drops, isn't it probably fine?
| unsafe fn create_panic_writer(config: Self::Config) -> impl IoWrite { | ||
| use uart::Configure as _; | ||
|
|
||
| let inner = Uarte::new(UARTE0_BASE); | ||
| inner.initialize( | ||
| pinmux::Pinmux::from_pin(config.txd), | ||
| pinmux::Pinmux::from_pin(config.rxd), | ||
| config.cts.map(|c| unsafe { pinmux::Pinmux::from_pin(c) }), | ||
| config.rts.map(|r| unsafe { pinmux::Pinmux::from_pin(r) }), | ||
| ); | ||
| let _ = inner.configure(config.params); | ||
| UartPanicWriter { inner } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this safe regardless of which state the UART is in? Do we need to cancel any (potentially) in-progress DMA operations?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first thing initialize does (via initialize_inner) is stop outstanding operations.
Pull Request Overview
For a long time, Tock's kernel-provided panic utilities required some type that implements
IoWriteto display panic messages. How a board created such a type was up to each board, and the io.rs files we have in boards have used a variety of methods to do this. This has had the effect of creating three issues:IoWrite) are often instantiated asstatic muts, which we are trying to get rid of.To try to fix these issues, this PR (from the static mut task force) proposes a new trait
PanicWriterthat chips would implement to provide a synchronous writer suitable for panic messages. Boards then only need to select the implementation ofPanicWriterthey want to use for panics, and the chip-provided implementation is used by debug.rs to correctly display the panic message.The trait looks like this:
The config type permits the board to provide whatever settings are necessary for the implementation to correctly configure the hardware for panic output.
This addresses the three concerns by:
create_panic_writer()function must instantiate the object on the stack, so it is not declared as astatic mut.Testing Strategy
todo
TODO or Help Wanted
Thoughts?
Need to port every chip.
Documentation Updated
/docs, or no updates are required.Formatting
make prepush.