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

["Pete Batard" <pete@akeo.ie>]

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 <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.

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
>