sys/pm: Correctly access pm_blocker #13977
Conversation
maribu
added
Type: bug
maribu
force-pushed
the
pm_atomic_access
branch
from
6bbbc9a
to
a689b9d
Compare
benpicco
added
CI: ready for build
| unsigned state = irq_disable(); | ||
| pm_blocker_t result = pm_blocker; | ||
| irq_restore(state); | ||
| return result; |
There was a problem hiding this comment.
This should only be necessary on non-32 bit machines, no?
There was a problem hiding this comment.
There sadly is no guarantee, that the compiler will generate a single read whenever possible. There were examples on GCC generating two 32 bit stores on x86_64 when writing to a volatile, as two stores with an immediate where faster than preparing the correct value in a register and writing that back to memory. As a result, some linux drivers stopped working when compiled with that GCC version. I think it is safe to say that the Linux kernel guys convinced the compiler guys that their compilers have no host to run on if they break kernels by splitting a single volatile accesses into two accesses. But for non-volatile accesses we should still be careful to do any assumptions on how the generated assembly looks like.
So what we could IMO safely do is something like this:
pm_blocker_t pm_get_blocker(void)
{
pm_blocker_t result;
if (ARCH_32BIT) {
volatile uint32_t *tmp = &pm_blocker.val_u32;
result.val_u32 = *tmp;
}
else {
unsigned state = irq_disable();
result = pm_blocker;
irq_restore(state);
}
return result;
}It would be nice to make the FEATURES like arch32_bit available to C code for this. The compiler should than drop the dead branch.
There was a problem hiding this comment.
ARCH_32_BIT is now available, can you adapt to your suggestion?
There was a problem hiding this comment.
I would merge without optimizations, that PR can take time and this is still potential source for issues.
|
Tested on nucleo-l152re |
|
@maribu can you push to trigger github-actions on this one as well? |
Replace `volatile` access to pm_blocker by guarding the accesses with `irq_disable()` ... `irq_restore()`. `volatile` does only guarantee that no compiler optimizations are performed on a variable access, but does not provide atomic access. E.g. on systems with a memory bus of less than 32 bit, the access to pm_blocker cannot be done with a single CPU instruction. Thus, resorting to disabling IRQs is the easiest and most portable way to actually achieve atomic access.
cgundogan
added
CI: ready for build
There was a problem hiding this comment.
ACK, there are possible optimization for 32bit platforms as mentioned in #13977 (comment), it was agreed to postpone those to after #14331 is in.
|
Thanks :-) |
Contribution description
Replace
volatileaccess to pm_blocker by guarding the accesses withirq_disable()...irq_restore().volatiledoes only guarantee that no compiler optimizations are performed on a variable access, but does not provide atomic access. E.g. on systems with a memory bus of less than 32 bit, the access to pm_blocker cannot be done with a single CPU instruction. Thus, resorting to disabling IRQs is the easiest and most portable way to actually achieve atomic access.Testing procedure
tests/periph_pmshould still workIssues/PRs references
Split out of #13973
Depends on and contains #13976