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

["Michael Brown" <mcb30@ipxe.org>]

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

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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-