Re: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data

["Jeremy Linton" <jeremy.linton@arm.com>]

Hi,

On 10/5/21 5:12 AM, Ard Biesheuvel wrote:
> On Sat, 2 Oct 2021 at 02:52, Jeremy Linton <jeremy.linton@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.linton@arm.com>
> 
> This fails to apply for me - can you rebase onto the last
> edk2-platform master please?

Sure. Is it whitespace, or general conflicts? I noticed that groups.io's 
web UI looked like I had MIME linebreaks sprinked through my patch 
despite specifying 8bit ASCII. I flirted with 7-bit ASCII with some of 
the previous patches, but its pretty clear that the arm foss server is 
mangling what it thinks are MIME emails and putting cr/lf's changes its 
behavior a bit.


Let me try and force the 7bit again, which git rejects if it finds utf 
in the files (and a few of them had it in the past <sigh>).


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