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

["Ranbir Singh" <rsingh@ventanamicro.com>]

[-- 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 --]