From: "Ard Biesheuvel" <ardb@kernel.org>
To: Jeremy Linton <jeremy.linton@arm.com>
Cc: edk2-devel-groups-io <devel@edk2.groups.io>,
Peter Batard <pete@akeo.ie>,
Ard Biesheuvel <ardb+tianocore@kernel.org>,
Leif Lindholm <leif@nuviainc.com>,
Andrei Warkentin <awarkentin@vmware.com>,
Sunny Wang <Sunny.Wang@arm.com>,
Samer El-Haj-Mahmoud <samer.el-haj-mahmoud@arm.com>
Subject: Re: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data
Date: Tue, 5 Oct 2021 12:12:09 +0200 [thread overview]
Message-ID: <CAMj1kXGC-c6sUZ4O=2EPGSD_z_cmvHU2RJeWNMXdmqj+WCDBkg@mail.gmail.com> (raw)
In-Reply-To: <20211002005238.40280-3-jeremy.linton@arm.com>
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
>
next prev parent reply other threads:[~2021-10-05 10:12 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
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
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAMj1kXGC-c6sUZ4O=2EPGSD_z_cmvHU2RJeWNMXdmqj+WCDBkg@mail.gmail.com' \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox