From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by mx.groups.io with SMTP id smtpd.web10.2555.1576583767954126333 for ; Tue, 17 Dec 2019 03:56:08 -0800 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.20, mailfrom: ray.ni@intel.com) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 17 Dec 2019 03:56:07 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.69,325,1571727600"; d="scan'208";a="221744541" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by fmsmga001.fm.intel.com with ESMTP; 17 Dec 2019 03:56:07 -0800 Received: from fmsmsx608.amr.corp.intel.com (10.18.126.88) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 17 Dec 2019 03:56:06 -0800 Received: from fmsmsx608.amr.corp.intel.com (10.18.126.88) by fmsmsx608.amr.corp.intel.com (10.18.126.88) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.1713.5; Tue, 17 Dec 2019 03:56:06 -0800 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by fmsmsx608.amr.corp.intel.com (10.18.126.88) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256) id 15.1.1713.5 via Frontend Transport; Tue, 17 Dec 2019 03:56:06 -0800 Received: from shsmsx104.ccr.corp.intel.com ([169.254.5.90]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.29]) with mapi id 14.03.0439.000; Tue, 17 Dec 2019 19:56:04 +0800 From: "Ni, Ray" To: "Javeed, Ashraf" , "devel@edk2.groups.io" CC: "Wang, Jian J" , "Wu, Hao A" Subject: Re: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 05/12] PciBusDxe: Setup sub-phases for PCI feature enumeration Thread-Topic: [edk2-devel] [edk2-staging/UEFI_PCI_ENHANCE-2 PATCH 05/12] PciBusDxe: Setup sub-phases for PCI feature enumeration Thread-Index: AQHVmdJK3T9Y1rxzHkawV8SqYkiDX6e92zmA Date: Tue, 17 Dec 2019 11:56:04 +0000 Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C3A05B7@SHSMSX104.ccr.corp.intel.com> References: <20191101150952.3340-1-ashraf.javeed@intel.com> <15D3127B934F51D3.12315@groups.io> <95C5C2B113DE604FB208120C742E9824579171C4@BGSMSX101.gar.corp.intel.com> In-Reply-To: <95C5C2B113DE604FB208120C742E9824579171C4@BGSMSX101.gar.corp.intel.com> Accept-Language: en-US, zh-CN X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiYTZiZmZiYmUtNGMyMS00NDY3LWE2MDktYWUyOWZlNmFkYTc4IiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiXC96aGlrOXA2Rm93UnFmM3JxXC95NUdoSmVtZENRQzJRdnkyWUJTVGlaWmpGTVJUalwvZDFqcGprVU1POEVkaWZVZCJ9 x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.2.0.6 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: ray.ni@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Please check comments below. I may have more comments regarding to the four phases after I finish review= of further patches. Besides the comments below, I have a general comments to the debug message:= can you please review the existing debug message in the PciBus driver and make sure your n= ewly added debug message aligns to existing style. And try to use less lines of debug messages with = still enough debug information. > > +PRIMARY_ROOT_PORT_NODE *mPrimaryRootPortList; > > + > > +/** > > + A global pointer to PCI_FEATURE_CONFIGURATION_COMPLETION_LIST, > > which > > +stores all > > + the PCI Root Bridge instances that are enumerated for the other PCI > > +features, > > + like MaxPayloadSize & MaxReadReqSize; during the the Start() > > +interface of the > > + driver binding protocol. The records pointed by this pointer would b= e > > +destroyed > > + when the DXE core invokes the Stop() interface. > > +**/ > > +PCI_FEATURE_CONFIGURATION_COMPLETION_LIST > > *mPciFeaturesConfigurationCompletionList =3D NULL; 1. Please follow existing linked list usage style. The first node in the li= st is an empty header node. LIST_ENTRY mPrimaryRootPortList; LIST_ENTRY mPciFeaturesConfigurationCompletionList; > > +BOOLEAN > > +CheckPciFeatureConfigurationRecordExist ( > > + IN PCI_IO_DEVICE *RootBridge, > > + OUT PCI_FEATURE_CONFIGURATION_COMPLETION_LIST > > +**PciFeatureConfigRecord > > + ) 2. Is this function to check whether the PCIE features under a root bridge is already initialized? Can you use the existing variable gFullEnumeration? The variable is set to TRUE when the enumeration is done to a host bridge i= n the first time. By using gFullEnumeration, the entire function is not needed. > > +EFI_STATUS AddRootBridgeInPciFeaturesConfigCompletionList ( > > + IN PCI_IO_DEVICE *RootBridge, > > + IN BOOLEAN ReEnumerationRequired > > + ) 3. Same question as #2. I think by using gFullEnumeration, this function is= not needed. > > +BOOLEAN > > +IsPciRootPortEmpty ( > > + IN PCI_IO_DEVICE *PciDevice > > + ) 4. Please use IsListEmpty() directly from callers and remove this function. > > +**/ > > +EFI_STATUS > > +EnumerateOtherPciFeatures ( 5. Can it be "EnumeratePcieFeatures"? > > + IN PCI_IO_DEVICE *RootBridge > > + ) > > +{ > > + EFI_STATUS Status; > > + UINTN OtherPciFeatureConfigPhase; > > + > > + // > > + // check on PCI features configuration is complete and re-enumeratio= n > > + is required // if (!CheckPciFeaturesConfigurationRequired > > + (RootBridge)) { > > + return EFI_ALREADY_STARTED; > > + } > > + > > + CHAR16 *Str; > > + Str =3D ConvertDevicePathToText ( > > + DevicePathFromHandle (RootBridge->Handle), > > + FALSE, > > + FALSE > > + ); > > + DEBUG ((DEBUG_INFO, "Enumerating PCI features for Root Bridge %s\n", > > + Str !=3D NULL ? Str : L"")); 6. Please use DEBUG_CODE macro to include ConvertDevicePathToText() and DEB= UG(). Please remember to call FreePool(). > > + > > + for ( OtherPciFeatureConfigPhase =3D PciFeatureRootBridgeScan > > + ; OtherPciFeatureConfigPhase <=3D PciFeatureConfigurationComplet= e > > + ; OtherPciFeatureConfigPhase++ > > + ) { > > + switch (OtherPciFeatureConfigPhase){ > > + case PciFeatureRootBridgeScan: > > + SetupPciFeaturesConfigurationDefaults (); > > + // > > + //first scan the entire root bridge heirarchy for the primary = PCI root > ports > > + // > > + RecordPciRootPortBridges (RootBridge); 7. How about "RecordPciRootPorts (RootBridge)"? The "Bridges" suffix is a b= it confusing. > > + case PciFeatureGetDevicePolicy: > > + case PciFeatureSetupPhase: 8. In SetupPciFeatures(), why do you need to call DeviceExist()? Did you see any case that a device is detected in the beginning of PciBus s= can but is hidden when calling SetupPciFeatures()? 9. In GetPciFeaturesConfigurationTable() when checking whether a PCI device belongs to a root port, we can use below simpler logic: SizeOfPciDevicePath =3D GetDevicePathSize (PciDevicePath); SizeOfRootPortDevicePath =3D GetDevicePathSize (RootPortPath); if ((SizeOfRootPortDevicePath < SizeOfPciDevicePath) && CompareMem (PciDevicePath, RootPortPath, SizeOfRootPortDevicePath - E= ND_DEVICE_PATH_LENGTH) =3D=3D 0)) { // PCI device belongs to the root port. } > > + Status =3D ProgramPciFeatures (RootBridge); 10. ProgramPcieFeatures()? > > + > > + if (Str !=3D NULL) { > > + FreePool (Str); > > + } 11. OK the Str is freed here because Str is needed for other debug messages= inside the function. > > + // > > + // mark this root bridge as PCI features configuration complete, and > > +no new > > + // enumeration is required > > + // > > + AddRootBridgeInPciFeaturesConfigCompletionList (RootBridge, FALSE); 12. Not needed. > > +_PRIMARY_ROOT_PORT_NODE { > > + // > > + // Signature header > > + // > > + UINT32 Signature; > > + // > > + // linked list pointers to next node > > + // > > + LIST_ENTRY NeighborRootPort; > > + // > > + // pointer to PCI_IO_DEVICE of the primary PCI Controller device > > + // > > + EFI_DEVICE_PATH_PROTOCOL *RootPortDevicePath; > > + // > > + // pointer to the corresponding PCI feature configuration Table node > > + // all the child PCI devices of the controller are aligned based on > > +this table > > + // > > + OTHER_PCI_FEATURES_CONFIGURATION_TABLE > > *OtherPciFeaturesConfigurationTable; > > +}; 13. Can you add the OTHER_PCI_FEATURES_CONFIGURATION_TABLE field to PCI_IO_= DEVICE structure? So this structure PRIMARY_ROOT_PORT_NODE is not needed. > > +struct _PCI_FEATURE_CONFIGURATION_COMPLETION_LIST { 14. This structure is not needed if using gFullEnumeration.