From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by mx.groups.io with SMTP id smtpd.web10.23603.1633428742306464452 for ; Tue, 05 Oct 2021 03:12:22 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=IoV3zdhB; spf=pass (domain: kernel.org, ip: 198.145.29.99, mailfrom: ardb@kernel.org) Received: by mail.kernel.org (Postfix) with ESMTPSA id AD66261507 for ; Tue, 5 Oct 2021 10:12:21 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1633428741; bh=yU0MGOPA0lBFwkuynaW36Sc0xl+sTeRPe2Wv9sNET4U=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=IoV3zdhBkOlagnxQu7fckgsm8VA/c/8ZdW7pHWQayUE2btqW5X/fj8WdXKvj8UOZa um5Qj6DAFL10MyFqPNahfdocxglbPRcQcqdS/IRXq0u8tTy46SBNBuauSqF8NhCj1K qGJ4cPeFuivgLo49gXLhQ5Xs1lOcsDPFXFfKfxTctJwMMffdJyoCFQWtTqf89dCPnj 5KwBY7GQCloWWrIpVb5o2eBZnqXGq79Hz6+7Tf7KeSfi+AtR+5yFMuE/9u9lDP3eVK uGS+IgUK/qcCGltVYKGHY8fm+8jIwr12tM5PvEOreh0uPyTeZC2159Rxur17ySnz7T 37+dYmDTzcxrg== Received: by mail-ot1-f49.google.com with SMTP id c6-20020a9d2786000000b005471981d559so25151913otb.5 for ; Tue, 05 Oct 2021 03:12:21 -0700 (PDT) X-Gm-Message-State: AOAM530aN0v+dz0XJXtqvlnZIcsojhjzJyokNuBMVGpYD45Qr/1A4wL3 xnu6dgJltW0X0Mindz1kqDA8h8Vs5FBiocWqLzw= X-Google-Smtp-Source: ABdhPJyaSqQYgrwA4DKGMsZVa8GpWe77NPambs7g+JVUKtzXCFxCzg74M9BNsRvbapyQdn2veMYxHCQfJiHFo5zuCk0= X-Received: by 2002:a9d:7b48:: with SMTP id f8mr13854590oto.112.1633428740763; Tue, 05 Oct 2021 03:12:20 -0700 (PDT) MIME-Version: 1.0 References: <20211002005238.40280-1-jeremy.linton@arm.com> <20211002005238.40280-3-jeremy.linton@arm.com> In-Reply-To: <20211002005238.40280-3-jeremy.linton@arm.com> From: "Ard Biesheuvel" Date: Tue, 5 Oct 2021 12:12:09 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data To: Jeremy Linton Cc: edk2-devel-groups-io , Peter Batard , Ard Biesheuvel , Leif Lindholm , Andrei Warkentin , Sunny Wang , Samer El-Haj-Mahmoud Content-Type: text/plain; charset="UTF-8" On Sat, 2 Oct 2021 at 02:52, Jeremy Linton wrote: > > While debugging problems with the GET/SET_CLOCK mailbox calls it appeared > that the locking in most of the mailbox commands isn't perfectly > correct. All UEFI firmware calls to the RPi mailbox share a single > mDmaBuffer which is used to fill out the command passed to the vc firmware, > and record its response. The buffer is protected by mMailboxLock, yet in > many cases the result of the request is copied from the buffer after the > lock has been released. This doesn't currently appear to be causing any > problems, but should probably be fixed anyway. > > There are a couple other minor tweaks in this patch that are hard to > justify on their own, one is a bit of whitespace cleanup, and the other is > the addition of a debug message to print the returned clock rate for the > requested clock. This latter print would have immediatly shown that the vc > firmware was returning 0 as the emmc clock rate rather than something > reasonable. > > Signed-off-by: Jeremy Linton This fails to apply for me - can you rebase onto the last edk2-platform master please? > --- > .../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 >