Re: [edk2-platforms][PATCH v2 1/5] Platform/RPi3/RpiFirmwareDxe: Add more query functions

["Leif Lindholm" <leif.lindholm@linaro.org>]

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 <pete@akeo.ie>
> > > ---
> > >   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