From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from NAM11-BN8-obe.outbound.protection.outlook.com (NAM11-BN8-obe.outbound.protection.outlook.com [40.107.236.67]) by mx.groups.io with SMTP id smtpd.web12.5993.1633137449659563603 for ; Fri, 01 Oct 2021 18:17:30 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@vmware.com header.s=selector2 header.b=uO/v7pRC; spf=pass (domain: vmware.com, ip: 40.107.236.67, mailfrom: awarkentin@vmware.com) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=nl2v+Q90jTxGlT4A0tBYGctWesD6wc35Si/othcAOtzVsLNTuSf8JueSOPAlBj/M3Dz0c4two2WPy0zyNn1NBG26gBwAEL2EqKSrZsMNBlR4vjc1/pbQicR69rhRiHS5iqM6W7D+U7KziDJ4YT48Bb6MsQmc/yVasw3FHaKZa5ruVQWsUR43TRbivIZWQeP7jHs6AxCLlUrIZ2OLjWYakUaF7IG6IxlWU9UrlU+ubL5aohenNElzjmlQXn87kUk4QDm2uInBcXRQ6T5J07h8JSboUxmNGy1jJkdiX4MXF7IEA0AI6u4CGk+hgGfRT7c+GjmbxrYpNGU1qHDpaL5Bhw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=PWlNlB0MluRxZmRxDQ4RT5L656HQApKIHNZ2OKORC9s=; b=odl5ZMNCVupEh59TXyfkCBJpYP1uClna3qeBQDEfdKJ4Ri4y+L8KuXYz9LXORRhnwhTrpvOuYBoOQJS7Aow2tgGT8SqqmzLviUEN6HpGKEeQjWr4WvnFa6LikovcFlIJuaVUoPt5Psi4L013UG07sck9pfYXO9t4Is1ct1a0UoCYJu6dKMRKXaHsUH50LHcDhNapCxmIRNn5mzsxskxLovSh1n/MrAhd9AkIVf/GtW7iuUTuzuAn+dxbJ39ZDchfBtJ3bnCejQDW8P2pn1W8JRO3KK9sE7UEofVvd9cuO4S/o0XlvuFFG2flNRIHeSVyD6Au/T0DtnKjUdAUvLENuA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=vmware.com; dmarc=pass action=none header.from=vmware.com; dkim=pass header.d=vmware.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=vmware.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=PWlNlB0MluRxZmRxDQ4RT5L656HQApKIHNZ2OKORC9s=; b=uO/v7pRCys0V29GNCMszYgoX+Kw57ELO2yaNnUJKQFoN/5ytDsZ4pp2vkNE/lzQJGuUAW/ZZ2fc6tDsvNojxCINtD61a9+/5afaGVabys32ll7kS3ZWAOty9/1c7WhFlfhMJ06eKOvPDaVs1LxEG4u4r7XYaATtzdmL9jxWCz1k= Received: from PH0PR05MB8702.namprd05.prod.outlook.com (2603:10b6:510:b2::21) by PH0PR05MB8800.namprd05.prod.outlook.com (2603:10b6:510:bb::22) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.4566.7; Sat, 2 Oct 2021 01:17:27 +0000 Received: from PH0PR05MB8702.namprd05.prod.outlook.com ([fe80::c88e:73c1:db37:5b94]) by PH0PR05MB8702.namprd05.prod.outlook.com ([fe80::c88e:73c1:db37:5b94%7]) with mapi id 15.20.4587.009; Sat, 2 Oct 2021 01:17:27 +0000 From: "Andrei Warkentin" 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" Subject: Re: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data Thread-Topic: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return data Thread-Index: AQHXtyfUuo4Itd3PYUWatEXt4Cxjf6u+6CCA Date: Sat, 2 Oct 2021 01:17:27 +0000 Message-ID: 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> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: suggested_attachment_session_id: 55dda263-4c67-dc3a-3bf0-515184a4ec68 authentication-results: arm.com; dkim=none (message not signed) header.d=none;arm.com; dmarc=none action=none header.from=vmware.com; x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: 1c690b95-39e5-49b5-3c54-08d9854265ef x-ms-traffictypediagnostic: PH0PR05MB8800: x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:6108; x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: kwzJFFMEuEPvQc2LmNWu6IC7bNjtAeSurQJ2LthDvfYcpXZ5tb+rwT8bEWLijTvOVGiRRHRYDfSRNlaMEWnutJqj+fW/fDtZvE0WXVz/EIC1XoHEL3N7kAnUelrMAj325qYld8MJvdtkzZocp0voWFNlpz9GQtTqc2SvGI5GLDCHnsWG3B1YrGvsKRJO21z0ByvkD2bxzafoCd5rzN5Ro6IpN4ZNqJGBUmdUTrw6/H+mfkkIGAptMWakGVifYZQ+vZ0l24A6k/X7LcEcdRzB4F229ntaKSX92hCiqab0UtmOYEocCnrMjbM7oa97657uid2jAUT9cooiE0NHjTgwKJ2KPS9edV8KtvRn7s2i1Yl3rKcvMYTIV4APhI6LPGdEA5KpqNCqTN58dZ4XU5K8sU0/USFGdQLrQwtGhGSPMPPz+lQGopUDoUEWUVYq0uLeXnSdEHFq87eQjGR7ixxvWPHedlaAom2ioMZ2yEooeWzyYY9/XwTLgyImpE5cxiRBOe2zFr7WW/Omx3zC+LbqQoHXsi/NlETRc5uDXLWEOq6DwMieUyC83gcFuba91H5xBa/T2xi261yphWn6Tg+bXqcdffInHa7hNbTpV9E/0DGk8eNQs3z7hUEXdICfoEb74795YXyv2jlteO0Xxoo3p8bBpyAyI3QIJGeq/W19OlUaDl2wb0cy6KyTyLDmKCA6pA1xeG2WvoYSEedHL7+g7Q== x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:PH0PR05MB8702.namprd05.prod.outlook.com;PTR:;CAT:NONE;SFS:(4636009)(366004)(55016002)(26005)(5660300002)(71200400001)(122000001)(52536014)(7696005)(9686003)(30864003)(8936002)(6506007)(66446008)(83380400001)(33656002)(508600001)(8676002)(66556008)(66946007)(91956017)(2906002)(38070700005)(66476007)(64756008)(186003)(38100700002)(316002)(76116006)(4326008)(110136005)(19627405001)(86362001)(54906003)(53546011);DIR:OUT;SFP:1101; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?NBR6eUbOP035jOeny7Bxc15oRcWLPWdFmucg85Afkoy6OZ9bOI0bt4qsR4Ti?= =?us-ascii?Q?i9YKOdy8vcgJzdxPc0aSv84iePoAOHEBfcjSWUcnUmgjPbyDX2zgE5f9aTf8?= =?us-ascii?Q?8AnYxLOXI2aGoZvXZILQ110/MsIYc4szxiGlU3lN1uYewMRSul4wTUQR5wQo?= =?us-ascii?Q?+KPENQ5kbJiMoyjLbzwzYv0irfKRt9WdQuumOnsBDJAvhG3DSraFZECZesM2?= =?us-ascii?Q?ox4Jr0NMfue/yH8BnOejmDnx5c4737BeuVMkZpJ1XW2PgL7WPl2kjKiR65p4?= =?us-ascii?Q?0K8vdmJObed67TPfFhpD+S6rAuVMFJM3uztHKDF+pIdFgxFc211xTULaMecM?= =?us-ascii?Q?Jnbc7t1JfyxpKYWnXw0p6QeDE5S0ghzL7F2z+jt5hqwQSf8gU7nRxXVdBBQd?= =?us-ascii?Q?Cq1FJiBykMVOU7XURba2yVs5aW2l765BE5Oop7WYZLIzkPOleB4VzD6airmf?= =?us-ascii?Q?WmNR8hde4MeQpzG/0pK3cNfejcbml9QsAwXxd1ZwyDjTlT6IDXOlwMlfIEz1?= =?us-ascii?Q?rASN+vhOC04Usg513Ge63LE8L3KoxNEv52LXbsk0AgtBm+mY0VTDEkc10lGN?= =?us-ascii?Q?4aK1SNLu+Aug8bObUVGkoKtAAndukml3N8WumKdeQC7wQMAUpTXqofGsJsmm?= =?us-ascii?Q?kq47XkTgfizWvodQthAaodtZWm/KwPUayd3iylJFX16fyHsP4cubhuaaNkRv?= =?us-ascii?Q?X03NaSu1r081FUq5W+AZkG9avzT5m1ZcEhsA3jdP23YT0ewbO9k8j80CUh4H?= =?us-ascii?Q?7h22mE3eqXpNYHQ1Y4vUdhYuMDg+V6Gx5iv6+c20xJwmNf2fYiXTwb7IMkuk?= =?us-ascii?Q?+hgAkkRNENWRPc91dQhBJd1T3bPO5OE30uDTwSfRjR6OppnE2hbmpB+6f51I?= =?us-ascii?Q?Q2L0D9rn3joWo+rQSNxbXgKs8n3MLK5Nv69BYJAmdIkVQsSEqW5sdpqgvBN2?= =?us-ascii?Q?PGY+iuWGfFnXqAYFaLMEMtvIV5rWOSRDJml6SZBoV3oUndnyy7rmadLYt5o4?= =?us-ascii?Q?lW5vgC/yvK3px+wnN7kFgsqHSpNiYnRoBi3uMpLEqBpFmGdTPUWF5ei5GuuW?= =?us-ascii?Q?a7IWh96LTVziJKmP2W5g6nx7f/xQ2GuuYu0FJfjeQPdRRSC45uXGE2et3lJ9?= =?us-ascii?Q?PhRhYhJlIc/bFHL4xi2k09F3w5gIQua6ueGXwvHVtAlZYv6FaUCmA0ad4kOj?= =?us-ascii?Q?j59+nDNqP4sKqE8e+/QoIlPpD0vwbwV3ADfuy16lTNo9tZ3/XeJdwgC7SB8O?= =?us-ascii?Q?mXCGpbMc+FoGYOBLcmTrSXXWiz194JcLEinHXb3CXF5o7LKoL2+Dd4OiTQ6T?= =?us-ascii?Q?ZXc=3D?= x-ms-exchange-transport-forked: True MIME-Version: 1.0 X-OriginatorOrg: vmware.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: PH0PR05MB8702.namprd05.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 1c690b95-39e5-49b5-3c54-08d9854265ef X-MS-Exchange-CrossTenant-originalarrivaltime: 02 Oct 2021 01:17:27.2516 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0 X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: LatqRq7dJWx9d/kpiiEyGnwU9L2vMM7DULwehqgnDW7ywgxRyW8TyA+5NlcSqR5qWZ7DoGql0Q1Eekk6hitQEQ== X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR05MB8800 Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_PH0PR05MB8702D456E14045829608066DB9AC9PH0PR05MB8702namp_" --_000_PH0PR05MB8702D456E14045829608066DB9AC9PH0PR05MB8702namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Yikes - that's embarrassing. Thanks for the fix. Reviewed-by: Andrei Warkentin ________________________________ From: Jeremy Linton Sent: Friday, October 1, 2021 7:52 PM To: devel@edk2.groups.io Cc: pete@akeo.ie ; ardb+tianocore@kernel.org ; leif@nuviainc.com ; Andrei Warkentin ; Sunny.Wang@arm.com ; samer.el-haj-ma= hmoud@arm.com ; Jeremy Linton Subject: [PATCH 2/5] Platform/RaspberryPi: Expand locking to cover return d= ata 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 --- .../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 =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D RPI_MBOX_RESP_SUCCESS) { @@ -219,6 +218,7 @@ RpiFirmwareSetPowerState ( __FUNCTION__, PowerState ? "en" : "dis", DeviceId)); Status =3D EFI_DEVICE_ERROR; } + ReleaseSpinLock (&mMailboxLock); return Status; } @@ -266,18 +266,20 @@ RpiFirmwareGetArmMemory ( Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D RPI_MBOX_RESP_SUCCESS) { DEBUG ((DEBUG_ERROR, "%a: mailbox transaction error: Status =3D=3D %r, Response =3D=3D 0x= %x\n", __FUNCTION__, Status, Cmd->BufferHead.Response)); + ReleaseSpinLock (&mMailboxLock); return EFI_DEVICE_ERROR; } *Base =3D Cmd->TagBody.Base; *Size =3D Cmd->TagBody.Size; + ReleaseSpinLock (&mMailboxLock); + return EFI_SUCCESS; } @@ -323,17 +325,18 @@ RpiFirmwareGetMacAddress ( Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); - if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D RPI_MBOX_RESP_SUCCESS) { DEBUG ((DEBUG_ERROR, "%a: mailbox transaction error: Status =3D=3D %r, Response =3D=3D 0x= %x\n", __FUNCTION__, Status, Cmd->BufferHead.Response)); + ReleaseSpinLock (&mMailboxLock); return EFI_DEVICE_ERROR; } CopyMem (MacAddress, Cmd->TagBody.MacAddress, sizeof (Cmd->TagBody.MacAd= dress)); + ReleaseSpinLock (&mMailboxLock); + return EFI_SUCCESS; } @@ -378,17 +381,17 @@ RpiFirmwareGetSerial ( Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); - if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D RPI_MBOX_RESP_SUCCESS) { DEBUG ((DEBUG_ERROR, "%a: mailbox transaction error: Status =3D=3D %r, Response =3D=3D 0x= %x\n", __FUNCTION__, Status, Cmd->BufferHead.Response)); + ReleaseSpinLock (&mMailboxLock); return EFI_DEVICE_ERROR; } *Serial =3D Cmd->TagBody.Serial; + ReleaseSpinLock (&mMailboxLock); // Some platforms return 0 or 0x0000000010000000 for serial. // For those, try to use the MAC address. if ((*Serial =3D=3D 0) || ((*Serial & 0xFFFFFFFF0FFFFFFFULL) =3D=3D 0)) = { @@ -441,17 +444,18 @@ RpiFirmwareGetModel ( Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); - if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D RPI_MBOX_RESP_SUCCESS) { DEBUG ((DEBUG_ERROR, "%a: mailbox transaction error: Status =3D=3D %r, Response =3D=3D 0x= %x\n", __FUNCTION__, Status, Cmd->BufferHead.Response)); + ReleaseSpinLock (&mMailboxLock); return EFI_DEVICE_ERROR; } *Model =3D Cmd->TagBody.Model; + ReleaseSpinLock (&mMailboxLock); + return EFI_SUCCESS; } @@ -496,17 +500,18 @@ RpiFirmwareGetModelRevision ( Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); - if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D RPI_MBOX_RESP_SUCCESS) { DEBUG ((DEBUG_ERROR, "%a: mailbox transaction error: Status =3D=3D %r, Response =3D=3D 0x= %x\n", __FUNCTION__, Status, Cmd->BufferHead.Response)); + ReleaseSpinLock (&mMailboxLock); return EFI_DEVICE_ERROR; } *Revision =3D Cmd->TagBody.Revision; + ReleaseSpinLock (&mMailboxLock); + return EFI_SUCCESS; } @@ -538,17 +543,18 @@ RpiFirmwareGetFirmwareRevision ( Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); - if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D RPI_MBOX_RESP_SUCCESS) { DEBUG ((DEBUG_ERROR, "%a: mailbox transaction error: Status =3D=3D %r, Response =3D=3D 0x= %x\n", __FUNCTION__, Status, Cmd->BufferHead.Response)); + ReleaseSpinLock (&mMailboxLock); return EFI_DEVICE_ERROR; } *Revision =3D Cmd->TagBody.Revision; + ReleaseSpinLock (&mMailboxLock); + return EFI_SUCCESS; } @@ -831,18 +837,19 @@ RpiFirmwareGetFbSize ( Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); - if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D RPI_MBOX_RESP_SUCCESS) { DEBUG ((DEBUG_ERROR, "%a: mailbox transaction error: Status =3D=3D %r, Response =3D=3D 0= x%x\n", __FUNCTION__, Status, Cmd->BufferHead.Response)); + ReleaseSpinLock (&mMailboxLock); return EFI_DEVICE_ERROR; } *Width =3D Cmd->TagBody.Width; *Height =3D Cmd->TagBody.Height; + ReleaseSpinLock (&mMailboxLock); + return EFI_SUCCESS; } @@ -872,16 +879,18 @@ RpiFirmwareFreeFb (VOID) Cmd->EndTag =3D 0; Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D RPI_MBOX_RESP_SUCCESS) { DEBUG ((DEBUG_ERROR, "%a: mailbox transaction error: Status =3D=3D %r, Response =3D=3D 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 =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); - if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D RPI_MBOX_RESP_SUCCESS) { DEBUG ((DEBUG_ERROR, "%a: mailbox transaction error: Status =3D=3D %r, Response =3D=3D 0x= %x\n", __FUNCTION__, Status, Cmd->BufferHead.Response)); + ReleaseSpinLock (&mMailboxLock); return EFI_DEVICE_ERROR; } *Pitch =3D Cmd->Pitch.Pitch; *FbBase =3D Cmd->AllocFb.AlignmentBase - BCM2836_DMA_DEVICE_OFFSET; *FbSize =3D Cmd->AllocFb.Size; + ReleaseSpinLock (&mMailboxLock); + return EFI_SUCCESS; } @@ -999,13 +1009,12 @@ RpiFirmwareGetCommmandLine ( Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); - if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D RPI_MBOX_RESP_SUCCESS) { DEBUG ((DEBUG_ERROR, "%a: mailbox transaction error: Status =3D=3D %r, Response =3D=3D 0x= %x\n", __FUNCTION__, Status, Cmd->BufferHead.Response)); + ReleaseSpinLock (&mMailboxLock); return EFI_DEVICE_ERROR; } @@ -1013,6 +1022,7 @@ RpiFirmwareGetCommmandLine ( if (Cmd->TagHead.TagValueSize >=3D BufferSize && Cmd->CommandLine[Cmd->TagHead.TagValueSize - 1] !=3D '\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] =3D '\0'; } + ReleaseSpinLock (&mMailboxLock); return EFI_SUCCESS; } @@ -1075,18 +1086,20 @@ RpiFirmwareSetClockRate ( Cmd->TagBody.SkipTurbo =3D SkipTurbo ? 1 : 0; Cmd->EndTag =3D 0; + DEBUG ((DEBUG_ERROR, "%a: Request clock rate %X =3D %d\n", __FUNCTION__,= ClockId, ClockRate)); Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); - if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D RPI_MBOX_RESP_SUCCESS) { DEBUG ((DEBUG_ERROR, "%a: mailbox transaction error: Status =3D=3D %r, Response =3D=3D 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 =3D 0; Cmd->TagBody.ClockId =3D ClockId; Cmd->EndTag =3D 0; - + Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); - if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D RPI_MBOX_RESP_SUCCESS) { DEBUG ((DEBUG_ERROR, "%a: mailbox transaction error: Status =3D=3D %r, Response =3D=3D 0x= %x\n", __FUNCTION__, Status, Cmd->BufferHead.Response)); + ReleaseSpinLock (&mMailboxLock); return EFI_DEVICE_ERROR; } *ClockRate =3D Cmd->TagBody.ClockRate; + ReleaseSpinLock (&mMailboxLock); + + DEBUG ((DEBUG_ERROR, "%a: Get Clock Rate return: ClockRate=3D%d ClockId= =3D%X\n", __FUNCTION__, *ClockRate, ClockId)); + return EFI_SUCCESS; } @@ -1191,7 +1207,7 @@ RpiFirmwareGetMinClockRate ( { return RpiFirmwareGetClockRate (ClockId, RPI_MBOX_GET_MIN_CLOCK_RATE, Cl= ockRate); } - + #pragma pack() typedef struct { UINT32 ClockId; @@ -1236,16 +1252,17 @@ RpiFirmwareSetClockState ( Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); - if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D RPI_MBOX_RESP_SUCCESS) { DEBUG ((DEBUG_ERROR, "%a: mailbox transaction error: Status =3D=3D %r, Response =3D=3D 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 =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); - if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D RPI_MBOX_RESP_SUCCESS) { DEBUG ((DEBUG_ERROR, "%a: mailbox transaction error: Status =3D=3D %r, Response =3D=3D 0= x%x\n", __FUNCTION__, Status, Cmd->BufferHead.Response)); } + ReleaseSpinLock (&mMailboxLock); } - + STATIC VOID EFIAPI @@ -1361,8 +1377,6 @@ RpiFirmwareNotifyXhciReset ( Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); - if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D 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 =3D Cmd->TagBody.Polarity; - ReleaseSpinLock (&mMailboxLock); - if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D 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 =3D RpiFirmwareNotifyGpioGetCfg (Gpio, &Result); if (EFI_ERROR (Status)) { - DEBUG ((DEBUG_ERROR, "%a: Failed to get GPIO polarity\n", __FUNCT= ION__)); - Result =3D 0; //default polarity + DEBUG ((DEBUG_ERROR, "%a: Failed to get GPIO polarity\n", __FUNCTION= __)); + Result =3D 0; //default polarity } @@ -1488,7 +1504,7 @@ RpiFirmwareNotifyGpioSetCfg ( Cmd->BufferHead.Response =3D 0; Cmd->TagHead.TagId =3D RPI_MBOX_SET_GPIO_CONFIG; Cmd->TagHead.TagSize =3D sizeof (Cmd->TagBody); - + Cmd->TagBody.Gpio =3D 128 + Gpio; Cmd->TagBody.Direction =3D Direction; Cmd->TagBody.Polarity =3D Result; @@ -1501,17 +1517,17 @@ RpiFirmwareNotifyGpioSetCfg ( Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, RPI_MBOX_VC_C= HANNEL, &Result); - ReleaseSpinLock (&mMailboxLock); - if (EFI_ERROR (Status) || Cmd->BufferHead.Response !=3D RPI_MBOX_RESP_SUCCESS) { DEBUG ((DEBUG_ERROR, "%a: mailbox transaction error: Status =3D=3D %r, Response =3D=3D 0= x%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 mRpiFirmwarePro= tocol =3D { RPiFirmwareGetModelInstalledMB, RpiFirmwareNotifyXhciReset, RpiFirmwareGetCurrentClockState, - RpiFirmwareSetClockState, + RpiFirmwareSetClockState, RpiFirmwareNotifyGpioSetCfg }; -- 2.13.7 --_000_PH0PR05MB8702D456E14045829608066DB9AC9PH0PR05MB8702namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable
Yikes - that's embarrassing. Thanks for the fix.

Reviewed-by: Andrei Warkentin <awarkentin@vmware.com>
From: Jeremy Linton <jer= emy.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 <Su= nny.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 r= eturn data
 
While debugging problems with the GET/SET_CLOCK ma= ilbox 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<= br> 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<= br> 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    &n= bsp;   | 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 =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
 
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
@@ -219,6 +218,7 @@ RpiFirmwareSetPowerState (
       __FUNCTION__, PowerState ? "en&qu= ot; : "dis", DeviceId));
     Status =3D EFI_DEVICE_ERROR;
   }
+  ReleaseSpinLock (&mMailboxLock);
 
   return Status;
 }
@@ -266,18 +266,20 @@ RpiFirmwareGetArmMemory (
 
   Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
 
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
       "%a: mailbox transaction error: S= tatus =3D=3D %r, Response =3D=3D 0x%x\n",
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   *Base =3D Cmd->TagBody.Base;
   *Size =3D Cmd->TagBody.Size;
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -323,17 +325,18 @@ RpiFirmwareGetMacAddress (
 
   Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
-
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
       "%a: mailbox transaction error: S= tatus =3D=3D %r, Response =3D=3D 0x%x\n",
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   CopyMem (MacAddress, Cmd->TagBody.MacAddress, sizeof (Cmd-&= gt;TagBody.MacAddress));
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -378,17 +381,17 @@ RpiFirmwareGetSerial (
 
   Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
-
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
       "%a: mailbox transaction error: S= tatus =3D=3D %r, Response =3D=3D 0x%x\n",
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   *Serial =3D Cmd->TagBody.Serial;
+  ReleaseSpinLock (&mMailboxLock);
   // Some platforms return 0 or 0x0000000010000000 for serial.    // For those, try to use the MAC address.
   if ((*Serial =3D=3D 0) || ((*Serial & 0xFFFFFFFF0FFFFFFFUL= L) =3D=3D 0)) {
@@ -441,17 +444,18 @@ RpiFirmwareGetModel (
 
   Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
-
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
       "%a: mailbox transaction error: S= tatus =3D=3D %r, Response =3D=3D 0x%x\n",
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   *Model =3D Cmd->TagBody.Model;
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -496,17 +500,18 @@ RpiFirmwareGetModelRevision (
 
   Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
-
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
       "%a: mailbox transaction error: S= tatus =3D=3D %r, Response =3D=3D 0x%x\n",
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   *Revision =3D Cmd->TagBody.Revision;
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -538,17 +543,18 @@ RpiFirmwareGetFirmwareRevision (
 
   Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
-
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
       "%a: mailbox transaction error: S= tatus =3D=3D %r, Response =3D=3D 0x%x\n",
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   *Revision =3D Cmd->TagBody.Revision;
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -831,18 +837,19 @@ RpiFirmwareGetFbSize (
 
   Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
-
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
       "%a: mailbox  transaction er= ror: Status =3D=3D %r, Response =3D=3D 0x%x\n",
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   *Width =3D Cmd->TagBody.Width;
   *Height =3D Cmd->TagBody.Height;
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -872,16 +879,18 @@ RpiFirmwareFreeFb (VOID)
   Cmd->EndTag        =           =3D 0;
 
   Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
-  ReleaseSpinLock (&mMailboxLock);
 
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
       "%a: mailbox transaction error: S= tatus =3D=3D %r, Response =3D=3D 0x%x\n",
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -935,19 +944,20 @@ RpiFirmwareAllocFb (
 
   Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
-
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
       "%a: mailbox transaction error: S= tatus =3D=3D %r, Response =3D=3D 0x%x\n",
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   *Pitch =3D Cmd->Pitch.Pitch;
   *FbBase =3D Cmd->AllocFb.AlignmentBase - BCM2836_DMA_DEVICE= _OFFSET;
   *FbSize =3D Cmd->AllocFb.Size;
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -999,13 +1009,12 @@ RpiFirmwareGetCommmandLine (
 
   Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
-
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
       "%a: mailbox transaction error: S= tatus =3D=3D %r, Response =3D=3D 0x%x\n",
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
@@ -1013,6 +1022,7 @@ RpiFirmwareGetCommmandLine (
   if (Cmd->TagHead.TagValueSize >=3D BufferSize &&=
       Cmd->CommandLine[Cmd->TagHead.Ta= gValueSize - 1] !=3D '\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] =3D '\0'= ;
   }
 
+  ReleaseSpinLock (&mMailboxLock);
   return EFI_SUCCESS;
 }
 
@@ -1075,18 +1086,20 @@ RpiFirmwareSetClockRate (
   Cmd->TagBody.SkipTurbo      =3D Sk= ipTurbo ? 1 : 0;
   Cmd->EndTag        =          =3D 0;
 
+  DEBUG ((DEBUG_ERROR, "%a: Request clock rate %X =3D %d\n"= , __FUNCTION__, ClockId, ClockRate));
   Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
-
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
       "%a: mailbox transaction error: S= tatus =3D=3D %r, Response =3D=3D 0x%x\n",
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -1131,20 +1144,23 @@ RpiFirmwareGetClockRate (
   Cmd->TagHead.TagValueSize   =3D 0;
   Cmd->TagBody.ClockId      &nb= sp; =3D ClockId;
   Cmd->EndTag        =          =3D 0;
-
+
   Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
-
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
       "%a: mailbox transaction error: S= tatus =3D=3D %r, Response =3D=3D 0x%x\n",
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
   *ClockRate =3D Cmd->TagBody.ClockRate;
+  ReleaseSpinLock (&mMailboxLock);
+
+  DEBUG ((DEBUG_ERROR, "%a: Get Clock Rate return: ClockRate=3D%= d ClockId=3D%X\n", __FUNCTION__, *ClockRate, ClockId));
+
   return EFI_SUCCESS;
 }
 
@@ -1191,7 +1207,7 @@ RpiFirmwareGetMinClockRate (
 {
   return RpiFirmwareGetClockRate (ClockId, RPI_MBOX_GET_MIN_CLOC= K_RATE, ClockRate);
 }
-
+
 #pragma pack()
 typedef struct {
   UINT32         &n= bsp;          ClockId;
@@ -1236,16 +1252,17 @@ RpiFirmwareSetClockState (
 
   Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
-
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
       "%a: mailbox transaction error: S= tatus =3D=3D %r, Response =3D=3D 0x%x\n",
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
+    ReleaseSpinLock (&mMailboxLock);
     return EFI_DEVICE_ERROR;
   }
 
+  ReleaseSpinLock (&mMailboxLock);
+
   return EFI_SUCCESS;
 }
 
@@ -1297,16 +1314,15 @@ RpiFirmwareSetGpio (
 
   Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
-
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
       "%a: mailbox  transaction er= ror: Status =3D=3D %r, Response =3D=3D 0x%x\n",
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
   }
+  ReleaseSpinLock (&mMailboxLock);
 }
-
+
 STATIC
 VOID
 EFIAPI
@@ -1361,8 +1377,6 @@ RpiFirmwareNotifyXhciReset (
 
   Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
-
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
@@ -1370,6 +1384,8 @@ RpiFirmwareNotifyXhciReset (
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
   }
 
+  ReleaseSpinLock (&mMailboxLock);
+
   return Status;
 }
 
@@ -1424,8 +1440,6 @@ RpiFirmwareNotifyGpioGetCfg (
 
   *Polarity =3D Cmd->TagBody.Polarity;
 
-  ReleaseSpinLock (&mMailboxLock);
-
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
@@ -1433,6 +1447,8 @@ RpiFirmwareNotifyGpioGetCfg (
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
   }
 
+  ReleaseSpinLock (&mMailboxLock);
+
   return Status;
 }
 
@@ -1471,8 +1487,8 @@ RpiFirmwareNotifyGpioSetCfg (
 
   Status =3D RpiFirmwareNotifyGpioGetCfg (Gpio, &Result);    if (EFI_ERROR (Status)) {
-         DEBUG ((DEBUG_ERROR, &quo= t;%a: Failed to get GPIO polarity\n", __FUNCTION__));
-         Result =3D 0; //default p= olarity
+      DEBUG ((DEBUG_ERROR, "%a: Failed to ge= t GPIO polarity\n", __FUNCTION__));
+      Result =3D 0; //default polarity
   }
 
 
@@ -1488,7 +1504,7 @@ RpiFirmwareNotifyGpioSetCfg (
   Cmd->BufferHead.Response    =3D 0;
   Cmd->TagHead.TagId       = ;   =3D RPI_MBOX_SET_GPIO_CONFIG;
   Cmd->TagHead.TagSize      &nb= sp; =3D sizeof (Cmd->TagBody);
-
+
   Cmd->TagBody.Gpio =3D 128 + Gpio;
   Cmd->TagBody.Direction =3D Direction;
   Cmd->TagBody.Polarity =3D Result;
@@ -1501,17 +1517,17 @@ RpiFirmwareNotifyGpioSetCfg (
 
   Status =3D MailboxTransaction (Cmd->BufferHead.BufferSize, = RPI_MBOX_VC_CHANNEL, &Result);
 
-  ReleaseSpinLock (&mMailboxLock);
-
   if (EFI_ERROR (Status) ||
       Cmd->BufferHead.Response !=3D RPI_M= BOX_RESP_SUCCESS) {
     DEBUG ((DEBUG_ERROR,
       "%a: mailbox  transaction er= ror: Status =3D=3D %r, Response =3D=3D 0x%x\n",
       __FUNCTION__, Status, Cmd->BufferHe= ad.Response));
   }
-
-  RpiFirmwareSetGpio (Gpio,!State);
-
+
+  ReleaseSpinLock (&mMailboxLock);
+
+  RpiFirmwareSetGpio (Gpio,!State);
+
 
   return Status;
 }
@@ -1540,7 +1556,7 @@ STATIC RASPBERRY_PI_FIRMWARE_PROTOCOL mRpiFirmwarePro= tocol =3D {
   RPiFirmwareGetModelInstalledMB,
   RpiFirmwareNotifyXhciReset,
   RpiFirmwareGetCurrentClockState,
-  RpiFirmwareSetClockState,
+  RpiFirmwareSetClockState,
   RpiFirmwareNotifyGpioSetCfg
 };
 
--
2.13.7

--_000_PH0PR05MB8702D456E14045829608066DB9AC9PH0PR05MB8702namp_--