[edk2-devel] [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data
Andrei Warkentin
awarkentin at vmware.com
Sat Oct 2 01:17:27 UTC 2021
Yikes - that's embarrassing. Thanks for the fix.
Reviewed-by: Andrei Warkentin <awarkentin at vmware.com>
________________________________
From: Jeremy Linton <jeremy.linton at arm.com>
Sent: Friday, October 1, 2021 7:52 PM
To: devel at edk2.groups.io <devel at edk2.groups.io>
Cc: pete at akeo.ie <pete at akeo.ie>; ardb+tianocore at kernel.org <ardb+tianocore at kernel.org>; leif at nuviainc.com <leif at nuviainc.com>; Andrei Warkentin <awarkentin at vmware.com>; Sunny.Wang at arm.com <Sunny.Wang at arm.com>; samer.el-haj-mahmoud at arm.com <samer.el-haj-mahmoud at arm.com>; Jeremy Linton <jeremy.linton at arm.com>
Subject: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data
While debugging problems with the GET/SET_CLOCK mailbox calls it appeared
that the locking in most of the mailbox commands isn't perfectly
correct. All UEFI firmware calls to the RPi mailbox share a single
mDmaBuffer which is used to fill out the command passed to the vc firmware,
and record its response. The buffer is protected by mMailboxLock, yet in
many cases the result of the request is copied from the buffer after the
lock has been released. This doesn't currently appear to be causing any
problems, but should probably be fixed anyway.
There are a couple other minor tweaks in this patch that are hard to
justify on their own, one is a bit of whitespace cleanup, and the other is
the addition of a debug message to print the returned clock rate for the
requested clock. This latter print would have immediatly shown that the vc
firmware was returning 0 as the emmc clock rate rather than something
reasonable.
Signed-off-by: Jeremy Linton <jeremy.linton at arm.com>
---
.../Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 102 ++++++++++++---------
1 file changed, 59 insertions(+), 43 deletions(-)
diff --git a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
index bf74148bbb..29719aa5ec 100644
--- a/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
+++ b/Platform/RaspberryPi/Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c
@@ -203,7 +203,6 @@ RpiFirmwareSetPowerState (
Status = MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_CHANNEL, &Result);
- ReleaseSpinLock (&mMailboxLock);
if (EFI_ERROR (Status) ||
Cmd->BufferHead.Response != RPI_MBOX_RESP_SUCCESS) {
@@ -219,6 +218,7 @@ RpiFirmwareSetPowerState (
__FUNCTION__, PowerState ? "en" : "dis", DeviceId));
Status = EFI_DEVICE_ERROR;
}
+ ReleaseSpinLock (&mMailboxLock);
return Status;
}
@@ -266,18 +266,20 @@ RpiFirmwareGetArmMemory (
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));
+ ReleaseSpinLock (&mMailboxLock);
return EFI_DEVICE_ERROR;
}
*Base = Cmd->TagBody.Base;
*Size = Cmd->TagBody.Size;
+ ReleaseSpinLock (&mMailboxLock);
+
return EFI_SUCCESS;
}
@@ -323,17 +325,18 @@ RpiFirmwareGetMacAddress (
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));
+ ReleaseSpinLock (&mMailboxLock);
return EFI_DEVICE_ERROR;
}
CopyMem (MacAddress, Cmd->TagBody.MacAddress, sizeof (Cmd->TagBody.MacAddress));
+ ReleaseSpinLock (&mMailboxLock);
+
return EFI_SUCCESS;
}
@@ -378,17 +381,17 @@ RpiFirmwareGetSerial (
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));
+ ReleaseSpinLock (&mMailboxLock);
return EFI_DEVICE_ERROR;
}
*Serial = Cmd->TagBody.Serial;
+ ReleaseSpinLock (&mMailboxLock);
// Some platforms return 0 or 0x0000000010000000 for serial.
// For those, try to use the MAC address.
if ((*Serial == 0) || ((*Serial & 0xFFFFFFFF0FFFFFFFULL) == 0)) {
@@ -441,17 +444,18 @@ RpiFirmwareGetModel (
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));
+ ReleaseSpinLock (&mMailboxLock);
return EFI_DEVICE_ERROR;
}
*Model = Cmd->TagBody.Model;
+ ReleaseSpinLock (&mMailboxLock);
+
return EFI_SUCCESS;
}
@@ -496,17 +500,18 @@ RpiFirmwareGetModelRevision (
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));
+ ReleaseSpinLock (&mMailboxLock);
return EFI_DEVICE_ERROR;
}
*Revision = Cmd->TagBody.Revision;
+ ReleaseSpinLock (&mMailboxLock);
+
return EFI_SUCCESS;
}
@@ -538,17 +543,18 @@ RpiFirmwareGetFirmwareRevision (
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));
+ ReleaseSpinLock (&mMailboxLock);
return EFI_DEVICE_ERROR;
}
*Revision = Cmd->TagBody.Revision;
+ ReleaseSpinLock (&mMailboxLock);
+
return EFI_SUCCESS;
}
@@ -831,18 +837,19 @@ RpiFirmwareGetFbSize (
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));
+ ReleaseSpinLock (&mMailboxLock);
return EFI_DEVICE_ERROR;
}
*Width = Cmd->TagBody.Width;
*Height = Cmd->TagBody.Height;
+ ReleaseSpinLock (&mMailboxLock);
+
return EFI_SUCCESS;
}
@@ -872,16 +879,18 @@ RpiFirmwareFreeFb (VOID)
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));
+ ReleaseSpinLock (&mMailboxLock);
return EFI_DEVICE_ERROR;
}
+ ReleaseSpinLock (&mMailboxLock);
+
return EFI_SUCCESS;
}
@@ -935,19 +944,20 @@ RpiFirmwareAllocFb (
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));
+ ReleaseSpinLock (&mMailboxLock);
return EFI_DEVICE_ERROR;
}
*Pitch = Cmd->Pitch.Pitch;
*FbBase = Cmd->AllocFb.AlignmentBase - BCM2836_DMA_DEVICE_OFFSET;
*FbSize = Cmd->AllocFb.Size;
+ ReleaseSpinLock (&mMailboxLock);
+
return EFI_SUCCESS;
}
@@ -999,13 +1009,12 @@ RpiFirmwareGetCommmandLine (
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));
+ ReleaseSpinLock (&mMailboxLock);
return EFI_DEVICE_ERROR;
}
@@ -1013,6 +1022,7 @@ RpiFirmwareGetCommmandLine (
if (Cmd->TagHead.TagValueSize >= BufferSize &&
Cmd->CommandLine[Cmd->TagHead.TagValueSize - 1] != '\0') {
DEBUG ((DEBUG_ERROR, "%a: insufficient buffer size\n", __FUNCTION__));
+ ReleaseSpinLock (&mMailboxLock);
return EFI_OUT_OF_RESOURCES;
}
@@ -1026,6 +1036,7 @@ RpiFirmwareGetCommmandLine (
CommandLine[Cmd->TagHead.TagValueSize] = '\0';
}
+ ReleaseSpinLock (&mMailboxLock);
return EFI_SUCCESS;
}
@@ -1075,18 +1086,20 @@ RpiFirmwareSetClockRate (
Cmd->TagBody.SkipTurbo = SkipTurbo ? 1 : 0;
Cmd->EndTag = 0;
+ DEBUG ((DEBUG_ERROR, "%a: Request clock rate %X = %d\n", __FUNCTION__, ClockId, ClockRate));
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));
+ ReleaseSpinLock (&mMailboxLock);
return EFI_DEVICE_ERROR;
}
+ ReleaseSpinLock (&mMailboxLock);
+
return EFI_SUCCESS;
}
@@ -1131,20 +1144,23 @@ RpiFirmwareGetClockRate (
Cmd->TagHead.TagValueSize = 0;
Cmd->TagBody.ClockId = ClockId;
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));
+ ReleaseSpinLock (&mMailboxLock);
return EFI_DEVICE_ERROR;
}
*ClockRate = Cmd->TagBody.ClockRate;
+ ReleaseSpinLock (&mMailboxLock);
+
+ DEBUG ((DEBUG_ERROR, "%a: Get Clock Rate return: ClockRate=%d ClockId=%X\n", __FUNCTION__, *ClockRate, ClockId));
+
return EFI_SUCCESS;
}
@@ -1191,7 +1207,7 @@ RpiFirmwareGetMinClockRate (
{
return RpiFirmwareGetClockRate (ClockId, RPI_MBOX_GET_MIN_CLOCK_RATE, ClockRate);
}
-
+
#pragma pack()
typedef struct {
UINT32 ClockId;
@@ -1236,16 +1252,17 @@ RpiFirmwareSetClockState (
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));
+ ReleaseSpinLock (&mMailboxLock);
return EFI_DEVICE_ERROR;
}
+ ReleaseSpinLock (&mMailboxLock);
+
return EFI_SUCCESS;
}
@@ -1297,16 +1314,15 @@ RpiFirmwareSetGpio (
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));
}
+ ReleaseSpinLock (&mMailboxLock);
}
-
+
STATIC
VOID
EFIAPI
@@ -1361,8 +1377,6 @@ RpiFirmwareNotifyXhciReset (
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,
@@ -1370,6 +1384,8 @@ RpiFirmwareNotifyXhciReset (
__FUNCTION__, Status, Cmd->BufferHead.Response));
}
+ ReleaseSpinLock (&mMailboxLock);
+
return Status;
}
@@ -1424,8 +1440,6 @@ RpiFirmwareNotifyGpioGetCfg (
*Polarity = Cmd->TagBody.Polarity;
- ReleaseSpinLock (&mMailboxLock);
-
if (EFI_ERROR (Status) ||
Cmd->BufferHead.Response != RPI_MBOX_RESP_SUCCESS) {
DEBUG ((DEBUG_ERROR,
@@ -1433,6 +1447,8 @@ RpiFirmwareNotifyGpioGetCfg (
__FUNCTION__, Status, Cmd->BufferHead.Response));
}
+ ReleaseSpinLock (&mMailboxLock);
+
return Status;
}
@@ -1471,8 +1487,8 @@ RpiFirmwareNotifyGpioSetCfg (
Status = RpiFirmwareNotifyGpioGetCfg (Gpio, &Result);
if (EFI_ERROR (Status)) {
- DEBUG ((DEBUG_ERROR, "%a: Failed to get GPIO polarity\n", __FUNCTION__));
- Result = 0; //default polarity
+ DEBUG ((DEBUG_ERROR, "%a: Failed to get GPIO polarity\n", __FUNCTION__));
+ Result = 0; //default polarity
}
@@ -1488,7 +1504,7 @@ RpiFirmwareNotifyGpioSetCfg (
Cmd->BufferHead.Response = 0;
Cmd->TagHead.TagId = RPI_MBOX_SET_GPIO_CONFIG;
Cmd->TagHead.TagSize = sizeof (Cmd->TagBody);
-
+
Cmd->TagBody.Gpio = 128 + Gpio;
Cmd->TagBody.Direction = Direction;
Cmd->TagBody.Polarity = Result;
@@ -1501,17 +1517,17 @@ RpiFirmwareNotifyGpioSetCfg (
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));
}
-
- RpiFirmwareSetGpio (Gpio,!State);
-
+
+ ReleaseSpinLock (&mMailboxLock);
+
+ RpiFirmwareSetGpio (Gpio,!State);
+
return Status;
}
@@ -1540,7 +1556,7 @@ STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL mRpiFirmwareProtocol = {
RPiFirmwareGetModelInstalledMB,
RpiFirmwareNotifyXhciReset,
RpiFirmwareGetCurrentClockState,
- RpiFirmwareSetClockState,
+ RpiFirmwareSetClockState,
RpiFirmwareNotifyGpioSetCfg
};
--
2.13.7
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81388): https://edk2.groups.io/g/devel/message/81388
Mute This Topic: https://groups.io/mt/86014862/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/edk2-devel-archive/attachments/20211002/d4876329/attachment.htm>
More information about the edk2-devel-archive
mailing list