Linux bluetooth development
 help / color / mirror / Atom feed
* [PATCH 1/6] Implement Find by Type request encode/decoding
From: Claudio Takahasi @ 2010-11-17 18:09 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Bruna Moreira

From: Bruna Moreira <bruna.moreira@openbossa.org>

---
 attrib/att.c |   72 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 attrib/att.h |    4 ++-
 2 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/attrib/att.c b/attrib/att.c
index fe41d0e..6c889f2 100644
--- a/attrib/att.c
+++ b/attrib/att.c
@@ -198,9 +198,77 @@ struct att_data_list *dec_read_by_grp_resp(const uint8_t *pdu, int len)
 }
 
 uint16_t enc_find_by_type_req(uint16_t start, uint16_t end, uuid_t *uuid,
-							uint8_t *pdu, int len)
+			const uint8_t *value, int vlen, uint8_t *pdu, int len)
+{
+	uint16_t min_len = sizeof(pdu[0]) + sizeof(start) + sizeof(end) +
+							sizeof(uint16_t);
+
+	if (pdu == NULL)
+		return 0;
+
+	if (!uuid)
+		return 0;
+
+	if (uuid->type != SDP_UUID16)
+		return 0;
+
+	if (len < min_len)
+		return 0;
+
+	if (vlen > len - min_len)
+		vlen = len - min_len;
+
+	pdu[0] = ATT_OP_FIND_BY_TYPE_REQ;
+	att_put_u16(start, &pdu[1]);
+	att_put_u16(end, &pdu[3]);
+	att_put_u16(uuid->value.uuid16, &pdu[5]);
+
+	if (vlen > 0) {
+		memcpy(&pdu[7], value, vlen);
+		return min_len + vlen;
+	}
+
+	return min_len;
+}
+
+uint16_t dec_find_by_type_req(const uint8_t *pdu, int len, uint16_t *start,
+			uint16_t *end, uuid_t *uuid, uint8_t *value, int *vlen)
 {
-	return 0;
+	int valuelen;
+	uint16_t min_len = sizeof(pdu[0]) + sizeof(*start) +
+						sizeof(*end) + sizeof(uint16_t);
+
+	if (pdu == NULL)
+		return 0;
+
+	if (len < min_len)
+		return 0;
+
+	if (pdu[0] != ATT_OP_FIND_BY_TYPE_REQ)
+		return 0;
+
+	/* First requested handle number */
+	if (start)
+		*start = att_get_u16(&pdu[1]);
+
+	/* Last requested handle number */
+	if (end)
+		*end = att_get_u16(&pdu[3]);
+
+	/* Always UUID16 */
+	if (uuid)
+		sdp_uuid16_create(uuid, att_get_u16(&pdu[5]));
+
+	valuelen = len - min_len;
+
+	/* Attribute value to find */
+	if (valuelen > 0 && value)
+		memcpy(value, pdu + min_len, valuelen);
+
+	if (vlen)
+		*vlen = valuelen;
+
+	return len;
 }
 
 uint16_t enc_read_by_type_req(uint16_t start, uint16_t end, uuid_t *uuid,
diff --git a/attrib/att.h b/attrib/att.h
index ea49dc2..9de338d 100644
--- a/attrib/att.h
+++ b/attrib/att.h
@@ -165,7 +165,9 @@ uint16_t dec_read_by_grp_req(const uint8_t *pdu, int len, uint16_t *start,
 						uint16_t *end, uuid_t *uuid);
 uint16_t enc_read_by_grp_resp(struct att_data_list *list, uint8_t *pdu, int len);
 uint16_t enc_find_by_type_req(uint16_t start, uint16_t end, uuid_t *uuid,
-							uint8_t *pdu, int len);
+			const uint8_t *value, int vlen, uint8_t *pdu, int len);
+uint16_t dec_find_by_type_req(const uint8_t *pdu, int len, uint16_t *start,
+		uint16_t *end, uuid_t *uuid, uint8_t *value, int *vlen);
 struct att_data_list *dec_read_by_grp_resp(const uint8_t *pdu, int len);
 uint16_t enc_read_by_type_req(uint16_t start, uint16_t end, uuid_t *uuid,
 							uint8_t *pdu, int len);
-- 
1.7.3.2


^ permalink raw reply related

* [PATCH 2/6] Add Find By Type Value Response encoding/decoding functions
From: Claudio Takahasi @ 2010-11-17 18:09 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1290017386-23969-1-git-send-email-claudio.takahasi@openbossa.org>

Find by type operation is used by Discover Primary Service by Service
UUID. Find By Type Value Response shall contain one or more group handles.
---
 attrib/att.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 attrib/att.h |    7 +++++++
 2 files changed, 53 insertions(+), 0 deletions(-)

diff --git a/attrib/att.c b/attrib/att.c
index 6c889f2..60fc74b 100644
--- a/attrib/att.c
+++ b/attrib/att.c
@@ -30,6 +30,8 @@
 #include <bluetooth/sdp.h>
 #include <bluetooth/sdp_lib.h>
 
+#include <glib.h>
+
 #include "att.h"
 
 const char *att_ecode2str(uint8_t status)
@@ -271,6 +273,50 @@ uint16_t dec_find_by_type_req(const uint8_t *pdu, int len, uint16_t *start,
 	return len;
 }
 
+uint16_t enc_find_by_type_resp(GSList *matches, uint8_t *pdu, int len)
+{
+	GSList *l;
+	uint16_t offset;
+
+	if (pdu == NULL || len < 5)
+		return 0;
+
+	pdu[0] = ATT_OP_FIND_BY_TYPE_RESP;
+
+	for (l = matches, offset = 1; l && len >= (offset + 4);
+					l = l->next, offset += 4) {
+		struct range *range = l->data;
+
+		att_put_u16(range->start, &pdu[offset]);
+		att_put_u16(range->end, &pdu[offset + 2]);
+	}
+
+	return offset;
+}
+
+GSList *dec_find_by_type_resp(const uint8_t *pdu, int len)
+{
+	struct range *range;
+	GSList *matches;
+	int offset;
+
+	if (pdu == NULL || len < 5)
+		return NULL;
+
+	if (pdu[0] != ATT_OP_FIND_BY_TYPE_RESP)
+		return NULL;
+
+	for (offset = 1, matches = NULL; len >= (offset + 4); offset += 4) {
+		range = malloc(sizeof(struct range));
+		range->start = att_get_u16(&pdu[offset]);
+		range->end = att_get_u16(&pdu[offset + 2]);
+
+		matches = g_slist_append(matches, range);
+	}
+
+	return matches;
+}
+
 uint16_t enc_read_by_type_req(uint16_t start, uint16_t end, uuid_t *uuid,
 							uint8_t *pdu, int len)
 {
diff --git a/attrib/att.h b/attrib/att.h
index 9de338d..c7260ff 100644
--- a/attrib/att.h
+++ b/attrib/att.h
@@ -122,6 +122,11 @@ struct att_data_list {
 	uint8_t **data;
 };
 
+struct range {
+	uint16_t start;
+	uint16_t end;
+};
+
 /* These functions do byte conversion */
 static inline uint8_t att_get_u8(const void *ptr)
 {
@@ -168,6 +173,8 @@ uint16_t enc_find_by_type_req(uint16_t start, uint16_t end, uuid_t *uuid,
 			const uint8_t *value, int vlen, uint8_t *pdu, int len);
 uint16_t dec_find_by_type_req(const uint8_t *pdu, int len, uint16_t *start,
 		uint16_t *end, uuid_t *uuid, uint8_t *value, int *vlen);
+uint16_t enc_find_by_type_resp(GSList *ranges, uint8_t *pdu, int len);
+GSList *dec_find_by_type_resp(const uint8_t *pdu, int len);
 struct att_data_list *dec_read_by_grp_resp(const uint8_t *pdu, int len);
 uint16_t enc_read_by_type_req(uint16_t start, uint16_t end, uuid_t *uuid,
 							uint8_t *pdu, int len);
-- 
1.7.3.2


^ permalink raw reply related

* [PATCH 3/6] Implement Find by Type Value Request in the atttribute server
From: Claudio Takahasi @ 2010-11-17 18:09 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1290017386-23969-1-git-send-email-claudio.takahasi@openbossa.org>

GATT Discover Primary Service by Service UUID sub-procedure is based
on ATT Find By Type Value Request/Response.

Implement an extra verification for broken requests: "Ending Handle"
different than 0xFFFF. The Group End Handle may be greater than the
"Ending Handle" in the Find By Type Value Request. Forces the "Ending
Handle" in the response to 0xFFFF to avoid another request from the
clients. 0xFFFF means that the sub-procedure is complete.
---
 src/attrib-server.c |   80 +++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 80 insertions(+), 0 deletions(-)

diff --git a/src/attrib-server.c b/src/attrib-server.c
index 375b731..05a0fb7 100644
--- a/src/attrib-server.c
+++ b/src/attrib-server.c
@@ -383,6 +383,76 @@ static int find_info(uint16_t start, uint16_t end, uint8_t *pdu, int len)
 	return length;
 }
 
+static int find_by_type(uint16_t start, uint16_t end, uuid_t *uuid,
+			const uint8_t *value, int vlen, uint8_t *opdu, int mtu)
+{
+	struct attribute *a;
+	struct range *range;
+	GSList *l, *matches;
+	int len;
+
+	if (start > end || start == 0x0000)
+		return enc_error_resp(ATT_OP_FIND_BY_TYPE_REQ, start,
+					ATT_ECODE_INVALID_HANDLE, opdu, mtu);
+
+	/* Searching first requested handle number */
+	for (l = database, matches = NULL, range = NULL; l; l = l->next) {
+		a = l->data;
+
+		if (a->handle < start)
+			continue;
+
+		if (a->handle > end)
+			break;
+
+		/* Primary service? Attribute value matches? */
+		if ((sdp_uuid_cmp(&a->uuid, uuid) == 0) && (a->len == vlen) &&
+					(memcmp(a->data, value, vlen) == 0)) {
+
+			range = g_new0(struct range, 1);
+			range->start = a->handle;
+
+			matches = g_slist_append(matches, range);
+		} else if (range) {
+			/*
+			 * Update the last found handle or reset the pointer
+			 * to track that a new group started: Primary or
+			 * Secondary service.
+			 */
+			if (sdp_uuid_cmp(&a->uuid, &prim_uuid) == 0 ||
+					sdp_uuid_cmp(&a->uuid, &snd_uuid) == 0)
+				range = NULL;
+			else
+				range->end = a->handle;
+		}
+	}
+
+	if (range) {
+		if (l == NULL) {
+			/* Avoids another iteration */
+			range->end = 0xFFFF;
+		} else if (range->end == 0) {
+			/*
+			 * Broken requests: requested End Handle is not 0xFFFF.
+			 * Given handle is in the middle of a service definition.
+			 */
+			matches = g_slist_remove(matches, range);
+			g_free(range);
+		}
+	}
+
+	if (matches == NULL)
+		return enc_error_resp(ATT_OP_FIND_BY_TYPE_REQ, start,
+				ATT_ECODE_ATTR_NOT_FOUND, opdu, mtu);
+
+	len = enc_find_by_type_resp(matches, opdu, mtu);
+
+	g_slist_foreach(matches, (GFunc) g_free, NULL);
+	g_slist_free(matches);
+
+	return len;
+}
+
 static int handle_cmp(gconstpointer a, gconstpointer b)
 {
 	const struct attribute *attrib = a;
@@ -522,6 +592,16 @@ static void channel_handler(const uint8_t *ipdu, uint16_t len,
 			write_value(start, value, vlen);
 		return;
 	case ATT_OP_FIND_BY_TYPE_REQ:
+		length = dec_find_by_type_req(ipdu, len, &start, &end,
+							&uuid, value, &vlen);
+		if (length == 0) {
+			status = ATT_ECODE_INVALID_PDU;
+			goto done;
+		}
+
+		length = find_by_type(start, end, &uuid, value, vlen,
+							opdu, channel->mtu);
+		break;
 	case ATT_OP_READ_BLOB_REQ:
 	case ATT_OP_READ_MULTI_REQ:
 	case ATT_OP_PREP_WRITE_REQ:
-- 
1.7.3.2


^ permalink raw reply related

* [PATCH 4/6] Extend bt_string2uuid to convert hex strings to UUID16
From: Claudio Takahasi @ 2010-11-17 18:09 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1290017386-23969-1-git-send-email-claudio.takahasi@openbossa.org>

Convert four or six(0x) digits length hexadecimal strings to UUID16.
---
 src/glib-helper.c |   22 ++++++++++++++++++++--
 1 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/src/glib-helper.c b/src/glib-helper.c
index 9d76626..232fc71 100644
--- a/src/glib-helper.c
+++ b/src/glib-helper.c
@@ -466,6 +466,24 @@ static inline gboolean is_uuid128(const char *string)
 			string[23] == '-');
 }
 
+static int string2uuid16(uuid_t *uuid, const char *string)
+{
+	int length = strlen(string);
+	char *endptr = NULL;
+	uint16_t u16;
+
+	if (length != 4 && length != 6)
+		return -EINVAL;
+
+	u16 = strtol(string, &endptr, 16);
+	if (endptr && *endptr == '\0') {
+		sdp_uuid16_create(uuid, u16);
+		return 0;
+	}
+
+	return -EINVAL;
+}
+
 char *bt_name2string(const char *pattern)
 {
 	uuid_t uuid;
@@ -529,9 +547,9 @@ int bt_string2uuid(uuid_t *uuid, const char *string)
 			sdp_uuid16_create(uuid, class);
 			return 0;
 		}
-	}
 
-	return -1;
+		return string2uuid16(uuid, string);
+	}
 }
 
 gchar *bt_list2string(GSList *list)
-- 
1.7.3.2


^ permalink raw reply related

* [PATCH 5/6] Add an extra parameter in the discovery primary to specify the UUID
From: Claudio Takahasi @ 2010-11-17 18:09 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1290017386-23969-1-git-send-email-claudio.takahasi@openbossa.org>

Extends discover primary function to perform discover by UUID. UUID
parameter defines which procedure will be executed: Discover All
Primary Services or Discover Primary Service by Service UUID.
---
 attrib/client.c   |    8 ++++----
 attrib/gatt.c     |   37 ++++++++++++++++++++++++++++++-------
 attrib/gatt.h     |    4 ++--
 attrib/gatttool.c |    6 ++++--
 4 files changed, 40 insertions(+), 15 deletions(-)

diff --git a/attrib/client.c b/attrib/client.c
index 955e623..a851a74 100644
--- a/attrib/client.c
+++ b/attrib/client.c
@@ -363,8 +363,8 @@ static void connect_cb(GIOChannel *chan, GError *gerr, gpointer user_data)
 		return;
 	}
 
-	atid = gatt_discover_primary(gatt->attrib, 0x0001, 0xffff, primary_cb,
-									gatt);
+	atid = gatt_discover_primary(gatt->attrib, 0x0001, 0xffff, NULL,
+							primary_cb, gatt);
 	if (atid == 0)
 		goto fail;
 
@@ -1311,8 +1311,8 @@ static void primary_cb(guint8 status, const guint8 *pdu, guint16 plen,
 	 * the Error Code is set to Attribute Not Found.
 	 */
 	gatt->attrib = g_attrib_ref(gatt->attrib);
-	gatt->atid = gatt_discover_primary(gatt->attrib,
-				end + 1, 0xffff, primary_cb, gatt);
+	gatt->atid = gatt_discover_primary(gatt->attrib, end + 1, 0xffff, NULL,
+							primary_cb, gatt);
 done:
 	g_attrib_unref(gatt->attrib);
 }
diff --git a/attrib/gatt.c b/attrib/gatt.c
index 24ec990..2c87daf 100644
--- a/attrib/gatt.c
+++ b/attrib/gatt.c
@@ -31,21 +31,44 @@
 #include "gattrib.h"
 #include "gatt.h"
 
-guint gatt_discover_primary(GAttrib *attrib, uint16_t start,
-		uint16_t end, GAttribResultFunc func, gpointer user_data)
+guint gatt_discover_primary(GAttrib *attrib, uint16_t start, uint16_t end,
+		uuid_t *uuid, GAttribResultFunc func, gpointer user_data)
 {
 	uint8_t pdu[ATT_DEFAULT_MTU];
-	uuid_t uuid;
+	uuid_t prim;
 	guint16 plen;
+	uint8_t op;
+
+	sdp_uuid16_create(&prim, GATT_PRIM_SVC_UUID);
+
+	if (uuid == NULL) {
+
+		/* Discover all primary services */
+		op = ATT_OP_READ_BY_GROUP_REQ;
+		plen = enc_read_by_grp_req(start, end, &prim, pdu, sizeof(pdu));
+	} else {
+		const void *value;
+		int vlen;
 
-	sdp_uuid16_create(&uuid, GATT_PRIM_SVC_UUID);
+		/* Discover primary service by service UUID */
+		op = ATT_OP_FIND_BY_TYPE_REQ;
+
+		if (uuid->type == SDP_UUID16) {
+			value = &uuid->value.uuid16;
+			vlen = sizeof(uuid->value.uuid16);
+		} else {
+			value = &uuid->value.uuid128;
+			vlen = sizeof(uuid->value.uuid128);
+		}
+
+		plen = enc_find_by_type_req(start, end, &prim, value, vlen,
+							pdu, sizeof(pdu));
+	}
 
-	plen = enc_read_by_grp_req(start, end, &uuid, pdu, sizeof(pdu));
 	if (plen == 0)
 		return 0;
 
-	return g_attrib_send(attrib, ATT_OP_READ_BY_GROUP_REQ,
-					pdu, plen, func, user_data, NULL);
+	return g_attrib_send(attrib, op, pdu, plen, func, user_data, NULL);
 }
 
 guint gatt_discover_char(GAttrib *attrib, uint16_t start, uint16_t end,
diff --git a/attrib/gatt.h b/attrib/gatt.h
index f1599c2..4e7d88b 100644
--- a/attrib/gatt.h
+++ b/attrib/gatt.h
@@ -24,8 +24,8 @@
 
 #define GATT_CID 4
 
-guint gatt_discover_primary(GAttrib *attrib, uint16_t start,
-		uint16_t end, GAttribResultFunc func, gpointer user_data);
+guint gatt_discover_primary(GAttrib *attrib, uint16_t start, uint16_t end,
+		uuid_t *uuid, GAttribResultFunc func, gpointer user_data);
 
 guint gatt_discover_char(GAttrib *attrib, uint16_t start, uint16_t end,
 				GAttribResultFunc func, gpointer user_data);
diff --git a/attrib/gatttool.c b/attrib/gatttool.c
index b9f5138..ac9745d 100644
--- a/attrib/gatttool.c
+++ b/attrib/gatttool.c
@@ -191,7 +191,8 @@ static void primary_cb(guint8 status, const guint8 *pdu, guint16 plen,
 	 * Read by Group Type Request until Error Response is received and
 	 * the Error Code is set to Attribute Not Found.
 	 */
-	gatt_discover_primary(attrib, end + 1, opt_end, primary_cb, attrib);
+	gatt_discover_primary(attrib, end + 1, opt_end, NULL, primary_cb,
+								attrib);
 
 	return;
 
@@ -250,7 +251,8 @@ static gboolean primary(gpointer user_data)
 {
 	GAttrib *attrib = user_data;
 
-	gatt_discover_primary(attrib, opt_start, opt_end, primary_cb, attrib);
+	gatt_discover_primary(attrib, opt_start, opt_end, NULL, primary_cb,
+								attrib);
 
 	return FALSE;
 }
-- 
1.7.3.2


^ permalink raw reply related

* [PATCH 6/6] Implement Discover Primary Service by Service UUID in the gatttool
From: Claudio Takahasi @ 2010-11-17 18:09 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Claudio Takahasi
In-Reply-To: <1290017386-23969-1-git-send-email-claudio.takahasi@openbossa.org>

Implement only the first interaction of the discovery procedure. If the
response doesn't fit in the MTU, "start" and "end" options can be used
to discover the handles ranges of the remaining primary service instances.
UUID16 and UUID128 are supported in the uuid option.

Usage example:
$gatttool -i hcix -b xx:xx:xx:xx:xx:xx --uuid=1801 --primary
---
 Makefile.am       |    3 +-
 attrib/gatttool.c |   65 ++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index da308a7..1d83943 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -177,7 +177,8 @@ if ATTRIBPLUGIN
 bin_PROGRAMS += attrib/gatttool
 
 attrib_gatttool_SOURCES = attrib/gatttool.c attrib/att.c attrib/gatt.c \
-			  attrib/gattrib.c btio/btio.c
+			  attrib/gattrib.c btio/btio.c \
+			  src/glib-helper.h src/glib-helper.c
 attrib_gatttool_LDADD = lib/libbluetooth.la @GLIB_LIBS@
 
 builtin_modules += attrib
diff --git a/attrib/gatttool.c b/attrib/gatttool.c
index ac9745d..61afd52 100644
--- a/attrib/gatttool.c
+++ b/attrib/gatttool.c
@@ -41,6 +41,7 @@
 #include "att.h"
 #include "btio.h"
 #include "gattrib.h"
+#include "glib-helper.h"
 #include "gatt.h"
 
 /* Minimum MTU for L2CAP connections over BR/EDR */
@@ -49,6 +50,7 @@
 static gchar *opt_src = NULL;
 static gchar *opt_dst = NULL;
 static gchar *opt_value = NULL;
+static uuid_t *opt_uuid = NULL;
 static int opt_start = 0x0001;
 static int opt_end = 0xffff;
 static int opt_handle = -1;
@@ -135,7 +137,7 @@ static GIOChannel *do_connect(gboolean le)
 	return chan;
 }
 
-static void primary_cb(guint8 status, const guint8 *pdu, guint16 plen,
+static void primary_all_cb(guint8 status, const guint8 *pdu, guint16 plen,
 							gpointer user_data)
 {
 	GAttrib *attrib = user_data;
@@ -191,9 +193,9 @@ static void primary_cb(guint8 status, const guint8 *pdu, guint16 plen,
 	 * Read by Group Type Request until Error Response is received and
 	 * the Error Code is set to Attribute Not Found.
 	 */
-	gatt_discover_primary(attrib, end + 1, opt_end, NULL, primary_cb,
-								attrib);
 
+	gatt_discover_primary(attrib, end + 1, opt_end, NULL, primary_all_cb,
+								attrib);
 	return;
 
 done:
@@ -201,6 +203,36 @@ done:
 		g_main_loop_quit(event_loop);
 }
 
+static void primary_by_uuid_cb(guint8 status, const guint8 *pdu, guint16 plen,
+							gpointer user_data)
+{
+	GSList *ranges, *l;
+
+	if (status != 0) {
+		g_printerr("Discover primary services by UUID failed: %s\n",
+							att_ecode2str(status));
+		goto done;
+	}
+
+	ranges = dec_find_by_type_resp(pdu, plen);
+	if (ranges == NULL) {
+		g_printerr("Protocol error!\n");
+		goto done;
+	}
+
+	for (l = ranges; l; l = l->next) {
+		struct range *range = l->data;
+		g_print("Starting handle: %04x Ending handle: %04x\n",
+						range->start, range->end);
+	}
+
+	g_slist_foreach(ranges, (GFunc) g_free, NULL);
+	g_slist_free(ranges);
+
+done:
+	g_main_loop_quit(event_loop);
+}
+
 static void events_handler(const uint8_t *pdu, uint16_t len, gpointer user_data)
 {
 	GAttrib *attrib = user_data;
@@ -251,8 +283,12 @@ static gboolean primary(gpointer user_data)
 {
 	GAttrib *attrib = user_data;
 
-	gatt_discover_primary(attrib, opt_start, opt_end, NULL, primary_cb,
-								attrib);
+	if (opt_uuid)
+		gatt_discover_primary(attrib, opt_start, opt_end, opt_uuid,
+						primary_by_uuid_cb, attrib);
+	else
+		gatt_discover_primary(attrib, opt_start, opt_end, NULL,
+						primary_all_cb, attrib);
 
 	return FALSE;
 }
@@ -477,11 +513,29 @@ static gboolean characteristics_desc(gpointer user_data)
 	return FALSE;
 }
 
+static gboolean parse_uuid(const char *key, const char *value,
+				gpointer user_data, GError **error)
+{
+	if (!value)
+		return FALSE;
+
+	opt_uuid = g_try_malloc(sizeof(uuid_t));
+	if (opt_uuid == NULL)
+		return FALSE;
+
+	if (bt_string2uuid(opt_uuid, value) < 0)
+		return FALSE;
+
+	return TRUE;
+}
+
 static GOptionEntry primary_char_options[] = {
 	{ "start", 's' , 0, G_OPTION_ARG_INT, &opt_start,
 		"Starting handle(optional)", "0x0001" },
 	{ "end", 'e' , 0, G_OPTION_ARG_INT, &opt_end,
 		"Ending handle(optional)", "0xffff" },
+	{ "uuid", 'u', G_OPTION_FLAG_OPTIONAL_ARG, G_OPTION_ARG_CALLBACK,
+		parse_uuid, "UUID16 or UUID128(optional)", "0x1801"},
 	{ NULL },
 };
 
@@ -610,6 +664,7 @@ done:
 	g_option_context_free(context);
 	g_free(opt_src);
 	g_free(opt_dst);
+	g_free(opt_uuid);
 
 	if (got_error)
 		exit(EXIT_FAILURE);
-- 
1.7.3.2


^ permalink raw reply related

* Re: Apple Wireless Keyboard connection issue
From: Johan Hedberg @ 2010-11-17 22:27 UTC (permalink / raw)
  To: Andre Kuehne; +Cc: Gustavo F. Padovan, linux-bluetooth
In-Reply-To: <4CE45F63.7030902@gmx.net>

Hi,

On Thu, Nov 18, 2010, Andre Kuehne wrote:
> On 11/17/2010 10:05 AM, Johan Hedberg wrote:
> >On Tue, Nov 16, 2010, Gustavo F. Padovan wrote:
> >>* "André Kühne"<andre.kuehne@gmx.net>  [2010-11-16 22:27:01 +0100]:
> >>>I noticed the following on my system: After upgrading to bluez-4.79
> >>>connecting my Apple Wireless Keyboard does not work anymore. With
> >>>bluez-4.77 the connection works just fine.
> >>
> >>My Microsoft keyboard is not working too. That is probably due to commit
> >>abe7cd44124a from Johan. It should be fixed soon.
> >
> >The only real difference that patch makes is the reuse of the HCI socket
> >inside hciops.c. Maybe that screws up the event filters somehow or
> >something similar. I don't have a keyboard to verify a fix, but could
> >you try the attached patch and see if it helps?
> >
> I installed
> 
>   http://www.kernel.org/pub/linux/bluetooth/bluez-4.79.tar.gz + hciops_encrypt.patch
> 
> but the patch does not work for me. For verification I downloaded
> 
>   http://www.kernel.org/pub/linux/bluetooth/bluez-4.77.tar.gz
> 
> and compiled/installed it the same way (without the patch) and the connection works.
> 
> I did the same using bluez-4.78 with the result that both my keyboard and mouse failed to connect.

Ok, so then it seems like abe7cd44124a might not be to blame. Could you
do a proper git bisect so we can figure out exactly at which point this
broke. abe7cd44124a is really the only commit touching HID code since
4.77 so this is starting to get a bit strange.

Johan

^ permalink raw reply

* Re: [PATCH 3/3] Bluetooth: Automate remote name requests
From: Johan Hedberg @ 2010-11-17 22:30 UTC (permalink / raw)
  To: Gustavo F. Padovan; +Cc: linux-bluetooth
In-Reply-To: <20101116220907.GA30115@vigoh>

Hi Gustavo,

On Tue, Nov 16, 2010, Gustavo F. Padovan wrote:
> > -static int request_outgoing_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
> > +static int outgoing_auth_needed(struct hci_dev *hdev, bdaddr_t *bdaddr)
> 
> Can you add a hci_ in the beginning of your functions, just to keep the
> coherency with the rest of the code.

Sure. I guess I'm too used to the userspace convention where strict
namespacing is only used for exported (non-static) functions.

> I'm not happy with have to lookup the hci_conn twice when we can do that
> once here. I've noted that always outgoing_auth_needed() returns 1 you do
> a request_auth, and always it returns 0 you don't, so I think we can
> embed request_auth() inside outgoing_auth_needed() as it was in patch
> 2/3 and the give a better name to outgoing_auth_needed() which you
> reflect the new behavior.

I don't think the check and request should be in the same function since
there are two places that need the check but not the request. What I can
do though (or actually what I already did) is move the hci_conn lookup
outside of the functions so it's not done multiple times. Will send new
patches in a minute.

Johan

^ permalink raw reply

* Re: [PATCH 3/3] Bluetooth: Automate remote name requests
From: Gustavo F. Padovan @ 2010-11-17 23:01 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <20101117223057.GB20735@jh-x301>

* Johan Hedberg <johan.hedberg@gmail.com> [2010-11-18 00:30:57 +0200]:

> Hi Gustavo,
> 
> On Tue, Nov 16, 2010, Gustavo F. Padovan wrote:
> > > -static int request_outgoing_auth(struct hci_dev *hdev, bdaddr_t *bdaddr)
> > > +static int outgoing_auth_needed(struct hci_dev *hdev, bdaddr_t *bdaddr)
> > 
> > Can you add a hci_ in the beginning of your functions, just to keep the
> > coherency with the rest of the code.
> 
> Sure. I guess I'm too used to the userspace convention where strict
> namespacing is only used for exported (non-static) functions.
> 
> > I'm not happy with have to lookup the hci_conn twice when we can do that
> > once here. I've noted that always outgoing_auth_needed() returns 1 you do
> > a request_auth, and always it returns 0 you don't, so I think we can
> > embed request_auth() inside outgoing_auth_needed() as it was in patch
> > 2/3 and the give a better name to outgoing_auth_needed() which you
> > reflect the new behavior.
> 
> I don't think the check and request should be in the same function since
> there are two places that need the check but not the request.

In that case such function will return after the check not doing any
request.

-- 
Gustavo F. Padovan
http://profusion.mobi

^ permalink raw reply

* Re: Apple Wireless Keyboard connection issue
From: Andre Kuehne @ 2010-11-17 23:04 UTC (permalink / raw)
  To: Gustavo F. Padovan, linux-bluetooth
In-Reply-To: <20101117090534.GA11494@jh-x301>

Hi Johan

On 11/17/2010 10:05 AM, Johan Hedberg wrote:
> Hi Gustavo,
>
> On Tue, Nov 16, 2010, Gustavo F. Padovan wrote:
>> * "André Kühne"<andre.kuehne@gmx.net>  [2010-11-16 22:27:01 +0100]:
>>> I noticed the following on my system: After upgrading to bluez-4.79
>>> connecting my Apple Wireless Keyboard does not work anymore. With
>>> bluez-4.77 the connection works just fine.
>>
>> My Microsoft keyboard is not working too. That is probably due to commit
>> abe7cd44124a from Johan. It should be fixed soon.
>
> The only real difference that patch makes is the reuse of the HCI socket
> inside hciops.c. Maybe that screws up the event filters somehow or
> something similar. I don't have a keyboard to verify a fix, but could
> you try the attached patch and see if it helps?
>
I installed

   http://www.kernel.org/pub/linux/bluetooth/bluez-4.79.tar.gz + hciops_encrypt.patch

but the patch does not work for me. For verification I downloaded

   http://www.kernel.org/pub/linux/bluetooth/bluez-4.77.tar.gz

and compiled/installed it the same way (without the patch) and the connection works.

I did the same using bluez-4.78 with the result that both my keyboard and mouse failed to connect.

Best regards
Andre

^ permalink raw reply

* Re: [PATCH 1/7] Bluetooth: Hold the lock inside l2cap_get_sock_by_addr()
From: Gustavo F. Padovan @ 2010-11-17 23:11 UTC (permalink / raw)
  To: Ville Tervo; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <20101105143711.GB9116@vigoh>

* Gustavo F. Padovan <padovan@profusion.mobi> [2010-11-05 10:37:11 -0400]:

> Hi Ville,
> 
> * Ville Tervo <ville.tervo@nokia.com> [2010-11-05 15:49:35 +0200]:
> 
> > Hi Gustavo,
> > 
> > On Tue, Nov 02, 2010 at 04:03:12PM +0100, ext Gustavo F. Padovan wrote:
> > > It also have to change the name of the function to
> > > l2cap_get_sock_by_addr() because we do hold the lock inside it now.
> > > 
> > > Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> > > ---
> > >  net/bluetooth/l2cap.c |   17 ++++++-----------
> > >  1 files changed, 6 insertions(+), 11 deletions(-)
> > > 
> > > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > > index 6f931cc..3d48867 100644
> > > --- a/net/bluetooth/l2cap.c
> > > +++ b/net/bluetooth/l2cap.c
> > > @@ -728,15 +728,18 @@ static inline void l2cap_chan_add(struct l2cap_conn *conn, struct sock *sk, stru
> > >  }
> > >  
> > >  /* ---- Socket interface ---- */
> > > -static struct sock *__l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > > +static struct sock *l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > >  {
> > >  	struct sock *sk;
> > >  	struct hlist_node *node;
> > > +
> > > +	write_lock_bh(&l2cap_sk_list.lock);
> > 
> > Code is only reading so read_lock_bh would be enough?
> 
> Sure, I didn't looked to that, I just keept the same code that we were
> using before. I'll fix it.

I figured out that we need write_lock_bh() here, because set the psm and
sport is like a new element to the list. l2cap_get_sock_by_addr()
searches for either psm or sport. 

I'm also dropping the option to use RCU on the bt_sk_list(), It does not
fit on our case. We can't have anyone writing the list while we are
reading it.

-- 
Gustavo F. Padovan
http://profusion.mobi

^ permalink raw reply

* Re: [PATCH 1/7] Bluetooth: Hold the lock inside l2cap_get_sock_by_addr()
From: Gustavo F. Padovan @ 2010-11-17 23:17 UTC (permalink / raw)
  To: Ville Tervo; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <20101117231141.GB32261@vigoh>

* Gustavo F. Padovan <padovan@profusion.mobi> [2010-11-17 21:11:41 -0200]:

> * Gustavo F. Padovan <padovan@profusion.mobi> [2010-11-05 10:37:11 -0400]:
> 
> > Hi Ville,
> > 
> > * Ville Tervo <ville.tervo@nokia.com> [2010-11-05 15:49:35 +0200]:
> > 
> > > Hi Gustavo,
> > > 
> > > On Tue, Nov 02, 2010 at 04:03:12PM +0100, ext Gustavo F. Padovan wrote:
> > > > It also have to change the name of the function to
> > > > l2cap_get_sock_by_addr() because we do hold the lock inside it now.
> > > > 
> > > > Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
> > > > ---
> > > >  net/bluetooth/l2cap.c |   17 ++++++-----------
> > > >  1 files changed, 6 insertions(+), 11 deletions(-)
> > > > 
> > > > diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
> > > > index 6f931cc..3d48867 100644
> > > > --- a/net/bluetooth/l2cap.c
> > > > +++ b/net/bluetooth/l2cap.c
> > > > @@ -728,15 +728,18 @@ static inline void l2cap_chan_add(struct l2cap_conn *conn, struct sock *sk, stru
> > > >  }
> > > >  
> > > >  /* ---- Socket interface ---- */
> > > > -static struct sock *__l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > > > +static struct sock *l2cap_get_sock_by_addr(__le16 psm, bdaddr_t *src)
> > > >  {
> > > >  	struct sock *sk;
> > > >  	struct hlist_node *node;
> > > > +
> > > > +	write_lock_bh(&l2cap_sk_list.lock);
> > > 
> > > Code is only reading so read_lock_bh would be enough?
> > 
> > Sure, I didn't looked to that, I just keept the same code that we were
> > using before. I'll fix it.
> 
> I figured out that we need write_lock_bh() here, because set the psm and
> sport is like a new element to the list. l2cap_get_sock_by_addr()
> searches for either psm or sport. 
> 
> I'm also dropping the option to use RCU on the bt_sk_list(), It does not
> fit on our case. We can't have anyone writing the list while we are
> reading it.

That said, only patch 4 and 5 are still valid (I'll resend them), and 6 is
so trivial that I put it upstream already.  

-- 
Gustavo F. Padovan
http://profusion.mobi

^ permalink raw reply

* [PATCH 1/2] Bluetooth: Get ride of __l2cap_get_sock_by_psm()
From: Gustavo F. Padovan @ 2010-11-17 23:22 UTC (permalink / raw)
  To: linux-bluetooth

l2cap_get_sock_by_psm() was the only user of this function, so I merged
both into l2cap_get_sock_by_psm(). The socket lock now should be hold
outside of l2cap_get_sock_by_psm() once we hold and release it inside the
same function now.

Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
---
 net/bluetooth/l2cap.c |   22 +++++++++-------------
 1 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/net/bluetooth/l2cap.c b/net/bluetooth/l2cap.c
index 18a802c..12b4aa2 100644
--- a/net/bluetooth/l2cap.c
+++ b/net/bluetooth/l2cap.c
@@ -751,11 +751,13 @@ found:
 /* Find socket with psm and source bdaddr.
  * Returns closest match.
  */
-static struct sock *__l2cap_get_sock_by_psm(int state, __le16 psm, bdaddr_t *src)
+static struct sock *l2cap_get_sock_by_psm(int state, __le16 psm, bdaddr_t *src)
 {
 	struct sock *sk = NULL, *sk1 = NULL;
 	struct hlist_node *node;
 
+	read_lock(&l2cap_sk_list.lock);
+
 	sk_for_each(sk, node, &l2cap_sk_list.head) {
 		if (state && sk->sk_state != state)
 			continue;
@@ -770,20 +772,10 @@ static struct sock *__l2cap_get_sock_by_psm(int state, __le16 psm, bdaddr_t *src
 				sk1 = sk;
 		}
 	}
-	return node ? sk : sk1;
-}
 
-/* Find socket with given address (psm, src).
- * Returns locked socket */
-static inline struct sock *l2cap_get_sock_by_psm(int state, __le16 psm, bdaddr_t *src)
-{
-	struct sock *s;
-	read_lock(&l2cap_sk_list.lock);
-	s = __l2cap_get_sock_by_psm(state, psm, src);
-	if (s)
-		bh_lock_sock(s);
 	read_unlock(&l2cap_sk_list.lock);
-	return s;
+
+	return node ? sk : sk1;
 }
 
 static void l2cap_sock_destruct(struct sock *sk)
@@ -2934,6 +2926,8 @@ static inline int l2cap_connect_req(struct l2cap_conn *conn, struct l2cap_cmd_hd
 		goto sendresp;
 	}
 
+	bh_lock_sock(parent);
+
 	/* Check if the ACL is secure enough (if not SDP) */
 	if (psm != cpu_to_le16(0x0001) &&
 				!hci_conn_check_link_mode(conn->hcon)) {
@@ -4464,6 +4458,8 @@ static inline int l2cap_conless_channel(struct l2cap_conn *conn, __le16 psm, str
 	if (!sk)
 		goto drop;
 
+	bh_lock_sock(sk);
+
 	BT_DBG("sk %p, len %d", sk, skb->len);
 
 	if (sk->sk_state != BT_BOUND && sk->sk_state != BT_CONNECTED)
-- 
1.7.3.1


^ permalink raw reply related

* [PATCH 2/2] Bluetooth: Get ride of __rfcomm_get_sock_by_channel()
From: Gustavo F. Padovan @ 2010-11-17 23:22 UTC (permalink / raw)
  To: linux-bluetooth
In-Reply-To: <1290036176-4022-1-git-send-email-padovan@profusion.mobi>

rfcomm_get_sock_by_channel() was the only user of this function, so I merged
both into rfcomm_get_sock_by_channel(). The socket lock now should be hold
outside of rfcomm_get_sock_by_channel() once we hold and release it inside the
same function now.

Signed-off-by: Gustavo F. Padovan <padovan@profusion.mobi>
---
 net/bluetooth/rfcomm/sock.c |   19 +++++++------------
 1 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/net/bluetooth/rfcomm/sock.c b/net/bluetooth/rfcomm/sock.c
index aec505f..0207bd6 100644
--- a/net/bluetooth/rfcomm/sock.c
+++ b/net/bluetooth/rfcomm/sock.c
@@ -140,11 +140,13 @@ static struct sock *__rfcomm_get_sock_by_addr(u8 channel, bdaddr_t *src)
 /* Find socket with channel and source bdaddr.
  * Returns closest match.
  */
-static struct sock *__rfcomm_get_sock_by_channel(int state, u8 channel, bdaddr_t *src)
+static struct sock *rfcomm_get_sock_by_channel(int state, u8 channel, bdaddr_t *src)
 {
 	struct sock *sk = NULL, *sk1 = NULL;
 	struct hlist_node *node;
 
+	read_lock(&rfcomm_sk_list.lock);
+
 	sk_for_each(sk, node, &rfcomm_sk_list.head) {
 		if (state && sk->sk_state != state)
 			continue;
@@ -159,19 +161,10 @@ static struct sock *__rfcomm_get_sock_by_channel(int state, u8 channel, bdaddr_t
 				sk1 = sk;
 		}
 	}
-	return node ? sk : sk1;
-}
 
-/* Find socket with given address (channel, src).
- * Returns locked socket */
-static inline struct sock *rfcomm_get_sock_by_channel(int state, u8 channel, bdaddr_t *src)
-{
-	struct sock *s;
-	read_lock(&rfcomm_sk_list.lock);
-	s = __rfcomm_get_sock_by_channel(state, channel, src);
-	if (s) bh_lock_sock(s);
 	read_unlock(&rfcomm_sk_list.lock);
-	return s;
+
+	return node ? sk : sk1;
 }
 
 static void rfcomm_sock_destruct(struct sock *sk)
@@ -945,6 +938,8 @@ int rfcomm_connect_ind(struct rfcomm_session *s, u8 channel, struct rfcomm_dlc *
 	if (!parent)
 		return 0;
 
+	bh_lock_sock(parent);
+
 	/* Check for backlog size */
 	if (sk_acceptq_is_full(parent)) {
 		BT_DBG("backlog full %d", parent->sk_ack_backlog);
-- 
1.7.3.1


^ permalink raw reply related

* Re: Apple Wireless Keyboard connection issue
From: Andre Kuehne @ 2010-11-18  1:48 UTC (permalink / raw)
  To: Gustavo F. Padovan, linux-bluetooth
In-Reply-To: <20101117222751.GA20735@jh-x301>

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

On 11/17/2010 11:27 PM, Johan Hedberg wrote:
> Hi,
>
> On Thu, Nov 18, 2010, Andre Kuehne wrote:
>> On 11/17/2010 10:05 AM, Johan Hedberg wrote:
>>> On Tue, Nov 16, 2010, Gustavo F. Padovan wrote:
>>>> * "André Kühne"<andre.kuehne@gmx.net>   [2010-11-16 22:27:01 +0100]:
>>>>> I noticed the following on my system: After upgrading to bluez-4.79
>>>>> connecting my Apple Wireless Keyboard does not work anymore. With
>>>>> bluez-4.77 the connection works just fine.
>>>>
>>>> My Microsoft keyboard is not working too. That is probably due to commit
>>>> abe7cd44124a from Johan. It should be fixed soon.
>>>
>>> The only real difference that patch makes is the reuse of the HCI socket
>>> inside hciops.c. Maybe that screws up the event filters somehow or
>>> something similar. I don't have a keyboard to verify a fix, but could
>>> you try the attached patch and see if it helps?
>>>
>> I installed
>>
>>    http://www.kernel.org/pub/linux/bluetooth/bluez-4.79.tar.gz + hciops_encrypt.patch
>>
>> but the patch does not work for me. For verification I downloaded
>>
>>    http://www.kernel.org/pub/linux/bluetooth/bluez-4.77.tar.gz
>>
>> and compiled/installed it the same way (without the patch) and the connection works.
>>
>> I did the same using bluez-4.78 with the result that both my keyboard and mouse failed to connect.
>
> Ok, so then it seems like abe7cd44124a might not be to blame. Could you
> do a proper git bisect so we can figure out exactly at which point this
> broke. abe7cd44124a is really the only commit touching HID code since
> 4.77 so this is starting to get a bit strange.
>
> Johan
>
Please find attached my bisect results.

Best regards
Andre

[-- Attachment #2: bluez-bisect.txt --]
[-- Type: text/plain, Size: 1540 bytes --]

> git bisect good
a352058752e541539b09e55124d411a534cc14af is the first bad commit
commit a352058752e541539b09e55124d411a534cc14af
Author: Johan Hedberg <johan.hedberg@nokia.com>
Date:   Thu Nov 4 04:38:35 2010 +0200

    Cache adapter address for quick lookup

:040000 040000 b7b14eb9f69c0b74ebda87b2fa40a4574f08be44 b96739e2d58fcc5a5c6654431ba49a36ee623932 M	plugins

> git bisect log
git bisect start
# good: [b079cca768419d11e011cc5a7d59bc802226e633] Release 4.77
git bisect good b079cca768419d11e011cc5a7d59bc802226e633
# bad: [018c5b71748c178dcd2ffb0164c1d975788a2acc] Release 4.78
git bisect bad 018c5b71748c178dcd2ffb0164c1d975788a2acc
# good: [d5d0fa3be8f7a20d128dcbaf8fc529c38bc52395] Optimize device disconnect callback processing
git bisect good d5d0fa3be8f7a20d128dcbaf8fc529c38bc52395
# good: [b67a8a42e6ea9c2d2b0877ac3832de75ec4e00e5] Fix bdaddr naming consistency
git bisect good b67a8a42e6ea9c2d2b0877ac3832de75ec4e00e5
# bad: [5295b6a0ab0ad67a27926273a3fbf61f02663219] Fix not ignoring case of uuid given to RegisterEndpoint
git bisect bad 5295b6a0ab0ad67a27926273a3fbf61f02663219
# good: [db5266f0afedc33e2fd6fc3601c16498865f746d] Remove redundant tracking of ignored adapters
git bisect good db5266f0afedc33e2fd6fc3601c16498865f746d
# bad: [a352058752e541539b09e55124d411a534cc14af] Cache adapter address for quick lookup
git bisect bad a352058752e541539b09e55124d411a534cc14af
# good: [b289e54d53c332a51be483c1660a42c267991dd3] Remove redundant hci_devinfo call
git bisect good b289e54d53c332a51be483c1660a42c267991dd3

^ permalink raw reply

* Re: [PATCH v5] Bluetooth: btwilink driver
From: Pavan Savoy @ 2010-11-18  5:18 UTC (permalink / raw)
  To: Gustavo F. Padovan; +Cc: Vitaly Wool, marcel, linux-bluetooth, linux-kernel
In-Reply-To: <20101117165248.GB21729@vigoh>

On Wed, Nov 17, 2010 at 10:22 PM, Gustavo F. Padovan
<padovan@profusion.mobi> wrote:
> Hi Pavan,
>
> * Pavan Savoy <pavan_savoy@sify.com> [2010-11-17 11:13:26 +0530]:
>
>> On Wed, Nov 17, 2010 at 4:50 AM, Vitaly Wool <vitalywool@gmail.com> wrot=
e:
>> >>> + =C2=A0 =C2=A0 /* Registration with ST layer is successful,
>> >>> + =C2=A0 =C2=A0 =C2=A0* hardware is ready to accept commands from HC=
I core.
>> >>> + =C2=A0 =C2=A0 =C2=A0*/
>> >>> + =C2=A0 =C2=A0 if (test_and_set_bit(HCI_RUNNING, &hdev->flags)) {
>> >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 clear_bit(HCI_RUNNING, &=
hdev->flags);
>> >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 err =3D st_unregister(ST=
_BT);
>> >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (err)
>> >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=
=A0 BT_ERR("st_unregister() failed with error %d", err);
>> >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 hst->st_write =3D NULL;
>> >>> + =C2=A0 =C2=A0 }
>> >>
>> >>
>> >> What are you trying to do here? test_and_set_bit() result doesn't say
>> >> nothing about error and you shall put test_and_set_bit should be in t=
he
>> >> beginning, to know if your device is already opened or not and then
>> >> clear_bit if some error ocurrs during the function.
>> >>
>> >
>> > Yeap, this piece of code beats me is well. Why is it an error if this
>> > bit wasn't already set?
>>
>> Vitaly, Gustavo,
>>
>> I suppose I never understood HCI_RUNNING flag that way, as in an error
>> check mechanism to avoid multiple hci0 ups.
>>
>> What I understood was that HCI_RUNNING suggested as to when hci0 was
>> ready to be used. With this understanding, I wanted to make sure I
>> downloaded the firmware for the chip before I proclaim to the world
>> that the hci0 is ready to be used, as in HCI_RUNNING.
>>
>> For example, I didn't want my _send_frame to be called before I did
>> the firmware download - since firmware download takes time - 45kb
>> send/wait commands :(
>>
>> But I suppose I now understand - What I would rather do is test_bit in
>> the beginning of function and do a set_bit at the end of function -
>> does this make sense ?
>
> It does, but does it as test_and_set and then clear if error as we do in
> other drivers.

Ok, I understand, will do it this way.
However, still I am not too convinced - I honestly don't want to set
HCI_RUNNING before firmware download required for WiLink happens - and
this happens inside the st_register function here.

So the question again, How can I ensure _send_frame is not called when
firmware download is in progress - one of the major reasons why I
delayed the setting of HCI_RUNNING.

As mentioned before I will go ahead and create the patch - But would
still like to have an answer to this.


> Gustavo F. Padovan
> http://profusion.mobi
>

^ permalink raw reply

* Re: Apple Wireless Keyboard connection issue
From: Johan Hedberg @ 2010-11-18  8:44 UTC (permalink / raw)
  To: Andre Kuehne; +Cc: Gustavo F. Padovan, linux-bluetooth
In-Reply-To: <4CE48602.1000009@gmx.net>

Hi Andre,

On Thu, Nov 18, 2010, Andre Kuehne wrote:
> > git bisect good
> a352058752e541539b09e55124d411a534cc14af is the first bad commit
> commit a352058752e541539b09e55124d411a534cc14af
> Author: Johan Hedberg <johan.hedberg@nokia.com>
> Date:   Thu Nov 4 04:38:35 2010 +0200
> 
>     Cache adapter address for quick lookup

That's actually not related to the HID issue. There's a later commit
that fixed the general initialization issue that this commit caused.

I was now able to get hold of a Bluetooth keyboard and fixed the issue.
The problem was indeed in the commit that Gustavo originally pointed
out. It's just that my original patch for it was incomplete and so
didn't properly fix the issue. Anyway, a proper fix has now been pushed
to git. Please test with that.

Johan

^ permalink raw reply

* Re: [PATCHv3 3/7] Add DBus OOB API documentation.
From: Szymon Janc @ 2010-11-18 10:51 UTC (permalink / raw)
  To: Johan Hedberg; +Cc: linux-bluetooth@vger.kernel.org
In-Reply-To: <201011171349.51512.szymon.janc@tieto.com>

> > > +Service         org.bluez
> > > +Interface       org.bluez.OOB
> > > +Object path     /org/bluez
> > 
> > I think this should use the same path as the Manager API, which at the
> > moment is /
> 
> I was following HealthManager way, but I'm fine if you prefer / path.
> 
> > > +		array{byte} hash, array{byte} randomizer
> > > +			ReadLocalOobData(object adapter)
> > 
> > Having to pass an adapter path as a parameter seems weird. Wouldn't it
> > make more sense to have this method in the Adapter path/interface
> > instead? Also, we could toy around with the thought of putting the two
> > other methods into the current Manager interface.
> 
> It is OK to me to move ReadLocalOobData to adapter path as this is an adapter
> related data.
> 
> My initial idea was to keep all oob related methods in org.bluez.OOB interface
> but I'm not going to insist on it.

Please see the following proposal.

Register/UnregisterProvider methods can be easily moved to manager interface if
decided (with name changes to i.e. RegisterOobProvider for clarity).

Moving ReadLocalOobData to adapter path requires some code changes in dbusoob
plugin so I prefer to clarify that before coding.


Any comments?


BlueZ D-Bus OOB API description
*******************************

Copyright (C) 2010  ST-Ericsson SA

Author: Szymon Janc <szymon.janc@tieto.com> for ST-Ericsson

OOB hierarchy
=================

Service         unique name
Interface       org.bluez.OOB
Object path     freely definable

Methods		array{byte} hash, array{byte} randomizer
			RequestRemoteOobData(object device)

			This method gets called when the service daemon needs to
			get device's hash and randomizer for an OOB
			authentication. Each array should be 16 bytes long.

			Possible errors: org.bluez.Error.NoData

		void Release()

			This method gets called when D-Bus plug-in for OOB was
			deactivated. There is no need to unregister provider,
			because when this method gets called it has already been
			unregistered.

--------------------------------------------------------------------------------

Service         org.bluez
Interface       org.bluez.OOB
Object path     /

Methods		void RegisterProvider(object provider)

			This method registers provider for D-Bus OOB plug-in.
			When provider is successfully registered plug-in becomes
			active. Only one provider can be registered at time.

			Possible errors: org.bluez.Error.AlreadyExists

		void UnregisterProvider(object provider)

			This method unregisters provider for D-Bus OOB plug-in.

			Possible errors: org.bluez.Error.DoesNotExist

--------------------------------------------------------------------------------

Service		org.bluez
Interface	org.bluez.Adapter
Object path	[variable prefix]/{hci0,hci1,...}

		array{byte} hash, array{byte} randomizer ReadLocalOobData()

			This method reads local OOB data for adapter. Return
			value is pair of arrays 16 bytes each. Only registered
			provider should call this method. This method is
			available only for 2.1 (or higher) adapters.

			Note: This method will generate and return new local
			OOB data.

			Possible errors: org.bluez.Error.ReadFailed
					 org.bluez.Error.NoProvider
					 org.bluez.Error.InProgress

--------------------------------------------------------------------------------

Service		unique name
Interface	org.bluez.Agent
Object path	freely definable

Methods		void RequestPairingApproval(object device)

			This method gets called when the service daemon
			needs to confirm incoming OOB pairing request.

			Possible errors: org.bluez.Error.Rejected
					 org.bluez.Error.Canceled



-- 
Szymon Janc
on behalf of ST-Ericsson

^ permalink raw reply

* [PATCH v3 2/2] Split pull_contacts to smaller functions
From: Bartosz Szatkowski @ 2010-11-18 11:04 UTC (permalink / raw)
  To: linux-bluetooth; +Cc: Bartosz Szatkowski

Parts of pull_contact responsible for filling contact fields moved
to new functions.
---
 plugins/phonebook-tracker.c |  176 ++++++++++++++++++++++++++----------------
 1 files changed, 109 insertions(+), 67 deletions(-)

diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
index 71ed2f0..6d8915e 100644
--- a/plugins/phonebook-tracker.c
+++ b/plugins/phonebook-tracker.c
@@ -1395,6 +1395,110 @@ static void add_affiliation(char **field, const char *value)
 	*field = g_strdup(value);
 }
 
+static void contact_init(struct phonebook_contact *contact, char **reply)
+{
+
+	contact->fullname = g_strdup(reply[COL_FULL_NAME]);
+	contact->family = g_strdup(reply[COL_FAMILY_NAME]);
+	contact->given = g_strdup(reply[COL_GIVEN_NAME]);
+	contact->additional = g_strdup(reply[COL_ADDITIONAL_NAME]);
+	contact->prefix = g_strdup(reply[COL_NAME_PREFIX]);
+	contact->suffix = g_strdup(reply[COL_NAME_SUFFIX]);
+	contact->birthday = g_strdup(reply[COL_BIRTH_DATE]);
+	contact->nickname = g_strdup(reply[COL_NICKNAME]);
+	contact->website = g_strdup(reply[COL_URL]);
+	contact->photo = g_strdup(reply[COL_PHOTO]);
+	contact->company = g_strdup(reply[COL_ORG_NAME]);
+	contact->department = g_strdup(reply[COL_ORG_DEPARTMENT]);
+	contact->role = g_strdup(reply[COL_ORG_ROLE]);
+	contact->uid = g_strdup(reply[COL_UID]);
+	contact->title = g_strdup(reply[COL_TITLE]);
+
+	set_call_type(contact, reply[COL_DATE], reply[COL_SENT],
+							reply[COL_ANSWERED]);
+}
+
+static void contact_add_numbers(struct phonebook_contact *contact,
+								char **reply)
+{
+	add_phone_number(contact, reply[COL_HOME_NUMBER], TEL_TYPE_HOME);
+	add_phone_number(contact, reply[COL_WORK_NUMBER], TEL_TYPE_WORK);
+	add_phone_number(contact, reply[COL_FAX_NUMBER], TEL_TYPE_FAX);
+	add_phone_number(contact, reply[COL_CELL_NUMBER], TEL_TYPE_MOBILE);
+
+	if (g_strcmp0(reply[COL_OTHER_NUMBER], reply[COL_CELL_NUMBER]) == 0)
+		return;
+
+	if (g_strcmp0(reply[COL_OTHER_NUMBER], reply[COL_WORK_NUMBER]) == 0)
+		return;
+
+	if (g_strcmp0(reply[COL_OTHER_NUMBER], reply[COL_HOME_NUMBER]) == 0)
+		return;
+
+	add_phone_number(contact, reply[COL_OTHER_NUMBER], TEL_TYPE_OTHER);
+}
+
+static void contact_add_emails(struct phonebook_contact *contact,
+								char **reply)
+{
+	add_email(contact, reply[COL_HOME_EMAIL], EMAIL_TYPE_HOME);
+	add_email(contact, reply[COL_WORK_EMAIL], EMAIL_TYPE_WORK);
+	add_email(contact, reply[COL_OTHER_EMAIL], EMAIL_TYPE_OTHER);
+}
+
+static void contact_add_addresses(struct phonebook_contact *contact,
+								char **reply)
+{
+
+	char *home_addr, *work_addr, *other_addr;
+
+	home_addr = g_strdup_printf("%s;%s;%s;%s;%s;%s;%s",
+					reply[COL_HOME_ADDR_POBOX],
+					reply[COL_HOME_ADDR_EXT],
+					reply[COL_HOME_ADDR_STREET],
+					reply[COL_HOME_ADDR_LOCALITY],
+					reply[COL_HOME_ADDR_REGION],
+					reply[COL_HOME_ADDR_CODE],
+					reply[COL_HOME_ADDR_COUNTRY]);
+
+	work_addr = g_strdup_printf("%s;%s;%s;%s;%s;%s;%s",
+					reply[COL_WORK_ADDR_POBOX],
+					reply[COL_WORK_ADDR_EXT],
+					reply[COL_WORK_ADDR_STREET],
+					reply[COL_WORK_ADDR_LOCALITY],
+					reply[COL_WORK_ADDR_REGION],
+					reply[COL_WORK_ADDR_CODE],
+					reply[COL_WORK_ADDR_COUNTRY]);
+
+	other_addr = g_strdup_printf("%s;%s;%s;%s;%s;%s;%s",
+					reply[COL_OTHER_ADDR_POBOX],
+					reply[COL_OTHER_ADDR_EXT],
+					reply[COL_OTHER_ADDR_STREET],
+					reply[COL_OTHER_ADDR_LOCALITY],
+					reply[COL_OTHER_ADDR_REGION],
+					reply[COL_OTHER_ADDR_CODE],
+					reply[COL_OTHER_ADDR_COUNTRY]);
+
+	add_address(contact, home_addr, ADDR_TYPE_HOME);
+	add_address(contact, work_addr, ADDR_TYPE_WORK);
+	add_address(contact, other_addr, ADDR_TYPE_OTHER);
+
+	g_free(home_addr);
+	g_free(work_addr);
+	g_free(other_addr);
+}
+
+static void contact_add_organization(struct phonebook_contact *contact,
+								char **reply)
+{
+	/* Adding fields connected by nco:hasAffiliation - they may be in
+	 * separate replies */
+	add_affiliation(&contact->title, reply[COL_TITLE]);
+	add_affiliation(&contact->company, reply[COL_ORG_NAME]);
+	add_affiliation(&contact->department, reply[COL_ORG_DEPARTMENT]);
+	add_affiliation(&contact->role, reply[COL_ORG_ROLE]);
+}
+
 static void pull_contacts(char **reply, int num_fields, void *user_data)
 {
 	struct phonebook_data *data = user_data;
@@ -1404,7 +1508,6 @@ static void pull_contacts(char **reply, int num_fields, void *user_data)
 	GString *vcards;
 	int last_index, i;
 	gboolean cdata_present = FALSE;
-	char *home_addr, *work_addr, *other_addr;
 	static char *temp_id = NULL;
 
 	if (num_fields < 0) {
@@ -1456,74 +1559,13 @@ static void pull_contacts(char **reply, int num_fields, void *user_data)
 
 add_entry:
 	contact = g_new0(struct phonebook_contact, 1);
-	contact->fullname = g_strdup(reply[COL_FULL_NAME]);
-	contact->family = g_strdup(reply[COL_FAMILY_NAME]);
-	contact->given = g_strdup(reply[COL_GIVEN_NAME]);
-	contact->additional = g_strdup(reply[COL_ADDITIONAL_NAME]);
-	contact->prefix = g_strdup(reply[COL_NAME_PREFIX]);
-	contact->suffix = g_strdup(reply[COL_NAME_SUFFIX]);
-	contact->birthday = g_strdup(reply[COL_BIRTH_DATE]);
-	contact->nickname = g_strdup(reply[COL_NICKNAME]);
-	contact->website = g_strdup(reply[COL_URL]);
-	contact->photo = g_strdup(reply[COL_PHOTO]);
-	contact->company = g_strdup(reply[COL_ORG_NAME]);
-	contact->department = g_strdup(reply[COL_ORG_DEPARTMENT]);
-	contact->role = g_strdup(reply[COL_ORG_ROLE]);
-	contact->uid = g_strdup(reply[COL_UID]);
-	contact->title = g_strdup(reply[COL_TITLE]);
-
-	set_call_type(contact, reply[COL_DATE], reply[COL_SENT],
-							reply[COL_ANSWERED]);
+	contact_init(contact, reply);
 
 add_numbers:
-	/* Adding phone numbers to contact struct */
-	add_phone_number(contact, reply[COL_HOME_NUMBER], TEL_TYPE_HOME);
-	add_phone_number(contact, reply[COL_WORK_NUMBER], TEL_TYPE_WORK);
-	add_phone_number(contact, reply[COL_FAX_NUMBER], TEL_TYPE_FAX);
-	add_phone_number(contact, reply[COL_CELL_NUMBER], TEL_TYPE_MOBILE);
-	if ((g_strcmp0(reply[COL_OTHER_NUMBER], reply[COL_CELL_NUMBER]) != 0) &&
-			(g_strcmp0(reply[COL_OTHER_NUMBER], reply[COL_WORK_NUMBER]) != 0) &&
-			(g_strcmp0(reply[COL_OTHER_NUMBER], reply[COL_HOME_NUMBER]) != 0))
-		add_phone_number(contact, reply[COL_OTHER_NUMBER], TEL_TYPE_OTHER);
-
-	/* Adding emails */
-	add_email(contact, reply[COL_HOME_EMAIL], EMAIL_TYPE_HOME);
-	add_email(contact, reply[COL_WORK_EMAIL], EMAIL_TYPE_WORK);
-	add_email(contact, reply[COL_OTHER_EMAIL], EMAIL_TYPE_OTHER);
-
-	/* Adding addresses */
-	home_addr = g_strdup_printf("%s;%s;%s;%s;%s;%s;%s",
-				reply[COL_HOME_ADDR_POBOX], reply[COL_HOME_ADDR_EXT],
-				reply[COL_HOME_ADDR_STREET], reply[COL_HOME_ADDR_LOCALITY],
-				reply[COL_HOME_ADDR_REGION], reply[COL_HOME_ADDR_CODE],
-				reply[COL_HOME_ADDR_COUNTRY]);
-
-	work_addr = g_strdup_printf("%s;%s;%s;%s;%s;%s;%s",
-				reply[COL_WORK_ADDR_POBOX], reply[COL_WORK_ADDR_EXT],
-				reply[COL_WORK_ADDR_STREET], reply[COL_WORK_ADDR_LOCALITY],
-				reply[COL_WORK_ADDR_REGION], reply[COL_WORK_ADDR_CODE],
-				reply[COL_WORK_ADDR_COUNTRY]);
-
-	other_addr = g_strdup_printf("%s;%s;%s;%s;%s;%s;%s",
-				reply[COL_OTHER_ADDR_POBOX], reply[COL_OTHER_ADDR_EXT],
-				reply[COL_OTHER_ADDR_STREET], reply[COL_OTHER_ADDR_LOCALITY],
-				reply[COL_OTHER_ADDR_REGION], reply[COL_OTHER_ADDR_CODE],
-				reply[COL_OTHER_ADDR_COUNTRY]);
-
-	add_address(contact, home_addr, ADDR_TYPE_HOME);
-	add_address(contact, work_addr, ADDR_TYPE_WORK);
-	add_address(contact, other_addr, ADDR_TYPE_OTHER);
-
-	g_free(home_addr);
-	g_free(work_addr);
-	g_free(other_addr);
-
-	/* Adding fields connected by nco:hasAffiliation - they may be in
-	 * separate replies */
-	add_affiliation(&contact->title, reply[COL_TITLE]);
-	add_affiliation(&contact->company, reply[COL_ORG_NAME]);
-	add_affiliation(&contact->department, reply[COL_ORG_DEPARTMENT]);
-	add_affiliation(&contact->role, reply[COL_ORG_ROLE]);
+	contact_add_numbers(contact, reply);
+	contact_add_emails(contact, reply);
+	contact_add_addresses(contact, reply);
+	contact_add_organization(contact, reply);
 
 	DBG("contact %p", contact);
 
-- 
1.7.0.4


^ permalink raw reply related

* Re: [PATCH] Adding a new option to specify security level for gatttool
From: Johan Hedberg @ 2010-11-18 12:19 UTC (permalink / raw)
  To: tim.howes; +Cc: Mike.Tsai, linux-bluetooth
In-Reply-To: <1AFE20D16950C745A2A83986B72E8748011F571E7497@EMEXM3131.dir.svc.accenture.com>

Hi Tim,

Nice to see you on this list! :)

On Wed, Nov 17, 2010, tim.howes@accenture.com wrote:
> > [Mtsai] I am not sure what are the definition of "low", "medium" or
> > "high". By the spec of Core 4.0, LE has 2 security modes and different
> > security levels based on the method of pairing (or bonding). It may be
> > appeal to end user with "low", "medium" and "high" definition, but it
> > can't be reference with LE spec. I would suggest, instead, following
> > terms,
> > 
> > 	"No security",
> > 	"unauthenticated encryption",
> > 	"authenticated encryption",
> > 	"unauthenticated data signing",
> > 	"authenticated data signing,
> 
> To some extent I agree; however, the semantics of such an API would
> have to be careful.  A particular profile should not "force" data
> signing because if the link is already encrypted there is little point
> using data signing.  So from that point of view exposing a more
> abstract API (a bit like "high") is better.  However, it is hard to
> map "high" onto any of the ones you listed (which I agree is a good
> list).  So perhaps it is better to have the API semantics as
> "advisory" or "requests" which can be fulfilled by the underlying
> stack in other ways (eg encryption for data-signing).

Something like that will probably be needed, yes. However the idea of
the current command line switch to gatttool is to simply map to the
existing kernel API, and that API only has low, medium and high. So at
least in the short term the patch is fine.

Johan

^ permalink raw reply

* Re: [PATCH v3] Adding a new option to specify security level for gatttool
From: Johan Hedberg @ 2010-11-18 12:21 UTC (permalink / raw)
  To: Sheldon Demario; +Cc: linux-bluetooth
In-Reply-To: <1290000400-20001-1-git-send-email-sheldon.demario@openbossa.org>

Hi Sheldon,

On Wed, Nov 17, 2010, Sheldon Demario wrote:
> ---
>  TODO              |    6 ------
>  attrib/gatttool.c |   15 +++++++++++++--
>  2 files changed, 13 insertions(+), 8 deletions(-)

The patch has been pushed upstream. Thanks.

Johan

^ permalink raw reply

* Re: [PATCH v3 2/2] Split pull_contacts to smaller functions
From: Johan Hedberg @ 2010-11-18 12:22 UTC (permalink / raw)
  To: Bartosz Szatkowski; +Cc: linux-bluetooth
In-Reply-To: <1290078266-16279-1-git-send-email-bulislaw@linux.com>

Hi Bartosz,

On Thu, Nov 18, 2010, Bartosz Szatkowski wrote:
> Parts of pull_contact responsible for filling contact fields moved
> to new functions.
> ---
>  plugins/phonebook-tracker.c |  176 ++++++++++++++++++++++++++----------------
>  1 files changed, 109 insertions(+), 67 deletions(-)

Thanks. The patch looks now fine and has been pushed upstream.

Johan

^ permalink raw reply

* Re: [PATCH 1/3] Advertising data: extract local name
From: Johan Hedberg @ 2010-11-18 12:33 UTC (permalink / raw)
  To: Bruna Moreira; +Cc: linux-bluetooth
In-Reply-To: <1290009593-13658-1-git-send-email-bruna.moreira@openbossa.org>

Hi Bruna,

On Wed, Nov 17, 2010, Bruna Moreira wrote:
> @@ -3007,6 +3007,7 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
>  	bdaddr_t bdaddr;
>  	gboolean new_dev;
>  	int8_t rssi;
> +	uint8_t type;
>  
>  	rssi = *(info->data + info->length);
>  	bdaddr = info->bdaddr;
> @@ -3023,6 +3024,14 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
>  
>  	adapter->found_devices = g_slist_sort(adapter->found_devices,
>  						(GCompareFunc) dev_rssi_cmp);
> +
> +	if (info->length) {
> +		char *tmp_name = bt_extract_eir_name(info->data, &type);
> +		if (tmp_name) {
> +			g_free(dev->name);
> +			dev->name = tmp_name;
> +		}
> +	}

Variables should be always declared in the smallest possible scope, so
your new type variable is in the wrong place (it should be declared
inside the if-statement. Since this was the only issue I found with this
patch I fixed it myself and pushed it upstream. Btw, is it really safe
to ignore the type here? What if it's EIR_NAME_SHORT? Wouldn't you then
want to perform full name discovery using e.g. the GAP GATT service
later?

Johan

^ permalink raw reply

* Re: [PATCH 2/3] Extract service UUIDs from advertising data
From: Johan Hedberg @ 2010-11-18 12:43 UTC (permalink / raw)
  To: Bruna Moreira; +Cc: linux-bluetooth
In-Reply-To: <1290009593-13658-2-git-send-email-bruna.moreira@openbossa.org>

Hi Bruna,

On Wed, Nov 17, 2010, Bruna Moreira wrote:
> +	/* Extract UUIDs from extended inquiry response if any */
> +	dev->services = get_eir_uuids(eir_data, eir_length, dev->services);
> +	uuid_count = g_slist_length(dev->services);
> +
> +	if (dev->services) {
> +		uuids = strlist2array(dev->services);
> +		g_slist_foreach(dev->services, (GFunc) g_free, NULL);
> +		g_slist_free(dev->services);
> +		dev->services = NULL;
> +	}

What's the reason that the get_eir_uuids API is designed so that it can
handle a list which already contains elements before calling the
function? Since you free the list right after creating the uuids array
it seems like this shouldn't happen. Or am I missing something?

Btw, is there something that could be optimized here since it seems like
you're regenerating the uuids array before every signal even though the
set of service might not have changed since the previous one. Maybe you
should track this and only regenerate the array if there has been a
changed from the previous signal. What do you think?

Johan

^ permalink raw reply

* Re: [PATCH v3] Add iwmmxt optimization for sbc for pxa series cpu
From: Siarhei Siamashka @ 2010-11-18 13:05 UTC (permalink / raw)
  To: Keith Mok; +Cc: linux-bluetooth
In-Reply-To: <201011151308.31972.siarhei.siamashka@gmail.com>

On Monday 15 November 2010 13:08:19 Siarhei Siamashka wrote:
> On Monday 15 November 2010 04:46:25 Keith Mok wrote:
> > > I sometimes use different indentation levels in such cases in order to
> > > improve readability after instructions reordering, so that each
> > > logically independent block of code has its own indentation level and
> > > it is still easily visible
> > 
> > > after instructions reordering. For example, with the original code:
> > Thanks for the hints. I rearranged the code.
> 
> Thanks, now the assembly code looks ok to me. I also discovered that qemu
> supports iwmmxt1 emulation just fine and also tried to test your
> optimizations for correctness myself (with a script which tries different
> encoding paramaters for different audio samples and checks md5 checksums),
> no problems detected.
> 
> So if somebody else could check whether the other things are right
> (copyright notices for example), then we are done with it.

As nobody else has stepped in, I guess it's still my responsibility to provide
some further guidance even though I'm a very infrequent contributor myself.
Hopefully somebody will correct me if I'm wrong.

So please

1. Make a final patch in such a form that can be pushed to git repository 
without any modifications, it means that you need a clean commit message and 
not just some text intermixed with the parts and quotations of discussion from
this mailing list.
2. "Signed-off-by" header is not needed for the userspace parts of bluez.
3. All files must have copyright notices, even a small one like 
'sbc_primitives_iwmmxt.h'. And probably you should just replicate all the
copyright notices from the source files with sbc mmx optimizations and add your
own copyright on top.

Hopefully that should be enough to get your optimizations applied. Thanks.

-- 
Best regards,
Siarhei Siamashka

^ permalink raw reply


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