Can I get help from someone to commit this change please? -----Original Message----- From: Ni, Ruiyu [mailto:ruiyu...@intel.com] Sent: Wednesday, September 30, 2015 12:35 AM To: Shifflett, Joseph <joseph.shiffl...@hpe.com>; El-Haj-Mahmoud, Samer <samer.el-haj-mahm...@hpe.com>; edk2-devel@lists.01.org Cc: Tian, Feng <feng.t...@intel.com>; El-Haj-Mahmoud, Samer <samer.el-haj-mahm...@hpe.com> Subject: RE: [edk2] [PATCH] MdeModulePkg: PciBusDxe: Properly exit PCI function loops early if the device is not a multifunction device.
Reviewed-by: Ruiyu Ni <ruiyu...@intel.com> -----Original Message----- From: Shifflett, Joseph [mailto:joseph.shiffl...@hpe.com] Sent: Wednesday, September 16, 2015 3:06 AM To: Ni, Ruiyu <ruiyu...@intel.com>; El-Haj-Mahmoud, Samer <samer.el-haj-mahm...@hpe.com>; edk2-devel@lists.01.org Cc: Tian, Feng <feng.t...@intel.com>; El-Haj-Mahmoud, Samer <samer.el-haj-mahm...@hpe.com> Subject: RE: [edk2] [PATCH] MdeModulePkg: PciBusDxe: Properly exit PCI function loops early if the device is not a multifunction device. Ray, we agree with your recommendation and have updated our patch: When looping through all PCI functions, code should not look for functions 1-7 if function 0 is not present or if function 0 indicates the device is not multifunction. Prior to this fix, the code would use stale data in a buffer to determine if a device is multifunction even if function 0 is not present. Contributed-under: TianoCore Contribution Agreement 1.0 Signed-off-by: Samer El-Haj-Mahmoud <samer.el-haj-mahm...@hpe.com> Index: DuetPkg/PciBusNoEnumerationDxe/PciEnumeratorSupport.c =================================================================== --- DuetPkg/PciBusNoEnumerationDxe/PciEnumeratorSupport.c (revision 18475) +++ DuetPkg/PciBusNoEnumerationDxe/PciEnumeratorSupport.c (working copy) @@ -216,6 +216,13 @@ (UINT8) Func ); + if (EFI_ERROR (Status) && Func == 0) { + // + // go to next device if there is no Function 0 + // + break; + } + if (!EFI_ERROR (Status)) { // Index: MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c =================================================================== --- MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c (revision 18475) +++ MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c (working copy) @@ -407,6 +407,13 @@ Func ); + if (EFI_ERROR (Status) && Func == 0) { + // + // go to next device if there is no Function 0 + // + break; + } + if (!EFI_ERROR (Status) && (IS_PCI_BRIDGE (&Pci) || IS_CARDBUS_BRIDGE (&Pci))) { Index: MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c =================================================================== --- MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c (revision 18475) +++ MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c (working copy) @@ -119,6 +119,14 @@ (UINT8) Device, (UINT8) Func ); + + if (EFI_ERROR (Status) && Func == 0) { + // + // go to next device if there is no Function 0 + // + break; + } + if (!EFI_ERROR (Status)) { // @@ -2597,6 +2605,13 @@ Func ); + if (EFI_ERROR (Status) && Func == 0) { + // + // go to next device if there is no Function 0 + // + break; + } + if (!EFI_ERROR (Status) && (IS_PCI_BRIDGE (&Pci))) { Register = 0; Index: MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c =================================================================== --- MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c (revision 18475) +++ MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c (working copy) @@ -1022,6 +1022,13 @@ Func ); + if (EFI_ERROR (Status) && Func == 0) { + // + // go to next device if there is no Function 0 + // + break; + } + if (EFI_ERROR (Status)) { continue; } -----Original Message----- From: Ni, Ruiyu [mailto:ruiyu...@intel.com] Sent: Monday, September 14, 2015 10:23 PM To: El-Haj-Mahmoud, Samer <samer.el-haj-mahm...@hpe.com>; edk2-devel@lists.01.org Cc: Shifflett, Joseph <joseph.shiffl...@hpe.com>; Feng Tian <feng.t...@intel.com>; El-Haj-Mahmoud, Samer <samer.el-haj-mahm...@hpe.com> Subject: Re: [edk2] [PATCH] MdeModulePkg: PciBusDxe: Properly exit PCI function loops early if the device is not a multifunction device. On 2015/9/15 4:19, Samer El-Haj-Mahmoud wrote: > From: "Shifflett, Joseph" <joseph.shiffl...@hpe.com> > > When looping through all PCI functions, code should not look for functions > 1-7 if function 0 is not present or if function 0 indicates the device is not > multifunction. Prior to this fix, the code would use stale data in a buffer > to determine if a device is multifunction even if function 0 is not present. > > Contributed-under: TianoCore Contribution Agreement 1.0 > Signed-off-by: Samer El-Haj-Mahmoud <samer.el-haj-mahm...@hpe.com> > --- > MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c | 10 ++++-- > .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c | 38 > ++++++++++++++-------- > MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c | 11 ++++++- > 3 files changed, 42 insertions(+), 17 deletions(-) > > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c > index 7329143..e638014 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumerator.c > @@ -2,6 +2,7 @@ > PCI eunmeration implementation on entire PCI bus system for PCI Bus > module. > > Copyright (c) 2006 - 2013, Intel Corporation. All rights > reserved.<BR> > +(C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD > License > which accompanies this distribution. The full text of the license > may be found at @@ -382,6 +383,7 @@ PciAssignBusNumber ( > UINT16 Register; > UINT8 Register8; > EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL *PciRootBridgeIo; > + BOOLEAN MultiFunctionDevice; > > PciRootBridgeIo = Bridge->PciRootBridgeIo; > > @@ -394,6 +396,7 @@ PciAssignBusNumber ( > // First check to see whether the parent is ppb > // > for (Device = 0; Device <= PCI_MAX_DEVICE; Device++) { > + MultiFunctionDevice = FALSE; > for (Func = 0; Func <= PCI_MAX_FUNC; Func++) { > > // > @@ -406,7 +409,9 @@ PciAssignBusNumber ( > Device, > Func > ); > - > + if (Func == 0) { > + MultiFunctionDevice = !EFI_ERROR (Status) && IS_PCI_MULTI_FUNC > (&Pci); > + } > if (!EFI_ERROR (Status) && > (IS_PCI_BRIDGE (&Pci) || IS_CARDBUS_BRIDGE (&Pci))) { > > @@ -482,7 +487,8 @@ PciAssignBusNumber ( > > } > > - if (Func == 0 && !IS_PCI_MULTI_FUNC (&Pci)) { > + if (Func == 0 && !MultiFunctionDevice) { > + > > // > // Skip sub functions, this is not a multi function device > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > index f46025e..7b57374 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c > @@ -2,6 +2,7 @@ > PCI emumeration support functions implementation for PCI Bus module. > > Copyright (c) 2006 - 2015, Intel Corporation. All rights > reserved.<BR> > +(C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD > License > which accompanies this distribution. The full text of the license > may be found at @@ -101,12 +102,13 @@ PciPciDeviceInfoCollector ( > UINT8 SecBus; > PCI_IO_DEVICE *PciIoDevice; > EFI_PCI_IO_PROTOCOL *PciIo; > + BOOLEAN MultiFunctionDevice; > > Status = EFI_SUCCESS; > SecBus = 0; > > for (Device = 0; Device <= PCI_MAX_DEVICE; Device++) { > - > + MultiFunctionDevice = FALSE; > for (Func = 0; Func <= PCI_MAX_FUNC; Func++) { > > // > @@ -119,6 +121,9 @@ PciPciDeviceInfoCollector ( > (UINT8) Device, > (UINT8) Func > ); > + if (Func == 0) { > + MultiFunctionDevice = !EFI_ERROR (Status) && IS_PCI_MULTI_FUNC > (&Pci); > + } > if (!EFI_ERROR (Status)) { > > // > @@ -169,17 +174,17 @@ PciPciDeviceInfoCollector ( > ); > > } > - > - if (Func == 0 && !IS_PCI_MULTI_FUNC (&Pci)) { > - > - // > - // Skip sub functions, this is not a multi function device > - // > - Func = PCI_MAX_FUNC; > - } > } > + > + if (Func == 0 && !MultiFunctionDevice) { > > + // > + // Skip sub functions, this is not a multi function device > + // > + Func = PCI_MAX_FUNC; > + } > } > + > } > > return EFI_SUCCESS; > @@ -334,9 +339,9 @@ DumpPciBars ( > > DEBUG (( > EFI_D_INFO, > - " BAR[%d]: Type = %s; Alignment = 0x%lx;\tLength = 0x%lx;\tOffset = > 0x%02x\n", > + " BAR[%d]: Type = %s; Alignment = 0x%lx;\tLength = 0x%lx;\tOffset = > 0x%02x;\tValue = 0x%lx\n", > Index, mBarTypeStr[MIN (PciIoDevice->PciBar[Index].BarType, > PciBarTypeMaxType)], > - PciIoDevice->PciBar[Index].Alignment, > PciIoDevice->PciBar[Index].Length, PciIoDevice->PciBar[Index].Offset > + PciIoDevice->PciBar[Index].Alignment, > + PciIoDevice->PciBar[Index].Length, > + PciIoDevice->PciBar[Index].Offset, > + PciIoDevice->PciBar[Index].BaseAddress > )); > } > > @@ -347,9 +352,9 @@ DumpPciBars ( > > DEBUG (( > EFI_D_INFO, > - " VFBAR[%d]: Type = %s; Alignment = 0x%lx;\tLength = 0x%lx;\tOffset = > 0x%02x\n", > + " VFBAR[%d]: Type = %s; Alignment = 0x%lx;\tLength = > + 0x%lx;\tOffset = 0x%02x;\tValue = 0x%lx\n", > Index, mBarTypeStr[MIN (PciIoDevice->VfPciBar[Index].BarType, > PciBarTypeMaxType)], > - PciIoDevice->VfPciBar[Index].Alignment, > PciIoDevice->VfPciBar[Index].Length, PciIoDevice->VfPciBar[Index].Offset > + PciIoDevice->VfPciBar[Index].Alignment, > + PciIoDevice->VfPciBar[Index].Length, > + PciIoDevice->VfPciBar[Index].Offset, > + PciIoDevice->VfPciBar[Index].BaseAddress > )); > } > DEBUG ((EFI_D_INFO, "\n")); > @@ -2580,10 +2585,12 @@ ResetAllPpbBusNumber ( > UINT64 Address; > UINT8 SecondaryBus; > EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL *PciRootBridgeIo; > + BOOLEAN MultiFunctionDevice; > > PciRootBridgeIo = Bridge->PciRootBridgeIo; > > for (Device = 0; Device <= PCI_MAX_DEVICE; Device++) { > + MultiFunctionDevice = FALSE; > for (Func = 0; Func <= PCI_MAX_FUNC; Func++) { > > // > @@ -2596,6 +2603,9 @@ ResetAllPpbBusNumber ( > Device, > Func > ); > + if (Func == 0) { > + MultiFunctionDevice = !EFI_ERROR (Status) && IS_PCI_MULTI_FUNC > (&Pci); > + } > > if (!EFI_ERROR (Status) && (IS_PCI_BRIDGE (&Pci))) { > > @@ -2627,7 +2637,7 @@ ResetAllPpbBusNumber ( > ); > } > > - if (Func == 0 && !IS_PCI_MULTI_FUNC (&Pci)) { > + if (Func == 0 && !MultiFunctionDevice) { > // > // Skip sub functions, this is not a multi function device > // > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > index b3d91a8..8d386da 100644 > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c > @@ -2,6 +2,7 @@ > Internal library implementation for PCI Bus module. > > Copyright (c) 2006 - 2013, Intel Corporation. All rights > reserved.<BR> > +(C) Copyright 2015 Hewlett Packard Enterprise Development LP<BR> > This program and the accompanying materials > are licensed and made available under the terms and conditions of the BSD > License > which accompanies this distribution. The full text of the license > may be found at @@ -996,6 +997,7 @@ PciScanBus ( > EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL *PciRootBridgeIo; > BOOLEAN BusPadding; > UINT32 TempReservedBusNum; > + BOOLEAN MultiFunctionDevice; > > PciRootBridgeIo = Bridge->PciRootBridgeIo; > SecondBus = 0; > @@ -1008,6 +1010,7 @@ PciScanBus ( > PciAddress = 0; > > for (Device = 0; Device <= PCI_MAX_DEVICE; Device++) { > + MultiFunctionDevice = FALSE; > TempReservedBusNum = 0; > for (Func = 0; Func <= PCI_MAX_FUNC; Func++) { > > @@ -1021,8 +1024,14 @@ PciScanBus ( > Device, > Func > ); > + if (Func == 0) { > + MultiFunctionDevice = !EFI_ERROR (Status) && IS_PCI_MULTI_FUNC > (&Pci); > + } > > if (EFI_ERROR (Status)) { > + if (Func == 0 && !MultiFunctionDevice) { > + Func = PCI_MAX_FUNC; > + } > continue; > } > > @@ -1270,7 +1279,7 @@ PciScanBus ( > } > } > > - if (Func == 0 && !IS_PCI_MULTI_FUNC (&Pci)) { > + if (Func == 0 && !MultiFunctionDevice) { > > // > // Skip sub functions, this is not a multi function device > Samer, How about just adding below check after calling PciDevicePresent()? So you needn't MultiFunctionDevice local variable and the other changes are not needed either. Status = PciDevicePresent ( PciRootBridgeIo, &Pci, StartBusNumber, Device, Func ); + // + // Do not detect other functions if function 0 isn't present + // + if (EFI_ERROR (Status) && Func == 0) { + break; + } Thanks, Ray _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel
