On 23/01/2024 16:55, Laszlo Ersek wrote:
+ ///
+ /// Number of self-tests performed.
+ ///
+ UINTN SelfTestCount;
} NESTED_INTERRUPT_STATE;
/**
I suggest that the new field be UINT32. The (exclusive) limit is a
32-bit PCD. Making the counter (potentially) wider than the limit is not
useful, but it's also a bit of a complication for the debug messages
(see below).
Great suggestion, thanks!
+ ///
+ /// Perform a limited number of self-tests on the first few
+ /// invocations.
+ ///
+ if ((IsrState->DeferredRestoreTPL == FALSE) &&
This comment applies to several locations in the patch:
BOOLEANs should not be checked using explicit "== TRUE" and "== FALSE"
operators / comparisons; they should only be evaluated in their logical
contexts:
We've had this conversation before :)
https://edk2.groups.io/g/devel/message/104369
I personally find that the "!" operators get visually lost in the EDK2
coding style with its very long variable and function names. That said,
I'm happy to omit all of the explicit comparisons, but I should then add
a precursor patch that changes the existing code style first. Thoughts?
+VOID
+NestedInterruptSelfTest (
+ IN NESTED_INTERRUPT_STATE *IsrState
+ )
+{
+ UINTN SelfTestCount;
+ UINTN TimeOut;
Did this pass the uncrustify check? I think uncrustify would insist on
inserting two spaces here.
For running uncrustify locally:
You are right, and thank you for reminding me how to run the EDK2
version of uncrustify. I've fixed up everything it identified.
+ // This error represents a bug in the platform-specific timer
+ // interrupt handler.
+ //
+ DEBUG ((
+ DEBUG_ERROR,
+ "Nested interrupt self-test %u/%u failed: no nested interrupt\n",
+ SelfTestCount,
+ NUMBER_OF_SELF_TESTS
+ ));
+ ASSERT (FALSE);
+}
I'd prefer something stronger than just ASSERT (FALSE) here, but -- per
previous discussion -- we don't have a generally accepted "panic" API
yet, and CpuDeadLoop() is not suitable for all platforms, so this should do.
With my trivial comments addressed:
Acked-by: Laszlo Ersek <ler...@redhat.com>
Comment on the general idea: I much like that the self-test is active on
every boot (without high costs).
Thank you!
Side idea: technically we could merge the first two patches in
separation (pending MdeModulePkg maintainer approval), and then consider
the last three patches as new improvements (possibly needing longer
review). This kind of splitting has both advantages and disadvantages;
the advantage is that the code movement / upstreaming to MdeModulePkg is
not blocked by (somewhat) unrelated discussion. The disadvantages are
that more admin work is needed (more posting, and more PRs), and that
patches in the series that one might consider to belong together will
fly apart in the git history. So I just figured I'd raise the option.
I'm happy to work either way, and shall await instruction.
In the absence of any instruction to the contrary, I'll send out a v4
tomorrow with everyone's suggestions and tags included, but without the
above-mentioned precursor patch to remove the boolean comparisons.
Thanks,
Michael
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114227): https://edk2.groups.io/g/devel/message/114227
Mute This Topic: https://groups.io/mt/103911608/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
