* [PATCH v2 5/7] Advertising data: extract local name
From: Vinicius Costa Gomes @ 2010-11-11 18:51 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Bruna Moreira
In-Reply-To: <1289501521-21825-1-git-send-email-vinicius.gomes@openbossa.org>
From: Bruna Moreira <bruna.moreira@openbossa.org>
Move extract_eir_name() to glib-helper.c file and rename function to
bt_extract_eir_name(). The local name is extracted from the advertising
data.
---
src/adapter.c | 7 +++++++
src/event.c | 23 +----------------------
src/glib-helper.c | 22 ++++++++++++++++++++++
src/glib-helper.h | 1 +
4 files changed, 31 insertions(+), 22 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 9c92e22..7488322 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3140,6 +3140,7 @@ void adapter_update_adv(struct btd_adapter *adapter, le_advertising_info *info)
bdaddr_t bdaddr;
gboolean new_dev;
int8_t rssi = 0;
+ uint8_t type = 0x00;
rssi = *(info->data + info->length);
bdaddr = info->bdaddr;
@@ -3156,6 +3157,12 @@ void adapter_update_adv(struct btd_adapter *adapter, le_advertising_info *info)
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)
+ dev->name = tmp_name;
+ }
}
void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
diff --git a/src/event.c b/src/event.c
index 8b03bc3..57bdf60 100644
--- a/src/event.c
+++ b/src/event.c
@@ -301,27 +301,6 @@ void btd_event_simple_pairing_complete(bdaddr_t *local, bdaddr_t *peer,
device_simple_pairing_complete(device, status);
}
-static char *extract_eir_name(uint8_t *data, uint8_t *type)
-{
- if (!data || !type)
- return NULL;
-
- if (data[0] == 0)
- return NULL;
-
- *type = data[1];
-
- switch (*type) {
- case 0x08:
- case 0x09:
- if (!g_utf8_validate((char *) (data + 2), data[0] - 1, NULL))
- return strdup("");
- return strndup((char *) (data + 2), data[0] - 1);
- }
-
- return NULL;
-}
-
void btd_event_adv(bdaddr_t *local, le_advertising_info *info)
{
struct btd_adapter *adapter;
@@ -410,7 +389,7 @@ void btd_event_inquiry_result(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
} else
legacy = TRUE;
- tmp_name = extract_eir_name(data, &name_type);
+ tmp_name = bt_extract_eir_name(data, &name_type);
if (tmp_name) {
if (name_type == 0x09) {
write_device_name(local, peer, tmp_name);
diff --git a/src/glib-helper.c b/src/glib-helper.c
index 9d76626..33668d7 100644
--- a/src/glib-helper.c
+++ b/src/glib-helper.c
@@ -43,6 +43,7 @@
#include <glib.h>
#include "glib-helper.h"
+#include "sdpd.h"
/* Number of seconds to keep a sdp_session_t in the cache */
#define CACHE_TIMEOUT 2
@@ -576,3 +577,24 @@ GSList *bt_string2list(const gchar *str)
return l;
}
+
+char *bt_extract_eir_name(uint8_t *data, uint8_t *type)
+{
+ if (!data || !type)
+ return NULL;
+
+ if (data[0] == 0)
+ return NULL;
+
+ *type = data[1];
+
+ switch (*type) {
+ case EIR_NAME_SHORT:
+ case EIR_NAME_COMPLETE:
+ if (!g_utf8_validate((char *) (data + 2), data[0] - 1, NULL))
+ return strdup("");
+ return strndup((char *) (data + 2), data[0] - 1);
+ }
+
+ return NULL;
+}
diff --git a/src/glib-helper.h b/src/glib-helper.h
index e89c2c6..dfe4123 100644
--- a/src/glib-helper.h
+++ b/src/glib-helper.h
@@ -38,3 +38,4 @@ char *bt_name2string(const char *string);
int bt_string2uuid(uuid_t *uuid, const char *string);
gchar *bt_list2string(GSList *list);
GSList *bt_string2list(const gchar *str);
+char *bt_extract_eir_name(uint8_t *data, uint8_t *type);
--
1.7.3.2
^ permalink raw reply related
* [PATCH v2 4/7] Initial advertising data parsing implementation
From: Vinicius Costa Gomes @ 2010-11-11 18:51 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Bruna Moreira
In-Reply-To: <1289501521-21825-1-git-send-email-vinicius.gomes@openbossa.org>
From: Bruna Moreira <bruna.moreira@openbossa.org>
Implement adapter_update_adv() function to parse advertising data
received by btd_event_adv() function. Add some fields for advertising
data in remote_device_info struct.
---
plugins/hciops.c | 9 +++------
src/adapter.c | 25 +++++++++++++++++++++++++
src/adapter.h | 5 +++++
src/event.c | 13 +++++++++++++
src/event.h | 1 +
5 files changed, 47 insertions(+), 6 deletions(-)
diff --git a/plugins/hciops.c b/plugins/hciops.c
index fc99275..dc7a657 100644
--- a/plugins/hciops.c
+++ b/plugins/hciops.c
@@ -1011,7 +1011,7 @@ static inline void le_metaevent(int index, void *ptr)
{
evt_le_meta_event *meta = ptr;
le_advertising_info *info;
- uint8_t *rssi, num, i;
+ uint8_t num, i;
DBG("LE Meta Event");
@@ -1022,11 +1022,8 @@ static inline void le_metaevent(int index, void *ptr)
info = (le_advertising_info *) (meta->data + 1);
for (i = 0; i < num; i++) {
- /* RSSI is last byte of the advertising report event */
- rssi = info->data + info->length;
- btd_event_inquiry_result(&BDADDR(index), &info->bdaddr, 0,
- *rssi, NULL);
- info = (le_advertising_info *) (rssi + 1);
+ btd_event_adv(&BDADDR(index), info);
+ info = (le_advertising_info *) (info->data + info->length + 1);
}
}
diff --git a/src/adapter.c b/src/adapter.c
index 6f4f2a3..9c92e22 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3134,6 +3134,30 @@ static struct remote_dev_info *get_found_dev(struct btd_adapter *adapter,
return dev;
}
+void adapter_update_adv(struct btd_adapter *adapter, le_advertising_info *info)
+{
+ struct remote_dev_info *dev;
+ bdaddr_t bdaddr;
+ gboolean new_dev;
+ int8_t rssi = 0;
+
+ rssi = *(info->data + info->length);
+ bdaddr = info->bdaddr;
+
+ dev = get_found_dev(adapter, &bdaddr, &new_dev);
+
+ if (new_dev) {
+ dev->le = TRUE;
+ dev->evt_type = info->evt_type;
+ } else if (dev->rssi == rssi)
+ return;
+
+ dev->rssi = rssi;
+
+ adapter->found_devices = g_slist_sort(adapter->found_devices,
+ (GCompareFunc) dev_rssi_cmp);
+}
+
void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
int8_t rssi, uint32_t class, const char *name,
const char *alias, gboolean legacy,
@@ -3151,6 +3175,7 @@ void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
if (alias)
dev->alias = g_strdup(alias);
+ dev->le = FALSE;
dev->class = class;
dev->legacy = legacy;
dev->name_status = name_status;
diff --git a/src/adapter.h b/src/adapter.h
index 89b07d7..766b079 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -69,6 +69,10 @@ struct remote_dev_info {
char *alias;
dbus_bool_t legacy;
name_status_t name_status;
+ gboolean le;
+ /* LE adv data */
+ uint8_t evt_type;
+ uint8_t bdaddr_type;
};
struct hci_dev {
@@ -118,6 +122,7 @@ int adapter_get_discover_type(struct btd_adapter *adapter);
gboolean adapter_is_ready(struct btd_adapter *adapter);
struct remote_dev_info *adapter_search_found_devices(struct btd_adapter *adapter,
struct remote_dev_info *match);
+void adapter_update_adv(struct btd_adapter *adapter, le_advertising_info *info);
void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
int8_t rssi, uint32_t class, const char *name,
const char *alias, gboolean legacy,
diff --git a/src/event.c b/src/event.c
index a057306..8b03bc3 100644
--- a/src/event.c
+++ b/src/event.c
@@ -322,6 +322,19 @@ static char *extract_eir_name(uint8_t *data, uint8_t *type)
return NULL;
}
+void btd_event_adv(bdaddr_t *local, le_advertising_info *info)
+{
+ struct btd_adapter *adapter;
+
+ adapter = manager_find_adapter(local);
+ if (adapter == NULL) {
+ error("No matching adapter found");
+ return;
+ }
+
+ adapter_update_adv(adapter, info);
+}
+
void btd_event_inquiry_result(bdaddr_t *local, bdaddr_t *peer, uint32_t class,
int8_t rssi, uint8_t *data)
{
diff --git a/src/event.h b/src/event.h
index 4a7b9c9..44e1462 100644
--- a/src/event.h
+++ b/src/event.h
@@ -23,6 +23,7 @@
*/
int btd_event_request_pin(bdaddr_t *sba, struct hci_conn_info *ci);
+void btd_event_adv(bdaddr_t *local, le_advertising_info *info);
void btd_event_inquiry_result(bdaddr_t *local, bdaddr_t *peer, uint32_t class, int8_t rssi, uint8_t *data);
void btd_event_set_legacy_pairing(bdaddr_t *local, bdaddr_t *peer, gboolean legacy);
void btd_event_remote_class(bdaddr_t *local, bdaddr_t *peer, uint32_t class);
--
1.7.3.2
^ permalink raw reply related
* [PATCH v2 3/7] Refactoring adapter_update_found_devices() function
From: Vinicius Costa Gomes @ 2010-11-11 18:51 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Bruna Moreira
In-Reply-To: <1289501521-21825-1-git-send-email-vinicius.gomes@openbossa.org>
From: Bruna Moreira <bruna.moreira@openbossa.org>
The common code from adapter_update_found_devices() was moved to
update_found_devices().
---
src/adapter.c | 50 +++++++++++++++++++++++++++++++-------------------
1 files changed, 31 insertions(+), 19 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 363ee94..6f4f2a3 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3108,10 +3108,8 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
g_strfreev(uuids);
}
-void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
- int8_t rssi, uint32_t class, const char *name,
- const char *alias, gboolean legacy,
- name_status_t name_status, uint8_t *eir_data)
+static struct remote_dev_info *get_found_dev(struct btd_adapter *adapter,
+ const bdaddr_t *bdaddr, gboolean *new_dev)
{
struct remote_dev_info *dev, match;
@@ -3121,30 +3119,44 @@ void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
dev = adapter_search_found_devices(adapter, &match);
if (dev) {
+ *new_dev = FALSE;
/* Out of range list update */
adapter->oor_devices = g_slist_remove(adapter->oor_devices,
dev);
+ } else {
+ *new_dev = TRUE;
+ dev = g_new0(struct remote_dev_info, 1);
+ bacpy(&dev->bdaddr, bdaddr);
+ adapter->found_devices = g_slist_prepend(adapter->found_devices,
+ dev);
+ }
- if (rssi == dev->rssi)
- return;
+ return dev;
+}
- goto done;
- }
+void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
+ int8_t rssi, uint32_t class, const char *name,
+ const char *alias, gboolean legacy,
+ name_status_t name_status, uint8_t *eir_data)
+{
+ struct remote_dev_info *dev;
+ gboolean new_dev;
- dev = g_new0(struct remote_dev_info, 1);
+ dev = get_found_dev(adapter, bdaddr, &new_dev);
- bacpy(&dev->bdaddr, bdaddr);
- dev->class = class;
- if (name)
- dev->name = g_strdup(name);
- if (alias)
- dev->alias = g_strdup(alias);
- dev->legacy = legacy;
- dev->name_status = name_status;
+ if (new_dev) {
+ if (name)
+ dev->name = g_strdup(name);
- adapter->found_devices = g_slist_prepend(adapter->found_devices, dev);
+ if (alias)
+ dev->alias = g_strdup(alias);
+
+ dev->class = class;
+ dev->legacy = legacy;
+ dev->name_status = name_status;
+ } else if (dev->rssi == rssi)
+ return;
-done:
dev->rssi = rssi;
adapter->found_devices = g_slist_sort(adapter->found_devices,
--
1.7.3.2
^ permalink raw reply related
* [PATCH v2 2/7] Refactor get_eir_uuids() to get EIR data length parameter
From: Vinicius Costa Gomes @ 2010-11-11 18:51 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Anderson Lizardo
In-Reply-To: <1289501521-21825-1-git-send-email-vinicius.gomes@openbossa.org>
From: Anderson Lizardo <anderson.lizardo@openbossa.org>
get_eir_uuids() will be reused to parse LE advertising data as well, as
they share the same format. But for Advertising, maximum data length is
different (31 bytes vs. 240 bytes for EIR), and the radio is not
required to send the non-significant (zero-filled) bytes.
adapter_emit_device_found() now also accepts a EIR data length
parameter, so it can be reused for LE and can propagate the exact data
length.
---
src/adapter.c | 17 ++++++++++-------
src/adapter.h | 2 +-
src/event.c | 2 +-
3 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 8b742b7..363ee94 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2962,7 +2962,8 @@ static void emit_device_found(const char *path, const char *address,
g_dbus_send_message(connection, signal);
}
-static char **get_eir_uuids(uint8_t *eir_data, size_t *uuid_count)
+static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
+ size_t *uuid_count)
{
uint16_t len = 0;
char **uuids;
@@ -2976,7 +2977,10 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t *uuid_count)
uuid_t service;
unsigned int i;
- while (len < EIR_DATA_LENGTH - 1) {
+ if (eir_data == NULL || eir_length == 0)
+ return NULL;
+
+ while (len < eir_length - 1) {
uint8_t field_len = eir_data[0];
/* Check for the end of EIR */
@@ -3006,7 +3010,7 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t *uuid_count)
}
/* Bail out if got incorrect length */
- if (len > EIR_DATA_LENGTH)
+ if (len > eir_length)
return NULL;
total = uuid16_count + uuid32_count + uuid128_count;
@@ -3056,7 +3060,7 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t *uuid_count)
}
void adapter_emit_device_found(struct btd_adapter *adapter,
- struct remote_dev_info *dev, uint8_t *eir_data)
+ struct remote_dev_info *dev, uint8_t *eir_data, size_t eir_length)
{
struct btd_device *device;
char peer_addr[18], local_addr[18];
@@ -3086,8 +3090,7 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
alias = g_strdup(dev->alias);
/* Extract UUIDs from extended inquiry response if any*/
- if (eir_data != NULL)
- uuids = get_eir_uuids(eir_data, &uuid_count);
+ uuids = get_eir_uuids(eir_data, eir_length, &uuid_count);
emit_device_found(adapter->path, paddr,
"Address", DBUS_TYPE_STRING, &paddr,
@@ -3147,7 +3150,7 @@ done:
adapter->found_devices = g_slist_sort(adapter->found_devices,
(GCompareFunc) dev_rssi_cmp);
- adapter_emit_device_found(adapter, dev, eir_data);
+ adapter_emit_device_found(adapter, dev, eir_data, EIR_DATA_LENGTH);
}
int adapter_remove_found_device(struct btd_adapter *adapter, bdaddr_t *bdaddr)
diff --git a/src/adapter.h b/src/adapter.h
index 8019cfc..89b07d7 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -124,7 +124,7 @@ void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
name_status_t name_status, uint8_t *eir_data);
int adapter_remove_found_device(struct btd_adapter *adapter, bdaddr_t *bdaddr);
void adapter_emit_device_found(struct btd_adapter *adapter,
- struct remote_dev_info *dev, uint8_t *eir_data);
+ struct remote_dev_info *dev, uint8_t *eir_data, size_t eir_length);
void adapter_mode_changed(struct btd_adapter *adapter, uint8_t scan_mode);
void adapter_setname_complete(bdaddr_t *local, uint8_t status);
void adapter_update_tx_power(bdaddr_t *bdaddr, uint8_t status, void *ptr);
diff --git a/src/event.c b/src/event.c
index 971bb35..a057306 100644
--- a/src/event.c
+++ b/src/event.c
@@ -510,7 +510,7 @@ void btd_event_remote_name(bdaddr_t *local, bdaddr_t *peer, uint8_t status,
if (dev_info) {
g_free(dev_info->name);
dev_info->name = g_strdup(name);
- adapter_emit_device_found(adapter, dev_info, NULL);
+ adapter_emit_device_found(adapter, dev_info, NULL, 0);
}
if (device)
--
1.7.3.2
^ permalink raw reply related
* [PATCH v2 1/7] Fix invalid memory access when EIR field length is zero
From: Vinicius Costa Gomes @ 2010-11-11 18:51 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Bruna Moreira
From: Bruna Moreira <bruna.moreira@openbossa.org>
---
src/adapter.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index b1aabbd..8b742b7 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -2977,14 +2977,13 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t *uuid_count)
unsigned int i;
while (len < EIR_DATA_LENGTH - 1) {
- uint8_t type = eir_data[1];
uint8_t field_len = eir_data[0];
/* Check for the end of EIR */
if (field_len == 0)
break;
- switch (type) {
+ switch (eir_data[1]) {
case EIR_UUID16_SOME:
case EIR_UUID16_ALL:
uuid16_count = field_len / 2;
--
1.7.3.2
^ permalink raw reply related
* [PATCH v3] Split up object and session in pbap.c
From: Dmitriy Paliy @ 2010-11-11 15:51 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Dmitriy Paliy
In-Reply-To: <1289460972-2971-1-git-send-email-dmitriy.paliy@nokia.com>
Object and session data is separated in PBAP plugin. Reason is that
when OBEX session firstly makes disconnect of service_data, which
corresponds to session in pbap.c, it than closes object, which also
corresponds to session in pbap.c.
Memory is deallocated after PBAP session is disconnected. When OBEX
driver closes the object, it is trying to dereference the deallocated
memory in order to free pbap->buffer data.
Here object and session are separated, while pointers are created to
make one-to-one mapping. pbap_object is created in vobject_..._open
functions after query to tracker submitted. Session and object are
handled separately when freed.
---
plugins/pbap.c | 94 +++++++++++++++++++++++++++++++++++---------------------
1 files changed, 59 insertions(+), 35 deletions(-)
diff --git a/plugins/pbap.c b/plugins/pbap.c
index c64377c..1a7d001 100644
--- a/plugins/pbap.c
+++ b/plugins/pbap.c
@@ -140,8 +140,13 @@ struct pbap_session {
struct apparam_field *params;
char *folder;
uint32_t find_handle;
- GString *buffer;
struct cache cache;
+ struct pbap_object *obj;
+};
+
+struct pbap_object {
+ GString *buffer;
+ struct pbap_session *session;
};
static const uint8_t PBAP_TARGET[TARGET_SIZE] = {
@@ -237,9 +242,9 @@ static void phonebook_size_result(const char *buffer, size_t bufsize,
hdr->len = PHONEBOOKSIZE_LEN;
memcpy(hdr->val, &phonebooksize, sizeof(phonebooksize));
- pbap->buffer = g_string_new_len(aparam, sizeof(aparam));
+ pbap->obj->buffer = g_string_new_len(aparam, sizeof(aparam));
- obex_object_set_io_flags(pbap, G_IO_IN, 0);
+ obex_object_set_io_flags(pbap->obj, G_IO_IN, 0);
}
static void query_result(const char *buffer, size_t bufsize, int vcards,
@@ -250,17 +255,17 @@ static void query_result(const char *buffer, size_t bufsize, int vcards,
DBG("");
if (vcards <= 0) {
- obex_object_set_io_flags(pbap, G_IO_ERR, -ENOENT);
+ obex_object_set_io_flags(pbap->obj, G_IO_ERR, -ENOENT);
return;
}
- if (!pbap->buffer)
- pbap->buffer = g_string_new_len(buffer, bufsize);
+ if (!pbap->obj->buffer)
+ pbap->obj->buffer = g_string_new_len(buffer, bufsize);
else
- pbap->buffer = g_string_append_len(pbap->buffer, buffer,
+ pbap->obj->buffer = g_string_append_len(pbap->obj->buffer, buffer,
bufsize);
- obex_object_set_io_flags(pbap, G_IO_IN, 0);
+ obex_object_set_io_flags(pbap->obj, G_IO_IN, 0);
}
static void cache_entry_notify(const char *id, uint32_t handle,
@@ -392,7 +397,7 @@ static void cache_ready_notify(void *user_data)
hdr->len = PHONEBOOKSIZE_LEN;
memcpy(hdr->val, &size, sizeof(size));
- pbap->buffer = g_string_new_len(aparam, sizeof(aparam));
+ pbap->obj->buffer = g_string_new_len(aparam, sizeof(aparam));
goto done;
}
@@ -406,29 +411,29 @@ static void cache_ready_notify(void *user_data)
if (sorted == NULL) {
pbap->cache.valid = TRUE;
- obex_object_set_io_flags(pbap, G_IO_ERR, -ENOENT);
+ obex_object_set_io_flags(pbap->obj, G_IO_ERR, -ENOENT);
return;
}
/* Computing offset considering first entry of the phonebook */
l = g_slist_nth(sorted, pbap->params->liststartoffset);
- pbap->buffer = g_string_new(VCARD_LISTING_BEGIN);
+ pbap->obj->buffer = g_string_new(VCARD_LISTING_BEGIN);
for (; l && max; l = l->next, max--) {
const struct cache_entry *entry = l->data;
- g_string_append_printf(pbap->buffer, VCARD_LISTING_ELEMENT,
+ g_string_append_printf(pbap->obj->buffer, VCARD_LISTING_ELEMENT,
entry->handle, entry->name);
}
- pbap->buffer = g_string_append(pbap->buffer, VCARD_LISTING_END);
+ pbap->obj->buffer = g_string_append(pbap->obj->buffer, VCARD_LISTING_END);
g_slist_free(sorted);
done:
if (!pbap->cache.valid) {
pbap->cache.valid = TRUE;
- obex_object_set_io_flags(pbap, G_IO_IN, 0);
+ obex_object_set_io_flags(pbap->obj, G_IO_IN, 0);
}
}
@@ -445,14 +450,14 @@ static void cache_entry_done(void *user_data)
id = cache_find(&pbap->cache, pbap->find_handle);
if (id == NULL) {
DBG("Entry %d not found on cache", pbap->find_handle);
- obex_object_set_io_flags(pbap, G_IO_ERR, -ENOENT);
+ obex_object_set_io_flags(pbap->obj, G_IO_ERR, -ENOENT);
return;
}
ret = phonebook_get_entry(pbap->folder, id, pbap->params,
query_result, pbap);
if (ret < 0)
- obex_object_set_io_flags(pbap, G_IO_ERR, ret);
+ obex_object_set_io_flags(pbap->obj, G_IO_ERR, ret);
}
static struct apparam_field *parse_aparam(const uint8_t *buffer, uint32_t hlen)
@@ -659,6 +664,9 @@ static void pbap_disconnect(struct obex_session *os, void *user_data)
manager_unregister_session(os);
+ if (pbap->obj)
+ pbap->obj->session = NULL;
+
if (pbap->params) {
g_free(pbap->params->searchval);
g_free(pbap->params);
@@ -689,6 +697,17 @@ static struct obex_service_driver pbap = {
.chkput = pbap_chkput
};
+static struct pbap_object *vobject_create(struct pbap_session *pbap)
+{
+ struct pbap_object *obj;
+
+ obj = g_new0(struct pbap_object, 1);
+ obj->session = pbap;
+ pbap->obj = obj;
+
+ return obj;
+}
+
static void *vobject_pull_open(const char *name, int oflag, mode_t mode,
void *context, size_t *size, int *err)
{
@@ -718,7 +737,7 @@ static void *vobject_pull_open(const char *name, int oflag, mode_t mode,
if (ret < 0)
goto fail;
- return pbap;
+ return vobject_create(pbap);
fail:
if (err)
@@ -752,7 +771,7 @@ static void *vobject_list_open(const char *name, int oflag, mode_t mode,
* Valid cache and empty buffer mean that cache was already
* created within a single session, but no data is available.
*/
- if (!pbap->buffer) {
+ if (!pbap->obj->buffer) {
ret = -ENOENT;
goto fail;
}
@@ -768,7 +787,7 @@ static void *vobject_list_open(const char *name, int oflag, mode_t mode,
goto fail;
done:
- return pbap;
+ return vobject_create(pbap);
fail:
if (err)
@@ -817,7 +836,7 @@ done:
if (ret < 0)
goto fail;
- return pbap;
+ return vobject_create(pbap);
fail:
if (err)
@@ -829,12 +848,13 @@ fail:
static ssize_t vobject_pull_read(void *object, void *buf, size_t count,
uint8_t *hi)
{
- struct pbap_session *pbap = object;
+ struct pbap_object *obj = object;
+ struct pbap_session *pbap = obj->session;
- DBG("buffer %p maxlistcount %d", pbap->buffer,
+ DBG("buffer %p maxlistcount %d", obj->buffer,
pbap->params->maxlistcount);
- if (!pbap->buffer)
+ if (!obj->buffer)
return -EAGAIN;
/* PhoneBookSize */
@@ -844,13 +864,14 @@ static ssize_t vobject_pull_read(void *object, void *buf, size_t count,
/* Stream data */
*hi = OBEX_HDR_BODY;
- return string_read(pbap->buffer, buf, count);
+ return string_read(obj->buffer, buf, count);
}
static ssize_t vobject_list_read(void *object, void *buf, size_t count,
uint8_t *hi)
{
- struct pbap_session *pbap = object;
+ struct pbap_object *obj = object;
+ struct pbap_session *pbap = obj->session;
DBG("valid %d maxlistcount %d", pbap->cache.valid,
pbap->params->maxlistcount);
@@ -864,31 +885,34 @@ static ssize_t vobject_list_read(void *object, void *buf, size_t count,
else
*hi = OBEX_HDR_BODY;
- return string_read(pbap->buffer, buf, count);
+ return string_read(obj->buffer, buf, count);
}
static ssize_t vobject_vcard_read(void *object, void *buf, size_t count,
uint8_t *hi)
{
- struct pbap_session *pbap = object;
+ struct pbap_object *obj = object;
- DBG("buffer %p", pbap->buffer);
+ DBG("buffer %p", obj->buffer);
- if (!pbap->buffer)
+ if (!obj->buffer)
return -EAGAIN;
*hi = OBEX_HDR_BODY;
- return string_read(pbap->buffer, buf, count);
+ return string_read(obj->buffer, buf, count);
}
static int vobject_close(void *object)
{
- struct pbap_session *pbap = object;
+ struct pbap_object *obj = object;
- if (pbap->buffer) {
- string_free(pbap->buffer);
- pbap->buffer = NULL;
- }
+ if (obj->session)
+ obj->session->obj = NULL;
+
+ if (obj->buffer)
+ g_string_free(obj->buffer, TRUE);
+
+ g_free(obj);
return 0;
}
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.
From: Arnd Bergmann @ 2010-11-11 15:12 UTC (permalink / raw)
To: Par-Gunnar Hjalmdahl
Cc: Alan Cox, linus.walleij, linux-bluetooth, linux-kernel,
Lukasz.Rymanowski
In-Reply-To: <AANLkTi=J_K0sEwyfr4PoFBzEYp4yqYVm3NCRLv6mfLBx@mail.gmail.com>
On Thursday 11 November 2010, Par-Gunnar Hjalmdahl wrote:
> > One option would of course be to modify the existing hci_ldisc.c but I
> > feel that be rather dangerous and which could create a lot of
> > problems.
>
> After looking again at the code I see that I was wrong.
> For the receiving path the data will go ldics->protocol->stack. It's
> actually the TX path (to the chip) that is a bit strange where
> Bluetooth data is going to stack->ldisc->protocol->ldisc->uart. Here
> we would have a separate path for FM and GPS.
Ok, but is that a problem? You really should not be afraid of
touching existing code in order to make it more generic or removing
strange code paths like the one you just described. Cleaning up
code is usually better than duplicating it when you notice something
wrong with the existing implementation.
Simply calling the underlying ldisc module from the FM and GPS modules
should not even require changes, right?
> I still don't like the idea of making a tty/ldisc for the SPI
> transport. I definitely would prefer instead a new ldisc which doesn't
> register to any stack on top.
Yes, agreed. You had already convinced me in the previous reply, sorry
if that wasn't clear.
> My preference would be to keep the
> solutions independent, where we use tty/ldisc for UART and a direct
> transport protocol driver for SPI (i.e. registering using
> spi_register_driver).
Yes. However, you can probably share substantial parts of the code
between the spi_driver and the hci_uart_proto implementations.
You can do this either by making them a single module that registers
to both hci_ldisc and spi_bus, or having a common cg2900 base module
that does the common parts and add separate modules for spi and
hci_uart that register a small wrapper to the respective subsystems.
Arnd
^ permalink raw reply
* Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.
From: Par-Gunnar Hjalmdahl @ 2010-11-11 14:40 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Alan Cox, linus.walleij, linux-bluetooth, linux-kernel,
Lukasz.Rymanowski
In-Reply-To: <AANLkTinzcRv5Mc+9HRwxYgxZCBNER_RdTVHTOyVtozJf@mail.gmail.com>
2010/11/11 Par-Gunnar Hjalmdahl <pghatwork@gmail.com>:
> 2010/11/8 Arnd Bergmann <arnd@arndb.de>:
>> On Friday 05 November 2010, Par-Gunnar Hjalmdahl wrote:
>>> 2010/10/31, Alan Cox <alan@lxorguk.ukuu.org.uk>:
>>> >> It's about the ldisc number. Both bluetooth and cg2900 register themselves
>>> >> to ldisc 15 (N_HCI). A user doing TIOCSETD can only get one of the two.
>>> >
>>> > Ah I see - I had assumed any actual final merge would be assigning it a
>>> > new LDISC code as is required.
>>>
>>> Sorry for not answering quicker. We in my department have been
>>> discussing new design as well as trying out some solutions.
>>>
>>> As an answer to previous comments, both from Arnd and Alan, we would
>>> like to do the following:
>>> - Introduce a new ldisc called N_H4_COMBO with a ldisc driver
>>> accordingly that will be placed under drivers/char
>>
>> I'm not convinced, although that's what Alan was suggesting. I'd like
>> to hear from the bluetooth people what they think about this.
>>
>> Could you summarize why you think that cg2900 is different enough from
>> an HCI to require a different line discipline? From your previous
>> explanation it sounded like it was mostly added functionality,
>> not something entirely different.
>>
>>> We were thinking about if we could re-use the existing hci_ldisc
>>> driver. As stated before the big problem here is however that
>>> hci_ldisc directly registers to the Bluetooth stack. We could separate
>>> the data for FM and GPS in the protocol driver, but it is pretty ugly
>>> to have two separate data paths within the same driver.
>>
>> One of us must be misreading the code. As far as I can tell, hci_ldisc
>> does not register to the bluetooth stack at all, it just provides
>> the basic multiplex for multiple HCI protocols, while hci_h4.c
>> communicates to the bluetooth stack.
>>
>> If I read it correctly, that means that you can still use hci_ldisc,
>> but need to add another protocol next to hci_h4 and hci_bcsp for
>> your cg2900.
>>
>
> If you look in function hci_ldisc.c/hci_uart_register_dev(), it here
> registers as a driver to the Bluetooth stack. This means that received
> Bluetooth packets would go ldisc->protocol->ldisc->bluetooth, while FM
> and GPS would go ldisc->protocol->(FM/GPS)stack. I think it's quite
> bad to have two different data paths like this. The new ldisc we're
> creating looks a lot like hci_ldisc, but it does not register itself
> to an overlaying stack directly.
> One option would of course be to modify the existing hci_ldisc.c but I
> feel that be rather dangerous and which could create a lot of
> problems.
>
> /P-G
>
After looking again at the code I see that I was wrong.
For the receiving path the data will go ldics->protocol->stack. It's
actually the TX path (to the chip) that is a bit strange where
Bluetooth data is going to stack->ldisc->protocol->ldisc->uart. Here
we would have a separate path for FM and GPS.
I still don't like the idea of making a tty/ldisc for the SPI
transport. I definitely would prefer instead a new ldisc which doesn't
register to any stack on top. My preference would be to keep the
solutions independent, where we use tty/ldisc for UART and a direct
transport protocol driver for SPI (i.e. registering using
spi_register_driver).
/P-G
^ permalink raw reply
* Re: [PATCH 5/9] mfd: Add UART support for the ST-Ericsson CG2900.
From: Par-Gunnar Hjalmdahl @ 2010-11-11 14:28 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Alan Cox, linus.walleij, linux-bluetooth, linux-kernel,
Lukasz.Rymanowski
In-Reply-To: <201011080624.49224.arnd@arndb.de>
2010/11/8 Arnd Bergmann <arnd@arndb.de>:
> On Friday 05 November 2010, Par-Gunnar Hjalmdahl wrote:
>> 2010/10/31, Alan Cox <alan@lxorguk.ukuu.org.uk>:
>> >> It's about the ldisc number. Both bluetooth and cg2900 register themselves
>> >> to ldisc 15 (N_HCI). A user doing TIOCSETD can only get one of the two.
>> >
>> > Ah I see - I had assumed any actual final merge would be assigning it a
>> > new LDISC code as is required.
>>
>> Sorry for not answering quicker. We in my department have been
>> discussing new design as well as trying out some solutions.
>>
>> As an answer to previous comments, both from Arnd and Alan, we would
>> like to do the following:
>> - Introduce a new ldisc called N_H4_COMBO with a ldisc driver
>> accordingly that will be placed under drivers/char
>
> I'm not convinced, although that's what Alan was suggesting. I'd like
> to hear from the bluetooth people what they think about this.
>
> Could you summarize why you think that cg2900 is different enough from
> an HCI to require a different line discipline? From your previous
> explanation it sounded like it was mostly added functionality,
> not something entirely different.
>
>> We were thinking about if we could re-use the existing hci_ldisc
>> driver. As stated before the big problem here is however that
>> hci_ldisc directly registers to the Bluetooth stack. We could separate
>> the data for FM and GPS in the protocol driver, but it is pretty ugly
>> to have two separate data paths within the same driver.
>
> One of us must be misreading the code. As far as I can tell, hci_ldisc
> does not register to the bluetooth stack at all, it just provides
> the basic multiplex for multiple HCI protocols, while hci_h4.c
> communicates to the bluetooth stack.
>
> If I read it correctly, that means that you can still use hci_ldisc,
> but need to add another protocol next to hci_h4 and hci_bcsp for
> your cg2900.
>
If you look in function hci_ldisc.c/hci_uart_register_dev(), it here
registers as a driver to the Bluetooth stack. This means that received
Bluetooth packets would go ldisc->protocol->ldisc->bluetooth, while FM
and GPS would go ldisc->protocol->(FM/GPS)stack. I think it's quite
bad to have two different data paths like this. The new ldisc we're
creating looks a lot like hci_ldisc, but it does not register itself
to an overlaying stack directly.
One option would of course be to modify the existing hci_ldisc.c but I
feel that be rather dangerous and which could create a lot of
problems.
/P-G
^ permalink raw reply
* Re: [PATCH] Fix test-attrib not being listed in EXTRA_DIST
From: Johan Hedberg @ 2010-11-11 14:24 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <1289484528-23630-1-git-send-email-luiz.dentz@gmail.com>
Hi Luiz,
On Thu, Nov 11, 2010, Luiz Augusto von Dentz wrote:
> --- a/Makefile.tools
> +++ b/Makefile.tools
> @@ -191,7 +191,8 @@ EXTRA_DIST += test/apitest test/hsplay test/hsmicro test/dbusdef.py \
> test/test-network test/simple-agent test/simple-service \
> test/simple-endpoint test/test-audio test/test-input \
> test/service-record.dtd test/service-did.xml \
> - test/service-spp.xml test/service-opp.xml test/service-ftp.xml
> + test/service-spp.xml test/service-opp.xml test/service-ftp.xml \
> + test/test-attrib
>
>
> if HIDD
Pushed upstream after fixing to group this together with the rest of the
test scripts.
Johan
^ permalink raw reply
* [PATCH] Fix test-attrib not being listed in EXTRA_DIST
From: Luiz Augusto von Dentz @ 2010-11-11 14:08 UTC (permalink / raw)
To: linux-bluetooth
From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
---
Makefile.tools | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/Makefile.tools b/Makefile.tools
index 797b53d..d682225 100644
--- a/Makefile.tools
+++ b/Makefile.tools
@@ -191,7 +191,8 @@ EXTRA_DIST += test/apitest test/hsplay test/hsmicro test/dbusdef.py \
test/test-network test/simple-agent test/simple-service \
test/simple-endpoint test/test-audio test/test-input \
test/service-record.dtd test/service-did.xml \
- test/service-spp.xml test/service-opp.xml test/service-ftp.xml
+ test/service-spp.xml test/service-opp.xml test/service-ftp.xml \
+ test/test-attrib
if HIDD
--
1.7.1
^ permalink raw reply related
* Re: [PATCH] Add a new configuration option to disable Low Energy support
From: Johan Hedberg @ 2010-11-11 13:30 UTC (permalink / raw)
To: Claudio Takahasi; +Cc: linux-bluetooth
In-Reply-To: <AANLkTinhT+sauN0HXPom_53i6FRQWDjeSLeBaneLwAmb@mail.gmail.com>
Hi Claudio,
> > If the option is called "le" it shouldn't affect GATT over BR/EDR, so I
> > think the check in attrib_server_init should only affect the LE socket
> > (or then rename the option to e.g. main.opts.attrib).
>
> It is not affecting GATT over BR/EDR. bt_io_listen for BR/EDR is above
> this checking.
>
> My patch "[PATCH] Fix possible memory leak of the GIOChannel in the
> attribute server" switched the order of the bt_io_listen calls between
> BR/EDR and LE. I forgot to send these patches as patch sequence.
Ok, then it's fine. The patch is now pushed upstram.
Johan
^ permalink raw reply
* Re: [PATCH] Add a new configuration option to disable Low Energy support
From: Claudio Takahasi @ 2010-11-11 13:20 UTC (permalink / raw)
To: Claudio Takahasi, linux-bluetooth
In-Reply-To: <20101111091857.GA2862@jh-x301>
Hi Johan,
On Thu, Nov 11, 2010 at 7:18 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Claudio,
>
> On Wed, Nov 10, 2010, Claudio Takahasi wrote:
>> @@ -623,6 +624,9 @@ int attrib_server_init(void)
>>
>> sdp_handle = record->handle;
>>
>> + if (!main_opts.le)
>> + return 0;
>> +
>
> If the option is called "le" it shouldn't affect GATT over BR/EDR, so I
> think the check in attrib_server_init should only affect the LE socket
> (or then rename the option to e.g. main.opts.attrib).
>
> Johan
>
It is not affecting GATT over BR/EDR. bt_io_listen for BR/EDR is above
this checking.
My patch "[PATCH] Fix possible memory leak of the GIOChannel in the
attribute server" switched the order of the bt_io_listen calls between
BR/EDR and LE. I forgot to send these patches as patch sequence.
Claudio.
^ permalink raw reply
* Re: [PATCH] sbc: added "cc" to the clobber list of mmx inline assembly
From: Johan Hedberg @ 2010-11-11 13:07 UTC (permalink / raw)
To: Siarhei Siamashka; +Cc: linux-bluetooth, Siarhei Siamashka
In-Reply-To: <1289467782-2472-1-git-send-email-siarhei.siamashka@gmail.com>
Hi Siarhei,
On Thu, Nov 11, 2010, Siarhei Siamashka wrote:
> In the case of scale factors calculation optimizations, the inline
> assembly code has instructions which update flags register, but
> "cc" was not mentioned in the clobber list. When optimizing code,
> gcc theoretically is allowed to do a comparison before the inline
> assembly block, and a conditional branch after it which would lead
> to a problem if the flags register gets clobbered. While this is
> apparently not happening in practice with the current versions of
> gcc, the clobber list needs to be corrected.
>
> Regarding the other inline assembly blocks. While most likely it
> is actually unnecessary based on quick review, "cc" is also added
> there to the clobber list because it should have no impact on
> performance in practice. It's kind of cargo cult, but relieves
> us from the need to track the potential updates of flags register
> in all these places.
> ---
> sbc/sbc_primitives_mmx.c | 6 +++---
> 1 files changed, 3 insertions(+), 3 deletions(-)
Thanks for the patch! It has now been pushed upstream.
Johan
^ permalink raw reply
* [PATCH 1/1 v2] Split up object and session in pbap.c
From: Dmitriy Paliy @ 2010-11-11 12:08 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Dmitriy Paliy
In-Reply-To: <1289460972-2971-1-git-send-email-dmitriy.paliy@nokia.com>
Object and session data is separated in PBAP plugin. Reason is that
when OBEX session firstly makes disconnect of service_data, which
corresponds to session in pbap.c, it than closes object, which also
corresponds to session in pbap.c.
Memory is deallocated after PBAP session is disconnected. When OBEX
driver closes the object, it is trying to dereference the deallocated
memory in order to free pbap->buffer data.
Here object and session are separated, while pointers are created to
make one-to-one mapping. pbap_object is created in vobject_..._open
functions after query to tracker submitted. Session and object are
handled separately when freed.
---
plugins/pbap.c | 76 +++++++++++++++++++++++++++++++++++--------------------
1 files changed, 48 insertions(+), 28 deletions(-)
diff --git a/plugins/pbap.c b/plugins/pbap.c
index c64377c..4e55c72 100644
--- a/plugins/pbap.c
+++ b/plugins/pbap.c
@@ -140,8 +140,13 @@ struct pbap_session {
struct apparam_field *params;
char *folder;
uint32_t find_handle;
- GString *buffer;
struct cache cache;
+ struct pbap_object *obj;
+};
+
+struct pbap_object {
+ GString *buffer;
+ struct pbap_session *session;
};
static const uint8_t PBAP_TARGET[TARGET_SIZE] = {
@@ -237,9 +242,9 @@ static void phonebook_size_result(const char *buffer, size_t bufsize,
hdr->len = PHONEBOOKSIZE_LEN;
memcpy(hdr->val, &phonebooksize, sizeof(phonebooksize));
- pbap->buffer = g_string_new_len(aparam, sizeof(aparam));
+ pbap->obj->buffer = g_string_new_len(aparam, sizeof(aparam));
- obex_object_set_io_flags(pbap, G_IO_IN, 0);
+ obex_object_set_io_flags(pbap->obj, G_IO_IN, 0);
}
static void query_result(const char *buffer, size_t bufsize, int vcards,
@@ -250,17 +255,17 @@ static void query_result(const char *buffer, size_t bufsize, int vcards,
DBG("");
if (vcards <= 0) {
- obex_object_set_io_flags(pbap, G_IO_ERR, -ENOENT);
+ obex_object_set_io_flags(pbap->obj, G_IO_ERR, -ENOENT);
return;
}
- if (!pbap->buffer)
- pbap->buffer = g_string_new_len(buffer, bufsize);
+ if (!pbap->obj->buffer)
+ pbap->obj->buffer = g_string_new_len(buffer, bufsize);
else
- pbap->buffer = g_string_append_len(pbap->buffer, buffer,
+ pbap->obj->buffer = g_string_append_len(pbap->obj->buffer, buffer,
bufsize);
- obex_object_set_io_flags(pbap, G_IO_IN, 0);
+ obex_object_set_io_flags(pbap->obj, G_IO_IN, 0);
}
static void cache_entry_notify(const char *id, uint32_t handle,
@@ -392,7 +397,7 @@ static void cache_ready_notify(void *user_data)
hdr->len = PHONEBOOKSIZE_LEN;
memcpy(hdr->val, &size, sizeof(size));
- pbap->buffer = g_string_new_len(aparam, sizeof(aparam));
+ pbap->obj->buffer = g_string_new_len(aparam, sizeof(aparam));
goto done;
}
@@ -406,29 +411,29 @@ static void cache_ready_notify(void *user_data)
if (sorted == NULL) {
pbap->cache.valid = TRUE;
- obex_object_set_io_flags(pbap, G_IO_ERR, -ENOENT);
+ obex_object_set_io_flags(pbap->obj, G_IO_ERR, -ENOENT);
return;
}
/* Computing offset considering first entry of the phonebook */
l = g_slist_nth(sorted, pbap->params->liststartoffset);
- pbap->buffer = g_string_new(VCARD_LISTING_BEGIN);
+ pbap->obj->buffer = g_string_new(VCARD_LISTING_BEGIN);
for (; l && max; l = l->next, max--) {
const struct cache_entry *entry = l->data;
- g_string_append_printf(pbap->buffer, VCARD_LISTING_ELEMENT,
+ g_string_append_printf(pbap->obj->buffer, VCARD_LISTING_ELEMENT,
entry->handle, entry->name);
}
- pbap->buffer = g_string_append(pbap->buffer, VCARD_LISTING_END);
+ pbap->obj->buffer = g_string_append(pbap->obj->buffer, VCARD_LISTING_END);
g_slist_free(sorted);
done:
if (!pbap->cache.valid) {
pbap->cache.valid = TRUE;
- obex_object_set_io_flags(pbap, G_IO_IN, 0);
+ obex_object_set_io_flags(pbap->obj, G_IO_IN, 0);
}
}
@@ -445,14 +450,14 @@ static void cache_entry_done(void *user_data)
id = cache_find(&pbap->cache, pbap->find_handle);
if (id == NULL) {
DBG("Entry %d not found on cache", pbap->find_handle);
- obex_object_set_io_flags(pbap, G_IO_ERR, -ENOENT);
+ obex_object_set_io_flags(pbap->obj, G_IO_ERR, -ENOENT);
return;
}
ret = phonebook_get_entry(pbap->folder, id, pbap->params,
query_result, pbap);
if (ret < 0)
- obex_object_set_io_flags(pbap, G_IO_ERR, ret);
+ obex_object_set_io_flags(pbap->obj, G_IO_ERR, ret);
}
static struct apparam_field *parse_aparam(const uint8_t *buffer, uint32_t hlen)
@@ -689,6 +694,17 @@ static struct obex_service_driver pbap = {
.chkput = pbap_chkput
};
+static struct pbap_object *vobject_create(struct pbap_session *pbap)
+{
+ struct pbap_object *obj;
+
+ obj = g_new0(struct pbap_object, 1);
+ obj->session = pbap;
+ pbap->obj = obj;
+
+ return obj;
+}
+
static void *vobject_pull_open(const char *name, int oflag, mode_t mode,
void *context, size_t *size, int *err)
{
@@ -718,7 +734,7 @@ static void *vobject_pull_open(const char *name, int oflag, mode_t mode,
if (ret < 0)
goto fail;
- return pbap;
+ return vobject_create(pbap);
fail:
if (err)
@@ -752,7 +768,7 @@ static void *vobject_list_open(const char *name, int oflag, mode_t mode,
* Valid cache and empty buffer mean that cache was already
* created within a single session, but no data is available.
*/
- if (!pbap->buffer) {
+ if (!pbap->obj->buffer) {
ret = -ENOENT;
goto fail;
}
@@ -768,7 +784,7 @@ static void *vobject_list_open(const char *name, int oflag, mode_t mode,
goto fail;
done:
- return pbap;
+ return vobject_create(pbap);
fail:
if (err)
@@ -817,7 +833,7 @@ done:
if (ret < 0)
goto fail;
- return pbap;
+ return vobject_create(pbap);
fail:
if (err)
@@ -829,12 +845,13 @@ fail:
static ssize_t vobject_pull_read(void *object, void *buf, size_t count,
uint8_t *hi)
{
- struct pbap_session *pbap = object;
+ struct pbap_object *obj = object;
+ struct pbap_session *pbap = obj->session;
- DBG("buffer %p maxlistcount %d", pbap->buffer,
+ DBG("buffer %p maxlistcount %d", obj->buffer,
pbap->params->maxlistcount);
- if (!pbap->buffer)
+ if (!obj->buffer)
return -EAGAIN;
/* PhoneBookSize */
@@ -844,13 +861,14 @@ static ssize_t vobject_pull_read(void *object, void *buf, size_t count,
/* Stream data */
*hi = OBEX_HDR_BODY;
- return string_read(pbap->buffer, buf, count);
+ return string_read(obj->buffer, buf, count);
}
static ssize_t vobject_list_read(void *object, void *buf, size_t count,
uint8_t *hi)
{
- struct pbap_session *pbap = object;
+ struct pbap_object *obj = object;
+ struct pbap_session *pbap = obj->session;
DBG("valid %d maxlistcount %d", pbap->cache.valid,
pbap->params->maxlistcount);
@@ -864,13 +882,13 @@ static ssize_t vobject_list_read(void *object, void *buf, size_t count,
else
*hi = OBEX_HDR_BODY;
- return string_read(pbap->buffer, buf, count);
+ return string_read(obj->buffer, buf, count);
}
static ssize_t vobject_vcard_read(void *object, void *buf, size_t count,
uint8_t *hi)
{
- struct pbap_session *pbap = object;
+ struct pbap_object *pbap = object;
DBG("buffer %p", pbap->buffer);
@@ -883,13 +901,15 @@ static ssize_t vobject_vcard_read(void *object, void *buf, size_t count,
static int vobject_close(void *object)
{
- struct pbap_session *pbap = object;
+ struct pbap_object *pbap = object;
if (pbap->buffer) {
string_free(pbap->buffer);
pbap->buffer = NULL;
}
+ g_free(pbap);
+
return 0;
}
--
1.7.0.4
^ permalink raw reply related
* [PATCH 0/1 v2] Split up object and session in pbap.c
From: Dmitriy Paliy @ 2010-11-11 12:08 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <1289460972-2971-1-git-send-email-dmitriy.paliy@nokia.com>
Hi,
This is updated version with some improvements to code as given
in comments to previous submission. Also one DBG printout is not
deleted as in previous version of the patch.
BR,
Dmitriy
^ permalink raw reply
* Re: [PATCH] Add iwmmxt optimization for sbc for pxa series cpu
From: Siarhei Siamashka @ 2010-11-11 11:46 UTC (permalink / raw)
To: Keith Mok; +Cc: linux-bluetooth
In-Reply-To: <AANLkTikPiwr+OtsDVbgoya281Zncg0P5Joq_v0UCzZDi@mail.gmail.com>
On Thursday 11 November 2010 10:05:46 Keith Mok wrote:
> This patch add iwmmxt (Intel wireless mmx, pxa platform) optimzation
> for sbc, based on the mmx code.
> Have verified the encoded result against the mmx generated one.
Nice, I guess it should provide a noticeable performance improvement on this
hardware.
Did you run some benchmarks with these optimizations to measure how much they
are helping? The most interesting numbers are for the "44100kHz audio
with bitpool set to 53, 8 subbands, joint stereo" case, which is typically
used for A2DP. This can be done by running:
$ time ./sbcenc -b53 -s8 -j test.au > /dev/null
In my opinion, commit messages for the performance patches are more descriptive
in the following format:
http://git.kernel.org/?p=bluetooth/bluez.git;a=commit;h=e80454d08b4ec098024ddfbdffbd71e9d2f81bd0
And splitting the patch into parts, adding one optimization at a time may be a
good idea (for bisecting purposes).
A few other comments below.
I don't have any IWMMXT capable hardware to test/benchmark, but I checked the
following manuals:
http://download.intel.com/design/intelxscale/31451001.pdf
http://download.intel.com/design/intelxscale/27347302.pdf
> +static inline void sbc_analyze_four_iwmmxt(const int16_t *in, int32_t
> *out, + const FIXED_T *consts)
> +{
> + asm volatile (
> + "tbcstw wr4, %2\n"
> + "wldrd wr0, [%0]\n"
> + "wldrd wr1, [%0, #8]\n"
> + "wldrd wr2, [%1]\n"
> + "wldrd wr3, [%1, #8]\n"
Using back-to-back WLDRD instructions has some performance penalty
"D.3.2.3 Memory Control Pipeline
There is also an additional stall introduced by the core when 2 double word (64
bits) are issued back to back such as:
WLDRD or WSTRD
WLDR[B,H,W,D] or WSTR[B,H,W,D] <- 1 cycle stall.
Critical inner loop sequences can use non memory related instructions following
a WLDRD or WSTRD."
It's better to try rearranging the code so that load instructions are
interleaved with the others whenever it is possible.
> + "wmadds wr0, wr2, wr0\n"
> + "wmadds wr1, wr3, wr1\n"
> + "waddwss wr0, wr0, wr4\n"
> + "waddwss wr1, wr1, wr4\n"
> + "\n"
> + "wldrd wr2, [%0, #16]\n"
> + "wldrd wr3, [%0, #24]\n"
> + "wldrd wr4, [%1, #16]\n"
^^^^^^ (1)
> + "wldrd wr5, [%1, #24]\n"
> + "wmadds wr2, wr4, wr2\n"
^^^^^^^ (2)
It also makes sense to pay attention to instruction latencies. Here you use wr4
register (2) after loading (1) with only one unrelated instruction in between.
And according to "Table D-1. Issue Cycle and Result Latency of the Intel®
Wireless MMX™ 2 Coprocessor Instructions", WLDRD has result latency 3, so that
it works best if you insert 2 unrelated instruction in between.
> + "wmadds wr3, wr5, wr3\n"
> + "waddwss wr0, wr2, wr0\n"
> + "waddwss wr1, wr3, wr1\n"
> + "\n"
> + "wldrd wr2, [%0, #32]\n"
> + "wldrd wr3, [%0, #40]\n"
> + "wldrd wr4, [%1, #32]\n"
> + "wldrd wr5, [%1, #40]\n"
> + "wmadds wr2, wr4, wr2\n"
> + "wmadds wr3, wr5, wr3\n"
According to "Table D-3. Resource Availability Delay for the Multiplier
Pipeline", back-to-back WMADD instructions may have a performance penalty.
> + "waddwss wr0, wr2, wr0\n"
> + "waddwss wr1, wr3, wr1\n"
> + "\n"
> + "wldrd wr2, [%0, #48]\n"
> + "wldrd wr3, [%0, #56]\n"
> + "wldrd wr4, [%1, #48]\n"
> + "wldrd wr5, [%1, #56]\n"
> + "wmadds wr2, wr4, wr2\n"
> + "wmadds wr3, wr5, wr3\n"
> + "waddwss wr0, wr2, wr0\n"
> + "waddwss wr1, wr3, wr1\n"
> + "\n"
> + "wldrd wr2, [%0, #64]\n"
> + "wldrd wr3, [%0, #72]\n"
> + "wldrd wr4, [%1, #64]\n"
> + "wldrd wr5, [%1, #72]\n"
> + "wmadds wr2, wr4, wr2\n"
> + "wmadds wr3, wr5, wr3\n"
> + "waddwss wr0, wr2, wr0\n"
> + "waddwss wr1, wr3, wr1\n"
> + "\n"
> + "tmcr wcgr0, %4\n"
> + "wsrawg wr0, wr0, wcgr0\n"
> + "wsrawg wr1, wr1, wcgr0\n"
> + "wpackwss wr0, wr0, wr0\n"
> + "wpackwss wr1, wr1, wr1\n"
> + "\n"
> + "wldrd wr4, [%1, #80]\n"
> + "wldrd wr5, [%1, #88]\n"
> + "wldrd wr6, [%1, #96]\n"
> + "wldrd wr7, [%1, #104]\n"
> + "wmadds wr2, wr5, wr0\n"
> + "wmadds wr0, wr4, wr0\n"
> + "\n"
> + "wmadds wr3, wr7, wr1\n"
> + "wmadds wr1, wr6, wr1\n"
> + "waddwss wr0, wr1, wr0\n"
> + "waddwss wr2, wr3, wr2\n"
> + "\n"
> + "wstrd wr0, [%3]\n"
> + "wstrd wr2, [%3, #8]\n"
> + :
> + : "r" (in), "r" (consts),
> + "r" (1 << (SBC_PROTO_FIXED4_SCALE - 1)), "r" (out),
> + "r" (SBC_PROTO_FIXED4_SCALE)
> + : "memory");
> +}
> +static void sbc_calc_scalefactors_iwmmxt(
> + int32_t sb_sample_f[16][2][8],
> + uint32_t scale_factor[2][8],
> + int blocks, int channels, int subbands)
> +{
> + int ch, sb;
> + intptr_t blk;
> + for (ch = 0; ch < channels; ch++) {
> + for (sb = 0; sb < subbands; sb += 2) {
> + int b;
> + blk = &sb_sample_f[0][ch][sb];
> + b = blocks;
> + asm volatile (
> + "tbcstw wr0, %4\n"
> + "1:\n"
> + "wldrd wr1, [%0], %2\n"
> + "wxor wr2, wr2, wr2\n"
> + "wcmpgtsw wr3, wr1, wr2\n"
The MMX code was using PCMPGTD and the other instructions just because MMX
instruction set is very limited and did not have the needed instructions. But
you can use WABS and WMAX instructions to do this job better. You can refer to
the original C code and also to ARM NEON optimizations to get some ideas about
how to do this operation faster.
> + "waddwss wr1, wr1, wr3\n"
> + "wcmpgtsw wr2, wr2, wr1\n"
> + "wxor wr1, wr1, wr2\n"
> +
> + "wor wr0, wr0, wr1\n"
> +
> + "subs %1, %1, #1\n"
> + "bne 1b\n"
> +
> + "tmrrc %0, %1, wr0\n"
> + "clz %0, %0\n"
> + "rsb %0, %0, %5\n"
> + "str %0, [%3]\n"
> +
> + "clz %1, %1\n"
> + "rsb %1, %1, %5\n"
> + "str %1, [%3, #4]\n"
> + : "+&r" (blk), "+&r" (b)
> + : "i" ((char *) &sb_sample_f[1][0][0] -
> + (char *) &sb_sample_f[0][0][0]),
> + "r" (&scale_factor[ch][sb]),
> + "r" (1 << SCALE_OUT_BITS),
> + "i" (SCALE_OUT_BITS+1)
> + : "memory");
And this is actually a bug, which exists in the original MMX code too (my
fault). In order to fix it, "cc" needs to be added to the clobber list. I have
just sent a patch for MMX code here:
http://marc.info/?l=linux-bluetooth&m=128946780706187&w=2
Such bug is more dangerous on ARM, because it is up to the developer whether to
update flags in each particular instruction or not. So while almost every
arithmetic x86 instruction updates flags unconditionally, on ARM the flags can
easily survive long enough. That makes it possible for the compiler to
implement more clever optimizations related to setting and checking flags, and
fail if the clobber list does not contain correct information.
> + }
> + }
> +}
--
Best regards,
Siarhei Siamashka
^ permalink raw reply
* Scripts which have been used to calculate the 'magic' constants for sbc encoder
From: Siarhei Siamashka @ 2010-11-11 10:47 UTC (permalink / raw)
To: linux-bluetooth
[-- Attachment #1.1: Type: Text/Plain, Size: 565 bytes --]
Hi,
The scripts which have been used to calculate the 'magic' constants introduced
in commit:
http://git.kernel.org/?p=bluetooth/bluez.git;a=commit;h=f70d1ada0aba5567fbf67cedfb4a4ba1a9f9852e
Remembered that these are not available in bluez repository so sending them
here basically for archival purposes. I also had scripts for finding a suitable
SIMD permutation, but IIRC these were totally unmaintainable (required several
scripts to run, with human intervention in between and tweaking intermediate
data).
--
Best regards,
Siarhei Siamashka
[-- Attachment #1.2: sbc-encoder-quality-tuning-scripts.tar.gz --]
[-- Type: application/x-compressed-tar, Size: 4686 bytes --]
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* Re: [PATCH 1/2] Remove a2dp setup callbacks after they return
From: Johan Hedberg @ 2010-11-11 10:36 UTC (permalink / raw)
To: Luiz Augusto von Dentz; +Cc: linux-bluetooth
In-Reply-To: <1289468425-11997-1-git-send-email-luiz.dentz@gmail.com>
Hi Luiz,
On Thu, Nov 11, 2010, Luiz Augusto von Dentz wrote:
> From: Luiz Augusto von Dentz <luiz.dentz-von@nokia.com>
>
> Since the callback won't be ever called again it make no sense to keep
> them, also this cause a2dp_cancel to assume there are still some pending
> callbacks to be processed and do not abort when it should.
> ---
> audio/a2dp.c | 55 +++++++++++++++++++++++++++++++++++--------------------
> 1 files changed, 35 insertions(+), 20 deletions(-)
Both patches have been pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH 3/3] Code clean up: cache->folder removed
From: Johan Hedberg @ 2010-11-11 10:26 UTC (permalink / raw)
To: Dmitriy Paliy; +Cc: linux-bluetooth
In-Reply-To: <1289460972-2971-3-git-send-email-dmitriy.paliy@nokia.com>
Hi Dmitriy,
On Thu, Nov 11, 2010, Dmitriy Paliy wrote:
> cache->folder is not used anywhere and therefore removed.
> ---
> plugins/pbap.c | 2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/plugins/pbap.c b/plugins/pbap.c
> index af60741..660b17d 100644
> --- a/plugins/pbap.c
> +++ b/plugins/pbap.c
> @@ -125,7 +125,6 @@ struct aparam_header {
> struct cache {
> gboolean valid;
> uint32_t index;
> - char *folder;
> GSList *entries;
> };
>
> @@ -219,7 +218,6 @@ static const char *cache_find(struct cache *cache, uint32_t handle)
>
> static void cache_clear(struct cache *cache)
> {
> - g_free(cache->folder);
> g_slist_foreach(cache->entries, (GFunc) cache_entry_free, NULL);
> g_slist_free(cache->entries);
> cache->entries = NULL;
This one is also now upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH 2/3] Code clean up: pbap->params = params removed
From: Johan Hedberg @ 2010-11-11 10:26 UTC (permalink / raw)
To: Dmitriy Paliy; +Cc: linux-bluetooth
In-Reply-To: <1289460972-2971-2-git-send-email-dmitriy.paliy@nokia.com>
Hi Dmitriy,
On Thu, Nov 11, 2010, Dmitriy Paliy wrote:
> pbap->params = params; removed due to the fact that this assignment is
> already used in the same function.
> ---
> plugins/pbap.c | 1 -
> 1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/plugins/pbap.c b/plugins/pbap.c
> index 7b9f1ff..af60741 100644
> --- a/plugins/pbap.c
> +++ b/plugins/pbap.c
> @@ -617,7 +617,6 @@ static int pbap_get(struct obex_session *os, obex_object_t *obj,
> if (path == NULL)
> return -EBADR;
>
> - pbap->params = params;
> ret = obex_get_stream_start(os, path);
>
> g_free(path);
Pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH 1/3] Split up pbap object and pbap session
From: Johan Hedberg @ 2010-11-11 10:25 UTC (permalink / raw)
To: Dmitriy Paliy; +Cc: linux-bluetooth
In-Reply-To: <1289460972-2971-1-git-send-email-dmitriy.paliy@nokia.com>
Hi Dmitriy,
On Thu, Nov 11, 2010, Dmitriy Paliy wrote:
> +static void *vobject_create(void *user_data)
There's no reason for the void * types here. Use specific types instead.
> @@ -718,10 +736,13 @@ static void *vobject_pull_open(const char *name, int oflag, mode_t mode,
> cb = query_result;
>
> ret = phonebook_pull(name, pbap->params, cb, pbap);
> +
> if (ret < 0)
> goto fail;
No need to add the empty line.
>
> - return pbap;
> + obj = vobject_create(pbap);
> +
> + return obj;
Just do "return vobject_create(pbap);"
> + obj = vobject_create(pbap);
> +
> + return obj;
Same here.
> + obj = vobject_create(pbap);
> +
> + return obj;
And here.
Johan
^ permalink raw reply
* Re: [PATCH] Fix possible memory leak of the GIOChannel in the attribute server
From: Johan Hedberg @ 2010-11-11 10:09 UTC (permalink / raw)
To: Claudio Takahasi; +Cc: linux-bluetooth
In-Reply-To: <1289426161-10045-2-git-send-email-claudio.takahasi@openbossa.org>
Hi Claudio,
On Wed, Nov 10, 2010, Claudio Takahasi wrote:
> ---
> src/attrib-server.c | 32 +++++++++++++++++++-------------
> 1 files changed, 19 insertions(+), 13 deletions(-)
Pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH] Use reference counting of the device object while discovering services
From: Johan Hedberg @ 2010-11-11 10:08 UTC (permalink / raw)
To: Claudio Takahasi; +Cc: linux-bluetooth
In-Reply-To: <1289426161-10045-1-git-send-email-claudio.takahasi@openbossa.org>
Hi Claudio,
On Wed, Nov 10, 2010, Claudio Takahasi wrote:
> ---
> attrib/client.c | 3 ++-
> 1 files changed, 2 insertions(+), 1 deletions(-)
Pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH] Print LE link type on hcitool
From: Johan Hedberg @ 2010-11-11 10:08 UTC (permalink / raw)
To: Sheldon Demario; +Cc: linux-bluetooth
In-Reply-To: <1289415288-18470-1-git-send-email-sheldon.demario@openbossa.org>
Hi Sheldon,
On Wed, Nov 10, 2010, Sheldon Demario wrote:
> ---
> lib/hci.h | 1 +
> tools/hcitool.c | 2 ++
> 2 files changed, 3 insertions(+), 0 deletions(-)
Pushed upstream. Thanks.
Johan
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox