Re: [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib

["Laszlo Ersek" <lersek@redhat.com>]

On 1/23/24 18:41, Michael Brown wrote:
> 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

Oops, sorry :)

> 
> 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?

I'm unsure.

Some people prefer (! Expression) over (!Expression), i.e., the extra
space, but I'm unsure if uncrustify tolerates that, and whether it
matches the edk2 coding style, regardless of uncrustify. How about:

  !(Expression)

such as

  if (!(IsrState->DeferredRestoreTPL) &&

? Does that make it more readable? (I believe this form would not trip
up uncrustify, and that it would satisfy the coding style too.)

With none of those working, the explicit ==TRUE / ==FALSE would not
"break" the coding style any stronger either, so in that case, feel free
(from my side) to just stick with the syntax in the patch (and in the
pre-patch code).

> 
>>> +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 <lersek@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.

A v4 like that sounds good (as far as I'm aware, the first two patches
have no maintainer review yet, anyway).

Thanks!
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114289): https://edk2.groups.io/g/devel/message/114289
Mute This Topic: https://groups.io/mt/103911608/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-