Merge "wifi(implementation): Make WifiLegacyHal.stop() blocking" into oc-mr1-dev

wifi/1.1/default/hidl_return_util.h

@@ -55,6 +55,25 @@ Return<void> validateAndCall(
   return Void();
 }
 
+// Use for HIDL methods which return only an instance of WifiStatus.
+// This version passes the global lock acquired to the body of the method.
+// Note: Only used by IWifi::stop() currently.
+template <typename ObjT, typename WorkFuncT, typename... Args>
+Return<void> validateAndCallWithLock(
+    ObjT* obj,
+    WifiStatusCode status_code_if_invalid,
+    WorkFuncT&& work,
+    const std::function<void(const WifiStatus&)>& hidl_cb,
+    Args&&... args) {
+  auto lock = hidl_sync_util::acquireGlobalLock();
+  if (obj->isValid()) {
+    hidl_cb((obj->*work)(&lock, std::forward<Args>(args)...));
+  } else {
+    hidl_cb(createWifiStatus(status_code_if_invalid));
+  }
+  return Void();
+}
+
 // Use for HIDL methods which return instance of WifiStatus and a single return
 // value.
 template <typename ObjT, typename WorkFuncT, typename ReturnT, typename... Args>

wifi/1.1/default/wifi.cpp

@@ -31,6 +31,7 @@ namespace wifi {
 namespace V1_1 {
 namespace implementation {
 using hidl_return_util::validateAndCall;
+using hidl_return_util::validateAndCallWithLock;
 
 Wifi::Wifi()
     : legacy_hal_(new legacy_hal::WifiLegacyHal()),
@@ -64,8 +65,8 @@ Return<void> Wifi::start(start_cb hidl_status_cb) {
 }
 
 Return<void> Wifi::stop(stop_cb hidl_status_cb) {
-  return validateAndCall(
+  return validateAndCallWithLock(this, WifiStatusCode::ERROR_UNKNOWN,
-      this, WifiStatusCode::ERROR_UNKNOWN, &Wifi::stopInternal, hidl_status_cb);
+                                 &Wifi::stopInternal, hidl_status_cb);
 }
 
 Return<void> Wifi::getChipIds(getChipIds_cb hidl_status_cb) {
@@ -120,7 +121,8 @@ WifiStatus Wifi::startInternal() {
   return wifi_status;
 }
 
-WifiStatus Wifi::stopInternal() {
+WifiStatus Wifi::stopInternal(
+    /* NONNULL */ std::unique_lock<std::recursive_mutex>* lock) {
   if (run_state_ == RunState::STOPPED) {
     return createWifiStatus(WifiStatusCode::SUCCESS);
   } else if (run_state_ == RunState::STOPPING) {
@@ -133,7 +135,7 @@ WifiStatus Wifi::stopInternal() {
     chip_->invalidate();
     chip_.clear();
   }
-  WifiStatus wifi_status = stopLegacyHalAndDeinitializeModeController();
+  WifiStatus wifi_status = stopLegacyHalAndDeinitializeModeController(lock);
   if (wifi_status.code == WifiStatusCode::SUCCESS) {
     for (const auto& callback : event_cb_handler_.getCallbacks()) {
       if (!callback->onStop().isOk()) {
@@ -180,11 +182,12 @@ WifiStatus Wifi::initializeLegacyHal() {
   return createWifiStatus(WifiStatusCode::SUCCESS);
 }
 
-WifiStatus Wifi::stopLegacyHalAndDeinitializeModeController() {
+WifiStatus Wifi::stopLegacyHalAndDeinitializeModeController(
+    /* NONNULL */ std::unique_lock<std::recursive_mutex>* lock) {
   run_state_ = RunState::STOPPING;
   const auto on_complete_callback_ = [&]() { run_state_ = RunState::STOPPED; };
   legacy_hal::wifi_error legacy_status =
-      legacy_hal_->stop(on_complete_callback_);
+      legacy_hal_->stop(lock, on_complete_callback_);
   if (legacy_status != legacy_hal::WIFI_SUCCESS) {
     LOG(ERROR) << "Failed to stop legacy HAL: "
                << legacyErrorToString(legacy_status);

wifi/1.1/default/wifi.h

@@ -61,12 +61,13 @@ class Wifi : public V1_1::IWifi {
   WifiStatus registerEventCallbackInternal(
       const sp<IWifiEventCallback>& event_callback);
   WifiStatus startInternal();
-  WifiStatus stopInternal();
+  WifiStatus stopInternal(std::unique_lock<std::recursive_mutex>* lock);
   std::pair<WifiStatus, std::vector<ChipId>> getChipIdsInternal();
   std::pair<WifiStatus, sp<IWifiChip>> getChipInternal(ChipId chip_id);
 
   WifiStatus initializeLegacyHal();
-  WifiStatus stopLegacyHalAndDeinitializeModeController();
+  WifiStatus stopLegacyHalAndDeinitializeModeController(
+      std::unique_lock<std::recursive_mutex>* lock);
 
   // Instance is created in this root level |IWifi| HIDL interface object
   // and shared with all the child HIDL interface objects.

wifi/1.1/default/wifi_legacy_hal.cpp

@@ -15,6 +15,7 @@
  */
 
 #include <array>
+#include <chrono>
 
 #include <android-base/logging.h>
 #include <cutils/properties.h>
@@ -34,6 +35,7 @@ static constexpr uint32_t kMaxGscanFrequenciesForBand = 64;
 static constexpr uint32_t kLinkLayerStatsDataMpduSizeThreshold = 128;
 static constexpr uint32_t kMaxWakeReasonStatsArraySize = 32;
 static constexpr uint32_t kMaxRingBuffers = 10;
+static constexpr uint32_t kMaxStopCompleteWaitMs = 50;
 
 // Helper function to create a non-const char* for legacy Hal API's.
 std::vector<char> makeCharVec(const std::string& str) {
@@ -53,7 +55,8 @@ namespace legacy_hal {
 // Legacy HAL functions accept "C" style function pointers, so use global
 // functions to pass to the legacy HAL function and store the corresponding
 // std::function methods to be invoked.
-// Callback to be invoked once |stop| is complete.
+//
+// Callback to be invoked once |stop| is complete
 std::function<void(wifi_handle handle)> on_stop_complete_internal_callback;
 void onAsyncStopComplete(wifi_handle handle) {
   const auto lock = hidl_sync_util::acquireGlobalLock();
@@ -369,6 +372,7 @@ wifi_error WifiLegacyHal::start() {
 }
 
 wifi_error WifiLegacyHal::stop(
+    /* NONNULL */ std::unique_lock<std::recursive_mutex>* lock,
     const std::function<void()>& on_stop_complete_user_callback) {
   if (!is_started_) {
     LOG(DEBUG) << "Legacy HAL already stopped";
@@ -376,19 +380,27 @@ wifi_error WifiLegacyHal::stop(
     return WIFI_SUCCESS;
   }
   LOG(DEBUG) << "Stopping legacy HAL";
-  on_stop_complete_internal_callback = [on_stop_complete_user_callback,
+  on_stop_complete_internal_callback =
-                                        this](wifi_handle handle) {
+      [on_stop_complete_user_callback, this](wifi_handle handle) {
     CHECK_EQ(global_handle_, handle) << "Handle mismatch";
+    LOG(INFO) << "Legacy HAL stop complete callback received";
     // Invalidate all the internal pointers now that the HAL is
     // stopped.
     invalidate();
     iface_tool_.SetWifiUpState(false);
     on_stop_complete_user_callback();
+    is_started_ = false;
   };
   awaiting_event_loop_termination_ = true;
   global_func_table_.wifi_cleanup(global_handle_, onAsyncStopComplete);
+  const auto status = stop_wait_cv_.wait_for(
+      *lock, std::chrono::milliseconds(kMaxStopCompleteWaitMs),
+      [this] { return !awaiting_event_loop_termination_; });
+  if (!status) {
+    LOG(ERROR) << "Legacy HAL stop failed or timed out";
+    return WIFI_ERROR_UNKNOWN;
+  }
   LOG(DEBUG) << "Legacy HAL stop complete";
-  is_started_ = false;
   return WIFI_SUCCESS;
 }
 
@@ -1257,11 +1269,13 @@ wifi_error WifiLegacyHal::retrieveWlanInterfaceHandle() {
 void WifiLegacyHal::runEventLoop() {
   LOG(DEBUG) << "Starting legacy HAL event loop";
   global_func_table_.wifi_event_loop(global_handle_);
+  const auto lock = hidl_sync_util::acquireGlobalLock();
   if (!awaiting_event_loop_termination_) {
     LOG(FATAL) << "Legacy HAL event loop terminated, but HAL was not stopping";
   }
   LOG(DEBUG) << "Legacy HAL event loop terminated";
   awaiting_event_loop_termination_ = false;
+  stop_wait_cv_.notify_one();
 }
 
 std::pair<wifi_error, std::vector<wifi_cached_scan_results>>

wifi/1.1/default/wifi_legacy_hal.h

@@ -20,6 +20,7 @@
 #include <functional>
 #include <thread>
 #include <vector>
+#include <condition_variable>
 
 #include <wifi_system/interface_tool.h>
 
@@ -149,8 +150,10 @@ class WifiLegacyHal {
   wifi_error initialize();
   // Start the legacy HAL and the event looper thread.
   wifi_error start();
-  // Deinitialize the legacy HAL and stop the event looper thread.
+  // Deinitialize the legacy HAL and wait for the event loop thread to exit
-  wifi_error stop(const std::function<void()>& on_complete_callback);
+  // using a predefined timeout.
+  wifi_error stop(std::unique_lock<std::recursive_mutex>* lock,
+                  const std::function<void()>& on_complete_callback);
   // Wrappers for all the functions in the legacy HAL function table.
   std::pair<wifi_error, std::string> getDriverVersion();
   std::pair<wifi_error, std::string> getFirmwareVersion();
@@ -293,6 +296,7 @@ class WifiLegacyHal {
   wifi_interface_handle wlan_interface_handle_;
   // Flag to indicate if we have initiated the cleanup of legacy HAL.
   std::atomic<bool> awaiting_event_loop_termination_;
+  std::condition_variable_any stop_wait_cv_;
   // Flag to indicate if the legacy HAL has been started.
   bool is_started_;
   wifi_system::InterfaceTool iface_tool_;