From 65b47f5f999c40c95010b235969503e4e2989566 Mon Sep 17 00:00:00 2001 From: Myles Watson Date: Thu, 26 Jan 2023 12:59:06 -0800 Subject: [PATCH] BluetoothHAL: Reset the chip on open(), close once Sending a reset and discarding extra packets helps ensure that tests are hermetic for devices that do not power off the chip with rfkill. Add a mutex to make sure that close only gets called once. Bug: 266221125 Test: atest VtsHalBluetoothTargetTest Change-Id: Ifb259a675202aa5399a8c7570ba8c4df2083e10e --- bluetooth/aidl/default/Android.bp | 7 + bluetooth/aidl/default/BluetoothHci.cpp | 123 +++++++++++++++++- bluetooth/aidl/default/BluetoothHci.h | 14 +- .../aidl/vts/VtsHalBluetoothTargetTest.cpp | 48 +++---- 4 files changed, 155 insertions(+), 37 deletions(-) diff --git a/bluetooth/aidl/default/Android.bp b/bluetooth/aidl/default/Android.bp index 3f4ba9903a..32d1a139e5 100644 --- a/bluetooth/aidl/default/Android.bp +++ b/bluetooth/aidl/default/Android.bp @@ -30,8 +30,15 @@ cc_library_static { defaults: ["android.hardware.bluetooth-service-build-defaults"], srcs: [ "BluetoothHci.cpp", + ":BluetoothPacketSources", "net_bluetooth_mgmt.cpp", ], + generated_headers: [ + "BluetoothGeneratedPackets_h", + ], + include_dirs: [ + "packages/modules/Bluetooth/system/gd", + ], } cc_binary { diff --git a/bluetooth/aidl/default/BluetoothHci.cpp b/bluetooth/aidl/default/BluetoothHci.cpp index eebbbc00d1..d4e4b34c6e 100644 --- a/bluetooth/aidl/default/BluetoothHci.cpp +++ b/bluetooth/aidl/default/BluetoothHci.cpp @@ -29,6 +29,11 @@ #include "log/log.h" +// TODO: Remove custom logging defines from PDL packets. +#undef LOG_INFO +#undef LOG_DEBUG +#include "hci/hci_packets.h" + namespace { int SetTerminalRaw(int fd) { termios terminal_settings; @@ -74,15 +79,22 @@ class BluetoothDeathRecipient { ALOGE("BluetoothDeathRecipient::serviceDied called but service not dead"); return; } - has_died_ = true; + { + std::lock_guard guard(mHasDiedMutex); + has_died_ = true; + } mHci->close(); } BluetoothHci* mHci; std::shared_ptr mCb; AIBinder_DeathRecipient* clientDeathRecipient_; - bool getHasDied() const { return has_died_; } + bool getHasDied() { + std::lock_guard guard(mHasDiedMutex); + return has_died_; + } private: + std::mutex mHasDiedMutex; bool has_died_{false}; }; @@ -114,16 +126,95 @@ int BluetoothHci::getFdFromDevPath() { return fd; } +void BluetoothHci::reset() { + // Send a reset command and wait until the command complete comes back. + + std::vector reset; + ::bluetooth::packet::BitInserter bi{reset}; + ::bluetooth::hci::ResetBuilder::Create()->Serialize(bi); + + auto resetPromise = std::make_shared>(); + auto resetFuture = resetPromise->get_future(); + + mH4 = std::make_shared( + mFd, + [](const std::vector& raw_command) { + ALOGI("Discarding %d bytes with command type", + static_cast(raw_command.size())); + }, + [](const std::vector& raw_acl) { + ALOGI("Discarding %d bytes with acl type", + static_cast(raw_acl.size())); + }, + [](const std::vector& raw_sco) { + ALOGI("Discarding %d bytes with sco type", + static_cast(raw_sco.size())); + }, + [resetPromise](const std::vector& raw_event) { + bool valid = ::bluetooth::hci::ResetCompleteView::Create( + ::bluetooth::hci::CommandCompleteView::Create( + ::bluetooth::hci::EventView::Create( + ::bluetooth::hci::PacketView( + std::make_shared>( + raw_event))))) + .IsValid(); + if (valid) { + resetPromise->set_value(); + } else { + ALOGI("Discarding %d bytes with event type", + static_cast(raw_event.size())); + } + }, + [](const std::vector& raw_iso) { + ALOGI("Discarding %d bytes with iso type", + static_cast(raw_iso.size())); + }, + [this]() { + ALOGI("HCI socket device disconnected while waiting for reset"); + mFdWatcher.StopWatchingFileDescriptors(); + }); + mFdWatcher.WatchFdForNonBlockingReads(mFd, + [this](int) { mH4->OnDataReady(); }); + + send(PacketType::COMMAND, reset); + auto status = resetFuture.wait_for(std::chrono::seconds(1)); + mFdWatcher.StopWatchingFileDescriptors(); + if (status == std::future_status::ready) { + ALOGI("HCI Reset successful"); + } else { + ALOGE("HCI Reset Response not received in one second"); + } + + resetPromise.reset(); +} + ndk::ScopedAStatus BluetoothHci::initialize( const std::shared_ptr& cb) { ALOGI(__func__); - mCb = cb; - if (mCb == nullptr) { + if (cb == nullptr) { ALOGE("cb == nullptr! -> Unable to call initializationComplete(ERR)"); return ndk::ScopedAStatus::fromServiceSpecificError(STATUS_BAD_VALUE); } + HalState old_state = HalState::READY; + { + std::lock_guard guard(mStateMutex); + if (mState != HalState::READY) { + old_state = mState; + } else { + mState = HalState::INITIALIZING; + } + } + + if (old_state != HalState::READY) { + ALOGE("initialize: Unexpected State %d", static_cast(old_state)); + close(); + cb->initializationComplete(Status::ALREADY_INITIALIZED); + return ndk::ScopedAStatus::ok(); + } + + mCb = cb; management_.reset(new NetBluetoothMgmt); mFd = management_->openHci(); if (mFd < 0) { @@ -132,12 +223,16 @@ ndk::ScopedAStatus BluetoothHci::initialize( ALOGI("Unable to open Linux interface, trying default path."); mFd = getFdFromDevPath(); if (mFd < 0) { - return ndk::ScopedAStatus::fromServiceSpecificError(STATUS_BAD_VALUE); + cb->initializationComplete(Status::UNABLE_TO_OPEN_INTERFACE); + return ndk::ScopedAStatus::ok(); } } mDeathRecipient->LinkToDeath(mCb); + // TODO: This should not be necessary when the device implements rfkill. + reset(); + mH4 = std::make_shared( mFd, [](const std::vector& /* raw_command */) { @@ -162,6 +257,10 @@ ndk::ScopedAStatus BluetoothHci::initialize( mFdWatcher.WatchFdForNonBlockingReads(mFd, [this](int) { mH4->OnDataReady(); }); + { + std::lock_guard guard(mStateMutex); + mState = HalState::ONE_CLIENT; + } ALOGI("initialization complete"); auto status = mCb->initializationComplete(Status::SUCCESS); if (!status.isOk()) { @@ -178,13 +277,27 @@ ndk::ScopedAStatus BluetoothHci::initialize( ndk::ScopedAStatus BluetoothHci::close() { ALOGI(__func__); + { + std::lock_guard guard(mStateMutex); + if (mState != HalState::ONE_CLIENT) { + ALOGI("Already closed"); + return ndk::ScopedAStatus::ok(); + } + mState = HalState::CLOSING; + } + mFdWatcher.StopWatchingFileDescriptors(); + if (management_) { management_->closeHci(); } else { ::close(mFd); } + { + std::lock_guard guard(mStateMutex); + mState = HalState::READY; + } return ndk::ScopedAStatus::ok(); } diff --git a/bluetooth/aidl/default/BluetoothHci.h b/bluetooth/aidl/default/BluetoothHci.h index a0908f840c..85aafc8197 100644 --- a/bluetooth/aidl/default/BluetoothHci.h +++ b/bluetooth/aidl/default/BluetoothHci.h @@ -18,8 +18,8 @@ #include #include -#include +#include #include #include "async_fd_watcher.h" @@ -69,6 +69,18 @@ class BluetoothHci : public BnBluetoothHci { void send(::android::hardware::bluetooth::hci::PacketType type, const std::vector& packet); std::unique_ptr management_{}; + + // Send a reset command and discard all packets until a reset is received. + void reset(); + + // Don't close twice or open before close is complete + std::mutex mStateMutex; + enum class HalState { + READY, + INITIALIZING, + ONE_CLIENT, + CLOSING, + } mState{HalState::READY}; }; } // namespace aidl::android::hardware::bluetooth::impl diff --git a/bluetooth/aidl/vts/VtsHalBluetoothTargetTest.cpp b/bluetooth/aidl/vts/VtsHalBluetoothTargetTest.cpp index 57a33616c5..7b9e211d5b 100644 --- a/bluetooth/aidl/vts/VtsHalBluetoothTargetTest.cpp +++ b/bluetooth/aidl/vts/VtsHalBluetoothTargetTest.cpp @@ -201,7 +201,6 @@ class BluetoothAidlTest : public ::testing::TestWithParam { void sendAndCheckAcl(int num_packets, size_t size, uint16_t handle); // Helper functions to try to get a handle on verbosity - void reset(); void enterLoopbackMode(); void handle_no_ops(); void discard_qca_debugging(); @@ -610,12 +609,15 @@ void BluetoothAidlTest::sendAndCheckAcl(int num_packets, size_t size, // Return the number of completed packets reported by the controller. int BluetoothAidlTest::wait_for_completed_packets_event(uint16_t handle) { int packets_processed = 0; - wait_for_event(false); - if (event_queue.empty()) { - ALOGW("%s: waitForBluetoothCallback timed out.", __func__); - return packets_processed; - } - while (!event_queue.empty()) { + while (true) { + // There should be at least one event. + wait_for_event(packets_processed == 0); + if (event_queue.empty()) { + if (packets_processed == 0) { + ALOGW("%s: waitForBluetoothCallback timed out.", __func__); + } + return packets_processed; + } std::vector event; EXPECT_TRUE(event_queue.pop(event)); @@ -630,15 +632,6 @@ int BluetoothAidlTest::wait_for_completed_packets_event(uint16_t handle) { return packets_processed; } -// Send the reset command and wait for a response. -void BluetoothAidlTest::reset() { - std::vector reset{kCommandHciReset, - kCommandHciReset + sizeof(kCommandHciReset)}; - hci->sendHciCommand(reset); - - wait_for_command_complete_event(reset); -} - // Send local loopback command and initialize SCO and ACL handles. void BluetoothAidlTest::enterLoopbackMode() { std::vector cmd{kCommandHciWriteLoopbackModeLocal, @@ -696,11 +689,16 @@ void BluetoothAidlTest::enterLoopbackMode() { TEST_P(BluetoothAidlTest, InitializeAndClose) {} // Send an HCI Reset with sendHciCommand and wait for a command complete event. -TEST_P(BluetoothAidlTest, HciReset) { reset(); } +TEST_P(BluetoothAidlTest, HciReset) { + std::vector reset{kCommandHciReset, + kCommandHciReset + sizeof(kCommandHciReset)}; + hci->sendHciCommand(reset); + + wait_for_command_complete_event(reset); +} // Read and check the HCI version of the controller. TEST_P(BluetoothAidlTest, HciVersionTest) { - reset(); std::vector cmd{kCommandHciReadLocalVersionInformation, kCommandHciReadLocalVersionInformation + sizeof(kCommandHciReadLocalVersionInformation)}; @@ -723,7 +721,6 @@ TEST_P(BluetoothAidlTest, HciVersionTest) { // Send an unknown HCI command and wait for the error message. TEST_P(BluetoothAidlTest, HciUnknownCommand) { - reset(); std::vector cmd{ kCommandHciShouldBeUnknown, kCommandHciShouldBeUnknown + sizeof(kCommandHciShouldBeUnknown)}; @@ -750,14 +747,10 @@ TEST_P(BluetoothAidlTest, HciUnknownCommand) { } // Enter loopback mode, but don't send any packets. -TEST_P(BluetoothAidlTest, WriteLoopbackMode) { - reset(); - enterLoopbackMode(); -} +TEST_P(BluetoothAidlTest, WriteLoopbackMode) { enterLoopbackMode(); } // Enter loopback mode and send a single command. TEST_P(BluetoothAidlTest, LoopbackModeSingleCommand) { - reset(); setBufferSizes(); enterLoopbackMode(); @@ -767,7 +760,6 @@ TEST_P(BluetoothAidlTest, LoopbackModeSingleCommand) { // Enter loopback mode and send a single SCO packet. TEST_P(BluetoothAidlTest, LoopbackModeSingleSco) { - reset(); setBufferSizes(); setSynchronousFlowControlEnable(); @@ -788,7 +780,6 @@ TEST_P(BluetoothAidlTest, LoopbackModeSingleSco) { // Enter loopback mode and send a single ACL packet. TEST_P(BluetoothAidlTest, LoopbackModeSingleAcl) { - reset(); setBufferSizes(); enterLoopbackMode(); @@ -810,7 +801,6 @@ TEST_P(BluetoothAidlTest, LoopbackModeSingleAcl) { // Enter loopback mode and send command packets for bandwidth measurements. TEST_P(BluetoothAidlTest, LoopbackModeCommandBandwidth) { - reset(); setBufferSizes(); enterLoopbackMode(); @@ -820,7 +810,6 @@ TEST_P(BluetoothAidlTest, LoopbackModeCommandBandwidth) { // Enter loopback mode and send SCO packets for bandwidth measurements. TEST_P(BluetoothAidlTest, LoopbackModeScoBandwidth) { - reset(); setBufferSizes(); setSynchronousFlowControlEnable(); @@ -842,7 +831,6 @@ TEST_P(BluetoothAidlTest, LoopbackModeScoBandwidth) { // Enter loopback mode and send packets for ACL bandwidth measurements. TEST_P(BluetoothAidlTest, LoopbackModeAclBandwidth) { - reset(); setBufferSizes(); enterLoopbackMode(); @@ -863,7 +851,6 @@ TEST_P(BluetoothAidlTest, LoopbackModeAclBandwidth) { // Set all bits in the event mask TEST_P(BluetoothAidlTest, SetEventMask) { - reset(); std::vector set_event_mask{ 0x01, 0x0c, 0x08 /*parameter bytes*/, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff}; @@ -873,7 +860,6 @@ TEST_P(BluetoothAidlTest, SetEventMask) { // Set all bits in the LE event mask TEST_P(BluetoothAidlTest, SetLeEventMask) { - reset(); std::vector set_event_mask{ 0x20, 0x0c, 0x08 /*parameter bytes*/, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff, 0xff};