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