On Tue, 5 Oct 2021 at 23:20, Jeremy Linton <[email protected]> wrote:
>
> Hi,
>
> On 10/5/21 5:12 AM, Ard Biesheuvel wrote:
> > On Sat, 2 Oct 2021 at 02:52, Jeremy Linton <[email protected]> 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 <[email protected]>
> >
> > 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
> >>
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#81562): https://edk2.groups.io/g/devel/message/81562
Mute This Topic: https://groups.io/mt/86014862/21656
Group Owner: [email protected]
Unsubscribe: https://edk2.groups.io/g/devel/unsub [[email protected]]
-=-=-=-=-=-=-=-=-=-=-=-
