Re: [PATCH 04/14] OvmfPkg/MptScsiDxe: Probe PCI devices and look for MptScsi

[Laszlo Ersek <lersek@redhat.com>]

On 01/31/19 11:07, Nikita Leshenko wrote:
> The MptScsiControllerSupported function is called for every handle
> that appears on the system

This statement is incorrect. The Supported() function is generally
called on such handles that the ConnectController() boot service tries
to connect. More distantly, that depends on what devices the platform
BDS attempts to connect (which is platform policy). In some cases, the
platform BDS policy is to "connect all devices to all drivers", and then
you indeed end up with the above behavior.

(1) I suggest "the MptScsiControllerSupported function is called on
handles passed in by the ConnectController() boot service".

> and if the handle is the lsi53c1030
> controller the function would return success. A successful return
> value will attach our driver to the device.
> 
> 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      | 55 ++++++++++++++++++++++++++++++-
>  OvmfPkg/MptScsiDxe/MptScsiDxe.inf |  5 +++
>  2 files changed, 59 insertions(+), 1 deletion(-)
> 
> diff --git a/OvmfPkg/MptScsiDxe/MptScsi.c b/OvmfPkg/MptScsiDxe/MptScsi.c
> index 38cdda1abe..57a17ca0cb 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsi.c
> +++ b/OvmfPkg/MptScsiDxe/MptScsi.c
> @@ -15,7 +15,20 @@
>  
>  **/
>  
> +#include <IndustryStandard/Pci.h>
> +#include <Library/DebugLib.h>
>  #include <Library/UefiLib.h>
> +#include <Library/UefiBootServicesTableLib.h>
> +#include <Protocol/PciIo.h>

(2) Please keep this list alphabetically sorted. Sorting helps with
keeping (longer) #include lists unique, plus it also helps with matching
#include <Library/*.h> directives against the [LibraryClasses] in the
INF file (assuming we keep the latter sorted as well).

> +
> +//
> +// Device offsets and constants
> +//
> +
> +#define LSI_LOGIC_PCI_VENDOR_ID 0x1000
> +#define LSI_53C1030_PCI_DEVICE_ID 0x0030
> +#define LSI_SAS1068_PCI_DEVICE_ID 0x0054
> +#define LSI_SAS1068E_PCI_DEVICE_ID 0x0058

(3) These should go into a new file under OvmfPkg/Include/IndustryStandard.

I guess the filename could be "FusionMptScsi.h", but feel free to
propose another name.

... Also, referring back to my comment (3) under [PATCH 01/14], once you
#include <IndustryStandard/FusionMptScsi.h> here, that will be the
actual first need to spell out "OvmfPkg/OvmfPkg.dec" under [Packages],
for the INF file.

>  
>  //
>  // Driver Binding
> @@ -30,7 +43,46 @@ MptScsiControllerSupported (
>    IN EFI_DEVICE_PATH_PROTOCOL               *RemainingDevicePath OPTIONAL
>    )
>  {
> -  return EFI_UNSUPPORTED;
> +  EFI_STATUS          Status;
> +  EFI_PCI_IO_PROTOCOL *PciIo = NULL;

(4) The edk2 coding style forbids initialization of variables with
automatic storage duration. If you need PciIo set to NULL, please add a
separate assignment.

> +  PCI_TYPE00          Pci;
> +
> +  Status = gBS->OpenProtocol (
> +                  ControllerHandle,
> +                  &gEfiPciIoProtocolGuid,
> +                  (VOID **) &PciIo,

(5) In patches written for OvmfPkg, please never insert a space in the
middle of a cast expression. It should be

  (VOID **)&PciIo

Type casts have one of the strongest operator bindings in the C
language, and inserting a space in the middle suggests otherwise. We've
had actual bugs due to misleading formatting like this. (I know that
other package maintainers have different opinions, that's fine; they
don't review OvmfPkg, and I don't review their packages. :) )


> +                  This->DriverBindingHandle,
> +                  ControllerHandle,
> +                  EFI_OPEN_PROTOCOL_BY_DRIVER);

(6) The closing paren of the function call is incorrectly placed. It
should be on a separate line, aligned with the arguments. (Not a typo:
it should be aligned with the arguments, and not with the OpenProtocol
member name.)

> +  if (EFI_ERROR (Status)) {
> +    return Status;
> +  }
> +
> +  Status = PciIo->Pci.Read (
> +                  PciIo,
> +                  EfiPciIoWidthUint32,
> +                  0, sizeof (Pci) / sizeof (UINT32),
> +                  &Pci);

(7) The arguments should be indented two spaces relative to [R]ead.

(8) Same comment as (5) applies to the closing paren.

(9) The edk2 coding style requires us to place all arguments on separate
lines in a multi-line function call. As a concession in OvmfPkg, we also
use (or even prefer) a more condensed style:


  Status = PciIo->Pci.Read (PciIo, EfiPciIoWidthUint32, 0,
                        sizeof (Pci) / sizeof (UINT32), &Pci);

In this case, we're free to "flow" the arguments; however, the
indentation remains the same (two spaces relative to the member function
name).

> +  if (EFI_ERROR (Status)) {
> +    goto Done;
> +  }
> +
> +  if (Pci.Hdr.VendorId == LSI_LOGIC_PCI_VENDOR_ID
> +      && (Pci.Hdr.DeviceId == LSI_53C1030_PCI_DEVICE_ID
> +          || Pci.Hdr.DeviceId == LSI_SAS1068_PCI_DEVICE_ID
> +          || Pci.Hdr.DeviceId == LSI_SAS1068E_PCI_DEVICE_ID)) {

(10) Here's the right formatting for the controlling expression:

  if (Pci.Hdr.VendorId == LSI_LOGIC_PCI_VENDOR_ID &&
      (Pci.Hdr.DeviceId == LSI_53C1030_PCI_DEVICE_ID ||
       Pci.Hdr.DeviceId == LSI_SAS1068_PCI_DEVICE_ID ||
       Pci.Hdr.DeviceId == LSI_SAS1068E_PCI_DEVICE_ID)) {

Basically, logical operators go to the end of the line, and nesting
implies *one* space character.

> +    Status = EFI_SUCCESS;
> +  } else {
> +    Status = EFI_UNSUPPORTED;
> +  }
> +
> +Done:
> +  gBS->CloseProtocol (
> +                  ControllerHandle,
> +                  &gEfiPciIoProtocolGuid,
> +                  This->DriverBindingHandle,
> +                  ControllerHandle);

(11) See comments (7) through (9).

> +  return Status;
>  }
>  
>  STATIC
> @@ -42,6 +94,7 @@ MptScsiControllerStart (
>    IN EFI_DEVICE_PATH_PROTOCOL               *RemainingDevicePath OPTIONAL
>    )
>  {
> +  DEBUG ((EFI_D_INFO, "Attempted to start MptScsi\n"));

(12) We no longer use the EFI_D_ prefix; please use DEBUG_ instead.

(13) If we can express the same debug information by referencing the
function name, that's better. In such cases (entry/exit messages in
functions), we should use:

  DEBUG ((DEBUG_VERBOSE, "%a: enter\n", __FUNCTION__));

because:
- it will not clutter the log of a non-verbose build,
- the message will refresh itself if the function is renamed,
- if you have an editor with ctags support, the log message lets you
jump to the function without grepping,
- the format string is more likely to match other such format strings,
which leads to better LZMA compression over the firmware volume(s).

(Side comment: in library functions, it may also make sense to print
"gEfiCallerBasename", also with "%a"; it contains the name of the driver
or application into which the library instance was statically linked.
Not necessary here, the function name is unique enough.)

>    return EFI_UNSUPPORTED;
>  }
>  
> diff --git a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> index 8a780a661e..3608cecaab 100644
> --- a/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> +++ b/OvmfPkg/MptScsiDxe/MptScsiDxe.inf
> @@ -30,5 +30,10 @@
>    OvmfPkg/OvmfPkg.dec
>  
>  [LibraryClasses]
> +  DebugLib
> +  UefiBootServicesTableLib
>    UefiDriverEntryPoint
>    UefiLib

Right, this looks well ordered.

> +
> +[Protocols]
> +  gEfiPciIoProtocolGuid        ## TO_START
> \ No newline at end of file
> 

(14) Hmmm, not sure how I missed "No newline at end of file" in the
earlier patches, but that should be fixed.

OK, I think I'll stop reviewing version 1 at this point. The style
issues force me to comment on them, and it's hard to focus on the actual
functionality that way. Please rework the series (all patches) based on
these guidelines. To re-iterate those that affect multiple (or all) patches:

- fix the years in the copyright notices
- make sure you use CRLF line terminators
- reference the BZ in the commit messages
- move interface macros to OvmfPkg/Include/IndustryStandard/...
- move remaining macros to a module-specific .h file, or at least to the
  top of the .c file
- comment style -- use empty "//" before and after
- use "m" prefix for module global variables
- stick with 80 chars line length
- indentation and general formatting of:
  - controlling expressions,
  - function calls
- don't initialize local variables
- keep #include lists and [LibraryClasses] sections sorted
- no extra space within cast expressions
- debug messages: use DEBUG_* for debug masks, and %a/__FUNCTION__ for
  referring to the containing function

I'll make an effort to review v2 reasonably quickly.

Thanks
Laszlo