From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=2607:f8b0:4001:c06::241; helo=mail-io0-x241.google.com; envelope-from=ard.biesheuvel@linaro.org; receiver=edk2-devel@lists.01.org Received: from mail-io0-x241.google.com (mail-io0-x241.google.com [IPv6:2607:f8b0:4001:c06::241]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 53CF6210E0F98 for ; Mon, 18 Jun 2018 10:01:26 -0700 (PDT) Received: by mail-io0-x241.google.com with SMTP id u4-v6so17489239iof.2 for ; Mon, 18 Jun 2018 10:01:26 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=1WvMZ7aVND1J+hKXziDwcOpW61eItZHPae4/9g1eQLw=; b=XCkG+//obQrJ847ma+4U/QR5zhOkGno8lJjLT6qBTMbDNtGiEw5lCBSRso/+uIbfsw SoFDNKuUUWF7sC2Q+nrHSgfty8aWYOuuMjjdKOXvZcca6dhQk3bJycrip4M1Otg+yVfH /UiPI5Gat1BFLuJ117h5RaetMP0BVZKRHi96w= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=1WvMZ7aVND1J+hKXziDwcOpW61eItZHPae4/9g1eQLw=; b=n1uU/e+ZMq1WL6mNsQzpaWRxvk8fEvrI6x+LLenN8IsAZ+0ZjVKc/o51iaNGII+uL4 AyS9l29G9o9j1wIG1GHIDxaDxmA85D+V9envRNSBqJ8zaOvos4hJahBGzDSF16rjKnwo gx3FdXNauZSF9JnJnNRciBrZTpcCwDLm+f66wk2ngkcZrwMSsyLiUi2R6Fl1jc8Yc9ns W4L/mY8Z87nvs5WnvqZvLH8ir2U0vEc78vzcxbWsuW4fATXvkylLATRzufs2lxWAsYR4 Lv/AfvlhbM1So6vsFDjhS9qhXsGJXyqqyBULlYq/QaEXw8I1/LREKkFU39dRQgX3mxgx G9fg== X-Gm-Message-State: APt69E37gDdXejc9AQByzD/0P4DYqB0ih1+QxpQj2136PiPwgbtztW6x udervc9xsoRK4l9Xv8lD2unbMvUqybjIvbgMJhxQrg== X-Google-Smtp-Source: ADUXVKJqAlY7qmuPE6jviushARozstNKUiGrPpTROk0i/GdPDnA9LL4U9Mnfa/wvY6qtUkK17xbnmpbFusG4N2e/iAc= X-Received: by 2002:a6b:520d:: with SMTP id g13-v6mr11359626iob.60.1529341286181; Mon, 18 Jun 2018 10:01:26 -0700 (PDT) MIME-Version: 1.0 Received: by 2002:a6b:bbc7:0:0:0:0:0 with HTTP; Mon, 18 Jun 2018 10:01:25 -0700 (PDT) In-Reply-To: <20180615141020.9428-2-girish.pathak@arm.com> References: <20180615141020.9428-1-girish.pathak@arm.com> <20180615141020.9428-2-girish.pathak@arm.com> From: Ard Biesheuvel Date: Mon, 18 Jun 2018 19:01:25 +0200 Message-ID: To: Girish Pathak Cc: "edk2-devel@lists.01.org" , Leif Lindholm , Matteo Carlini , Stephanie Hughes-Fitt , nd Subject: Re: [PATCH v1 1/2] ArmPkg/ArmScmiDxe: Fix ASSERT error in SCMI DXE X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 18 Jun 2018 17:01:27 -0000 Content-Type: text/plain; charset="UTF-8" Hello Girish, On 15 June 2018 at 16:10, Girish Pathak wrote: > This change fixes a bug in the SCMI DXE which is observed with the > upcoming release of the SCP firmware. > > The PROTOCOL_ID_MASK (0xF) which is used to generate an index in > the ProtocolInitFxns is wrong because protocol ids can be > anywhere in 0x10 - 15 or 0x80 - FF range. This mask generates > the same index for two different protocols e.g. for protocol ids > 0x10 and 0x90, which causes duplicate initialization of a protocol > resulting in a failure. > > This change removes the use of PROTOCOL_ID_MASK and instead > uses a list of protocol ids and their initialization functions > to identify a supported protocol and initialize it. > > Change-Id: Ib004294d2bec853045ed6e811d123832fba555cd > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Girish Pathak > --- > ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c | 35 ++++++++++---------- > ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.h | 8 +++-- > 2 files changed, 22 insertions(+), 21 deletions(-) > > diff --git a/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c b/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c > index 2920c6f6f33c5bb8ac00c903a0b199ba5f06f4de..cc68cbc922fcc06bff8f7e0aa8b6bf64a9932874 100644 > --- a/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c > +++ b/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.c > @@ -29,13 +29,10 @@ > #include "ScmiDxe.h" > #include "ScmiPrivate.h" > > -STATIC CONST SCMI_PROTOCOL_INIT_TABLE ProtocolInitFxns[MAX_PROTOCOLS] = { > - { ScmiBaseProtocolInit }, > - { NULL }, > - { NULL }, > - { ScmiPerformanceProtocolInit }, > - { ScmiClockProtocolInit }, > - { NULL } > +STATIC CONST SCMI_PROTOCOL_ENTRY Protocols[] = { > + { SCMI_PROTOCOL_ID_BASE, ScmiBaseProtocolInit }, > + { SCMI_PROTOCOL_ID_PERFORMANCE, ScmiPerformanceProtocolInit }, > + { SCMI_PROTOCOL_ID_CLOCK, ScmiClockProtocolInit } > }; > > /** ARM SCMI driver entry point function. > @@ -65,14 +62,14 @@ ArmScmiDxeEntryPoint ( > UINT32 Version; > UINT32 Index; > UINT32 NumProtocols; > - UINT32 ProtocolNo; > + UINT32 ProtocolIndex = 0; We don't permit initialized automatic variables. > UINT8 SupportedList[MAX_PROTOCOLS]; > UINT32 SupportedListSize = sizeof (SupportedList); This applies here as well, but it slipped through the cracks last time. Care to fix this one as well? Thanks. > > - ProtocolNo = SCMI_PROTOCOL_ID_BASE & PROTOCOL_ID_MASK; > - > // Every SCMI implementation must implement the base protocol. > - Status = ProtocolInitFxns[ProtocolNo].Init (&ImageHandle); > + ASSERT (Protocols[ProtocolIndex].Id == SCMI_PROTOCOL_ID_BASE); > + Just use Protocols[0].Id here instead of obfuscating the code with a var reference known to be == 0 > + Status = Protocols[ProtocolIndex].InitFxn (&ImageHandle); Same here. In fact, why not just call ScmiBaseProtocolInit() here? > if (EFI_ERROR (Status)) { > ASSERT (FALSE); > return Status; > @@ -123,13 +120,15 @@ ArmScmiDxeEntryPoint ( > } > > // Install supported protocol on ImageHandle. > - for (Index = 0; Index < NumProtocols; Index++) { > - ProtocolNo = SupportedList[Index] & PROTOCOL_ID_MASK; > - if (ProtocolInitFxns[ProtocolNo].Init != NULL) { > - Status = ProtocolInitFxns[ProtocolNo].Init (&ImageHandle); > - if (EFI_ERROR (Status)) { > - ASSERT (FALSE); > - return Status; > + while (++ProtocolIndex < ARRAY_SIZE (Protocols)) { Please use a for () loop here, including the 0 assignment that replaces the initializer above. > + for (Index = 0; Index < NumProtocols; Index++) { > + if (Protocols[ProtocolIndex].Id == SupportedList[Index]) { > + Status = Protocols[ProtocolIndex].InitFxn (&ImageHandle); > + if (EFI_ERROR (Status)) { > + ASSERT (FALSE); In general, ASSERT_EFI_ERROR () is really much more useful than ASSERT (FALSE), because the DEBUG output will tell you the actual value of Status. Just a tip. > + return Status; > + } > + break; > } > } > } > diff --git a/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.h b/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.h > index 29cdde173659c701116b021a3c437a92b473e4e5..5815e1e78074067760b6f618e248526ee25e59c8 100644 > --- a/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.h > +++ b/ArmPkg/Drivers/ArmScmiDxe/ScmiDxe.h > @@ -17,8 +17,9 @@ > #ifndef SCMI_DXE_H_ > #define SCMI_DXE_H_ > > +#include "ScmiPrivate.h" > + > #define MAX_PROTOCOLS 6 > -#define PROTOCOL_ID_MASK 0xF > #define MAX_VENDOR_LEN SCMI_MAX_STR_LEN > > /** Pointer to protocol initialization function. > @@ -35,7 +36,8 @@ EFI_STATUS > ); > > typedef struct { > - SCMI_PROTOCOL_INIT_FXN Init; > -} SCMI_PROTOCOL_INIT_TABLE; > + SCMI_PROTOCOL_ID Id; // Protocol Id. > + SCMI_PROTOCOL_INIT_FXN InitFxn; // Protocol init function. Could you replace this with InitFn please? Fxn as an abbreviation for function is not idiomatic in any of the projects I have ever been involved in, so let's not confuse people unnecessarily. > +} SCMI_PROTOCOL_ENTRY; > > #endif /* SCMI_DXE_H_ */ > -- > 'Guid(CE165669-3EF3-493F-B85D-6190EE5B9759)' > >