Linux bluetooth development
 help / color / mirror / Atom feed
* [PATCH 0/5] android: Update HAL to Android 4.4 API
@ 2013-12-20 10:25 Szymon Janc
  2013-12-20 10:25 ` [PATCH 1/5] android/hal-bluetooth: Add support for sending LE_TEST_MODE command Szymon Janc
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Szymon Janc @ 2013-12-20 10:25 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Szymon Janc

Hi,

It is important to fully implement bt_interface_t (at least with stubs/NULLs)
since Android is not checking size field from HAL and just assume memory
layout of BT HAL. This was causing crashes on 4.4 due to Android trying
to call not existing methods.

BT snoop logs enabling function is a bit special since it will be implemented
only in HAL so no need to extend IPC with that (plan is to have dedicated
service that will dump HCI traffic).

For now 4.2+ API is if-deffed in HAL library - this will be removed when
minimal Android version required will be dropped.

-- 
BR
Szymon Janc

Szymon Janc (5):
  android/hal-bluetooth: Add support for sending LE_TEST_MODE command
  android/hal-bluetooth: Add support for enabling HCI snoop dump
  android/hal-bluetooth: Add support for remote version info property
  android/hal-bluetooth: Add support for device service record property
  android/bluetooth: Add stubs for missing commands

 android/bluetooth.c         |  56 ++++++++++++++++++++++++
 android/cutils/properties.h |   1 +
 android/hal-bluetooth.c     | 104 +++++++++++++++++++++++++++++++++++++++++++-
 android/hal-msg.h           |  13 ++++++
 4 files changed, 172 insertions(+), 2 deletions(-)

-- 
1.8.3.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/5] android/hal-bluetooth: Add support for sending LE_TEST_MODE command
  2013-12-20 10:25 [PATCH 0/5] android: Update HAL to Android 4.4 API Szymon Janc
@ 2013-12-20 10:25 ` Szymon Janc
  2013-12-20 11:57   ` Anderson Lizardo
  2013-12-20 10:25 ` [PATCH 2/5] android/hal-bluetooth: Add support for enabling HCI snoop dump Szymon Janc
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Szymon Janc @ 2013-12-20 10:25 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Szymon Janc

This allows to send LE_TEST_MODE command introduced in Android 4.3.
---
 android/hal-bluetooth.c | 44 +++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 43 insertions(+), 1 deletion(-)

diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
index 7cac15c..11f2a9f 100644
--- a/android/hal-bluetooth.c
+++ b/android/hal-bluetooth.c
@@ -307,6 +307,18 @@ static void handle_dut_mode_receive(void *buf, uint16_t len)
 		bt_hal_cbacks->dut_mode_recv_cb(ev->opcode, ev->data, ev->len);
 }
 
+#if PLATFORM_SDK_VERSION > 17
+static void handle_le_test_mode(void *buf, uint16_t len)
+{
+	struct hal_ev_le_test_mode *ev = buf;
+
+	DBG("");
+
+	if (bt_hal_cbacks->le_test_mode_cb)
+		bt_hal_cbacks->le_test_mode_cb(ev->status, ev->num_packets);
+}
+#endif
+
 /* handlers will be called from notification thread context,
  * index in table equals to 'opcode - HAL_MINIMUM_EVENT' */
 static const struct hal_ipc_handler ev_handlers[] = {
@@ -363,6 +375,13 @@ static const struct hal_ipc_handler ev_handlers[] = {
 		.var_len = true,
 		.data_len = sizeof(struct hal_ev_dut_mode_receive),
 	},
+#if PLATFORM_SDK_VERSION > 17
+	{	/* HAL_EV_LE_TEST_MODE */
+		.handler = handle_le_test_mode,
+		.var_len = false,
+		.data_len = sizeof(struct hal_ev_le_test_mode),
+	}
+#endif
 };
 
 static int init(bt_callbacks_t *callbacks)
@@ -752,6 +771,26 @@ static int dut_mode_send(uint16_t opcode, uint8_t *buf, uint8_t len)
 					sizeof(cmd_buf), cmd, 0, NULL, NULL);
 }
 
+#if PLATFORM_SDK_VERSION > 17
+static int le_test_mode(uint16_t opcode, uint8_t *buf, uint8_t len)
+{
+	uint8_t cmd_buf[sizeof(struct hal_cmd_dut_mode_send) + len];
+	struct hal_cmd_le_test_mode *cmd = (void *) cmd_buf;
+
+	DBG("opcode %u len %u", opcode, len);
+
+	if (!interface_ready())
+		return BT_STATUS_NOT_READY;
+
+	cmd->opcode = opcode;
+	cmd->len = len;
+	memcpy(cmd->data, buf, cmd->len);
+
+	return hal_ipc_cmd(HAL_SERVICE_ID_BLUETOOTH, HAL_OP_LE_TEST_MODE,
+					sizeof(cmd_buf), cmd, 0, NULL, NULL);
+}
+#endif
+
 static const bt_interface_t bluetooth_if = {
 	.size = sizeof(bt_interface_t),
 	.init = init,
@@ -775,7 +814,10 @@ static const bt_interface_t bluetooth_if = {
 	.ssp_reply = ssp_reply,
 	.get_profile_interface = get_profile_interface,
 	.dut_mode_configure = dut_mode_configure,
-	.dut_mode_send = dut_mode_send
+	.dut_mode_send = dut_mode_send,
+#if PLATFORM_SDK_VERSION > 17
+	.le_test_mode = le_test_mode,
+#endif
 };
 
 static const bt_interface_t *get_bluetooth_interface(void)
-- 
1.8.3.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/5] android/hal-bluetooth: Add support for enabling HCI snoop dump
  2013-12-20 10:25 [PATCH 0/5] android: Update HAL to Android 4.4 API Szymon Janc
  2013-12-20 10:25 ` [PATCH 1/5] android/hal-bluetooth: Add support for sending LE_TEST_MODE command Szymon Janc
@ 2013-12-20 10:25 ` Szymon Janc
  2013-12-20 10:25 ` [PATCH 3/5] android/hal-bluetooth: Add support for remote version info property Szymon Janc
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Szymon Janc @ 2013-12-20 10:25 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Szymon Janc

This allows to enable HCI SNOOP dump from HAL. This requires that
service named 'bluetoothd_snoop' be available in Android system.
---
 android/cutils/properties.h |  1 +
 android/hal-bluetooth.c     | 24 ++++++++++++++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/android/cutils/properties.h b/android/cutils/properties.h
index 7951585..256bf13 100644
--- a/android/cutils/properties.h
+++ b/android/cutils/properties.h
@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+#include <unistd.h>
 #include <stdio.h>
 #include <string.h>
 #include <sys/socket.h>
diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
index 11f2a9f..905b293 100644
--- a/android/hal-bluetooth.c
+++ b/android/hal-bluetooth.c
@@ -20,12 +20,16 @@
 #include <stdbool.h>
 #include <string.h>
 
+#include <cutils/properties.h>
+
 #include "hal-log.h"
 #include "hal.h"
 #include "hal-msg.h"
 #include "hal-ipc.h"
 #include "hal-utils.h"
 
+#define SNOOP_SERVICE_NAME "bluetoothd_snoop"
+
 static const bt_callbacks_t *bt_hal_cbacks = NULL;
 
 #define enum_prop_to_hal(prop, hal_prop, type) do { \
@@ -791,6 +795,23 @@ static int le_test_mode(uint16_t opcode, uint8_t *buf, uint8_t len)
 }
 #endif
 
+#if PLATFORM_SDK_VERSION > 18
+static int config_hci_snoop_log(uint8_t enable)
+{
+	if (enable && property_set("ctl.start", SNOOP_SERVICE_NAME) < 0) {
+		error("Failed to start service %s", SNOOP_SERVICE_NAME);
+		return BT_STATUS_FAIL;
+	}
+
+	if (!enable && property_set("ctl.stop", SNOOP_SERVICE_NAME) < 0) {
+		error("Failed to stop service %s", SNOOP_SERVICE_NAME);
+		return BT_STATUS_FAIL;
+	}
+
+	return BT_STATUS_SUCCESS;
+}
+#endif
+
 static const bt_interface_t bluetooth_if = {
 	.size = sizeof(bt_interface_t),
 	.init = init,
@@ -818,6 +839,9 @@ static const bt_interface_t bluetooth_if = {
 #if PLATFORM_SDK_VERSION > 17
 	.le_test_mode = le_test_mode,
 #endif
+#if PLATFORM_SDK_VERSION > 18
+	.config_hci_snoop_log = config_hci_snoop_log,
+#endif
 };
 
 static const bt_interface_t *get_bluetooth_interface(void)
-- 
1.8.3.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/5] android/hal-bluetooth: Add support for remote version info property
  2013-12-20 10:25 [PATCH 0/5] android: Update HAL to Android 4.4 API Szymon Janc
  2013-12-20 10:25 ` [PATCH 1/5] android/hal-bluetooth: Add support for sending LE_TEST_MODE command Szymon Janc
  2013-12-20 10:25 ` [PATCH 2/5] android/hal-bluetooth: Add support for enabling HCI snoop dump Szymon Janc
@ 2013-12-20 10:25 ` Szymon Janc
  2013-12-20 12:07   ` Anderson Lizardo
  2013-12-20 10:25 ` [PATCH 4/5] android/hal-bluetooth: Add support for device service record property Szymon Janc
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Szymon Janc @ 2013-12-20 10:25 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Szymon Janc

This allows to correctly handle remote version info property. Although
this property is marked as get/set in HAL only get is implemented as
I fail to see how this property could be settable.
---
 android/hal-bluetooth.c | 21 ++++++++++++++++++++-
 android/hal-msg.h       |  6 ++++++
 2 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
index 905b293..d0c1c01 100644
--- a/android/hal-bluetooth.c
+++ b/android/hal-bluetooth.c
@@ -145,8 +145,27 @@ static void device_props_to_hal(bt_property_t *send_props,
 			enum_prop_to_hal(send_props[i], prop,
 							bt_device_type_t);
 			break;
-		case HAL_PROP_DEVICE_SERVICE_REC:
+#if PLATFORM_SDK_VERSION > 17
 		case HAL_PROP_DEVICE_VERSION_INFO:
+		{
+			static bt_remote_version_t e;
+			const struct hal_prop_device_info *p;
+			uint16_t tmp;
+
+			send_props[i].val = &e;
+			send_props[i].len = sizeof(e);
+
+			p = (struct hal_prop_device_info *) prop->val;
+
+			memcpy(&tmp, &p->manufacturer, sizeof(tmp));
+			e.manufacturer = tmp;
+			memcpy(&tmp, &p->sub_version, sizeof(tmp));
+			e.sub_ver = tmp;
+			e.version = p->version;
+		}
+			break;
+#endif
+		case HAL_PROP_DEVICE_SERVICE_REC:
 		default:
 			send_props[i].len = prop->len;
 			send_props[i].val = prop->val;
diff --git a/android/hal-msg.h b/android/hal-msg.h
index 3be91aa..bb196d8 100644
--- a/android/hal-msg.h
+++ b/android/hal-msg.h
@@ -110,6 +110,12 @@ struct hal_cmd_get_adapter_prop {
 #define HAL_PROP_DEVICE_FRIENDLY_NAME		0x0a
 #define HAL_PROP_DEVICE_RSSI			0x0b
 #define HAL_PROP_DEVICE_VERSION_INFO		0x0c
+struct hal_prop_device_info {
+	uint8_t version;
+	uint16_t sub_version;
+	uint16_t manufacturer;
+} __attribute__((packed));
+
 #define HAL_PROP_DEVICE_TIMESTAMP		0xFF
 
 #define HAL_ADAPTER_SCAN_MODE_NONE		0x00
-- 
1.8.3.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/5] android/hal-bluetooth: Add support for device service record property
  2013-12-20 10:25 [PATCH 0/5] android: Update HAL to Android 4.4 API Szymon Janc
                   ` (2 preceding siblings ...)
  2013-12-20 10:25 ` [PATCH 3/5] android/hal-bluetooth: Add support for remote version info property Szymon Janc
@ 2013-12-20 10:25 ` Szymon Janc
  2013-12-20 13:18   ` Marcel Holtmann
  2013-12-20 10:25 ` [PATCH 5/5] android/bluetooth: Add stubs for missing commands Szymon Janc
  2013-12-20 10:58 ` [PATCH 0/5] android: Update HAL to Android 4.4 API Johan Hedberg
  5 siblings, 1 reply; 13+ messages in thread
From: Szymon Janc @ 2013-12-20 10:25 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Szymon Janc

This allows to correctly handle device service record property.
---
 android/hal-bluetooth.c | 15 +++++++++++++++
 android/hal-msg.h       |  7 +++++++
 2 files changed, 22 insertions(+)

diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
index d0c1c01..4ef2ebe 100644
--- a/android/hal-bluetooth.c
+++ b/android/hal-bluetooth.c
@@ -166,6 +166,21 @@ static void device_props_to_hal(bt_property_t *send_props,
 			break;
 #endif
 		case HAL_PROP_DEVICE_SERVICE_REC:
+		{
+			static bt_service_record_t e;
+			const struct hal_prop_device_service_rec *p;
+
+			send_props[i].val = &e;
+			send_props[i].len = sizeof(e);
+
+			p = (struct hal_prop_device_service_rec *) prop->val;
+
+			memset(&e, 0, sizeof(e));
+			memcpy(&e.channel, &p->channel, sizeof(e.channel));
+			memcpy(e.uuid.uu, p->uuid, sizeof(e.uuid.uu));
+			memcpy(e.name, p->name, p->name_len);
+		}
+			break;
 		default:
 			send_props[i].len = prop->len;
 			send_props[i].val = prop->val;
diff --git a/android/hal-msg.h b/android/hal-msg.h
index bb196d8..c351501 100644
--- a/android/hal-msg.h
+++ b/android/hal-msg.h
@@ -107,6 +107,13 @@ struct hal_cmd_get_adapter_prop {
 #define HAL_PROP_DEVICE_CLASS			0x04
 #define HAL_PROP_DEVICE_TYPE			0x05
 #define HAL_PROP_DEVICE_SERVICE_REC		0x06
+struct hal_prop_device_service_rec {
+	uint8_t uuid[16];
+	uint16_t channel;
+	uint8_t name_len;
+	uint8_t name[];
+} __attribute__((packed));
+
 #define HAL_PROP_DEVICE_FRIENDLY_NAME		0x0a
 #define HAL_PROP_DEVICE_RSSI			0x0b
 #define HAL_PROP_DEVICE_VERSION_INFO		0x0c
-- 
1.8.3.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 5/5] android/bluetooth: Add stubs for missing commands
  2013-12-20 10:25 [PATCH 0/5] android: Update HAL to Android 4.4 API Szymon Janc
                   ` (3 preceding siblings ...)
  2013-12-20 10:25 ` [PATCH 4/5] android/hal-bluetooth: Add support for device service record property Szymon Janc
@ 2013-12-20 10:25 ` Szymon Janc
  2013-12-20 10:58 ` [PATCH 0/5] android: Update HAL to Android 4.4 API Johan Hedberg
  5 siblings, 0 replies; 13+ messages in thread
From: Szymon Janc @ 2013-12-20 10:25 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Szymon Janc

This adds stubs for commands that were introduced by BT HAL changes
in Android 4.3+.
---
 android/bluetooth.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/android/bluetooth.c b/android/bluetooth.c
index efaa424..6bfe786 100644
--- a/android/bluetooth.c
+++ b/android/bluetooth.c
@@ -2567,6 +2567,54 @@ failed:
 	ipc_send_rsp(HAL_SERVICE_ID_BLUETOOTH, HAL_OP_CANCEL_DISCOVERY, status);
 }
 
+static void handle_dut_mode_conf_cmd(const void *buf, uint16_t len)
+{
+	const struct hal_cmd_dut_mode_conf *cmd = buf;
+
+	DBG("enable %u", cmd->enable);
+
+	/* TODO */
+
+	ipc_send_rsp(HAL_SERVICE_ID_BLUETOOTH, HAL_OP_DUT_MODE_CONF,
+							HAL_STATUS_FAILED);
+}
+
+static void handle_dut_mode_send_cmd(const void *buf, uint16_t len)
+{
+	const struct hal_cmd_dut_mode_send *cmd = buf;
+
+	if (len != sizeof(*cmd) + cmd->len) {
+		error("Invalid dut mode send cmd, terminating");
+		raise(SIGTERM);
+		return;
+	}
+
+	DBG("opcode %u", cmd->opcode);
+
+	/* TODO */
+
+	ipc_send_rsp(HAL_SERVICE_ID_BLUETOOTH, HAL_OP_DUT_MODE_SEND,
+							HAL_STATUS_FAILED);
+}
+
+static void handle_le_test_mode_cmd(const void *buf, uint16_t len)
+{
+	const struct hal_cmd_le_test_mode *cmd = buf;
+
+	if (len != sizeof(*cmd) + cmd->len) {
+		error("Invalid le test mode cmd, terminating");
+		raise(SIGTERM);
+		return;
+	}
+
+	DBG("opcode %u", cmd->opcode);
+
+	/* TODO */
+
+	ipc_send_rsp(HAL_SERVICE_ID_BLUETOOTH, HAL_OP_LE_TEST_MODE,
+							HAL_STATUS_FAILED);
+}
+
 static const struct ipc_handler cmd_handlers[] = {
 	/* HAL_OP_ENABLE */
 	{ handle_enable_cmd, false, 0 },
@@ -2609,6 +2657,14 @@ static const struct ipc_handler cmd_handlers[] = {
 	{ handle_pin_reply_cmd, false, sizeof(struct hal_cmd_pin_reply) },
 	/* HAL_OP_SSP_REPLY */
 	{ handle_ssp_reply_cmd, false, sizeof(struct hal_cmd_ssp_reply) },
+	/* HAL_OP_DUT_MODE_CONF */
+	{ handle_dut_mode_conf_cmd, false,
+					sizeof(struct hal_cmd_dut_mode_conf) },
+	/* HAL_OP_DUT_MODE_SEND */
+	{ handle_dut_mode_send_cmd, true,
+					sizeof(struct hal_cmd_dut_mode_send) },
+	/* HAL_OP_LE_TEST_MODE */
+	{ handle_le_test_mode_cmd, true, sizeof(struct hal_cmd_le_test_mode) },
 };
 
 void bt_bluetooth_register(void)
-- 
1.8.3.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/5] android: Update HAL to Android 4.4 API
  2013-12-20 10:25 [PATCH 0/5] android: Update HAL to Android 4.4 API Szymon Janc
                   ` (4 preceding siblings ...)
  2013-12-20 10:25 ` [PATCH 5/5] android/bluetooth: Add stubs for missing commands Szymon Janc
@ 2013-12-20 10:58 ` Johan Hedberg
  5 siblings, 0 replies; 13+ messages in thread
From: Johan Hedberg @ 2013-12-20 10:58 UTC (permalink / raw)
  To: Szymon Janc; +Cc: linux-bluetooth

Hi Szymon,

On Fri, Dec 20, 2013, Szymon Janc wrote:
> It is important to fully implement bt_interface_t (at least with stubs/NULLs)
> since Android is not checking size field from HAL and just assume memory
> layout of BT HAL. This was causing crashes on 4.4 due to Android trying
> to call not existing methods.
> 
> BT snoop logs enabling function is a bit special since it will be implemented
> only in HAL so no need to extend IPC with that (plan is to have dedicated
> service that will dump HCI traffic).
> 
> For now 4.2+ API is if-deffed in HAL library - this will be removed when
> minimal Android version required will be dropped.
> 
> -- 
> BR
> Szymon Janc
> 
> Szymon Janc (5):
>   android/hal-bluetooth: Add support for sending LE_TEST_MODE command
>   android/hal-bluetooth: Add support for enabling HCI snoop dump
>   android/hal-bluetooth: Add support for remote version info property
>   android/hal-bluetooth: Add support for device service record property
>   android/bluetooth: Add stubs for missing commands
> 
>  android/bluetooth.c         |  56 ++++++++++++++++++++++++
>  android/cutils/properties.h |   1 +
>  android/hal-bluetooth.c     | 104 +++++++++++++++++++++++++++++++++++++++++++-
>  android/hal-msg.h           |  13 ++++++
>  4 files changed, 172 insertions(+), 2 deletions(-)

All patches in this set have been applied. Thanks.

Johan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/5] android/hal-bluetooth: Add support for sending LE_TEST_MODE command
  2013-12-20 10:25 ` [PATCH 1/5] android/hal-bluetooth: Add support for sending LE_TEST_MODE command Szymon Janc
@ 2013-12-20 11:57   ` Anderson Lizardo
  0 siblings, 0 replies; 13+ messages in thread
From: Anderson Lizardo @ 2013-12-20 11:57 UTC (permalink / raw)
  To: Szymon Janc; +Cc: BlueZ development

Hi Szymon,

On Fri, Dec 20, 2013 at 6:25 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> +#if PLATFORM_SDK_VERSION > 17
> +static int le_test_mode(uint16_t opcode, uint8_t *buf, uint8_t len)
> +{
> +       uint8_t cmd_buf[sizeof(struct hal_cmd_dut_mode_send) + len];
> +       struct hal_cmd_le_test_mode *cmd = (void *) cmd_buf;

You are using two different structs above (which happen to have the
same fields).

> +
> +       DBG("opcode %u len %u", opcode, len);
> +
> +       if (!interface_ready())
> +               return BT_STATUS_NOT_READY;
> +
> +       cmd->opcode = opcode;
> +       cmd->len = len;
> +       memcpy(cmd->data, buf, cmd->len);
> +
> +       return hal_ipc_cmd(HAL_SERVICE_ID_BLUETOOTH, HAL_OP_LE_TEST_MODE,
> +                                       sizeof(cmd_buf), cmd, 0, NULL, NULL);

Just to be consistent above, I would use "..., sizeof(cmd_buf),
cmd_buf, ...", although I noticed other similar code already upstream.

Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/5] android/hal-bluetooth: Add support for remote version info property
  2013-12-20 10:25 ` [PATCH 3/5] android/hal-bluetooth: Add support for remote version info property Szymon Janc
@ 2013-12-20 12:07   ` Anderson Lizardo
  0 siblings, 0 replies; 13+ messages in thread
From: Anderson Lizardo @ 2013-12-20 12:07 UTC (permalink / raw)
  To: Szymon Janc; +Cc: BlueZ development

Hi Szymon,

On Fri, Dec 20, 2013 at 6:25 AM, Szymon Janc <szymon.janc@tieto.com> wrote:
> +#if PLATFORM_SDK_VERSION > 17
>                 case HAL_PROP_DEVICE_VERSION_INFO:
> +               {
> +                       static bt_remote_version_t e;
> +                       const struct hal_prop_device_info *p;
> +                       uint16_t tmp;
> +
> +                       send_props[i].val = &e;
> +                       send_props[i].len = sizeof(e);
> +
> +                       p = (struct hal_prop_device_info *) prop->val;
> +
> +                       memcpy(&tmp, &p->manufacturer, sizeof(tmp));
> +                       e.manufacturer = tmp;
> +                       memcpy(&tmp, &p->sub_version, sizeof(tmp));
> +                       e.sub_ver = tmp;
> +                       e.version = p->version;

I fail to see why you need the "tmp" variable above (p->manufacturer
and p->sub_version are both uint16_t according to hal-msg.h).

> +               }
> +                       break;
> +#endif
> +               case HAL_PROP_DEVICE_SERVICE_REC:
>                 default:
>                         send_props[i].len = prop->len;
>                         send_props[i].val = prop->val;

Best Regards,
-- 
Anderson Lizardo
Instituto Nokia de Tecnologia - INdT
Manaus - Brazil

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/5] android/hal-bluetooth: Add support for device service record property
  2013-12-20 10:25 ` [PATCH 4/5] android/hal-bluetooth: Add support for device service record property Szymon Janc
@ 2013-12-20 13:18   ` Marcel Holtmann
  2013-12-20 13:38     ` Szymon Janc
  0 siblings, 1 reply; 13+ messages in thread
From: Marcel Holtmann @ 2013-12-20 13:18 UTC (permalink / raw)
  To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org development

Hi Szymon,

> This allows to correctly handle device service record property.
> ---
> android/hal-bluetooth.c | 15 +++++++++++++++
> android/hal-msg.h       |  7 +++++++
> 2 files changed, 22 insertions(+)
> 
> diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
> index d0c1c01..4ef2ebe 100644
> --- a/android/hal-bluetooth.c
> +++ b/android/hal-bluetooth.c
> @@ -166,6 +166,21 @@ static void device_props_to_hal(bt_property_t *send_props,
> 			break;
> #endif
> 		case HAL_PROP_DEVICE_SERVICE_REC:
> +		{
> +			static bt_service_record_t e;

why static here. Since we are essentially multi-threaded, this can be dangerous.

> +			const struct hal_prop_device_service_rec *p;
> +
> +			send_props[i].val = &e;
> +			send_props[i].len = sizeof(e);
> +
> +			p = (struct hal_prop_device_service_rec *) prop->val;
> +
> +			memset(&e, 0, sizeof(e));
> +			memcpy(&e.channel, &p->channel, sizeof(e.channel));
> +			memcpy(e.uuid.uu, p->uuid, sizeof(e.uuid.uu));
> +			memcpy(e.name, p->name, p->name_len);
> +		}
> +			break;
> 		default:
> 			send_props[i].len = prop->len;
> 			send_props[i].val = prop->val;

Regards

Marcel


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/5] android/hal-bluetooth: Add support for device service record property
  2013-12-20 13:18   ` Marcel Holtmann
@ 2013-12-20 13:38     ` Szymon Janc
  2013-12-20 13:43       ` Marcel Holtmann
  0 siblings, 1 reply; 13+ messages in thread
From: Szymon Janc @ 2013-12-20 13:38 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org development

Hi Marcel,

> Hi Szymon,
> 
> > This allows to correctly handle device service record property.
> > ---
> > android/hal-bluetooth.c | 15 +++++++++++++++
> > android/hal-msg.h       |  7 +++++++
> > 2 files changed, 22 insertions(+)
> > 
> > diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
> > index d0c1c01..4ef2ebe 100644
> > --- a/android/hal-bluetooth.c
> > +++ b/android/hal-bluetooth.c
> > @@ -166,6 +166,21 @@ static void device_props_to_hal(bt_property_t *send_props,
> > 			break;
> > #endif
> > 		case HAL_PROP_DEVICE_SERVICE_REC:
> > +		{
> > +			static bt_service_record_t e;
> 
> why static here. Since we are essentially multi-threaded, this can be dangerous.

This is executed only in notification thread context, so not multi-threading here.
(yes, commit message could be better)

-- 
BR
Szymon Janc

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/5] android/hal-bluetooth: Add support for device service record property
  2013-12-20 13:38     ` Szymon Janc
@ 2013-12-20 13:43       ` Marcel Holtmann
  2013-12-20 14:00         ` Szymon Janc
  0 siblings, 1 reply; 13+ messages in thread
From: Marcel Holtmann @ 2013-12-20 13:43 UTC (permalink / raw)
  To: Szymon Janc; +Cc: linux-bluetooth@vger.kernel.org development

Hi Szymon,

>>> This allows to correctly handle device service record property.
>>> ---
>>> android/hal-bluetooth.c | 15 +++++++++++++++
>>> android/hal-msg.h       |  7 +++++++
>>> 2 files changed, 22 insertions(+)
>>> 
>>> diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
>>> index d0c1c01..4ef2ebe 100644
>>> --- a/android/hal-bluetooth.c
>>> +++ b/android/hal-bluetooth.c
>>> @@ -166,6 +166,21 @@ static void device_props_to_hal(bt_property_t *send_props,
>>> 			break;
>>> #endif
>>> 		case HAL_PROP_DEVICE_SERVICE_REC:
>>> +		{
>>> +			static bt_service_record_t e;
>> 
>> why static here. Since we are essentially multi-threaded, this can be dangerous.
> 
> This is executed only in notification thread context, so not multi-threading here.
> (yes, commit message could be better)

and what is wrong with having it on the stack.

Regards

Marcel


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/5] android/hal-bluetooth: Add support for device service record property
  2013-12-20 13:43       ` Marcel Holtmann
@ 2013-12-20 14:00         ` Szymon Janc
  0 siblings, 0 replies; 13+ messages in thread
From: Szymon Janc @ 2013-12-20 14:00 UTC (permalink / raw)
  To: Marcel Holtmann; +Cc: linux-bluetooth@vger.kernel.org development

Hi Marcel,

> Hi Szymon,
> 
> >>> This allows to correctly handle device service record property.
> >>> ---
> >>> android/hal-bluetooth.c | 15 +++++++++++++++
> >>> android/hal-msg.h       |  7 +++++++
> >>> 2 files changed, 22 insertions(+)
> >>> 
> >>> diff --git a/android/hal-bluetooth.c b/android/hal-bluetooth.c
> >>> index d0c1c01..4ef2ebe 100644
> >>> --- a/android/hal-bluetooth.c
> >>> +++ b/android/hal-bluetooth.c
> >>> @@ -166,6 +166,21 @@ static void device_props_to_hal(bt_property_t *send_props,
> >>> 			break;
> >>> #endif
> >>> 		case HAL_PROP_DEVICE_SERVICE_REC:
> >>> +		{
> >>> +			static bt_service_record_t e;
> >> 
> >> why static here. Since we are essentially multi-threaded, this can be dangerous.
> > 
> > This is executed only in notification thread context, so not multi-threading here.
> > (yes, commit message could be better)
> 
> and what is wrong with having it on the stack.

This function is used to convert from what we get on IPC to HAL, so that data
needs to be also accessible outside of this function scope (so Java can copy it).
Since we won't get more than 1 prop of each type in single notification this is
safe and allows for simpler code.

-- 
BR
Szymon Janc

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2013-12-20 14:00 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-12-20 10:25 [PATCH 0/5] android: Update HAL to Android 4.4 API Szymon Janc
2013-12-20 10:25 ` [PATCH 1/5] android/hal-bluetooth: Add support for sending LE_TEST_MODE command Szymon Janc
2013-12-20 11:57   ` Anderson Lizardo
2013-12-20 10:25 ` [PATCH 2/5] android/hal-bluetooth: Add support for enabling HCI snoop dump Szymon Janc
2013-12-20 10:25 ` [PATCH 3/5] android/hal-bluetooth: Add support for remote version info property Szymon Janc
2013-12-20 12:07   ` Anderson Lizardo
2013-12-20 10:25 ` [PATCH 4/5] android/hal-bluetooth: Add support for device service record property Szymon Janc
2013-12-20 13:18   ` Marcel Holtmann
2013-12-20 13:38     ` Szymon Janc
2013-12-20 13:43       ` Marcel Holtmann
2013-12-20 14:00         ` Szymon Janc
2013-12-20 10:25 ` [PATCH 5/5] android/bluetooth: Add stubs for missing commands Szymon Janc
2013-12-20 10:58 ` [PATCH 0/5] android: Update HAL to Android 4.4 API Johan Hedberg

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox