Hi Zachary, Insert all my comments as below.
Besides: where defined this protocol (EFI_WIFI_PROFILE_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 description about the protocol usage. 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 connection 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 > returned > succefully > + @retval EFI_UNSUPPORTED Profile protocol sharing not supported 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?? Think more, Does AMT support maintain multiple mappings? Image we have multiple network socket, how AMT handle such case? > + > +/** > + 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_CONNECT_NETWORK_RESULT_CODE??? > + @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 succesfully > + @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_NETWORK_RESULT_CODE? We should not mix the EFI_80211_CONNECT_NETWORK_RESULT_CODE & EFI_STATUS. > + > +// > +// 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? > 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 = NULL; > + mWifiConnectionCount = 0; > + Nic = 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 = NetLibGetMacAddress ( > - ControllerHandle, > - (EFI_MAC_ADDRESS *)&Nic->MacAddress, > - &AddressSize > - ); > - if (EFI_ERROR (Status)) { > - goto ERROR2; > - } > - > - // > - // Create and start the timer for the status check > - // > - Status = gBS->CreateEvent ( > - EVT_NOTIFY_SIGNAL | EVT_TIMER, > - TPL_CALLBACK, > - WifiMgrOnTimerTick, > - Nic, > - &Nic->TickTimer > + Status = gBS->LocateProtocol ( > + &gEfiWiFiProfileSyncProtocolGuid, > + NULL, > + (VOID **)&WiFiProfileSyncProtocol > ); > - if (EFI_ERROR (Status)) { > - goto ERROR2; > - } > + if (!EFI_ERROR (Status)) { > + Nic->ConnectPendingNetwork = (WIFI_MGR_NETWORK_PROFILE > *)AllocateZeroPool (sizeof (WIFI_MGR_NETWORK_PROFILE)); > + if (Nic->ConnectPendingNetwork == NULL) { > + Status = EFI_OUT_OF_RESOURCES; > + goto ERROR1; > + } > > - Status = 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 the driver binging start, each NIC device will map to the same the MacAddress from the WifiProfileSync protocol! > + if (Nic->ConnectPendingNetwork != NULL) { > + Status = WifiMgrConnectToNetwork (Nic, Nic- > >ConnectPendingNetwork); Why we need do this? because we have defined the timer for the connection. See the WifiMgrOnTimerTick(). > + 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. > + } else { > + // > + // Record the MAC address of the incoming NIC. > + // > } > > - AsciiPassword = AllocateZeroPool ((StrLen (Profile->Password) + 1) * sizeof > (UINT8)); > + if (StrLen (Profile->Password) >= PASSWORD_STORAGE_SIZE) { > + ASSERT (EFI_INVALID_PARAMETER); > + return EFI_INVALID_PARAMETER; > + } > + > + AsciiPassword = AllocateZeroPool ((StrLen (Profile->Password) + 1) * > sizeof (CHAR8)); > if (AsciiPassword == NULL) { > return EFI_OUT_OF_RESOURCES; > } > > - UnicodeStrToAsciiStrS (Profile->Password, (CHAR8 *)AsciiPassword, > PASSWORD_STORAGE_SIZE); > - Status = Supplicant->SetData ( > - Supplicant, > - EfiSupplicant80211PskPassword, > - AsciiPassword, > - (StrLen (Profile->Password) + 1) * sizeof (UINT8) > - ); > + Status = UnicodeStrToAsciiStrS (Profile->Password, (CHAR8 > *)AsciiPassword, (StrLen (Profile->Password) + 1)); > + if (!EFI_ERROR (Status)) { > + Status = Supplicant->SetData ( > + Supplicant, > + EfiSupplicant80211PskPassword, > + AsciiPassword, > + (StrLen (Profile->Password) + 1) * sizeof (CHAR8) > + ); > + } > + > ZeroMem (AsciiPassword, AsciiStrLen ((CHAR8 *)AsciiPassword) + 1); > FreePool (AsciiPassword); > Looks above change is not related to the changes! could we separate to another patch? > + > +**/ > +EFI_STATUS > + ConnectionRetry ( > + IN EFI_WIFI_PROFILE_SYNC_PROTOCOL *WiFiProfileSyncProtocol > + ) > +{ Why need ConnectionRetry? -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#98060): https://edk2.groups.io/g/devel/message/98060 Mute This Topic: https://groups.io/mt/95025543/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-