From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=209.132.183.28; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 1CAA7211C6075 for ; Fri, 1 Feb 2019 03:48:56 -0800 (PST) Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 892787F76D; Fri, 1 Feb 2019 11:48:55 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-49.rdu2.redhat.com [10.10.120.49]) by smtp.corp.redhat.com (Postfix) with ESMTP id 75572600CC; Fri, 1 Feb 2019 11:48:54 +0000 (UTC) To: Nikita Leshenko , edk2-devel@lists.01.org Cc: liran.alon@oracle.com References: <20190131100724.20907-1-nikita.leshchenko@oracle.com> <20190131100724.20907-5-nikita.leshchenko@oracle.com> From: Laszlo Ersek Message-ID: Date: Fri, 1 Feb 2019 12:48:53 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: <20190131100724.20907-5-nikita.leshchenko@oracle.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.11 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.27]); Fri, 01 Feb 2019 11:48:55 +0000 (UTC) Subject: Re: [PATCH 04/14] OvmfPkg/MptScsiDxe: Probe PCI devices and look for MptScsi X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 01 Feb 2019 11:48:57 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit 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.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