Re: [PATCH v2 09/13] OvmfPkg/MptScsiDxe: Open PciIo protocol for later use

["Laszlo Ersek" <lersek@redhat.com>]

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.leshchenko@oracle.com>
> Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> Reviewed-by: Aaron Young <aaron.young@oracle.com>
> Reviewed-by: Liran Alon <liran.alon@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