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.11191.1673054235774953471 for ; Fri, 06 Jan 2023 17:17:16 -0800 Authentication-Results: mx.groups.io; dkim=fail reason="unable to parse pub key" header.i=@intel.com header.s=intel header.b=IOIrYk7r; spf=pass (domain: intel.com, ip: 134.134.136.20, mailfrom: zachary.clark-williams@intel.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1673054235; x=1704590235; h=from:to:cc:subject:date:message-id:references: in-reply-to:mime-version; bh=m2FJL+BapxrjqgHg7DTcfyH8FslsSXpaVUooxd5MN5Y=; b=IOIrYk7rNles+vpq3ak1pHNb+IN6ZvphWcrAcWuy/CC3VHf/tOLrTPaG /8dKG4eEt/ahIoC9+LycKy5+hAvx6s/65EH1P8BwGUVV2D/ts11WXA1Th 49q7/JAF2ONf2i2BKBv7D7CyTeCVPVZtqhoSeBc6XmBUl6vj0b8jwg/i8 vpfQc5TWNfy4BZ/Vt6be+D+Rl9oh0D8sHrsEnNPKxyOGmDItxE/tCbS1k hxxz2hnNaYkjLC9DhMQuQLL1BWhn4HFukZmGW5RFPbY3UiG2FXjltMZ9C KGUsq+7YP5x2siz+FkY2wpu3jIozmur7yOvYTenMnUnoojUezHCN9jnBW A==; X-IronPort-AV: E=McAfee;i="6500,9779,10582"; a="310391359" X-IronPort-AV: E=Sophos;i="5.96,307,1665471600"; d="scan'208,217";a="310391359" Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga101.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 06 Jan 2023 17:17:14 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=McAfee;i="6500,9779,10582"; a="763693748" X-IronPort-AV: E=Sophos;i="5.96,307,1665471600"; d="scan'208,217";a="763693748" Received: from fmsmsx601.amr.corp.intel.com ([10.18.126.81]) by fmsmga002.fm.intel.com with ESMTP; 06 Jan 2023 17:17:14 -0800 Received: from fmsmsx610.amr.corp.intel.com (10.18.126.90) by fmsmsx601.amr.corp.intel.com (10.18.126.81) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.16; Fri, 6 Jan 2023 17:17:14 -0800 Received: from fmsedg602.ED.cps.intel.com (10.1.192.136) by fmsmsx610.amr.corp.intel.com (10.18.126.90) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256) id 15.1.2507.16 via Frontend Transport; Fri, 6 Jan 2023 17:17:14 -0800 Received: from NAM11-CO1-obe.outbound.protection.outlook.com (104.47.56.173) by edgegateway.intel.com (192.55.55.71) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.1.2507.16; Fri, 6 Jan 2023 17:17:13 -0800 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=HD1xeyuV4nFT8RLGi5E9okNG07nqIbzsRDE7IEnJ/fgzSdsk0Dn/3D7dSVZLXQER+XngYDqO0UlXJR9NU4bqVl16EcQ2K8NAX3xAmVCxMWkIXbZ3DZmBmLwwH3Fi87Yg33Eyqd5UeMtKJzxacWUdJ5YgP0wi0JabraC8p+JYTyCWrRHFlsiDMjEhvy8DCk98dqGC12yJEEviGU7F16ehOPqH+GgVIB7uchtRIIZExHL+rph5MNs/YrGsuhawhgX2b1DzznNeKc//FgzNqO8MS/4OfsNGU5SuOWi5S8Lc9WngN7MfkuwkNDAOu2VqvjX78I26kfwCmETw54NqmvyKiQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=TTXuqI+/3NZmlFU2K0AYOhuR/gjsDnfjxTYJkxLMcDU=; b=OjET17Jahm3Xsgcf14hinit383j+UtrRvGK7qd8gVhvCUAXzSylt0pi26AJiJ2Owa7dWD2jfsqZ6A4k12S+7X7OZ/cANsRj3cafkmdQtVKwVxWJSQ/Yt17XBDLZSDqGpTbhbZ8IY2iF/WWGYXXAg/AdYKS86j0ACd/w9jjKJ0Ow+ohi9DDOwApIprNnysDZjj1HlVYAHEy8qPXiCPchBi17PgX4L+eaq1jf5frhyTt8VgC6iXm7UwDBc5ZR26umBbHQF3EXyaDy+/8dSUiEk5T0oCZDlMNAaAcsE9X8msfaVDX3Nd5CR60K55tFXZ90bFgBWmweAd4DVgBVT6eVl9g== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=intel.com; dmarc=pass action=none header.from=intel.com; dkim=pass header.d=intel.com; arc=none Received: from CO6PR11MB5601.namprd11.prod.outlook.com (2603:10b6:303:13d::7) by PH0PR11MB5143.namprd11.prod.outlook.com (2603:10b6:510:3f::15) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.5944.19; Sat, 7 Jan 2023 01:17:12 +0000 Received: from CO6PR11MB5601.namprd11.prod.outlook.com ([fe80::5add:c261:a682:569e]) by CO6PR11MB5601.namprd11.prod.outlook.com ([fe80::5add:c261:a682:569e%3]) with mapi id 15.20.5944.019; Sat, 7 Jan 2023 01:17:12 +0000 From: "Clark-williams, Zachary" To: "Wu, Jiaxin" , "devel@edk2.groups.io" , "Kinney, Michael D" CC: Zachary Clark-Williams , Maciej Rabeda , "Otcheretianski, Andrei" , "Zeng, Star" Subject: Re: [PATCH] NetworkPkg: Add WiFi profile sync protocol support Thread-Topic: [PATCH] NetworkPkg: Add WiFi profile sync protocol support Thread-Index: AQHY+FSjbHnMk1AnoEG48qLgyxhznK6RKXEAgADrtaA= Date: Sat, 7 Jan 2023 01:17:11 +0000 Message-ID: References: <20221114181152.2540-1-zachary.clark-williams@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: dkim=none (message not signed) header.d=none;dmarc=none action=none header.from=intel.com; x-ms-publictraffictype: Email x-ms-traffictypediagnostic: CO6PR11MB5601:EE_|PH0PR11MB5143:EE_ x-ms-office365-filtering-correlation-id: 2bb07520-ba51-439c-6143-08daf04ce796 x-ms-exchange-senderadcheck: 1 x-ms-exchange-antispam-relay: 0 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: 8EG3R1zEFoQ3PaoSTryve15EF7JGYylRs7Q2dB65x9Z8aff1SHLGf3rm8N+mp8Y61U833cRNhW63ujTs6NsYQCjoMhlviAXiUJp6YLPP03sFfui9EfWf7XeY1ic5RLlMMfTP1GnoNgzHcURSVmELQo5FLyTSj459BjYajx3aGJ5zVQ/K5GYkYVPB3L+U451g139aOIHayR28ecDI/fF7UnAUHezlTSwCjo8tgReO89OShUgXYYOP+8FAVH8P04FvzXfhlCOfUEFxxJ5GpPMrGH9VndhMwjn4gVHG2HwN/DfDbiP3H4Ao9P/0j1HtAMAQO6H4scq/xhkeWHQIPVthd/uzbJsEIaSRgTl92mGpfAuy9A3NO6wwqaWrF3rZQQyobbeP+BwZAPM/lNwkCziJ7CKaj6Nw67RO5go6haJipalRZKIWhhn0PJhRtvDlEaPGkvg3GziBFFUA2ApFPWAE/cpni82i4uJe2z31q7TyYE1EjjQXnKRCv98P/fUU5JifGKDN3fVm59TnDgASAsPEjIwZ2BqYHY39gJuyjGXTOMEwpC8dBIblfEm6rKgroQ4IlOq+vvE8+hcHkOdQFr3yhDlCoMS3pg42AuSi3UmoaTvIQJtG/SkSIOEVEaMhlxvRFRSaiMLDCpEbmm0l+TUAHOVIV09IsNO6NZBqW3d1QJm9wNks9ak4KN2WaL4fWJBZE5O9Z4aothKsZNFHMoSSvlZxrryz2sg8P/a3m5CiTD0= x-forefront-antispam-report: CIP:255.255.255.255;CTRY:;LANG:en;SCL:1;SRV:;IPV:NLI;SFV:NSPM;H:CO6PR11MB5601.namprd11.prod.outlook.com;PTR:;CAT:NONE;SFS:(13230022)(6029001)(346002)(39860400002)(136003)(366004)(396003)(376002)(451199015)(33656002)(5660300002)(41300700001)(38100700002)(122000001)(8936002)(52536014)(83380400001)(38070700005)(86362001)(71200400001)(66946007)(110136005)(54906003)(82960400001)(66556008)(76116006)(2906002)(6636002)(478600001)(8676002)(7696005)(55016003)(66476007)(64756008)(26005)(186003)(53546011)(66446008)(9686003)(6506007)(4326008)(316002)(44824005);DIR:OUT;SFP:1102; x-ms-exchange-antispam-messagedata-chunkcount: 1 x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?QAKQYjeyv4a0tddqyB2cGWtXQyPwa9Ek87CgpJQbupmpmgHz0QKfYL9maMPX?= =?us-ascii?Q?Rtd4UqjN3i/Ct4S6RbxYRoShLblTdvK/YqA2L8pAPeeUj9tOiuqaoJCFXqX0?= =?us-ascii?Q?Non2JB11Jcry5c8VSxbPwXa068bkCO6CsLgWOjiDxwJGxWD0AzDaCFbxTjqH?= =?us-ascii?Q?bDkwGE2OrejYYyIuX4qcnifPolAI3bKCjkUULH9sSIpWGdDA6guCos/VhC+Q?= =?us-ascii?Q?L5VBUe7nrECvQLojJR3dLLe6y5MyUtAFQ2MRPRZ6B0Ik0lA8hADbktzQcNKH?= =?us-ascii?Q?GIjFaJfb4Mzx0dHoqpVWi5bEAmAQynixAtJW5mBCv22yknkBzSH71oeqsK1v?= =?us-ascii?Q?RuQEuLm04UTcZm/ceFFGqvVQidcuV2pR2/85SuUd97qNUBU0bL/ay67LpHBu?= =?us-ascii?Q?c4BEYHPB/ihRdmM55uTR/kgZYPVbRftQ1ebEgSL/veAQutfO6NbjQAqQQpDM?= =?us-ascii?Q?vAyjnu3eExQQP8W9TnttoBlCtagDTQQQw9reX9ziiltZyEZzne3x8ksNBxQQ?= =?us-ascii?Q?any/IgQ+mgs/cKCi/I1KNm5umS93M14TiA0YOAHOM9yQuUtKhhIKZD9I75cG?= =?us-ascii?Q?yXTQpRxBXJptnkCz1x171b2PW/3dt++XOoix1eExm1S9Y5y7mkY6JmK8FVF2?= =?us-ascii?Q?FOMaUmSyNeoslR+zt18M8ZHQ2v5prtmwkvIRuVvNhKrBgG4adijAUPPvm5we?= =?us-ascii?Q?teW18GY1zeczgYr1X0gG5FpGt7JGS8W5XLLn65NlPrct/KCW6/zP8ToZMXSq?= =?us-ascii?Q?kBcRPVzV0qfL+6wD/GO8ao+JfaUZVRqG1IOY4QcWUsW+hKIHJugffY17t2M7?= =?us-ascii?Q?4XICqCrTvp5QWvIbnILIscizD8WODG+aolV2CxzcFbEvU+Z5K5LTd4oS8tZA?= =?us-ascii?Q?QAxUxJYn0J/ulF3z8iES+mbxOA9EM7HHMZ63uNpoE4opCK123+CCQEizoeqh?= =?us-ascii?Q?6Y1tWryLcrCMftEThF1I47V86h4PFZCrpsW2F4uViKxWZ5M4YmZVueWq4rgY?= =?us-ascii?Q?lDSLfK89ahD00QZQ5JvUsQzOcQn8YFm9mDbKSl6ttAA/ke8v2n4HP2GEKPXF?= =?us-ascii?Q?o9A1Adn18CER1WRH+h/25bBL5+MdMi4gMDbRGPszx4CrNN69GxdPf2IK0PGM?= =?us-ascii?Q?8tbpnwm1iV7/4RMOMSnEyozQc74h74bhvC0gKxSOmg3chYF0MS1CrgvqrvZa?= =?us-ascii?Q?gaENmt7wIlMt/68m/8yZ7PjZdYo1H1ejznirRTH3x07ufnasoLRFZdxS7G7a?= =?us-ascii?Q?Yb6VRgmTEupurwpNxf7N5vWbQP3SGDcemGSdaiAnChEVvxLKn/70rDx6R9jj?= =?us-ascii?Q?4K/Nm+OWUIGTYXuo4eBQHpHJkGv+seMV6HQ+u65nuv7DdR5i5sxjs39qHv48?= =?us-ascii?Q?y6hQvQXap3IsX3pkvMFa3syop8ta/7jTrEVbX4wkAZOq0oYK2ph5XIe0I5cz?= =?us-ascii?Q?m5pvqvl6r2yb+Ffeb3OpeeM2k8yjiv29RsghZbgEniABolM18KlvudNS+ruU?= =?us-ascii?Q?0aY15gsYMcpxI9s2mCaf60FrxUbEfntHwpL+6DtiQjpsy4ZCUREgW37RliwH?= =?us-ascii?Q?V8GSuHSI8rXDLF9xfiizdfMrBNyLeL0rNKwJfbzU9qjpZVXytC/HIZTtZI5a?= =?us-ascii?Q?AA=3D=3D?= MIME-Version: 1.0 X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-AuthSource: CO6PR11MB5601.namprd11.prod.outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 2bb07520-ba51-439c-6143-08daf04ce796 X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Jan 2023 01:17:11.9450 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: N0UfFjL593b64x2n+Uwiu+l+WjFsauTnxvqhX5tRNdj1eNaRLSc9I6RlmOW5Nmst3Pgo9xm774nn5hX4fACEGYC2BDKnNhrPOyZrRZoOOaqIuqyZVMuDv6zwtSxew9kF X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB5143 Return-Path: zachary.clark-williams@intel.com X-OriginatorOrg: intel.com Content-Language: en-US Content-Type: multipart/alternative; boundary="_000_CO6PR11MB56019FBBC4A90EF230F16047C9F89CO6PR11MB5601namp_" --_000_CO6PR11MB56019FBBC4A90EF230F16047C9F89CO6PR11MB5601namp_ Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Thank you Jiaxin, Find my responses inline. An updated patch with requested changes is in route today. Thanks, Zack -----Original Message----- From: Wu, Jiaxin Sent: Thursday, January 5, 2023 9:09 PM To: Clark-williams, Zachary ; devel@edk2.= groups.io; Kinney, Michael D Cc: Zachary Clark-Williams ; Maciej Rabeda ; Otcheretianski, Andrei ; Zeng, Star Subject: RE: [PATCH] NetworkPkg: Add WiFi profile sync protocol support Hi Zachary, Insert all my comments as below. Besides: where defined this protocol (EFI_WIFI_PROFILE_SYNC_PROTOCOL)? I di= dn't find in the UEFI spec, in such a case, could we named it as EDKII_WIFI= _PROFILE_SYNC_PROTOCOL? please add more description about the protocol usag= e. - Yes, Will update naming convention Thanks, Jiaxin > +/** > + Used by the WiFi connection manager to get the WiFi profile that > +AMT > shared > + and was stored in WiFi profile protocol. Aligns the AMT WiFi > + profile data to the WiFi connection manager profile structure fo conne= ction use. > + > + @param[in, out] WcmProfile WiFi Connection Manager profile > structure > + @param[in, out] MacAddress MAC address from AMT saved to NiC > MAC address > + > + @retval EFI_SUCCESS Stored WiFi profile converted and re= turned > succefully > + @retval EFI_UNSUPPORTED Profile protocol sharing not support= ed or > enabled > + @retval EFI_NOT_FOUND No profiles to returned > + @retval Others Error Occurred > +**/ > +typedef > +EFI_STATUS > +(EFIAPI *WIFI_PROFILE_GET)( > + IN OUT WIFI_MGR_NETWORK_PROFILE *Profile, > + IN OUT EFI_80211_MAC_ADDRESS MacAddress > + ); Does it mean the returned Profile is only for the returned MacAddress? Does= it must 1:1 mapping?? * No, this is an Input and Output because we are both fulfilling the Wi= Fi profile data, converting it from the AMT structure to the WifiConnection= Manager structure. The MAC address is stored in a separate place In the Nic= than the profile data so we pass both locations so we don't need to send t= he full Nic just these 2 data structure locations. This is necessary as we need consistency in the MAC from AMT to correspond= with the Profile data. Think more, Does AMT support maintain multiple mappings? Image we have mult= iple network socket, how AMT handle such case? * At this point the AMT and CSME doe not support multiple NiC's, or WiF= i cards for this process. There will be one NiC, 1 Card and 1 Profile used = to keep secrets secure. > + > +/** > + Saves the WiFi connection status recieved by the > +WiFiConnectionManager > when > + in a KVM OR One Click Recovery WLAN recovery flow. Input as > + EFI_80211_CONNECT_NETWORK_RESULT_CODE then converted and > stored as EFI_STATUS type. > + Why need stored as EFI_STATUS type since we have defined the EFI_80211_CONN= ECT_NETWORK_RESULT_CODE??? * This is necessary to decouple the code for more modularity, otherwise= we would have cross-repo dependencies and very very ugly mess of code. Thi= s conversion is simpler and cleaner. > + @param[in] ConnectionStatus WiFi connection attempt results > +**/ > +typedef > +VOID > +(EFIAPI *WIFI_SET_CONNECT_STATE)( > + IN EFI_80211_CONNECT_NETWORK_RESULT_CODE ConnectionStatus > + ); > + > +/** > + Retrieves the stored WiFi connection status when in either KVM OR > +One > Click > + Recovery WLAN recovery flow. > + > + @retval EFI_SUCCESS WiFi connection completed succesfull= y > + @retval Others Connection failure occurred > +**/ > +typedef > +EFI_STATUS > +(EFIAPI *WIFI_GET_CONNECT_STATE)( > + VOID > + ); What's the output? Only EFI_STATUS? why not return the EFI_80211_CONNECT_NE= TWORK_RESULT_CODE? We should not mix the EFI_80211_CONNECT_NETWORK_RESULT_C= ODE & EFI_STATUS. * See answer to previous > + > +// > +// WiFi Profile Sync Protocol structure. > +// > +typedef struct { > + UINT32 Revision; > + WIFI_SET_CONNECT_STATE WifiProfileSyncSetConnectState; > + WIFI_GET_CONNECT_STATE WifiProfileSyncGetConnectState; > + WIFI_PROFILE_GET WifiProfileSyncGetProfile; > +} EFI_WIFI_PROFILE_SYNC_PROTOCOL; > + Could we remove the prefix -- WifiProfileSync? * Great call, update and in next patch > Tests to see if this driver supports a given controller. If a child > device is provided, > it further tests to see if this driver supports creating a handle > for the specified child device. > @@ -167,8 +172,10 @@ WifiMgrDxeDriverBindingStart ( > EFI_WIRELESS_MAC_CONNECTION_II_PROTOCOL *Wmp; > EFI_SUPPLICANT_PROTOCOL *Supplicant; > EFI_EAP_CONFIGURATION_PROTOCOL *EapConfig; > + EFI_WIFI_PROFILE_SYNC_PROTOCOL *WiFiProfileSyncProtocol; > > - Nic =3D NULL; > + mWifiConnectionCount =3D 0; > + Nic =3D NULL; > > // > // Open Protocols > @@ -236,47 +243,73 @@ WifiMgrDxeDriverBindingStart ( > InitializeListHead (&Nic->ProfileList); > > // > - // Record the MAC address of the incoming NIC. > + // WiFi profile sync protocol installation check for OS recovery flow. > // > - Status =3D NetLibGetMacAddress ( > - ControllerHandle, > - (EFI_MAC_ADDRESS *)&Nic->MacAddress, > - &AddressSize > - ); > - if (EFI_ERROR (Status)) { > - goto ERROR2; > - } > - > - // > - // Create and start the timer for the status check > - // > - Status =3D gBS->CreateEvent ( > - EVT_NOTIFY_SIGNAL | EVT_TIMER, > - TPL_CALLBACK, > - WifiMgrOnTimerTick, > - Nic, > - &Nic->TickTimer > + Status =3D gBS->LocateProtocol ( > + &gEfiWiFiProfileSyncProtocolGuid, > + NULL, > + (VOID **)&WiFiProfileSyncProtocol > ); > - if (EFI_ERROR (Status)) { > - goto ERROR2; > - } > + if (!EFI_ERROR (Status)) { > + Nic->ConnectPendingNetwork =3D (WIFI_MGR_NETWORK_PROFILE > *)AllocateZeroPool (sizeof (WIFI_MGR_NETWORK_PROFILE)); > + if (Nic->ConnectPendingNetwork =3D=3D NULL) { > + Status =3D EFI_OUT_OF_RESOURCES; > + goto ERROR1; > + } > > - Status =3D gBS->SetTimer (Nic->TickTimer, TimerPeriodic, > EFI_TIMER_PERIOD_MILLISECONDS (500)); > - if (EFI_ERROR (Status)) { > - goto ERROR3; > - } > + WiFiProfileSyncProtocol->WifiProfileSyncGetProfile (Nic- > >ConnectPendingNetwork, Nic->MacAddress); This is incorrect! With this change, you might map the profile to the wrong= ControllerHandle with incorrect Nic->MacAddress since this is called in th= e driver binging start, each NIC device will map to the same the MacAddress= from the WifiProfileSync protocol! * There will only be 1 NiC in this operation and so no miss-mapping wil= l occur. > + if (Nic->ConnectPendingNetwork !=3D NULL) { > + Status =3D WifiMgrConnectToNetwork (Nic, Nic- > >ConnectPendingNetwork); Why we need do this? because we have defined the timer for the connection. = See the WifiMgrOnTimerTick(). * We do not want to scan for a profile, we only want to use the profile= provided by AMT. so we bypass the timer scanning and jump to the connectio= n with the profile. > + if (EFI_ERROR (Status)) { > + WiFiProfileSyncProtocol->WifiProfileSyncSetConnectState (Status)= ; > + } > + } else { > + goto ERROR1; > + } Could we refine the if... else logic? For example, If (Error) { goto ERROR1; } Then do the right things. Existing patch has too much if else condition. * Great coding improvement point, updated and in next patch > + } else { > + // > + // Record the MAC address of the incoming NIC. > + // > } > > - AsciiPassword =3D AllocateZeroPool ((StrLen (Profile->Password) + 1) > * sizeof (UINT8)); > + if (StrLen (Profile->Password) >=3D PASSWORD_STORAGE_SIZE) { > + ASSERT (EFI_INVALID_PARAMETER); > + return EFI_INVALID_PARAMETER; > + } > + > + AsciiPassword =3D AllocateZeroPool ((StrLen (Profile->Password) + 1) > + * > sizeof (CHAR8)); > if (AsciiPassword =3D=3D NULL) { > return EFI_OUT_OF_RESOURCES; > } > > - UnicodeStrToAsciiStrS (Profile->Password, (CHAR8 *)AsciiPassword, > PASSWORD_STORAGE_SIZE); > - Status =3D Supplicant->SetData ( > - Supplicant, > - EfiSupplicant80211PskPassword, > - AsciiPassword, > - (StrLen (Profile->Password) + 1) * sizeof (UINT= 8) > - ); > + Status =3D UnicodeStrToAsciiStrS (Profile->Password, (CHAR8 > *)AsciiPassword, (StrLen (Profile->Password) + 1)); > + if (!EFI_ERROR (Status)) { > + Status =3D Supplicant->SetData ( > + Supplicant, > + EfiSupplicant80211PskPassword, > + AsciiPassword, > + (StrLen (Profile->Password) + 1) * sizeof (CH= AR8) > + ); > + } > + > ZeroMem (AsciiPassword, AsciiStrLen ((CHAR8 *)AsciiPassword) + 1); > FreePool (AsciiPassword); > Looks above change is not related to the changes! could we separate to anot= her patch? * These status checks and alignments are necessary for proper operating= functionality. They've been tested and validated with this code. It can an= d should be merged here as it was changed and tested with this feature chan= ge. > + > +**/ > +EFI_STATUS > + ConnectionRetry ( > + IN EFI_WIFI_PROFILE_SYNC_PROTOCOL *WiFiProfileSyncProtocol > + ) > +{ Why need ConnectionRetry? * This is a feature definition addition to the flow of the WLAN recover= y. If an error occurs the system will retry connection 3 times incase of co= nnection interruption. --_000_CO6PR11MB56019FBBC4A90EF230F16047C9F89CO6PR11MB5601namp_ Content-Type: text/html; charset="us-ascii" Content-Transfer-Encoding: quoted-printable

Thank you Jiaxin,

 

Find my responses inlin= e.

 

An updated patch with requested changes is in rou= te today.

 

Thanks,

Zack

 

-----Original Message-----
From: Wu, Jiaxin <jiaxin.wu@intel.com>
Sent: Thursday, January 5, 2023 9:09 PM
To: Clark-williams, Zachary <zachary.clark-williams@intel.com>; devel= @edk2.groups.io; Kinney, Michael D <michael.d.kinney@intel.com>
Cc: Zachary Clark-Williams <zclarkw112@gmail.com>; Maciej Rabeda <= maciej.rabeda@linux.intel.com>; Otcheretianski, Andrei <andrei.otcher= etianski@intel.com>; Zeng, Star <star.zeng@intel.com>
Subject: RE: [PATCH] NetworkPkg: Add WiFi profile sync protocol support

 

Hi Zachary,

 

Insert all my comments as below.

 

Besides: where defined this protocol (EFI_WIFI_PR= OFILE_SYNC_PROTOCOL)? I didn't find in the UEFI spec, in such a case, could= we named it as EDKII_WIFI_PROFILE_SYNC_PROTOCOL? please add more descripti= on about the protocol usage.

 

-  &nb= sp;            =        Yes, Will update n= aming convention

 

Thanks,

Jiaxin

 

 

> +/**

> +  Used by the WiFi connection manager = to get the WiFi profile that

> +AMT

> shared

> +  and was stored in WiFi profile proto= col. Aligns the AMT WiFi

> + profile data to  the WiFi connection = manager profile structure fo connection use.

> +

> +  @param[in, out]  WcmProfile&nbs= p;      WiFi Connection Manager profile=

> structure

> +  @param[in, out]  MacAddress&nbs= p;      MAC address from AMT saved to NiC

> MAC address

> +

> +  @retval EFI_SUCCESS   = ;            Stored = WiFi profile converted and returned

> succefully

> +  @retval EFI_UNSUPPORTED  &= nbsp;        Profile protocol sharing no= t supported or

> enabled

> +  @retval EFI_NOT_FOUND  &nb= sp;          No profiles to re= turned

> +  @retval Others   &nbs= p;            &= nbsp;   Error Occurred

> +**/

> +typedef

> +EFI_STATUS

> +(EFIAPI *WIFI_PROFILE_GET)(

> +  IN OUT  WIFI_MGR_NETWORK_PROFIL= E  *Profile,

> +  IN OUT  EFI_80211_MAC_ADDRESS&n= bsp;    MacAddress

> +  );

 

Does it mean the returned Profile is only for the= returned MacAddress? Does it must 1:1 mapping??

  • No, this is an Input and Output because we are both fulfilling = the WiFi profile data, converting it from the AMT structure to the WifiConn= ectionManager structure. The MAC address is stored in a separate place In the Nic than the profile data so we pass = both locations so we don’t need to send the full Nic just these 2 dat= a structure locations.

This is necessary as we need consistency in the MAC from AMT  to c= orrespond with the Profile data.

Think more, Does AMT support maintain multiple ma= ppings? Image we have multiple network socket, how AMT handle such case?&nb= sp;

  • At this point the AMT and CSME doe not support mult= iple NiC’s, or WiFi cards for this process. There will be one NiC, 1 = Card and 1 Profile used to keep secrets secure.

 

 

 

 

> +

> +/**

> +  Saves the WiFi connection status rec= ieved by the

> +WiFiConnectionManager

> when

> +  in a KVM OR One Click Recovery WLAN = recovery flow. Input as 

> + EFI_80211_CONNECT_NETWORK_RESULT_CODE then= converted and

> stored as EFI_STATUS type.

> +

 

Why need stored as EFI_STATUS type since we have = defined the EFI_80211_CONNECT_NETWORK_RESULT_CODE???

  • This= is necessary to decouple the code for more modularity, otherwise we would = have cross-repo dependencies and very very ugly mess of code. This conversi= on is simpler and cleaner.

 

 

 

> +  @param[in] ConnectionStatus &nb= sp;   WiFi connection attempt results

> +**/

> +typedef

> +VOID

> +(EFIAPI *WIFI_SET_CONNECT_STATE)(

> +  IN  EFI_80211_CONNECT_NETWORK_R= ESULT_CODE ConnectionStatus

> +  );

> +

> +/**

> +  Retrieves the stored WiFi connection= status when in either KVM OR

> +One

> Click

> +  Recovery WLAN recovery flow.

> +

> +  @retval EFI_SUCCESS   = ;            WiFi co= nnection completed succesfully

> +  @retval Others   &nbs= p;            &= nbsp;   Connection failure occurred

> +**/

> +typedef

> +EFI_STATUS

> +(EFIAPI *WIFI_GET_CONNECT_STATE)(

> +  VOID

> +  );

 

 

What's the output? Only EFI_STATUS? why not retur= n the EFI_80211_CONNECT_NETWORK_RESULT_CODE? We should not mix the EFI_8021= 1_CONNECT_NETWORK_RESULT_CODE & EFI_STATUS.

  • See = answer to previous

 

 

> +

> +//

> +//  WiFi Profile Sync Protocol structu= re.

> +//

> +typedef struct {

> +  UINT32     =             &nb= sp;  Revision;

> +  WIFI_SET_CONNECT_STATE  &n= bsp; WifiProfileSyncSetConnectState;

> +  WIFI_GET_CONNECT_STATE  &n= bsp; WifiProfileSyncGetConnectState;

> +  WIFI_PROFILE_GET   &n= bsp;      WifiProfileSyncGetProfile;

> +} EFI_WIFI_PROFILE_SYNC_PROTOCOL;

> +

 

 

Could we remove the prefix -- WifiProfileSync?

  • Great call, update and in next patch

    •  

     

    >    Tests to see if this drive= r supports a given controller. If a child

    > device is provided,

    >    it further tests to see if= this driver supports creating a handle

    > for the specified child device.

    > @@ -167,8 +172,10 @@ WifiMgrDxeDriverBinding= Start (

    >    EFI_WIRELESS_MAC_CONNECTIO= N_II_PROTOCOL  *Wmp;

    >    EFI_SUPPLICANT_PROTOCOL&nb= sp;            =      *Supplicant;

    >    EFI_EAP_CONFIGURATION_PROT= OCOL           *EapConfig= ;

    > +  EFI_WIFI_PROFILE_SYNC_PROTOCOL =           *WiFiProfileSyncProt= ocol;

    >

    > -  Nic =3D NULL;

    > +  mWifiConnectionCount =3D 0;

    > +  Nic     &nb= sp;            =3D N= ULL;

    >

    >    //

    >    // Open Protocols

    > @@ -236,47 +243,73 @@ WifiMgrDxeDriverBindin= gStart (

    >    InitializeListHead (&N= ic->ProfileList);

    >

    >    //

    > -  // Record the MAC address of the inc= oming NIC.

    > +  // WiFi profile sync protocol instal= lation check for OS recovery flow.

    >    //

    > -  Status =3D NetLibGetMacAddress (

    > -       &= nbsp;     ControllerHandle,

    > -       &= nbsp;     (EFI_MAC_ADDRESS *)&Nic->MacAddress,

    > -       &= nbsp;     &AddressSize

    > -       &= nbsp;     );

    > -  if (EFI_ERROR (Status)) {=

    > -    goto ERROR2;<= /p>

    > -  }

    > -

    > -  //

    > -  // Create and start the timer for th= e status check

    > -  //

    > -  Status =3D gBS->CreateEvent (

    > -       &= nbsp;          EVT_NOTIFY_SIGN= AL | EVT_TIMER,

    > -       &= nbsp;          TPL_CALLBACK,

    > -       &= nbsp;          WifiMgrOnTimerT= ick,

    > -       &= nbsp;          Nic,=

    > -       &= nbsp;          &Nic->Ti= ckTimer

    > +  Status =3D gBS->LocateProtocol (<= o:p>

    > +        =           &gEfiWiFiPr= ofileSyncProtocolGuid,

    > +       &= nbsp;          NULL,

    > +       &= nbsp;          (VOID **)&W= iFiProfileSyncProtocol

    >       &nb= sp;            );

    > -  if (EFI_ERROR (Status)) {=

    > -    goto ERROR2;<= /p>

    > -  }

    > +  if (!EFI_ERROR (Status)) {

    > +    Nic->ConnectPendingNe= twork =3D (WIFI_MGR_NETWORK_PROFILE

    > *)AllocateZeroPool (sizeof (WIFI_MGR_NETWORK= _PROFILE));

    > +    if (Nic->ConnectPendi= ngNetwork =3D=3D NULL) {

    > +      Status =3D E= FI_OUT_OF_RESOURCES;

    > +      goto ERROR1;=

    > +    }

    >

    > -  Status =3D gBS->SetTimer (Nic->= ;TickTimer, TimerPeriodic,

    > EFI_TIMER_PERIOD_MILLISECONDS (500));

    > -  if (EFI_ERROR (Status)) {=

    > -    goto ERROR3;<= /p>

    > -  }

    > +    WiFiProfileSyncProtocol-= >WifiProfileSyncGetProfile (Nic-

    > >ConnectPendingNetwork, Nic->MacAddres= s);

     

     

     

    This is incorrect! With this change, you might ma= p the profile to the wrong ControllerHandle with incorrect Nic->MacAddre= ss since this is called in the driver binging start, each NIC device will m= ap to the same the MacAddress from the WifiProfileSync protocol!

    • Ther= e will only be 1 NiC in this operation and so no miss-mapping will occur.

     

     

     

     

     

    > +    if (Nic->ConnectPendi= ngNetwork !=3D NULL) {

    > +      Status =3D W= ifiMgrConnectToNetwork (Nic, Nic-

    > >ConnectPendingNetwork);

     

     

    Why we need do this? because we have defined the = timer for the connection. See the WifiMgrOnTimerTick().

    • We do not want to scan for a profile, we only want to use the p= rofile provided by AMT. so we bypass the timer scanning and jump to the con= nection with the profile.

     

     

    > +      if (EFI_ERRO= R (Status)) {

    > +        = WiFiProfileSyncProtocol->WifiProfileSyncSetConnectState (Status);

    > +      }=

    > +    } else {

    > +      goto ERROR1;=

    > +    }

     

    Could we refine the if... else logic? For example= , If (Error) {

            &= nbsp;       goto ERROR1;

    }

    Then do the right things. Existing patch has too = much if else condition.

    • Grea= t coding improvement point, updated and in next patch

     

     

    > +  } else {

    > +    //

    > +    // Record the MAC addres= s of the incoming NIC.

    > +    //

     

     

     

     

    >    }

    >

    > -  AsciiPassword =3D AllocateZeroPool (= (StrLen (Profile->Password) + 1)

    > * sizeof (UINT8));

    > +  if (StrLen (Profile->Password) &g= t;=3D PASSWORD_STORAGE_SIZE) {

    > +    ASSERT (EFI_INVALID_PARA= METER);

    > +    return EFI_INVALID_PARAM= ETER;

    > +  }

    > +

    > +  AsciiPassword =3D AllocateZeroPool (= (StrLen (Profile->Password) + 1)

    > + *

    > sizeof (CHAR8));

    >    if (AsciiPassword =3D=3D N= ULL) {

    >      return EFI_OUT= _OF_RESOURCES;

    >    }

    >

    > -  UnicodeStrToAsciiStrS (Profile->P= assword, (CHAR8 *)AsciiPassword,

    > PASSWORD_STORAGE_SIZE);

    > -  Status =3D Supplicant->SetData (<= o:p>

    > -       &= nbsp;           &nbs= p;     Supplicant,

    > -       &= nbsp;           &nbs= p;     EfiSupplicant80211PskPassword,

    > -       &= nbsp;            &nb= sp;    AsciiPassword,

    > -       &= nbsp;           &nbs= p;     (StrLen (Profile->Password) + 1) * sizeof (UI= NT8)

    > -       &= nbsp;           &nbs= p;     );

    > +  Status =3D UnicodeStrToAsciiStrS (Pr= ofile->Password, (CHAR8

    > *)AsciiPassword, (StrLen (Profile->Passwo= rd) + 1));

    > +  if (!EFI_ERROR (Status)) {

    > +    Status =3D Supplicant-&g= t;SetData (

    > +       &= nbsp;           &nbs= p;       Supplicant,

    > +       &= nbsp;           &nbs= p;       EfiSupplicant80211PskPassword,<= /o:p>

    > +       &= nbsp;           &nbs= p;       AsciiPassword,

    > +       &= nbsp;           &nbs= p;       (StrLen (Profile->Password) + 1) = * sizeof (CHAR8)

    > +       &= nbsp;           &nbs= p;       );

    > +  }

    > +

    >    ZeroMem (AsciiPassword, As= ciiStrLen ((CHAR8 *)AsciiPassword) + 1);

    >    FreePool (AsciiPassword);<= o:p>

    >

     

     

    Looks above change is not related to the changes!= could we separate to another patch?

    • These status checks and alignments are necessary for proper ope= rating functionality. They’ve been tested and validated with this cod= e. It can and should be merged here as it was changed and tested with this feature change.

     

     

    > +

    > +**/

    > +EFI_STATUS

    > + ConnectionRetry (

    > +  IN   EFI_WIFI_PROFILE_SYNC= _PROTOCOL  *WiFiProfileSyncProtocol

    > +  )

    > +{

     

    Why need ConnectionRetry?

    • This is a feature definition addition to the flow o= f the WLAN recovery. If an error occurs the system will retry connection 3 = times incase of connection interruption.

     

     

--_000_CO6PR11MB56019FBBC4A90EF230F16047C9F89CO6PR11MB5601namp_--