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 <ler...@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...@intel.com> > > Signed-off-by: Ranbir Singh <rsi...@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/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-