From: Roshan Pius Date: Tue, 31 Jan 2017 00:40:50 +0000 (-0800) Subject: wifi(implementation): Add global lock X-Git-Url: http://git.osdn.net/view?a=commitdiff_plain;h=091e1c1f3abac6c3358263ce0c09475f31b3ae68;p=android-x86%2Fhardware-interfaces.git wifi(implementation): Add global lock Add a global lock to address synchronization issues between the main HIDL method servicing thread and the legacy HAL's event loop thread. Also, added some documentation for the threading model used. Bug: 34261034 Test: Compiles Change-Id: I881111814ff5ebd601d6a4c85cf284b30ae47ed3 --- diff --git a/wifi/1.0/default/Android.mk b/wifi/1.0/default/Android.mk index 144c067b..ac484b62 100644 --- a/wifi/1.0/default/Android.mk +++ b/wifi/1.0/default/Android.mk @@ -19,6 +19,7 @@ LOCAL_MODULE_RELATIVE_PATH := hw LOCAL_CPPFLAGS := -Wall -Werror -Wextra LOCAL_SRC_FILES := \ hidl_struct_util.cpp \ + hidl_sync_util.cpp \ service.cpp \ wifi.cpp \ wifi_ap_iface.cpp \ diff --git a/wifi/1.0/default/THREADING.README b/wifi/1.0/default/THREADING.README new file mode 100644 index 00000000..8366ca02 --- /dev/null +++ b/wifi/1.0/default/THREADING.README @@ -0,0 +1,35 @@ +Vendor HAL Threading Model +========================== +The vendor HAL service has two threads: +1. HIDL thread: This is the main thread which processes all the incoming HIDL +RPC's. +2. Legacy HAL event loop thread: This is the thread forked off for processing +the legacy HAL event loop (wifi_event_loop()). This thread is used to process +any asynchronous netlink events posted by the driver. Any asynchronous +callbacks passed to the legacy HAL API's are invoked on this thread. + +Synchronization Concerns +======================== +wifi_legacy_hal.cpp has a bunch of global "C" style functions to handle the +legacy callbacks. Each of these "C" style function invokes a corresponding +"std::function" version of the callback which does the actual processing. +The variables holding these "std::function" callbacks are reset from the HIDL +thread when they are no longer used. For example: stopGscan() will reset the +corresponding "on_gscan_*" callback variables which were set when startGscan() +was invoked. This is not thread safe since these callback variables are +accesed from the legacy hal event loop thread as well. + +Synchronization Solution +======================== +Adding a global lock seems to be the most trivial solution to the problem. +a) All of the asynchronous "C" style callbacks will acquire the global lock +before invoking the corresponding "std::function" callback variables. +b) All of the HIDL methods will also acquire the global lock before processing +(in hidl_return_util::validateAndCall()). + +Note: It's important that we only acquire the global lock for asynchronous +callbacks, because there is no guarantee (or documentation to clarify) that the +synchronous callbacks are invoked on the same invocation thread. If that is not +the case in some implementation, we will end up deadlocking the system since the +HIDL thread would have acquired the global lock which is needed by the +synchronous callback executed on the legacy hal event loop thread. diff --git a/wifi/1.0/default/hidl_return_util.h b/wifi/1.0/default/hidl_return_util.h index 29861655..3f6364be 100644 --- a/wifi/1.0/default/hidl_return_util.h +++ b/wifi/1.0/default/hidl_return_util.h @@ -17,6 +17,7 @@ #ifndef HIDL_RETURN_UTIL_H_ #define HIDL_RETURN_UTIL_H_ +#include "hidl_sync_util.h" #include "wifi_status_util.h" namespace android { @@ -44,6 +45,7 @@ Return validateAndCall( WorkFuncT&& work, const std::function& hidl_cb, Args&&... args) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (obj->isValid()) { hidl_cb((obj->*work)(std::forward(args)...)); } else { @@ -61,6 +63,7 @@ Return validateAndCall( WorkFuncT&& work, const std::function& hidl_cb, Args&&... args) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (obj->isValid()) { const auto& ret_pair = (obj->*work)(std::forward(args)...); const WifiStatus& status = std::get<0>(ret_pair); @@ -86,6 +89,7 @@ Return validateAndCall( WorkFuncT&& work, const std::function& hidl_cb, Args&&... args) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (obj->isValid()) { const auto& ret_tuple = (obj->*work)(std::forward(args)...); const WifiStatus& status = std::get<0>(ret_tuple); diff --git a/wifi/1.0/default/hidl_sync_util.cpp b/wifi/1.0/default/hidl_sync_util.cpp new file mode 100644 index 00000000..7d47f2f7 --- /dev/null +++ b/wifi/1.0/default/hidl_sync_util.cpp @@ -0,0 +1,39 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "hidl_sync_util.h" + +namespace { +std::recursive_mutex g_mutex; +} // namespace + +namespace android { +namespace hardware { +namespace wifi { +namespace V1_0 { +namespace implementation { +namespace hidl_sync_util { + +std::unique_lock acquireGlobalLock() { + return std::unique_lock{g_mutex}; +} + +} // namespace hidl_sync_util +} // namespace implementation +} // namespace V1_0 +} // namespace wifi +} // namespace hardware +} // namespace android diff --git a/wifi/1.0/default/hidl_sync_util.h b/wifi/1.0/default/hidl_sync_util.h new file mode 100644 index 00000000..6631e55b --- /dev/null +++ b/wifi/1.0/default/hidl_sync_util.h @@ -0,0 +1,37 @@ +/* + * Copyright (C) 2016 The Android Open Source Project + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef HIDL_SYNC_UTIL_H_ +#define HIDL_SYNC_UTIL_H_ + +#include + +// Utility that provides a global lock to synchronize access between +// the HIDL thread and the legacy HAL's event loop. +namespace android { +namespace hardware { +namespace wifi { +namespace V1_0 { +namespace implementation { +namespace hidl_sync_util { +std::unique_lock acquireGlobalLock(); +} // namespace hidl_sync_util +} // namespace implementation +} // namespace V1_0 +} // namespace wifi +} // namespace hardware +} // namespace android +#endif // HIDL_SYNC_UTIL_H_ diff --git a/wifi/1.0/default/wifi_legacy_hal.cpp b/wifi/1.0/default/wifi_legacy_hal.cpp index b0b0f963..40f55a3d 100644 --- a/wifi/1.0/default/wifi_legacy_hal.cpp +++ b/wifi/1.0/default/wifi_legacy_hal.cpp @@ -19,6 +19,7 @@ #include #include +#include "hidl_sync_util.h" #include "wifi_legacy_hal.h" #include "wifi_legacy_hal_stubs.h" @@ -54,7 +55,8 @@ namespace legacy_hal { // std::function methods to be invoked. // Callback to be invoked once |stop| is complete. std::function on_stop_complete_internal_callback; -void onStopComplete(wifi_handle handle) { +void onAsyncStopComplete(wifi_handle handle) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_stop_complete_internal_callback) { on_stop_complete_internal_callback(handle); } @@ -62,7 +64,7 @@ void onStopComplete(wifi_handle handle) { // Callback to be invoked for driver dump. std::function on_driver_memory_dump_internal_callback; -void onDriverMemoryDump(char* buffer, int buffer_size) { +void onSyncDriverMemoryDump(char* buffer, int buffer_size) { if (on_driver_memory_dump_internal_callback) { on_driver_memory_dump_internal_callback(buffer, buffer_size); } @@ -70,7 +72,7 @@ void onDriverMemoryDump(char* buffer, int buffer_size) { // Callback to be invoked for firmware dump. std::function on_firmware_memory_dump_internal_callback; -void onFirmwareMemoryDump(char* buffer, int buffer_size) { +void onSyncFirmwareMemoryDump(char* buffer, int buffer_size) { if (on_firmware_memory_dump_internal_callback) { on_firmware_memory_dump_internal_callback(buffer, buffer_size); } @@ -79,7 +81,8 @@ void onFirmwareMemoryDump(char* buffer, int buffer_size) { // Callback to be invoked for Gscan events. std::function on_gscan_event_internal_callback; -void onGscanEvent(wifi_request_id id, wifi_scan_event event) { +void onAsyncGscanEvent(wifi_request_id id, wifi_scan_event event) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_gscan_event_internal_callback) { on_gscan_event_internal_callback(id, event); } @@ -88,9 +91,10 @@ void onGscanEvent(wifi_request_id id, wifi_scan_event event) { // Callback to be invoked for Gscan full results. std::function on_gscan_full_result_internal_callback; -void onGscanFullResult(wifi_request_id id, - wifi_scan_result* result, - uint32_t buckets_scanned) { +void onAsyncGscanFullResult(wifi_request_id id, + wifi_scan_result* result, + uint32_t buckets_scanned) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_gscan_full_result_internal_callback) { on_gscan_full_result_internal_callback(id, result, buckets_scanned); } @@ -99,10 +103,10 @@ void onGscanFullResult(wifi_request_id id, // Callback to be invoked for link layer stats results. std::function on_link_layer_stats_result_internal_callback; -void onLinkLayerStatsResult(wifi_request_id id, - wifi_iface_stat* iface_stat, - int num_radios, - wifi_radio_stat* radio_stat) { +void onSyncLinkLayerStatsResult(wifi_request_id id, + wifi_iface_stat* iface_stat, + int num_radios, + wifi_radio_stat* radio_stat) { if (on_link_layer_stats_result_internal_callback) { on_link_layer_stats_result_internal_callback( id, iface_stat, num_radios, radio_stat); @@ -112,7 +116,10 @@ void onLinkLayerStatsResult(wifi_request_id id, // Callback to be invoked for rssi threshold breach. std::function on_rssi_threshold_breached_internal_callback; -void onRssiThresholdBreached(wifi_request_id id, uint8_t* bssid, int8_t rssi) { +void onAsyncRssiThresholdBreached(wifi_request_id id, + uint8_t* bssid, + int8_t rssi) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_rssi_threshold_breached_internal_callback) { on_rssi_threshold_breached_internal_callback(id, bssid, rssi); } @@ -121,10 +128,11 @@ void onRssiThresholdBreached(wifi_request_id id, uint8_t* bssid, int8_t rssi) { // Callback to be invoked for ring buffer data indication. std::function on_ring_buffer_data_internal_callback; -void onRingBufferData(char* ring_name, - char* buffer, - int buffer_size, - wifi_ring_buffer_status* status) { +void onAsyncRingBufferData(char* ring_name, + char* buffer, + int buffer_size, + wifi_ring_buffer_status* status) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_ring_buffer_data_internal_callback) { on_ring_buffer_data_internal_callback( ring_name, buffer, buffer_size, status); @@ -134,10 +142,11 @@ void onRingBufferData(char* ring_name, // Callback to be invoked for error alert indication. std::function on_error_alert_internal_callback; -void onErrorAlert(wifi_request_id id, - char* buffer, - int buffer_size, - int err_code) { +void onAsyncErrorAlert(wifi_request_id id, + char* buffer, + int buffer_size, + int err_code) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_error_alert_internal_callback) { on_error_alert_internal_callback(id, buffer, buffer_size, err_code); } @@ -147,9 +156,10 @@ void onErrorAlert(wifi_request_id id, std::function on_rtt_results_internal_callback; -void onRttResults(wifi_request_id id, - unsigned num_results, - wifi_rtt_result* rtt_results[]) { +void onAsyncRttResults(wifi_request_id id, + unsigned num_results, + wifi_rtt_result* rtt_results[]) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_rtt_results_internal_callback) { on_rtt_results_internal_callback(id, num_results, rtt_results); } @@ -161,7 +171,8 @@ void onRttResults(wifi_request_id id, // So, handle all of them here directly to avoid adding an unnecessary layer. std::function on_nan_notify_response_user_callback; -void onNanNotifyResponse(transaction_id id, NanResponseMsg* msg) { +void onAysncNanNotifyResponse(transaction_id id, NanResponseMsg* msg) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_nan_notify_response_user_callback && msg) { on_nan_notify_response_user_callback(id, *msg); } @@ -169,14 +180,16 @@ void onNanNotifyResponse(transaction_id id, NanResponseMsg* msg) { std::function on_nan_event_publish_terminated_user_callback; -void onNanEventPublishTerminated(NanPublishTerminatedInd* event) { +void onAysncNanEventPublishTerminated(NanPublishTerminatedInd* event) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_nan_event_publish_terminated_user_callback && event) { on_nan_event_publish_terminated_user_callback(*event); } } std::function on_nan_event_match_user_callback; -void onNanEventMatch(NanMatchInd* event) { +void onAysncNanEventMatch(NanMatchInd* event) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_nan_event_match_user_callback && event) { on_nan_event_match_user_callback(*event); } @@ -184,7 +197,8 @@ void onNanEventMatch(NanMatchInd* event) { std::function on_nan_event_match_expired_user_callback; -void onNanEventMatchExpired(NanMatchExpiredInd* event) { +void onAysncNanEventMatchExpired(NanMatchExpiredInd* event) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_nan_event_match_expired_user_callback && event) { on_nan_event_match_expired_user_callback(*event); } @@ -192,14 +206,16 @@ void onNanEventMatchExpired(NanMatchExpiredInd* event) { std::function on_nan_event_subscribe_terminated_user_callback; -void onNanEventSubscribeTerminated(NanSubscribeTerminatedInd* event) { +void onAysncNanEventSubscribeTerminated(NanSubscribeTerminatedInd* event) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_nan_event_subscribe_terminated_user_callback && event) { on_nan_event_subscribe_terminated_user_callback(*event); } } std::function on_nan_event_followup_user_callback; -void onNanEventFollowup(NanFollowupInd* event) { +void onAysncNanEventFollowup(NanFollowupInd* event) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_nan_event_followup_user_callback && event) { on_nan_event_followup_user_callback(*event); } @@ -207,21 +223,24 @@ void onNanEventFollowup(NanFollowupInd* event) { std::function on_nan_event_disc_eng_event_user_callback; -void onNanEventDiscEngEvent(NanDiscEngEventInd* event) { +void onAysncNanEventDiscEngEvent(NanDiscEngEventInd* event) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_nan_event_disc_eng_event_user_callback && event) { on_nan_event_disc_eng_event_user_callback(*event); } } std::function on_nan_event_disabled_user_callback; -void onNanEventDisabled(NanDisabledInd* event) { +void onAysncNanEventDisabled(NanDisabledInd* event) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_nan_event_disabled_user_callback && event) { on_nan_event_disabled_user_callback(*event); } } std::function on_nan_event_tca_user_callback; -void onNanEventTca(NanTCAInd* event) { +void onAysncNanEventTca(NanTCAInd* event) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_nan_event_tca_user_callback && event) { on_nan_event_tca_user_callback(*event); } @@ -229,7 +248,8 @@ void onNanEventTca(NanTCAInd* event) { std::function on_nan_event_beacon_sdf_payload_user_callback; -void onNanEventBeaconSdfPayload(NanBeaconSdfPayloadInd* event) { +void onAysncNanEventBeaconSdfPayload(NanBeaconSdfPayloadInd* event) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_nan_event_beacon_sdf_payload_user_callback && event) { on_nan_event_beacon_sdf_payload_user_callback(*event); } @@ -237,14 +257,16 @@ void onNanEventBeaconSdfPayload(NanBeaconSdfPayloadInd* event) { std::function on_nan_event_data_path_request_user_callback; -void onNanEventDataPathRequest(NanDataPathRequestInd* event) { +void onAysncNanEventDataPathRequest(NanDataPathRequestInd* event) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_nan_event_data_path_request_user_callback && event) { on_nan_event_data_path_request_user_callback(*event); } } std::function on_nan_event_data_path_confirm_user_callback; -void onNanEventDataPathConfirm(NanDataPathConfirmInd* event) { +void onAysncNanEventDataPathConfirm(NanDataPathConfirmInd* event) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_nan_event_data_path_confirm_user_callback && event) { on_nan_event_data_path_confirm_user_callback(*event); } @@ -252,7 +274,8 @@ void onNanEventDataPathConfirm(NanDataPathConfirmInd* event) { std::function on_nan_event_data_path_end_user_callback; -void onNanEventDataPathEnd(NanDataPathEndInd* event) { +void onAysncNanEventDataPathEnd(NanDataPathEndInd* event) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_nan_event_data_path_end_user_callback && event) { on_nan_event_data_path_end_user_callback(*event); } @@ -260,7 +283,8 @@ void onNanEventDataPathEnd(NanDataPathEndInd* event) { std::function on_nan_event_transmit_follow_up_user_callback; -void onNanEventTransmitFollowUp(NanTransmitFollowupInd* event) { +void onAysncNanEventTransmitFollowUp(NanTransmitFollowupInd* event) { + const auto lock = hidl_sync_util::acquireGlobalLock(); if (on_nan_event_transmit_follow_up_user_callback && event) { on_nan_event_transmit_follow_up_user_callback(*event); } @@ -335,7 +359,7 @@ wifi_error WifiLegacyHal::stop( on_stop_complete_user_callback(); }; awaiting_event_loop_termination_ = true; - global_func_table_.wifi_cleanup(global_handle_, onStopComplete); + global_func_table_.wifi_cleanup(global_handle_, onAsyncStopComplete); LOG(DEBUG) << "Legacy HAL stop complete"; is_started_ = false; return WIFI_SUCCESS; @@ -391,7 +415,7 @@ WifiLegacyHal::requestDriverMemoryDump() { reinterpret_cast(buffer) + buffer_size); }; wifi_error status = global_func_table_.wifi_get_driver_memory_dump( - wlan_interface_handle_, {onDriverMemoryDump}); + wlan_interface_handle_, {onSyncDriverMemoryDump}); on_driver_memory_dump_internal_callback = nullptr; return {status, std::move(driver_dump)}; } @@ -406,7 +430,7 @@ WifiLegacyHal::requestFirmwareMemoryDump() { reinterpret_cast(buffer) + buffer_size); }; wifi_error status = global_func_table_.wifi_get_firmware_memory_dump( - wlan_interface_handle_, {onFirmwareMemoryDump}); + wlan_interface_handle_, {onSyncFirmwareMemoryDump}); on_firmware_memory_dump_internal_callback = nullptr; return {status, std::move(firmware_dump)}; } @@ -488,7 +512,8 @@ wifi_error WifiLegacyHal::startGscan( } }; - wifi_scan_result_handler handler = {onGscanFullResult, onGscanEvent}; + wifi_scan_result_handler handler = {onAsyncGscanFullResult, + onAsyncGscanEvent}; wifi_error status = global_func_table_.wifi_start_gscan( id, wlan_interface_handle_, params, handler); if (status != WIFI_SUCCESS) { @@ -584,7 +609,7 @@ std::pair WifiLegacyHal::getLinkLayerStats() { }; wifi_error status = global_func_table_.wifi_get_link_stats( - 0, wlan_interface_handle_, {onLinkLayerStatsResult}); + 0, wlan_interface_handle_, {onSyncLinkLayerStatsResult}); on_link_layer_stats_result_internal_callback = nullptr; return {status, link_stats}; } @@ -609,12 +634,12 @@ wifi_error WifiLegacyHal::startRssiMonitoring( std::copy(bssid_ptr, bssid_ptr + 6, std::begin(bssid_arr)); on_threshold_breached_user_callback(id, bssid_arr, rssi); }; - wifi_error status = - global_func_table_.wifi_start_rssi_monitoring(id, - wlan_interface_handle_, - max_rssi, - min_rssi, - {onRssiThresholdBreached}); + wifi_error status = global_func_table_.wifi_start_rssi_monitoring( + id, + wlan_interface_handle_, + max_rssi, + min_rssi, + {onAsyncRssiThresholdBreached}); if (status != WIFI_SUCCESS) { on_rssi_threshold_breached_internal_callback = nullptr; } @@ -789,7 +814,7 @@ wifi_error WifiLegacyHal::registerRingBufferCallbackHandler( } }; wifi_error status = global_func_table_.wifi_set_log_handler( - 0, wlan_interface_handle_, {onRingBufferData}); + 0, wlan_interface_handle_, {onAsyncRingBufferData}); if (status != WIFI_SUCCESS) { on_ring_buffer_data_internal_callback = nullptr; } @@ -850,7 +875,7 @@ wifi_error WifiLegacyHal::registerErrorAlertCallbackHandler( } }; wifi_error status = global_func_table_.wifi_set_alert_handler( - 0, wlan_interface_handle_, {onErrorAlert}); + 0, wlan_interface_handle_, {onAsyncErrorAlert}); if (status != WIFI_SUCCESS) { on_error_alert_internal_callback = nullptr; } @@ -896,7 +921,7 @@ wifi_error WifiLegacyHal::startRttRangeRequest( wlan_interface_handle_, rtt_configs.size(), rtt_configs_internal.data(), - {onRttResults}); + {onAsyncRttResults}); if (status != WIFI_SUCCESS) { on_rtt_results_internal_callback = nullptr; } @@ -1000,20 +1025,20 @@ wifi_error WifiLegacyHal::nanRegisterCallbackHandlers( return global_func_table_.wifi_nan_register_handler( wlan_interface_handle_, - {onNanNotifyResponse, - onNanEventPublishTerminated, - onNanEventMatch, - onNanEventMatchExpired, - onNanEventSubscribeTerminated, - onNanEventFollowup, - onNanEventDiscEngEvent, - onNanEventDisabled, - onNanEventTca, - onNanEventBeaconSdfPayload, - onNanEventDataPathRequest, - onNanEventDataPathConfirm, - onNanEventDataPathEnd, - onNanEventTransmitFollowUp}); + {onAysncNanNotifyResponse, + onAysncNanEventPublishTerminated, + onAysncNanEventMatch, + onAysncNanEventMatchExpired, + onAysncNanEventSubscribeTerminated, + onAysncNanEventFollowup, + onAysncNanEventDiscEngEvent, + onAysncNanEventDisabled, + onAysncNanEventTca, + onAysncNanEventBeaconSdfPayload, + onAysncNanEventDataPathRequest, + onAysncNanEventDataPathConfirm, + onAysncNanEventDataPathEnd, + onAysncNanEventTransmitFollowUp}); } wifi_error WifiLegacyHal::nanEnableRequest(transaction_id id,