shill: alternate hidden scans when limit is 1
It was reported that the lenovo x131e with the BCM43228 wireless card
was unable to connect to hidden networks. This was because the wl driver
for devices such as this is only able to support 1 SSID per scan.
In order to fix this issue, alternate between scanning a hidden network
and doing the broadcast probe when MaxScanSSID == 1.
This change is necessary for reven builds.
BUG=b:179823373
TEST=cros_workon_make --board=reven shill --test, and this change was
also tested on CloudReady 1.0 on the lenovo x131e by first testing it
without the change to confirm that this fix is still needed, and then
testing it with the change to ensure it works as expected.
I also measured latency on the lenovo x131e. Because there is a
hidden ssid on the hidden network list, and maxScanSSID = 1 for that
chipset, that seems to add a latency of between 2s and 5s for regular
networks. Latency for the hidden network was somewhat higher, ranging
between 2s-15s. This is an expected casualty of this chipset's limited
scanning capability.
Change-Id: Iddc2bfc64b0df0685345e28bd520ade4f3f98c40
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/3126325
Reviewed-by: Jeffery Miller <jefferymiller@google.com>
Reviewed-by: Matthew Wang <matthewmwang@chromium.org>
Reviewed-by: Abhishek Kumar <kuabhs@chromium.org>
Reviewed-by: Alain Michaud <alainm@chromium.org>
Tested-by: Esther Shimanovich <eshimanovich@google.com>
Commit-Queue: Esther Shimanovich <eshimanovich@google.com>
diff --git a/shill/wifi/wifi.cc b/shill/wifi/wifi.cc
index 97b4dd0..5ffe929 100644
--- a/shill/wifi/wifi.cc
+++ b/shill/wifi/wifi.cc
@@ -169,6 +169,7 @@
sched_scan_supported_(false),
scan_state_(kScanIdle),
scan_method_(kScanMethodNone),
+ broadcast_probe_was_skipped_(false),
receive_byte_count_at_connect_(0),
wiphy_index_(kDefaultWiphyIndex),
wifi_cqm_(new WiFiCQM(metrics(), this)),
@@ -1352,7 +1353,6 @@
return;
}
-
// Look for scheduled scan support.
AttributeListConstRefPtr cmds;
if (nl80211_message.const_attributes()->ConstGetNestedAttributeList(
@@ -1864,7 +1864,8 @@
}
if (max_ssids_per_scan_ <= 1)
- LOG(WARNING) << "MaxScanSSID <= 1, no hidden SSID will be used.";
+ LOG(WARNING) << "MaxScanSSID <= 1, scans will alternate between single "
+ << "hidden SSID and broadcast scan.";
}
void WiFi::ScanTask() {
@@ -1889,14 +1890,14 @@
WPASupplicant::kScanTypeActive);
ByteArrays hidden_ssids = provider_->GetHiddenSSIDList();
-
if (!hidden_ssids.empty()) {
- // The empty '' "broadcast SSID" counts toward the max scan limit, so the
- // capability needs to be >= 2 to have at least 1 hidden SSID.
+ // Determine how many hidden ssids to pass in, based on max_ssids_per_scan_
if (max_ssids_per_scan_ > 1) {
+ // The empty '' "broadcast SSID" counts toward the max scan limit, so the
+ // capability needs to be >= 2 to have at least 1 hidden SSID.
if (hidden_ssids.size() >= static_cast<size_t>(max_ssids_per_scan_)) {
// TODO(b/172220260): Devise a better method for time-sharing with SSIDs
- // that do not fit in.
+ // that do not fit in
hidden_ssids.erase(hidden_ssids.begin() + max_ssids_per_scan_ - 1,
hidden_ssids.end());
}
@@ -1905,6 +1906,14 @@
// behavior of doing a broadcast probe.
hidden_ssids.push_back(ByteArray());
+ } else if (max_ssids_per_scan_ == 1) {
+ // Handle case where driver can only accept one scan_ssid at a time
+ AlternateSingleScans(&hidden_ssids);
+ } else { // if max_ssids_per_scan_ < 1
+ hidden_ssids.resize(0);
+ }
+
+ if (!hidden_ssids.empty()) {
scan_args.Set<ByteArrays>(WPASupplicant::kPropertyScanSSIDs,
hidden_ssids);
}
@@ -1927,6 +1936,18 @@
}
}
+void WiFi::AlternateSingleScans(ByteArrays* hidden_ssids) {
+ // Ensure at least one hidden SSID is probed.
+ if (broadcast_probe_was_skipped_) {
+ SLOG(this, 2) << "Doing broadcast probe instead of directed probe.";
+ hidden_ssids->resize(0);
+ } else {
+ SLOG(this, 2) << "Doing directed probe instead of broadcast probe.";
+ hidden_ssids->resize(1);
+ }
+ broadcast_probe_was_skipped_ = !broadcast_probe_was_skipped_;
+}
+
std::string WiFi::GetServiceLeaseName(const WiFiService& service) {
return service.GetStorageIdentifier();
}
diff --git a/shill/wifi/wifi.h b/shill/wifi/wifi.h
index 248b84c..13c2165 100644
--- a/shill/wifi/wifi.h
+++ b/shill/wifi/wifi.h
@@ -419,6 +419,10 @@
// [ScanDone]-->[ScanDoneTask]-->[UpdateScanStateAfterScanDone]
void UpdateScanStateAfterScanDone();
void ScanTask();
+ // When scans are limited to one ssid, alternate between broadcast probes
+ // and directed probes. This is necessary because the broadcast probe takes
+ // up one SSID slot, leaving no space for the directed probe.
+ void AlternateSingleScans(ByteArrays* hidden_ssids);
void StateChanged(const std::string& new_state);
// Heuristic check if a connection failure was due to bad credentials.
// Returns true and puts type of failure in |failure| if a credential
@@ -750,6 +754,9 @@
ScanState scan_state_;
ScanMethod scan_method_;
+ // Indicates if the last scan skipped the broadcast probe.
+ bool broadcast_probe_was_skipped_;
+
// Used to compute the number of bytes received since the link went up.
uint64_t receive_byte_count_at_connect_;
diff --git a/shill/wifi/wifi_test.cc b/shill/wifi/wifi_test.cc
index 2bd8c35..8e507a8 100644
--- a/shill/wifi/wifi_test.cc
+++ b/shill/wifi/wifi_test.cc
@@ -1143,6 +1143,10 @@
void StartScanTimer() { wifi_->StartScanTimer(); }
+ bool GetBroadcastProbeWasSkipped() {
+ return wifi_->broadcast_probe_was_skipped_;
+ }
+
bool ParseWiphyIndex(const Nl80211Message& nl80211_message) {
return wifi_->ParseWiphyIndex(nl80211_message);
}
@@ -2267,6 +2271,18 @@
EXPECT_TRUE(GetReconnectTimeoutCallback().IsCancelled());
}
+MATCHER_P(ScanRequestHasHiddenSSIDAndSkipsBroadcast, ssid, "") {
+ if (!arg.template Contains<ByteArrays>(WPASupplicant::kPropertyScanSSIDs)) {
+ return false;
+ }
+
+ ByteArrays ssids =
+ arg.template Get<ByteArrays>(WPASupplicant::kPropertyScanSSIDs);
+ // When the ssid limit is 1, a valid Scan containing a
+ // single hidden SSID should only contain the SSID we are looking for.
+ return ssids.size() == 1 && ssids[0] == ssid;
+}
+
MATCHER_P(ScanRequestHasHiddenSSID, ssid, "") {
if (!arg.template Contains<ByteArrays>(WPASupplicant::kPropertyScanSSIDs)) {
return false;
@@ -2322,9 +2338,9 @@
event_dispatcher_->DispatchPendingEvents();
}
-// When the driver reports that it supports 1 SSIDs in the scan request, no
-// hidden SSIDs should be included either, because shill would also need to
-// include a brodacast entry.
+// When the driver reports that it supports 1 SSIDs in the scan request, it
+// should alternate between including a hidden SSID and including a broadcast
+// entry.
TEST_F(WiFiMainTest, ScanHiddenRespectsMaxSSIDs1) {
SetInterfaceScanLimit(1);
@@ -2332,11 +2348,27 @@
ByteArrays ssids{{'a'}, {'b'}, {'c'}, {'d'}, {'e'}, {'f'}, {'g'}, {'h'}};
EXPECT_CALL(*wifi_provider(), GetHiddenSSIDList())
.WillRepeatedly(Return(ssids));
- StartWiFi();
+ // First scan
+ StartWiFi();
+ // Start by including hidden entry
+ EXPECT_CALL(*GetSupplicantInterfaceProxy(),
+ Scan(ScanRequestHasHiddenSSIDAndSkipsBroadcast(ssids[0])));
+ event_dispatcher_->DispatchPendingEvents();
+
+ // Second scan
+ InitiateScan();
+ // Now we have no hidden SSID and do the broadcast scan
EXPECT_CALL(*GetSupplicantInterfaceProxy(),
Scan(ScanRequestHasNoHiddenSSID()));
event_dispatcher_->DispatchPendingEvents();
+
+ // Third scan
+ InitiateScan();
+ // back to doing a hidden SSID scan
+ EXPECT_CALL(*GetSupplicantInterfaceProxy(),
+ Scan(ScanRequestHasHiddenSSIDAndSkipsBroadcast(ssids[0])));
+ event_dispatcher_->DispatchPendingEvents();
}
// When the driver reports that it supports smaller number of SSIDs than we have
