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

Commit d197ce9a authored by Rahul Arya's avatar Rahul Arya
Browse files

Avoid invalid state in search/discovery queue state machine 

The sequence of events in the state machine that causes the bug is as follows:
- We start a search, and move from the IDLE state to the SEARCH_ACTIVE state.
- We initiate a pairing request, that sets pairing_cb.state to BT_BOND_STATE_BONDING.
- We cancel the search, returning to the IDLE state.
- We then try to start a second search. In BTA_DmSearch, we check if we are bonding, and if so enqueue the request.
- But we remain in the IDLE state! So we never dequeue the search request, getting us into the stuck state.

This CL fixes the issue by removing the *_QUEUE_* events, making the state machine the single arbiter for whether a request should be executed or queued. In particular, for the above scenario, the SDP request will queue behind the second search request, if it arrives afterwards, *even if* bonding has already begun. This prevents us from getting in a stuck state.

I believe that this is safe since we preserve the invariant of only ever have one request going through at a time, but manual testing is needed to be certain. The preservation of this invariant also means that we should not regress b/237352700, though I am unable to test that.

Note that this CL is stacked on top of the revert^2s at ag/19019716, so it will exhibit merge conflicts until those CLs are landed.

Bug: 230277335
Test: Manual, using the repro steps from the bug thread.
Tag: #stability
Ignore-AOSP-First: Hotfix for T

Change-Id: I74e83964995b10f4bee35bae3182ec482bdedb39
parent f2cd2308

system/bta/dm/bta_dm_act.cc

+0 −8
Original line number Diff line number Diff line
@@ -1469,11 +1469,6 @@ void bta_dm_queue_disc(tBTA_DM_MSG* p_data) { 
 ******************************************************************************/

void bta_dm_execute_queued_request() {

  if (bta_dm_search_cb.p_pending_search) {

    // Updated queued event to search event to trigger start search

    if (bta_dm_search_cb.p_pending_search->hdr.event ==

        BTA_DM_API_QUEUE_SEARCH_EVT) {

      bta_dm_search_cb.p_pending_search->hdr.event = BTA_DM_API_SEARCH_EVT;

    }

    LOG_INFO("%s Start pending search", __func__);

    bta_sys_sendmsg(bta_dm_search_cb.p_pending_search);

    bta_dm_search_cb.p_pending_search = NULL;
@@ -1481,9 +1476,6 @@ void bta_dm_execute_queued_request() { 
    tBTA_DM_MSG* p_pending_discovery = (tBTA_DM_MSG*)fixed_queue_try_dequeue(

        bta_dm_search_cb.pending_discovery_queue);

    if (p_pending_discovery) {

      if (p_pending_discovery->hdr.event == BTA_DM_API_QUEUE_DISCOVER_EVT) {

        p_pending_discovery->hdr.event = BTA_DM_API_DISCOVER_EVT;

      }

      LOG_INFO("%s Start pending discovery", __func__);

      bta_sys_sendmsg(p_pending_discovery);

    }

system/bta/dm/bta_dm_api.cc

+4 −13
Original line number Diff line number Diff line
@@ -80,16 +80,11 @@ void BTA_DmSetDeviceName(const char* p_name) { 
 * Returns          void

 *

 ******************************************************************************/

void BTA_DmSearch(tBTA_DM_SEARCH_CBACK* p_cback, bool is_bonding_or_sdp) {

void BTA_DmSearch(tBTA_DM_SEARCH_CBACK* p_cback) {

  tBTA_DM_API_SEARCH* p_msg =

      (tBTA_DM_API_SEARCH*)osi_calloc(sizeof(tBTA_DM_API_SEARCH));



  /* Queue request if a device is bonding or performing service discovery */

  if (is_bonding_or_sdp) {

    p_msg->hdr.event = BTA_DM_API_QUEUE_SEARCH_EVT;

  } else {

  p_msg->hdr.event = BTA_DM_API_SEARCH_EVT;

  }

  p_msg->p_cback = p_cback;



  bta_sys_sendmsg(p_msg);
@@ -138,15 +133,11 @@ void BTA_DmSearchCancel(void) { 
 *

 ******************************************************************************/

void BTA_DmDiscover(const RawAddress& bd_addr, tBTA_DM_SEARCH_CBACK* p_cback,

                    tBT_TRANSPORT transport, bool is_bonding_or_sdp) {

                    tBT_TRANSPORT transport) {

  tBTA_DM_API_DISCOVER* p_msg =

      (tBTA_DM_API_DISCOVER*)osi_calloc(sizeof(tBTA_DM_API_DISCOVER));



  if (is_bonding_or_sdp) {

    p_msg->hdr.event = BTA_DM_API_QUEUE_DISCOVER_EVT;

  } else {

  p_msg->hdr.event = BTA_DM_API_DISCOVER_EVT;

  }

  p_msg->bd_addr = bd_addr;

  p_msg->transport = transport;

  p_msg->p_cback = p_cback;

system/bta/dm/bta_dm_int.h

+2 −4
Original line number Diff line number Diff line
@@ -71,18 +71,16 @@ enum { 
  BTA_DM_SEARCH_CMPL_EVT,

  BTA_DM_DISCOVERY_RESULT_EVT,

  BTA_DM_DISC_CLOSE_TOUT_EVT,

  BTA_DM_API_QUEUE_SEARCH_EVT,

  BTA_DM_API_QUEUE_DISCOVER_EVT

};



/* data type for BTA_DM_API_SEARCH_EVT and BTA_DM_API_QUEUE_SEARCH_EVT */

/* data type for BTA_DM_API_SEARCH_EVT */

typedef struct {

  BT_HDR_RIGID hdr;

  tBTA_SERVICE_MASK services;

  tBTA_DM_SEARCH_CBACK* p_cback;

} tBTA_DM_API_SEARCH;



/* data type for BTA_DM_API_DISCOVER_EVT and BTA_DM_API_QUEUE_DISCOVER_EVT */

/* data type for BTA_DM_API_DISCOVER_EVT */

typedef struct {

  BT_HDR_RIGID hdr;

  RawAddress bd_addr;

system/bta/dm/bta_dm_main.cc

+0 −11
Original line number Diff line number Diff line
@@ -82,12 +82,6 @@ bool bta_dm_search_sm_execute(BT_HDR_RIGID* p_msg) { 
        case BTA_DM_DISC_CLOSE_TOUT_EVT:

          bta_dm_close_gatt_conn(message);

          break;

        case BTA_DM_API_QUEUE_SEARCH_EVT:

          bta_dm_queue_search(message);

          break;

        case BTA_DM_API_QUEUE_DISCOVER_EVT:

          bta_dm_queue_disc(message);

          break;

      }

      break;

    case BTA_DM_SEARCH_ACTIVE:
@@ -108,7 +102,6 @@ bool bta_dm_search_sm_execute(BT_HDR_RIGID* p_msg) { 
          bta_dm_close_gatt_conn(message);

          break;

        case BTA_DM_API_DISCOVER_EVT:

        case BTA_DM_API_QUEUE_DISCOVER_EVT:

          bta_dm_queue_disc(message);

          break;

      }
@@ -116,11 +109,9 @@ bool bta_dm_search_sm_execute(BT_HDR_RIGID* p_msg) { 
    case BTA_DM_SEARCH_CANCELLING:

      switch (p_msg->event) {

        case BTA_DM_API_SEARCH_EVT:

        case BTA_DM_API_QUEUE_SEARCH_EVT:

          bta_dm_queue_search(message);

          break;

        case BTA_DM_API_DISCOVER_EVT:

        case BTA_DM_API_QUEUE_DISCOVER_EVT:

          bta_dm_queue_disc(message);

          break;

        case BTA_DM_SDP_RESULT_EVT:
@@ -149,11 +140,9 @@ bool bta_dm_search_sm_execute(BT_HDR_RIGID* p_msg) { 
          bta_dm_disc_result(message);

          break;

        case BTA_DM_API_SEARCH_EVT:

        case BTA_DM_API_QUEUE_SEARCH_EVT:

          bta_dm_queue_search(message);

          break;

        case BTA_DM_API_DISCOVER_EVT:

        case BTA_DM_API_QUEUE_DISCOVER_EVT:

          bta_dm_queue_disc(message);

          break;

      }

system/bta/include/bta_api.h

+3 −5
Original line number Diff line number Diff line
@@ -710,15 +710,13 @@ extern bool BTA_DmSetVisibility(bt_scan_mode_t mode); 
 *                  first performs an inquiry; for each device found from the

 *                  inquiry it gets the remote name of the device.  If

 *                  parameter services is nonzero, service discovery will be

 *                  performed on each device for the services specified. If the

 *                  parameter is_bonding_or_sdp is true, the request will be

 *                  queued until bonding or sdp completes

 *                  performed on each device for the services specified.

 *

 *

 * Returns          void

 *

 ******************************************************************************/

extern void BTA_DmSearch(tBTA_DM_SEARCH_CBACK* p_cback, bool is_bonding_or_sdp);

extern void BTA_DmSearch(tBTA_DM_SEARCH_CBACK* p_cback);



/*******************************************************************************

 *
@@ -746,7 +744,7 @@ extern void BTA_DmSearchCancel(void); 
 ******************************************************************************/

extern void BTA_DmDiscover(const RawAddress& bd_addr,

                           tBTA_DM_SEARCH_CBACK* p_cback,

                           tBT_TRANSPORT transport, bool is_bonding_or_sdp);

                           tBT_TRANSPORT transport);



/*******************************************************************************

 *