Restore original, 2018 mtrace_event(atomic) spin lock logic#4246
Merged
lgirdwood merged 3 commits intothesofproject:mainfrom May 27, 2021
Merged
Restore original, 2018 mtrace_event(atomic) spin lock logic#4246lgirdwood merged 3 commits intothesofproject:mainfrom
lgirdwood merged 3 commits intothesofproject:mainfrom
Conversation
Set the error code in "ret" and only exit the loop so we don't lose the optional warnings at the end of the loop. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Because why not. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
Fixes bad commit b284ac3 ("debugability: Macro Metaprogramming Refactor") that accidentally swapped mtrace_event() and mtrace_event_atomic() Before that accidental inversion, _trace_event_mboxN() functions invoked spin_lock_irq() whereas _trace_even_mbox_atomicN() did NOT take locks. That was consistent with the non-mbox, DMA variants of these functions. As explained in the message of commit 3dca7b7 ("trace: dma: Add atomic and nowait DMA tracing support."), functions with the `_atomic()` suffix,, are meant to be used in atomic contexts where the lock is _already_ taken. Searching for spin_lock_irq() in the code after the bad commit shows that the logic was inverted for the _mbox variants: the lock became used for the _atomic() variants. The reasons why I can't reproduce any actual issue are likely the same reasons why this inversion was never noticed: - no multicore - very little use of trace_.*_atomic variants - spin_lock_irq() is re-entrant? I merely noticed the inconsistency with DMA tracing while trying to solve unrelated issues in the same places. The non-mbox, DMA variants were not affected by the bad commit because for DMA the lock was and is managed in a different file: dma-trace.c. Searching "spin_lock_irq" in later commit b12a662 ("trace: Remove metaprogramming from trace implementation") shows that it preserved this inversion across this second refactor; it left the bug untouched. Signed-off-by: Marc Herbert <marc.herbert@intel.com>
lgirdwood
approved these changes
May 26, 2021
Member
lgirdwood
left a comment
There was a problem hiding this comment.
Last patch will be needed for rc3 and maybe TGL13. @keyonjie @slawblauciak @mwasko
iuliana-prodan
approved these changes
May 26, 2021
Merged
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
2 cosmetic commits + this "atomic" one:
Fixes bad commit b284ac3 ("debugability: Macro Metaprogramming
Refactor") that accidentally swapped mtrace_event() and
mtrace_event_atomic()
Before that accidental inversion, _trace_event_mboxN() functions invoked
spin_lock_irq() whereas _trace_even_mbox_atomicN() did NOT take
locks. That was consistent with the non-mbox, DMA variants of these
functions. As explained in the message of commit 3dca7b7 ("trace:
dma: Add atomic and nowait DMA tracing support."), functions with the
_atomic()suffix,, are meant to be used in atomic contexts where thelock is already taken.
Searching for spin_lock_irq() in the code after the bad commit shows
that the logic was inverted for the _mbox variants: the lock became used
for the _atomic() variants.
The reasons why I can't reproduce any actual issue are likely the same
reasons why this inversion was never noticed:
I merely noticed the inconsistency with DMA tracing while trying to
solve unrelated issues in the same places.
The non-mbox, DMA variants were not affected by the bad commit because
for DMA the lock was and is managed in a different file: dma-trace.c.
Searching "spin_lock_irq" in later commit b12a662 ("trace:
Remove metaprogramming from trace implementation") shows that it
preserved this inversion across this second refactor; it left the bug
untouched.