From 155344b8660975700905023032da42fda59632c7 Mon Sep 17 00:00:00 2001 From: Roshan Pius Date: Fri, 11 Aug 2017 15:47:42 -0700 Subject: [PATCH] wifi(implementation): Make WifiLegacyHal.stop() blocking IWifi::stop() is currently non-blocking which makes it hard for the client to determing when the stop is fully complete. This for example causes wificond to disable the wlan0 interface while the legacy HAL stop is being processed. So, add a timed wait to let the legacy HAL complete processing of the stop before we unblock the IWifi::stop() HIDL call. Bug: 64611487 Test: Manual tests by wifi state toggling and verifying the order of events in logs: 08-15 19:17:53.302 796 796 I android.hardware.wifi@1.0-service: Stopping legacy HAL 08-15 19:17:53.302 796 796 I WifiHAL : Sent msg on exit sock to unblock poll() 08-15 19:17:53.302 796 4793 E CLD80211: /vendor/bin/hw/android.hardware.wifi@1.0-service: Could not find group host_logs, errno: 0 id: -2 08-15 19:17:53.302 796 4793 E CLD80211: /vendor/bin/hw/android.hardware.wifi@1.0-service: Could not find group fw_logs, errno: 0 id: -2 08-15 19:17:53.302 796 4793 E CLD80211: /vendor/bin/hw/android.hardware.wifi@1.0-service: Could not find group per_pkt_stats, errno: 0 id: -2 08-15 19:17:53.302 796 4793 E CLD80211: /vendor/bin/hw/android.hardware.wifi@1.0-service: Could not find group diag_events, errno: 0 id: -2 08-15 19:17:53.302 796 4793 E CLD80211: /vendor/bin/hw/android.hardware.wifi@1.0-service: Could not find group fatal_events, errno: 0 id: -2 08-15 19:17:53.302 796 4793 I CLD80211: /vendor/bin/hw/android.hardware.wifi@1.0-service: Sent msg on exit sock to unblock poll() 08-15 19:17:53.302 796 4793 I android.hardware.wifi@1.0-service: Legacy HAL stop complete callback received 08-15 19:17:53.304 802 838 D CHRE : @ 151.328: [Platform] wifi: has 0, enabled 0 08-15 19:17:53.321 796 4793 I android.hardware.wifi@1.0-service: Legacy HAL event loop terminated 08-15 19:17:53.321 796 796 I android.hardware.wifi@1.0-service: Legacy HAL stop complete 08-15 19:17:53.522 796 796 I android.hardware.wifi@1.0-service: Wifi HAL stopped Test: Will send for regression tests. Change-Id: I394c11724e9459a4b9a6b970e2bcb4e0ad65fefc --- wifi/1.1/default/hidl_return_util.h | 19 +++++++++++++++++++ wifi/1.1/default/wifi.cpp | 15 +++++++++------ wifi/1.1/default/wifi.h | 5 +++-- wifi/1.1/default/wifi_legacy_hal.cpp | 22 ++++++++++++++++++---- wifi/1.1/default/wifi_legacy_hal.h | 8 ++++++-- 5 files changed, 55 insertions(+), 14 deletions(-) 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_;