diff --git a/wifi/1.1/default/hidl_return_util.h b/wifi/1.1/default/hidl_return_util.h index 2f95c23d7e..f36c8bda8a 100644 --- a/wifi/1.1/default/hidl_return_util.h +++ b/wifi/1.1/default/hidl_return_util.h @@ -55,6 +55,25 @@ Return 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 +Return validateAndCallWithLock( + ObjT* obj, + WifiStatusCode status_code_if_invalid, + WorkFuncT&& work, + const std::function& hidl_cb, + Args&&... args) { + auto lock = hidl_sync_util::acquireGlobalLock(); + if (obj->isValid()) { + hidl_cb((obj->*work)(&lock, std::forward(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 diff --git a/wifi/1.1/default/wifi.cpp b/wifi/1.1/default/wifi.cpp index 8456b90e9f..1741af44ba 100644 --- a/wifi/1.1/default/wifi.cpp +++ b/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 Wifi::start(start_cb hidl_status_cb) { } Return Wifi::stop(stop_cb hidl_status_cb) { - return validateAndCall( - this, WifiStatusCode::ERROR_UNKNOWN, &Wifi::stopInternal, hidl_status_cb); + return validateAndCallWithLock(this, WifiStatusCode::ERROR_UNKNOWN, + &Wifi::stopInternal, hidl_status_cb); } Return 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* 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* 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); diff --git a/wifi/1.1/default/wifi.h b/wifi/1.1/default/wifi.h index 1ade2d8b9b..3a64cbd490 100644 --- a/wifi/1.1/default/wifi.h +++ b/wifi/1.1/default/wifi.h @@ -61,12 +61,13 @@ class Wifi : public V1_1::IWifi { WifiStatus registerEventCallbackInternal( const sp& event_callback); WifiStatus startInternal(); - WifiStatus stopInternal(); + WifiStatus stopInternal(std::unique_lock* lock); std::pair> getChipIdsInternal(); std::pair> getChipInternal(ChipId chip_id); WifiStatus initializeLegacyHal(); - WifiStatus stopLegacyHalAndDeinitializeModeController(); + WifiStatus stopLegacyHalAndDeinitializeModeController( + std::unique_lock* lock); // Instance is created in this root level |IWifi| HIDL interface object // and shared with all the child HIDL interface objects. diff --git a/wifi/1.1/default/wifi_legacy_hal.cpp b/wifi/1.1/default/wifi_legacy_hal.cpp index 151a6009bb..a6f6971f38 100644 --- a/wifi/1.1/default/wifi_legacy_hal.cpp +++ b/wifi/1.1/default/wifi_legacy_hal.cpp @@ -15,6 +15,7 @@ */ #include +#include #include #include @@ -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 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 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* lock, const std::function& 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, - this](wifi_handle handle) { + on_stop_complete_internal_callback = + [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> diff --git a/wifi/1.1/default/wifi_legacy_hal.h b/wifi/1.1/default/wifi_legacy_hal.h index caa1bd50fc..549880398f 100644 --- a/wifi/1.1/default/wifi_legacy_hal.h +++ b/wifi/1.1/default/wifi_legacy_hal.h @@ -20,6 +20,7 @@ #include #include #include +#include #include @@ -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. - wifi_error stop(const std::function& on_complete_callback); + // Deinitialize the legacy HAL and wait for the event loop thread to exit + // using a predefined timeout. + wifi_error stop(std::unique_lock* lock, + const std::function& on_complete_callback); // Wrappers for all the functions in the legacy HAL function table. std::pair getDriverVersion(); std::pair 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 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_;