-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
core/mutex: clean up #18619
core/mutex: clean up #18619
Conversation
Static tests has some comments, otherwise this looks good. |
I don't know of any code that used (This is sidestepping the question of whether trylock should or should not stay inlined down to the busy parts; about that, I don't care). |
* @internal | ||
* @note This function is intended for use by languages incompatible | ||
* with C (such as C++). Code in C should use @ref mutex_trylock | ||
* instead |
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.
Are you sure you can drop
mutex_trylock_ffi()
?
The doc says it is internal :)
5cc0244
to
f367ea4
Compare
9f593b3
to
5ff92dc
Compare
5ff92dc
to
2be434e
Compare
I also applied @fabian18 's comment in #18620 (comment) |
|
||
#include "kernel_defines.h" | ||
#include "list.h" | ||
#include "thread.h" | ||
|
||
#ifndef __cplusplus | ||
#include "irq.h" |
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.
CI found some users that relied on this implicit include. Better add the include to the .c file that makes use of it.
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.
Looks like there are still some missing.
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.
bme680_hal.c
also needs an update
2be434e
to
ce88f86
Compare
This looks like the rust bindings do care that Update this is the log of a random failed build:
I'll abort the CI run now, so that the other jobs have a chance to get through until tomorrow |
Yes; last time I checked, the functions would have kept their staticness (but maybe I missed it). It's one of these things where the bindings are stricter than C when it comes to API stability. There is a mechanism for "it's OK that this function may be static-inline or not" in the -sys crate, I'll look into updating it. |
I've pushed a fix, and added a branch switch to try out the fix on this branch. If this builds fine, I'll merge RIOT-OS/rust-riot-sys#10. |
I threw in an unrelated fix and a commit to limit the build to (one of) the app the failed building due to the API change from rust's point of view |
7006724
to
243e38d
Compare
243e38d
to
0055484
Compare
This restores a pre-existing design decision to implement both blocking and non-blocking mutex locking with the same code. Those implementations have been split prior to the introduction of the `core_mutex_priority_inheritance` module when `mutex_trylock()` indeed was trivial. This decision didn't age well, so undo it.
0055484
to
2d2bb4b
Compare
Finally, all green. Technically a second ACK is still needed. |
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.
Looks good to me as well.
#if (MAXTHREADS > 1) | ||
mutex_lock_internal(mutex, true); | ||
#else | ||
/* dummy implementation for when no scheduler is used */ | ||
/* (ab)use next pointer as lock variable */ | ||
volatile uintptr_t *lock = (void *)&mutex->queue.next; |
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.
I'm unsure whether volatile
has the right semantics for this (it does guarantee that the write is made, but does it ensure the ordering relative to other operations on memory?), but that's not a thing that needs to be addressed here.
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.
this is indeed another misuse of volatile
. Previously they seemed to "fix" things because irq_disable()
and irq_restore()
did not include compiler memory barriers, but the corresponding inline asm was also marked as volatile
. I am quite sure that the actual bugs are fixed for all platforms now, so that this workaround should be dropped now. But that is something for a different PR IMO
thx :) |
18620: core: add core_mutex_debug to aid debugging deadlocks r=benpicco a=maribu ### Contribution description Adding `USEMODULE += core_mutex_debug` to your `Makefile` results in on log messages such as [mutex] waiting for thread 1 (pc = 0x800024d) being added whenever `mutex_lock()` blocks. This makes tracing down deadlocks easier. ### Testing procedure Run e.g. ```sh USEMODULE=core_mutex_debug BOARD=nucleo-f767zi make -C tests/mutex_cancel flash test ``` which should provide output such as ``` Welcome to pyterm! Type '/exit' to exit. READY s [mutex] waiting for thread 1 (pc = 0x8000f35) START main(): This is RIOT! (Version: 2022.10-devel-841-g5cc02-core/mutex/debug) Test Application for mutex_cancel / mutex_lock_cancelable ========================================================= Test without cancellation: OK Test early cancellation: OK Verify no side effects on subsequent calls: [mutex] waiting for thread 1 (pc = 0x800024d) OK Test late cancellation: [mutex] waiting for thread 1 (pc = 0x0) OK TEST PASSED ``` ```sh $ arm-none-eabi-addr2line -a 0x800024d -e tests/mutex_cancel/bin/nucleo-f767zi/tests_mutex_cancel.elf 0x0800024d /home/maribu/Repos/software/RIOT/tests/mutex_cancel/main.c:51 ``` ### Issues/PRs references Depends on and includes #18619 19296: nanocoap: allow to define CoAP resources as XFA r=benpicco a=benpicco 19504: cpu/cc26xx_cc13xx: Fix bogus array-bound warning r=benpicco a=maribu ### Contribution description GCC 12 create a bogus array out of bounds warning as it assumes that because there is special handling for `uart == 0` and `uart == 1`, `uart` can indeed be `1`. There is an `assert(uart < UART_NUMOF)` above that would blow up prior to any out of bounds access. In any case, optimizing out the special handling of `uart == 1` for when `UART_NUMOF == 1` likely improves the generated code and fixes the warning. /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:88:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds] 88 | ctx[uart].rx_cb = rx_cb; | ~~~^~~~~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx' 52 | static uart_isr_ctx_t ctx[UART_NUMOF]; | ^~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:89:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds] 89 | ctx[uart].arg = arg; | ~~~^~~~~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx' 52 | static uart_isr_ctx_t ctx[UART_NUMOF]; | ^~~ ### Testing procedure The actual change is a pretty obvious one-liner, so that code review and a green CI should be sufficient. If not, running any UART example app without regression should do. ### Issues/PRs references None 19506: tools/openocd: Fix handling of OPENOCD_CMD_RESET_HALT r=maribu a=maribu ### Contribution description The OPENOCD_CMD_RESET_HALT was not longer correctly passed to the script. This fixes the issue. ### Testing procedure Flashing of e.g. the `cc2650-launchpad` with upstream OpenOCD should work again. ### Issues/PRs references The change was added to #19050 after testing the PR and before merging. I'm not sure if the fix never worked because of this, or if behavior of `target-export-variables` or GNU Make changed. 19507: cpu/cc26x0_cc13x0: Drop feature cortexm_mpu r=benpicco a=maribu ### Contribution description At least the CC2650 doesn't have an MPU, I assume this is also true for the rest of the family. The CC2652 does have an MPU according to the datasheet. So I keep the feature there in place. ### Testing procedure E.g. ``` make BOARD=cc2650-launchpad -C tests/mpu_noexec_ram flash test ``` fails. (Note: A successful test run would also crash but with a mem manage handler rather than a hardfault due to an invalid instruction on the stack being executed.) It would be nice to also test the same for a `cc2652-launchpad`, for which the MPU should work. ### Issues/PRs references None Co-authored-by: Marian Buschsieweke <marian.buschsieweke@ovgu.de> Co-authored-by: Benjamin Valentin <benjamin.valentin@bht-berlin.de> Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
18620: core: add core_mutex_debug to aid debugging deadlocks r=benpicco a=maribu ### Contribution description Adding `USEMODULE += core_mutex_debug` to your `Makefile` results in on log messages such as [mutex] waiting for thread 1 (pc = 0x800024d) being added whenever `mutex_lock()` blocks. This makes tracing down deadlocks easier. ### Testing procedure Run e.g. ```sh USEMODULE=core_mutex_debug BOARD=nucleo-f767zi make -C tests/mutex_cancel flash test ``` which should provide output such as ``` Welcome to pyterm! Type '/exit' to exit. READY s [mutex] waiting for thread 1 (pc = 0x8000f35) START main(): This is RIOT! (Version: 2022.10-devel-841-g5cc02-core/mutex/debug) Test Application for mutex_cancel / mutex_lock_cancelable ========================================================= Test without cancellation: OK Test early cancellation: OK Verify no side effects on subsequent calls: [mutex] waiting for thread 1 (pc = 0x800024d) OK Test late cancellation: [mutex] waiting for thread 1 (pc = 0x0) OK TEST PASSED ``` ```sh $ arm-none-eabi-addr2line -a 0x800024d -e tests/mutex_cancel/bin/nucleo-f767zi/tests_mutex_cancel.elf 0x0800024d /home/maribu/Repos/software/RIOT/tests/mutex_cancel/main.c:51 ``` ### Issues/PRs references Depends on and includes #18619 19296: nanocoap: allow to define CoAP resources as XFA r=benpicco a=benpicco Co-authored-by: Marian Buschsieweke <marian.buschsieweke@ovgu.de> Co-authored-by: Benjamin Valentin <benjamin.valentin@bht-berlin.de> Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
18620: core: add core_mutex_debug to aid debugging deadlocks r=maribu a=maribu ### Contribution description Adding `USEMODULE += core_mutex_debug` to your `Makefile` results in on log messages such as [mutex] waiting for thread 1 (pc = 0x800024d) being added whenever `mutex_lock()` blocks. This makes tracing down deadlocks easier. ### Testing procedure Run e.g. ```sh USEMODULE=core_mutex_debug BOARD=nucleo-f767zi make -C tests/mutex_cancel flash test ``` which should provide output such as ``` Welcome to pyterm! Type '/exit' to exit. READY s [mutex] waiting for thread 1 (pc = 0x8000f35) START main(): This is RIOT! (Version: 2022.10-devel-841-g5cc02-core/mutex/debug) Test Application for mutex_cancel / mutex_lock_cancelable ========================================================= Test without cancellation: OK Test early cancellation: OK Verify no side effects on subsequent calls: [mutex] waiting for thread 1 (pc = 0x800024d) OK Test late cancellation: [mutex] waiting for thread 1 (pc = 0x0) OK TEST PASSED ``` ```sh $ arm-none-eabi-addr2line -a 0x800024d -e tests/mutex_cancel/bin/nucleo-f767zi/tests_mutex_cancel.elf 0x0800024d /home/maribu/Repos/software/RIOT/tests/mutex_cancel/main.c:51 ``` ### Issues/PRs references Depends on and includes #18619 Co-authored-by: Marian Buschsieweke <marian.buschsieweke@ovgu.de>
18620: core: add core_mutex_debug to aid debugging deadlocks r=maribu a=maribu ### Contribution description Adding `USEMODULE += core_mutex_debug` to your `Makefile` results in on log messages such as [mutex] waiting for thread 1 (pc = 0x800024d) being added whenever `mutex_lock()` blocks. This makes tracing down deadlocks easier. ### Testing procedure Run e.g. ```sh USEMODULE=core_mutex_debug BOARD=nucleo-f767zi make -C tests/mutex_cancel flash test ``` which should provide output such as ``` Welcome to pyterm! Type '/exit' to exit. READY s [mutex] waiting for thread 1 (pc = 0x8000f35) START main(): This is RIOT! (Version: 2022.10-devel-841-g5cc02-core/mutex/debug) Test Application for mutex_cancel / mutex_lock_cancelable ========================================================= Test without cancellation: OK Test early cancellation: OK Verify no side effects on subsequent calls: [mutex] waiting for thread 1 (pc = 0x800024d) OK Test late cancellation: [mutex] waiting for thread 1 (pc = 0x0) OK TEST PASSED ``` ```sh $ arm-none-eabi-addr2line -a 0x800024d -e tests/mutex_cancel/bin/nucleo-f767zi/tests_mutex_cancel.elf 0x0800024d /home/maribu/Repos/software/RIOT/tests/mutex_cancel/main.c:51 ``` ### Issues/PRs references Depends on and includes #18619 19296: nanocoap: allow to define CoAP resources as XFA r=maribu a=benpicco 19504: cpu/cc26xx_cc13xx: Fix bogus array-bound warning r=maribu a=maribu ### Contribution description GCC 12 create a bogus array out of bounds warning as it assumes that because there is special handling for `uart == 0` and `uart == 1`, `uart` can indeed be `1`. There is an `assert(uart < UART_NUMOF)` above that would blow up prior to any out of bounds access. In any case, optimizing out the special handling of `uart == 1` for when `UART_NUMOF == 1` likely improves the generated code and fixes the warning. /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:88:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds] 88 | ctx[uart].rx_cb = rx_cb; | ~~~^~~~~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx' 52 | static uart_isr_ctx_t ctx[UART_NUMOF]; | ^~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:89:8: error: array subscript 1 is above array bounds of 'uart_isr_ctx_t[1]' [-Werror=array-bounds] 89 | ctx[uart].arg = arg; | ~~~^~~~~~ /home/maribu/Repos/software/RIOT/cc2650/cpu/cc26xx_cc13xx/periph/uart.c:52:23: note: while referencing 'ctx' 52 | static uart_isr_ctx_t ctx[UART_NUMOF]; | ^~~ ### Testing procedure The actual change is a pretty obvious one-liner, so that code review and a green CI should be sufficient. If not, running any UART example app without regression should do. ### Issues/PRs references None 19506: tools/openocd: Fix handling of OPENOCD_CMD_RESET_HALT r=maribu a=maribu ### Contribution description The OPENOCD_CMD_RESET_HALT was not longer correctly passed to the script. This fixes the issue. ### Testing procedure Flashing of e.g. the `cc2650-launchpad` with upstream OpenOCD should work again. ### Issues/PRs references The change was added to #19050 after testing the PR and before merging. I'm not sure if the fix never worked because of this, or if behavior of `target-export-variables` or GNU Make changed. Co-authored-by: Marian Buschsieweke <marian.buschsieweke@ovgu.de> Co-authored-by: Benjamin Valentin <benjamin.valentin@bht-berlin.de> Co-authored-by: Benjamin Valentin <benjamin.valentin@ml-pa.com>
Contribution description
This restores a pre-existing design decision to implement both blocking and non-blocking mutex locking with the same code. Those implementations have been split prior to the introduction of the
core_mutex_priority_inheritance
module whenmutex_trylock()
indeed was trivial. This decision didn't age well, so undo it.Testing procedure
Let's have the CI run some tests.
Issues/PRs references
None