From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 76AD7AC11D6 for ; Wed, 24 Jan 2024 12:58:40 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=5xZyP3QwBgFWhMxsflNFbmmDIW/LaQ/h49YBdqNIO18=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1706101119; v=1; b=Qip/VaSf3ZdZKBj1iQokErbLwna8YnL71tkx0QqnBvhUgL/agFPX2zh2nXamCQZPI2U2yuNl WT12rTsnOKG9bF2EGgwVYrMHz73eh7jD1Ttzo2amlurnOsiBouwGczbIAG9NOQTJM+t5tAXdtwv fw9HNCRtwvlpFpyTmALhfOE8= X-Received: by 127.0.0.2 with SMTP id o8QkYY7687511x6u6EalIMBW; Wed, 24 Jan 2024 04:58:39 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.21836.1706101118437746114 for ; Wed, 24 Jan 2024 04:58:38 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-518-RWLJJn4mPk6PcjgNT_R9kA-1; Wed, 24 Jan 2024 07:58:36 -0500 X-MC-Unique: RWLJJn4mPk6PcjgNT_R9kA-1 X-Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id E616A101A526; Wed, 24 Jan 2024 12:58:35 +0000 (UTC) X-Received: from [10.39.195.127] (unknown [10.39.195.127]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 2613E2166B32; Wed, 24 Jan 2024 12:58:35 +0000 (UTC) Message-ID: Date: Wed, 24 Jan 2024 13:58:34 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib To: Michael Brown , devel@edk2.groups.io Cc: Ray Ni , Gerd Hoffmann References: <17ACFF3FDD20CD9A.13754@groups.io> <20240123153104.2451759-1-mcb30@ipxe.org> <0102018d36f28154-cb2f6e0a-93ea-490d-96bd-5c804c984e2a-000000@eu-west-1.amazonses.com> <0102018d376a18e4-553f2c8e-20e7-4a27-a34e-4a00ac8cfd19-000000@eu-west-1.amazonses.com> From: "Laszlo Ersek" In-Reply-To: <0102018d376a18e4-553f2c8e-20e7-4a27-a34e-4a00ac8cfd19-000000@eu-west-1.amazonses.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: sFrUAqLXJBxWsvpe7CU6mqInx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="Qip/VaSf"; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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 >> >> 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] -=-=-=-=-=-=-=-=-=-=-=-