Problem with BT Interface 1.0 in Android 8,9,10 (and maybe 11) with non-uart BT

[Eric Bentley]

I discovered an issue with the BT interface code on Android 8 and newer when utilizing non-uart BT modules.

The module I am using creates a pseudo /dev/device UART in order to pass inside the libbt-vendor.so for the bluetooth_manager to be able to send data to the device.  As each write() of the 'UART' is received, it is bundled and sent to the transport layer (in my case, SDIO and USB) as a packet.

Starting with Android 8, I see that the H4 protocol code now creates two calls to write() for each BT packet.  The first is the type and the second is the remainder of the packet.  This forces me to capture the first write() and hold it and then append the second write() data to it prior to passing to the transport layer.

In hardware/interfaces/bluetooth/1.0/default/h4_protocol.cc the Send() function is: (Android 8)

size_t H4Protocol::Send(uint8_t type, const uint8_t* data, size_t length) {

   struct iovec iov[] = {{&type, sizeof(type)},

                        {const_cast<uint8_t*>(data), length}};

  ssize_t ret = 0;

  do {

    ret = TEMP_FAILURE_RETRY(writev(uart_fd_, iov, sizeof(iov) / sizeof(iov[0])));

  } while (-1 == ret && EAGAIN == errno);

  if (ret == -1) {

    ALOGE("%s error writing to UART (%s)", __func__, strerror(errno));

  } else if (ret < static_cast<ssize_t>(length + 1)) {

    ALOGE("%s: %d / %d bytes written - something went wrong...", __func__,

          static_cast<int>(ret), static_cast<int>(length + 1));
  }

  return ret;
}

Because struct iovec contains 2 entries, we get two calls to write().  This simpler code using vectors and the WriteSafely() function already in the chi_protocol solves the issue and provides the error/ALOGE logic:

size_t H4Protocol::Send(uint8_t type, const uint8_t* data, size_t length) {

  std::vector<uint8_t> tmp;

  tmp.reserve(1 + length);

  tmp.push_back(type);

  tmp.insert(tmp.end(), data, data + length);

  return WriteSafely(uart_fd_, tmp.data(), tmp.size());
}

How can I get this (or a different appropriate fix) into the mainstream?

Thanks,

-Eric

[Eric Bentley]

Digging into the history of this code, I see that this code was created to prevent the exact problem I described.  Unfortunately, it didn't.  Here is that commit message:

Author: Peng Qi <removed - see commit for address>  2017-06-19 06:01:07

Committer: Ting Zheng <removed - see commit for address>  2017-06-30 15:32:11

Parent: 917efb1c0e2458434390463ca11bd9be935c2e54 (Bring multi-channel transport into the glorious new age)

Child:  3e272a70763ff6191ac96d8560724e0a02f827ac (Bluetooth: Change CHECK() to LOG_ALWAYS_FATAL())

Branches: remotes/m/laird-imx-p9.0.0_1.0.0-ga^{} and many more (86)

Follows: android-o-preview-3

Precedes: android-o-preview-4

    BT HAL H4 write flow

    

    If to send type and data separately for one HCI packet,

    it will cause two system call context switch to kernel space,

    which will introduce software overhead on data path.

    Plus, if vendor does not use pure UART interface, it causes different

    data behavior on BUS and may not adapt to all vendors as legacy HAL did.

    Considering backward-compatibility, to use writev to send

    type and data together once as legacy BT HAL did.

    

    Test: H4 UTTest, BT VTS test, Bluetooth on/off

    Change-Id: I2d93085fe0c01b48d0e3729a3fa85b5b27335b2c

--------------------- bluetooth/1.0/default/h4_protocol.cc ---------------------

index 8f24b5eea..2df2b3b91 100644

@@ -18,8 +18,10 @@

 

 #define LOG_TAG "android.hardware.bluetooth-hci-h4"

 #include <android-base/logging.h>

-#include <assert.h>

+#include <errno.h>

 #include <fcntl.h>

+#include <log/log.h>

+#include <sys/uio.h>

 

 namespace android {

 namespace hardware {

@@ -27,11 +29,20 @@ namespace bluetooth {

 namespace hci {

 

 size_t H4Protocol::Send(uint8_t type, const uint8_t* data, size_t length) {

-  int rv = WriteSafely(uart_fd_, &type, sizeof(type));

-  if (rv == sizeof(type)) {

-    rv = WriteSafely(uart_fd_, data, length);

+  struct iovec iov[] = {{&type, sizeof(type)},

+                        {const_cast<uint8_t*>(data), length}};

+  ssize_t ret = 0;

+  do {

+    ret = TEMP_FAILURE_RETRY(writev(uart_fd_, iov, sizeof(iov) / sizeof(iov[0])));

+  } while (-1 == ret && EAGAIN == errno);

+

+  if (ret == -1) {

+    ALOGE("%s error writing to UART (%s)", __func__, strerror(errno));

+  } else if (ret == 0) {

+    // Nothing written :(

+    ALOGE("%s zero bytes written - something went wrong...", __func__);

   }

-  return rv;

+  return ret;

 }

 

 void H4Protocol::OnPacketReady() {