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 <p...@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
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#48743): https://edk2.groups.io/g/devel/message/48743
Mute This Topic: https://groups.io/mt/34441816/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
