public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Jeremy Linton" <jeremy.linton@arm.com>
To: devel@edk2.groups.io
Cc: pete@akeo.ie, ardb+tianocore@kernel.org, leif@nuviainc.com,
	awarkentin@vmware.com, Sunny.Wang@arm.com,
	samer.el-haj-mahmoud@arm.com, kettenis@openbsd.org,
	Jeremy Linton <jeremy.linton@arm.com>
Subject: [PATCH v2 3/5] Platform/RaspberryPi: Expand locking to cover return data
Date: Mon, 18 Oct 2021 15:51:25 -0500	[thread overview]
Message-ID: <20211018205127.7099-4-jeremy.linton@arm.com> (raw)
In-Reply-To: <20211018205127.7099-1-jeremy.linton@arm.com>

It appears that the locking for many of the mailbox commands is
incorrect. All UEFI firmware calls to the RPi mailbox share a single
mDmaBuffer. That buffer 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 mailbox response is copied from
the buffer after the lock has been released. This doesn't currently
appear to be causing any problems, but should 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@arm.com>
Reviewed-by: Andrei Warkentin <awarkentin@vmware.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..d38ffbc7d3 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_INFO, "%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_INFO, "%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


  parent reply	other threads:[~2021-10-18 20:51 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-18 20:51 [PATCH v2 0/5] Platform/Rpi: Various cleanups + DT booting Jeremy Linton
2021-10-18 20:51 ` [PATCH v2 1/5] Platform/RaspberryPi: Always use non translating DMA in DT mode Jeremy Linton
2021-10-18 20:51 ` [PATCH v2 2/5] Platform/RaspberryPi: Fix vfr warning caused by two defaults Jeremy Linton
2021-10-18 20:51 ` Jeremy Linton [this message]
2021-10-18 20:51 ` [PATCH v2 4/5] Platform/RaspberryPi: Normal memory should not be marked as uncached Jeremy Linton
2021-10-18 20:51 ` [PATCH v2 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot Jeremy Linton
2021-10-19  7:23 ` [PATCH v2 0/5] Platform/Rpi: Various cleanups + DT booting Ard Biesheuvel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20211018205127.7099-4-jeremy.linton@arm.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox