From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from aserp2120.oracle.com (aserp2120.oracle.com [141.146.126.78]) by mx.groups.io with SMTP id smtpd.web11.6993.1585655222177623400 for ; Tue, 31 Mar 2020 04:47:02 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@oracle.com header.s=corp-2020-01-29 header.b=Trh/tiRu; spf=pass (domain: oracle.com, ip: 141.146.126.78, mailfrom: liran.alon@oracle.com) Received: from pps.filterd (aserp2120.oracle.com [127.0.0.1]) by aserp2120.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 02VBhNbN190069; Tue, 31 Mar 2020 11:47:01 GMT DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=from : to : cc : subject : date : message-id : mime-version : content-transfer-encoding; s=corp-2020-01-29; bh=kEN3zjIQYR4UyQTkNSZVVdaPgABgx629E4eT2W9F1Zg=; b=Trh/tiRuFJ8VvXegw8OviCgQjDL5keiIfCgPtU4HUKCxELRW9OcgtoQMc/ifUhrEMJ5t B9StCH9T55UlZWGAKoif380d6M4TFMRm7pv1TC2WAiBmyuQLJnUxVu9O7ifkrGy+D7+Q LS0UOVu25NxQx2cmPQAIdcaOuYL/8c90XZc9RtsqexjxsHPTZCNz8wFi8qveZhmDKzc+ QJKceT9wTSwRsnnGV/KpsB3dQASX13aiFaLh6ljwoHbtpMEKKgRwORaSklj1ftDZsOrI AfdzgEN6cMO7PN4iy3e7TOoOqbT0vX3uyCki2xL+QUlD+s8ohOBV8uZZWfECZDzM0Ms8 1Q== Received: from aserp3020.oracle.com (aserp3020.oracle.com [141.146.126.70]) by aserp2120.oracle.com with ESMTP id 303yun1n9g-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 31 Mar 2020 11:47:01 +0000 Received: from pps.filterd (aserp3020.oracle.com [127.0.0.1]) by aserp3020.oracle.com (8.16.0.42/8.16.0.42) with SMTP id 02VBhYXq055854; Tue, 31 Mar 2020 11:45:01 GMT Received: from userv0122.oracle.com (userv0122.oracle.com [156.151.31.75]) by aserp3020.oracle.com with ESMTP id 302gccbfk5-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES256-GCM-SHA384 bits=256 verify=OK); Tue, 31 Mar 2020 11:45:00 +0000 Received: from abhmp0002.oracle.com (abhmp0002.oracle.com [141.146.116.8]) by userv0122.oracle.com (8.14.4/8.14.4) with ESMTP id 02VBivjJ013483; Tue, 31 Mar 2020 11:44:58 GMT Received: from spark.ravello.local (/213.57.127.2) by default (Oracle Beehive Gateway v4.0) with ESMTP ; Tue, 31 Mar 2020 04:44:57 -0700 From: "Liran Alon" To: devel@edk2.groups.io, lersek@redhat.com Cc: nikita.leshchenko@oracle.com, aaron.young@oracle.com, jordan.l.justen@intel.com, ard.biesheuvel@linaro.org, Liran Alon Subject: [PATCH] OvmfPkg/PvScsiDxe: Refactor setup of rings to separate function Date: Tue, 31 Mar 2020 14:47:30 +0300 Message-Id: <20200331114730.62947-1-liran.alon@oracle.com> X-Mailer: git-send-email 2.20.1 MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9576 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 bulkscore=0 spamscore=0 malwarescore=0 mlxlogscore=999 adultscore=0 suspectscore=2 phishscore=0 mlxscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003310107 X-Proofpoint-Virus-Version: vendor=nai engine=6000 definitions=9576 signatures=668685 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 phishscore=0 lowpriorityscore=0 malwarescore=0 adultscore=0 priorityscore=1501 mlxlogscore=999 bulkscore=0 suspectscore=2 mlxscore=0 spamscore=0 impostorscore=0 clxscore=1015 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2003020000 definitions=main-2003310107 Content-Transfer-Encoding: 8bit Previous to this change, PvScsiFreeRings() was not undoing all operations that was done by PvScsiInitRings(). This is because PvScsiInitRings() was both preparing rings (Allocate memory and map it for device DMA) and setup the rings against device by issueing a device command. While PvScsiFreeRings() only unmaps the rings and free their memory. Driver do not have a functional error as it makes sure to reset device before every call site to PvScsiFreeRings(). However, this is not intuitive. Therefore, prefer to refactor the setup of the ring against device to a separate function than PvScsiInitRings(). Signed-off-by: Liran Alon --- OvmfPkg/PvScsiDxe/PvScsi.c | 150 +++++++++++++++++++------------------ 1 file changed, 76 insertions(+), 74 deletions(-) diff --git a/OvmfPkg/PvScsiDxe/PvScsi.c b/OvmfPkg/PvScsiDxe/PvScsi.c index 1ca50390c0e5..5b7fdcbda10b 100644 --- a/OvmfPkg/PvScsiDxe/PvScsi.c +++ b/OvmfPkg/PvScsiDxe/PvScsi.c @@ -991,13 +991,6 @@ PvScsiInitRings ( ) { EFI_STATUS Status; - union { - PVSCSI_CMD_DESC_SETUP_RINGS Cmd; - UINT32 Uint32; - } AlignedCmd; - PVSCSI_CMD_DESC_SETUP_RINGS *Cmd; - - Cmd = &AlignedCmd.Cmd; Status = PvScsiAllocateSharedPages ( Dev, @@ -1032,6 +1025,69 @@ PvScsiInitRings ( } ZeroMem (Dev->RingDesc.RingCmps, EFI_PAGE_SIZE); + return EFI_SUCCESS; + +FreeRingReqs: + PvScsiFreeSharedPages ( + Dev, + 1, + Dev->RingDesc.RingReqs, + &Dev->RingDesc.RingReqsDmaDesc + ); + +FreeRingState: + PvScsiFreeSharedPages ( + Dev, + 1, + Dev->RingDesc.RingState, + &Dev->RingDesc.RingStateDmaDesc + ); + + return Status; +} + +STATIC +VOID +PvScsiFreeRings ( + IN OUT PVSCSI_DEV *Dev + ) +{ + PvScsiFreeSharedPages ( + Dev, + 1, + Dev->RingDesc.RingCmps, + &Dev->RingDesc.RingCmpsDmaDesc + ); + + PvScsiFreeSharedPages ( + Dev, + 1, + Dev->RingDesc.RingReqs, + &Dev->RingDesc.RingReqsDmaDesc + ); + + PvScsiFreeSharedPages ( + Dev, + 1, + Dev->RingDesc.RingState, + &Dev->RingDesc.RingStateDmaDesc + ); +} + +STATIC +EFI_STATUS +PvScsiSetupRings ( + IN OUT PVSCSI_DEV *Dev + ) +{ + union { + PVSCSI_CMD_DESC_SETUP_RINGS Cmd; + UINT32 Uint32; + } AlignedCmd; + PVSCSI_CMD_DESC_SETUP_RINGS *Cmd; + + Cmd = &AlignedCmd.Cmd; + ZeroMem (Cmd, sizeof (*Cmd)); Cmd->ReqRingNumPages = 1; Cmd->CmpRingNumPages = 1; @@ -1052,71 +1108,12 @@ PvScsiInitRings ( sizeof (*Cmd) % sizeof (UINT32) == 0, "Cmd must be multiple of 32-bit words" ); - Status = PvScsiWriteCmdDesc ( - Dev, - PvScsiCmdSetupRings, - (UINT32 *)Cmd, - sizeof (*Cmd) / sizeof (UINT32) - ); - if (EFI_ERROR (Status)) { - goto FreeRingCmps; - } - - return EFI_SUCCESS; - -FreeRingCmps: - PvScsiFreeSharedPages ( - Dev, - 1, - Dev->RingDesc.RingCmps, - &Dev->RingDesc.RingCmpsDmaDesc - ); - -FreeRingReqs: - PvScsiFreeSharedPages ( - Dev, - 1, - Dev->RingDesc.RingReqs, - &Dev->RingDesc.RingReqsDmaDesc - ); - -FreeRingState: - PvScsiFreeSharedPages ( - Dev, - 1, - Dev->RingDesc.RingState, - &Dev->RingDesc.RingStateDmaDesc - ); - - return Status; -} - -STATIC -VOID -PvScsiFreeRings ( - IN OUT PVSCSI_DEV *Dev - ) -{ - PvScsiFreeSharedPages ( - Dev, - 1, - Dev->RingDesc.RingCmps, - &Dev->RingDesc.RingCmpsDmaDesc - ); - - PvScsiFreeSharedPages ( - Dev, - 1, - Dev->RingDesc.RingReqs, - &Dev->RingDesc.RingReqsDmaDesc - ); - - PvScsiFreeSharedPages ( - Dev, - 1, - Dev->RingDesc.RingState, - &Dev->RingDesc.RingStateDmaDesc - ); + return PvScsiWriteCmdDesc ( + Dev, + PvScsiCmdSetupRings, + (UINT32 *)Cmd, + sizeof (*Cmd) / sizeof (UINT32) + ); } STATIC @@ -1157,6 +1154,10 @@ PvScsiInit ( if (EFI_ERROR (Status)) { goto RestorePciAttributes; } + Status = PvScsiSetupRings (Dev); + if (EFI_ERROR (Status)) { + goto FreeRings; + } // // Allocate DMA communication buffer @@ -1168,7 +1169,7 @@ PvScsiInit ( &Dev->DmaBufDmaDesc ); if (EFI_ERROR (Status)) { - goto FreeRings; + goto UnsetupRings; } // @@ -1202,13 +1203,14 @@ PvScsiInit ( return EFI_SUCCESS; -FreeRings: +UnsetupRings: // // Reset device to stop device usage of the rings. // This is required to safely free the rings. // PvScsiResetAdapter (Dev); +FreeRings: PvScsiFreeRings (Dev); RestorePciAttributes: -- 2.20.1