From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web12.11879.1582883411635794414 for ; Fri, 28 Feb 2020 01:50:11 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=L9DrgCdp; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1582883410; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Sm4aYXshfC9sMKpJqCcH+nCDv1te3b3adNstSX6Iphk=; b=L9DrgCdp0t/Kb2P7Vb23IInI6wZOJk0n/mtldxq1BIrhd1Kmtru63/9dJFu37/LMMIViAl w6wPvehFYh4XLhRj5EAEinTySs4JpQ9QBownvbxHFAzSC3SlFPaVXM+5brr4E535V1QEh8 6l7wjaBDlJN1g6aPf6/KwrvL/1IlJdo= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-102-y0Z5Re9WNLWuIFpXhJ67sg-1; Fri, 28 Feb 2020 04:50:04 -0500 X-MC-Unique: y0Z5Re9WNLWuIFpXhJ67sg-1 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id CB83B13B5A2; Fri, 28 Feb 2020 09:50:02 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-243.ams2.redhat.com [10.36.116.243]) by smtp.corp.redhat.com (Postfix) with ESMTP id 0F86B8C087; Fri, 28 Feb 2020 09:50:00 +0000 (UTC) Subject: Re: [PATCH v2 09/13] OvmfPkg/MptScsiDxe: Open PciIo protocol for later use To: Nikita Leshenko , devel@edk2.groups.io Cc: liran.alon@oracle.com, aaron.young@oracle.com, jordan.l.justen@intel.com, ard.biesheuvel@linaro.org References: <20200226164151.125182-1-nikita.leshchenko@oracle.com> <20200226164151.125182-10-nikita.leshchenko@oracle.com> From: "Laszlo Ersek" Message-ID: <3f352fe2-fec1-d827-dc32-b95e5bea8c1f@redhat.com> Date: Fri, 28 Feb 2020 10:50:00 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20200226164151.125182-10-nikita.leshchenko@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 02/26/20 17:41, Nikita Leshenko wrote: > This will give us an exclusive access to the PciIo of this device > after it was started and until is will be stopped. (1) s/is/it/ > > Ref: https://bugzilla.tianocore.org/show_bug.cgi?id=2390 > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Nikita Leshenko > Reviewed-by: Konrad Rzeszutek Wilk > Reviewed-by: Aaron Young > Reviewed-by: Liran Alon > --- > OvmfPkg/MptScsiDxe/MptScsi.c | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c > index d72af2b3f7..22001da763 100644 > --- a/OvmfPkg/MptScsiDxe/MptScsi.c > +++ b/OvmfPkg/MptScsiDxe/MptScsi.c > @@ -40,6 +40,7 @@ typedef struct { > UINT32 Signature; > EFI_EXT_SCSI_PASS_THRU_PROTOCOL PassThru; > EFI_EXT_SCSI_PASS_THRU_MODE PassThruMode; > + EFI_PCI_IO_PROTOCOL *PciIo; > } MPT_SCSI_DEV; > > #define MPT_SCSI_FROM_PASS_THRU(PassThruPtr) \ > @@ -270,6 +271,18 @@ MptScsiControllerStart ( > > Dev->Signature = MPT_SCSI_DEV_SIGNATURE; > > + Status = gBS->OpenProtocol ( > + ControllerHandle, > + &gEfiPciIoProtocolGuid, > + (VOID **)&Dev->PciIo, > + This->DriverBindingHandle, > + ControllerHandle, > + EFI_OPEN_PROTOCOL_BY_DRIVER > + ); > + if (EFI_ERROR (Status)) { > + goto Done; > + } > + > // > // Host adapter channel, doesn't exist > // > @@ -299,6 +312,15 @@ MptScsiControllerStart ( > > Done: > if (EFI_ERROR (Status)) { > + if (Dev->PciIo) { > + gBS->CloseProtocol ( > + ControllerHandle, > + &gEfiPciIoProtocolGuid, > + This->DriverBindingHandle, > + ControllerHandle > + ); > + } > + > FreePool (Dev); > } > I don't like where this is going. It is bad style to mix: - "goto" statements that jump to the end of the function "epilogue" - with *conditional* releasing of resources in the epilogue *unless*: (a) some of those resources are indeed optional, or (b) we intentionally reuse the epilogue for both success and error (that is, when the epilogue releases *temporary* resources) In this case, neither excuse (a) or excuse (b) apply, so please don't do this. I was a bit concerned at patch "OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU" already, seeing how you added an EFI_ERROR() check under the "Done" label. And this patch confirms (and the final state of the function proves) the direction we're headed, and it's not good. Mixing goto with conditionalized release is the most difficult approach to reason about. With nested "ifs", the explicit block scopes help us reason about lifecycles. With a cascade of labels, the label names help us reason about lifecycles. But if we have just one label, that gives us neither useful label names, nor scoping help, and we're down to awkwardly checking whether each individual resource should be released or not. This forces the reviewer to think about a combinatorial explosion of *seemingly* possible states. Either use nested "if"s *only* (no gotos), or use "goto"s exclusively (multiple lables, no "if" nesting). Again, unless we have excuse (a) or (b), but those don't apply now. And, in edk2, we don't generally use nested "if"s, because identifiers are long, so we don't want to waste horizontal space on deep indentation. So please stick with the "goto"s. So, in the final state of this function, the epilogue should reflect the Stop() function almost verbatim, except you'd have different labels (a cascade of labels) placed between the individual actions. The cascade (releasing of resources) should occur in reverse order of allocation. And, instead of introducing awkward BOOLEAN variables like "PciAttributesChanged", the context (jump origin, and jump target) would express what resources need to be released. In particular, in patch "OvmfPkg/MptScsiDxe: Install stubbed EXT_SCSI_PASS_THRU", the pattern should be laid out like this: ----------- Status = gBS->InstallProtocolInterface (...); if (EFI_ERROR (Status)) { goto FreeDev; } return EFI_SUCCESS; FreeDev: FreePool (Dev); return Status; ----------- and then the rest of the patches should build upon that -- introduce new labels always at the top of the existent "stack" of labels. > @@ -339,6 +361,13 @@ MptScsiControllerStop ( > &Dev->PassThru > ); > > + gBS->CloseProtocol ( > + ControllerHandle, > + &gEfiPciIoProtocolGuid, > + This->DriverBindingHandle, > + ControllerHandle > + ); > + > FreePool (Dev); > > return Status; > This hunk is good. Thanks Laszlo