Skip to content

WifiConnectionManager: PreScan Check for Connect #11039

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
Zclarkwilliams wants to merge 1 commit into from
Closed

WifiConnectionManager: PreScan Check for Connect #11039

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions NetworkPkg/WifiConnectionManagerDxe/WifiConnectionMgrImpl.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1489,8 +1489,26 @@ WifiMgrOnTimerTick (
}

Nic = (WIFI_MGR_DEVICE_DATA *)Context;
if (Nic->ConnectPendingNetwork == NULL) {
DEBUG ((DEBUG_INFO, "[WiFi Connection Manager] No profile for connection, no scan triggered!\n"));
gBS->CloseEvent (Event);
return;
}

if (StrLen (Nic->ConnectPendingNetwork->SSId) < SSID_MIN_LEN) {
DEBUG ((DEBUG_INFO, "[WiFi Connection Manager] Invalid SSID length for connection, no scan triggered!\n"));
gBS->CloseEvent (Event);
Copy link
Member

@mdkinney mdkinney May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do these error return paths close the event and others in this function do not?

Copy link
Contributor Author

@Zclarkwilliams Zclarkwilliams May 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These failure conditions indicate that the scan is invalid and should not be executed, so we close the event. Others are possible valid scans where there was a hiccup in the system where the scan is still valid but didn't execute fully this round, but should still scan.

Copy link
Member

@mdkinney mdkinney May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps some comments to explain why the event is being closed.

Copy link
Member

@ardbiesheuvel ardbiesheuvel May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WifiMgrOnTimerTick() is called by a timer event, and IIUC, closing it will prevent it from being called again until the driver unbinds and binds again. Is that really the right thing to do here?

return;
}

NET_CHECK_SIGNATURE (Nic, WIFI_MGR_DEVICE_DATA_SIGNATURE);

if ((Nic->ConnectState == WifiMgrConnectedToAp) || (Nic->ConnectState == WifiMgrConnectingToAp)) {
DEBUG ((DEBUG_INFO, "[WiFi Connection Manager] Already connecting to AP, no scan triggered!\n"));
gBS->CloseEvent (Event);
return;
}

Copy link
Member

@ardbiesheuvel ardbiesheuvel May 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These early returns from the function make some of the code in the switch below unreachable, so it seems to me that the logic should be combined in some way.

Status = WifiMgrGetLinkState (Nic, &LinkState);
if (EFI_ERROR (Status)) {
DEBUG ((DEBUG_INFO, "[WiFi Connection Manager] Error: Failed to get link state!\n"));
Expand Down
Loading