Skip to content

use crate critical-section in defmt-rtt #640

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

Merged
bors merged 3 commits into from
Feb 10, 2022
Merged

use crate critical-section in defmt-rtt #640

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
6dbaa89
use crate critical-section in defmt-rtt
jannic Nov 26, 2021
d6fa863
remove depenency on cortex-m
jannic Nov 26, 2021
890a4a8
add changelog entry
jannic Dec 1, 2021
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
2 changes: 1 addition & 1 deletion CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/).

## [Unreleased]

- None
- [#640]: use crate [critical-section](https://crates.io/crates/critical-section) in defmt-rtt

## [v0.3.1] - 2021-11-26

Expand Down
2 changes: 1 addition & 1 deletion firmware/defmt-rtt/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,5 @@ repository = "https://github.com/knurling-rs/defmt"
version = "0.3.1"

[dependencies]
cortex-m = "0.7"
defmt = { version = "0.3", path = "../../defmt" }
critical-section = "0.2.5"
16 changes: 5 additions & 11 deletions firmware/defmt-rtt/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,7 @@

mod channel;

use core::sync::atomic::{AtomicBool, AtomicUsize, Ordering};

use cortex_m::{interrupt, register};
use core::sync::atomic::{AtomicBool, AtomicU8, AtomicUsize, Ordering};

use crate::channel::Channel;

Expand All @@ -39,13 +37,12 @@ struct Logger;

/// Global logger lock.
static TAKEN: AtomicBool = AtomicBool::new(false);
static INTERRUPTS_ACTIVE: AtomicBool = AtomicBool::new(false);
static INTERRUPTS_ACTIVE: AtomicU8 = AtomicU8::new(0);
static mut ENCODER: defmt::Encoder = defmt::Encoder::new();

unsafe impl defmt::Logger for Logger {
fn acquire() {
let primask = register::primask::read();
interrupt::disable();
let token = unsafe { critical_section::acquire() };
Copy link
Copy Markdown
Contributor

@jonas-schievink jonas-schievink Jan 12, 2022

Choose a reason for hiding this comment

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

What are the safety invariants of this function that we have to uphold?

Copy link
Copy Markdown
Member

@Urhengulas Urhengulas Jan 18, 2022

Choose a reason for hiding this comment

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

@jannic Could you shed light on this?

Copy link
Copy Markdown
Contributor Author

@jannic jannic Jan 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

As @jonas-schievink already mentioned, critical-section is missing some documentation.
This is part of it: The intended safety-guarantees are not specified.

As far as I can tell, there are no real requirements. The current implementations of critical_section::acquire() are not really unsafe, but just disable interrupts.

My guess is that it's just for symmetry with critical_section::release(), which is obviously unsafe, as enabling interrupts could break other code expecting to run exclusively.

@Dirbaio, any comments?

Copy link
Copy Markdown
Member

@Urhengulas Urhengulas Feb 10, 2022

Choose a reason for hiding this comment

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

@jannic Could you please add safety comments for critical_section::acquire and critical_section::release, with what you wrote here? Then we should be good to go.

Copy link
Copy Markdown
Contributor

@Dirbaio Dirbaio Feb 10, 2022

Choose a reason for hiding this comment

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

I still have to write docs for critical-section (sorry! 🙈 ), but the safety contract is essentially:

  • Each acquire must be paired with a release with the same token.
  • acquire/release pairs must be "properly nested", ie it's not OK to do a=acquire(); b=acquire(); release(a); release(b);.

Code in this PR complies with the safety contract, so LGTM 👍

Copy link
Copy Markdown
Contributor

@jonas-schievink jonas-schievink Feb 10, 2022

Choose a reason for hiding this comment

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

Thanks, that sounds good for now.

Copy link
Copy Markdown
Contributor Author

@jannic jannic Feb 10, 2022

Choose a reason for hiding this comment

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

acquire() has to be unsafe because you can use it to break the "properly nested" requirement in someone else's acquire/release pair deeper in the call stack, if you don't do the corresponding release() call.

As fn acquire() is not unsafe, one could break this contract by just calling Logger::acquire() in some random location.
But as far as I can tell, nothing bad would happen with that alone? eg:

critical_section::with(|_| Logger::acquire());

This would lead to a sequence of calls like a=acquire(); b=acquire(); release(a).
But as long as you neither call release(b) (which would be unsafe) or rely on being inside a critical section after release(a), I don't see any bad effect of that call sequence.

Copy link
Copy Markdown
Contributor Author

@jannic jannic Feb 10, 2022

Choose a reason for hiding this comment

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

As fn acquire() is not unsafe, one could break this contract by just calling Logger::acquire() in some random location.

One can't, as Logger::acquire() is private.
I still think that it wouldn't do harm if one could call it, but as one can't, either way it doesn't matter.

Copy link
Copy Markdown
Contributor Author

@jannic jannic Feb 10, 2022

Choose a reason for hiding this comment

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

Oops, too fast...
There is defmt::export::acquire();, which is pub and not unsafe. So it's perfectly possible to call acquire() out of order. AFAIK the only thing bad which will happen is that the next call to a defmt logger will panic ("defmt logger taken reentrantly"), which seems to be a perfectly fine response.

However, there is something much more dangerous: defmt::export::release(). which allows to end a critical section unconditionally (or, before merging this pull request, to enable interrupts). I think this should be unsafe, right?

info!("something");  // log something while interrupts are enabled. `INTERRUPTS_ACTIVE` remembers that interrupts were enabled
cortex_m::interrupt::free(|cs| {
  // interrupts are disabled
  defmt::export::release();
  // oh no! interrupts are enabled, again
  // but we still have the cs token:
  let borrowed = cortex_m_mutex.borrow(cs);
  // do something with shared value while interrupts are enabled
}

Copy link
Copy Markdown
Contributor Author

@jannic jannic Feb 10, 2022

Choose a reason for hiding this comment

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

→ #659


if TAKEN.load(Ordering::Relaxed) {
panic!("defmt logger taken reentrantly")
Expand All @@ -54,7 +51,7 @@ unsafe impl defmt::Logger for Logger {
// no need for CAS because interrupts are disabled
TAKEN.store(true, Ordering::Relaxed);

INTERRUPTS_ACTIVE.store(primask.is_active(), Ordering::Relaxed);
INTERRUPTS_ACTIVE.store(token, Ordering::Relaxed);

// safety: accessing the `static mut` is OK because we have disabled interrupts.
unsafe { ENCODER.start_frame(do_write) }
Expand All @@ -70,10 +67,7 @@ unsafe impl defmt::Logger for Logger {
ENCODER.end_frame(do_write);

TAKEN.store(false, Ordering::Relaxed);
if INTERRUPTS_ACTIVE.load(Ordering::Relaxed) {
// re-enable interrupts
interrupt::enable()
}
critical_section::release(INTERRUPTS_ACTIVE.load(Ordering::Relaxed));
}

unsafe fn write(bytes: &[u8]) {
Expand Down
⚡