On Sat, 2 Oct 2021 at 02:52, Jeremy Linton <jeremy.lin...@arm.com> wrote:
>
> 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.lin...@arm.com>
This fails to apply for me - can you rebase onto the last
edk2-platform master please?
> ---
> .../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 (#81508): https://edk2.groups.io/g/devel/message/81508
Mute This Topic: https://groups.io/mt/86014862/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
