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 <nikita.leshche...@oracle.com> > Reviewed-by: Konrad Rzeszutek Wilk <konrad.w...@oracle.com> > Reviewed-by: Aaron Young <aaron.yo...@oracle.com> > Reviewed-by: Liran Alon <liran.a...@oracle.com> > --- > 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 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#55061): https://edk2.groups.io/g/devel/message/55061 Mute This Topic: https://groups.io/mt/71570013/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
