Skip to content
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.

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

Draft: sys/ztimer: guard against spurious ISR #20925

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Enoch247
Copy link
Contributor

@Enoch247 Enoch247 commented Oct 18, 2024

Contribution description

This patch changes ztimer_handler() to properly deal with spurious ISRs. Without the patch, a spurious ISR will potentially cause mutliple timers to fire early. With this patch, these ISRs are properly ignored.

The downside of this PR is that it increases the overhead of the ztimer ISR execution. One option would be to only enable it when DEVELHELP is set. Another option is to make this the default behavior, but revert to the old behavior for MCU's where the periph_timer is well tested and known the never cause spurious ISRs. This would ease porting of new MCUs where the timer driver may not be as mature yet.

Testing procedure

Testing procedure is the same as #20924.

Issues/PRs references

This patch changes ztimer_handler() to properly deal with spurious ISRs.
Without the patch, a spurious ISR will potentially cause mutliple timers
to fire early. With this patch, these ISRs are properly ignored.
@github-actions github-actions bot added Area: timers Area: timer subsystems Area: sys Area: System labels Oct 18, 2024
@Enoch247 Enoch247 changed the title sys/ztimer: gaurd against spurious ISR Draft: sys/ztimer: gaurd against spurious ISR Oct 25, 2024
@Enoch247
Copy link
Contributor Author

I am marking this as draft because of the potential performance impact. I need to quantify it. If it is not small, this feature should be made optional. I have had some hints in using this code that the performance impact may not be acceptable as a general solution to all users.

@mguetschow mguetschow marked this pull request as draft October 29, 2024 16:25
@benpicco
Copy link
Contributor

benpicco commented Nov 13, 2024

What is a spurious IRQ and why would they occur?
Sounds more like a bug in the timer driver, not something we should work around in an upper layer, because that will bite every hw timer user, not just ztimer.

@kaspar030 kaspar030 changed the title Draft: sys/ztimer: gaurd against spurious ISR Draft: sys/ztimer: guard against spurious ISR Nov 13, 2024
@Enoch247
Copy link
Contributor Author

Spurious IRQs are jumps into the ISR that are unwanted/unexpected. A bug could be a likely cause, but the term can also be applied (as I understand it) in situations that arrive due to sequential code interacting with parallel hardware.

I made this change to work around spurious IRQs into ztimer that I was seeing. I later figured out the root cause and fixed that in PR #20924. In that same investigation, I discovered that there was another possible source of spurious ISRs and that proposed fix is in #20926.

I am fine with dropping this PR outright, or making it a pseudomodule, since it is a workaround. I think @maribu was in favor of the change, so may want to give him a chance to speak up before closing it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System Area: timers Area: timer subsystems
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants