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.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.otcheretianski@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_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??

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? 

 

 

 

 

> +

> +/**

> +  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?