[edk2-devel] [PATCH v2 0/5] BZ 4239: Fix MdeModulePkg/Bus/Pci/PciBusDxe issues pointed by Coverity

[edk2-devel] [PATCH v2 0/5] BZ 4239: Fix MdeModulePkg/Bus/Pci/PciBusDxe issues pointed by Coverity

[Ranbir Singh]

v1 -> v2:
  - Rebased to latest master HEAD
  - Updated Cc list
  - Updated the commit message wrt MISSING_BREAK issues
  - Removed the ASSERT wrt NULL_RETURNS issue

Ranbir Singh (5):
  MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue
  MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
  MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON Coverity issues
  MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue
  MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues

 MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c     | 73 +++++++++++---------
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c        |  1 -
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 48 +++++++++----
 MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c               | 45 ++++++++++--
 4 files changed, 113 insertions(+), 54 deletions(-)

-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110810): https://edk2.groups.io/g/devel/message/110810
Mute This Topic: https://groups.io/mt/102438297/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 1/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The function PciHotPlugRequestNotify has two if blocks towards the end
of function both containing return. Due to the parameter checks ensured
at the beginning of the function, one of the two if blocks is bound to
come in execution flow. Hence, the return EFI_SUCCESS; at line 2112 is
redundant/deadcode.

To fix it, either of line 2109 or 2112 can be deleted.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239

Cc: Ray Ni <ray.ni@intel.com>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
index 3f8c6e6da7dc..5b71e152f3ea 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
@@ -2106,7 +2106,6 @@ PciHotPlugRequestNotify (
     //
     // End for
     //
-    return EFI_SUCCESS;
   }
 
   return EFI_SUCCESS;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110811): https://edk2.groups.io/g/devel/message/110811
Mute This Topic: https://groups.io/mt/102438298/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The function UpdatePciInfo has switch-case code in which there are fall
through from case 32: to case 64:. While this is seeemingly intentional,
it is not evident to any general code reader why there is no break; in
between. Adding

    // No break; here as this is an intentional fallthrough.

as comment in between makes it explicit. Incidentally, the comment
satisfies Coverity as well.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239

Cc: Ray Ni <ray.ni@intel.com>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
index 6594b8eae83f..eda97285ee18 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
@@ -1428,6 +1428,9 @@ UpdatePciInfo (
               switch (Ptr->AddrSpaceGranularity) {
                 case 32:
                   PciIoDevice->PciBar[BarIndex].BarType = PciBarTypeMem32;
+                  //
+                  // No break; here as this is an intentional fall through.
+                  //
                 case 64:
                   PciIoDevice->PciBar[BarIndex].BarTypeFixed = TRUE;
                   break;
@@ -1440,6 +1443,9 @@ UpdatePciInfo (
               switch (Ptr->AddrSpaceGranularity) {
                 case 32:
                   PciIoDevice->PciBar[BarIndex].BarType = PciBarTypePMem32;
+                  //
+                  // No break; here as this is an intentional fall through.
+                  //
                 case 64:
                   PciIoDevice->PciBar[BarIndex].BarTypeFixed = TRUE;
                   break;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110812): https://edk2.groups.io/g/devel/message/110812
Mute This Topic: https://groups.io/mt/102438299/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 3/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON Coverity issues

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The function PciHostBridgeResourceAllocator is not making use of the
generic approach as is used in one of the other function namely -
DumpResourceMap. As a result, the following warnings can be seen as
reported by Coverity e.g.

(30) Event address_of:	Taking address with "&IoBridge" yields a
     singleton pointer.
(31) Event callee_ptr_arith:	Passing "&IoBridge" to function
     "FindResourceNode" which uses it as an array. This might corrupt
     or misinterpret adjacent memory locations.

Hence, adopt the generic approach to fix the issues at relevant points.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239

Cc: Ray Ni <ray.ni@intel.com>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 37 ++++++++++++++++----
 1 file changed, 31 insertions(+), 6 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
index 84fc0161a19c..71767d3793d4 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
@@ -485,6 +485,8 @@ PciHostBridgeResourceAllocator (
   UINT64                                         Mem64ResStatus;
   UINT64                                         PMem64ResStatus;
   UINT32                                         MaxOptionRomSize;
+  PCI_RESOURCE_NODE                              **ChildResources;
+  UINTN                                          ChildResourceCount;
   PCI_RESOURCE_NODE                              *IoBridge;
   PCI_RESOURCE_NODE                              *Mem32Bridge;
   PCI_RESOURCE_NODE                              *PMem32Bridge;
@@ -895,16 +897,39 @@ PciHostBridgeResourceAllocator (
     // Create the entire system resource map from the information collected by
     // enumerator. Several resource tree was created
     //
-    FindResourceNode (RootBridgeDev, &IoPool, &IoBridge);
-    FindResourceNode (RootBridgeDev, &Mem32Pool, &Mem32Bridge);
-    FindResourceNode (RootBridgeDev, &PMem32Pool, &PMem32Bridge);
-    FindResourceNode (RootBridgeDev, &Mem64Pool, &Mem64Bridge);
-    FindResourceNode (RootBridgeDev, &PMem64Pool, &PMem64Bridge);
-
+    ChildResourceCount = FindResourceNode (RootBridgeDev, &IoPool, NULL);
+    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount);
+    ASSERT (ChildResources != NULL);
+    FindResourceNode (RootBridgeDev, &IoPool, &ChildResources[0]);
+    IoBridge = ChildResources[0];
     ASSERT (IoBridge     != NULL);
+
+    ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem32Pool, NULL);
+    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount);
+    ASSERT (ChildResources != NULL);
+    FindResourceNode (RootBridgeDev, &Mem32Pool, &ChildResources[0]);
+    Mem32Bridge = ChildResources[0];
     ASSERT (Mem32Bridge  != NULL);
+
+    ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem32Pool, NULL);
+    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount);
+    ASSERT (ChildResources != NULL);
+    FindResourceNode (RootBridgeDev, &PMem32Pool, &ChildResources[0]);
+    PMem32Bridge = ChildResources[0];
     ASSERT (PMem32Bridge != NULL);
+
+    ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem64Pool, NULL);
+    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount);
+    ASSERT (ChildResources != NULL);
+    FindResourceNode (RootBridgeDev, &Mem64Pool, &ChildResources[0]);
+    Mem64Bridge = ChildResources[0];
     ASSERT (Mem64Bridge  != NULL);
+
+    ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem64Pool, NULL);
+    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount);
+    ASSERT (ChildResources != NULL);
+    FindResourceNode (RootBridgeDev, &PMem64Pool, &ChildResources[0]);
+    PMem64Bridge = ChildResources[0];
     ASSERT (PMem64Bridge != NULL);
 
     //
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110813): https://edk2.groups.io/g/devel/message/110813
Mute This Topic: https://groups.io/mt/102438300/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Ranbir Singh]

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The function StartPciDevices has a check

    ASSERT (RootBridge != NULL);

but this comes into play only in DEBUG mode. In Release mode, there
is no handling if the RootBridge value is NULL and the code proceeds
to unconditionally dereference "RootBridge" which will lead to CRASH.

Hence, for safety add NULL pointer checks always and return
EFI_NOT_READY if RootBridge value is NULL which is one of the return
values as mentioned in the function description header.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239

Cc: Ray Ni <ray.ni@intel.com>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
index 581e9075ad41..3de80d98370e 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
@@ -772,7 +772,10 @@ StartPciDevices (
   LIST_ENTRY     *CurrentLink;
 
   RootBridge = GetRootBridgeByHandle (Controller);
-  ASSERT (RootBridge != NULL);
+  if (RootBridge == NULL) {
+    return EFI_NOT_READY;
+  }
+
   ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle;
 
   CurrentLink = mPciDevicePool.ForwardLink;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110814): https://edk2.groups.io/g/devel/message/110814
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v2 5/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues

[Ranbir Singh]

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 <ray.ni@intel.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 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(-)

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
+               );
       }
 
       //
@@ -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 {
@@ -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;
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;
           }
@@ -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)) {
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,
@@ -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;
-- 
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110815): https://edk2.groups.io/g/devel/message/110815
Mute This Topic: https://groups.io/mt/102438321/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 2902 bytes --]

Reviewed-by: Ray Ni <ray.ni@intel.com>

Thanks,
Ray
________________________________
From: Ranbir Singh <rsingh@ventanamicro.com>
Sent: Tuesday, November 7, 2023 2:19 PM
To: devel@edk2.groups.io <devel@edk2.groups.io>; rsingh@ventanamicro.com <rsingh@ventanamicro.com>
Cc: Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Subject: [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues

From: Ranbir Singh <Ranbir.Singh3@Dell.com>

The function UpdatePciInfo has switch-case code in which there are fall
through from case 32: to case 64:. While this is seeemingly intentional,
it is not evident to any general code reader why there is no break; in
between. Adding

    // No break; here as this is an intentional fallthrough.

as comment in between makes it explicit. Incidentally, the comment
satisfies Coverity as well.

REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239

Cc: Ray Ni <ray.ni@intel.com>
Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
---
 MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
index 6594b8eae83f..eda97285ee18 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
@@ -1428,6 +1428,9 @@ UpdatePciInfo (
               switch (Ptr->AddrSpaceGranularity) {

                 case 32:

                   PciIoDevice->PciBar[BarIndex].BarType = PciBarTypeMem32;

+                  //

+                  // No break; here as this is an intentional fall through.

+                  //

                 case 64:

                   PciIoDevice->PciBar[BarIndex].BarTypeFixed = TRUE;

                   break;

@@ -1440,6 +1443,9 @@ UpdatePciInfo (
               switch (Ptr->AddrSpaceGranularity) {

                 case 32:

                   PciIoDevice->PciBar[BarIndex].BarType = PciBarTypePMem32;

+                  //

+                  // No break; here as this is an intentional fall through.

+                  //

                 case 64:

                   PciIoDevice->PciBar[BarIndex].BarTypeFixed = TRUE;

                   break;

--
2.34.1



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110821): https://edk2.groups.io/g/devel/message/110821
Mute This Topic: https://groups.io/mt/102438299/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 6582 bytes --]

Re: [edk2-devel] [PATCH v2 1/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue

[Laszlo Ersek]

On 11/7/23 07:19, Ranbir Singh wrote:
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> 
> The function PciHotPlugRequestNotify has two if blocks towards the end
> of function both containing return. Due to the parameter checks ensured
> at the beginning of the function, one of the two if blocks is bound to
> come in execution flow. Hence, the return EFI_SUCCESS; at line 2112 is
> redundant/deadcode.

Agree with the analysis.

> 
> To fix it, either of line 2109 or 2112 can be deleted.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
> index 3f8c6e6da7dc..5b71e152f3ea 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
> @@ -2106,7 +2106,6 @@ PciHotPlugRequestNotify (
>      //
>      // End for
>      //
> -    return EFI_SUCCESS;
>    }
>  
>    return EFI_SUCCESS;

Disagree with the fix.

Given the checks on "Operation" at the top of the function, the
condition (near the end of the function)

  if (Operation == EfiPciHotplugRequestRemove) {

will always evaluate to TRUE. (Operation can be only Add or Remove, and
if it is Add, then we don't reach this location.)

Therefore, we should remove this condition, and *unnest* the dependent
logic.

As a result of *that*, you'll have

  return EFI_SUCCESS;
  return EFI_SUCCESS;

at the end of the function, and *then* you should remove either one of them.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110862): https://edk2.groups.io/g/devel/message/110862
Mute This Topic: https://groups.io/mt/102438298/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues

[Laszlo Ersek]

On 11/7/23 07:19, Ranbir Singh wrote:
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> 
> The function UpdatePciInfo has switch-case code in which there are fall
> through from case 32: to case 64:. While this is seeemingly intentional,
> it is not evident to any general code reader why there is no break; in
> between. Adding
> 
>     // No break; here as this is an intentional fallthrough.
> 
> as comment in between makes it explicit. Incidentally, the comment
> satisfies Coverity as well.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> index 6594b8eae83f..eda97285ee18 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> @@ -1428,6 +1428,9 @@ UpdatePciInfo (
>                switch (Ptr->AddrSpaceGranularity) {
>                  case 32:
>                    PciIoDevice->PciBar[BarIndex].BarType = PciBarTypeMem32;
> +                  //
> +                  // No break; here as this is an intentional fall through.
> +                  //
>                  case 64:
>                    PciIoDevice->PciBar[BarIndex].BarTypeFixed = TRUE;
>                    break;
> @@ -1440,6 +1443,9 @@ UpdatePciInfo (
>                switch (Ptr->AddrSpaceGranularity) {
>                  case 32:
>                    PciIoDevice->PciBar[BarIndex].BarType = PciBarTypePMem32;
> +                  //
> +                  // No break; here as this is an intentional fall through.
> +                  //
>                  case 64:
>                    PciIoDevice->PciBar[BarIndex].BarTypeFixed = TRUE;
>                    break;

Agree, but the semicolon's placement is awkward. I propose

  No break here, as this is an intentional fall through.

Reviewed-by: Laszlo Ersek <lersek@redhat.com>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110863): https://edk2.groups.io/g/devel/message/110863
Mute This Topic: https://groups.io/mt/102438299/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 3/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON Coverity issues

[Laszlo Ersek]

On 11/7/23 07:19, Ranbir Singh wrote:
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> 
> The function PciHostBridgeResourceAllocator is not making use of the
> generic approach as is used in one of the other function namely -
> DumpResourceMap. As a result, the following warnings can be seen as
> reported by Coverity e.g.
> 
> (30) Event address_of:	Taking address with "&IoBridge" yields a
>      singleton pointer.
> (31) Event callee_ptr_arith:	Passing "&IoBridge" to function
>      "FindResourceNode" which uses it as an array. This might corrupt
>      or misinterpret adjacent memory locations.
> 
> Hence, adopt the generic approach to fix the issues at relevant points.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 37 ++++++++++++++++----
>  1 file changed, 31 insertions(+), 6 deletions(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> index 84fc0161a19c..71767d3793d4 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> @@ -485,6 +485,8 @@ PciHostBridgeResourceAllocator (
>    UINT64                                         Mem64ResStatus;
>    UINT64                                         PMem64ResStatus;
>    UINT32                                         MaxOptionRomSize;
> +  PCI_RESOURCE_NODE                              **ChildResources;
> +  UINTN                                          ChildResourceCount;
>    PCI_RESOURCE_NODE                              *IoBridge;
>    PCI_RESOURCE_NODE                              *Mem32Bridge;
>    PCI_RESOURCE_NODE                              *PMem32Bridge;
> @@ -895,16 +897,39 @@ PciHostBridgeResourceAllocator (
>      // Create the entire system resource map from the information collected by
>      // enumerator. Several resource tree was created
>      //
> -    FindResourceNode (RootBridgeDev, &IoPool, &IoBridge);
> -    FindResourceNode (RootBridgeDev, &Mem32Pool, &Mem32Bridge);
> -    FindResourceNode (RootBridgeDev, &PMem32Pool, &PMem32Bridge);
> -    FindResourceNode (RootBridgeDev, &Mem64Pool, &Mem64Bridge);
> -    FindResourceNode (RootBridgeDev, &PMem64Pool, &PMem64Bridge);
> -
> +    ChildResourceCount = FindResourceNode (RootBridgeDev, &IoPool, NULL);
> +    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount);
> +    ASSERT (ChildResources != NULL);
> +    FindResourceNode (RootBridgeDev, &IoPool, &ChildResources[0]);
> +    IoBridge = ChildResources[0];
>      ASSERT (IoBridge     != NULL);
> +
> +    ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem32Pool, NULL);
> +    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount);
> +    ASSERT (ChildResources != NULL);
> +    FindResourceNode (RootBridgeDev, &Mem32Pool, &ChildResources[0]);
> +    Mem32Bridge = ChildResources[0];
>      ASSERT (Mem32Bridge  != NULL);
> +
> +    ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem32Pool, NULL);
> +    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount);
> +    ASSERT (ChildResources != NULL);
> +    FindResourceNode (RootBridgeDev, &PMem32Pool, &ChildResources[0]);
> +    PMem32Bridge = ChildResources[0];
>      ASSERT (PMem32Bridge != NULL);
> +
> +    ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem64Pool, NULL);
> +    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount);
> +    ASSERT (ChildResources != NULL);
> +    FindResourceNode (RootBridgeDev, &Mem64Pool, &ChildResources[0]);
> +    Mem64Bridge = ChildResources[0];
>      ASSERT (Mem64Bridge  != NULL);
> +
> +    ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem64Pool, NULL);
> +    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * ChildResourceCount);
> +    ASSERT (ChildResources != NULL);
> +    FindResourceNode (RootBridgeDev, &PMem64Pool, &ChildResources[0]);
> +    PMem64Bridge = ChildResources[0];
>      ASSERT (PMem64Bridge != NULL);
>  
>      //

Sorry, but this is terrible.

* First of all, Coverity is *wrong*. The C spec clearly states that, for
the purposes of pointer arithmetic, a singleton object behaves
identically to the first element of an array.

So, immediately, the idea arises that we should just use

  PCI_RESOURCE_NODE *IoBridgeArray[1];

  FindResourceNode (RootBridgeDev, &IoPool, IoBridgeArray)

to shut up Coverity.

* Unfortunately, I expect that would only create a different warning: a
warning about potentially overflowing this single-element array. Which
is in fact a deeper problem in FindResourceNode() -- it happily
overwrites an array that is too small.

* Finally, this generic approach is both ugly (lots of code
duplication!), and worse, it allocates memory without proper error
checking (ASSERT() is not error checking), and then it leaks
ChildResources *multiple times*!

I suggest the following, for solving all of these issues:

- create a function called FindFirstResourceNode(). It should have the
same function prototype as FindResourceNode(), except the last parameter
should be mandatory, not OPTIONAL.

- the internal logic should be the same, except we shouldn't be
counting, the return value should be a BOOLEAN. If we find a match, then
output the first match and return TRUE. Otherwise, set the output to
NULL and return FALSE.

- replace the FindResourceNode calls with FindFirstResourceNode calls

- Those call sites are already followed by ASSERT()s, saying that all
search attempts will succeed. If coverity is happy with them, that's
good; otherwise, we'll have to find a solution for them as well.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110864): https://edk2.groups.io/g/devel/message/110864
Mute This Topic: https://groups.io/mt/102438300/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Laszlo Ersek]

On 11/7/23 07:19, Ranbir Singh wrote:
> From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> 
> The function StartPciDevices has a check
> 
>     ASSERT (RootBridge != NULL);
> 
> but this comes into play only in DEBUG mode. In Release mode, there
> is no handling if the RootBridge value is NULL and the code proceeds
> to unconditionally dereference "RootBridge" which will lead to CRASH.
> 
> Hence, for safety add NULL pointer checks always and return
> EFI_NOT_READY if RootBridge value is NULL which is one of the return
> values as mentioned in the function description header.
> 
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> 
> Cc: Ray Ni <ray.ni@intel.com>
> Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> index 581e9075ad41..3de80d98370e 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> @@ -772,7 +772,10 @@ StartPciDevices (
>    LIST_ENTRY     *CurrentLink;
>  
>    RootBridge = GetRootBridgeByHandle (Controller);
> -  ASSERT (RootBridge != NULL);
> +  if (RootBridge == NULL) {
> +    return EFI_NOT_READY;
> +  }
> +
>    ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle;
>  
>    CurrentLink = mPciDevicePool.ForwardLink;

I don't think this is a good fix.

There is one call site, namely in PciBusDriverBindingStart(). That call
site does not check the return value. (Of course /s)

I think that this ASSERT() can indeed never fail. Therefore I suggest
CpuDeadLoop() instead.

If you insist that CpuDeadLoop() is "too risky" here, then the patch is
acceptable, but then the StartPciDevices() call site in
PciBusDriverBindingStart() must check the error properly: we must not
install "gEfiPciEnumerationCompleteProtocolGuid", and the function must
propagate the error outwards.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110865): https://edk2.groups.io/g/devel/message/110865
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 5/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues

[Laszlo Ersek]

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 <ray.ni@intel.com>
> Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> ---
>  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]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues

[Michael D Kinney]

This comment style only works with Coverity.

Other static analysis tools may flag the same issue again.

It is better to update the logic so no static analysis tool will
flag this issue.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> Ersek
> Sent: Tuesday, November 7, 2023 8:23 AM
> To: devel@edk2.groups.io; rsingh@ventanamicro.com
> Cc: Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli
> <veeresh.sangolli@dellteam.com>
> Subject: Re: [edk2-devel] [PATCH v2 2/5]
> MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
> 
> On 11/7/23 07:19, Ranbir Singh wrote:
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >
> > The function UpdatePciInfo has switch-case code in which there are
> fall
> > through from case 32: to case 64:. While this is seeemingly
> intentional,
> > it is not evident to any general code reader why there is no break;
> in
> > between. Adding
> >
> >     // No break; here as this is an intentional fallthrough.
> >
> > as comment in between makes it explicit. Incidentally, the comment
> > satisfies Coverity as well.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > index 6594b8eae83f..eda97285ee18 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > @@ -1428,6 +1428,9 @@ UpdatePciInfo (
> >                switch (Ptr->AddrSpaceGranularity) {
> >                  case 32:
> >                    PciIoDevice->PciBar[BarIndex].BarType =
> PciBarTypeMem32;
> > +                  //
> > +                  // No break; here as this is an intentional fall
> through.
> > +                  //
> >                  case 64:
> >                    PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> TRUE;
> >                    break;
> > @@ -1440,6 +1443,9 @@ UpdatePciInfo (
> >                switch (Ptr->AddrSpaceGranularity) {
> >                  case 32:
> >                    PciIoDevice->PciBar[BarIndex].BarType =
> PciBarTypePMem32;
> > +                  //
> > +                  // No break; here as this is an intentional fall
> through.
> > +                  //
> >                  case 64:
> >                    PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> TRUE;
> >                    break;
> 
> Agree, but the semicolon's placement is awkward. I propose
> 
>   No break here, as this is an intentional fall through.
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110870): https://edk2.groups.io/g/devel/message/110870
Mute This Topic: https://groups.io/mt/102438299/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 4464 bytes --]

As mentioned in the commit message, the comment helps in making it
explicit and evident that the missing break is not a human miss, but
intentional.
Hence, the comment should be considered as being added for the human code
readers / developers.

So even if some static analysis tool flags such an issue, it can be fairly
easy now to ignore that on manual inspection. If desired this can also be
stated in the comment itself like -

+                  //
+                  // No break here as this is an intentional fall through.
+                  // Ignore any static tool issue if pointed.
+                  //

Yes, there can be other solutions (which may or may not be worth the
effort), but for now I went with the least code change approach.

On Tue, Nov 7, 2023 at 11:29 PM Kinney, Michael D <
michael.d.kinney@intel.com> wrote:

> This comment style only works with Coverity.
>
> Other static analysis tools may flag the same issue again.
>
> It is better to update the logic so no static analysis tool will
> flag this issue.
>
> Mike
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> > Ersek
> > Sent: Tuesday, November 7, 2023 8:23 AM
> > To: devel@edk2.groups.io; rsingh@ventanamicro.com
> > Cc: Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli
> > <veeresh.sangolli@dellteam.com>
> > Subject: Re: [edk2-devel] [PATCH v2 2/5]
> > MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
> >
> > On 11/7/23 07:19, Ranbir Singh wrote:
> > > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > >
> > > The function UpdatePciInfo has switch-case code in which there are
> > fall
> > > through from case 32: to case 64:. While this is seeemingly
> > intentional,
> > > it is not evident to any general code reader why there is no break;
> > in
> > > between. Adding
> > >
> > >     // No break; here as this is an intentional fallthrough.
> > >
> > > as comment in between makes it explicit. Incidentally, the comment
> > > satisfies Coverity as well.
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> > >
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> > > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > > ---
> > >  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > index 6594b8eae83f..eda97285ee18 100644
> > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > @@ -1428,6 +1428,9 @@ UpdatePciInfo (
> > >                switch (Ptr->AddrSpaceGranularity) {
> > >                  case 32:
> > >                    PciIoDevice->PciBar[BarIndex].BarType =
> > PciBarTypeMem32;
> > > +                  //
> > > +                  // No break; here as this is an intentional fall
> > through.
> > > +                  //
> > >                  case 64:
> > >                    PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> > TRUE;
> > >                    break;
> > > @@ -1440,6 +1443,9 @@ UpdatePciInfo (
> > >                switch (Ptr->AddrSpaceGranularity) {
> > >                  case 32:
> > >                    PciIoDevice->PciBar[BarIndex].BarType =
> > PciBarTypePMem32;
> > > +                  //
> > > +                  // No break; here as this is an intentional fall
> > through.
> > > +                  //
> > >                  case 64:
> > >                    PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> > TRUE;
> > >                    break;
> >
> > Agree, but the semicolon's placement is awkward. I propose
> >
> >   No break here, as this is an intentional fall through.
> >
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >
> >
> >
> > 
> >
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110890): https://edk2.groups.io/g/devel/message/110890
Mute This Topic: https://groups.io/mt/102438299/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 6779 bytes --]

Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 5460 bytes --]

Hi Ranbir,

Ignoring false positive in static analysis tools is typically a burden.

It is better to avoid false positives with code changes as long as the code changes do not make the implementation confusing and hard to maintain.

I think depending on fall through case statements is confusing and removing them will make the code easier to understand and maintain.

Mike

From: Ranbir Singh <rsingh@ventanamicro.com>
Sent: Tuesday, November 7, 2023 7:51 PM
To: Kinney, Michael D <michael.d.kinney@intel.com>
Cc: devel@edk2.groups.io; lersek@redhat.com; Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Subject: Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues

As mentioned in the commit message, the comment helps in making it explicit and evident that the missing break is not a human miss, but intentional.
Hence, the comment should be considered as being added for the human code readers / developers.

So even if some static analysis tool flags such an issue, it can be fairly easy now to ignore that on manual inspection. If desired this can also be stated in the comment itself like -

+                  //
+                  // No break here as this is an intentional fall through.
+                  // Ignore any static tool issue if pointed.
+                  //

Yes, there can be other solutions (which may or may not be worth the effort), but for now I went with the least code change approach.

On Tue, Nov 7, 2023 at 11:29 PM Kinney, Michael D <michael.d.kinney@intel.com<mailto:michael.d.kinney@intel.com>> wrote:
This comment style only works with Coverity.

Other static analysis tools may flag the same issue again.

It is better to update the logic so no static analysis tool will
flag this issue.

Mike

> -----Original Message-----
> From: devel@edk2.groups.io<mailto:devel@edk2.groups.io> <devel@edk2.groups.io<mailto:devel@edk2.groups.io>> On Behalf Of Laszlo
> Ersek
> Sent: Tuesday, November 7, 2023 8:23 AM
> To: devel@edk2.groups.io<mailto:devel@edk2.groups.io>; rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>
> Cc: Ni, Ray <ray.ni@intel.com<mailto:ray.ni@intel.com>>; Veeresh Sangolli
> <veeresh.sangolli@dellteam.com<mailto:veeresh.sangolli@dellteam.com>>
> Subject: Re: [edk2-devel] [PATCH v2 2/5]
> MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
>
> On 11/7/23 07:19, Ranbir Singh wrote:
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com<mailto:Ranbir.Singh3@Dell.com>>
> >
> > The function UpdatePciInfo has switch-case code in which there are
> fall
> > through from case 32: to case 64:. While this is seeemingly
> intentional,
> > it is not evident to any general code reader why there is no break;
> in
> > between. Adding
> >
> >     // No break; here as this is an intentional fallthrough.
> >
> > as comment in between makes it explicit. Incidentally, the comment
> > satisfies Coverity as well.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> >
> > Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com>>
> > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com<mailto:veeresh.sangolli@dellteam.com>>
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com<mailto:Ranbir.Singh3@Dell.com>>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>>
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > index 6594b8eae83f..eda97285ee18 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > @@ -1428,6 +1428,9 @@ UpdatePciInfo (
> >                switch (Ptr->AddrSpaceGranularity) {
> >                  case 32:
> >                    PciIoDevice->PciBar[BarIndex].BarType =
> PciBarTypeMem32;
> > +                  //
> > +                  // No break; here as this is an intentional fall
> through.
> > +                  //
> >                  case 64:
> >                    PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> TRUE;
> >                    break;
> > @@ -1440,6 +1443,9 @@ UpdatePciInfo (
> >                switch (Ptr->AddrSpaceGranularity) {
> >                  case 32:
> >                    PciIoDevice->PciBar[BarIndex].BarType =
> PciBarTypePMem32;
> > +                  //
> > +                  // No break; here as this is an intentional fall
> through.
> > +                  //
> >                  case 64:
> >                    PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> TRUE;
> >                    break;
>
> Agree, but the semicolon's placement is awkward. I propose
>
>   No break here, as this is an intentional fall through.
>
> Reviewed-by: Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>>
>
>
>
> 
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110892): https://edk2.groups.io/g/devel/message/110892
Mute This Topic: https://groups.io/mt/102438299/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 11149 bytes --]

Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 6517 bytes --]

Hi Mike,

I agree that any manual inspection is sort of a burden, more so when it
becomes repetitive in the long run.

When I was doing these Coverity checks (Nov-Dec' 2022), I was working in
Dell and had access to the real systems to check the execution flow as well
as the Coverity status. I could never post these patches while being there,
but happened to raise Bugzilla's and post them there instead hoping that
they would be taken up by somebody further.

I am no longer with Dell and later on when I found that those BZ / issues
pointed out by Coverity still exist as there are no code changes in related
contexts, I thought of taking them forward in whatever limited capacity I
can. I am a bit hesitant to do any further code changes as now I do not
have any systems to check the execution flow as well as the Coverity
status. So, I do not guarantee, but will try to make the code changes
wherever it is easy to ascertain that the functional flow would not be
impacted and the same issue won't exist anymore.

Ranbir Singh

On Wed, Nov 8, 2023 at 9:35 AM Kinney, Michael D <michael.d.kinney@intel.com>
wrote:

> Hi Ranbir,
>
>
>
> Ignoring false positive in static analysis tools is typically a burden.
>
>
>
> It is better to avoid false positives with code changes as long as the
> code changes do not make the implementation confusing and hard to maintain.
>
>
>
> I think depending on fall through case statements is confusing and
> removing them will make the code easier to understand and maintain.
>
>
>
> Mike
>
>
>
> *From:* Ranbir Singh <rsingh@ventanamicro.com>
> *Sent:* Tuesday, November 7, 2023 7:51 PM
> *To:* Kinney, Michael D <michael.d.kinney@intel.com>
> *Cc:* devel@edk2.groups.io; lersek@redhat.com; Ni, Ray <ray.ni@intel.com>;
> Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> *Subject:* Re: [edk2-devel] [PATCH v2 2/5]
> MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
>
>
>
> As mentioned in the commit message, the comment helps in making it
> explicit and evident that the missing break is not a human miss, but
> intentional.
>
> Hence, the comment should be considered as being added for the human code
> readers / developers.
>
>
>
> So even if some static analysis tool flags such an issue, it can be fairly
> easy now to ignore that on manual inspection. If desired this can also be
> stated in the comment itself like -
>
>
>
> +                  //
> +                  // No break here as this is an intentional fall through.
>
> +                  // Ignore any static tool issue if pointed.
> +                  //
>
>
>
> Yes, there can be other solutions (which may or may not be worth the
> effort), but for now I went with the least code change approach.
>
>
>
> On Tue, Nov 7, 2023 at 11:29 PM Kinney, Michael D <
> michael.d.kinney@intel.com> wrote:
>
> This comment style only works with Coverity.
>
> Other static analysis tools may flag the same issue again.
>
> It is better to update the logic so no static analysis tool will
> flag this issue.
>
> Mike
>
> > -----Original Message-----
> > From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Laszlo
> > Ersek
> > Sent: Tuesday, November 7, 2023 8:23 AM
> > To: devel@edk2.groups.io; rsingh@ventanamicro.com
> > Cc: Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli
> > <veeresh.sangolli@dellteam.com>
> > Subject: Re: [edk2-devel] [PATCH v2 2/5]
> > MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues
> >
> > On 11/7/23 07:19, Ranbir Singh wrote:
> > > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > >
> > > The function UpdatePciInfo has switch-case code in which there are
> > fall
> > > through from case 32: to case 64:. While this is seeemingly
> > intentional,
> > > it is not evident to any general code reader why there is no break;
> > in
> > > between. Adding
> > >
> > >     // No break; here as this is an intentional fallthrough.
> > >
> > > as comment in between makes it explicit. Incidentally, the comment
> > > satisfies Coverity as well.
> > >
> > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> > >
> > > Cc: Ray Ni <ray.ni@intel.com>
> > > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> > > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > > ---
> > >  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > >
> > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > index 6594b8eae83f..eda97285ee18 100644
> > > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > @@ -1428,6 +1428,9 @@ UpdatePciInfo (
> > >                switch (Ptr->AddrSpaceGranularity) {
> > >                  case 32:
> > >                    PciIoDevice->PciBar[BarIndex].BarType =
> > PciBarTypeMem32;
> > > +                  //
> > > +                  // No break; here as this is an intentional fall
> > through.
> > > +                  //
> > >                  case 64:
> > >                    PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> > TRUE;
> > >                    break;
> > > @@ -1440,6 +1443,9 @@ UpdatePciInfo (
> > >                switch (Ptr->AddrSpaceGranularity) {
> > >                  case 32:
> > >                    PciIoDevice->PciBar[BarIndex].BarType =
> > PciBarTypePMem32;
> > > +                  //
> > > +                  // No break; here as this is an intentional fall
> > through.
> > > +                  //
> > >                  case 64:
> > >                    PciIoDevice->PciBar[BarIndex].BarTypeFixed =
> > TRUE;
> > >                    break;
> >
> > Agree, but the semicolon's placement is awkward. I propose
> >
> >   No break here, as this is an intentional fall through.
> >
> > Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> >
> >
> >
> > 
> >
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#110896): https://edk2.groups.io/g/devel/message/110896
Mute This Topic: https://groups.io/mt/102438299/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 10984 bytes --]

Re: [edk2-devel] [PATCH v2 3/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix ARRAY_VS_SINGLETON Coverity issues

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 6889 bytes --]

I was apprehensive about this from the very beginning.

Anyway, for now I will be dropping this issue from the series.

On Tue, Nov 7, 2023 at 10:11 PM Laszlo Ersek <lersek@redhat.com> wrote:

> On 11/7/23 07:19, Ranbir Singh wrote:
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >
> > The function PciHostBridgeResourceAllocator is not making use of the
> > generic approach as is used in one of the other function namely -
> > DumpResourceMap. As a result, the following warnings can be seen as
> > reported by Coverity e.g.
> >
> > (30) Event address_of:        Taking address with "&IoBridge" yields a
> >      singleton pointer.
> > (31) Event callee_ptr_arith:  Passing "&IoBridge" to function
> >      "FindResourceNode" which uses it as an array. This might corrupt
> >      or misinterpret adjacent memory locations.
> >
> > Hence, adopt the generic approach to fix the issues at relevant points.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 37 ++++++++++++++++----
> >  1 file changed, 31 insertions(+), 6 deletions(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > index 84fc0161a19c..71767d3793d4 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > @@ -485,6 +485,8 @@ PciHostBridgeResourceAllocator (
> >    UINT64                                         Mem64ResStatus;
> >    UINT64                                         PMem64ResStatus;
> >    UINT32                                         MaxOptionRomSize;
> > +  PCI_RESOURCE_NODE                              **ChildResources;
> > +  UINTN                                          ChildResourceCount;
> >    PCI_RESOURCE_NODE                              *IoBridge;
> >    PCI_RESOURCE_NODE                              *Mem32Bridge;
> >    PCI_RESOURCE_NODE                              *PMem32Bridge;
> > @@ -895,16 +897,39 @@ PciHostBridgeResourceAllocator (
> >      // Create the entire system resource map from the information
> collected by
> >      // enumerator. Several resource tree was created
> >      //
> > -    FindResourceNode (RootBridgeDev, &IoPool, &IoBridge);
> > -    FindResourceNode (RootBridgeDev, &Mem32Pool, &Mem32Bridge);
> > -    FindResourceNode (RootBridgeDev, &PMem32Pool, &PMem32Bridge);
> > -    FindResourceNode (RootBridgeDev, &Mem64Pool, &Mem64Bridge);
> > -    FindResourceNode (RootBridgeDev, &PMem64Pool, &PMem64Bridge);
> > -
> > +    ChildResourceCount = FindResourceNode (RootBridgeDev, &IoPool,
> NULL);
> > +    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) *
> ChildResourceCount);
> > +    ASSERT (ChildResources != NULL);
> > +    FindResourceNode (RootBridgeDev, &IoPool, &ChildResources[0]);
> > +    IoBridge = ChildResources[0];
> >      ASSERT (IoBridge     != NULL);
> > +
> > +    ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem32Pool,
> NULL);
> > +    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) *
> ChildResourceCount);
> > +    ASSERT (ChildResources != NULL);
> > +    FindResourceNode (RootBridgeDev, &Mem32Pool, &ChildResources[0]);
> > +    Mem32Bridge = ChildResources[0];
> >      ASSERT (Mem32Bridge  != NULL);
> > +
> > +    ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem32Pool,
> NULL);
> > +    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) *
> ChildResourceCount);
> > +    ASSERT (ChildResources != NULL);
> > +    FindResourceNode (RootBridgeDev, &PMem32Pool, &ChildResources[0]);
> > +    PMem32Bridge = ChildResources[0];
> >      ASSERT (PMem32Bridge != NULL);
> > +
> > +    ChildResourceCount = FindResourceNode (RootBridgeDev, &Mem64Pool,
> NULL);
> > +    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) *
> ChildResourceCount);
> > +    ASSERT (ChildResources != NULL);
> > +    FindResourceNode (RootBridgeDev, &Mem64Pool, &ChildResources[0]);
> > +    Mem64Bridge = ChildResources[0];
> >      ASSERT (Mem64Bridge  != NULL);
> > +
> > +    ChildResourceCount = FindResourceNode (RootBridgeDev, &PMem64Pool,
> NULL);
> > +    ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) *
> ChildResourceCount);
> > +    ASSERT (ChildResources != NULL);
> > +    FindResourceNode (RootBridgeDev, &PMem64Pool, &ChildResources[0]);
> > +    PMem64Bridge = ChildResources[0];
> >      ASSERT (PMem64Bridge != NULL);
> >
> >      //
>
> Sorry, but this is terrible.
>
> * First of all, Coverity is *wrong*. The C spec clearly states that, for
> the purposes of pointer arithmetic, a singleton object behaves
> identically to the first element of an array.
>
> So, immediately, the idea arises that we should just use
>
>   PCI_RESOURCE_NODE *IoBridgeArray[1];
>
>   FindResourceNode (RootBridgeDev, &IoPool, IoBridgeArray)
>
> to shut up Coverity.
>
> * Unfortunately, I expect that would only create a different warning: a
> warning about potentially overflowing this single-element array. Which
> is in fact a deeper problem in FindResourceNode() -- it happily
> overwrites an array that is too small.
>
> * Finally, this generic approach is both ugly (lots of code
> duplication!), and worse, it allocates memory without proper error
> checking (ASSERT() is not error checking), and then it leaks
> ChildResources *multiple times*!
>
> I suggest the following, for solving all of these issues:
>
> - create a function called FindFirstResourceNode(). It should have the
> same function prototype as FindResourceNode(), except the last parameter
> should be mandatory, not OPTIONAL.
>
> - the internal logic should be the same, except we shouldn't be
> counting, the return value should be a BOOLEAN. If we find a match, then
> output the first match and return TRUE. Otherwise, set the output to
> NULL and return FALSE.
>
> - replace the FindResourceNode calls with FindFirstResourceNode calls
>
> - Those call sites are already followed by ASSERT()s, saying that all
> search attempts will succeed. If coverity is happy with them, that's
> good; otherwise, we'll have to find a solution for them as well.
>
> Laszlo
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111016): https://edk2.groups.io/g/devel/message/111016
Mute This Topic: https://groups.io/mt/102438300/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 8824 bytes --]

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 3077 bytes --]

EFI_NOT_READY was listed as one of the error return values in the function
header of StartPciDevices(). So, I considered it here.

Maybe we can go by a dual approach, including CpuDeadLoop() in
StartPciDevices() as well as add return value check at the call site in
PciBusDriverBindingStart().

On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com> wrote:

> On 11/7/23 07:19, Ranbir Singh wrote:
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >
> > The function StartPciDevices has a check
> >
> >     ASSERT (RootBridge != NULL);
> >
> > but this comes into play only in DEBUG mode. In Release mode, there
> > is no handling if the RootBridge value is NULL and the code proceeds
> > to unconditionally dereference "RootBridge" which will lead to CRASH.
> >
> > Hence, for safety add NULL pointer checks always and return
> > EFI_NOT_READY if RootBridge value is NULL which is one of the return
> > values as mentioned in the function description header.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > index 581e9075ad41..3de80d98370e 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> > @@ -772,7 +772,10 @@ StartPciDevices (
> >    LIST_ENTRY     *CurrentLink;
> >
> >    RootBridge = GetRootBridgeByHandle (Controller);
> > -  ASSERT (RootBridge != NULL);
> > +  if (RootBridge == NULL) {
> > +    return EFI_NOT_READY;
> > +  }
> > +
> >    ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle;
> >
> >    CurrentLink = mPciDevicePool.ForwardLink;
>
> I don't think this is a good fix.
>
> There is one call site, namely in PciBusDriverBindingStart(). That call
> site does not check the return value. (Of course /s)
>
> I think that this ASSERT() can indeed never fail. Therefore I suggest
> CpuDeadLoop() instead.
>
> If you insist that CpuDeadLoop() is "too risky" here, then the patch is
> acceptable, but then the StartPciDevices() call site in
> PciBusDriverBindingStart() must check the error properly: we must not
> install "gEfiPciEnumerationCompleteProtocolGuid", and the function must
> propagate the error outwards.
>
> Laszlo
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111017): https://edk2.groups.io/g/devel/message/111017
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 4406 bytes --]

Re: [edk2-devel] [PATCH v2 1/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix DEADCODE Coverity issue

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 2635 bytes --]

I considered the least code change approach without considering any
optimization which also works as such.

However, yes, the duplicated /unnecessary subsequent checks can be removed.

On Tue, Nov 7, 2023 at 9:51 PM Laszlo Ersek <lersek@redhat.com> wrote:

> On 11/7/23 07:19, Ranbir Singh wrote:
> > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >
> > The function PciHotPlugRequestNotify has two if blocks towards the end
> > of function both containing return. Due to the parameter checks ensured
> > at the beginning of the function, one of the two if blocks is bound to
> > come in execution flow. Hence, the return EFI_SUCCESS; at line 2112 is
> > redundant/deadcode.
>
> Agree with the analysis.
>
> >
> > To fix it, either of line 2109 or 2112 can be deleted.
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> >
> > Cc: Ray Ni <ray.ni@intel.com>
> > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com>
> > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > ---
> >  MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
> > index 3f8c6e6da7dc..5b71e152f3ea 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c
> > @@ -2106,7 +2106,6 @@ PciHotPlugRequestNotify (
> >      //
> >      // End for
> >      //
> > -    return EFI_SUCCESS;
> >    }
> >
> >    return EFI_SUCCESS;
>
> Disagree with the fix.
>
> Given the checks on "Operation" at the top of the function, the
> condition (near the end of the function)
>
>   if (Operation == EfiPciHotplugRequestRemove) {
>
> will always evaluate to TRUE. (Operation can be only Add or Remove, and
> if it is Add, then we don't reach this location.)
>
> Therefore, we should remove this condition, and *unnest* the dependent
> logic.
>
> As a result of *that*, you'll have
>
>   return EFI_SUCCESS;
>   return EFI_SUCCESS;
>
> at the end of the function, and *then* you should remove either one of
> them.
>
> Thanks
> Laszlo
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111018): https://edk2.groups.io/g/devel/message/111018
Mute This Topic: https://groups.io/mt/102438298/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 3917 bytes --]

Re: [edk2-devel] [PATCH v2 5/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix UNUSED_VALUE Coverity issues

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 12573 bytes --]

It was hard to conclude at my end as well what to do where. So I just threw
it open ...

- Status assignment can be ignored or
- Maintain the existing behaviour and just log the error given the original
code existence for quite a long now

Various files were clubbed together being part of the same module and all
having the same UNUSED_VALUE issue.
If you say so, I will split, but many lines are just cosmetic changes
(indentation level) - after impact of inclusion / removal of Status storage
/ Status value check.
Will need to figure out what is the best split though :-)

On Tue, Nov 7, 2023 at 10:50 PM Laszlo Ersek <lersek@redhat.com> wrote:

> 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 <ray.ni@intel.com>
> > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com>
> > ---
> >  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 (#111019): https://edk2.groups.io/g/devel/message/111019
Mute This Topic: https://groups.io/mt/102438321/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 16715 bytes --]

Re: [edk2-devel] [PATCH v2 2/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix MISSING_BREAK Coverity issues

[Laszlo Ersek]

On 11/8/23 05:29, Ranbir Singh wrote:
> Hi Mike,
> 
> I agree that any manual inspection is sort of a burden, more so when it
> becomes repetitive in the long run.
> 
> When I was doing these Coverity checks (Nov-Dec' 2022), I was working in
> Dell and had access to the real systems to check the execution flow as
> well as the Coverity status. I could never post these patches while
> being there, but happened to raise Bugzilla's and post them there
> instead hoping that they would be taken up by somebody further.
> 
> I am no longer with Dell and later on when I found that those BZ /
> issues pointed out by Coverity still exist as there are no code changes
> in related contexts, I thought of taking them forward in whatever
> limited capacity I can. I am a bit hesitant to do any further code
> changes as now I do not have any systems to check the execution flow as
> well as the Coverity status. So, I do not guarantee, but will try to
> make the code changes wherever it is easy to ascertain that the
> functional flow would not be impacted and the same issue won't exist
> anymore.

This makes me a bit sad.

I was happy to see that a company seriously invested in cleaning up
Coverity issue reports all over edk2.

If you don't have the environment and/or the assignment to continue
doing that, then I agree that further tweaking these patches is
unjustifed. (Sorry, I didn't realize the background when I reviewed your
patches.) New versions would effectively mean "untested" code (untested
as in not tested with Coverity specifically). In particular because the
purported warning fixes will require real edk2 review (and occasionally
regression testing even), which doesn't look good if we can't even be
sure that the change actually placates coverity.

I guess we should upstream those of your patches that are fine right as
they are, and drop the rest for now. (A pity!) Accordingly, if you think
some of my review comments are not especially important in this light
(i.e., whenever it is better to take the patch as is, than to drop an
updated version due to untestability), then please do comment so explicitly!

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111142): https://edk2.groups.io/g/devel/message/111142
Mute This Topic: https://groups.io/mt/102438299/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Laszlo Ersek]

On 11/10/23 07:11, Ranbir Singh wrote:
> EFI_NOT_READY was listed as one of the error return values in the
> function header of StartPciDevices(). So, I considered it here.
> 
> Maybe we can go by a dual approach, including CpuDeadLoop() in
> StartPciDevices() as well as add return value check at the call site in
> PciBusDriverBindingStart().

I don't think this makes much sense, given that execution is not
supposed to continue past CpuDeadLoop(), so the new error check would
not be reached.

I think two options are realistic:

- replace the assert() with a CpuDeadLoop() -- this is my preference

- keep the assert, add the "if", and in the caller, handle the
EFI_NOT_READY error. This is workable too. (Keeping the assert is goog
because it shows that we don't expect the "if" to fire.)

Anyway, now that we have no way to verify the change against Coverity, I
don't know if this patch is worth the churn. :( As I said, this ASSERT()
is one of those few justified ones in edk2 core that can indeed never
fail, IMO.

Laszlo


> 
> On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com>> wrote:
> 
>     On 11/7/23 07:19, Ranbir Singh wrote:
>     > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
>     >
>     > The function StartPciDevices has a check
>     >
>     >     ASSERT (RootBridge != NULL);
>     >
>     > but this comes into play only in DEBUG mode. In Release mode, there
>     > is no handling if the RootBridge value is NULL and the code proceeds
>     > to unconditionally dereference "RootBridge" which will lead to CRASH.
>     >
>     > Hence, for safety add NULL pointer checks always and return
>     > EFI_NOT_READY if RootBridge value is NULL which is one of the return
>     > values as mentioned in the function description header.
>     >
>     > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
>     <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>
>     >
>     > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
>     > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com
>     <mailto:veeresh.sangolli@dellteam.com>>
>     > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
>     > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com
>     <mailto:rsingh@ventanamicro.com>>
>     > ---
>     >  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++-
>     >  1 file changed, 4 insertions(+), 1 deletion(-)
>     >
>     > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>     b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>     > index 581e9075ad41..3de80d98370e 100644
>     > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>     > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>     > @@ -772,7 +772,10 @@ StartPciDevices (
>     >    LIST_ENTRY     *CurrentLink;
>     > 
>     >    RootBridge = GetRootBridgeByHandle (Controller);
>     > -  ASSERT (RootBridge != NULL);
>     > +  if (RootBridge == NULL) {
>     > +    return EFI_NOT_READY;
>     > +  }
>     > +
>     >    ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle;
>     > 
>     >    CurrentLink = mPciDevicePool.ForwardLink;
> 
>     I don't think this is a good fix.
> 
>     There is one call site, namely in PciBusDriverBindingStart(). That call
>     site does not check the return value. (Of course /s)
> 
>     I think that this ASSERT() can indeed never fail. Therefore I suggest
>     CpuDeadLoop() instead.
> 
>     If you insist that CpuDeadLoop() is "too risky" here, then the patch is
>     acceptable, but then the StartPciDevices() call site in
>     PciBusDriverBindingStart() must check the error properly: we must not
>     install "gEfiPciEnumerationCompleteProtocolGuid", and the function must
>     propagate the error outwards.
> 
>     Laszlo
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111143): https://edk2.groups.io/g/devel/message/111143
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 5096 bytes --]

On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek <lersek@redhat.com> wrote:

> On 11/10/23 07:11, Ranbir Singh wrote:
> > EFI_NOT_READY was listed as one of the error return values in the
> > function header of StartPciDevices(). So, I considered it here.
> >
> > Maybe we can go by a dual approach, including CpuDeadLoop() in
> > StartPciDevices() as well as add return value check at the call site in
> > PciBusDriverBindingStart().
>
> I don't think this makes much sense, given that execution is not
> supposed to continue past CpuDeadLoop(), so the new error check would
> not be reached.
>
> I think two options are realistic:
>
> - replace the assert() with a CpuDeadLoop() -- this is my preference
>
> - keep the assert, add the "if", and in the caller, handle the
> EFI_NOT_READY error. This is workable too. (Keeping the assert is goog
> because it shows that we don't expect the "if" to fire.)
>
> Anyway, now that we have no way to verify the change against Coverity, I
> don't know if this patch is worth the churn. :( As I said, this ASSERT()
> is one of those few justified ones in edk2 core that can indeed never
> fail, IMO.
>
> Laszlo
>
>
See, Does the following change look acceptable to you ?

    ASSERT (RootBridge != NULL);
+  if (RootBridge == NULL) {
+    CpuDeadLoop ();
+    return EFI_NOT_READY;
+  }
+

which retains the existing assert, adds the NULL pointer check and includes
CpuDeadLoop () within the if block. As a result of CpuDeadLoop (), the
return statement afterwards will never reach execution (=> no need to
handle this return value at the call sites), but will satisfy static
analysis tools as the "RootBridge" dereference scenario doesn't arise
thereafter.


> >
> > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com
> > <mailto:lersek@redhat.com>> wrote:
> >
> >     On 11/7/23 07:19, Ranbir Singh wrote:
> >     > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >     >
> >     > The function StartPciDevices has a check
> >     >
> >     >     ASSERT (RootBridge != NULL);
> >     >
> >     > but this comes into play only in DEBUG mode. In Release mode, there
> >     > is no handling if the RootBridge value is NULL and the code
> proceeds
> >     > to unconditionally dereference "RootBridge" which will lead to
> CRASH.
> >     >
> >     > Hence, for safety add NULL pointer checks always and return
> >     > EFI_NOT_READY if RootBridge value is NULL which is one of the
> return
> >     > values as mentioned in the function description header.
> >     >
> >     > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> >     <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>
> >     >
> >     > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
> >     > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com
> >     <mailto:veeresh.sangolli@dellteam.com>>
> >     > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >     > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com
> >     <mailto:rsingh@ventanamicro.com>>
> >     > ---
> >     >  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++-
> >     >  1 file changed, 4 insertions(+), 1 deletion(-)
> >     >
> >     > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> >     b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> >     > index 581e9075ad41..3de80d98370e 100644
> >     > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> >     > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> >     > @@ -772,7 +772,10 @@ StartPciDevices (
> >     >    LIST_ENTRY     *CurrentLink;
> >     >
> >     >    RootBridge = GetRootBridgeByHandle (Controller);
> >     > -  ASSERT (RootBridge != NULL);
> >     > +  if (RootBridge == NULL) {
> >     > +    return EFI_NOT_READY;
> >     > +  }
> >     > +
> >     >    ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle;
> >     >
> >     >    CurrentLink = mPciDevicePool.ForwardLink;
> >
> >     I don't think this is a good fix.
> >
> >     There is one call site, namely in PciBusDriverBindingStart(). That
> call
> >     site does not check the return value. (Of course /s)
> >
> >     I think that this ASSERT() can indeed never fail. Therefore I suggest
> >     CpuDeadLoop() instead.
> >
> >     If you insist that CpuDeadLoop() is "too risky" here, then the patch
> is
> >     acceptable, but then the StartPciDevices() call site in
> >     PciBusDriverBindingStart() must check the error properly: we must not
> >     install "gEfiPciEnumerationCompleteProtocolGuid", and the function
> must
> >     propagate the error outwards.
> >
> >     Laszlo
> >
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111202): https://edk2.groups.io/g/devel/message/111202
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 7728 bytes --]

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Michael D Kinney]

[-- Attachment #1: Type: text/plain, Size: 6674 bytes --]

Hi Ranbir,

First I want to recognize your efforts to collect Coverity issues and propose changes to address
them.

I still disagree with adding CpuDealLoop() for any static analysis issues.

There have been previous discussions about adding a PANIC() or FATAL() macro that would
perform platform specific actions if a condition is detected where the boot of the platform
can not continue.  A platform get to make the choice to log or reboot or hang, etc.  Not the
code that detected the condition.

https://edk2.groups.io/g/devel/message/86597

Unfortunately, in order to fix some of these static analysis issues correctly, we are going
to have to identify the use of ASSERT() that really is a fatal condition that can not continue
and introduce an implementation approach that provides a platform handler and
satisfies the static analysis tools.

We also have to evaluate if a return error status with a DEBUG_ERROR message would be a better
choice than an ASSERT() that can be filtered out by build options.

Best regards,

Mike


From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Ranbir Singh
Sent: Tuesday, November 14, 2023 7:08 AM
To: Laszlo Ersek <lersek@redhat.com>
Cc: devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli <veeresh.sangolli@dellteam.com>
Subject: Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue



On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> wrote:
On 11/10/23 07:11, Ranbir Singh wrote:
> EFI_NOT_READY was listed as one of the error return values in the
> function header of StartPciDevices(). So, I considered it here.
>
> Maybe we can go by a dual approach, including CpuDeadLoop() in
> StartPciDevices() as well as add return value check at the call site in
> PciBusDriverBindingStart().

I don't think this makes much sense, given that execution is not
supposed to continue past CpuDeadLoop(), so the new error check would
not be reached.

I think two options are realistic:

- replace the assert() with a CpuDeadLoop() -- this is my preference

- keep the assert, add the "if", and in the caller, handle the
EFI_NOT_READY error. This is workable too. (Keeping the assert is goog
because it shows that we don't expect the "if" to fire.)

Anyway, now that we have no way to verify the change against Coverity, I
don't know if this patch is worth the churn. :( As I said, this ASSERT()
is one of those few justified ones in edk2 core that can indeed never
fail, IMO.

Laszlo

See, Does the following change look acceptable to you ?

    ASSERT (RootBridge != NULL);
+  if (RootBridge == NULL) {
+    CpuDeadLoop ();
+    return EFI_NOT_READY;
+  }
+

which retains the existing assert, adds the NULL pointer check and includes CpuDeadLoop () within the if block. As a result of CpuDeadLoop (), the return statement afterwards will never reach execution (=> no need to handle this return value at the call sites), but will satisfy static analysis tools as the "RootBridge" dereference scenario doesn't arise thereafter.


>
> On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>
> <mailto:lersek@redhat.com<mailto:lersek@redhat.com>>> wrote:
>
>     On 11/7/23 07:19, Ranbir Singh wrote:
>     > From: Ranbir Singh <Ranbir.Singh3@Dell.com<mailto:Ranbir.Singh3@Dell.com>>
>     >
>     > The function StartPciDevices has a check
>     >
>     >     ASSERT (RootBridge != NULL);
>     >
>     > but this comes into play only in DEBUG mode. In Release mode, there
>     > is no handling if the RootBridge value is NULL and the code proceeds
>     > to unconditionally dereference "RootBridge" which will lead to CRASH.
>     >
>     > Hence, for safety add NULL pointer checks always and return
>     > EFI_NOT_READY if RootBridge value is NULL which is one of the return
>     > values as mentioned in the function description header.
>     >
>     > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
>     <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>
>     >
>     > Cc: Ray Ni <ray.ni@intel.com<mailto:ray.ni@intel.com> <mailto:ray.ni@intel.com<mailto:ray.ni@intel.com>>>
>     > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com<mailto:veeresh.sangolli@dellteam.com>
>     <mailto:veeresh.sangolli@dellteam.com<mailto:veeresh.sangolli@dellteam.com>>>
>     > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com<mailto:Ranbir.Singh3@Dell.com>>
>     > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>
>     <mailto:rsingh@ventanamicro.com<mailto:rsingh@ventanamicro.com>>>
>     > ---
>     >  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++-
>     >  1 file changed, 4 insertions(+), 1 deletion(-)
>     >
>     > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>     b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>     > index 581e9075ad41..3de80d98370e 100644
>     > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>     > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>     > @@ -772,7 +772,10 @@ StartPciDevices (
>     >    LIST_ENTRY     *CurrentLink;
>     >
>     >    RootBridge = GetRootBridgeByHandle (Controller);
>     > -  ASSERT (RootBridge != NULL);
>     > +  if (RootBridge == NULL) {
>     > +    return EFI_NOT_READY;
>     > +  }
>     > +
>     >    ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle;
>     >
>     >    CurrentLink = mPciDevicePool.ForwardLink;
>
>     I don't think this is a good fix.
>
>     There is one call site, namely in PciBusDriverBindingStart(). That call
>     site does not check the return value. (Of course /s)
>
>     I think that this ASSERT() can indeed never fail. Therefore I suggest
>     CpuDeadLoop() instead.
>
>     If you insist that CpuDeadLoop() is "too risky" here, then the patch is
>     acceptable, but then the StartPciDevices() call site in
>     PciBusDriverBindingStart() must check the error properly: we must not
>     install "gEfiPciEnumerationCompleteProtocolGuid", and the function must
>     propagate the error outwards.
>
>     Laszlo
>



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111207): https://edk2.groups.io/g/devel/message/111207
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 13770 bytes --]

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 7650 bytes --]

On Tue, Nov 14, 2023 at 9:51 PM Kinney, Michael D <
michael.d.kinney@intel.com> wrote:

> Hi Ranbir,
>
>
>
> First I want to recognize your efforts to collect Coverity issues and
> propose changes to address
>
> them.
>

Thanks Mike.


>
>
> I still disagree with adding CpuDealLoop() for any static analysis issues.
>

A bit of correction here.
CpuDeadLoop() is not exactly an addition for static analysis issues. For
that, the NULL pointer check and return statement in the if block are
sufficient.
However, CpuDeadLoop() is added as suggested by Laszlo to introduce a hang
behaviour in RELEASE builds as well when the situation is anyway not safe
to progress normally. That said, it may not still be required at every
point and hence needs to be assessed on a case to case basis.


>
> There have been previous discussions about adding a PANIC() or FATAL()
> macro that would
>
> perform platform specific actions if a condition is detected where the
> boot of the platform
>
> can not continue.  A platform get to make the choice to log or reboot or
> hang, etc.  Not the
>
> code that detected the condition.
>
>
>
> https://edk2.groups.io/g/devel/message/86597
>
>
>
> Unfortunately, in order to fix some of these static analysis issues
> correctly, we are going
>
> to have to identify the use of ASSERT() that really is a fatal condition
> that can not continue
>
> and introduce an implementation approach that provides a platform handler
> and
>
> satisfies the static analysis tools.
>
>
>
> We also have to evaluate if a return error status with a DEBUG_ERROR
> message would be a better
>
> choice than an ASSERT() that can be filtered out by build options.
>

Note that the existing ASSERT will give a DEBUG message even when
CpuDeadLoop is added in the code later.


>
>
> Best regards,
>
>
>
> Mike
>
>
>

Generally speaking, there now seems to be different views coming from you
and Laszlo. We might have to wait for some sort of agreement to be reached.


>
>
> *From:* devel@edk2.groups.io <devel@edk2.groups.io> * On Behalf Of *Ranbir
> Singh
> *Sent:* Tuesday, November 14, 2023 7:08 AM
> *To:* Laszlo Ersek <lersek@redhat.com>
> *Cc:* devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli <
> veeresh.sangolli@dellteam.com>
> *Subject:* Re: [edk2-devel] [PATCH v2 4/5]
> MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue
>
>
>
>
>
>
>
> On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 11/10/23 07:11, Ranbir Singh wrote:
> > EFI_NOT_READY was listed as one of the error return values in the
> > function header of StartPciDevices(). So, I considered it here.
> >
> > Maybe we can go by a dual approach, including CpuDeadLoop() in
> > StartPciDevices() as well as add return value check at the call site in
> > PciBusDriverBindingStart().
>
> I don't think this makes much sense, given that execution is not
> supposed to continue past CpuDeadLoop(), so the new error check would
> not be reached.
>
> I think two options are realistic:
>
> - replace the assert() with a CpuDeadLoop() -- this is my preference
>
> - keep the assert, add the "if", and in the caller, handle the
> EFI_NOT_READY error. This is workable too. (Keeping the assert is goog
> because it shows that we don't expect the "if" to fire.)
>
> Anyway, now that we have no way to verify the change against Coverity, I
> don't know if this patch is worth the churn. :( As I said, this ASSERT()
> is one of those few justified ones in edk2 core that can indeed never
> fail, IMO.
>
> Laszlo
>
>
>
> See, Does the following change look acceptable to you ?
>
>
>
>     ASSERT (RootBridge != NULL);
> +  if (RootBridge == NULL) {
>
> +    CpuDeadLoop ();
> +    return EFI_NOT_READY;
> +  }
>
> +
>
>
>
> which retains the existing assert, adds the NULL pointer check and
> includes CpuDeadLoop () within the if block. As a result of CpuDeadLoop (),
> the return statement afterwards will never reach execution (=> no need to
> handle this return value at the call sites), but will satisfy static
> analysis tools as the "RootBridge" dereference scenario doesn't arise
> thereafter.
>
>
>
>
> >
> > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com
> > <mailto:lersek@redhat.com>> wrote:
> >
> >     On 11/7/23 07:19, Ranbir Singh wrote:
> >     > From: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >     >
> >     > The function StartPciDevices has a check
> >     >
> >     >     ASSERT (RootBridge != NULL);
> >     >
> >     > but this comes into play only in DEBUG mode. In Release mode, there
> >     > is no handling if the RootBridge value is NULL and the code
> proceeds
> >     > to unconditionally dereference "RootBridge" which will lead to
> CRASH.
> >     >
> >     > Hence, for safety add NULL pointer checks always and return
> >     > EFI_NOT_READY if RootBridge value is NULL which is one of the
> return
> >     > values as mentioned in the function description header.
> >     >
> >     > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
> >     <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>
> >     >
> >     > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>>
> >     > Co-authored-by: Veeresh Sangolli <veeresh.sangolli@dellteam.com
> >     <mailto:veeresh.sangolli@dellteam.com>>
> >     > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com>
> >     > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com
> >     <mailto:rsingh@ventanamicro.com>>
> >     > ---
> >     >  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++-
> >     >  1 file changed, 4 insertions(+), 1 deletion(-)
> >     >
> >     > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> >     b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> >     > index 581e9075ad41..3de80d98370e 100644
> >     > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> >     > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
> >     > @@ -772,7 +772,10 @@ StartPciDevices (
> >     >    LIST_ENTRY     *CurrentLink;
> >     >
> >     >    RootBridge = GetRootBridgeByHandle (Controller);
> >     > -  ASSERT (RootBridge != NULL);
> >     > +  if (RootBridge == NULL) {
> >     > +    return EFI_NOT_READY;
> >     > +  }
> >     > +
> >     >    ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle;
> >     >
> >     >    CurrentLink = mPciDevicePool.ForwardLink;
> >
> >     I don't think this is a good fix.
> >
> >     There is one call site, namely in PciBusDriverBindingStart(). That
> call
> >     site does not check the return value. (Of course /s)
> >
> >     I think that this ASSERT() can indeed never fail. Therefore I suggest
> >     CpuDeadLoop() instead.
> >
> >     If you insist that CpuDeadLoop() is "too risky" here, then the patch
> is
> >     acceptable, but then the StartPciDevices() call site in
> >     PciBusDriverBindingStart() must check the error properly: we must not
> >     install "gEfiPciEnumerationCompleteProtocolGuid", and the function
> must
> >     propagate the error outwards.
> >
> >     Laszlo
> >
>
> 
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111209): https://edk2.groups.io/g/devel/message/111209
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 14653 bytes --]

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Michael Kubacki]

On 11/14/2023 11:21 AM, Michael D Kinney wrote:
> Hi Ranbir,
> 
> First I want to recognize your efforts to collect Coverity issues and 
> propose changes to address
> 
> them.
> 
> I still disagree with adding CpuDealLoop() for any static analysis issues.
> 
> There have been previous discussions about adding a PANIC() or FATAL() 
> macro that would
> 
> perform platform specific actions if a condition is detected where the 
> boot of the platform
> 
> can not continue.  A platform get to make the choice to log or reboot or 
> hang, etc.  Not the
> 
> code that detected the condition.
> 
> https://edk2.groups.io/g/devel/message/86597 
> <https://edk2.groups.io/g/devel/message/86597>
> 
After going through hundreds of edk2 static analysis findings, we found 
a small number of cases where an interface such as PanicLib was useful 
and recently added an implementation 
https://github.com/microsoft/mu_basecore/blob/release/202302/MdePkg/Include/Library/PanicLib.h.

For example, the return value from calls to MpInitLibWhoAmI() in 
exception related code often goes unchecked and it's been used there. 
Being able to separate the library instance implementation linked to a 
given module from a more broad library class like DebugLib for these 
cases has also been helpful.

> Unfortunately, in order to fix some of these static analysis issues 
> correctly, we are going
> 
> to have to identify the use of ASSERT() that really is a fatal condition 
> that can not continue
> 
> and introduce an implementation approach that provides a platform 
> handler and
> 
> satisfies the static analysis tools.
> 
> We also have to evaluate if a return error status with a DEBUG_ERROR 
> message would be a better
> 
> choice than an ASSERT() that can be filtered out by build options.
> 
> Best regards,
> 
> Mike
> 
> *From:* devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of 
> *Ranbir Singh
> *Sent:* Tuesday, November 14, 2023 7:08 AM
> *To:* Laszlo Ersek <lersek@redhat.com>
> *Cc:* devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Veeresh Sangolli 
> <veeresh.sangolli@dellteam.com>
> *Subject:* Re: [edk2-devel] [PATCH v2 4/5] 
> MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue
> 
> On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek <lersek@redhat.com 
> <mailto:lersek@redhat.com>> wrote:
> 
>     On 11/10/23 07:11, Ranbir Singh wrote:
>      > EFI_NOT_READY was listed as one of the error return values in the
>      > function header of StartPciDevices(). So, I considered it here.
>      >
>      > Maybe we can go by a dual approach, including CpuDeadLoop() in
>      > StartPciDevices() as well as add return value check at the call
>     site in
>      > PciBusDriverBindingStart().
> 
>     I don't think this makes much sense, given that execution is not
>     supposed to continue past CpuDeadLoop(), so the new error check would
>     not be reached.
> 
>     I think two options are realistic:
> 
>     - replace the assert() with a CpuDeadLoop() -- this is my preference
> 
>     - keep the assert, add the "if", and in the caller, handle the
>     EFI_NOT_READY error. This is workable too. (Keeping the assert is goog
>     because it shows that we don't expect the "if" to fire.)
> 
>     Anyway, now that we have no way to verify the change against Coverity, I
>     don't know if this patch is worth the churn. :( As I said, this ASSERT()
>     is one of those few justified ones in edk2 core that can indeed never
>     fail, IMO.
> 
>     Laszlo
> 
> See, Does the following change look acceptable to you ?
> 
>      ASSERT (RootBridge != NULL);
> +  if (RootBridge == NULL) {
> 
> +    CpuDeadLoop ();
> +    return EFI_NOT_READY;
> +  }
> 
> +
> 
> which retains the existing assert, adds the NULL pointer check and 
> includes CpuDeadLoop () within the if block. As a result of CpuDeadLoop 
> (), the return statement afterwards will never reach execution (=> no 
> need to handle this return value at the call sites), but will satisfy 
> static analysis tools as the "RootBridge" dereference scenario doesn't 
> arise thereafter.
> 
> 
>      >
>      > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com
>     <mailto:lersek@redhat.com>
>      > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote:
>      >
>      >     On 11/7/23 07:19, Ranbir Singh wrote:
>      >     > From: Ranbir Singh <Ranbir.Singh3@Dell.com
>     <mailto:Ranbir.Singh3@Dell.com>>
>      >     >
>      >     > The function StartPciDevices has a check
>      >     >
>      >     >     ASSERT (RootBridge != NULL);
>      >     >
>      >     > but this comes into play only in DEBUG mode. In Release
>     mode, there
>      >     > is no handling if the RootBridge value is NULL and the code
>     proceeds
>      >     > to unconditionally dereference "RootBridge" which will lead
>     to CRASH.
>      >     >
>      >     > Hence, for safety add NULL pointer checks always and return
>      >     > EFI_NOT_READY if RootBridge value is NULL which is one of
>     the return
>      >     > values as mentioned in the function description header.
>      >     >
>      >     > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
>     <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>
>      >     <https://bugzilla.tianocore.org/show_bug.cgi?id=4239
>     <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>>
>      >     >
>      >     > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>
>     <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com>>>
>      >     > Co-authored-by: Veeresh Sangolli
>     <veeresh.sangolli@dellteam.com <mailto:veeresh.sangolli@dellteam.com>
>      >     <mailto:veeresh.sangolli@dellteam.com
>     <mailto:veeresh.sangolli@dellteam.com>>>
>      >     > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com
>     <mailto:Ranbir.Singh3@Dell.com>>
>      >     > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com
>     <mailto:rsingh@ventanamicro.com>
>      >     <mailto:rsingh@ventanamicro.com
>     <mailto:rsingh@ventanamicro.com>>>
>      >     > ---
>      >     >  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++-
>      >     >  1 file changed, 4 insertions(+), 1 deletion(-)
>      >     >
>      >     > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>      >     b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>      >     > index 581e9075ad41..3de80d98370e 100644
>      >     > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>      >     > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>      >     > @@ -772,7 +772,10 @@ StartPciDevices (
>      >     >    LIST_ENTRY     *CurrentLink;
>      >     >
>      >     >    RootBridge = GetRootBridgeByHandle (Controller);
>      >     > -  ASSERT (RootBridge != NULL);
>      >     > +  if (RootBridge == NULL) {
>      >     > +    return EFI_NOT_READY;
>      >     > +  }
>      >     > +
>      >     >    ThisHostBridge = RootBridge->PciRootBridgeIo->ParentHandle;
>      >     >
>      >     >    CurrentLink = mPciDevicePool.ForwardLink;
>      >
>      >     I don't think this is a good fix.
>      >
>      >     There is one call site, namely in PciBusDriverBindingStart().
>     That call
>      >     site does not check the return value. (Of course /s)
>      >
>      >     I think that this ASSERT() can indeed never fail. Therefore I
>     suggest
>      >     CpuDeadLoop() instead.
>      >
>      >     If you insist that CpuDeadLoop() is "too risky" here, then
>     the patch is
>      >     acceptable, but then the StartPciDevices() call site in
>      >     PciBusDriverBindingStart() must check the error properly: we
>     must not
>      >     install "gEfiPciEnumerationCompleteProtocolGuid", and the
>     function must
>      >     propagate the error outwards.
>      >
>      >     Laszlo
>      >
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111214): https://edk2.groups.io/g/devel/message/111214
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Laszlo Ersek]

On 11/14/23 17:21, Kinney, Michael D wrote:
> Hi Ranbir,
> 
>  
> 
> First I want to recognize your efforts to collect Coverity issues and
> propose changes to address
> them.
> 
> I still disagree with adding CpuDealLoop() for any static analysis issues.
> 
> There have been previous discussions about adding a PANIC() or FATAL()
> macro that would
> perform platform specific actions if a condition is detected where the
> boot of the platform
> can not continue.  A platform get to make the choice to log or reboot or
> hang, etc.  Not the
> code that detected the condition.
> 
> https://edk2.groups.io/g/devel/message/86597
> <https://edk2.groups.io/g/devel/message/86597>

This is indeed a great idea.

I didn't know about that discussion. Perhaps a new DebugLib API would
handle this (i.e., "panic").

I've been certainly proposing CpuDeadLoop() as a means to panic.

Did the thread conclude anything tangible?

> Unfortunately, in order to fix some of these static analysis issues
> correctly, we are going
> to have to identify the use of ASSERT() that really is a fatal condition
> that can not continue

Absolutely.

> and introduce an implementation approach that provides a platform
> handler and
> satisfies the static analysis tools.

The "platform handler" is the difficult part. If the above-noted
discussion from 2022 didn't produce an agreement, should we really block
the static analyzer fixes on an agreed-upon panic API? I'm concerned
that would just cause these fixes to get stuck. And I don't consider
CpuDeadLoop()s added for this purpose serious technical debt. They are
easy to find and update later, assuming we also add comments.

> We also have to evaluate if a return error status with a DEBUG_ERROR
> message would be a better
> choice than an ASSERT() that can be filtered out by build options.

I agree 100% that this would be better for the codebase, but the work
needed for this is (IMO) impossible to contain. ASSERT() has been abused
for a long time *because* it seemed to allow the programmer to ignore
any related error paths. If we now want to implement those error paths
retroactively (which would be absolutely the right thing to do from a
software engineering perspective), then immense amounts of work are
going to be needed (patch review and regression testing), and I think
it's just not practical to dump all that on someone that wants to
improve the status quo. Replacing an invalid ASSERT() with a panic is
honest about the current situation, is safer regarding RELEASE builds,
and its work demand (regression testing, patch review) is tolerable.

I do agree that, if the error path mostly exists already, then returning
errors for data/environment-based error conditions (i.e., not for
algorithmic invariant failures) is best.

Where we need to be extremely vigilant is new patches. We must
uncompromisingly reject *new* abuses of ASSERT(), in new patches.

Anyway, it seems that we've been trying to steer Ranbir in opposite
directions. I'll let you take the lead on this; for one, I've not been
aware of the panic api discussion for 2022!

(I don't feel especially pushy about fixing coverity issues, it's just
that Ranbir has been contributing such patches, which I've found very
welcome, and I wanted to help out with reviews.)

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111247): https://edk2.groups.io/g/devel/message/111247
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Laszlo Ersek]

On 11/14/23 17:53, Ranbir Singh wrote:

> Generally speaking, there now seems to be different views coming from
> you and Laszlo.

Yes.

> We might have to wait for some sort of agreement to be
> reached.

I don't insist on CpuDeadLoop() *specifically*. Only the following two
generic points matter to me:

(1) Stop abusing ASSERT (both because it is compiled out of RELEASE
builds, and because it is conceptually unsuitable for catching data- and
environment-dependent error conditions). ASSERT must only be used for
stating (well, "asserting") algorithmic invariants.

(2) Upon detecting an algorithmic invariant failure, call *some* API
that, at the same time: (2.a) prevents execution from continuing, (2.b)
*cannot* be removed from RELEASE builds, (2.c) informs all static
analysis tools we use that execution cannot continue past that point.

For (2), Mike seems to have an additional requirement: (2.d) make the
implementation customizable by the platform, including any information
shown to, or logged for, the user (or supervisor software).

I have nothing against that additional requirement.

My concern is that "perfect" is going to get in the way of "good enough"
once again.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111248): https://edk2.groups.io/g/devel/message/111248
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Laszlo Ersek]

On 11/14/23 20:37, Michael Kubacki wrote:
> On 11/14/2023 11:21 AM, Michael D Kinney wrote:
>> Hi Ranbir,
>>
>> First I want to recognize your efforts to collect Coverity issues and
>> propose changes to address
>>
>> them.
>>
>> I still disagree with adding CpuDealLoop() for any static analysis
>> issues.
>>
>> There have been previous discussions about adding a PANIC() or FATAL()
>> macro that would
>>
>> perform platform specific actions if a condition is detected where the
>> boot of the platform
>>
>> can not continue.  A platform get to make the choice to log or reboot
>> or hang, etc.  Not the
>>
>> code that detected the condition.
>>
>> https://edk2.groups.io/g/devel/message/86597
>> <https://edk2.groups.io/g/devel/message/86597>
>>
> After going through hundreds of edk2 static analysis findings, we found
> a small number of cases where an interface such as PanicLib was useful
> and recently added an implementation
> https://github.com/microsoft/mu_basecore/blob/release/202302/MdePkg/Include/Library/PanicLib.h.
> 
> For example, the return value from calls to MpInitLibWhoAmI() in
> exception related code often goes unchecked and it's been used there.
> Being able to separate the library instance implementation linked to a
> given module from a more broad library class like DebugLib for these
> cases has also been helpful.

Ah, great reminder that we have ANALYZER_UNREACHABLE. I've totally
forgotten about that; my apologies.

... I initially thought that a plain "CONST CHAR8  *Description" was not
too flexible, but on second thought, it should be exactly right. Reason
being, it's very easy to print. Format specifiers and variable arguments
(PrintLib style) may be too complex to implement safely within
PanicReport(). Arguably, no PanicReport() implementation should be
obligated (by the interface) to depend on, or to reimplement, PrintLib.
If the calling context permits, the caller can just use PrintLib to
format the message to a local buffer (on the stack), and pass that to
PanicReport.

So this looks very useful to me; can you upstream it?

Laszlo


> 
>> Unfortunately, in order to fix some of these static analysis issues
>> correctly, we are going
>>
>> to have to identify the use of ASSERT() that really is a fatal
>> condition that can not continue
>>
>> and introduce an implementation approach that provides a platform
>> handler and
>>
>> satisfies the static analysis tools.
>>
>> We also have to evaluate if a return error status with a DEBUG_ERROR
>> message would be a better
>>
>> choice than an ASSERT() that can be filtered out by build options.
>>
>> Best regards,
>>
>> Mike
>>
>> *From:* devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of
>> *Ranbir Singh
>> *Sent:* Tuesday, November 14, 2023 7:08 AM
>> *To:* Laszlo Ersek <lersek@redhat.com>
>> *Cc:* devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Veeresh
>> Sangolli <veeresh.sangolli@dellteam.com>
>> *Subject:* Re: [edk2-devel] [PATCH v2 4/5]
>> MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue
>>
>> On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek <lersek@redhat.com
>> <mailto:lersek@redhat.com>> wrote:
>>
>>     On 11/10/23 07:11, Ranbir Singh wrote:
>>      > EFI_NOT_READY was listed as one of the error return values in the
>>      > function header of StartPciDevices(). So, I considered it here.
>>      >
>>      > Maybe we can go by a dual approach, including CpuDeadLoop() in
>>      > StartPciDevices() as well as add return value check at the call
>>     site in
>>      > PciBusDriverBindingStart().
>>
>>     I don't think this makes much sense, given that execution is not
>>     supposed to continue past CpuDeadLoop(), so the new error check would
>>     not be reached.
>>
>>     I think two options are realistic:
>>
>>     - replace the assert() with a CpuDeadLoop() -- this is my preference
>>
>>     - keep the assert, add the "if", and in the caller, handle the
>>     EFI_NOT_READY error. This is workable too. (Keeping the assert is
>> goog
>>     because it shows that we don't expect the "if" to fire.)
>>
>>     Anyway, now that we have no way to verify the change against
>> Coverity, I
>>     don't know if this patch is worth the churn. :( As I said, this
>> ASSERT()
>>     is one of those few justified ones in edk2 core that can indeed never
>>     fail, IMO.
>>
>>     Laszlo
>>
>> See, Does the following change look acceptable to you ?
>>
>>      ASSERT (RootBridge != NULL);
>> +  if (RootBridge == NULL) {
>>
>> +    CpuDeadLoop ();
>> +    return EFI_NOT_READY;
>> +  }
>>
>> +
>>
>> which retains the existing assert, adds the NULL pointer check and
>> includes CpuDeadLoop () within the if block. As a result of
>> CpuDeadLoop (), the return statement afterwards will never reach
>> execution (=> no need to handle this return value at the call sites),
>> but will satisfy static analysis tools as the "RootBridge" dereference
>> scenario doesn't arise thereafter.
>>
>>
>>      >
>>      > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com
>>     <mailto:lersek@redhat.com>
>>      > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote:
>>      >
>>      >     On 11/7/23 07:19, Ranbir Singh wrote:
>>      >     > From: Ranbir Singh <Ranbir.Singh3@Dell.com
>>     <mailto:Ranbir.Singh3@Dell.com>>
>>      >     >
>>      >     > The function StartPciDevices has a check
>>      >     >
>>      >     >     ASSERT (RootBridge != NULL);
>>      >     >
>>      >     > but this comes into play only in DEBUG mode. In Release
>>     mode, there
>>      >     > is no handling if the RootBridge value is NULL and the code
>>     proceeds
>>      >     > to unconditionally dereference "RootBridge" which will lead
>>     to CRASH.
>>      >     >
>>      >     > Hence, for safety add NULL pointer checks always and return
>>      >     > EFI_NOT_READY if RootBridge value is NULL which is one of
>>     the return
>>      >     > values as mentioned in the function description header.
>>      >     >
>>      >     > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
>>     <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>
>>      >     <https://bugzilla.tianocore.org/show_bug.cgi?id=4239
>>     <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>>
>>      >     >
>>      >     > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>
>>     <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com>>>
>>      >     > Co-authored-by: Veeresh Sangolli
>>     <veeresh.sangolli@dellteam.com <mailto:veeresh.sangolli@dellteam.com>
>>      >     <mailto:veeresh.sangolli@dellteam.com
>>     <mailto:veeresh.sangolli@dellteam.com>>>
>>      >     > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com
>>     <mailto:Ranbir.Singh3@Dell.com>>
>>      >     > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com
>>     <mailto:rsingh@ventanamicro.com>
>>      >     <mailto:rsingh@ventanamicro.com
>>     <mailto:rsingh@ventanamicro.com>>>
>>      >     > ---
>>      >     >  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++-
>>      >     >  1 file changed, 4 insertions(+), 1 deletion(-)
>>      >     >
>>      >     > diff --git
>> a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>>      >     b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>>      >     > index 581e9075ad41..3de80d98370e 100644
>>      >     > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>>      >     > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>>      >     > @@ -772,7 +772,10 @@ StartPciDevices (
>>      >     >    LIST_ENTRY     *CurrentLink;
>>      >     >
>>      >     >    RootBridge = GetRootBridgeByHandle (Controller);
>>      >     > -  ASSERT (RootBridge != NULL);
>>      >     > +  if (RootBridge == NULL) {
>>      >     > +    return EFI_NOT_READY;
>>      >     > +  }
>>      >     > +
>>      >     >    ThisHostBridge =
>> RootBridge->PciRootBridgeIo->ParentHandle;
>>      >     >
>>      >     >    CurrentLink = mPciDevicePool.ForwardLink;
>>      >
>>      >     I don't think this is a good fix.
>>      >
>>      >     There is one call site, namely in PciBusDriverBindingStart().
>>     That call
>>      >     site does not check the return value. (Of course /s)
>>      >
>>      >     I think that this ASSERT() can indeed never fail. Therefore I
>>     suggest
>>      >     CpuDeadLoop() instead.
>>      >
>>      >     If you insist that CpuDeadLoop() is "too risky" here, then
>>     the patch is
>>      >     acceptable, but then the StartPciDevices() call site in
>>      >     PciBusDriverBindingStart() must check the error properly: we
>>     must not
>>      >     install "gEfiPciEnumerationCompleteProtocolGuid", and the
>>     function must
>>      >     propagate the error outwards.
>>      >
>>      >     Laszlo
>>      >
>>
>> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111249): https://edk2.groups.io/g/devel/message/111249
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Ranbir Singh]

[-- Attachment #1: Type: text/plain, Size: 4405 bytes --]

On Wed, Nov 15, 2023 at 3:20 PM Laszlo Ersek <lersek@redhat.com> wrote:

> On 11/14/23 17:21, Kinney, Michael D wrote:
> > Hi Ranbir,
> >
> >
> >
> > First I want to recognize your efforts to collect Coverity issues and
> > propose changes to address
> > them.
> >
> > I still disagree with adding CpuDealLoop() for any static analysis
> issues.
> >
> > There have been previous discussions about adding a PANIC() or FATAL()
> > macro that would
> > perform platform specific actions if a condition is detected where the
> > boot of the platform
> > can not continue.  A platform get to make the choice to log or reboot or
> > hang, etc.  Not the
> > code that detected the condition.
> >
> > https://edk2.groups.io/g/devel/message/86597
> > <https://edk2.groups.io/g/devel/message/86597>
>
> This is indeed a great idea.
>
> I didn't know about that discussion. Perhaps a new DebugLib API would
> handle this (i.e., "panic").
>
> I've been certainly proposing CpuDeadLoop() as a means to panic.
>
> Did the thread conclude anything tangible?
>
> > Unfortunately, in order to fix some of these static analysis issues
> > correctly, we are going
> > to have to identify the use of ASSERT() that really is a fatal condition
> > that can not continue
>
> Absolutely.
>
> > and introduce an implementation approach that provides a platform
> > handler and
> > satisfies the static analysis tools.
>
> The "platform handler" is the difficult part. If the above-noted
> discussion from 2022 didn't produce an agreement, should we really block
> the static analyzer fixes on an agreed-upon panic API? I'm concerned
> that would just cause these fixes to get stuck. And I don't consider
> CpuDeadLoop()s added for this purpose serious technical debt. They are
> easy to find and update later, assuming we also add comments.
>
>
I agree with the approach to not gate current fixes adding CpuDeadLoop().
Later on, it can be updated with the desired panic API and I can contribute
for those required changes related to patches submitted by me.

I can update current patches to carry additional comment in suffix form to
ease later search like
    CpuDeadLoop (); // TBD: replace with Panic API in future

Laszlo, Mike - Let me know if that works for now.


> > We also have to evaluate if a return error status with a DEBUG_ERROR
> > message would be a better
> > choice than an ASSERT() that can be filtered out by build options.
>
> I agree 100% that this would be better for the codebase, but the work
> needed for this is (IMO) impossible to contain. ASSERT() has been abused
> for a long time *because* it seemed to allow the programmer to ignore
> any related error paths. If we now want to implement those error paths
> retroactively (which would be absolutely the right thing to do from a
> software engineering perspective), then immense amounts of work are
> going to be needed (patch review and regression testing), and I think
> it's just not practical to dump all that on someone that wants to
> improve the status quo. Replacing an invalid ASSERT() with a panic is
> honest about the current situation, is safer regarding RELEASE builds,
> and its work demand (regression testing, patch review) is tolerable.
>
> I do agree that, if the error path mostly exists already, then returning
> errors for data/environment-based error conditions (i.e., not for
> algorithmic invariant failures) is best.
>
> Where we need to be extremely vigilant is new patches. We must
> uncompromisingly reject *new* abuses of ASSERT(), in new patches.
>
> Anyway, it seems that we've been trying to steer Ranbir in opposite
> directions. I'll let you take the lead on this; for one, I've not been
> aware of the panic api discussion for 2022!
>
> (I don't feel especially pushy about fixing coverity issues, it's just
> that Ranbir has been contributing such patches, which I've found very
> welcome, and I wanted to help out with reviews.)
>
> Laszlo
>
>


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111444): https://edk2.groups.io/g/devel/message/111444
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 5872 bytes --]

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Michael Kubacki]

On 11/15/2023 5:10 AM, Laszlo Ersek wrote:
> On 11/14/23 20:37, Michael Kubacki wrote:
>> On 11/14/2023 11:21 AM, Michael D Kinney wrote:
>>> Hi Ranbir,
>>>
>>> First I want to recognize your efforts to collect Coverity issues and
>>> propose changes to address
>>>
>>> them.
>>>
>>> I still disagree with adding CpuDealLoop() for any static analysis
>>> issues.
>>>
>>> There have been previous discussions about adding a PANIC() or FATAL()
>>> macro that would
>>>
>>> perform platform specific actions if a condition is detected where the
>>> boot of the platform
>>>
>>> can not continue.  A platform get to make the choice to log or reboot
>>> or hang, etc.  Not the
>>>
>>> code that detected the condition.
>>>
>>> https://edk2.groups.io/g/devel/message/86597
>>> <https://edk2.groups.io/g/devel/message/86597>
>>>
>> After going through hundreds of edk2 static analysis findings, we found
>> a small number of cases where an interface such as PanicLib was useful
>> and recently added an implementation
>> https://github.com/microsoft/mu_basecore/blob/release/202302/MdePkg/Include/Library/PanicLib.h.
>>
>> For example, the return value from calls to MpInitLibWhoAmI() in
>> exception related code often goes unchecked and it's been used there.
>> Being able to separate the library instance implementation linked to a
>> given module from a more broad library class like DebugLib for these
>> cases has also been helpful.
> 
> Ah, great reminder that we have ANALYZER_UNREACHABLE. I've totally
> forgotten about that; my apologies.
> 
> ... I initially thought that a plain "CONST CHAR8  *Description" was not
> too flexible, but on second thought, it should be exactly right. Reason
> being, it's very easy to print. Format specifiers and variable arguments
> (PrintLib style) may be too complex to implement safely within
> PanicReport(). Arguably, no PanicReport() implementation should be
> obligated (by the interface) to depend on, or to reimplement, PrintLib.
> If the calling context permits, the caller can just use PrintLib to
> format the message to a local buffer (on the stack), and pass that to
> PanicReport.
> 
> So this looks very useful to me; can you upstream it?
> 
Thanks for taking a look. We definitely want to align with edk2 on a 
consistent way to handle these scenarios. We'll send a patch for further 
discussion and review.

We've waited to do so to allow the original author (Ken Lautner) to 
return from his time out of office to send that patch.

- Michael

> Laszlo
> 
> 
>>
>>> Unfortunately, in order to fix some of these static analysis issues
>>> correctly, we are going
>>>
>>> to have to identify the use of ASSERT() that really is a fatal
>>> condition that can not continue
>>>
>>> and introduce an implementation approach that provides a platform
>>> handler and
>>>
>>> satisfies the static analysis tools.
>>>
>>> We also have to evaluate if a return error status with a DEBUG_ERROR
>>> message would be a better
>>>
>>> choice than an ASSERT() that can be filtered out by build options.
>>>
>>> Best regards,
>>>
>>> Mike
>>>
>>> *From:* devel@edk2.groups.io <devel@edk2.groups.io> *On Behalf Of
>>> *Ranbir Singh
>>> *Sent:* Tuesday, November 14, 2023 7:08 AM
>>> *To:* Laszlo Ersek <lersek@redhat.com>
>>> *Cc:* devel@edk2.groups.io; Ni, Ray <ray.ni@intel.com>; Veeresh
>>> Sangolli <veeresh.sangolli@dellteam.com>
>>> *Subject:* Re: [edk2-devel] [PATCH v2 4/5]
>>> MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue
>>>
>>> On Mon, Nov 13, 2023 at 4:58 PM Laszlo Ersek <lersek@redhat.com
>>> <mailto:lersek@redhat.com>> wrote:
>>>
>>>      On 11/10/23 07:11, Ranbir Singh wrote:
>>>       > EFI_NOT_READY was listed as one of the error return values in the
>>>       > function header of StartPciDevices(). So, I considered it here.
>>>       >
>>>       > Maybe we can go by a dual approach, including CpuDeadLoop() in
>>>       > StartPciDevices() as well as add return value check at the call
>>>      site in
>>>       > PciBusDriverBindingStart().
>>>
>>>      I don't think this makes much sense, given that execution is not
>>>      supposed to continue past CpuDeadLoop(), so the new error check would
>>>      not be reached.
>>>
>>>      I think two options are realistic:
>>>
>>>      - replace the assert() with a CpuDeadLoop() -- this is my preference
>>>
>>>      - keep the assert, add the "if", and in the caller, handle the
>>>      EFI_NOT_READY error. This is workable too. (Keeping the assert is
>>> goog
>>>      because it shows that we don't expect the "if" to fire.)
>>>
>>>      Anyway, now that we have no way to verify the change against
>>> Coverity, I
>>>      don't know if this patch is worth the churn. :( As I said, this
>>> ASSERT()
>>>      is one of those few justified ones in edk2 core that can indeed never
>>>      fail, IMO.
>>>
>>>      Laszlo
>>>
>>> See, Does the following change look acceptable to you ?
>>>
>>>       ASSERT (RootBridge != NULL);
>>> +  if (RootBridge == NULL) {
>>>
>>> +    CpuDeadLoop ();
>>> +    return EFI_NOT_READY;
>>> +  }
>>>
>>> +
>>>
>>> which retains the existing assert, adds the NULL pointer check and
>>> includes CpuDeadLoop () within the if block. As a result of
>>> CpuDeadLoop (), the return statement afterwards will never reach
>>> execution (=> no need to handle this return value at the call sites),
>>> but will satisfy static analysis tools as the "RootBridge" dereference
>>> scenario doesn't arise thereafter.
>>>
>>>
>>>       >
>>>       > On Tue, Nov 7, 2023 at 10:18 PM Laszlo Ersek <lersek@redhat.com
>>>      <mailto:lersek@redhat.com>
>>>       > <mailto:lersek@redhat.com <mailto:lersek@redhat.com>>> wrote:
>>>       >
>>>       >     On 11/7/23 07:19, Ranbir Singh wrote:
>>>       >     > From: Ranbir Singh <Ranbir.Singh3@Dell.com
>>>      <mailto:Ranbir.Singh3@Dell.com>>
>>>       >     >
>>>       >     > The function StartPciDevices has a check
>>>       >     >
>>>       >     >     ASSERT (RootBridge != NULL);
>>>       >     >
>>>       >     > but this comes into play only in DEBUG mode. In Release
>>>      mode, there
>>>       >     > is no handling if the RootBridge value is NULL and the code
>>>      proceeds
>>>       >     > to unconditionally dereference "RootBridge" which will lead
>>>      to CRASH.
>>>       >     >
>>>       >     > Hence, for safety add NULL pointer checks always and return
>>>       >     > EFI_NOT_READY if RootBridge value is NULL which is one of
>>>      the return
>>>       >     > values as mentioned in the function description header.
>>>       >     >
>>>       >     > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4239
>>>      <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>
>>>       >     <https://bugzilla.tianocore.org/show_bug.cgi?id=4239
>>>      <https://bugzilla.tianocore.org/show_bug.cgi?id=4239>>
>>>       >     >
>>>       >     > Cc: Ray Ni <ray.ni@intel.com <mailto:ray.ni@intel.com>
>>>      <mailto:ray.ni@intel.com <mailto:ray.ni@intel.com>>>
>>>       >     > Co-authored-by: Veeresh Sangolli
>>>      <veeresh.sangolli@dellteam.com <mailto:veeresh.sangolli@dellteam.com>
>>>       >     <mailto:veeresh.sangolli@dellteam.com
>>>      <mailto:veeresh.sangolli@dellteam.com>>>
>>>       >     > Signed-off-by: Ranbir Singh <Ranbir.Singh3@Dell.com
>>>      <mailto:Ranbir.Singh3@Dell.com>>
>>>       >     > Signed-off-by: Ranbir Singh <rsingh@ventanamicro.com
>>>      <mailto:rsingh@ventanamicro.com>
>>>       >     <mailto:rsingh@ventanamicro.com
>>>      <mailto:rsingh@ventanamicro.com>>>
>>>       >     > ---
>>>       >     >  MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c | 5 ++++-
>>>       >     >  1 file changed, 4 insertions(+), 1 deletion(-)
>>>       >     >
>>>       >     > diff --git
>>> a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>>>       >     b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>>>       >     > index 581e9075ad41..3de80d98370e 100644
>>>       >     > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>>>       >     > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
>>>       >     > @@ -772,7 +772,10 @@ StartPciDevices (
>>>       >     >    LIST_ENTRY     *CurrentLink;
>>>       >     >
>>>       >     >    RootBridge = GetRootBridgeByHandle (Controller);
>>>       >     > -  ASSERT (RootBridge != NULL);
>>>       >     > +  if (RootBridge == NULL) {
>>>       >     > +    return EFI_NOT_READY;
>>>       >     > +  }
>>>       >     > +
>>>       >     >    ThisHostBridge =
>>> RootBridge->PciRootBridgeIo->ParentHandle;
>>>       >     >
>>>       >     >    CurrentLink = mPciDevicePool.ForwardLink;
>>>       >
>>>       >     I don't think this is a good fix.
>>>       >
>>>       >     There is one call site, namely in PciBusDriverBindingStart().
>>>      That call
>>>       >     site does not check the return value. (Of course /s)
>>>       >
>>>       >     I think that this ASSERT() can indeed never fail. Therefore I
>>>      suggest
>>>       >     CpuDeadLoop() instead.
>>>       >
>>>       >     If you insist that CpuDeadLoop() is "too risky" here, then
>>>      the patch is
>>>       >     acceptable, but then the StartPciDevices() call site in
>>>       >     PciBusDriverBindingStart() must check the error properly: we
>>>      must not
>>>       >     install "gEfiPciEnumerationCompleteProtocolGuid", and the
>>>      function must
>>>       >     propagate the error outwards.
>>>       >
>>>       >     Laszlo
>>>       >
>>>
>>>
>>
> 
> 
> 
> 
> 


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111482): https://edk2.groups.io/g/devel/message/111482
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

Re: [edk2-devel] [PATCH v2 4/5] MdeModulePkg/Bus/Pci/PciBusDxe: Fix NULL_RETURNS Coverity issue

[Laszlo Ersek]

On 11/20/23 04:57, Ranbir Singh wrote:
> 
> 
> On Wed, Nov 15, 2023 at 3:20 PM Laszlo Ersek <lersek@redhat.com
> <mailto:lersek@redhat.com>> wrote:
> 
>     On 11/14/23 17:21, Kinney, Michael D wrote:
>     > Hi Ranbir,
>     >
>     >  
>     >
>     > First I want to recognize your efforts to collect Coverity issues and
>     > propose changes to address
>     > them.
>     >
>     > I still disagree with adding CpuDealLoop() for any static analysis
>     issues.
>     >
>     > There have been previous discussions about adding a PANIC() or FATAL()
>     > macro that would
>     > perform platform specific actions if a condition is detected where the
>     > boot of the platform
>     > can not continue.  A platform get to make the choice to log or
>     reboot or
>     > hang, etc.  Not the
>     > code that detected the condition.
>     >
>     > https://edk2.groups.io/g/devel/message/86597
>     <https://edk2.groups.io/g/devel/message/86597>
>     > <https://edk2.groups.io/g/devel/message/86597
>     <https://edk2.groups.io/g/devel/message/86597>>
> 
>     This is indeed a great idea.
> 
>     I didn't know about that discussion. Perhaps a new DebugLib API would
>     handle this (i.e., "panic").
> 
>     I've been certainly proposing CpuDeadLoop() as a means to panic.
> 
>     Did the thread conclude anything tangible?
> 
>     > Unfortunately, in order to fix some of these static analysis issues
>     > correctly, we are going
>     > to have to identify the use of ASSERT() that really is a fatal
>     condition
>     > that can not continue
> 
>     Absolutely.
> 
>     > and introduce an implementation approach that provides a platform
>     > handler and
>     > satisfies the static analysis tools.
> 
>     The "platform handler" is the difficult part. If the above-noted
>     discussion from 2022 didn't produce an agreement, should we really block
>     the static analyzer fixes on an agreed-upon panic API? I'm concerned
>     that would just cause these fixes to get stuck. And I don't consider
>     CpuDeadLoop()s added for this purpose serious technical debt. They are
>     easy to find and update later, assuming we also add comments.
> 
> 
> I agree with the approach to not gate current fixes adding
> CpuDeadLoop(). Later on, it can be updated with the desired panic API
> and I can contribute for those required changes related to patches
> submitted by me.
> 
> I can update current patches to carry additional comment in suffix form
> to ease later search like
>     CpuDeadLoop (); // TBD: replace with Panic API in future
> 
> Laszlo, Mike - Let me know if that works for now.

It works for me.

Of course the risk is always that the proper panic API might never
materialize, and then we'll be stuck with these comments forever as yet
another piece of technical debt. From that perspective minimally, it
would be reasonable for Mike not to accept these reminder comments. (A
reminder BZ, with the commit hash and exact code locations listed, could
be a stronger reminder, but we've seen such BZs too fall through the
cracks over time.)

So, for me, I'm OK; but if Mike doesn't like this approach, I'll
certainly accept that (and then we can't fix the coverity warnings until
the panic API arrives).

Thanks
Laszlo

>  
> 
>     > We also have to evaluate if a return error status with a DEBUG_ERROR
>     > message would be a better
>     > choice than an ASSERT() that can be filtered out by build options.
> 
>     I agree 100% that this would be better for the codebase, but the work
>     needed for this is (IMO) impossible to contain. ASSERT() has been abused
>     for a long time *because* it seemed to allow the programmer to ignore
>     any related error paths. If we now want to implement those error paths
>     retroactively (which would be absolutely the right thing to do from a
>     software engineering perspective), then immense amounts of work are
>     going to be needed (patch review and regression testing), and I think
>     it's just not practical to dump all that on someone that wants to
>     improve the status quo. Replacing an invalid ASSERT() with a panic is
>     honest about the current situation, is safer regarding RELEASE builds,
>     and its work demand (regression testing, patch review) is tolerable.
> 
>     I do agree that, if the error path mostly exists already, then returning
>     errors for data/environment-based error conditions (i.e., not for
>     algorithmic invariant failures) is best.
> 
>     Where we need to be extremely vigilant is new patches. We must
>     uncompromisingly reject *new* abuses of ASSERT(), in new patches.
> 
>     Anyway, it seems that we've been trying to steer Ranbir in opposite
>     directions. I'll let you take the lead on this; for one, I've not been
>     aware of the panic api discussion for 2022!
> 
>     (I don't feel especially pushy about fixing coverity issues, it's just
>     that Ranbir has been contributing such patches, which I've found very
>     welcome, and I wanted to help out with reviews.)
> 
>     Laszlo
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#111587): https://edk2.groups.io/g/devel/message/111587
Mute This Topic: https://groups.io/mt/102438320/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-