* [PATCH 0/5] Platform/Rpi: Various cleanups
@ 2021-10-02 0:52 Jeremy Linton
2021-10-02 0:52 ` [PATCH 1/5] Platform/RaspberryPi: Fix vfr warning caused by two defaults Jeremy Linton
` (4 more replies)
0 siblings, 5 replies; 19+ messages in thread
From: Jeremy Linton @ 2021-10-02 0:52 UTC (permalink / raw)
To: devel
Cc: pete, ardb+tianocore, leif, awarkentin, Sunny.Wang,
samer.el-haj-mahmoud, Jeremy Linton
This set is a few patches I've been collecting to fix minor issues I've seen
while debugging other problems, or just various things I think should probably
be changed. Generally I don't think they fix anything that is currently user
visible, although its quite possible some mysterious failures go away.
I've been running all of them in some form or another for a few months and
generally nothing has broken because of them AFAIK. So its probably time to
start getting a few of them out of my private tree.
The first is just a compiler warning. The second is mostly expanding the
mailbox lock to cover the return data. The third is an update to make the
hopefully merged soon CM4 quirk actually work with the patches currently on
LKML. Number 4 is an odd one because it just looks wrong, and I'm worried its
causing random bugs. The final is a corrected shutdown sequence that was
discussed months ago. It looks right. but didn't actually fix the data
persistence problems that resulted in the couple second reboot delay
that is currently in place.
Jeremy Linton (5):
Platform/RaspberryPi: Fix vfr warning caused by two defaults
Platform/RaspberryPi: Expand locking to cover return data
Platform/RaspberryPi: Update Linux quirk name
Platform/RaspberryPi: Normal memory should not be marked as uncached
Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot
Platform/RaspberryPi/AcpiTables/Pci.asl | 2 +-
Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 4 +-
.../RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 2 +-
.../Drivers/RpiFirmwareDxe/RpiFirmwareDxe.c | 102 ++++++++++++---------
Platform/RaspberryPi/Library/ResetLib/ResetLib.c | 44 +++++++++
5 files changed, 107 insertions(+), 47 deletions(-)
--
2.13.7
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH 1/5] Platform/RaspberryPi: Fix vfr warning caused by two defaults 2021-10-02 0:52 [PATCH 0/5] Platform/Rpi: Various cleanups Jeremy Linton @ 2021-10-02 0:52 ` Jeremy Linton 2021-10-02 1:12 ` Andrei Warkentin 2021-10-02 0:52 ` [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data Jeremy Linton ` (3 subsequent siblings) 4 siblings, 1 reply; 19+ messages in thread From: Jeremy Linton @ 2021-10-02 0:52 UTC (permalink / raw) To: devel Cc: pete, ardb+tianocore, leif, awarkentin, Sunny.Wang, samer.el-haj-mahmoud, Jeremy Linton The build has been tossing a warning about having two defaults for a while now, lets fix it. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr index 18b3ec726e..e8bf16312d 100644 --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr @@ -192,7 +192,7 @@ formset flags = NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED, option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_ACPI), value = SYSTEM_TABLE_MODE_ACPI, flags = 0; option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_BOTH), value = SYSTEM_TABLE_MODE_BOTH, flags = DEFAULT; - option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_DT), value = SYSTEM_TABLE_MODE_DT, flags = DEFAULT; + option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_DT), value = SYSTEM_TABLE_MODE_DT, flags =0; endoneof; #if (RPI_MODEL == 4) -- 2.13.7 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/5] Platform/RaspberryPi: Fix vfr warning caused by two defaults 2021-10-02 0:52 ` [PATCH 1/5] Platform/RaspberryPi: Fix vfr warning caused by two defaults Jeremy Linton @ 2021-10-02 1:12 ` Andrei Warkentin 0 siblings, 0 replies; 19+ messages in thread From: Andrei Warkentin @ 2021-10-02 1:12 UTC (permalink / raw) To: Jeremy Linton, devel@edk2.groups.io Cc: pete@akeo.ie, ardb+tianocore@kernel.org, leif@nuviainc.com, Sunny.Wang@arm.com, samer.el-haj-mahmoud@arm.com [-- Attachment #1: Type: text/plain, Size: 1789 bytes --] Reviewed-by: Andrei Warkentin <awarkentin@vmware.com> ________________________________ From: Jeremy Linton <jeremy.linton@arm.com> Sent: Friday, October 1, 2021 7:52 PM To: devel@edk2.groups.io <devel@edk2.groups.io> Cc: pete@akeo.ie <pete@akeo.ie>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; leif@nuviainc.com <leif@nuviainc.com>; Andrei Warkentin <awarkentin@vmware.com>; Sunny.Wang@arm.com <Sunny.Wang@arm.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Jeremy Linton <jeremy.linton@arm.com> Subject: [PATCH 1/5] Platform/RaspberryPi: Fix vfr warning caused by two defaults The build has been tossing a warning about having two defaults for a while now, lets fix it. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr index 18b3ec726e..e8bf16312d 100644 --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxeHii.vfr @@ -192,7 +192,7 @@ formset flags = NUMERIC_SIZE_4 | INTERACTIVE | RESET_REQUIRED, option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_ACPI), value = SYSTEM_TABLE_MODE_ACPI, flags = 0; option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_BOTH), value = SYSTEM_TABLE_MODE_BOTH, flags = DEFAULT; - option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_DT), value = SYSTEM_TABLE_MODE_DT, flags = DEFAULT; + option text = STRING_TOKEN(STR_ADVANCED_SYSTAB_DT), value = SYSTEM_TABLE_MODE_DT, flags =0; endoneof; #if (RPI_MODEL == 4) -- 2.13.7 [-- Attachment #2: Type: text/html, Size: 3031 bytes --] ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data 2021-10-02 0:52 [PATCH 0/5] Platform/Rpi: Various cleanups Jeremy Linton 2021-10-02 0:52 ` [PATCH 1/5] Platform/RaspberryPi: Fix vfr warning caused by two defaults Jeremy Linton @ 2021-10-02 0:52 ` Jeremy Linton 2021-10-02 1:17 ` Andrei Warkentin 2021-10-05 10:12 ` Ard Biesheuvel 2021-10-02 0:52 ` [PATCH 3/5] Platform/RaspberryPi: Update Linux quirk name Jeremy Linton ` (2 subsequent siblings) 4 siblings, 2 replies; 19+ messages in thread From: Jeremy Linton @ 2021-10-02 0:52 UTC (permalink / raw) To: devel Cc: pete, ardb+tianocore, leif, awarkentin, Sunny.Wang, samer.el-haj-mahmoud, Jeremy Linton 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> --- .../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 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data 2021-10-02 0:52 ` [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data Jeremy Linton @ 2021-10-02 1:17 ` Andrei Warkentin 2021-10-05 10:12 ` Ard Biesheuvel 1 sibling, 0 replies; 19+ messages in thread From: Andrei Warkentin @ 2021-10-02 1:17 UTC (permalink / raw) To: Jeremy Linton, devel@edk2.groups.io Cc: pete@akeo.ie, ardb+tianocore@kernel.org, leif@nuviainc.com, Sunny.Wang@arm.com, samer.el-haj-mahmoud@arm.com [-- Attachment #1: Type: text/plain, Size: 15078 bytes --] Yikes - that's embarrassing. Thanks for the fix. Reviewed-by: Andrei Warkentin <awarkentin@vmware.com> ________________________________ From: Jeremy Linton <jeremy.linton@arm.com> Sent: Friday, October 1, 2021 7:52 PM To: devel@edk2.groups.io <devel@edk2.groups.io> Cc: pete@akeo.ie <pete@akeo.ie>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; leif@nuviainc.com <leif@nuviainc.com>; Andrei Warkentin <awarkentin@vmware.com>; Sunny.Wang@arm.com <Sunny.Wang@arm.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Jeremy Linton <jeremy.linton@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@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 [-- Attachment #2: Type: text/html, Size: 23645 bytes --] ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data 2021-10-02 0:52 ` [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data Jeremy Linton 2021-10-02 1:17 ` Andrei Warkentin @ 2021-10-05 10:12 ` Ard Biesheuvel 2021-10-05 21:19 ` Jeremy Linton 1 sibling, 1 reply; 19+ messages in thread From: Ard Biesheuvel @ 2021-10-05 10:12 UTC (permalink / raw) To: Jeremy Linton Cc: edk2-devel-groups-io, Peter Batard, Ard Biesheuvel, Leif Lindholm, Andrei Warkentin, Sunny Wang, Samer El-Haj-Mahmoud 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? > --- > .../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 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data 2021-10-05 10:12 ` Ard Biesheuvel @ 2021-10-05 21:19 ` Jeremy Linton 2021-10-06 15:31 ` Ard Biesheuvel 0 siblings, 1 reply; 19+ messages in thread From: Jeremy Linton @ 2021-10-05 21:19 UTC (permalink / raw) To: Ard Biesheuvel Cc: edk2-devel-groups-io, Peter Batard, Ard Biesheuvel, Leif Lindholm, Andrei Warkentin, Sunny Wang, Samer El-Haj-Mahmoud 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 >> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data 2021-10-05 21:19 ` Jeremy Linton @ 2021-10-06 15:31 ` Ard Biesheuvel 0 siblings, 0 replies; 19+ messages in thread From: Ard Biesheuvel @ 2021-10-06 15:31 UTC (permalink / raw) To: Jeremy Linton Cc: edk2-devel-groups-io, Peter Batard, Ard Biesheuvel, Leif Lindholm, Andrei Warkentin, Sunny Wang, Samer El-Haj-Mahmoud On Tue, 5 Oct 2021 at 23:20, Jeremy Linton <jeremy.linton@arm.com> wrote: > > 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>). > Yeah, it looked like whitespace, to be honest. The first failing hunk was a whitespace only change, but when I edited that out, it failed on the following one. All the other patches applied fine so I'm not sure what happened here. > > > > >> --- > >> .../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 > >> > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/5] Platform/RaspberryPi: Update Linux quirk name 2021-10-02 0:52 [PATCH 0/5] Platform/Rpi: Various cleanups Jeremy Linton 2021-10-02 0:52 ` [PATCH 1/5] Platform/RaspberryPi: Fix vfr warning caused by two defaults Jeremy Linton 2021-10-02 0:52 ` [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data Jeremy Linton @ 2021-10-02 0:52 ` Jeremy Linton 2021-10-02 1:13 ` Andrei Warkentin 2021-10-14 21:22 ` Jeremy Linton 2021-10-02 0:52 ` [PATCH 4/5] Platform/RaspberryPi: Normal memory should not be marked as uncached Jeremy Linton 2021-10-02 0:52 ` [PATCH 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot Jeremy Linton 4 siblings, 2 replies; 19+ messages in thread From: Jeremy Linton @ 2021-10-02 0:52 UTC (permalink / raw) To: devel Cc: pete, ardb+tianocore, leif, awarkentin, Sunny.Wang, samer.el-haj-mahmoud, Jeremy Linton During review/merge of the linux ecam quirk, some logic was added to require the quirk name to be exactly 6 characters, matching the MADT field its overriding. As such, the rpi quirk here needed to be shorted by a character to avoid confusion. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- Platform/RaspberryPi/AcpiTables/Pci.asl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Platform/RaspberryPi/AcpiTables/Pci.asl b/Platform/RaspberryPi/AcpiTables/Pci.asl index ee37b7a21e..e5fe755923 100644 --- a/Platform/RaspberryPi/AcpiTables/Pci.asl +++ b/Platform/RaspberryPi/AcpiTables/Pci.asl @@ -65,7 +65,7 @@ DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", "RPI4PCIE", 2) Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { - Package () { "linux-ecam-quirk-id", "bcm2711" }, + Package () { "linux-ecam-quirk-id", "bc2711" }, } }) -- 2.13.7 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/5] Platform/RaspberryPi: Update Linux quirk name 2021-10-02 0:52 ` [PATCH 3/5] Platform/RaspberryPi: Update Linux quirk name Jeremy Linton @ 2021-10-02 1:13 ` Andrei Warkentin 2021-10-14 21:22 ` Jeremy Linton 1 sibling, 0 replies; 19+ messages in thread From: Andrei Warkentin @ 2021-10-02 1:13 UTC (permalink / raw) To: Jeremy Linton, devel@edk2.groups.io Cc: pete@akeo.ie, ardb+tianocore@kernel.org, leif@nuviainc.com, Sunny.Wang@arm.com, samer.el-haj-mahmoud@arm.com [-- Attachment #1: Type: text/plain, Size: 1604 bytes --] Reviewed-by: Andrei Warkentin <awarkentin@vmware.com> ________________________________ From: Jeremy Linton <jeremy.linton@arm.com> Sent: Friday, October 1, 2021 7:52 PM To: devel@edk2.groups.io <devel@edk2.groups.io> Cc: pete@akeo.ie <pete@akeo.ie>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; leif@nuviainc.com <leif@nuviainc.com>; Andrei Warkentin <awarkentin@vmware.com>; Sunny.Wang@arm.com <Sunny.Wang@arm.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Jeremy Linton <jeremy.linton@arm.com> Subject: [PATCH 3/5] Platform/RaspberryPi: Update Linux quirk name During review/merge of the linux ecam quirk, some logic was added to require the quirk name to be exactly 6 characters, matching the MADT field its overriding. As such, the rpi quirk here needed to be shorted by a character to avoid confusion. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- Platform/RaspberryPi/AcpiTables/Pci.asl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Platform/RaspberryPi/AcpiTables/Pci.asl b/Platform/RaspberryPi/AcpiTables/Pci.asl index ee37b7a21e..e5fe755923 100644 --- a/Platform/RaspberryPi/AcpiTables/Pci.asl +++ b/Platform/RaspberryPi/AcpiTables/Pci.asl @@ -65,7 +65,7 @@ DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", "RPI4PCIE", 2) Name (_DSD, Package () { ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), Package () { - Package () { "linux-ecam-quirk-id", "bcm2711" }, + Package () { "linux-ecam-quirk-id", "bc2711" }, } }) -- 2.13.7 [-- Attachment #2: Type: text/html, Size: 2887 bytes --] ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 3/5] Platform/RaspberryPi: Update Linux quirk name 2021-10-02 0:52 ` [PATCH 3/5] Platform/RaspberryPi: Update Linux quirk name Jeremy Linton 2021-10-02 1:13 ` Andrei Warkentin @ 2021-10-14 21:22 ` Jeremy Linton 1 sibling, 0 replies; 19+ messages in thread From: Jeremy Linton @ 2021-10-14 21:22 UTC (permalink / raw) To: devel Cc: pete, ardb+tianocore, leif, awarkentin, Sunny.Wang, samer.el-haj-mahmoud Hi, On 10/1/21 7:52 PM, Jeremy Linton wrote: > During review/merge of the linux ecam quirk, some logic > was added to require the quirk name to be exactly 6 > characters, matching the MADT field its overriding. > > As such, the rpi quirk here needed to be shorted by > a character to avoid confusion. I'm going to drop this from the v2 of this set because while the mainline patch has a bunch of Ack's, its stuck because we are still discussing whether a DSD here is the right choice. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > Platform/RaspberryPi/AcpiTables/Pci.asl | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/Platform/RaspberryPi/AcpiTables/Pci.asl b/Platform/RaspberryPi/AcpiTables/Pci.asl > index ee37b7a21e..e5fe755923 100644 > --- a/Platform/RaspberryPi/AcpiTables/Pci.asl > +++ b/Platform/RaspberryPi/AcpiTables/Pci.asl > @@ -65,7 +65,7 @@ DefinitionBlock (__FILE__, "SSDT", 5, "RPIFDN", "RPI4PCIE", 2) > Name (_DSD, Package () { > ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"), > Package () { > - Package () { "linux-ecam-quirk-id", "bcm2711" }, > + Package () { "linux-ecam-quirk-id", "bc2711" }, > } > }) > > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 4/5] Platform/RaspberryPi: Normal memory should not be marked as uncached 2021-10-02 0:52 [PATCH 0/5] Platform/Rpi: Various cleanups Jeremy Linton ` (2 preceding siblings ...) 2021-10-02 0:52 ` [PATCH 3/5] Platform/RaspberryPi: Update Linux quirk name Jeremy Linton @ 2021-10-02 0:52 ` Jeremy Linton 2021-10-02 1:14 ` Andrei Warkentin 2021-10-02 0:52 ` [PATCH 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot Jeremy Linton 4 siblings, 1 reply; 19+ messages in thread From: Jeremy Linton @ 2021-10-02 0:52 UTC (permalink / raw) To: devel Cc: pete, ardb+tianocore, leif, awarkentin, Sunny.Wang, samer.el-haj-mahmoud, Jeremy Linton The efi spec seems to indicate that the efi uncacheable attribute should be mapped to device memory rather than normal-nc. This means that the uefi mem attribute for the >3G ram doesn't match the remainder of the RAM in the machine. So, lets remove the uncacheable attribute to make it more consistent. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c index 2ef7da67bd..415d99fadb 100644 --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c @@ -499,7 +499,7 @@ ApplyVariables ( Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, 3UL * BASE_1GB, SystemMemorySizeBelow4GB - (3UL * SIZE_1GB), - EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB); + EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB); ASSERT_EFI_ERROR (Status); Status = gDS->SetMemorySpaceAttributes (3UL * BASE_1GB, SystemMemorySizeBelow4GB - (3UL * SIZE_1GB), EFI_MEMORY_WB); @@ -511,7 +511,7 @@ ApplyVariables ( // Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, 4UL * BASE_1GB, SystemMemorySize - (4UL * SIZE_1GB), - EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB); + EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB); ASSERT_EFI_ERROR (Status); Status = gDS->SetMemorySpaceAttributes (4UL * BASE_1GB, SystemMemorySize - (4UL * SIZE_1GB), EFI_MEMORY_WB); -- 2.13.7 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 4/5] Platform/RaspberryPi: Normal memory should not be marked as uncached 2021-10-02 0:52 ` [PATCH 4/5] Platform/RaspberryPi: Normal memory should not be marked as uncached Jeremy Linton @ 2021-10-02 1:14 ` Andrei Warkentin 2021-10-05 10:05 ` Ard Biesheuvel 0 siblings, 1 reply; 19+ messages in thread From: Andrei Warkentin @ 2021-10-02 1:14 UTC (permalink / raw) To: Jeremy Linton, devel@edk2.groups.io Cc: pete@akeo.ie, ardb+tianocore@kernel.org, leif@nuviainc.com, Sunny.Wang@arm.com, samer.el-haj-mahmoud@arm.com [-- Attachment #1: Type: text/plain, Size: 2517 bytes --] I may have misunderstood the flags as being valid ways of mapping the added range. Should we also then take out WC and WT? ________________________________ From: Jeremy Linton <jeremy.linton@arm.com> Sent: Friday, October 1, 2021 7:52 PM To: devel@edk2.groups.io <devel@edk2.groups.io> Cc: pete@akeo.ie <pete@akeo.ie>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; leif@nuviainc.com <leif@nuviainc.com>; Andrei Warkentin <awarkentin@vmware.com>; Sunny.Wang@arm.com <Sunny.Wang@arm.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Jeremy Linton <jeremy.linton@arm.com> Subject: [PATCH 4/5] Platform/RaspberryPi: Normal memory should not be marked as uncached The efi spec seems to indicate that the efi uncacheable attribute should be mapped to device memory rather than normal-nc. This means that the uefi mem attribute for the >3G ram doesn't match the remainder of the RAM in the machine. So, lets remove the uncacheable attribute to make it more consistent. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c index 2ef7da67bd..415d99fadb 100644 --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c @@ -499,7 +499,7 @@ ApplyVariables ( Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, 3UL * BASE_1GB, SystemMemorySizeBelow4GB - (3UL * SIZE_1GB), - EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB); + EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB); ASSERT_EFI_ERROR (Status); Status = gDS->SetMemorySpaceAttributes (3UL * BASE_1GB, SystemMemorySizeBelow4GB - (3UL * SIZE_1GB), EFI_MEMORY_WB); @@ -511,7 +511,7 @@ ApplyVariables ( // Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, 4UL * BASE_1GB, SystemMemorySize - (4UL * SIZE_1GB), - EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB); + EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB); ASSERT_EFI_ERROR (Status); Status = gDS->SetMemorySpaceAttributes (4UL * BASE_1GB, SystemMemorySize - (4UL * SIZE_1GB), EFI_MEMORY_WB); -- 2.13.7 [-- Attachment #2: Type: text/html, Size: 4455 bytes --] ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 4/5] Platform/RaspberryPi: Normal memory should not be marked as uncached 2021-10-02 1:14 ` Andrei Warkentin @ 2021-10-05 10:05 ` Ard Biesheuvel 0 siblings, 0 replies; 19+ messages in thread From: Ard Biesheuvel @ 2021-10-05 10:05 UTC (permalink / raw) To: Andrei Warkentin Cc: Jeremy Linton, devel@edk2.groups.io, pete@akeo.ie, ardb+tianocore@kernel.org, leif@nuviainc.com, Sunny.Wang@arm.com, samer.el-haj-mahmoud@arm.com On Sat, 2 Oct 2021 at 03:14, Andrei Warkentin <awarkentin@vmware.com> wrote: > > I may have misunderstood the flags as being valid ways of mapping the added range. Should we also then take out WC and WT? No, you understood correctly. The problem is that normal memory ceases to behave like normal memory when you map it UC (ie., unaligned accesses and DC ZVA instructions are no longer allowed) so it should be omitted. We use it in the memory map for things like the runtime MMIO mappings for NOR flash and RTC. WC and WT are reasonable for bare metal, but note that we omit those as well for ArmVirtPkg, as using those breaks coherency, which means the hypervisor/virtualization host's view of the guest's memory goes out of sync. > ________________________________ > From: Jeremy Linton <jeremy.linton@arm.com> > Sent: Friday, October 1, 2021 7:52 PM > To: devel@edk2.groups.io <devel@edk2.groups.io> > Cc: pete@akeo.ie <pete@akeo.ie>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; leif@nuviainc.com <leif@nuviainc.com>; Andrei Warkentin <awarkentin@vmware.com>; Sunny.Wang@arm.com <Sunny.Wang@arm.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Jeremy Linton <jeremy.linton@arm.com> > Subject: [PATCH 4/5] Platform/RaspberryPi: Normal memory should not be marked as uncached > > The efi spec seems to indicate that the efi uncacheable attribute > should be mapped to device memory rather than normal-nc. This means > that the uefi mem attribute for the >3G ram doesn't match the remainder > of the RAM in the machine. > > So, lets remove the uncacheable attribute to make it more consistent. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c > index 2ef7da67bd..415d99fadb 100644 > --- a/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c > +++ b/Platform/RaspberryPi/Drivers/ConfigDxe/ConfigDxe.c > @@ -499,7 +499,7 @@ ApplyVariables ( > > Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, 3UL * BASE_1GB, > SystemMemorySizeBelow4GB - (3UL * SIZE_1GB), > - EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB); > + EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB); > ASSERT_EFI_ERROR (Status); > Status = gDS->SetMemorySpaceAttributes (3UL * BASE_1GB, > SystemMemorySizeBelow4GB - (3UL * SIZE_1GB), EFI_MEMORY_WB); > @@ -511,7 +511,7 @@ ApplyVariables ( > // > Status = gDS->AddMemorySpace (EfiGcdMemoryTypeSystemMemory, 4UL * BASE_1GB, > SystemMemorySize - (4UL * SIZE_1GB), > - EFI_MEMORY_UC | EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB); > + EFI_MEMORY_WC | EFI_MEMORY_WT | EFI_MEMORY_WB); > ASSERT_EFI_ERROR (Status); > Status = gDS->SetMemorySpaceAttributes (4UL * BASE_1GB, > SystemMemorySize - (4UL * SIZE_1GB), EFI_MEMORY_WB); > -- > 2.13.7 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot 2021-10-02 0:52 [PATCH 0/5] Platform/Rpi: Various cleanups Jeremy Linton ` (3 preceding siblings ...) 2021-10-02 0:52 ` [PATCH 4/5] Platform/RaspberryPi: Normal memory should not be marked as uncached Jeremy Linton @ 2021-10-02 0:52 ` Jeremy Linton 2021-10-02 1:16 ` Andrei Warkentin 2021-10-05 10:11 ` Ard Biesheuvel 4 siblings, 2 replies; 19+ messages in thread From: Jeremy Linton @ 2021-10-02 0:52 UTC (permalink / raw) To: devel Cc: pete, ardb+tianocore, leif, awarkentin, Sunny.Wang, samer.el-haj-mahmoud, Jeremy Linton In theory we should be properly cleaning up all the device drivers before pulling the big switch. Particularly the partition mgr will issue flush commands to attached disks as it goes down. This assures that devices running in WB mode (that correctly handle flush/sync/etc) commands are persisted to physical media before we hit reset. Without this, there are definitly cases where the relevant specifications don't guarantee persistence of data in their buffers in the face of reset conditions. We can't really do anything about the many devices that don't honor persistance requests but we can start here. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- Platform/RaspberryPi/Library/ResetLib/ResetLib.c | 44 ++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c index a70eee485d..036f619cb5 100644 --- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c +++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c @@ -19,11 +19,54 @@ #include <Library/TimerLib.h> #include <Library/EfiResetSystemLib.h> #include <Library/ArmSmcLib.h> +#include <Library/UefiBootServicesTableLib.h> #include <Library/UefiLib.h> #include <Library/UefiRuntimeLib.h> #include <IndustryStandard/ArmStdSmc.h> + +/** + Disconnect everything. + Modified from the UEFI 2.3 spec (May 2009 version) + + @retval EFI_SUCCESS The operation was successful. + +**/ +EFI_STATUS +DisconnectAll( + VOID + ) +{ + EFI_STATUS Status; + UINTN HandleCount; + EFI_HANDLE *HandleBuffer; + UINTN HandleIndex; + + // + // Retrieve the list of all handles from the handle database + // + Status = gBS->LocateHandleBuffer ( + AllHandles, + NULL, + NULL, + &HandleCount, + &HandleBuffer + ); + if (!EFI_ERROR (Status)) { + for (HandleIndex = 0; HandleIndex < HandleCount; HandleIndex++) { + Status = gBS->DisconnectController ( + HandleBuffer[HandleIndex], + NULL, + NULL + ); + } + gBS->FreePool(HandleBuffer); + } + return (EFI_SUCCESS); +} + + /** Resets the entire platform. @@ -57,6 +100,7 @@ LibResetSystem ( if (Delay != 0) { DEBUG ((DEBUG_INFO, "Platform will be reset in %d.%d seconds...\n", Delay / 1000000, (Delay % 1000000) / 100000)); + DisconnectAll (); MicroSecondDelay (Delay); } } -- 2.13.7 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot 2021-10-02 0:52 ` [PATCH 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot Jeremy Linton @ 2021-10-02 1:16 ` Andrei Warkentin 2021-10-05 10:11 ` Ard Biesheuvel 1 sibling, 0 replies; 19+ messages in thread From: Andrei Warkentin @ 2021-10-02 1:16 UTC (permalink / raw) To: Jeremy Linton, devel@edk2.groups.io Cc: pete@akeo.ie, ardb+tianocore@kernel.org, leif@nuviainc.com, Sunny.Wang@arm.com, samer.el-haj-mahmoud@arm.com [-- Attachment #1: Type: text/plain, Size: 3158 bytes --] Seems smart to do. Reviewed-by: Andrei Warkentin <awarkentin@vmware.com> ________________________________ From: Jeremy Linton <jeremy.linton@arm.com> Sent: Friday, October 1, 2021 7:52 PM To: devel@edk2.groups.io <devel@edk2.groups.io> Cc: pete@akeo.ie <pete@akeo.ie>; ardb+tianocore@kernel.org <ardb+tianocore@kernel.org>; leif@nuviainc.com <leif@nuviainc.com>; Andrei Warkentin <awarkentin@vmware.com>; Sunny.Wang@arm.com <Sunny.Wang@arm.com>; samer.el-haj-mahmoud@arm.com <samer.el-haj-mahmoud@arm.com>; Jeremy Linton <jeremy.linton@arm.com> Subject: [PATCH 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot In theory we should be properly cleaning up all the device drivers before pulling the big switch. Particularly the partition mgr will issue flush commands to attached disks as it goes down. This assures that devices running in WB mode (that correctly handle flush/sync/etc) commands are persisted to physical media before we hit reset. Without this, there are definitly cases where the relevant specifications don't guarantee persistence of data in their buffers in the face of reset conditions. We can't really do anything about the many devices that don't honor persistance requests but we can start here. Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> --- Platform/RaspberryPi/Library/ResetLib/ResetLib.c | 44 ++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c index a70eee485d..036f619cb5 100644 --- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c +++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c @@ -19,11 +19,54 @@ #include <Library/TimerLib.h> #include <Library/EfiResetSystemLib.h> #include <Library/ArmSmcLib.h> +#include <Library/UefiBootServicesTableLib.h> #include <Library/UefiLib.h> #include <Library/UefiRuntimeLib.h> #include <IndustryStandard/ArmStdSmc.h> + +/** + Disconnect everything. + Modified from the UEFI 2.3 spec (May 2009 version) + + @retval EFI_SUCCESS The operation was successful. + +**/ +EFI_STATUS +DisconnectAll( + VOID + ) +{ + EFI_STATUS Status; + UINTN HandleCount; + EFI_HANDLE *HandleBuffer; + UINTN HandleIndex; + + // + // Retrieve the list of all handles from the handle database + // + Status = gBS->LocateHandleBuffer ( + AllHandles, + NULL, + NULL, + &HandleCount, + &HandleBuffer + ); + if (!EFI_ERROR (Status)) { + for (HandleIndex = 0; HandleIndex < HandleCount; HandleIndex++) { + Status = gBS->DisconnectController ( + HandleBuffer[HandleIndex], + NULL, + NULL + ); + } + gBS->FreePool(HandleBuffer); + } + return (EFI_SUCCESS); +} + + /** Resets the entire platform. @@ -57,6 +100,7 @@ LibResetSystem ( if (Delay != 0) { DEBUG ((DEBUG_INFO, "Platform will be reset in %d.%d seconds...\n", Delay / 1000000, (Delay % 1000000) / 100000)); + DisconnectAll (); MicroSecondDelay (Delay); } } -- 2.13.7 [-- Attachment #2: Type: text/html, Size: 5217 bytes --] ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot 2021-10-02 0:52 ` [PATCH 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot Jeremy Linton 2021-10-02 1:16 ` Andrei Warkentin @ 2021-10-05 10:11 ` Ard Biesheuvel 2021-10-05 21:24 ` Jeremy Linton 1 sibling, 1 reply; 19+ messages in thread From: Ard Biesheuvel @ 2021-10-05 10:11 UTC (permalink / raw) To: Jeremy Linton Cc: edk2-devel-groups-io, Peter Batard, Ard Biesheuvel, Leif Lindholm, Andrei Warkentin, Sunny Wang, Samer El-Haj-Mahmoud On Sat, 2 Oct 2021 at 02:52, Jeremy Linton <jeremy.linton@arm.com> wrote: > > In theory we should be properly cleaning up all the device drivers before > pulling the big switch. Particularly the partition mgr will issue > flush commands to attached disks as it goes down. This assures that > devices running in WB mode (that correctly handle flush/sync/etc) commands > are persisted to physical media before we hit reset. > > Without this, there are definitly cases where the relevant specifications > don't guarantee persistence of data in their buffers in the face of > reset conditions. We can't really do anything about the many > devices that don't honor persistance requests but we can start here. > > Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > --- > Platform/RaspberryPi/Library/ResetLib/ResetLib.c | 44 ++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > > diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c > index a70eee485d..036f619cb5 100644 > --- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c > +++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c > @@ -19,11 +19,54 @@ > #include <Library/TimerLib.h> > #include <Library/EfiResetSystemLib.h> > #include <Library/ArmSmcLib.h> > +#include <Library/UefiBootServicesTableLib.h> > #include <Library/UefiLib.h> > #include <Library/UefiRuntimeLib.h> > > #include <IndustryStandard/ArmStdSmc.h> > > + > +/** > + Disconnect everything. > + Modified from the UEFI 2.3 spec (May 2009 version) > + > + @retval EFI_SUCCESS The operation was successful. > + > +**/ STATIC > +EFI_STATUS > +DisconnectAll( Space before ( > + VOID > + ) > +{ > + EFI_STATUS Status; > + UINTN HandleCount; > + EFI_HANDLE *HandleBuffer; > + UINTN HandleIndex; > + > + // > + // Retrieve the list of all handles from the handle database > + // > + Status = gBS->LocateHandleBuffer ( > + AllHandles, > + NULL, > + NULL, > + &HandleCount, > + &HandleBuffer > + ); > + if (!EFI_ERROR (Status)) { I understand that this code is copy/pasted but I'd still prefer to avoid the 'success handling' anti pattern here. if (EFI_ERROR (Status)) { return Status; } > + for (HandleIndex = 0; HandleIndex < HandleCount; HandleIndex++) { > + Status = gBS->DisconnectController ( > + HandleBuffer[HandleIndex], > + NULL, > + NULL > + ); > + } > + gBS->FreePool(HandleBuffer); > + } > + return (EFI_SUCCESS); No need for () > +} > + > + > /** > Resets the entire platform. > > @@ -57,6 +100,7 @@ LibResetSystem ( > if (Delay != 0) { > DEBUG ((DEBUG_INFO, "Platform will be reset in %d.%d seconds...\n", > Delay / 1000000, (Delay % 1000000) / 100000)); > + DisconnectAll (); Capture Status here and ASSERT_EFI_ERROR() ?? Maybe it is overkill, and maybe DisconnectController() fails spuriously, so I am not entirely sure, but adding a local function that returns a value and then ignore it seems slightly sloppy to me. > MicroSecondDelay (Delay); > } > } > -- > 2.13.7 > ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot 2021-10-05 10:11 ` Ard Biesheuvel @ 2021-10-05 21:24 ` Jeremy Linton 2021-10-05 21:35 ` Ard Biesheuvel 0 siblings, 1 reply; 19+ messages in thread From: Jeremy Linton @ 2021-10-05 21:24 UTC (permalink / raw) To: Ard Biesheuvel Cc: edk2-devel-groups-io, Peter Batard, Ard Biesheuvel, Leif Lindholm, Andrei Warkentin, Sunny Wang, Samer El-Haj-Mahmoud Hi, On 10/5/21 5:11 AM, Ard Biesheuvel wrote: > On Sat, 2 Oct 2021 at 02:52, Jeremy Linton <jeremy.linton@arm.com> wrote: >> >> In theory we should be properly cleaning up all the device drivers before >> pulling the big switch. Particularly the partition mgr will issue >> flush commands to attached disks as it goes down. This assures that >> devices running in WB mode (that correctly handle flush/sync/etc) commands >> are persisted to physical media before we hit reset. >> >> Without this, there are definitly cases where the relevant specifications >> don't guarantee persistence of data in their buffers in the face of >> reset conditions. We can't really do anything about the many >> devices that don't honor persistance requests but we can start here. >> >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> >> --- >> Platform/RaspberryPi/Library/ResetLib/ResetLib.c | 44 ++++++++++++++++++++++++ >> 1 file changed, 44 insertions(+) >> >> diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c >> index a70eee485d..036f619cb5 100644 >> --- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c >> +++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c >> @@ -19,11 +19,54 @@ >> #include <Library/TimerLib.h> >> #include <Library/EfiResetSystemLib.h> >> #include <Library/ArmSmcLib.h> >> +#include <Library/UefiBootServicesTableLib.h> >> #include <Library/UefiLib.h> >> #include <Library/UefiRuntimeLib.h> >> >> #include <IndustryStandard/ArmStdSmc.h> >> >> + >> +/** >> + Disconnect everything. >> + Modified from the UEFI 2.3 spec (May 2009 version) >> + >> + @retval EFI_SUCCESS The operation was successful. >> + >> +**/ > > STATIC > >> +EFI_STATUS >> +DisconnectAll( > > Space before ( > >> + VOID >> + ) >> +{ >> + EFI_STATUS Status; >> + UINTN HandleCount; >> + EFI_HANDLE *HandleBuffer; >> + UINTN HandleIndex; >> + >> + // >> + // Retrieve the list of all handles from the handle database >> + // >> + Status = gBS->LocateHandleBuffer ( >> + AllHandles, >> + NULL, >> + NULL, >> + &HandleCount, >> + &HandleBuffer >> + ); >> + if (!EFI_ERROR (Status)) { > > I understand that this code is copy/pasted but I'd still prefer to > avoid the 'success handling' anti pattern here. Sure. > > if (EFI_ERROR (Status)) { > return Status; > } > >> + for (HandleIndex = 0; HandleIndex < HandleCount; HandleIndex++) { >> + Status = gBS->DisconnectController ( >> + HandleBuffer[HandleIndex], >> + NULL, >> + NULL >> + ); >> + } >> + gBS->FreePool(HandleBuffer); >> + } >> + return (EFI_SUCCESS); > > No need for () Yup > >> +} >> + >> + >> /** >> Resets the entire platform. >> >> @@ -57,6 +100,7 @@ LibResetSystem ( >> if (Delay != 0) { >> DEBUG ((DEBUG_INFO, "Platform will be reset in %d.%d seconds...\n", >> Delay / 1000000, (Delay % 1000000) / 100000)); >> + DisconnectAll (); > > Capture Status here and ASSERT_EFI_ERROR() ?? > > Maybe it is overkill, and maybe DisconnectController() fails > spuriously, so I am not entirely sure, but adding a local function > that returns a value and then ignore it seems slightly sloppy to me. Which makes the above bits about failure returns sorta redundant as I should probably just make DisconnectAll() void. There isn't really anything to do with a failed return other than print a message and ignore it. > >> MicroSecondDelay (Delay); >> } >> } >> -- >> 2.13.7 >> ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot 2021-10-05 21:24 ` Jeremy Linton @ 2021-10-05 21:35 ` Ard Biesheuvel 0 siblings, 0 replies; 19+ messages in thread From: Ard Biesheuvel @ 2021-10-05 21:35 UTC (permalink / raw) To: Jeremy Linton Cc: edk2-devel-groups-io, Peter Batard, Ard Biesheuvel, Leif Lindholm, Andrei Warkentin, Sunny Wang, Samer El-Haj-Mahmoud On Tue, 5 Oct 2021 at 23:25, Jeremy Linton <jeremy.linton@arm.com> wrote: > > Hi, > > On 10/5/21 5:11 AM, Ard Biesheuvel wrote: > > On Sat, 2 Oct 2021 at 02:52, Jeremy Linton <jeremy.linton@arm.com> wrote: > >> > >> In theory we should be properly cleaning up all the device drivers before > >> pulling the big switch. Particularly the partition mgr will issue > >> flush commands to attached disks as it goes down. This assures that > >> devices running in WB mode (that correctly handle flush/sync/etc) commands > >> are persisted to physical media before we hit reset. > >> > >> Without this, there are definitly cases where the relevant specifications > >> don't guarantee persistence of data in their buffers in the face of > >> reset conditions. We can't really do anything about the many > >> devices that don't honor persistance requests but we can start here. > >> > >> Signed-off-by: Jeremy Linton <jeremy.linton@arm.com> > >> --- > >> Platform/RaspberryPi/Library/ResetLib/ResetLib.c | 44 ++++++++++++++++++++++++ > >> 1 file changed, 44 insertions(+) > >> > >> diff --git a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c > >> index a70eee485d..036f619cb5 100644 > >> --- a/Platform/RaspberryPi/Library/ResetLib/ResetLib.c > >> +++ b/Platform/RaspberryPi/Library/ResetLib/ResetLib.c > >> @@ -19,11 +19,54 @@ > >> #include <Library/TimerLib.h> > >> #include <Library/EfiResetSystemLib.h> > >> #include <Library/ArmSmcLib.h> > >> +#include <Library/UefiBootServicesTableLib.h> > >> #include <Library/UefiLib.h> > >> #include <Library/UefiRuntimeLib.h> > >> > >> #include <IndustryStandard/ArmStdSmc.h> > >> > >> + > >> +/** > >> + Disconnect everything. > >> + Modified from the UEFI 2.3 spec (May 2009 version) > >> + > >> + @retval EFI_SUCCESS The operation was successful. > >> + > >> +**/ > > > > STATIC > > > >> +EFI_STATUS > >> +DisconnectAll( > > > > Space before ( > > > >> + VOID > >> + ) > >> +{ > >> + EFI_STATUS Status; > >> + UINTN HandleCount; > >> + EFI_HANDLE *HandleBuffer; > >> + UINTN HandleIndex; > >> + > >> + // > >> + // Retrieve the list of all handles from the handle database > >> + // > >> + Status = gBS->LocateHandleBuffer ( > >> + AllHandles, > >> + NULL, > >> + NULL, > >> + &HandleCount, > >> + &HandleBuffer > >> + ); > >> + if (!EFI_ERROR (Status)) { > > > > I understand that this code is copy/pasted but I'd still prefer to > > avoid the 'success handling' anti pattern here. > > Sure. > > > > > if (EFI_ERROR (Status)) { > > return Status; > > } > > > >> + for (HandleIndex = 0; HandleIndex < HandleCount; HandleIndex++) { > >> + Status = gBS->DisconnectController ( > >> + HandleBuffer[HandleIndex], > >> + NULL, > >> + NULL > >> + ); > >> + } > >> + gBS->FreePool(HandleBuffer); > >> + } > >> + return (EFI_SUCCESS); > > > > No need for () > > Yup > > > > >> +} > >> + > >> + > >> /** > >> Resets the entire platform. > >> > >> @@ -57,6 +100,7 @@ LibResetSystem ( > >> if (Delay != 0) { > >> DEBUG ((DEBUG_INFO, "Platform will be reset in %d.%d seconds...\n", > >> Delay / 1000000, (Delay % 1000000) / 100000)); > >> + DisconnectAll (); > > > > Capture Status here and ASSERT_EFI_ERROR() ?? > > > > Maybe it is overkill, and maybe DisconnectController() fails > > spuriously, so I am not entirely sure, but adding a local function > > that returns a value and then ignore it seems slightly sloppy to me. > > Which makes the above bits about failure returns sorta redundant as I > should probably just make DisconnectAll() void. There isn't really > anything to do with a failed return other than print a message and > ignore it. > > Works for me. > > > >> MicroSecondDelay (Delay); > >> } > >> } > >> -- > >> 2.13.7 > >> > ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2021-10-14 21:22 UTC | newest] Thread overview: 19+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-10-02 0:52 [PATCH 0/5] Platform/Rpi: Various cleanups Jeremy Linton 2021-10-02 0:52 ` [PATCH 1/5] Platform/RaspberryPi: Fix vfr warning caused by two defaults Jeremy Linton 2021-10-02 1:12 ` Andrei Warkentin 2021-10-02 0:52 ` [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data Jeremy Linton 2021-10-02 1:17 ` Andrei Warkentin 2021-10-05 10:12 ` Ard Biesheuvel 2021-10-05 21:19 ` Jeremy Linton 2021-10-06 15:31 ` Ard Biesheuvel 2021-10-02 0:52 ` [PATCH 3/5] Platform/RaspberryPi: Update Linux quirk name Jeremy Linton 2021-10-02 1:13 ` Andrei Warkentin 2021-10-14 21:22 ` Jeremy Linton 2021-10-02 0:52 ` [PATCH 4/5] Platform/RaspberryPi: Normal memory should not be marked as uncached Jeremy Linton 2021-10-02 1:14 ` Andrei Warkentin 2021-10-05 10:05 ` Ard Biesheuvel 2021-10-02 0:52 ` [PATCH 5/5] Platform/RaspberryPi: Disconnect/shutdown all drivers before reboot Jeremy Linton 2021-10-02 1:16 ` Andrei Warkentin 2021-10-05 10:11 ` Ard Biesheuvel 2021-10-05 21:24 ` Jeremy Linton 2021-10-05 21:35 ` Ard Biesheuvel
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox