From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 6C2DBD80A13 for ; Tue, 7 Nov 2023 17:20:54 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=uZlQbrNb026ATmpBLJ+v9h1fGjOP9eSfnd5MZz17W88=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1699377653; v=1; b=uwgkXbqHFGPQteILgZVJ3nF9N+QikCKhQ04HIbLbONgXaKXAbq6jklwjI9HiEIIpS+ZCsV7e kbVQdy0e4j1m2kFaPyDdlMzj0P7XbQTiQhVbQ8KlweOR0Am7tLsg1Fi4Y4jSOjWowkL9u1Qfz8j BevazIO3v01Z0E/vq2OAqTp4= X-Received: by 127.0.0.2 with SMTP id jyPyYY7687511xq729ULiGkT; Tue, 07 Nov 2023 09:20:53 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.694.1699377652313721515 for ; Tue, 07 Nov 2023 09:20:52 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-58-3dBbOoHyNsmsMfkeMQpWdw-1; Tue, 07 Nov 2023 12:20:48 -0500 X-MC-Unique: 3dBbOoHyNsmsMfkeMQpWdw-1 X-Received: from smtp.corp.redhat.com (int-mx03.intmail.prod.int.rdu2.redhat.com [10.11.54.3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id D1B6F84AF86; Tue, 7 Nov 2023 17:20:47 +0000 (UTC) X-Received: from [10.39.193.64] (unknown [10.39.193.64]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 280AE1121306; Tue, 7 Nov 2023 17:20:47 +0000 (UTC) Message-ID: <3d51a063-5687-9c40-ace2-a9aeb7f3afa3@redhat.com> Date: Tue, 7 Nov 2023 18:20:45 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2 5/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues To: devel@edk2.groups.io, rsingh@ventanamicro.com Cc: Ray Ni References: <20231107061959.113213-1-rsingh@ventanamicro.com> <20231107061959.113213-6-rsingh@ventanamicro.com> From: "Laszlo Ersek" In-Reply-To: <20231107061959.113213-6-rsingh@ventanamicro.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.3 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: E3vc1wgQI7O7oS2RzimmjdxQx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=uwgkXbqH; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) On 11/7/23 07:19, Ranbir Singh wrote: > The return value after calls to functions > gBS->UninstallMultipleProtocolInterfaces, StartPciDevicesOnBridge, > PciPciDeviceInfoCollector, BarExisted, PciRootBridgeIo->Pci.Write, > gPciHotPlugInit->InitializeRootHpc and PciRootBridgeP2CProcess is > stored in Status, but it is not made of any use and later Status > gets overridden. > > Remove the return value storage in Status or add Status check as > seems appropriate at a particular point. > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239 > > Cc: Ray Ni > Signed-off-by: Ranbir Singh > --- > MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 68 +++++++++++--------- > MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 42 ++++++++---- > MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 8 +++ > 3 files changed, 72 insertions(+), 46 deletions(-) First of all, please split up this patch. It's hard to review it like this, with unrelated pieces of logic lumped together. > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > index 3de80d98370e..9b0587c94d05 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c > @@ -544,12 +544,12 @@ DeRegisterPciDevice ( > EFI_OPEN_PROTOCOL_TEST_PROTOCOL > ); > if (!EFI_ERROR (Status)) { > - Status = gBS->UninstallMultipleProtocolInterfaces ( > - Handle, > - &gEfiLoadFile2ProtocolGuid, > - &PciIoDevice->LoadFile2, > - NULL > - ); > + gBS->UninstallMultipleProtocolInterfaces ( > + Handle, > + &gEfiLoadFile2ProtocolGuid, > + &PciIoDevice->LoadFile2, > + NULL > + ); > } > > // OK > @@ -678,19 +678,21 @@ StartPciDevicesOnBridge ( > ChildHandleBuffer > ); > > - PciIoDevice->PciIo.Attributes ( > - &(PciIoDevice->PciIo), > - EfiPciIoAttributeOperationSupported, > - 0, > - &Supports > - ); > - Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; > - PciIoDevice->PciIo.Attributes ( > - &(PciIoDevice->PciIo), > - EfiPciIoAttributeOperationEnable, > - Supports, > - NULL > - ); > + if (!EFI_ERROR (Status)) { > + PciIoDevice->PciIo.Attributes ( > + &(PciIoDevice->PciIo), > + EfiPciIoAttributeOperationSupported, > + 0, > + &Supports > + ); > + Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; > + PciIoDevice->PciIo.Attributes ( > + &(PciIoDevice->PciIo), > + EfiPciIoAttributeOperationEnable, > + Supports, > + NULL > + ); > + } > > return Status; > } else { OK > @@ -726,19 +728,21 @@ StartPciDevicesOnBridge ( > ChildHandleBuffer > ); > > - PciIoDevice->PciIo.Attributes ( > - &(PciIoDevice->PciIo), > - EfiPciIoAttributeOperationSupported, > - 0, > - &Supports > - ); > - Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; > - PciIoDevice->PciIo.Attributes ( > - &(PciIoDevice->PciIo), > - EfiPciIoAttributeOperationEnable, > - Supports, > - NULL > - ); > + if (!EFI_ERROR (Status)) { > + PciIoDevice->PciIo.Attributes ( > + &(PciIoDevice->PciIo), > + EfiPciIoAttributeOperationSupported, > + 0, > + &Supports > + ); > + Supports &= (UINT64)EFI_PCI_DEVICE_ENABLE; > + PciIoDevice->PciIo.Attributes ( > + &(PciIoDevice->PciIo), > + EfiPciIoAttributeOperationEnable, > + Supports, > + NULL > + ); > + } > } > > CurrentLink = CurrentLink->ForwardLink; I don't really like this; the original code is inconsistent. In the first branch, StartPciDevicesOnBridge() failure is fatal, here it isn't. :/ Anyway, I agree we can at least restrict the enablement. OK. > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > index eda97285ee18..636885dd189d 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > @@ -2796,6 +2796,20 @@ IsPciDeviceRejected ( > // Test its high 32-Bit BAR > // > Status = BarExisted (PciIoDevice, BarOffset, &TestValue, &OldValue); > + if (EFI_ERROR (Status)) { > + // > + // Not sure if it is correct to skip the below if (TestValue == OldValue) check in this special scenario. > + // If correct, then remove these 11 comment lines eventually. > + // If not correct, then replace "continue;" with blank "; // Nothing to do" and also remove these 11 comment lines eventually > + // OR > + // Remove the newly added if (EFI_ERROR (Status)) { ... } block completely and change > + // Status = BarExisted (PciIoDevice, BarOffset, &TestValue, &OldValue); > + // => > + // BarExisted (PciIoDevice, BarOffset, &TestValue, &OldValue); > + // i.e., no return value storage in Status > + // > + continue; > + } > if (TestValue == OldValue) { > return TRUE; > } "continue" is not right here. We have already determined (from the least significant dword of the BAR) that the BAR exists. Continue seems only right when the BAR doesn't exist. In my opinion (but Ray should correct me if I'm wrong), we should not assign Status here, as we don't care whether BarExisted() finds that the response Value from the device is zero or not. That only matters if we're testing the low qword. So just remove "Status =". > @@ -2861,13 +2875,13 @@ ResetAllPpbBusNumber ( > if (!EFI_ERROR (Status) && (IS_PCI_BRIDGE (&Pci))) { > Register = 0; > Address = EFI_PCI_ADDRESS (StartBusNumber, Device, Func, 0x18); > - Status = PciRootBridgeIo->Pci.Read ( > - PciRootBridgeIo, > - EfiPciWidthUint32, > - Address, > - 1, > - &Register > - ); > + PciRootBridgeIo->Pci.Read ( > + PciRootBridgeIo, > + EfiPciWidthUint32, > + Address, > + 1, > + &Register > + ); > SecondaryBus = (UINT8)(Register >> 8); > > if (SecondaryBus != 0) { > @@ -2878,13 +2892,13 @@ ResetAllPpbBusNumber ( > // Reset register 18h, 19h, 1Ah on PCI Bridge > // > Register &= 0xFF000000; > - Status = PciRootBridgeIo->Pci.Write ( > - PciRootBridgeIo, > - EfiPciWidthUint32, > - Address, > - 1, > - &Register > - ); > + PciRootBridgeIo->Pci.Write ( > + PciRootBridgeIo, > + EfiPciWidthUint32, > + Address, > + 1, > + &Register > + ); > } > > if ((Func == 0) && !IS_PCI_MULTI_FUNC (&Pci)) { Wow, the original code is so sloppy. :( I guess itś best if we just don't assign Status here. If these accesses don't work, then nothing will. > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > index 71767d3793d4..087fe563c0bc 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > @@ -1250,6 +1250,10 @@ PciScanBus ( > &State > ); > > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_WARN, "Failed to initialize Hotplug PCI Controller, Status %r\n", Status)); > + } > + > PreprocessController ( > PciDevice, > PciDevice->BusNumber, Honestly, I don't have the slightest idea. The original code is utterly broken. We have a PCI device that seems like a root hotplug controller, we fail to initialzie the root hotplug controller, we capture the Status there, and then happily go on to "pre-process" the controller. What the heck is this. If the root HPC init is not a pre-requisite for preprocessing, then why capture the status??? Well, I guess your approach is the safest. Log it, but otherwise, preserve the current behavior. Jesus. > @@ -1501,6 +1505,10 @@ PciRootBridgeP2CProcess ( > > if (!IsListEmpty (&Temp->ChildList)) { > Status = PciRootBridgeP2CProcess (Temp); > + > + if (EFI_ERROR (Status)) { > + DEBUG ((DEBUG_WARN, "Failed to process Option Rom on PCI root bridge %p, Status %r\n", Temp, Status)); > + } > } > > CurrentLink = CurrentLink->ForwardLink; Yeah, I guess so. On a tangent, best cast Temp to (VOID*)Temp, for logging with %p. I didn't expect that the original code was this terrible. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#110867): https://edk2.groups.io/g/devel/message/110867 Mute This Topic: https://groups.io/mt/102438321/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-