Re: [PATCH 02/14] OvmfPkg/MptScsiDxe: Install DriverBinding Protocol

[Laszlo Ersek <lersek@redhat.com>]

On 01/31/19 11:07, Nikita Leshenko wrote:
> In order to probe and connect to the MptScsi device we need this
> protocol. Currently it does nothing.
> 
> 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      | 65 ++++++++++++++++++++++++++++++-
>  OvmfPkg/MptScsiDxe/MptScsiDxe.inf |  1 +
>  2 files changed, 65 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 73a693741d..4dcb1b1ae5 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -15,6 +15,62 @@
>  
>  **/
>  
> +#include <Library/UefiLib.h>
> +
> +//
> +// Driver Binding
> +//
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +MptScsiControllerSupported (
> +  IN EFI_DRIVER_BINDING_PROTOCOL            *This,
> +  IN EFI_HANDLE                             ControllerHandle,
> +  IN EFI_DEVICE_PATH_PROTOCOL               *RemainingDevicePath OPTIONAL
> +  )
> +{
> +  return EFI_UNSUPPORTED;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +MptScsiControllerStart (
> +  IN EFI_DRIVER_BINDING_PROTOCOL            *This,
> +  IN EFI_HANDLE                             ControllerHandle,
> +  IN EFI_DEVICE_PATH_PROTOCOL               *RemainingDevicePath OPTIONAL
> +  )
> +{
> +  return EFI_UNSUPPORTED;
> +}
> +
> +STATIC
> +EFI_STATUS
> +EFIAPI
> +MptScsiControllerStop (
> +  IN EFI_DRIVER_BINDING_PROTOCOL            *This,
> +  IN  EFI_HANDLE                            ControllerHandle,
> +  IN  UINTN                                 NumberOfChildren,
> +  IN  EFI_HANDLE                            *ChildHandleBuffer
> +  )
> +{
> +  return EFI_UNSUPPORTED;
> +}
> +
> +// Higher versions will be used before lower, 0x10-0xffffffef is the version
> +// range for IVH (Indie Hardware Vendors)
> +#define MPT_SCSI_BINDING_VERSION 0x10

If we'd like to use a macro + a comment for this, then:

(1) I suggest moving them near the top of the file (just below the
#include(s))

(2) The comment should be formatted as

//
// Higher versions...
// ...
//

I.e., add an empty "//" comment line, both before and after.


> +STATIC
> +EFI_DRIVER_BINDING_PROTOCOL gMptScsiDriverBinding = {

(3) Please replace the "g" prefix with "m". This object is not
firmware-"g"lobal but specific to the "m"odule. (Yes, I agree we used it
inconsistently in OvmfPkg in the past.)

> +  &MptScsiControllerSupported,
> +  &MptScsiControllerStart,
> +  &MptScsiControllerStop,
> +  MPT_SCSI_BINDING_VERSION,
> +  NULL, // ImageHandle filled by EfiLibInstallDriverBindingComponentName2
> +  NULL, // DriverBindingHandle filled by EfiLibInstallDriverBindingComponentName2

(4) This line is too long (81 chars). Please make sure that no new line
added in this series exceeds 80 characters in length.

> +};
> +
>  //
>  // Entry Point
>  //
> @@ -26,5 +82,12 @@ MptScsiEntryPoint (
>    IN EFI_SYSTEM_TABLE *SystemTable
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  return EfiLibInstallDriverBindingComponentName2 (
> +    ImageHandle,
> +    SystemTable,
> +    &gMptScsiDriverBinding,
> +    ImageHandle, // The handle to install onto
> +    NULL, // TODO Component name
> +    NULL // TODO Component name
> +    );

(5) The indentation of the arguments is not idiomatic; it should be two
spaces relative to the first letter ("E") in
"EfiLibInstallDriverBindingComponentName2".

>  }
> diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> index c558d78034..8a780a661e 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -31,3 +31,4 @@
>  
>  [LibraryClasses]
>    UefiDriverEntryPoint
> +  UefiLib
> 

Thanks
Laszlo