Assert that the Matter stack lock is held while reading/writing attributes #15152
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
pullapprove
bot
requested review from
andy31415,
anush-apple,
austinh0,
balducci-apple,
Byungjoo-Lee,
carol-apple,
chrisdecenzo,
chshu,
chulspro,
Damian-Nordic,
dhrishi,
electrocucaracha,
emargolis,
erjiaqing,
franck-apple,
gjc13,
hawk248,
holbrookt,
harsha-rajendran,
isiu-apple,
jelderton,
jepenven-silabs,
jmartinez-silabs,
kpschoedel,
lazarkov,
LuDuda,
mrjerryjohns and
msandstedt
src/include/platform/internal/GenericPlatformManagerImpl_FreeRTOS.cpp
Outdated
Show resolved
Hide resolved
tcarmelveilleux
approved these changes
Feb 14, 2022
|
PR #15152: Size comparison from 75c7845 to 13e994e Increases (4 builds for k32w, linux)
Full report (8 builds for cyw30739, k32w, linux, telink)
|
|
PR #15152: Size comparison from 75c7845 to 453b331 Increases (16 builds for efr32, esp32, k32w, linux)
Decreases (16 builds for cyw30739, linux, nrfconnect)
Full report (37 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, telink)
|
mrjerryjohns
approved these changes
Feb 14, 2022
|
PR #15152: Size comparison from 75c7845 to 62ebd2f Increases above 0.2%:
Increases (21 builds for efr32, esp32, k32w, linux, p6)
Decreases (37 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
Full report (40 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
msandstedt
approved these changes
Feb 14, 2022
bzbarsky-apple
force-pushed
the
chip-stack-locks
branch
from
62ebd2f to
b07779e
Compare
|
PR #15152: Size comparison from d1c7e85 to 29c87a5 Increases (18 builds for efr32, esp32, k32w, linux)
Full report (40 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, telink)
|
bzbarsky-apple
force-pushed
the
chip-stack-locks
branch
from
29c87a5 to
e5e1a80
Compare
|
PR #15152: Size comparison from 6ad78e2 to e5e1a80 Increases (18 builds for efr32, esp32, k32w, linux)
Full report (43 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
…butes. Several changes here: 1) Add an assert that when attributes are being read/written the Matter stack lock is held, so the read/write does not race with interaction model messages touching the attribute store. 2) Implement the Matter stack lock assertion bits on FreeRTOS. 3) Add FreeRTOS to the set of configurations that actually enable Matter stack lock assertions. 4) For the configurations that enable the assertions, change the default behavior from "log" to "fatal", so we actually notice the
bzbarsky-apple
force-pushed
the
chip-stack-locks
branch
from
e5e1a80 to
939cc1a
Compare
|
PR #15152: Size comparison from a35b95a to 939cc1a Increases (18 builds for efr32, esp32, k32w, linux)
Full report (43 builds for cyw30739, efr32, esp32, k32w, linux, mbed, nrfconnect, p6, qpg, telink)
|
Damian-Nordic
approved these changes
Feb 15, 2022
jamesluo11
pushed a commit
to jamesluo11/connectedhomeip
that referenced
this pull request
Apr 26, 2022
…butes (project-chip#15152) * Assert that the Matter stack lock is held while reading/writing attributes. Several changes here: 1) Add an assert that when attributes are being read/written the Matter stack lock is held, so the read/write does not race with interaction model messages touching the attribute store. 2) Implement the Matter stack lock assertion bits on FreeRTOS. 3) Add FreeRTOS to the set of configurations that actually enable Matter stack lock assertions. 4) For the configurations that enable the assertions, change the default behavior from "log" to "fatal", so we actually notice the * Remove stray debugging log. * Make p6 and qpg CI compile. * Also assert in MatterReportingAttributeChangeCallback.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Several changes here:
Matter stack lock is held, so the read/write does not race with
interaction model messages touching the attribute store.
Matter stack lock assertions.
default behavior from "log" to "fatal", so we actually notice the
Problem
Too easy to (especially) write attributes without holding the Matter lock.
Change overview
Make it harder to miss when that's happening, at least on Linux/Android/Tizen/FreeRTOS
Testing
Verified that the new assert triggers if I place it in
OTARequestor::Initand run the ESP32 ota-requestor app on an m5stack.It does not trigger in the attribute code, because the ESP32 ota-requestor app never reaches that during its init (because it has not started up the data model yet at that point). But the EFR32 ota-requestor (which I don't have a way to test locally) should absolutely trigger this.