Thank you Jiaxin,
Find my responses inline.
An updated patch with requested changes is in route today.
Thanks,
Zack
-----Original Message-----
From: Wu, Jiaxin <jiaxin...@intel.com>
Sent: Thursday, January 5, 2023 9:09 PM
To: Clark-williams, Zachary <zachary.clark-willi...@intel.com>;
devel@edk2.groups.io; Kinney, Michael D <michael.d.kin...@intel.com>
Cc: Zachary Clark-Williams <zclarkw...@gmail.com>; Maciej Rabeda
<maciej.rab...@linux.intel.com>; Otcheretianski, Andrei
<andrei.otcheretian...@intel.com>; Zeng, Star <star.z...@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_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.
- 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
> 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??
* No, this is an Input and Output because we are both fulfilling the WiFi
profile data, converting it from the AMT structure to the WifiConnectionManager
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 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 multiple
network socket, how AMT handle such case?
* At this point the AMT and CSME doe not support multiple 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 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???
* 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
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 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.
* 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 = 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!
* There will only be 1 NiC in this operation and so no miss-mapping will
occur.
> + 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().
* 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 connection
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 = 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?
* These status checks and alignments are necessary for proper operating
functionality. They've been tested and validated with this code. 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 of the WLAN recovery.
If an error occurs the system will retry connection 3 times incase of
connection interruption.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#98144): https://edk2.groups.io/g/devel/message/98144
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]
-=-=-=-=-=-=-=-=-=-=-=-
