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
> Reviewed-by: Konrad Rzeszutek Wilk
> Reviewed-by: Aaron Young
> Reviewed-by: Liran Alon
> ---
> 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
> +#include
> #include
> +#include
> +#include
(2) Please keep this list alphabetically sorted. Sorting helps with
keeping (longer) #include lists unique, plus it also helps with matching
#include 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 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.H
