Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit 4820362a authored by Pavlin Radoslavov's avatar Pavlin Radoslavov Committed by Myles Watson
Browse files

Fix A2DP Suspend related multi-component deadlock

Apparently, the processing of A2DP Suspend from the Audio HAL
could result in a deadlock across multiple components:
  A2dpService -> AudioService  -> Audio-BT HAL -> BT Native Stack ->
  BT JNI upcall -> A2dpService

1) A2dpService -> AudioService:
   - Waiting inside setActiveDevice() -> synchronized (mStateMachines) ->
   mAudioManager.setBluetoothA2dpDeviceConnectionStateSuppressNoisyIntent()
   - The mAudioManager call can be blocking: a followup call would wait
     for the previous call to complete.
2) AudioService -> Audio-BT HAL
   The AudioService is wating on the Audio-BT HAL to complete its request.
   In this case the request is SUSPEND
3) Audio-BT HAL -> BT Native Stack
   The Audio side of the Audio-BT HAL is waiting for the BT Native stack
   to ACK the SUSPEND command.
   More specifically, it is retrying few times with timeout of 250ms
   each iteration.

4) BT Native Stack -> BT JNI upcall
   - In the BT Native Stack the BtifAv FSM (see file btif/src/btif_av.cc)
     is in state StateStarted and has received the
     BTIF_AV_SUSPEND_STREAM_REQ_EVT because of the SUSPEND command from the
     Audio HAL.
   - As a result, the LocalSuspendPending flag has been set:
     peer_.SetFlags(BtifAvPeer::kFlagLocalSuspendPending);
   - Only after the above flag is reset, then the BT Native Stack will
     ACK the SUSPEND command from the HAL
   - The above flag will be reset when event BTA_AV_SUSPEND_EVT is received
     from the underlying stack
   - The BtifAv state machine has received the BTA_AV_SUSPEND_EVT event.
     However, prior to clearing the pending status, the FSM tries to
     report the audio state via JNI to the Java layer:
       btif_report_audio_state();
       ...
       peer_.ClearFlags(BtifAvPeer::kFlagLocalSuspendPending);
  - Apparently, all the processing inside the BtifAv FSM is done on
    the JNI thread, including the btif_report_audio_state() upcall.
5) BT JNI upcall -> A2dpService
  - The btif_report_audio_state() upcall calls indirectly
    A2dpNativeInterface.onAudioStateChanged() -> sendMessageToService() ->
    A2dpService.messageFromNative()
  - Within the A2dpService.messageFromNative() processing, there is
    "synchronized (mStateMachines) {}" block, and the processing there
    blocks because per (1) above A2dpService.setActiveDevice() is
    already inside its own "synchronized (mStateMachines) {}" block

The above deadlock is fixed by the following:
 - Modified the BTA_AV_SUSPEND_EVT processing such that
   the clearing of the kFlagLocalSuspendPending flag is done BEFORE
   the btif_report_audio_state()
 - Modified the BtifAv FSM such that the internal processing of the FSM
   events is done on the BTA thread instead of the JNI thread. Thus,
   the FSM processing cannot be blocked by JNI upcalls.

Also:
 - Updated the do_in_bta_thread() function to have a return value
   so it is more aligned with the corresponding do_in_jni_thread()
   implementation
 - Updated the reordering of some other btif_report_*() upcalls inside
   btif_av.cc so they are after the FSM internal processing in the
   current state.
 - Added few extra logs to the Audio-BT HAL (file audio_a2dp_hw.cc).

Bug: 72823323
Test: Disconnect/reconnect with Philips SBH7000 Headset
Change-Id: I7ccb558e458a8e90d0414f94463bda8d4b3fdc97
parent 60e27d55
Loading
Loading
Loading
Loading
+10 −4
Original line number Original line Diff line number Diff line
@@ -427,10 +427,12 @@ static int a2dp_command(struct a2dp_stream_common* common, tA2DP_CTRL_CMD cmd) {
  DEBUG("A2DP COMMAND %s", audio_a2dp_hw_dump_ctrl_event(cmd));
  DEBUG("A2DP COMMAND %s", audio_a2dp_hw_dump_ctrl_event(cmd));


  if (common->ctrl_fd == AUDIO_SKT_DISCONNECTED) {
  if (common->ctrl_fd == AUDIO_SKT_DISCONNECTED) {
    INFO("starting up or recovering from previous error");
    INFO("starting up or recovering from previous error: command=%s",
         audio_a2dp_hw_dump_ctrl_event(cmd));
    a2dp_open_ctrl_path(common);
    a2dp_open_ctrl_path(common);
    if (common->ctrl_fd == AUDIO_SKT_DISCONNECTED) {
    if (common->ctrl_fd == AUDIO_SKT_DISCONNECTED) {
      ERROR("failure to open ctrl path");
      ERROR("failure to open ctrl path: command=%s",
            audio_a2dp_hw_dump_ctrl_event(cmd));
      return -1;
      return -1;
    }
    }
  }
  }
@@ -439,7 +441,8 @@ static int a2dp_command(struct a2dp_stream_common* common, tA2DP_CTRL_CMD cmd) {
  ssize_t sent;
  ssize_t sent;
  OSI_NO_INTR(sent = send(common->ctrl_fd, &cmd, 1, MSG_NOSIGNAL));
  OSI_NO_INTR(sent = send(common->ctrl_fd, &cmd, 1, MSG_NOSIGNAL));
  if (sent == -1) {
  if (sent == -1) {
    ERROR("cmd failed (%s)", strerror(errno));
    ERROR("cmd failed (%s): command=%s", strerror(errno),
          audio_a2dp_hw_dump_ctrl_event(cmd));
    skt_disconnect(common->ctrl_fd);
    skt_disconnect(common->ctrl_fd);
    common->ctrl_fd = AUDIO_SKT_DISCONNECTED;
    common->ctrl_fd = AUDIO_SKT_DISCONNECTED;
    return -1;
    return -1;
@@ -454,7 +457,10 @@ static int a2dp_command(struct a2dp_stream_common* common, tA2DP_CTRL_CMD cmd) {
  DEBUG("A2DP COMMAND %s DONE STATUS %d", audio_a2dp_hw_dump_ctrl_event(cmd),
  DEBUG("A2DP COMMAND %s DONE STATUS %d", audio_a2dp_hw_dump_ctrl_event(cmd),
        ack);
        ack);


  if (ack == A2DP_CTRL_ACK_INCALL_FAILURE) return ack;
  if (ack == A2DP_CTRL_ACK_INCALL_FAILURE) {
    ERROR("A2DP COMMAND %s error %d", audio_a2dp_hw_dump_ctrl_event(cmd), ack);
    return ack;
  }
  if (ack != A2DP_CTRL_ACK_SUCCESS) {
  if (ack != A2DP_CTRL_ACK_SUCCESS) {
    ERROR("A2DP COMMAND %s error %d", audio_a2dp_hw_dump_ctrl_event(cmd), ack);
    ERROR("A2DP COMMAND %s error %d", audio_a2dp_hw_dump_ctrl_event(cmd), ack);
    return -1;
    return -1;
+4 −2
Original line number Original line Diff line number Diff line
@@ -23,6 +23,8 @@
#include <base/callback_forward.h>
#include <base/callback_forward.h>
#include <base/location.h>
#include <base/location.h>


#include <hardware/bluetooth.h>

/*
/*
 * This method post a closure for execution on bta thread. Please see
 * This method post a closure for execution on bta thread. Please see
 * documentation at
 * documentation at
@@ -30,7 +32,7 @@
 * for how to handle dynamic memory ownership/smart pointers with base::Owned(),
 * for how to handle dynamic memory ownership/smart pointers with base::Owned(),
 * base::Passed(), base::ConstRef() and others.
 * base::Passed(), base::ConstRef() and others.
 */
 */
void do_in_bta_thread(const tracked_objects::Location& from_here,
bt_status_t do_in_bta_thread(const tracked_objects::Location& from_here,
                             const base::Closure& task);
                             const base::Closure& task);


#endif /* BTA_CLOSURE_API_H */
#endif /* BTA_CLOSURE_API_H */
+2 −2
Original line number Original line Diff line number Diff line
@@ -31,6 +31,8 @@
#include <base/logging.h>
#include <base/logging.h>
#include <base/threading/thread.h>
#include <base/threading/thread.h>


#include "bta/include/bta_closure_api.h"

/*****************************************************************************
/*****************************************************************************
 *  Constants and data types
 *  Constants and data types
 ****************************************************************************/
 ****************************************************************************/
@@ -222,8 +224,6 @@ extern void bta_sys_deregister(uint8_t id);
extern bool bta_sys_is_register(uint8_t id);
extern bool bta_sys_is_register(uint8_t id);
extern uint16_t bta_sys_get_sys_features(void);
extern uint16_t bta_sys_get_sys_features(void);
extern void bta_sys_sendmsg(void* p_msg);
extern void bta_sys_sendmsg(void* p_msg);
extern void do_in_bta_thread(const tracked_objects::Location& from_here,
                             const base::Closure& task);
extern void bta_sys_start_timer(alarm_t* alarm, period_ms_t interval,
extern void bta_sys_start_timer(alarm_t* alarm, period_ms_t interval,
                                uint16_t event, uint16_t layer_specific);
                                uint16_t event, uint16_t layer_specific);
extern void bta_sys_disable(tBTA_SYS_HW_MODULE module);
extern void bta_sys_disable(tBTA_SYS_HW_MODULE module);
+10 −6
Original line number Original line Diff line number Diff line
@@ -547,25 +547,29 @@ void bta_sys_sendmsg(void* p_msg) {
 *
 *
 * Description      Post a closure to be ran in the bta thread
 * Description      Post a closure to be ran in the bta thread
 *
 *
 * Returns          void
 * Returns          BT_STATUS_SUCCESS on success
 *
 *
 ******************************************************************************/
 ******************************************************************************/
void do_in_bta_thread(const tracked_objects::Location& from_here,
bt_status_t do_in_bta_thread(const tracked_objects::Location& from_here,
                             const base::Closure& task) {
                             const base::Closure& task) {
  base::MessageLoop* bta_message_loop = get_message_loop();
  base::MessageLoop* bta_message_loop = get_message_loop();
  if (!bta_message_loop) {
  if (!bta_message_loop) {
    APPL_TRACE_ERROR("%s: MessageLooper not initialized", __func__);
    APPL_TRACE_ERROR("%s: MessageLooper not initialized", __func__);
    return;
    return BT_STATUS_FAIL;
  }
  }


  scoped_refptr<base::SingleThreadTaskRunner> task_runner =
  scoped_refptr<base::SingleThreadTaskRunner> task_runner =
      bta_message_loop->task_runner();
      bta_message_loop->task_runner();
  if (!task_runner.get()) {
  if (!task_runner.get()) {
    APPL_TRACE_ERROR("%s: task runner is dead", __func__);
    APPL_TRACE_ERROR("%s: task runner is dead", __func__);
    return;
    return BT_STATUS_FAIL;
  }
  }


  task_runner->PostTask(from_here, task);
  if (!task_runner->PostTask(from_here, task)) {
    APPL_TRACE_ERROR("%s: Post task to task runner failed!", __func__);
    return BT_STATUS_FAIL;
  }
  return BT_STATUS_SUCCESS;
}
}


/*******************************************************************************
/*******************************************************************************
+3 −4
Original line number Original line Diff line number Diff line
@@ -1612,10 +1612,9 @@ bool BtaAvCo::ReportSourceCodecState(BtaAvCoPeer* p_peer) {
  APPL_TRACE_DEBUG("%s: peer %s codec_config=%s", __func__,
  APPL_TRACE_DEBUG("%s: peer %s codec_config=%s", __func__,
                   p_peer->addr.ToString().c_str(),
                   p_peer->addr.ToString().c_str(),
                   codec_config.ToString().c_str());
                   codec_config.ToString().c_str());
  do_in_jni_thread(
  btif_av_report_source_codec_state(p_peer->addr, codec_config,
      FROM_HERE,
                                    codecs_local_capabilities,
      base::Bind(&btif_av_report_source_codec_state, p_peer->addr, codec_config,
                                    codecs_selectable_capabilities);
                 codecs_local_capabilities, codecs_selectable_capabilities));
  return true;
  return true;
}
}


Loading