* Re: [PATCH v2 1/7] Fix invalid memory access when EIR field length is zero
From: Johan Hedberg @ 2010-11-12 16:54 UTC (permalink / raw)
To: Inga Stotland
Cc: 'Vinicius Costa Gomes', linux-bluetooth,
'Bruna Moreira'
In-Reply-To: <000b01cb8200$02c24c90$0846e5b0$@org>
Hi Inga,
On Thu, Nov 11, 2010, Inga Stotland wrote:
> Was there a bug to begin with? :)
> The access to eir_data[1] was always valid due to the check (len <
> EIR_DATA_LENGTH - 1)
> and the fact that eir_data is a buffer of fixed length of EIR_DATA_LENGTH
> (240 bytes).
On closer inspection it seems you might be right, however it'd be nice
to get some comments from the original patch author about this (were
there e.g. crashes or some valgrind warnings observed or was this just
speculation based on looking at the code).
Btw, it seems I may need to slow down on my response time to patches so
there's better time for other people to review them too. E.g. both you
and Luiz were a bit late to the game on a couple of recent patches.
Maybe a 24 hour period before I push anything might be good enough?
Johan
^ permalink raw reply
* [PATCH] [RFC] Show multiple classes in Device UUIDs property
From: Elvis Pfützenreuter @ 2010-11-12 14:52 UTC (permalink / raw)
To: linux-bluetooth; +Cc: epx
This patch adds support for multiple service classes in a single SDP record,
This is a common case for HDP profile (a HDP device which is both source and
sink will have classes 0x1401 and 0x1402, but often just one SDP record).
Current implementation gets only the first UUID in record, which can be
any of the two, and prevents an application to filter e.g. sources by
UUIDs property alone.
The patch also prevents a device driver from being probed and/or removed
twice, if two different UUIDs are served by the same driver. Moreover, it
detects the case when one UUID is removed (e.g. 0x1401) but another one
showed up (0x1402) and both are served by the same driver, which means
the driver must not be removed.
NOTE: the main reason I am submitting this patch, is to reopen the
multiple-UUIDs-in-a-profile discussion. This has been tested mainly with
HDP but there may be issues that affect other drivers and invalidates this
solution completely. I am prepared for the lapidation :)
---
src/device.c | 123 +++++++++++++++++++++++++++++++++++++++++----------------
1 files changed, 88 insertions(+), 35 deletions(-)
diff --git a/src/device.c b/src/device.c
index 7c421e3..83ca441 100644
--- a/src/device.c
+++ b/src/device.c
@@ -1186,6 +1186,14 @@ static GSList *device_match_driver(struct btd_device *device,
return uuids;
}
+static int device_drivers_cmp(const void *a, const void *b)
+{
+ const struct btd_driver_data *driver_data = a;
+ const struct btd_device_driver *driver = b;
+
+ return (driver_data->driver == driver ? 0 : 1);
+}
+
void device_probe_drivers(struct btd_device *device, GSList *profiles)
{
GSList *list;
@@ -1208,20 +1216,25 @@ void device_probe_drivers(struct btd_device *device, GSList *profiles)
if (!probe_uuids)
continue;
- driver_data = g_new0(struct btd_driver_data, 1);
-
err = driver->probe(device, probe_uuids);
if (err < 0) {
error("probe failed with driver %s for device %s",
driver->name, device->path);
- g_free(driver_data);
g_slist_free(probe_uuids);
continue;
}
+ if (g_slist_find_custom(device->drivers, driver,
+ (GCompareFunc) device_drivers_cmp)) {
+ g_slist_free(probe_uuids);
+ continue;
+ }
+
+ driver_data = g_new0(struct btd_driver_data, 1);
driver_data->driver = driver;
device->drivers = g_slist_append(device->drivers, driver_data);
+
g_slist_free(probe_uuids);
}
@@ -1254,13 +1267,45 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
DBG("Remove drivers for %s", device->path);
+ for (list = uuids; list; list = list->next) {
+ sdp_record_t *rec;
+
+ device->uuids = g_slist_remove(device->uuids, list->data);
+
+ rec = find_record_in_list(records, list->data);
+ if (!rec)
+ continue;
+
+ delete_record(srcaddr, dstaddr, rec->handle);
+
+ records = sdp_list_remove(records, rec);
+ sdp_record_free(rec);
+
+ }
+
+ if (records)
+ sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
+
for (list = device->drivers; list; list = next) {
struct btd_driver_data *driver_data = list->data;
struct btd_device_driver *driver = driver_data->driver;
const char **uuid;
+ gboolean in_use_by_another_uuid;
next = list->next;
+ in_use_by_another_uuid = FALSE;
+ for (uuid = driver->uuids; *uuid; uuid++) {
+ if (g_slist_find_custom(device->uuids, *uuid,
+ (GCompareFunc) strcasecmp)) {
+ in_use_by_another_uuid = TRUE;
+ break;
+ }
+ }
+
+ if (in_use_by_another_uuid)
+ continue;
+
for (uuid = driver->uuids; *uuid; uuid++) {
if (!g_slist_find_custom(uuids, *uuid,
(GCompareFunc) strcasecmp))
@@ -1277,25 +1322,6 @@ static void device_remove_drivers(struct btd_device *device, GSList *uuids)
break;
}
}
-
- for (list = uuids; list; list = list->next) {
- sdp_record_t *rec;
-
- device->uuids = g_slist_remove(device->uuids, list->data);
-
- rec = find_record_in_list(records, list->data);
- if (!rec)
- continue;
-
- delete_record(srcaddr, dstaddr, rec->handle);
-
- records = sdp_list_remove(records, rec);
- sdp_record_free(rec);
-
- }
-
- if (records)
- sdp_list_free(records, (sdp_free_func_t) sdp_record_free);
}
static void services_changed(struct btd_device *device)
@@ -1323,6 +1349,43 @@ static int rec_cmp(const void *a, const void *b)
return r1->handle - r2->handle;
}
+static void append_uuids(GSList *existing, GSList **uuids, sdp_list_t *svcclass)
+{
+ gchar *uuid;
+ GSList *l;
+
+ do {
+ uuid = bt_uuid2string(svcclass->data);
+
+ l = g_slist_find_custom(existing, uuid, (GCompareFunc) strcmp);
+ if (l) {
+ g_free(uuid);
+ continue;
+ }
+
+ l = g_slist_find_custom(*uuids, uuid, (GCompareFunc) strcmp);
+ if (!l)
+ *uuids = g_slist_append(*uuids, uuid);
+ else
+ g_free(uuid);
+ } while ((svcclass = svcclass->next));
+}
+
+static void remove_uuids(GSList **uuids, sdp_list_t *svcclass)
+{
+ gchar *uuid;
+ GSList *l;
+
+ do {
+ uuid = bt_uuid2string(svcclass->data);
+ l = g_slist_find_custom(*uuids, uuid, (GCompareFunc) strcmp);
+
+ if (l)
+ *uuids = g_slist_remove(*uuids, l->data);
+ g_free(uuid);
+ } while ((svcclass = svcclass->next));
+}
+
static void update_services(struct browse_req *req, sdp_list_t *recs)
{
struct btd_device *device = req->device;
@@ -1339,7 +1402,6 @@ static void update_services(struct browse_req *req, sdp_list_t *recs)
sdp_record_t *rec = (sdp_record_t *) seq->data;
sdp_list_t *svcclass = NULL;
gchar *profile_uuid;
- GSList *l;
if (!rec)
break;
@@ -1394,19 +1456,10 @@ static void update_services(struct browse_req *req, sdp_list_t *recs)
req->records = sdp_list_append(req->records,
sdp_copy_record(rec));
- l = g_slist_find_custom(device->uuids, profile_uuid,
- (GCompareFunc) strcmp);
- if (!l)
- req->profiles_added =
- g_slist_append(req->profiles_added,
- profile_uuid);
- else {
- req->profiles_removed =
- g_slist_remove(req->profiles_removed,
- l->data);
- g_free(profile_uuid);
- }
+ append_uuids(device->uuids, &req->profiles_added, svcclass);
+ remove_uuids(&req->profiles_removed, svcclass);
+ g_free(profile_uuid);
sdp_list_free(svcclass, free);
}
}
--
1.7.0.4
^ permalink raw reply related
* Re: [PATCH v2] Add iwmmxt optimization for sbc for pxa series cpu
From: Siarhei Siamashka @ 2010-11-12 13:22 UTC (permalink / raw)
To: Keith Mok; +Cc: linux-bluetooth
In-Reply-To: <AANLkTi=rVvmwXwtZgMgwfBigyFXKsWRGy==rdcVjkitW@mail.gmail.com>
[-- Attachment #1: Type: Text/Plain, Size: 5034 bytes --]
On Friday 12 November 2010 09:35:04 Keith Mok wrote:
> > Did you run some benchmarks with these optimizations to measure how much
> > they are helping?
>
> Tested on Marvell PXA platform.
> == Before ==
> $ time ./sbcenc -b53 -s8 -j c.au > /dev/null
> real 0m 0.41s
> user 0m 0.40s
> sys 0m 0.00s
>
> == After ==
> $ time ./sbcenc -b53 -s8 -j c.au > /dev/null
> real 0m 0.19s
> user 0m 0.17s
> sys 0m 0.02s
Thanks, this looks consistent with the results of optimizations on the other
platforms where the performance increases roughly twice after adding SIMD
optimizations to the sbc analysis filter.
But maybe it's better to use a bit bigger test file, so that the total time
increases to at least several seconds. With very small times, it's hard to say
whether it is an actual improvement or random noise. It may be ok for such a
huge performance improvement, but with less significant optimizations the
precision of measurements may become a problem.
Also do you have oprofile available on PXA platform? It may provide a nice
statistics about what functions are used and are the performance hot spots.
> > Using back-to-back WLDRD instructions has some performance penalty
>
> I rearrange the instructions and keep the original one as for reference in
> the block that comment out. Since the code is really difficult to read
> after interleaved.
Thanks, this looks like it really should run quite a bit faster than the
previous variant (based on my understanding of intel pdf files).
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:
A1
A2
A3
A4
B1
B2
B3
B4
If the instructions need to be reordered in order to improve scheduling for the
cpu pipeline, then for example
A1
A2
B1
A3
B2
A4
B3
B4
looks much more readable to me than
A1
A2
B1
A3
B2
A4
B3
B4
With different indentation levels, one can still see the flow of instructions
as independent streams. If different levels of indentation in inline assembly
pass coding style test by checkpatch.pl script, then it should be fine.
Also I'm quite curious whether better instructions scheduling provide any clear
improvement, so some numbers comparing older and newer implementation would
be appreciated. I did not suggest that just for entertainment purposes ;) It
really should provide some practical benefit.
If you have time and want to make such a test, iwmmxt intrinsics could be also
tried, so that instructions scheduling and registers allocation becomes a
responsibility of the compiler. But my previous experiments with arm neon
intrinsics showed that the compiler does a very poor job and can't be trusted
to generate fast code. But maybe iwmmxt could be different or gcc could have
improved since than.
> > 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.
>
> Changed as suggested.
> But got a question that the __IWMMXT__ builtin gcc definition is not a
> reliable way to
> determine whether mcpu=iwmmxt2 is turned on or not. It will break when
> compile under pxa270
> which does not support wabs with just mcpu=iwmmx on.
Well, as I said before, I'm not familiar with iwmmxt and pxa platform. And I
did not notice that there are actually several revisions of iwmmxt isa, my bad.
So looks like iwmmxt1 is just as restrictive as the original mmx and the direct
conversion from mmx like you did before may be the right thing. For arm neon
optimizations, the effect of using vector ABS/MAX instructions was just
about 1% of overall performance improvement. Not so much, but every little bit
helps. And if for iwmmxt it causes such backwards compatibility issues, then it
might be not worth it. It's up to you to decide.
I would still suggest to initially have just optimizations for
sbc_analyze_four_iwmmxt/sbc_analyze_eight_iwmmxt in the first patch (or maybe
in two patches). And then add optimization for sbc_calc_scalefactors in a
separate patch later.
Regarding the benchmarks and functions usage:
1. sbc_analyze_four_iwmmxt is important for 4 subbands case ('-s4' option for
sbcenc)
2. sbc_analyze_eight_iwmmxt is important for 8 subbands case ('-s8' option for
sbcenc)
3. sbc_calc_scalefactors is important for either mono audio, or when joint
stereo is *not* used (sbcenc is run without '-j' option).
All of this is better to be benchmarked/tested separately.
--
Best regards,
Siarhei Siamashka
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* [PATCH] Fix pull phonebook with non-zero offset parameter
From: Rafal Michalski @ 2010-11-12 10:39 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Rafal Michalski
For non-zero liststartoffset parameter, contacts which index was before
offset were indexed more than once (e.g. when contact had more than one
phone number or address etc.), so pulling was started earlier - before
offset index was reached. This patch fix this problem and each contact
is indexed only once.
---
plugins/phonebook-tracker.c | 11 ++++++++++-
1 files changed, 10 insertions(+), 1 deletions(-)
diff --git a/plugins/phonebook-tracker.c b/plugins/phonebook-tracker.c
index 672d59f..811e4f0 100644
--- a/plugins/phonebook-tracker.c
+++ b/plugins/phonebook-tracker.c
@@ -1361,6 +1361,7 @@ static void pull_contacts(char **reply, int num_fields, void *user_data)
int last_index, i;
gboolean cdata_present = FALSE;
char *home_addr, *work_addr, *other_addr;
+ static gchar *temp_id;
if (num_fields < 0) {
data->cb(NULL, 0, num_fields, 0, data->user_data);
@@ -1396,7 +1397,14 @@ static void pull_contacts(char **reply, int num_fields, void *user_data)
TRACKER_DEFAULT_CONTACT_ME))
return;
- data->index++;
+ if (data->index == 0)
+ temp_id = NULL;
+
+ if (g_strcmp0(temp_id, reply[CONTACTS_ID_COL])) {
+ data->index++;
+ g_free(temp_id);
+ temp_id = g_strdup(reply[CONTACTS_ID_COL]);
+ }
last_index = params->liststartoffset + params->maxlistcount;
@@ -1495,6 +1503,7 @@ done:
fail:
g_slist_free(data->contacts);
g_free(data);
+ g_free(temp_id);
}
static void add_to_cache(char **reply, int num_fields, void *user_data)
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH v2] Add iwmmxt optimization for sbc for pxa series cpu
From: Keith Mok @ 2010-11-12 7:35 UTC (permalink / raw)
To: Siarhei Siamashka; +Cc: linux-bluetooth
In-Reply-To: <201011111346.58906.siarhei.siamashka@gmail.com>
> Did you run some benchmarks with these optimizations to measure how much they
> are helping?
Tested on Marvell PXA platform.
== Before ==
$ time ./sbcenc -b53 -s8 -j c.au > /dev/null
real 0m 0.41s
user 0m 0.40s
sys 0m 0.00s
== After ==
$ time ./sbcenc -b53 -s8 -j c.au > /dev/null
real 0m 0.19s
user 0m 0.17s
sys 0m 0.02s
> Using back-to-back WLDRD instructions has some performance penalty
I rearrange the instructions and keep the original one as for reference in
the block that comment out. Since the code is really difficult to read
after interleaved.
> 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.
Changed as suggested.
But got a question that the __IWMMXT__ builtin gcc definition is not a
reliable way to
determine whether mcpu=iwmmxt2 is turned on or not. It will break when
compile under pxa270
which does not support wabs with just mcpu=iwmmx on.
Keith
Signed-off-by: Keith Mok <ek9852@gmail.com>
---
diff --git a/Makefile.am b/Makefile.am
index da308a7..03a9bf2 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -65,6 +65,7 @@ noinst_LTLIBRARIES += sbc/libsbc.la
sbc_libsbc_la_SOURCES = sbc/sbc.h sbc/sbc.c sbc/sbc_math.h sbc/sbc_tables.h \
sbc/sbc_primitives.h sbc/sbc_primitives.c \
sbc/sbc_primitives_mmx.h sbc/sbc_primitives_mmx.c \
+ sbc/sbc_primitives_iwmmxt.h sbc/sbc_primitives_iwmmxt.c \
sbc/sbc_primitives_neon.h sbc/sbc_primitives_neon.c \
sbc/sbc_primitives_armv6.h sbc/sbc_primitives_armv6.c
diff --git a/sbc/sbc_primitives.c b/sbc/sbc_primitives.c
index f87fb5a..ad780d0 100644
--- a/sbc/sbc_primitives.c
+++ b/sbc/sbc_primitives.c
@@ -33,6 +33,7 @@
#include "sbc_primitives.h"
#include "sbc_primitives_mmx.h"
+#include "sbc_primitives_iwmmxt.h"
#include "sbc_primitives_neon.h"
#include "sbc_primitives_armv6.h"
@@ -544,6 +545,9 @@ void sbc_init_primitives(struct sbc_encoder_state *state)
#ifdef SBC_BUILD_WITH_ARMV6_SUPPORT
sbc_init_primitives_armv6(state);
#endif
+#ifdef SBC_BUILD_WITH_IWMMXT_SUPPORT
+ sbc_init_primitives_iwmmxt(state);
+#endif
#ifdef SBC_BUILD_WITH_NEON_SUPPORT
sbc_init_primitives_neon(state);
#endif
diff --git a/sbc/sbc_primitives_iwmmxt.c b/sbc/sbc_primitives_iwmmxt.c
new file mode 100644
index 0000000..b988bb1
--- /dev/null
+++ b/sbc/sbc_primitives_iwmmxt.c
@@ -0,0 +1,599 @@
+/*
+ *
+ * Bluetooth low-complexity, subband codec (SBC) library
+ *
+ * Copyright (C) 2010 Keith Mok <ek9852@gmail.com>
+ * Based on sbc_primitives_mmx.c
+ *
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#include <stdint.h>
+#include <limits.h>
+#include "sbc.h"
+#include "sbc_math.h"
+#include "sbc_tables.h"
+
+#include "sbc_primitives_iwmmxt.h"
+
+/*
+ * IWMMXT optimizations
+ */
+
+#ifdef SBC_BUILD_WITH_IWMMXT_SUPPORT
+
+static inline void sbc_analyze_four_iwmmxt(const int16_t *in, int32_t *out,
+ const FIXED_T *consts)
+{
+ asm volatile (
+ "wldrd wr0, [%0]\n"
+ "tbcstw wr4, %2\n"
+ "wldrd wr2, [%1]\n"
+ "wldrd wr1, [%0, #8]\n"
+ "wldrd wr3, [%1, #8]\n"
+ "wmadds wr0, wr2, wr0\n"
+ "wldrd wr6, [%0, #16]\n"
+ "wmadds wr1, wr3, wr1\n"
+ "wldrd wr7, [%0, #24]\n"
+ "waddwss wr0, wr0, wr4\n"
+ "wldrd wr8, [%1, #16]\n"
+ "waddwss wr1, wr1, wr4\n"
+ "wldrd wr9, [%1, #24]\n"
+ "wmadds wr6, wr8, wr6\n"
+ "wldrd wr2, [%0, #32]\n"
+ "wmadds wr7, wr9, wr7\n"
+ "wldrd wr3, [%0, #40]\n"
+ "waddwss wr0, wr6, wr0\n"
+ "wldrd wr4, [%1, #32]\n"
+ "waddwss wr1, wr7, wr1\n"
+ "wldrd wr5, [%1, #40]\n"
+ "wmadds wr2, wr4, wr2\n"
+ "wldrd wr6, [%0, #48]\n"
+ "wmadds wr3, wr5, wr3\n"
+ "wldrd wr7, [%0, #56]\n"
+ "waddwss wr0, wr2, wr0\n"
+ "wldrd wr8, [%1, #48]\n"
+ "waddwss wr1, wr3, wr1\n"
+ "wldrd wr9, [%1, #56]\n"
+ "wmadds wr6, wr8, wr6\n"
+ "wldrd wr2, [%0, #64]\n"
+ "wmadds wr7, wr9, wr7\n"
+ "wldrd wr3, [%0, #72]\n"
+ "waddwss wr0, wr6, wr0\n"
+ "wldrd wr4, [%1, #64]\n"
+ "waddwss wr1, wr7, wr1\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"
+ "wldrd wr4, [%1, #80]\n"
+ "wsrawg wr1, wr1, wcgr0\n"
+ "wldrd wr5, [%1, #88]\n"
+ "wpackwss wr0, wr0, wr0\n"
+ "wldrd wr6, [%1, #96]\n"
+ "wpackwss wr1, wr1, wr1\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)
+ : "wr0", "wr1", "wr2", "wr3", "wr4", "wr5", "wr6", "wr7",
+ "wr8", "wr9", "wcgr0", "memory");
+#if 0
+ /* without pipeline and resultant latency consideration
+ * keep it here for reference
+ * since the latency optimizated code above is difficult to read */
+ asm volatile (
+ "tbcstw wr4, %2\n"
+ "wldrd wr0, [%0]\n"
+ "wldrd wr1, [%0, #8]\n"
+ "wldrd wr2, [%1]\n"
+ "wldrd wr3, [%1, #8]\n"
+ "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"
+ "wldrd wr5, [%1, #24]\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, #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"
+ "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");
+#endif
+}
+
+static inline void sbc_analyze_eight_iwmmxt(const int16_t *in, int32_t *out,
+ const FIXED_T *consts)
+{
+ asm volatile (
+ "wldrd wr0, [%0]\n"
+ "tbcstw wr15, %2\n"
+ "wldrd wr1, [%0, #8]\n"
+ "wldrd wr2, [%0, #16]\n"
+ "wldrd wr3, [%0, #24]\n"
+ "wldrd wr4, [%1]\n"
+ "wldrd wr5, [%1, #8]\n"
+ "wldrd wr6, [%1, #16]\n"
+ "wldrd wr7, [%1, #24]\n"
+ "wmadds wr0, wr0, wr4\n"
+ "wldrd wr8, [%1, #32]\n"
+ "wmadds wr1, wr1, wr5\n"
+ "wldrd wr9, [%1, #40]\n"
+ "wmadds wr2, wr2, wr6\n"
+ "wldrd wr10, [%1, #48]\n"
+ "wmadds wr3, wr3, wr7\n"
+ "wldrd wr11, [%1, #56]\n"
+ "waddwss wr0, wr0, wr15\n"
+ "wldrd wr4, [%0, #32]\n"
+ "waddwss wr1, wr1, wr15\n"
+ "wldrd wr5, [%0, #40]\n"
+ "waddwss wr2, wr2, wr15\n"
+ "wldrd wr6, [%0, #48]\n"
+ "waddwss wr3, wr3, wr15\n"
+ "wldrd wr7, [%0, #56]\n"
+ "wmadds wr4, wr4, wr8\n"
+ "wldrd wr12, [%0, #64]\n"
+ "wmadds wr5, wr5, wr9\n"
+ "wldrd wr13, [%0, #72]\n"
+ "wmadds wr6, wr6, wr10\n"
+ "wldrd wr14, [%0, #80]\n"
+ "wmadds wr7, wr7, wr11\n"
+ "wldrd wr15, [%0, #88]\n"
+ "waddwss wr0, wr4, wr0\n"
+ "wldrd wr8, [%1, #64]\n"
+ "waddwss wr1, wr5, wr1\n"
+ "wldrd wr9, [%1, #72]\n"
+ "waddwss wr2, wr6, wr2\n"
+ "wldrd wr10, [%1, #80]\n"
+ "waddwss wr3, wr7, wr3\n"
+ "wldrd wr11, [%1, #88]\n"
+ "wmadds wr12, wr12, wr8\n"
+ "wldrd wr4, [%0, #96]\n"
+ "wmadds wr13, wr13, wr9\n"
+ "wldrd wr5, [%0, #104]\n"
+ "wmadds wr14, wr14, wr10\n"
+ "wldrd wr6, [%0, #112]\n"
+ "wmadds wr15, wr15, wr11\n"
+ "wldrd wr7, [%0, #120]\n"
+ "waddwss wr0, wr12, wr0\n"
+ "wldrd wr8, [%1, #96]\n"
+ "waddwss wr1, wr13, wr1\n"
+ "wldrd wr9, [%1, #104]\n"
+ "waddwss wr2, wr14, wr2\n"
+ "wldrd wr10, [%1, #112]\n"
+ "waddwss wr3, wr15, wr3\n"
+ "wldrd wr11, [%1, #120]\n"
+ "wmadds wr4, wr4, wr8\n"
+ "wldrd wr12, [%0, #128]\n"
+ "wmadds wr5, wr5, wr9\n"
+ "wldrd wr13, [%0, #136]\n"
+ "wmadds wr6, wr6, wr10\n"
+ "wldrd wr14, [%0, #144]\n"
+ "wmadds wr7, wr7, wr11\n"
+ "wldrd wr15, [%0, #152]\n"
+ "waddwss wr0, wr4, wr0\n"
+ "wldrd wr8, [%1, #128]\n"
+ "waddwss wr1, wr5, wr1\n"
+ "wldrd wr9, [%1, #136]\n"
+ "waddwss wr2, wr6, wr2\n"
+ "wldrd wr10, [%1, #144]\n"
+ "waddwss wr3, wr7, wr3\n"
+ "wldrd wr11, [%1, #152]\n"
+ "wmadds wr12, wr12, wr8\n"
+ "wmadds wr13, wr13, wr9\n"
+ "wmadds wr14, wr14, wr10\n"
+ "wmadds wr15, wr15, wr11\n"
+ "waddwss wr0, wr12, wr0\n"
+ "waddwss wr1, wr13, wr1\n"
+ "waddwss wr2, wr14, wr2\n"
+ "waddwss wr3, wr15, wr3\n"
+ "\n"
+ "tmcr wcgr0, %4\n"
+ "wsrawg wr0, wr0, wcgr0\n"
+ "wsrawg wr1, wr1, wcgr0\n"
+ "wsrawg wr2, wr2, wcgr0\n"
+ "wsrawg wr3, wr3, wcgr0\n"
+ "\n"
+ "wpackwss wr0, wr0, wr0\n"
+ "wpackwss wr1, wr1, wr1\n"
+ "wldrd wr4, [%1, #160]\n"
+ "wpackwss wr2, wr2, wr2\n"
+ "wldrd wr5, [%1, #168]\n"
+ "wpackwss wr3, wr3, wr3\n"
+ "wldrd wr6, [%1, #192]\n"
+ "wmadds wr4, wr4, wr0\n"
+ "wldrd wr7, [%1, #200]\n"
+ "wmadds wr5, wr5, wr0\n"
+ "wldrd wr8, [%1, #224]\n"
+ "wmadds wr6, wr6, wr1\n"
+ "wldrd wr9, [%1, #232]\n"
+ "wmadds wr7, wr7, wr1\n"
+ "waddwss wr4, wr6, wr4\n"
+ "waddwss wr5, wr7, wr5\n"
+ "wmadds wr8, wr8, wr2\n"
+ "wldrd wr6, [%1, #256]\n"
+ "wmadds wr9, wr9, wr2\n"
+ "wldrd wr7, [%1, #264]\n"
+ "waddwss wr4, wr8, wr4\n"
+ "waddwss wr5, wr9, wr5\n"
+ "wmadds wr6, wr6, wr3\n"
+ "wmadds wr7, wr7, wr3\n"
+ "waddwss wr4, wr6, wr4\n"
+ "waddwss wr5, wr7, wr5\n"
+ "\n"
+ "wstrd wr4, [%3]\n"
+ "wstrd wr5, [%3, #8]\n"
+ "\n"
+ "wldrd wr6, [%1, #176]\n"
+ "wldrd wr5, [%1, #184]\n"
+ "wmadds wr5, wr5, wr0\n"
+ "wldrd wr8, [%1, #208]\n"
+ "wmadds wr0, wr6, wr0\n"
+ "wldrd wr9, [%1, #216]\n"
+ "wmadds wr9, wr9, wr1\n"
+ "wldrd wr6, [%1, #240]\n"
+ "wmadds wr1, wr8, wr1\n"
+ "wldrd wr7, [%1, #248]\n"
+ "waddwss wr0, wr1, wr0\n"
+ "waddwss wr5, wr9, wr5\n"
+ "wmadds wr7, wr7, wr2\n"
+ "wldrd wr8, [%1, #272]\n"
+ "wmadds wr2, wr6, wr2\n"
+ "wldrd wr9, [%1, #280]\n"
+ "waddwss wr0, wr2, wr0\n"
+ "waddwss wr5, wr7, wr5\n"
+ "wmadds wr9, wr9, wr3\n"
+ "wmadds wr3, wr8, wr3\n"
+ "waddwss wr0, wr3, wr0\n"
+ "waddwss wr5, wr9, wr5\n"
+ "\n"
+ "wstrd wr0, [%3, #16]\n"
+ "wstrd wr5, [%3, #24]\n"
+ :
+ : "r" (in), "r" (consts),
+ "r" (1 << (SBC_PROTO_FIXED8_SCALE - 1)), "r" (out),
+ "r" (SBC_PROTO_FIXED8_SCALE)
+ : "wr0", "wr1", "wr2", "wr3", "wr4", "wr5", "wr6", "wr7",
+ "wr8", "wr9", "wr10", "wr11", "wr12", "wr13", "wr14", "wr15",
+ "wcgr0", "memory");
+#if 0
+ /* without pipeline and resultant latency consideration
+ * keep it here for reference
+ * since the latency optimizated code above is difficult to read */
+ asm volatile (
+ "tbcstw wr8, %2\n"
+ "wldrd wr0, [%0]\n"
+ "wldrd wr1, [%0, #8]\n"
+ "wldrd wr2, [%0, #16]\n"
+ "wldrd wr3, [%0, #24]\n"
+ "wldrd wr4, [%1]\n"
+ "wldrd wr5, [%1, #8]\n"
+ "wldrd wr6, [%1, #16]\n"
+ "wldrd wr7, [%1, #24]\n"
+ "wmadds wr0, wr0, wr4\n"
+ "wmadds wr1, wr1, wr5\n"
+ "wmadds wr2, wr2, wr6\n"
+ "wmadds wr3, wr3, wr7\n"
+ "waddwss wr0, wr0, wr8\n"
+ "waddwss wr1, wr1, wr8\n"
+ "waddwss wr2, wr2, wr8\n"
+ "waddwss wr3, wr3, wr8\n"
+ "\n"
+ "wldrd wr4, [%0, #32]\n"
+ "wldrd wr5, [%0, #40]\n"
+ "wldrd wr6, [%0, #48]\n"
+ "wldrd wr7, [%0, #56]\n"
+ "wldrd wr8, [%1, #32]\n"
+ "wldrd wr9, [%1, #40]\n"
+ "wldrd wr10, [%1, #48]\n"
+ "wldrd wr11, [%1, #56]\n"
+ "wmadds wr4, wr4, wr8\n"
+ "wmadds wr5, wr5, wr9\n"
+ "wmadds wr6, wr6, wr10\n"
+ "wmadds wr7, wr7, wr11\n"
+ "waddwss wr0, wr4, wr0\n"
+ "waddwss wr1, wr5, wr1\n"
+ "waddwss wr2, wr6, wr2\n"
+ "waddwss wr3, wr7, wr3\n"
+ "\n"
+ "wldrd wr4, [%0, #64]\n"
+ "wldrd wr5, [%0, #72]\n"
+ "wldrd wr6, [%0, #80]\n"
+ "wldrd wr7, [%0, #88]\n"
+ "wldrd wr8, [%1, #64]\n"
+ "wldrd wr9, [%1, #72]\n"
+ "wldrd wr10, [%1, #80]\n"
+ "wldrd wr11, [%1, #88]\n"
+ "wmadds wr4, wr4, wr8\n"
+ "wmadds wr5, wr5, wr9\n"
+ "wmadds wr6, wr6, wr10\n"
+ "wmadds wr7, wr7, wr11\n"
+ "waddwss wr0, wr4, wr0\n"
+ "waddwss wr1, wr5, wr1\n"
+ "waddwss wr2, wr6, wr2\n"
+ "waddwss wr3, wr7, wr3\n"
+ "\n"
+ "wldrd wr4, [%0, #96]\n"
+ "wldrd wr5, [%0, #104]\n"
+ "wldrd wr6, [%0, #112]\n"
+ "wldrd wr7, [%0, #120]\n"
+ "wldrd wr8, [%1, #96]\n"
+ "wldrd wr9, [%1, #104]\n"
+ "wldrd wr10, [%1, #112]\n"
+ "wldrd wr11, [%1, #120]\n"
+ "wmadds wr4, wr4, wr8\n"
+ "wmadds wr5, wr5, wr9\n"
+ "wmadds wr6, wr6, wr10\n"
+ "wmadds wr7, wr7, wr11\n"
+ "waddwss wr0, wr4, wr0\n"
+ "waddwss wr1, wr5, wr1\n"
+ "waddwss wr2, wr6, wr2\n"
+ "waddwss wr3, wr7, wr3\n"
+ "\n"
+ "wldrd wr4, [%0, #128]\n"
+ "wldrd wr5, [%0, #136]\n"
+ "wldrd wr6, [%0, #144]\n"
+ "wldrd wr7, [%0, #152]\n"
+ "wldrd wr8, [%1, #128]\n"
+ "wldrd wr9, [%1, #136]\n"
+ "wldrd wr10, [%1, #144]\n"
+ "wldrd wr11, [%1, #152]\n"
+ "wmadds wr4, wr4, wr8\n"
+ "wmadds wr5, wr5, wr9\n"
+ "wmadds wr6, wr6, wr10\n"
+ "wmadds wr7, wr7, wr11\n"
+ "waddwss wr0, wr4, wr0\n"
+ "waddwss wr1, wr5, wr1\n"
+ "waddwss wr2, wr6, wr2\n"
+ "waddwss wr3, wr7, wr3\n"
+ "\n"
+ "tmcr wcgr0, %4\n"
+ "wsrawg wr0, wr0, wcgr0\n"
+ "wsrawg wr1, wr1, wcgr0\n"
+ "wsrawg wr2, wr2, wcgr0\n"
+ "wsrawg wr3, wr3, wcgr0\n"
+ "\n"
+ "wpackwss wr0, wr0, wr0\n"
+ "wpackwss wr1, wr1, wr1\n"
+ "wpackwss wr2, wr2, wr2\n"
+ "wpackwss wr3, wr3, wr3\n"
+ "\n"
+ "wldrd wr4, [%1, #160]\n"
+ "wldrd wr5, [%1, #168]\n"
+ "wmadds wr4, wr4, wr0\n"
+ "wmadds wr5, wr5, wr0\n"
+ "\n"
+ "wldrd wr6, [%1, #192]\n"
+ "wldrd wr7, [%1, #200]\n"
+ "wmadds wr6, wr6, wr1\n"
+ "wmadds wr7, wr7, wr1\n"
+ "waddwss wr4, wr6, wr4\n"
+ "waddwss wr5, wr7, wr5\n"
+ "\n"
+ "wldrd wr6, [%1, #224]\n"
+ "wldrd wr7, [%1, #232]\n"
+ "wmadds wr6, wr6, wr2\n"
+ "wmadds wr7, wr7, wr2\n"
+ "waddwss wr4, wr6, wr4\n"
+ "waddwss wr5, wr7, wr5\n"
+ "\n"
+ "wldrd wr6, [%1, #256]\n"
+ "wldrd wr7, [%1, #264]\n"
+ "wmadds wr6, wr6, wr3\n"
+ "wmadds wr7, wr7, wr3\n"
+ "waddwss wr4, wr6, wr4\n"
+ "waddwss wr5, wr7, wr5\n"
+ "\n"
+ "wstrd wr4, [%3]\n"
+ "wstrd wr5, [%3, #8]\n"
+ "\n"
+ "wldrd wr4, [%1, #176]\n"
+ "wldrd wr5, [%1, #184]\n"
+ "wmadds wr5, wr5, wr0\n"
+ "wmadds wr0, wr4, wr0\n"
+ "\n"
+ "wldrd wr4, [%1, #208]\n"
+ "wldrd wr7, [%1, #216]\n"
+ "wmadds wr7, wr7, wr1\n"
+ "wmadds wr1, wr4, wr1\n"
+ "waddwss wr0, wr1, wr0\n"
+ "waddwss wr5, wr7, wr5\n"
+ "\n"
+ "wldrd wr4, [%1, #240]\n"
+ "wldrd wr7, [%1, #248]\n"
+ "wmadds wr7, wr7, wr2\n"
+ "wmadds wr2, wr4, wr2\n"
+ "waddwss wr0, wr2, wr0\n"
+ "waddwss wr5, wr7, wr5\n"
+ "\n"
+ "wldrd wr4, [%1, #272]\n"
+ "wldrd wr7, [%1, #280]\n"
+ "wmadds wr7, wr7, wr3\n"
+ "wmadds wr3, wr4, wr3\n"
+ "waddwss wr0, wr3, wr0\n"
+ "waddwss wr5, wr7, wr5\n"
+ "\n"
+ "wstrd wr0, [%3, #16]\n"
+ "wstrd wr5, [%3, #24]\n"
+ :
+ : "r" (in), "r" (consts),
+ "r" (1 << (SBC_PROTO_FIXED8_SCALE - 1)), "r" (out),
+ "r" (SBC_PROTO_FIXED8_SCALE)
+ : "memory");
+#endif
+}
+
+static inline void sbc_analyze_4b_4s_iwmmxt(int16_t *x, int32_t *out,
+ int out_stride)
+{
+ /* Analyze blocks */
+ sbc_analyze_four_iwmmxt(x + 12, out, analysis_consts_fixed4_simd_odd);
+ out += out_stride;
+ sbc_analyze_four_iwmmxt(x + 8, out, analysis_consts_fixed4_simd_even);
+ out += out_stride;
+ sbc_analyze_four_iwmmxt(x + 4, out, analysis_consts_fixed4_simd_odd);
+ out += out_stride;
+ sbc_analyze_four_iwmmxt(x + 0, out, analysis_consts_fixed4_simd_even);
+}
+
+static inline void sbc_analyze_4b_8s_iwmmxt(int16_t *x, int32_t *out,
+ int out_stride)
+{
+ /* Analyze blocks */
+ sbc_analyze_eight_iwmmxt(x + 24, out, analysis_consts_fixed8_simd_odd);
+ out += out_stride;
+ sbc_analyze_eight_iwmmxt(x + 16, out, analysis_consts_fixed8_simd_even);
+ out += out_stride;
+ sbc_analyze_eight_iwmmxt(x + 8, out, analysis_consts_fixed8_simd_odd);
+ out += out_stride;
+ sbc_analyze_eight_iwmmxt(x + 0, out, analysis_consts_fixed8_simd_even);
+}
+
+static void sbc_calc_scalefactors_iwmmxt2(
+ int32_t sb_sample_f[16][2][8],
+ uint32_t scale_factor[2][8],
+ int blocks, int channels, int subbands)
+{
+ int ch, sb;
+ for (ch = 0; ch < channels; ch++) {
+ for (sb = 0; sb < subbands; sb += 2) {
+ int blk = blocks;
+ int32_t *in = &sb_sample_f[0][ch][sb];
+ /* For iwmmxt2, since we use wabs */
+ asm volatile (
+ "wldrd wr1, [%[in]], %[inc]\n"
+ "tbcstw wr0, %[c1]\n"
+ "wldrd wr2, [%[in]], %[inc]\n"
+ "wldrd wr3, [%[in]], %[inc]\n"
+ "wldrd wr4, [%[in]], %[inc]\n"
+ "1:\n"
+ "wabsw wr1, wr1\n"
+ "wabsw wr2, wr2\n"
+ "wabsw wr3, wr3\n"
+ "wabsw wr4, wr4\n"
+ "wmaxuw wr5, wr1, wr2\n"
+ "wldrd wr1, [%[in]], %[inc]\n"
+ "wmaxuw wr6, wr3, wr4\n"
+ "wldrd wr2, [%[in]], %[inc]\n"
+ "wmaxuw wr5, wr5, wr6\n"
+ "wldrd wr3, [%[in]], %[inc]\n"
+ "wmaxuw wr0, wr0, wr5\n"
+ "wldrd wr4, [%[in]], %[inc]\n"
+ "subs %[blk], %[blk], #4\n"
+ "bgt 1b\n"
+
+ "tmrrc %0, %1, wr0\n"
+ "sub %0, %0, #1\n"
+ "clz %0, %0\n"
+ "rsb %0, %0, %[c2]\n"
+ "str %0, [%[out]]\n"
+
+ "sub %1, %1, #1\n"
+ "clz %1, %1\n"
+ "rsb %1, %1, %[c2]\n"
+ "str %1, [%[out], #4]\n"
+ : [in] "+r" (in), [blk] "+r" (blk)
+ : [inc] "i" ((char *) &sb_sample_f[1][0][0] -
+ (char *) &sb_sample_f[0][0][0]),
+ [out] "r" (&scale_factor[ch][sb]),
+ [c1] "r" ((1 << SCALE_OUT_BITS) + 1),
+ [c2] "i" (SCALE_OUT_BITS+1)
+ : "wr0", "wr1", "wr2", "wr3", "wr4", "wr5", "wr6",
+ "cc", "memory");
+ }
+ }
+}
+
+void sbc_init_primitives_iwmmxt(struct sbc_encoder_state *state)
+{
+ state->sbc_analyze_4b_4s = sbc_analyze_4b_4s_iwmmxt;
+ state->sbc_analyze_4b_8s = sbc_analyze_4b_8s_iwmmxt;
+ state->sbc_calc_scalefactors = sbc_calc_scalefactors_iwmmxt2;
+ state->implementation_info = "IWMMXT";
+}
+
+#endif
diff --git a/sbc/sbc_primitives_iwmmxt.h b/sbc/sbc_primitives_iwmmxt.h
new file mode 100644
index 0000000..827d811
--- /dev/null
+++ b/sbc/sbc_primitives_iwmmxt.h
@@ -0,0 +1,38 @@
+/*
+ *
+ * Bluetooth low-complexity, subband codec (SBC) library
+ *
+ * Based on sbc_primitives_mmx.c
+ *
+ *
+ * This library is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU Lesser General Public
+ * License as published by the Free Software Foundation; either
+ * version 2.1 of the License, or (at your option) any later version.
+ *
+ * This library is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * Lesser General Public License for more details.
+ *
+ * You should have received a copy of the GNU Lesser General Public
+ * License along with this library; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA
+ *
+ */
+
+#ifndef __SBC_PRIMITIVES_IWMMXT_H
+#define __SBC_PRIMITIVES_IWMMXT_H
+
+#include "sbc_primitives.h"
+
+#if defined(__GNUC__) && defined(__IWMMXT__) && \
+ !defined(SBC_HIGH_PRECISION) && (SCALE_OUT_BITS == 15)
+
+#define SBC_BUILD_WITH_IWMMXT_SUPPORT
+
+void sbc_init_primitives_iwmmxt(struct sbc_encoder_state *encoder_state);
+
+#endif
+
+#endif
^ permalink raw reply related
* RE: [PATCH v2 1/7] Fix invalid memory access when EIR field length is zero
From: Inga Stotland @ 2010-11-12 0:24 UTC (permalink / raw)
To: 'Johan Hedberg', 'Vinicius Costa Gomes'
Cc: linux-bluetooth, 'Bruna Moreira'
In-Reply-To: <20101111210705.GB24514@jh-x301>
Hi Johan,
-----Original Message-----
From: linux-bluetooth-owner@vger.kernel.org
[mailto:linux-bluetooth-owner@vger.kernel.org] On Behalf Of Johan Hedberg
Sent: Thursday, November 11, 2010 1:07 PM
To: Vinicius Costa Gomes
Cc: linux-bluetooth@vger.kernel.org; Bruna Moreira
Subject: Re: [PATCH v2 1/7] Fix invalid memory access when EIR field length
is zero
Hi,
On Thu, Nov 11, 2010, Vinicius Costa Gomes wrote:
> 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;
Pushed upstream. Thanks.
Johan
--
Was there a bug to begin with? :)
The access to eir_data[1] was always valid due to the check (len <
EIR_DATA_LENGTH - 1)
and the fact that eir_data is a buffer of fixed length of EIR_DATA_LENGTH
(240 bytes).
Oh well, it's upstream already.
Inga
^ permalink raw reply
* [PATCH v3 4/4] Emit "DeviceFound" signal for LE devices
From: Vinicius Costa Gomes @ 2010-11-11 23:39 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Bruna Moreira
In-Reply-To: <1289518769-3009-1-git-send-email-vinicius.gomes@openbossa.org>
From: Bruna Moreira <bruna.moreira@openbossa.org>
The adapter_emit_device_found() function was modified to emit
DeviceFound signal for LE devices as well.
---
src/adapter.c | 30 +++++++++++++++++++++++++-----
1 files changed, 25 insertions(+), 5 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 49ba5fc..92087f6 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3110,6 +3110,27 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
if (device)
paired = device_is_paired(device);
+ /* 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);
+
+ if (dev->le) {
+ emit_device_found(adapter->path, paddr,
+ "Address", DBUS_TYPE_STRING, &paddr,
+ "RSSI", DBUS_TYPE_INT16, &rssi,
+ "Name", DBUS_TYPE_STRING, &dev->name,
+ "Paired", DBUS_TYPE_BOOLEAN, &paired,
+ "UUIDs", DBUS_TYPE_ARRAY, &uuids, uuid_count,
+ NULL);
+
+ g_strfreev(uuids);
+
+ return;
+ }
+
icon = class_to_icon(dev->class);
if (!dev->alias) {
@@ -3121,12 +3142,7 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
} else
alias = g_strdup(dev->alias);
- /* 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;
@@ -3205,6 +3221,10 @@ void adapter_update_device_from_info(struct btd_adapter *adapter,
if (tmp_name)
dev->name = tmp_name;
}
+
+ /* FIXME: check if other information was changed before emitting the
+ * signal */
+ adapter_emit_device_found(adapter, dev, info->data, info->length);
}
void adapter_update_found_devices(struct btd_adapter *adapter, bdaddr_t *bdaddr,
--
1.7.3.2
^ permalink raw reply related
* [PATCH v3 3/4] Extract service UUIDs from advertising data
From: Vinicius Costa Gomes @ 2010-11-11 23:39 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Bruna Moreira
In-Reply-To: <1289518769-3009-1-git-send-email-vinicius.gomes@openbossa.org>
From: Bruna Moreira <bruna.moreira@openbossa.org>
Make get_eir_uuids() return a GSList of strings, so it can be reused to
extract UUIDs from LE advertising data. The bt_strlist2array() helper
function was created to convert a GSList into a plain array of strings
(needed to send through D-Bus).
---
src/adapter.c | 71 ++++++++++++++++++++++++++++++++++++++++++++-------------
src/adapter.h | 1 +
2 files changed, 56 insertions(+), 16 deletions(-)
diff --git a/src/adapter.c b/src/adapter.c
index 724ea74..49ba5fc 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -202,6 +202,8 @@ static void dev_info_free(struct remote_dev_info *dev)
{
g_free(dev->name);
g_free(dev->alias);
+ g_slist_foreach(dev->services, (GFunc) g_free, NULL);
+ g_slist_free(dev->services);
g_free(dev);
}
@@ -2962,11 +2964,27 @@ 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 eir_length,
- size_t *uuid_count)
+static char **strlist2array(GSList *list)
+{
+ GSList *l;
+ unsigned int i, n;
+ char **array;
+
+ if (list == NULL)
+ return NULL;
+
+ n = g_slist_length(list);
+ array = g_new0(char *, n + 1);
+
+ for (l = list, i = 0; l; l = l->next, i++)
+ array[i] = g_strdup((const gchar *) l->data);
+
+ return array;
+}
+
+static GSList *get_eir_uuids(uint8_t *eir_data, size_t eir_length, GSList *list)
{
uint16_t len = 0;
- char **uuids;
size_t total;
size_t uuid16_count = 0;
size_t uuid32_count = 0;
@@ -2975,10 +2993,11 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
uint8_t *uuid32;
uint8_t *uuid128;
uuid_t service;
+ char *uuid_str;
unsigned int i;
if (eir_data == NULL || eir_length == 0)
- return NULL;
+ return list;
while (len < eir_length - 1) {
uint8_t field_len = eir_data[0];
@@ -3011,15 +3030,12 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
/* Bail out if got incorrect length */
if (len > eir_length)
- return NULL;
+ return list;
total = uuid16_count + uuid32_count + uuid128_count;
- *uuid_count = total;
if (!total)
- return NULL;
-
- uuids = g_new0(char *, total + 1);
+ return list;
/* Generate uuids in SDP format (EIR data is Little Endian) */
service.type = SDP_UUID16;
@@ -3028,7 +3044,12 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
val16 = (val16 << 8) + uuid16[0];
service.value.uuid16 = val16;
- uuids[i] = bt_uuid2string(&service);
+ uuid_str = bt_uuid2string(&service);
+ if (g_slist_find_custom(list, uuid_str,
+ (GCompareFunc) strcmp) == NULL)
+ list = g_slist_append(list, uuid_str);
+ else
+ g_free(uuid_str);
uuid16 += 2;
}
@@ -3041,7 +3062,12 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
val32 = (val32 << 8) + uuid32[k];
service.value.uuid32 = val32;
- uuids[i] = bt_uuid2string(&service);
+ uuid_str = bt_uuid2string(&service);
+ if (g_slist_find_custom(list, uuid_str,
+ (GCompareFunc) strcmp) == NULL)
+ list = g_slist_append(list, uuid_str);
+ else
+ g_free(uuid_str);
uuid32 += 4;
}
@@ -3052,11 +3078,16 @@ static char **get_eir_uuids(uint8_t *eir_data, size_t eir_length,
for (k = 0; k < 16; k++)
service.value.uuid128.data[k] = uuid128[16 - k - 1];
- uuids[i] = bt_uuid2string(&service);
+ uuid_str = bt_uuid2string(&service);
+ if (g_slist_find_custom(list, uuid_str,
+ (GCompareFunc) strcmp) == NULL)
+ list = g_slist_append(list, uuid_str);
+ else
+ g_free(uuid_str);
uuid128 += 16;
}
- return uuids;
+ return list;
}
void adapter_emit_device_found(struct btd_adapter *adapter,
@@ -3070,7 +3101,7 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
dbus_int16_t rssi = dev->rssi;
char *alias;
char **uuids = NULL;
- size_t uuid_count = 0;
+ size_t uuid_count;
ba2str(&dev->bdaddr, peer_addr);
ba2str(&adapter->bdaddr, local_addr);
@@ -3090,8 +3121,16 @@ void adapter_emit_device_found(struct btd_adapter *adapter,
} else
alias = g_strdup(dev->alias);
- /* Extract UUIDs from extended inquiry response if any*/
- uuids = get_eir_uuids(eir_data, eir_length, &uuid_count);
+ /* 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;
+ }
emit_device_found(adapter->path, paddr,
"Address", DBUS_TYPE_STRING, &paddr,
diff --git a/src/adapter.h b/src/adapter.h
index e6d2fe6..25fd3c8 100644
--- a/src/adapter.h
+++ b/src/adapter.h
@@ -71,6 +71,7 @@ struct remote_dev_info {
name_status_t name_status;
gboolean le;
/* LE adv data */
+ GSList *services;
uint8_t evt_type;
uint8_t bdaddr_type;
};
--
1.7.3.2
^ permalink raw reply related
* [PATCH v3 2/4] Advertising data: extract local name
From: Vinicius Costa Gomes @ 2010-11-11 23:39 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Bruna Moreira
In-Reply-To: <1289518769-3009-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 4ebf953..724ea74 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3143,6 +3143,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;
@@ -3159,6 +3160,12 @@ 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)
+ 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 b6a851e..833b208 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_advertising_report(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 v3 1/4] Initial advertising data parsing implementation
From: Vinicius Costa Gomes @ 2010-11-11 23:39 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Bruna Moreira
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 | 26 ++++++++++++++++++++++++++
src/adapter.h | 6 ++++++
src/event.c | 13 +++++++++++++
src/event.h | 1 +
5 files changed, 49 insertions(+), 6 deletions(-)
diff --git a/plugins/hciops.c b/plugins/hciops.c
index 418c824..6a88294 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_advertising_report(&BDADDR(index), info);
+ info = (le_advertising_info *) (info->data + info->length + 1);
}
}
diff --git a/src/adapter.c b/src/adapter.c
index 1f39bcc..4ebf953 100644
--- a/src/adapter.c
+++ b/src/adapter.c
@@ -3136,6 +3136,31 @@ static struct remote_dev_info *get_found_dev(struct btd_adapter *adapter,
return dev;
}
+void adapter_update_device_from_info(struct btd_adapter *adapter,
+ le_advertising_info *info)
+{
+ struct remote_dev_info *dev;
+ bdaddr_t bdaddr;
+ gboolean new_dev;
+ int8_t rssi;
+
+ 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,
@@ -3153,6 +3178,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 b0eedb6..e6d2fe6 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,8 @@ 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_device_from_info(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..b6a851e 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_advertising_report(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_device_from_info(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..66f3b56 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_advertising_report(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
* Re: [PATCH 2/2] string_free is specific to filesystem only
From: Johan Hedberg @ 2010-11-11 21:23 UTC (permalink / raw)
To: Dmitriy Paliy; +Cc: linux-bluetooth, Dmitriy Paliy
In-Reply-To: <1289508380-8873-2-git-send-email-dmitriy.paliy@nokia.com>
Hi Dmitriy,
On Thu, Nov 11, 2010, Dmitriy Paliy wrote:
> string_free is a callback function specific to filesystem plugin only.
> It is used by OBEX drivers when closing OBEX object and not API
> exposed to other plugins. g_string_free can be used instead of it when
> required.
> ---
> plugins/filesystem.c | 2 +-
> plugins/filesystem.h | 1 -
> 2 files changed, 1 insertions(+), 2 deletions(-)
Also pushed upstream after similar fixes as with the first patch.
Johan
^ permalink raw reply
* Re: [PATCH 1/2] g_string_free shell be used in IRMC
From: Johan Hedberg @ 2010-11-11 21:23 UTC (permalink / raw)
To: Dmitriy Paliy; +Cc: linux-bluetooth, Dmitriy Paliy
In-Reply-To: <1289508380-8873-1-git-send-email-dmitriy.paliy@nokia.com>
Hi Dmitriy,
On Thu, Nov 11, 2010, Dmitriy Paliy wrote:
> g_string_free instead of string_free shell be used in IRMC plugin.
> string_free is a callback function specific to filesystem plugin
> only and used by OBEX drivers to close an OBEX object.
> ---
> plugins/irmc.c | 10 ++++------
> 1 files changed, 4 insertions(+), 6 deletions(-)
Thanks, pushed upstream after I fixed your email address (should be
@nokia.com, check your gitconfig) and summary line (which should start
with Add/Fix/Make/Use etc to be consistent with the rest of the commit
history.
Johan
^ permalink raw reply
* Re: [PATCH v2] Fix interleave discovery regression
From: Johan Hedberg @ 2010-11-11 21:17 UTC (permalink / raw)
To: Claudio Takahasi; +Cc: linux-bluetooth
In-Reply-To: <1289508326-18143-1-git-send-email-claudio.takahasi@openbossa.org>
Hi Claudio,
On Thu, Nov 11, 2010, Claudio Takahasi wrote:
> Command complete event for LE commands are not being handled properly
> due blocking calls of LE Set Scan Parameters and LE Set Scan Enable
> Commands. Fix wrong Discovering signal emission in the interleaved
> discovery mode.
> ---
> plugins/hciops.c | 34 ++++++++++++++++++++++++++--------
> 1 files changed, 26 insertions(+), 8 deletions(-)
Pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH v2 4/7] Initial advertising data parsing implementation
From: Johan Hedberg @ 2010-11-11 21:16 UTC (permalink / raw)
To: Vinicius Costa Gomes; +Cc: linux-bluetooth, Bruna Moreira
In-Reply-To: <1289501521-21825-4-git-send-email-vinicius.gomes@openbossa.org>
Hi,
On Thu, Nov 11, 2010, Vinicius Costa Gomes wrote:
> + int8_t rssi = 0;
> +
> + rssi = *(info->data + info->length);
There's no point in initializing rssi upon declaration since the very
next thing you do is assign a new value to it (not to mention that in
general we try to avoid initialization upon declaration in the code).
> +void btd_event_adv(bdaddr_t *local, le_advertising_info *info)
I'm not really a fan of the adv abreviation, and the function name isn't
even in danger of growing horribly long if you don't abreviate it. So
could you come up with something more clear? Maybe
btd_event_advertising_info or btd_event_advertising_report?
I'll stop reviewing this patch-set here since the rest seem to depend on
the name of this function.
Johan
^ permalink raw reply
* Re: [PATCH v2 4/7] Initial advertising data parsing implementation
From: Luiz Augusto von Dentz @ 2010-11-11 21:10 UTC (permalink / raw)
To: Vinicius Costa Gomes; +Cc: linux-bluetooth, Bruna Moreira
In-Reply-To: <1289501521-21825-4-git-send-email-vinicius.gomes@openbossa.org>
Hi,
On Thu, Nov 11, 2010 at 8:51 PM, Vinicius Costa Gomes
<vinicius.gomes@openbossa.org> wrote:
> 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;
Again this does sound like a good idea to do the member initialization
in a different function, also isn't there a possibility that the le
bdaddr random/private clashes with br/edr bdaddr? I would say the
search function need to take the address and its type a least.
> + 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;
> };
I would use bdaddr_type and create one for br/edr, we can probably
speed up the search if we can check the address type before the
address and make it impossible to mix LE with BR/EDR devices, their
types would simply not match.
> 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Luiz Augusto von Dentz
Computer Engineer
^ permalink raw reply
* Re: [PATCH v2 3/7] Refactoring adapter_update_found_devices() function
From: Johan Hedberg @ 2010-11-11 21:10 UTC (permalink / raw)
To: Vinicius Costa Gomes; +Cc: linux-bluetooth, Bruna Moreira
In-Reply-To: <1289501521-21825-3-git-send-email-vinicius.gomes@openbossa.org>
Hi,
On Thu, Nov 11, 2010, Vinicius Costa Gomes wrote:
> 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(-)
This one has also been pushed upstream after fixing the following coding
style issue:
> -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)
Same thing as with the previous patch: the parameters on the second line
need to be indented past the opening ( on the first line.
Johan
^ permalink raw reply
* Re: [PATCH v2 2/7] Refactor get_eir_uuids() to get EIR data length parameter
From: Johan Hedberg @ 2010-11-11 21:09 UTC (permalink / raw)
To: Vinicius Costa Gomes; +Cc: linux-bluetooth, Anderson Lizardo
In-Reply-To: <1289501521-21825-2-git-send-email-vinicius.gomes@openbossa.org>
Hi,
On Thu, Nov 11, 2010, Vinicius Costa Gomes wrote:
> 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(-)
Pushed upstream after I fixed the following coding style issue:
> 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)
The parameters on the second line should be indented if possible enough
that they're past the opening ( on the line above (in this case it means
that you need three lines for the parameters).
> 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);
Same here.
Johan
^ permalink raw reply
* Re: [PATCH v2 1/7] Fix invalid memory access when EIR field length is zero
From: Johan Hedberg @ 2010-11-11 21:07 UTC (permalink / raw)
To: Vinicius Costa Gomes; +Cc: linux-bluetooth, Bruna Moreira
In-Reply-To: <1289501521-21825-1-git-send-email-vinicius.gomes@openbossa.org>
Hi,
On Thu, Nov 11, 2010, Vinicius Costa Gomes wrote:
> 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;
Pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH v2 1/7] Fix invalid memory access when EIR field length is zero
From: Johan Hedberg @ 2010-11-11 21:00 UTC (permalink / raw)
To: Luiz Augusto von Dentz
Cc: Vinicius Costa Gomes, linux-bluetooth, Bruna Moreira
In-Reply-To: <AANLkTinSrAkbcZpK9cO=xLJyH253LDuZrGZCgzEKymLG@mail.gmail.com>
Hi Luiz,
On Thu, Nov 11, 2010, Luiz Augusto von Dentz wrote:
> > 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;
>
> IMO type is easier to understand here, we just need to initialize it
> latter after the length check.
True, however I wasn't bothered enough about this and went ahead and
pushed the patch anyway upstream. If someone feels like it, feel free to
reintroduce the variable ;)
Johan
^ permalink raw reply
* Re: [PATCH v3] Split up object and session in pbap.c
From: Johan Hedberg @ 2010-11-11 20:57 UTC (permalink / raw)
To: Dmitriy Paliy; +Cc: linux-bluetooth
In-Reply-To: <1289490660-14884-1-git-send-email-dmitriy.paliy@nokia.com>
Hi Dmitriy,
On Thu, Nov 11, 2010, Dmitriy Paliy wrote:
> 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(-)
The patch has been pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH v2 1/7] Fix invalid memory access when EIR field length is zero
From: Luiz Augusto von Dentz @ 2010-11-11 20:54 UTC (permalink / raw)
To: Vinicius Costa Gomes; +Cc: linux-bluetooth, Bruna Moreira
In-Reply-To: <1289501521-21825-1-git-send-email-vinicius.gomes@openbossa.org>
Hi,
2010/11/11 Vinicius Costa Gomes <vinicius.gomes@openbossa.org>:
> 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;
IMO type is easier to understand here, we just need to initialize it
latter after the length check.
--
Luiz Augusto von Dentz
Computer Engineer
^ permalink raw reply
* Re: [PATCH] Adding a new option to specify medium security level for gatttool
From: Johan Hedberg @ 2010-11-11 20:54 UTC (permalink / raw)
To: Sheldon Demario; +Cc: linux-bluetooth
In-Reply-To: <1289424095-26628-1-git-send-email-sheldon.demario@openbossa.org>
Hi Sheldon,
On Wed, Nov 10, 2010, Sheldon Demario wrote:
> -- Add command line support to use medium instead of (default) low
> - security level with gatttool (--sec-level)
> -
> - Priority: Low
> - Complexity: C1
> -
<snip>
> + { "sec-medium", 's', 0, G_OPTION_ARG_NONE, &opt_medium_sec,
> + "Use medium instead of default low security level",
> + NULL},
Maybe the TODO entry should have been more clear, but I'd prefer if the
option would be of the format "--sec-level <level>" where <level> is
"low", "medium" or "high". I'm not completely sure I like --sec-level
either (even though I did write the original TODO entry :) What do you
think about simply "--security <level>"?
Johan
^ permalink raw reply
* Re: [PATCH v2 3/7] Refactoring adapter_update_found_devices() function
From: Luiz Augusto von Dentz @ 2010-11-11 20:49 UTC (permalink / raw)
To: Vinicius Costa Gomes; +Cc: linux-bluetooth, Bruna Moreira
In-Reply-To: <1289501521-21825-3-git-send-email-vinicius.gomes@openbossa.org>
Hi,
On Thu, Nov 11, 2010 at 8:51 PM, Vinicius Costa Gomes
<vinicius.gomes@openbossa.org> wrote:
> 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
Im not so sure this is doing any good to the code, apparently this add
more code without a clear reason. IMO the only thing that could be
really be beneficial is to have some helper functions such as
dev_info_new/dev_info_free, splitting the memory allocation and the
members initialization doesn't sound a good idea too.
--
Luiz Augusto von Dentz
Computer Engineer
^ permalink raw reply
* [PATCH 2/2] string_free is specific to filesystem only
From: Dmitriy Paliy @ 2010-11-11 20:46 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Dmitriy Paliy
In-Reply-To: <1289508380-8873-1-git-send-email-dmitriy.paliy@nokia.com>
string_free is a callback function specific to filesystem plugin only.
It is used by OBEX drivers when closing OBEX object and not API
exposed to other plugins. g_string_free can be used instead of it when
required.
---
plugins/filesystem.c | 2 +-
plugins/filesystem.h | 1 -
2 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/plugins/filesystem.c b/plugins/filesystem.c
index bf00ac2..bb758ab 100644
--- a/plugins/filesystem.c
+++ b/plugins/filesystem.c
@@ -474,7 +474,7 @@ static void *pcsuite_open(const char *name, int oflag, mode_t mode,
return append_listing(object, name, TRUE, size, err);
}
-int string_free(void *object)
+static int string_free(void *object)
{
GString *string = object;
diff --git a/plugins/filesystem.h b/plugins/filesystem.h
index 712653f..9c7ad9a 100644
--- a/plugins/filesystem.h
+++ b/plugins/filesystem.h
@@ -21,5 +21,4 @@
*
*/
-int string_free(void *object);
ssize_t string_read(void *object, void *buf, size_t count);
--
1.7.0.4
^ permalink raw reply related
* [PATCH 1/2] g_string_free shell be used in IRMC
From: Dmitriy Paliy @ 2010-11-11 20:46 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Dmitriy Paliy
g_string_free instead of string_free shell be used in IRMC plugin.
string_free is a callback function specific to filesystem plugin
only and used by OBEX drivers to close an OBEX object.
---
plugins/irmc.c | 10 ++++------
1 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/plugins/irmc.c b/plugins/irmc.c
index 28c5e50..f7ad33b 100644
--- a/plugins/irmc.c
+++ b/plugins/irmc.c
@@ -254,10 +254,8 @@ static void irmc_disconnect(struct obex_session *os, void *user_data)
g_free(irmc->params);
}
- if (irmc->buffer) {
- string_free(irmc->buffer);
- irmc->buffer = NULL;
- }
+ if (irmc->buffer)
+ g_string_free(irmc->buffer, TRUE);
g_free(irmc);
}
@@ -355,7 +353,7 @@ static void *irmc_open_pb(const char *name, struct irmc_session *irmc,
irmc->buffer = mybuf;
else {
irmc->buffer = g_string_append(irmc->buffer, mybuf->str);
- string_free(mybuf);
+ g_string_free(mybuf, TRUE);
}
return irmc;
@@ -433,7 +431,7 @@ static int irmc_close(void *object)
DBG("");
if (irmc->buffer) {
- string_free(irmc->buffer);
+ g_string_free(irmc->buffer, TRUE);
irmc->buffer = NULL;
}
--
1.7.0.4
^ permalink raw reply related
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