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.137.1570725823602361554 for ; Thu, 10 Oct 2019 09:43:43 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@linaro.org header.s=google header.b=xCeDImUu; spf=pass (domain: linaro.org, ip: 209.85.221.68, mailfrom: leif.lindholm@linaro.org) Received: by mail-wr1-f68.google.com with SMTP id y19so8741102wrd.3 for ; Thu, 10 Oct 2019 09:43:43 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=0GzJxN1Gf/IAlYcXSP85aczUFDVj3W+9sbRpCot7J50=; b=xCeDImUuuaBIkZzlpcjsx/gA1ZgwQMQgJemtsx+GRH66lLzdA3lXFn82sI4KccNIHI MgNFc3cE1OGw84c7yD+464HtdU/hXzCqXLVttit9WSlJ/B71cZJzU+omL5eh5ZGBZSyA HwRZNNQsPB0hLVRM+POZf+d4PDvQ4h3k5cS/UwvJo04bFJ/7Yx4/zXLJFUtbYPsQz6Cb dFrZW6Wq4r0p0OJqhbTUgEQr7oo7fAaiWZWJXEo0NEIB9DXMECYNkpDjU6ERJIWlm3f6 b0VkQCNcmGjNr+J5zM/PJDC9eFz6Cnn9MB9WqsPF9tigHFbmU3Nfd2u0qLBDLY1xsXJz IMBA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=0GzJxN1Gf/IAlYcXSP85aczUFDVj3W+9sbRpCot7J50=; b=sMkqDmmGlRTuhmiUagVKhKvcnvWkvW3Ep3RDsUbyiRPUW/88b88jVF6rWH3texl4Ta BTQbqXGG4WBr2fq3P3Cz6/K3gGd2rlJrTKGwgLq10u/QCdit6v0zpSCqpIOM6gDSkVpT Lwi8B5q57KClDHZvhCGt4Wbig91XR56jkgGPGiZLjdjDVPiiQyn1xeJmZgxlCbnpKkhA KZvBvVnp3UYUSHykuXLjjQvVGJdU395O8HHUboCbVOBLTwwiS3yHiHYqEgYecAQTWnDh zzVzVDe/N4TH0nevJHU3/mWssKUIoo6XZM7amt1hGiKdTOAvOUVVtKfCiqvkNurmtEz4 Urag== X-Gm-Message-State: APjAAAWs9q+WWNuIHWqRKocaXTaQCprK+IjxWljdUwaSKMTC4R1xUb3d VZutNCPhT7KhPkFSvcr1ZfoN3Q== X-Google-Smtp-Source: APXvYqz5jvXhkelDVwWr4tF38oj3e5GbQYP9KhSGYKZLEgt3+McVH7G8Oxc0B5EcCGjwzD1zN3yVQg== X-Received: by 2002:adf:f90f:: with SMTP id b15mr9378168wrr.76.1570725822066; Thu, 10 Oct 2019 09:43:42 -0700 (PDT) Return-Path: Received: from bivouac.eciton.net (bivouac.eciton.net. [2a00:1098:0:86:1000:23:0:2]) by smtp.gmail.com with ESMTPSA id r10sm7915368wml.46.2019.10.10.09.43.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 10 Oct 2019 09:43:41 -0700 (PDT) Date: Thu, 10 Oct 2019 17:43:39 +0100 From: "Leif Lindholm" To: Pete Batard Cc: devel@edk2.groups.io, ard.biesheuvel@linaro.org Subject: Re: [edk2-platforms][PATCH v2 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions Message-ID: <20191010164339.GO25504@bivouac.eciton.net> References: <20191008123841.12952-1-pete@akeo.ie> <20191008123841.12952-2-pete@akeo.ie> <20191010083948.GS25504@bivouac.eciton.net> MIME-Version: 1.0 In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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. - 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? Am I completely misreading what this code does? Does GetModelRevision() return different values depending on when you call it? Best Regards, Leif