Leif suggested to split the patch. -----Original Message----- From: Andrew Fish [mailto:af...@apple.com] Sent: Tuesday, November 3, 2015 10:35 AM To: niru...@users.sourceforge.net Cc: edk2-commits@lists.sourceforge.net Subject: Re: edk2[18717] Revert "MdeModulePkg: Fix a PciBusDxe hot plug bug"
> On Nov 2, 2015, at 6:33 PM, niru...@users.sourceforge.net wrote: > > Revision: 18717 > http://sourceforge.net/p/edk2/code/18717 > Author: niruiyu > Date: 2015-11-03 02:33:05 +0000 (Tue, 03 Nov 2015) > Log Message: > ----------- > Revert "MdeModulePkg: Fix a PciBusDxe hot plug bug" > > This reverts commit 73b7f115c653c807b9d0be97bf516871d8aff7ba. > Why was it reverted? Thanks, Andrew Fish > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Ruiyu Ni <ruiyu...@intel.com> > Reviewed-by: Feng Tian <feng.t...@intel.com> > > Modified Paths: > -------------- > trunk/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > trunk/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h > trunk/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > trunk/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > > Modified: trunk/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > =================================================================== > --- trunk/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > 2015-11-03 02:06:57 UTC (rev 18716) > +++ trunk/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > 2015-11-03 02:33:05 UTC (rev 18717) > @@ -325,81 +325,6 @@ > } > > /** > - Dump the PPB padding resource information. > - > - @param PciIoDevice PCI IO instance. > - @param ResourceType The desired resource type to dump. > - PciBarTypeUnknown means to dump all types of > resources. > -**/ > -VOID > -DumpPpbPaddingResource ( > - IN PCI_IO_DEVICE *PciIoDevice, > - IN PCI_BAR_TYPE ResourceType > - ) > -{ > - EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor; > - PCI_BAR_TYPE Type; > - > - if (PciIoDevice->ResourcePaddingDescriptors == NULL) { > - return; > - } > - > - if (ResourceType == PciBarTypeIo16 || ResourceType == PciBarTypeIo32) { > - ResourceType = PciBarTypeIo; > - } > - > - for (Descriptor = PciIoDevice->ResourcePaddingDescriptors; > Descriptor->Desc != ACPI_END_TAG_DESCRIPTOR; Descriptor++) { > - > - Type = PciBarTypeUnknown; > - if (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR && > Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_IO) { > - Type = PciBarTypeIo; > - } else if (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR && > Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) { > - > - if (Descriptor->AddrSpaceGranularity == 32) { > - // > - // prefechable > - // > - if (Descriptor->SpecificFlag == > EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) { > - Type = PciBarTypePMem32; > - } > - > - // > - // Non-prefechable > - // > - if (Descriptor->SpecificFlag == 0) { > - Type = PciBarTypeMem32; > - } > - } > - > - if (Descriptor->AddrSpaceGranularity == 64) { > - // > - // prefechable > - // > - if (Descriptor->SpecificFlag == > EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) { > - Type = PciBarTypePMem64; > - } > - > - // > - // Non-prefechable > - // > - if (Descriptor->SpecificFlag == 0) { > - Type = PciBarTypeMem64; > - } > - } > - } > - > - if ((Type != PciBarTypeUnknown) && ((ResourceType == PciBarTypeUnknown) > || (ResourceType == Type))) { > - DEBUG (( > - EFI_D_INFO, > - " Padding: Type = %s; Alignment = 0x%lx;\tLength = 0x%lx\n", > - mBarTypeStr[Type], Descriptor->AddrRangeMax, Descriptor->AddrLen > - )); > - } > - } > - > -} > - > -/** > Dump the PCI BAR information. > > @param PciIoDevice PCI IO instance. > @@ -661,10 +586,7 @@ > > GetResourcePaddingPpb (PciIoDevice); > > - DEBUG_CODE ( > - DumpPpbPaddingResource (PciIoDevice, PciBarTypeUnknown); > - DumpPciBars (PciIoDevice); > - ); > + DEBUG_CODE (DumpPciBars (PciIoDevice);); > > return PciIoDevice; > } > > Modified: trunk/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h > =================================================================== > --- trunk/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h > 2015-11-03 02:06:57 UTC (rev 18716) > +++ trunk/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h > 2015-11-03 02:33:05 UTC (rev 18717) > @@ -460,17 +460,4 @@ > IN UINT8 StartBusNumber > ); > > -/** > - Dump the PPB padding resource information. > - > - @param PciIoDevice PCI IO instance. > - @param ResourceType The desired resource type to dump. > - PciBarTypeUnknown means to dump all types of > resources. > -**/ > -VOID > -DumpPpbPaddingResource ( > - IN PCI_IO_DEVICE *PciIoDevice, > - IN PCI_BAR_TYPE ResourceType > - ); > - > #endif > > Modified: trunk/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > =================================================================== > --- trunk/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c 2015-11-03 > 02:06:57 UTC (rev 18716) > +++ trunk/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c 2015-11-03 > 02:33:05 UTC (rev 18717) > @@ -188,21 +188,19 @@ > BridgeResource->PciDev->PciBar[BridgeResource->Bar].BaseAddress, > BridgeResource->Length, BridgeResource->Alignment > )); > - for ( Link = GetFirstNode (&BridgeResource->ChildList) > - ; !IsNull (&BridgeResource->ChildList, Link) > - ; Link = GetNextNode (&BridgeResource->ChildList, Link) > + for ( Link = BridgeResource->ChildList.ForwardLink > + ; Link != &BridgeResource->ChildList > + ; Link = Link->ForwardLink > ) { > Resource = RESOURCE_NODE_FROM_LINK (Link); > if (Resource->ResourceUsage == PciResUsageTypical) { > Bar = Resource->Virtual ? Resource->PciDev->VfPciBar : > Resource->PciDev->PciBar; > DEBUG (( > - EFI_D_INFO, " Base = 0x%lx;\tLength = 0x%lx;\tAlignment = > 0x%lx;\tOwner = %s [%02x|%02x|%02x:", > + EFI_D_INFO, " Base = 0x%lx;\tLength = 0x%lx;\tAlignment = > 0x%lx;\tOwner = %s ", > Bar[Resource->Bar].BaseAddress, Resource->Length, > Resource->Alignment, > IS_PCI_BRIDGE (&Resource->PciDev->Pci) ? L"PPB" : > IS_CARDBUS_BRIDGE (&Resource->PciDev->Pci) ? L"P2C" : > - L"PCI", > - Resource->PciDev->BusNumber, Resource->PciDev->DeviceNumber, > - Resource->PciDev->FunctionNumber > + L"PCI" > )); > > if ((!IS_PCI_BRIDGE (&Resource->PciDev->Pci) && !IS_CARDBUS_BRIDGE > (&Resource->PciDev->Pci)) || > @@ -212,20 +210,24 @@ > // > // The resource requirement comes from the device itself. > // > - DEBUG ((EFI_D_INFO, "%02x]", Bar[Resource->Bar].Offset)); > + DEBUG (( > + EFI_D_INFO, " [%02x|%02x|%02x:%02x]\n", > + Resource->PciDev->BusNumber, Resource->PciDev->DeviceNumber, > + Resource->PciDev->FunctionNumber, Bar[Resource->Bar].Offset > + )); > } else { > // > // The resource requirement comes from the subordinate devices. > // > - DEBUG ((EFI_D_INFO, "**]")); > + DEBUG (( > + EFI_D_INFO, " [%02x|%02x|%02x:**]\n", > + Resource->PciDev->BusNumber, Resource->PciDev->DeviceNumber, > + Resource->PciDev->FunctionNumber > + )); > } > } else { > - DEBUG ((EFI_D_INFO, " Base = Padding;\tLength = 0x%lx;\tAlignment > = 0x%lx", Resource->Length, Resource->Alignment)); > + DEBUG ((EFI_D_INFO, " Padding:Length = 0x%lx;\tAlignment = 0x%lx\n", > Resource->Length, Resource->Alignment)); > } > - if (BridgeResource->ResType != Resource->ResType) { > - DEBUG ((EFI_D_INFO, "; Type = %s", mBarTypeStr[MIN > (Resource->ResType, PciBarTypeMaxType)])); > - } > - DEBUG ((EFI_D_INFO, "\n")); > } > } > } > @@ -233,61 +235,63 @@ > /** > Find the corresponding resource node for the Device in child list of > BridgeResource. > > - @param[in] Device Pointer to PCI_IO_DEVICE. > - @param[in] BridgeResource Pointer to PCI_RESOURCE_NODE. > - @param[out] DeviceResources Pointer to a buffer to receive resources for > the Device. > + @param[in] Device Pointer to PCI_IO_DEVICE. > + @param[in] BridgeResource Pointer to PCI_RESOURCE_NODE. > > - @return Count of the resource descriptors returned. > + @return !NULL The corresponding resource node for the Device. > + @return NULL No corresponding resource node for the Device. > **/ > -UINTN > +PCI_RESOURCE_NODE * > FindResourceNode ( > - IN PCI_IO_DEVICE *Device, > - IN PCI_RESOURCE_NODE *BridgeResource, > - OUT PCI_RESOURCE_NODE **DeviceResources OPTIONAL > + IN PCI_IO_DEVICE *Device, > + IN PCI_RESOURCE_NODE *BridgeResource > ) > { > LIST_ENTRY *Link; > PCI_RESOURCE_NODE *Resource; > - UINTN Count; > > - Count = 0; > for ( Link = BridgeResource->ChildList.ForwardLink > ; Link != &BridgeResource->ChildList > ; Link = Link->ForwardLink > ) { > Resource = RESOURCE_NODE_FROM_LINK (Link); > if (Resource->PciDev == Device) { > - if (DeviceResources != NULL) { > - DeviceResources[Count] = Resource; > - } > - Count++; > + return Resource; > } > } > > - return Count; > + return NULL; > } > > /** > Dump the resource map of all the devices under Bridge. > > - @param[in] Bridge Bridge device instance. > - @param[in] Resources Resource descriptors for the bridge device. > - @param[in] ResourceCount Count of resource descriptors. > + @param[in] Bridge Bridge device instance. > + @param[in] IoNode IO resource descriptor for the bridge device. > + @param[in] Mem32Node Mem32 resource descriptor for the bridge device. > + @param[in] PMem32Node PMem32 resource descriptor for the bridge device. > + @param[in] Mem64Node Mem64 resource descriptor for the bridge device. > + @param[in] PMem64Node PMem64 resource descriptor for the bridge device. > **/ > VOID > DumpResourceMap ( > IN PCI_IO_DEVICE *Bridge, > - IN PCI_RESOURCE_NODE **Resources, > - IN UINTN ResourceCount > + IN PCI_RESOURCE_NODE *IoNode, > + IN PCI_RESOURCE_NODE *Mem32Node, > + IN PCI_RESOURCE_NODE *PMem32Node, > + IN PCI_RESOURCE_NODE *Mem64Node, > + IN PCI_RESOURCE_NODE *PMem64Node > ) > { > - EFI_STATUS Status; > - LIST_ENTRY *Link; > - PCI_IO_DEVICE *Device; > - UINTN Index; > - CHAR16 *Str; > - PCI_RESOURCE_NODE **ChildResources; > - UINTN ChildResourceCount; > + EFI_STATUS Status; > + LIST_ENTRY *Link; > + PCI_IO_DEVICE *Device; > + PCI_RESOURCE_NODE *ChildIoNode; > + PCI_RESOURCE_NODE *ChildMem32Node; > + PCI_RESOURCE_NODE *ChildPMem32Node; > + PCI_RESOURCE_NODE *ChildMem64Node; > + PCI_RESOURCE_NODE *ChildPMem64Node; > + CHAR16 *Str; > > DEBUG ((EFI_D_INFO, "PciBus: Resource Map for ")); > > @@ -316,9 +320,11 @@ > } > } > > - for (Index = 0; Index < ResourceCount; Index++) { > - DumpBridgeResource (Resources[Index]); > - } > + DumpBridgeResource (IoNode); > + DumpBridgeResource (Mem32Node); > + DumpBridgeResource (PMem32Node); > + DumpBridgeResource (Mem64Node); > + DumpBridgeResource (PMem64Node); > DEBUG ((EFI_D_INFO, "\n")); > > for ( Link = Bridge->ChildList.ForwardLink > @@ -328,19 +334,20 @@ > Device = PCI_IO_DEVICE_FROM_LINK (Link); > if (IS_PCI_BRIDGE (&Device->Pci)) { > > - ChildResourceCount = 0; > - for (Index = 0; Index < ResourceCount; Index++) { > - ChildResourceCount += FindResourceNode (Device, Resources[Index], > NULL); > - } > - ChildResources = AllocatePool (sizeof (PCI_RESOURCE_NODE *) * > ChildResourceCount); > - ASSERT (ChildResources != NULL); > - ChildResourceCount = 0; > - for (Index = 0; Index < ResourceCount; Index++) { > - ChildResourceCount += FindResourceNode (Device, Resources[Index], > &ChildResources[ChildResourceCount]); > - } > + ChildIoNode = (IoNode == NULL ? NULL : FindResourceNode > (Device, IoNode)); > + ChildMem32Node = (Mem32Node == NULL ? NULL : FindResourceNode > (Device, Mem32Node)); > + ChildPMem32Node = (PMem32Node == NULL ? NULL : FindResourceNode > (Device, PMem32Node)); > + ChildMem64Node = (Mem64Node == NULL ? NULL : FindResourceNode > (Device, Mem64Node)); > + ChildPMem64Node = (PMem64Node == NULL ? NULL : FindResourceNode > (Device, PMem64Node)); > > - DumpResourceMap (Device, ChildResources, ChildResourceCount); > - FreePool (ChildResources); > + DumpResourceMap ( > + Device, > + ChildIoNode, > + ChildMem32Node, > + ChildPMem32Node, > + ChildMem64Node, > + ChildPMem64Node > + ); > } > } > } > @@ -800,11 +807,11 @@ > // 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); > + IoBridge = FindResourceNode (RootBridgeDev, &IoPool); > + Mem32Bridge = FindResourceNode (RootBridgeDev, &Mem32Pool); > + PMem32Bridge = FindResourceNode (RootBridgeDev, &PMem32Pool); > + Mem64Bridge = FindResourceNode (RootBridgeDev, &Mem64Pool); > + PMem64Bridge = FindResourceNode (RootBridgeDev, &PMem64Pool); > > ASSERT (IoBridge != NULL); > ASSERT (Mem32Bridge != NULL); > @@ -862,13 +869,14 @@ > // Dump the resource map for current root bridge > // > DEBUG_CODE ( > - PCI_RESOURCE_NODE *Resources[5]; > - Resources[0] = IoBridge; > - Resources[1] = Mem32Bridge; > - Resources[2] = PMem32Bridge; > - Resources[3] = Mem64Bridge; > - Resources[4] = PMem64Bridge; > - DumpResourceMap (RootBridgeDev, Resources, sizeof (Resources) / sizeof > (Resources[0])); > + DumpResourceMap ( > + RootBridgeDev, > + IoBridge, > + Mem32Bridge, > + PMem32Bridge, > + Mem64Bridge, > + PMem64Bridge > + ); > ); > > FreePool (AcpiConfig); > @@ -976,8 +984,7 @@ > UINT8 Device; > UINT8 Func; > UINT64 Address; > - UINT8 SecondBus; > - UINT8 PaddedSubBus; > + UINTN SecondBus; > UINT16 Register; > UINTN HpIndex; > PCI_IO_DEVICE *PciDevice; > @@ -1211,7 +1218,7 @@ > > Status = PciScanBus ( > PciDevice, > - SecondBus, > + (UINT8) (SecondBus), > SubBusNumber, > PaddedBusRange > ); > @@ -1227,16 +1234,12 @@ > if ((Attributes == EfiPaddingPciRootBridge) && > (State & EFI_HPC_STATE_ENABLED) != 0 && > (State & EFI_HPC_STATE_INITIALIZED) != 0) { > - *PaddedBusRange = (UINT8) ((UINT8) (BusRange) + *PaddedBusRange); > + *PaddedBusRange = (UINT8) ((UINT8) (BusRange) +*PaddedBusRange); > } else { > - // > - // Reserve the larger one between the actual occupied bus number > and padded bus number > - // > - Status = PciAllocateBusNumber (PciDevice, StartBusNumber, > (UINT8) (BusRange), &PaddedSubBus); > + Status = PciAllocateBusNumber (PciDevice, *SubBusNumber, (UINT8) > (BusRange), SubBusNumber); > if (EFI_ERROR (Status)) { > return Status; > } > - *SubBusNumber = MAX (PaddedSubBus, *SubBusNumber); > } > } > > > Modified: trunk/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > =================================================================== > --- trunk/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > 2015-11-03 02:06:57 UTC (rev 18716) > +++ trunk/edk2/MdeModulePkg/Bus/Pci/PciBusDxe/PciResourceSupport.c > 2015-11-03 02:33:05 UTC (rev 18717) > @@ -196,7 +196,6 @@ > PCI_RESOURCE_NODE *Node; > UINT64 Offset; > EFI_PCI_PLATFORM_POLICY PciPolicy; > - UINT64 PaddingAperture; > > if (!mPolicyDetermined) { > // > @@ -229,27 +228,21 @@ > mPolicyDetermined = TRUE; > } > > - Aperture = 0; > - PaddingAperture = 0; > + Aperture = 0; > > if (Bridge == NULL) { > return ; > } > > + CurrentLink = Bridge->ChildList.ForwardLink; > + > // > // Assume the bridge is aligned > // > - for ( CurrentLink = GetFirstNode (&Bridge->ChildList) > - ; !IsNull (&Bridge->ChildList, CurrentLink) > - ; CurrentLink = GetNextNode (&Bridge->ChildList, CurrentLink) > - ) { > + while (CurrentLink != &Bridge->ChildList) { > > Node = RESOURCE_NODE_FROM_LINK (CurrentLink); > - if (Node->ResourceUsage == PciResUsagePadding) { > - ASSERT (PaddingAperture == 0); > - PaddingAperture = Node->Length; > - continue; > - } > + > // > // Consider the aperture alignment > // > @@ -300,10 +293,13 @@ > // Increment aperture by the length of node > // > Aperture += Node->Length; > + > + CurrentLink = CurrentLink->ForwardLink; > } > > // > - // Adjust the aperture with the bridge's alignment > + // At last, adjust the aperture with the bridge's > + // alignment > // > Offset = Aperture & (Bridge->Alignment); > > @@ -323,12 +319,6 @@ > Bridge->Alignment = Node->Alignment; > } > } > - > - // > - // Hotplug controller needs padding resources. > - // Use the larger one between the padding resource and actual occupied > resource. > - // > - Bridge->Length = MAX (Bridge->Length, PaddingAperture); > } > > /** > @@ -346,11 +336,10 @@ > UINT64 Aperture; > LIST_ENTRY *CurrentLink; > PCI_RESOURCE_NODE *Node; > - UINT64 PaddingAperture; > + > UINT64 Offset; > > - Aperture = 0; > - PaddingAperture = 0; > + Aperture = 0; > > if (Bridge == NULL) { > return ; > @@ -362,20 +351,14 @@ > return ; > } > > + CurrentLink = Bridge->ChildList.ForwardLink; > + > // > // Assume the bridge is aligned > // > - for ( CurrentLink = GetFirstNode (&Bridge->ChildList) > - ; !IsNull (&Bridge->ChildList, CurrentLink) > - ; CurrentLink = GetNextNode (&Bridge->ChildList, CurrentLink) > - ) { > + while (CurrentLink != &Bridge->ChildList) { > > Node = RESOURCE_NODE_FROM_LINK (CurrentLink); > - if (Node->ResourceUsage == PciResUsagePadding) { > - ASSERT (PaddingAperture == 0); > - PaddingAperture = Node->Length; > - continue; > - } > > // > // Apply padding resource if available > @@ -398,6 +381,11 @@ > // Increment aperture by the length of node > // > Aperture += Node->Length; > + > + // > + // Consider the aperture alignment > + // > + CurrentLink = CurrentLink->ForwardLink; > } > > // > @@ -419,7 +407,7 @@ > } > > // > - // Adjust the bridge's alignment to the first child's alignment > + // At last, adjust the bridge's alignment to the first child's alignment > // if the bridge has at least one child > // > CurrentLink = Bridge->ChildList.ForwardLink; > @@ -429,12 +417,6 @@ > Bridge->Alignment = Node->Alignment; > } > } > - > - // > - // Hotplug controller needs padding resources. > - // Use the larger one between the padding resource and actual occupied > resource. > - // > - Bridge->Length = MAX (Bridge->Length, PaddingAperture); > } > > /** > > > ------------------------------------------------------------------------------ > _______________________________________________ > edk2-commits mailing list > edk2-commits@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/edk2-commits ------------------------------------------------------------------------------ _______________________________________________ edk2-commits mailing list edk2-commits@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-commits ------------------------------------------------------------------------------ _______________________________________________ edk2-commits mailing list edk2-commits@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/edk2-commits
