From e35e7e46bbcf4a8632b98b0ca726359feffbc3b8 Mon Sep 17 00:00:00 2001 From: Gabriel Biren Date: Tue, 14 Feb 2023 22:31:56 +0000 Subject: [PATCH 1/2] Remove DEBUG_ capabilities from ChipCapabilityMask. Also update the IWifiChip VTS test to reflect this change. Bug: 266521428 Test: atest VtsHalWifiChipTargetTest Change-Id: I537588c405d8a01091a6818020084acbe2679e14 --- .../android/hardware/wifi/IWifiChip.aidl | 40 +++++------ .../aidl/android/hardware/wifi/IWifiChip.aidl | 50 +++----------- .../vts/functional/wifi_chip_aidl_test.cpp | 68 ++++--------------- 3 files changed, 37 insertions(+), 121 deletions(-) diff --git a/wifi/aidl/aidl_api/android.hardware.wifi/current/android/hardware/wifi/IWifiChip.aidl b/wifi/aidl/aidl_api/android.hardware.wifi/current/android/hardware/wifi/IWifiChip.aidl index fd59888d56..6d0ddaa4f3 100644 --- a/wifi/aidl/aidl_api/android.hardware.wifi/current/android/hardware/wifi/IWifiChip.aidl +++ b/wifi/aidl/aidl_api/android.hardware.wifi/current/android/hardware/wifi/IWifiChip.aidl @@ -85,22 +85,14 @@ interface IWifiChip { const int NO_POWER_CAP_CONSTANT = 0x7FFFFFFF; @Backing(type="int") @VintfStability enum ChipCapabilityMask { - DEBUG_MEMORY_FIRMWARE_DUMP = (1 << 0), - DEBUG_MEMORY_DRIVER_DUMP = (1 << 1), - DEBUG_RING_BUFFER_CONNECT_EVENT = (1 << 2), - DEBUG_RING_BUFFER_POWER_EVENT = (1 << 3), - DEBUG_RING_BUFFER_WAKELOCK_EVENT = (1 << 4), - DEBUG_RING_BUFFER_VENDOR_DATA = (1 << 5), - DEBUG_HOST_WAKE_REASON_STATS = (1 << 6), - DEBUG_ERROR_ALERTS = (1 << 7), - SET_TX_POWER_LIMIT = (1 << 8), - D2D_RTT = (1 << 9), - D2AP_RTT = (1 << 10), - USE_BODY_HEAD_SAR = (1 << 11), - SET_LATENCY_MODE = (1 << 12), - P2P_RAND_MAC = (1 << 13), - WIGIG = (1 << 14), - SET_AFC_CHANNEL_ALLOWANCE = (1 << 15), + SET_TX_POWER_LIMIT = (1 << 0) /* 1 */, + D2D_RTT = (1 << 1) /* 2 */, + D2AP_RTT = (1 << 2) /* 4 */, + USE_BODY_HEAD_SAR = (1 << 3) /* 8 */, + SET_LATENCY_MODE = (1 << 4) /* 16 */, + P2P_RAND_MAC = (1 << 5) /* 32 */, + WIGIG = (1 << 6) /* 64 */, + SET_AFC_CHANNEL_ALLOWANCE = (1 << 7) /* 128 */, } @VintfStability parcelable ChipConcurrencyCombinationLimit { @@ -132,9 +124,9 @@ interface IWifiChip { } @Backing(type="int") @VintfStability enum CoexRestriction { - WIFI_DIRECT = (1 << 0), - SOFTAP = (1 << 1), - WIFI_AWARE = (1 << 2), + WIFI_DIRECT = (1 << 0) /* 1 */, + SOFTAP = (1 << 1) /* 2 */, + WIFI_AWARE = (1 << 2) /* 4 */, } @VintfStability parcelable CoexUnsafeChannel { @@ -162,13 +154,13 @@ interface IWifiChip { } @Backing(type="int") @VintfStability enum UsableChannelFilter { - CELLULAR_COEXISTENCE = (1 << 0), - CONCURRENCY = (1 << 1), - NAN_INSTANT_MODE = (1 << 2), + CELLULAR_COEXISTENCE = (1 << 0) /* 1 */, + CONCURRENCY = (1 << 1) /* 2 */, + NAN_INSTANT_MODE = (1 << 2) /* 4 */, } @Backing(type="int") @VintfStability enum ChannelCategoryMask { - INDOOR_CHANNEL = (1 << 0), - DFS_CHANNEL = (1 << 1), + INDOOR_CHANNEL = (1 << 0) /* 1 */, + DFS_CHANNEL = (1 << 1) /* 2 */, } } diff --git a/wifi/aidl/android/hardware/wifi/IWifiChip.aidl b/wifi/aidl/android/hardware/wifi/IWifiChip.aidl index 41ff7e68ae..8d415fdc68 100644 --- a/wifi/aidl/android/hardware/wifi/IWifiChip.aidl +++ b/wifi/aidl/android/hardware/wifi/IWifiChip.aidl @@ -45,72 +45,39 @@ interface IWifiChip { @VintfStability @Backing(type="int") enum ChipCapabilityMask { - /** - * Memory dump of Firmware. - */ - DEBUG_MEMORY_FIRMWARE_DUMP = 1 << 0, - /** - * Memory dump of Driver. - */ - DEBUG_MEMORY_DRIVER_DUMP = 1 << 1, - /** - * Connectivity events reported via debug ring buffer. - */ - DEBUG_RING_BUFFER_CONNECT_EVENT = 1 << 2, - /** - * Power events reported via debug ring buffer. - */ - DEBUG_RING_BUFFER_POWER_EVENT = 1 << 3, - /** - * Wakelock events reported via debug ring buffer. - */ - DEBUG_RING_BUFFER_WAKELOCK_EVENT = 1 << 4, - /** - * Vendor data reported via debug ring buffer. - * This mostly contains firmware event logs. - */ - DEBUG_RING_BUFFER_VENDOR_DATA = 1 << 5, - /** - * Host wake reasons stats collection. - */ - DEBUG_HOST_WAKE_REASON_STATS = 1 << 6, - /** - * Error alerts. - */ - DEBUG_ERROR_ALERTS = 1 << 7, /** * Set/Reset Tx Power limits. */ - SET_TX_POWER_LIMIT = 1 << 8, + SET_TX_POWER_LIMIT = 1 << 0, /** * Device to Device RTT. */ - D2D_RTT = 1 << 9, + D2D_RTT = 1 << 1, /** * Device to AP RTT. */ - D2AP_RTT = 1 << 10, + D2AP_RTT = 1 << 2, /** * Set/Reset Tx Power limits. */ - USE_BODY_HEAD_SAR = 1 << 11, + USE_BODY_HEAD_SAR = 1 << 3, /** * Set Latency Mode. */ - SET_LATENCY_MODE = 1 << 12, + SET_LATENCY_MODE = 1 << 4, /** * Support P2P MAC randomization. */ - P2P_RAND_MAC = 1 << 13, + P2P_RAND_MAC = 1 << 5, /** * Chip can operate in the 60GHz band (WiGig chip). */ - WIGIG = 1 << 14, + WIGIG = 1 << 6, /** * Chip supports setting allowed channels along with PSD in 6GHz band * for AFC purposes. */ - SET_AFC_CHANNEL_ALLOWANCE = 1 << 15, + SET_AFC_CHANNEL_ALLOWANCE = 1 << 7, } /** @@ -540,7 +507,6 @@ interface IWifiChip { * API to enable/disable alert notifications from the chip. * These alerts must be used to notify the framework of any fatal error events * that the chip encounters via |IWifiChipEventCallback.onDebugErrorAlert| method. - * Must fail if |ChipCapabilityMask.DEBUG_ERROR_ALERTS| is not set. * * @param enable true to enable, false to disable. * @throws ServiceSpecificException with one of the following values: diff --git a/wifi/aidl/vts/functional/wifi_chip_aidl_test.cpp b/wifi/aidl/vts/functional/wifi_chip_aidl_test.cpp index fec9122e07..f908be64c7 100644 --- a/wifi/aidl/vts/functional/wifi_chip_aidl_test.cpp +++ b/wifi/aidl/vts/functional/wifi_chip_aidl_test.cpp @@ -131,16 +131,6 @@ class WifiChipAidlTest : public testing::TestWithParam { return {iface1, iface2}; } - bool hasAnyRingBufferCapabilities(int32_t caps) { - return caps & - (static_cast( - IWifiChip::ChipCapabilityMask::DEBUG_RING_BUFFER_CONNECT_EVENT) | - static_cast(IWifiChip::ChipCapabilityMask::DEBUG_RING_BUFFER_POWER_EVENT) | - static_cast( - IWifiChip::ChipCapabilityMask::DEBUG_RING_BUFFER_WAKELOCK_EVENT) | - static_cast(IWifiChip::ChipCapabilityMask::DEBUG_RING_BUFFER_VENDOR_DATA)); - } - const char* getInstanceName() { return GetParam().c_str(); } std::shared_ptr wifi_chip_; @@ -452,14 +442,9 @@ TEST_P(WifiChipAidlTest, RequestChipDebugInfo) { */ TEST_P(WifiChipAidlTest, RequestFirmwareDebugDump) { configureChipForConcurrencyType(IfaceConcurrencyType::STA); - int32_t caps = getChipCapabilities(wifi_chip_); std::vector debug_dump; auto status = wifi_chip_->requestFirmwareDebugDump(&debug_dump); - if (caps & static_cast(IWifiChip::ChipCapabilityMask::DEBUG_MEMORY_FIRMWARE_DUMP)) { - EXPECT_TRUE(status.isOk()); - } else { - EXPECT_TRUE(checkStatusCode(&status, WifiStatusCode::ERROR_NOT_SUPPORTED)); - } + EXPECT_TRUE(status.isOk() || checkStatusCode(&status, WifiStatusCode::ERROR_NOT_SUPPORTED)); } /* @@ -467,14 +452,9 @@ TEST_P(WifiChipAidlTest, RequestFirmwareDebugDump) { */ TEST_P(WifiChipAidlTest, RequestDriverDebugDump) { configureChipForConcurrencyType(IfaceConcurrencyType::STA); - int32_t caps = getChipCapabilities(wifi_chip_); std::vector debug_dump; auto status = wifi_chip_->requestDriverDebugDump(&debug_dump); - if (caps & static_cast(IWifiChip::ChipCapabilityMask::DEBUG_MEMORY_DRIVER_DUMP)) { - EXPECT_TRUE(status.isOk()); - } else { - EXPECT_TRUE(checkStatusCode(&status, WifiStatusCode::ERROR_NOT_SUPPORTED)); - } + EXPECT_TRUE(status.isOk() || checkStatusCode(&status, WifiStatusCode::ERROR_NOT_SUPPORTED)); } /* @@ -482,17 +462,14 @@ TEST_P(WifiChipAidlTest, RequestDriverDebugDump) { */ TEST_P(WifiChipAidlTest, GetDebugRingBuffersStatus) { configureChipForConcurrencyType(IfaceConcurrencyType::STA); - int32_t caps = getChipCapabilities(wifi_chip_); std::vector ring_buffer_status; auto status = wifi_chip_->getDebugRingBuffersStatus(&ring_buffer_status); - if (hasAnyRingBufferCapabilities(caps)) { - EXPECT_TRUE(status.isOk()); + EXPECT_TRUE(status.isOk() || checkStatusCode(&status, WifiStatusCode::ERROR_NOT_SUPPORTED)); + if (status.isOk()) { ASSERT_NE(ring_buffer_status.size(), 0); for (const auto& ring_buffer : ring_buffer_status) { EXPECT_NE(ring_buffer.ringName.size(), 0); } - } else { - EXPECT_TRUE(checkStatusCode(&status, WifiStatusCode::ERROR_NOT_SUPPORTED)); } } @@ -501,14 +478,9 @@ TEST_P(WifiChipAidlTest, GetDebugRingBuffersStatus) { */ TEST_P(WifiChipAidlTest, GetDebugHostWakeReasonStats) { configureChipForConcurrencyType(IfaceConcurrencyType::STA); - int32_t caps = getChipCapabilities(wifi_chip_); WifiDebugHostWakeReasonStats wake_reason_stats = {}; auto status = wifi_chip_->getDebugHostWakeReasonStats(&wake_reason_stats); - if (caps & static_cast(IWifiChip::ChipCapabilityMask::DEBUG_HOST_WAKE_REASON_STATS)) { - EXPECT_TRUE(status.isOk()); - } else { - EXPECT_TRUE(checkStatusCode(&status, WifiStatusCode::ERROR_NOT_SUPPORTED)); - } + EXPECT_TRUE(status.isOk() || checkStatusCode(&status, WifiStatusCode::ERROR_NOT_SUPPORTED)); } /* @@ -516,26 +488,19 @@ TEST_P(WifiChipAidlTest, GetDebugHostWakeReasonStats) { */ TEST_P(WifiChipAidlTest, StartLoggingToDebugRingBuffer) { configureChipForConcurrencyType(IfaceConcurrencyType::STA); - int32_t caps = getChipCapabilities(wifi_chip_); std::string ring_name; std::vector ring_buffer_status; - auto status = wifi_chip_->getDebugRingBuffersStatus(&ring_buffer_status); - if (hasAnyRingBufferCapabilities(caps)) { - EXPECT_TRUE(status.isOk()); + auto status = wifi_chip_->getDebugRingBuffersStatus(&ring_buffer_status); + EXPECT_TRUE(status.isOk() || checkStatusCode(&status, WifiStatusCode::ERROR_NOT_SUPPORTED)); + if (status.isOk()) { ASSERT_NE(ring_buffer_status.size(), 0); ring_name = ring_buffer_status[0].ringName; - } else { - EXPECT_TRUE(checkStatusCode(&status, WifiStatusCode::ERROR_NOT_SUPPORTED)); } status = wifi_chip_->startLoggingToDebugRingBuffer( ring_name, WifiDebugRingBufferVerboseLevel::VERBOSE, 5, 1024); - if (hasAnyRingBufferCapabilities(caps)) { - EXPECT_TRUE(status.isOk()); - } else { - EXPECT_TRUE(checkStatusCode(&status, WifiStatusCode::ERROR_NOT_SUPPORTED)); - } + EXPECT_TRUE(status.isOk() || checkStatusCode(&status, WifiStatusCode::ERROR_NOT_SUPPORTED)); } /* @@ -543,25 +508,18 @@ TEST_P(WifiChipAidlTest, StartLoggingToDebugRingBuffer) { */ TEST_P(WifiChipAidlTest, ForceDumpToDebugRingBuffer) { configureChipForConcurrencyType(IfaceConcurrencyType::STA); - int32_t caps = getChipCapabilities(wifi_chip_); std::string ring_name; std::vector ring_buffer_status; - auto status = wifi_chip_->getDebugRingBuffersStatus(&ring_buffer_status); - if (hasAnyRingBufferCapabilities(caps)) { - EXPECT_TRUE(status.isOk()); + auto status = wifi_chip_->getDebugRingBuffersStatus(&ring_buffer_status); + EXPECT_TRUE(status.isOk() || checkStatusCode(&status, WifiStatusCode::ERROR_NOT_SUPPORTED)); + if (status.isOk()) { ASSERT_NE(ring_buffer_status.size(), 0); ring_name = ring_buffer_status[0].ringName; - } else { - EXPECT_TRUE(checkStatusCode(&status, WifiStatusCode::ERROR_NOT_SUPPORTED)); } status = wifi_chip_->forceDumpToDebugRingBuffer(ring_name); - if (hasAnyRingBufferCapabilities(caps)) { - EXPECT_TRUE(status.isOk()); - } else { - EXPECT_TRUE(checkStatusCode(&status, WifiStatusCode::ERROR_NOT_SUPPORTED)); - } + EXPECT_TRUE(status.isOk() || checkStatusCode(&status, WifiStatusCode::ERROR_NOT_SUPPORTED)); } /* From bff0e408dfc65320a264d5790b88a1f165d20778 Mon Sep 17 00:00:00 2001 From: Gabriel Biren Date: Tue, 14 Feb 2023 22:42:20 +0000 Subject: [PATCH 2/2] Remove any references to DEBUG_ capabilities in the Vendor HAL service. Bug: 266521428 Test: Vendor HAL gTest suite (see the modified test case) Change-Id: I86e0c465b9f9d2f31413d75727e36b42c2b6b88a --- wifi/aidl/default/aidl_struct_util.cpp | 38 +------------------ wifi/aidl/default/aidl_struct_util.h | 4 +- .../tests/aidl_struct_util_unit_tests.cpp | 12 ++---- wifi/aidl/default/wifi_chip.cpp | 4 +- 4 files changed, 7 insertions(+), 51 deletions(-) diff --git a/wifi/aidl/default/aidl_struct_util.cpp b/wifi/aidl/default/aidl_struct_util.cpp index 921b5dc38c..8463eac82a 100644 --- a/wifi/aidl/default/aidl_struct_util.cpp +++ b/wifi/aidl/default/aidl_struct_util.cpp @@ -41,23 +41,6 @@ inline std::vector uintToIntVec(const std::vector& in) { return std::vector(in.begin(), in.end()); } -IWifiChip::ChipCapabilityMask convertLegacyLoggerFeatureToAidlChipCapability(uint32_t feature) { - switch (feature) { - case legacy_hal::WIFI_LOGGER_MEMORY_DUMP_SUPPORTED: - return IWifiChip::ChipCapabilityMask::DEBUG_MEMORY_FIRMWARE_DUMP; - case legacy_hal::WIFI_LOGGER_DRIVER_DUMP_SUPPORTED: - return IWifiChip::ChipCapabilityMask::DEBUG_MEMORY_DRIVER_DUMP; - case legacy_hal::WIFI_LOGGER_CONNECT_EVENT_SUPPORTED: - return IWifiChip::ChipCapabilityMask::DEBUG_RING_BUFFER_CONNECT_EVENT; - case legacy_hal::WIFI_LOGGER_POWER_EVENT_SUPPORTED: - return IWifiChip::ChipCapabilityMask::DEBUG_RING_BUFFER_POWER_EVENT; - case legacy_hal::WIFI_LOGGER_WAKE_LOCK_SUPPORTED: - return IWifiChip::ChipCapabilityMask::DEBUG_RING_BUFFER_WAKELOCK_EVENT; - }; - CHECK(false) << "Unknown legacy feature: " << feature; - return {}; -} - IWifiStaIface::StaIfaceCapabilityMask convertLegacyLoggerFeatureToAidlStaIfaceCapability( uint32_t feature) { switch (feature) { @@ -125,23 +108,11 @@ IWifiStaIface::StaIfaceCapabilityMask convertLegacyFeatureToAidlStaIfaceCapabili return {}; } -bool convertLegacyFeaturesToAidlChipCapabilities(uint64_t legacy_feature_set, - uint32_t legacy_logger_feature_set, - uint32_t* aidl_caps) { +bool convertLegacyFeaturesToAidlChipCapabilities(uint64_t legacy_feature_set, uint32_t* aidl_caps) { if (!aidl_caps) { return false; } *aidl_caps = {}; - for (const auto feature : {legacy_hal::WIFI_LOGGER_MEMORY_DUMP_SUPPORTED, - legacy_hal::WIFI_LOGGER_DRIVER_DUMP_SUPPORTED, - legacy_hal::WIFI_LOGGER_CONNECT_EVENT_SUPPORTED, - legacy_hal::WIFI_LOGGER_POWER_EVENT_SUPPORTED, - legacy_hal::WIFI_LOGGER_WAKE_LOCK_SUPPORTED}) { - if (feature & legacy_logger_feature_set) { - *aidl_caps |= - static_cast(convertLegacyLoggerFeatureToAidlChipCapability(feature)); - } - } std::vector features = {WIFI_FEATURE_SET_TX_POWER_LIMIT, WIFI_FEATURE_USE_BODY_HEAD_SAR, WIFI_FEATURE_D2D_RTT, @@ -156,13 +127,6 @@ bool convertLegacyFeaturesToAidlChipCapabilities(uint64_t legacy_feature_set, } } - // There are no flags for these 3 in the legacy feature set. Adding them to - // the set because all the current devices support it. - *aidl_caps |= - static_cast(IWifiChip::ChipCapabilityMask::DEBUG_RING_BUFFER_VENDOR_DATA); - *aidl_caps |= - static_cast(IWifiChip::ChipCapabilityMask::DEBUG_HOST_WAKE_REASON_STATS); - *aidl_caps |= static_cast(IWifiChip::ChipCapabilityMask::DEBUG_ERROR_ALERTS); return true; } diff --git a/wifi/aidl/default/aidl_struct_util.h b/wifi/aidl/default/aidl_struct_util.h index 6407d321a0..df3785e511 100644 --- a/wifi/aidl/default/aidl_struct_util.h +++ b/wifi/aidl/default/aidl_struct_util.h @@ -37,9 +37,7 @@ namespace wifi { namespace aidl_struct_util { // Chip conversion methods. -bool convertLegacyFeaturesToAidlChipCapabilities(uint64_t legacy_feature_set, - uint32_t legacy_logger_feature_set, - uint32_t* aidl_caps); +bool convertLegacyFeaturesToAidlChipCapabilities(uint64_t legacy_feature_set, uint32_t* aidl_caps); bool convertLegacyDebugRingBufferStatusToAidl( const legacy_hal::wifi_ring_buffer_status& legacy_status, WifiDebugRingBufferStatus* aidl_status); diff --git a/wifi/aidl/default/tests/aidl_struct_util_unit_tests.cpp b/wifi/aidl/default/tests/aidl_struct_util_unit_tests.cpp index f97c846800..9997937ef2 100644 --- a/wifi/aidl/default/tests/aidl_struct_util_unit_tests.cpp +++ b/wifi/aidl/default/tests/aidl_struct_util_unit_tests.cpp @@ -661,18 +661,12 @@ TEST_F(AidlStructUtilTest, CanConvertLegacyFeaturesToAidl) { using AidlChipCaps = IWifiChip::ChipCapabilityMask; uint32_t aidl_caps; - uint32_t legacy_feature_set = WIFI_FEATURE_D2D_RTT | WIFI_FEATURE_SET_LATENCY_MODE; - uint32_t legacy_logger_feature_set = legacy_hal::WIFI_LOGGER_DRIVER_DUMP_SUPPORTED; - ASSERT_TRUE(aidl_struct_util::convertLegacyFeaturesToAidlChipCapabilities( - legacy_feature_set, legacy_logger_feature_set, &aidl_caps)); + ASSERT_TRUE(aidl_struct_util::convertLegacyFeaturesToAidlChipCapabilities(legacy_feature_set, + &aidl_caps)); - EXPECT_EQ((uint32_t)AidlChipCaps::DEBUG_RING_BUFFER_VENDOR_DATA | - (uint32_t)AidlChipCaps::DEBUG_HOST_WAKE_REASON_STATS | - (uint32_t)AidlChipCaps::DEBUG_ERROR_ALERTS | (uint32_t)AidlChipCaps::D2D_RTT | - (uint32_t)AidlChipCaps::SET_LATENCY_MODE | - (uint32_t)AidlChipCaps::DEBUG_MEMORY_DRIVER_DUMP, + EXPECT_EQ((uint32_t)AidlChipCaps::D2D_RTT | (uint32_t)AidlChipCaps::SET_LATENCY_MODE, aidl_caps); } diff --git a/wifi/aidl/default/wifi_chip.cpp b/wifi/aidl/default/wifi_chip.cpp index 541de166ce..41912b5d66 100644 --- a/wifi/aidl/default/wifi_chip.cpp +++ b/wifi/aidl/default/wifi_chip.cpp @@ -769,8 +769,8 @@ std::pair WifiChip::getCapabi legacy_logger_feature_set = 0; } uint32_t aidl_caps; - if (!aidl_struct_util::convertLegacyFeaturesToAidlChipCapabilities( - legacy_feature_set, legacy_logger_feature_set, &aidl_caps)) { + if (!aidl_struct_util::convertLegacyFeaturesToAidlChipCapabilities(legacy_feature_set, + &aidl_caps)) { return {IWifiChip::ChipCapabilityMask{}, createWifiStatus(WifiStatusCode::ERROR_UNKNOWN)}; } return {static_cast(aidl_caps), ndk::ScopedAStatus::ok()};