On 02/22/16 03:52, Ni, Ruiyu wrote: > OK.OK.JI will generate V3. > > A improper patch can never escape from your eyes.
:) Thank you for making the extra effort! Laszlo > > > > Regards, > > Ray > > > > *From:*edk2-devel [mailto:[email protected]] *On Behalf Of > *Laszlo Ersek > *Sent:* Friday, February 19, 2016 6:41 PM > *To:* Ni, Ruiyu <[email protected]>; [email protected] > *Cc:* Tian, Feng <[email protected]>; Fan, Jeff <[email protected]> > *Subject:* Re: [edk2] [Patch V2 1/2] MdeModulePkg/PciBus: Refine to > reduce code lines. > > > > On 02/19/16 03:18, Ruiyu Ni wrote: >> Contributed-under: TianoCore Contribution Agreement 1.0 >> Signed-off-by: Ruiyu Ni <[email protected] <mailto:[email protected]>> >> Cc: Feng Tian <[email protected] <mailto:[email protected]>> >> Cc: Jeff Fan <[email protected] <mailto:[email protected]>> >> --- >> MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c | 70 >> ++++++++++++---------------------- >> 1 file changed, 24 insertions(+), 46 deletions(-) > > I think this patch is correct. However, it is very hard to read. > > I'd like to propose the following two improvements: > > (1) Please split this patch in two. In the first step, please do not > reorganize the control flow, *only* rename the variable. > > In the second step (a separate patch), the control flow can be > reorganized. > > Both of the patches should point out that they incur no observable > change. > > (2) In the reorganized version, please add > > // > // fall through > // > > comments. Although they are not required by the coding style, I've seen > them elsewhere in edk2, and they are quite helpful. > > Thanks > Laszlo > > >> >> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c >> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c >> index 4160632..50ed866 100644 >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciIo.c >> @@ -1,7 +1,7 @@ >> /** @file >> EFI PCI IO protocol functions implementation for PCI Bus module. >> >> -Copyright (c) 2006 - 2015, Intel Corporation. All rights reserved.<BR> >> +Copyright (c) 2006 - 2016, Intel Corporation. All rights reserved.<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 >> @@ -1774,9 +1774,8 @@ PciIoGetBarAttributes ( >> OUT VOID **Resources OPTIONAL >> ) >> { >> - UINT8 *Configuration; >> PCI_IO_DEVICE *PciIoDevice; >> - EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *AddressSpace; >> + EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor; >> EFI_ACPI_END_TAG_DESCRIPTOR *End; >> >> PciIoDevice = PCI_IO_DEVICE_FROM_PCI_IO_THIS (This); >> @@ -1798,19 +1797,18 @@ PciIoGetBarAttributes ( >> } >> >> if (Resources != NULL) { >> - Configuration = AllocateZeroPool (sizeof >> (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) + sizeof (EFI_ACPI_END_TAG_DESCRIPTOR)); >> - if (Configuration == NULL) { >> + Descriptor = AllocateZeroPool (sizeof >> (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) + sizeof (EFI_ACPI_END_TAG_DESCRIPTOR)); >> + if (Descriptor == NULL) { >> return EFI_OUT_OF_RESOURCES; >> } >> >> - AddressSpace = (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *) Configuration; >> + *Resources = Descriptor; >> >> - AddressSpace->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR; >> - AddressSpace->Len = (UINT16) (sizeof >> (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3); >> - >> - AddressSpace->AddrRangeMin = PciIoDevice->PciBar[BarIndex].BaseAddress; >> - AddressSpace->AddrLen = PciIoDevice->PciBar[BarIndex].Length; >> - AddressSpace->AddrRangeMax = PciIoDevice->PciBar[BarIndex].Alignment; >> + Descriptor->Desc = ACPI_ADDRESS_SPACE_DESCRIPTOR; >> + Descriptor->Len = (UINT16) (sizeof >> (EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR) - 3); >> + Descriptor->AddrRangeMin = PciIoDevice->PciBar[BarIndex].BaseAddress; >> + Descriptor->AddrLen = PciIoDevice->PciBar[BarIndex].Length; >> + Descriptor->AddrRangeMax = PciIoDevice->PciBar[BarIndex].Alignment; >> >> switch (PciIoDevice->PciBar[BarIndex].BarType) { >> case PciBarTypeIo16: >> @@ -1818,59 +1816,41 @@ PciIoGetBarAttributes ( >> // >> // Io >> // >> - AddressSpace->ResType = ACPI_ADDRESS_SPACE_TYPE_IO; >> - break; >> - >> - case PciBarTypeMem32: >> - // >> - // Mem >> - // >> - AddressSpace->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; >> - // >> - // 32 bit >> - // >> - AddressSpace->AddrSpaceGranularity = 32; >> + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_IO; >> break; >> >> case PciBarTypePMem32: >> // >> - // Mem >> - // >> - AddressSpace->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; >> - // >> // prefechable >> // >> - AddressSpace->SpecificFlag = 0x6; >> - // >> - // 32 bit >> - // >> - AddressSpace->AddrSpaceGranularity = 32; >> - break; >> + Descriptor->SpecificFlag = >> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE; >> >> - case PciBarTypeMem64: >> + case PciBarTypeMem32: >> // >> // Mem >> // >> - AddressSpace->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; >> + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; >> // >> - // 64 bit >> + // 32 bit >> // >> - AddressSpace->AddrSpaceGranularity = 64; >> + Descriptor->AddrSpaceGranularity = 32; >> break; >> >> case PciBarTypePMem64: >> // >> - // Mem >> + // prefechable >> // >> - AddressSpace->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; >> + Descriptor->SpecificFlag = >> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE; >> + >> + case PciBarTypeMem64: >> // >> - // prefechable >> + // Mem >> // >> - AddressSpace->SpecificFlag = 0x6; >> + Descriptor->ResType = ACPI_ADDRESS_SPACE_TYPE_MEM; >> // >> // 64 bit >> // >> - AddressSpace->AddrSpaceGranularity = 64; >> + Descriptor->AddrSpaceGranularity = 64; >> break; >> >> default: >> @@ -1880,11 +1860,9 @@ PciIoGetBarAttributes ( >> // >> // put the checksum >> // >> - End = (EFI_ACPI_END_TAG_DESCRIPTOR *) (AddressSpace + 1); >> + End = (EFI_ACPI_END_TAG_DESCRIPTOR *) (Descriptor + 1); >> End->Desc = ACPI_END_TAG_DESCRIPTOR; >> End->Checksum = 0; >> - >> - *Resources = Configuration; >> } >> >> return EFI_SUCCESS; >> > > _______________________________________________ > edk2-devel mailing list > [email protected] <mailto:[email protected]> > https://lists.01.org/mailman/listinfo/edk2-devel > _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
