From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-wr1-f68.google.com (mail-wr1-f68.google.com [209.85.221.68]) by mx.groups.io with SMTP id smtpd.web10.582.1570728473151366123 for ; Thu, 10 Oct 2019 10:27:53 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@akeo-ie.20150623.gappssmtp.com header.s=20150623 header.b=rMy+2rll; spf=none, err=permanent DNS error (domain: akeo.ie, ip: 209.85.221.68, mailfrom: pete@akeo.ie) Received: by mail-wr1-f68.google.com with SMTP id z9so8878841wrl.11 for ; Thu, 10 Oct 2019 10:27:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=akeo-ie.20150623.gappssmtp.com; s=20150623; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=B86VR6sP9TXik++Y8ZWBj4wHhhxoeIpnSweiWPYDSCM=; b=rMy+2rll5D5FP1mDzYQDgx2HD3BrUV8rc0kHtTneHKrdcudZFbEuXrsa0KBbtUcHGD jKC6FHN0HY/7T3ji2fhFD5R40mF54PwaFig5SFrOZMjkf5bVjc06ZyYliEFPms5H1en3 j5VHDqJrd6BiXB63O6ngAjESFCaIE8RjL4/4H/ZorGzH/HkUw2GWiIrnLEk5ZXt1L/BB tn2VIXYNaTZ2txs4KY+p1R4rRH7P9fgfL6xp42c4v+edonYnZpcxyZ/YpbY0GVmHj1EW dj/SpHA7o8boU4IUu0sggPNusRjb/kdjmMBW+lArps1v7ZrI9hlkNUqxGt0uL/Mj1y7K kPsg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=B86VR6sP9TXik++Y8ZWBj4wHhhxoeIpnSweiWPYDSCM=; b=RAGqjl2/6zHqKFibHViFgMEmNsbMVNyAiCav4feS8Yt3FPaZfsEeZtPQuBqeOSeJej FBFBeUcKENuUlgKG6rY3dyGCsj3I7qtuPKrxn5dtYuuyxWnsR91vAxGgK/kzpOA+qXqJ NV1uO+r+cpVJhm1bFmeG8JD30ew1d62D/5Xfusj1Jg8XV8G7sIshp3d6MXX4f6A2hiBg DtGzKGa0MiQNZ4SIIdiw7izTUsPhbI6ItQ4psuvOUZelitOFE8GbqJ4gP2eDBh4oTI46 qdSxsI082pMHzlujyhGx/xpTqnLGkWcCiki4ulx19rrKqJvhZnNe8inCW6941PYzN9Dt aPhw== X-Gm-Message-State: APjAAAVHEUKdey/gue60Rmpx5P3P0cIuzf0hIrJYC/WZUbxlD0V9Frgg brp1McecJL1JQv4fIPXhQPt9+w== X-Google-Smtp-Source: APXvYqxnssrUFnSf9AF9fAR8Mquq04PqVre599IIXpn5fYlEZTXnABsDoY9JbLtWNM1EQWKVNhkXCw== X-Received: by 2002:a5d:4302:: with SMTP id h2mr9990079wrq.35.1570728471510; Thu, 10 Oct 2019 10:27:51 -0700 (PDT) Return-Path: Received: from [10.0.0.122] ([84.203.40.122]) by smtp.googlemail.com with ESMTPSA id i11sm9148882wrq.48.2019.10.10.10.27.49 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 10 Oct 2019 10:27:50 -0700 (PDT) Subject: Re: [edk2-platforms][PATCH v2 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions To: Leif Lindholm Cc: devel@edk2.groups.io, ard.biesheuvel@linaro.org References: <20191008123841.12952-1-pete@akeo.ie> <20191008123841.12952-2-pete@akeo.ie> <20191010083948.GS25504@bivouac.eciton.net> <20191010164339.GO25504@bivouac.eciton.net> From: "Pete Batard" Message-ID: Date: Thu, 10 Oct 2019 18:27:49 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <20191010164339.GO25504@bivouac.eciton.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit On 2019.10.10 17:43, Leif Lindholm wrote: > On Thu, Oct 10, 2019 at 01:41:06PM +0100, Pete Batard wrote: >> Hi Leif, >> >> On 2019.10.10 09:39, Leif Lindholm wrote: >>> On Tue, Oct 08, 2019 at 01:38:37PM +0100, Pete Batard wrote: >>>> This patch introduces the capability to also query the Model Name/ >>>> Manufacturer Name/CPU Name/Firmware Revision using the RpiFirmware >>>> protocol. This is aims at making the driver more suitable to cater >>>> for platforms other than the Raspberry Pi 3 as well as simplifying >>>> the population of entries in PlatformSmbiosDxe. >>>> >>>> Signed-off-by: Pete Batard >>>> --- >>>> Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 155 +++++++++++++++++++- >>>> Platform/RaspberryPi/RPi3/Include/Protocol/RpiFirmware.h | 58 ++++++-- >>>> 2 files changed, 196 insertions(+), 17 deletions(-) >>>> >>>> diff --git a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c >>>> index 9b5ee1946279..378c99bcba05 100644 >>>> --- a/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c >>>> +++ b/Platform/RaspberryPi/RPi3/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c >>>> @@ -461,7 +461,7 @@ typedef struct { >>>> RPI_FW_TAG_HEAD TagHead; >>>> RPI_FW_MODEL_REVISION_TAG TagBody; >>>> UINT32 EndTag; >>>> -} RPI_FW_GET_MODEL_REVISION_CMD; >>>> +} RPI_FW_GET_REVISION_CMD; >>>> #pragma pack() >>>> STATIC >>>> @@ -471,7 +471,7 @@ RpiFirmwareGetModelRevision ( >>>> OUT UINT32 *Revision >>>> ) >>>> { >>>> - RPI_FW_GET_MODEL_REVISION_CMD *Cmd; >>>> + RPI_FW_GET_REVISION_CMD *Cmd; >>>> EFI_STATUS Status; >>>> UINT32 Result; >>>> @@ -506,6 +506,153 @@ RpiFirmwareGetModelRevision ( >>>> return EFI_SUCCESS; >>>> } >>>> +STATIC >>>> +EFI_STATUS >>>> +EFIAPI >>>> +RpiFirmwareGetFirmwareRevision ( >>>> + OUT UINT32 *Revision >>>> + ) >>>> +{ >>>> + RPI_FW_GET_REVISION_CMD *Cmd; >>>> + EFI_STATUS Status; >>>> + UINT32 Result; >>>> + >>>> + if (!AcquireSpinLockOrFail (&mMailboxLock)) { >>>> + DEBUG ((DEBUG_ERROR, "%a: failed to acquire spinlock\n", __FUNCTION__)); >>>> + return EFI_DEVICE_ERROR; >>>> + } >>>> + >>>> + Cmd = mDmaBuffer; >>>> + ZeroMem (Cmd, sizeof (*Cmd)); >>>> + >>>> + Cmd->BufferHead.BufferSize = sizeof (*Cmd); >>>> + Cmd->BufferHead.Response = 0; >>>> + Cmd->TagHead.TagId = RPI_MBOX_GET_REVISION; >>>> + Cmd->TagHead.TagSize = sizeof (Cmd->TagBody); >>>> + Cmd->TagHead.TagValueSize = 0; >>>> + Cmd->EndTag = 0; >>>> + >>>> + Status = MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_CHANNEL, &Result); >>>> + >>>> + ReleaseSpinLock (&mMailboxLock); >>>> + >>>> + if (EFI_ERROR (Status) || >>>> + Cmd->BufferHead.Response != RPI_MBOX_RESP_SUCCESS) { >>>> + DEBUG ((DEBUG_ERROR, >>>> + "%a: mailbox transaction error: Status == %r, Response == 0x%x\n", >>>> + __FUNCTION__, Status, Cmd->BufferHead.Response)); >>>> + return EFI_DEVICE_ERROR; >>>> + } >>>> + >>>> + *Revision = Cmd->TagBody.Revision; >>>> + return EFI_SUCCESS; >>>> +} >>>> + >>>> +STATIC >>>> +CHAR8* >>>> +EFIAPI >>>> +RpiFirmwareGetModelName ( >>>> + IN INTN ModelId >>>> + ) >>>> +{ >>>> + UINT32 Revision; >>>> + >>>> + // If a negative ModelId is passed, detect it. >>>> + if ((ModelId < 0) && (RpiFirmwareGetModelRevision (&Revision) == EFI_SUCCESS)) >>> >>> Style-wise, please always use {} with if-statements. >> >> Will do. >> >>> Beyond that, this pattern repeats identcally in three functions in >>> this file. Meanwhile, there is never any error handling of >>> RpiFirmwareGetModelRevision other than if not successful, we leave it >>> as negative. >> >> Yes, because then we'll get the default case (that returns "Unknown ..."), >> which is precisely what we want if we can't access the model revision for >> any reason. >> >>> Are there other intended uses of that function, or could >>> we move this logic there? >> >> I'm not sure how that would work, since ModelId, ManufacturerId, and CpuId >> are for different parts of the bit fields, so, if I understand what you're >> suggesting correctly (but I'm not really sure I do) trying to move the check >> for negative value into RpiFirmwareGetModelRevision () wouldn't work that >> great. Plus then the function name would become a misnommer. >> >> Are you really seeing a functional issue with the current code, or something >> that would make a possible contributor wanting to modify this section >> puzzled as to what we're trying to accomplish here (which I think is pretty >> clear, but then again, I wrote the code so maybe I'm not the most impartial? >> Because if not, I'd rather save my limited supply of time, and just go with >> a v3 that only adds the {} suggested. > > I see no functional issues with the code, but with my own limited > supply of time I try to ensure the code stays as simple as possible so > I don't need to stop and think where there isn't actually anything > complicated going on (like here). I am after all a bear of very little > brain. > > I also will *always* trigger on seeing a near-identical pattern > repeated many times in the same file. > > In this instance, I find myself spending way too much time untangling > what is actually going on. I see > - SysInfoUpdateSmbiosType1 () calling mFwProtocol->GetModelRevision (), > and then using the result from that to determine which parameters to > pass to mFwProtocol->GetModelName() and > mFwProtocol->GetManufacturerName(). > - Both of which if the GetModelRevision () call was unsuccessful get > passed -1. The whole point is to create new generic calls that may be used by any driver that needs them, and not only PlatformSmbiosDxe. That's our starting point. And the idea is that, unlike what currently happens in SysInfoUpdateSmbiosType1 (), not all of these calls may also happen to be calling GetModelRevision (). As such, I believe it makes a lot of sense to have the new calls designed as they are, else we find ourselves trying to cover the case of minimizing calls to GetModelRevision (), either in PlatformSmbiosDxe or RpiFirmwareDxe, which seems an unwarranted micro optimization goal to me. > - Now, when the parameter is -1, they both call straight into > GetModelRevision (). Why are we expecting the second call to succeed > when the first one fails? We're not expecting anything. We're receiving a call from a driver, of which we have no idea what actions it performed beforehand, that may happens to ask us to replicate some of these actions. And, in a generic manner, and regardless of a state we should not have any idea about due to encapsulation, we do issue a call to GetModelRevision (). Or, let me put it this way: From PlatformSmbiosDxe:SysInfoUpdateSmbiosType1 ()'s perspective, the call to RpiFirmwareDxe:GetModelName () is supposed to be a black box that returns a result it can use. And from RpiFirmwareDxe:GetModelName ()'s perspective, what potentially duplicated actions the caller did beforehand is also a black box. Ideally, this is intended to make both drivers, with their better abstracted goals, easier to understand and maintain, so I can't help but feel quite dismayed that it's the very opposite that appears to be occurring here. Still, if anything, I would assert that any action that aims at reducing calls to GetModelRevision () should be reported against PlatformSmbiosDxe rather than the lower level (and therefore more generic) RpiFirmwareDxe. Regards, /Pete > Am I completely misreading what this code does? > > Does GetModelRevision() return different values depending on when you > call it? > > Best Regards, > > Leif >