[edk2-devel] [PATCH v1 0/5] BZ 4239: Fix PciBusDxe issues pointed by

[edk2-devel] [PATCH v1 0/5] BZ 4239: Fix PciBusDxe issues pointed by

[Ranbir Singh]

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, 114 insertions(+), 53 deletions(-)

-- 
2.34.1



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

[edk2-devel] [PATCH v1 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: Hao A Wu <hao.a.wu@intel.com>
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 (#109108): https://edk2.groups.io/g/devel/message/109108
Mute This Topic: https://groups.io/mt/101612807/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 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 static analyzer tool. Just adding

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

as explicit comment in between makes a reader as well as Coverity happy.

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

Cc: Hao A Wu <hao.a.wu@intel.com>
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 (#109109): https://edk2.groups.io/g/devel/message/109109
Mute This Topic: https://groups.io/mt/101612808/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 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: Hao A Wu <hao.a.wu@intel.com>
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 (#109110): https://edk2.groups.io/g/devel/message/109110
Mute This Topic: https://groups.io/mt/101612809/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 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: Hao A Wu <hao.a.wu@intel.com>
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, 5 insertions(+)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
index 581e9075ad41..f43f10325f16 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciDeviceSupport.c
@@ -773,6 +773,11 @@ StartPciDevices (
 
   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 (#109111): https://edk2.groups.io/g/devel/message/109111
Mute This Topic: https://groups.io/mt/101612810/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

[edk2-devel] [PATCH v1 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: Hao A Wu <hao.a.wu@intel.com>
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 f43f10325f16..ae770d766381 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 (#109112): https://edk2.groups.io/g/devel/message/109112
Mute This Topic: https://groups.io/mt/101612811/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-