-
-
Notifications
You must be signed in to change notification settings - Fork 178
Add set_watchdog_timer implementation and test #27
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
Conversation
Thanks for your contribution
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 method can return status codes other than Success if the wrong parameters are passed in or the system does not have a watchdog timer, therefore a Result should be propagated.
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.
@GabrielMajeri Already applied some changes on master :)
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.
Gotta go fast! 😄
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 method should probably be unsafe since 1/it takes an unchecked pointer as a parameter and 2/data_size is not related in any way to the watchdog_data pointer so safety-critical caller mistakes can happen here as well.
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.
A safe alternative would be to pass in a slice of u16s.
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 a more minor point, but I'm not sure if the UEFI spec allows you to use 0 as a watchdog code. It does say that codes from 0 to 0xffff are reserved by the implementation...
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.
Firmware is required to set a 5-minute watchdog timer before running an image, setting the code 0 to 0 disables 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.
You can find a more detailed explanation here: https://wiki.osdev.org/UEFI#My_UEFI_application_hangs.2Fresets_after_about_5_minutes
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.
According to the UEFI spec, it is the timeout parameter that must be set to zero to disable the watchdog timer. The WatchdogCode is something that will be logged on a timeout event. Now, since we're disabling the timeout, that will never happen, but an UEFI implementation that carefully validates its inputs could still complain about our use of a reserved code.
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.
The rather hypothetical nature of an UEFI implementation that checks its inputs so carefully is why I classified this as a less pressing issue 😄
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 opened #28 to resolve this.
This implements the
set_watchdog_timerfunction and correspondent test.