From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by mx.groups.io with SMTP id smtpd.web12.2990.1633468809564274116 for ; Tue, 05 Oct 2021 14:20:10 -0700 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: arm.com, ip: 217.140.110.172, mailfrom: jeremy.linton@arm.com) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 98E741FB; Tue, 5 Oct 2021 14:20:02 -0700 (PDT) Received: from [192.168.122.166] (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 0E4B03F70D; Tue, 5 Oct 2021 14:20:01 -0700 (PDT) Subject: Re: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data To: Ard Biesheuvel Cc: edk2-devel-groups-io , Peter Batard , Ard Biesheuvel , Leif Lindholm , Andrei Warkentin , Sunny Wang , Samer El-Haj-Mahmoud References: <20211002005238.40280-1-jeremy.linton@arm.com> <20211002005238.40280-3-jeremy.linton@arm.com> From: "Jeremy Linton" Message-ID: Date: Tue, 5 Oct 2021 16:19:57 -0500 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.10.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Hi, On 10/5/21 5:12 AM, Ard Biesheuvel wrote: > On Sat, 2 Oct 2021 at 02:52, Jeremy Linton 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 > > 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 ). > >> --- >> .../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 >>