* Re: Is it valid BlueZ Serail-API ?
From: Johan Hedberg @ 2010-06-30 8:44 UTC (permalink / raw)
To: Chanyeol,Park; +Cc: linux-bluetooth
In-Reply-To: <AANLkTinBv-1bUF3ccpMMWcpbAlVB9repG2zuluYMBtUR@mail.gmail.com>
Hi,
On Thu, Jun 24, 2010, Chanyeol,Park wrote:
> There is a log like the bottom.
>
> Current Code doesn't have the below documents.(serial-api.txt) after
> the git-log , however their code is still alive.
>
> I am little bit confused.
>
> Could you tell me whethr their API are valid or not?
>
> If you explain why you removed, it would be helpul.
>
> BR
> Chanyeol.Park
>
> Remove old serial-api.txt documentation file
> author Marcel Holtmann <marcel@holtmann.org>
> Mon, 22 Dec 2008 07:10:37 +0000 (08:10 +0100)
> committer Marcel Holtmann <marcel@holtmann.org>
> Mon, 22 Dec 2008 07:10:37 +0000 (08:10 +0100)
> serial/Makefile.am patch | blob | history
> serial/serial-api.txt [deleted file] patch | blob | history
I think the API simply got moved to doc/serial-api.txt
Johan
^ permalink raw reply
* Re: SBC encoder/decoder API & errors handling
From: Luiz Augusto von Dentz @ 2010-06-30 8:31 UTC (permalink / raw)
To: Siarhei Siamashka, linux-bluetooth, Lennart Poettering
In-Reply-To: <20100630075348.GA23471@jh-x301>
Hi,
On Wed, Jun 30, 2010 at 10:53 AM, Johan Hedberg <johan.hedberg@gmail.com> wrote:
> Hi Siarhei,
>
> On Tue, Jun 29, 2010, Siarhei Siamashka wrote:
>> When I tried to run some SBC encoder tests a few days ago, I noticed that
>> there is a regression introduced by commit:
>> http://git.kernel.org/?p=bluetooth/bluez.git;a=commit;h=c43f8bdcc1d527e2d77481a66217771038be3acd
>>
>> It is caused by the change from 'int' to 'size_t' for the type of variable
>> 'encoded' in 'sbcenc.c'. After this modification, the check for 'encoded <= 0'
>> does not catch the case when 'sbc_encode' tries to return a negative number in
>> 'encoded' variable. Later we end up calling 'write' function with a negative
>> size for the data block.
>>
>> Right now, in the case of error 'sbc_encode' function may either return a
>> negative number as a return value, or return a negative value in 'encoded'
>> variable. But this second type of error is apparently not handled by anything
>> other than 'sbcenc' tool at the moment.
>>
>> Any opinions about how to fix it in the best way? Because it is a flaw in the
>> library API, comments from the interested parties are welcome.
>
> In general the API feels a bit weird in that it can return errors in two
> different ways. A less obtrusive fix would be to make the variable type
> ssize_t instead of size_t. Regarding breaking the SBC library API, I
> don't see any big issue with that since it's internal to bluetoothd and
> we can easily update the pulseaudio copy accordingly. Would the attached
> patch make sense?
I guess it make sense, but we should also fix alsa and gstreamer code
to use variables of type ssize_t instead of int:
audio/pcm_bluetooth.c:1060: encoded = sbc_encode(&a2dp->sbc,
data->buffer, a2dp->codesize,
audio/pcm_bluetooth.c:1096: encoded = sbc_encode(&a2dp->sbc, buff,
a2dp->codesize,
audio/gstsbcenc.c:391: consumed = sbc_encode(&enc->sbc, (gpointer) data,
--
Luiz Augusto von Dentz
Computer Engineer
^ permalink raw reply
* Re: [PATCH 2/2] Added support for updating EIR whenever record is added or removed
From: Johan Hedberg @ 2010-06-30 8:29 UTC (permalink / raw)
To: Inga Stotland; +Cc: linux-bluetooth, rshaffer
In-Reply-To: <1276795490-17262-3-git-send-email-ingas@codeaurora.org>
Hi Inga,
On Thu, Jun 17, 2010, Inga Stotland wrote:
> ---
> plugins/service.c | 18 ++++++
> src/adapter.c | 148 ++++++++++++++++++++++++++++++++++++++++++++++++++--
> src/adapter.h | 5 +-
> src/dbus-hci.c | 6 +-
> src/sdpd-service.c | 106 +++++++++++++++++++++++++++++++------
> src/sdpd.h | 20 +++++++
> 6 files changed, 277 insertions(+), 26 deletions(-)
Could you please split this patch up into smaller chunks if possible. It
seems it has several parts which could be considered logically
independent.
Some other comments:
> void adapter_set_class_complete(bdaddr_t *bdaddr, uint8_t status)
> {
> uint8_t class[3];
> @@ -2677,6 +2698,7 @@ static void append_dict_valist(DBusMessageIter *iter,
> DBusMessageIter dict;
> const char *key;
> int type;
> + int n_elements;
> void *val;
>
> dbus_message_iter_open_container(iter, DBUS_TYPE_ARRAY,
> @@ -2688,7 +2710,12 @@ static void append_dict_valist(DBusMessageIter *iter,
> while (key) {
> type = va_arg(var_args, int);
> val = va_arg(var_args, void *);
> - dict_append_entry(&dict, key, type, val);
> + if (type == DBUS_TYPE_ARRAY) {
> + n_elements = va_arg(var_args, int);
> + if (n_elements > 0)
> + dict_append_array(&dict, key, DBUS_TYPE_STRING, val, n_elements);
> + } else
> + dict_append_entry(&dict, key, type, val);
> key = va_arg(var_args, char *);
> }
>
It looks like this at least could be a separate patch (DBUS_TYPE_ARRAY
support for append_dict_valist).
> @@ -2719,8 +2746,106 @@ static void emit_device_found(const char *path, const char *address,
> g_dbus_send_message(connection, signal);
> }
>
> +
> +static int get_uuid_count_eir (uint8_t *eir_data)
Why the extra empty line? In general there should never be a need for
two consecutive empty lines.
> +{
> + uint8_t type;
> + uint8_t len = 0;
> + uint8_t field_len;
> + int count = 0;
> +
> + while (len < EIR_DATA_LENGTH) {
> + type = eir_data[1];
> + field_len = eir_data[0];
> + if ((type == EIR_UUID16_SOME) || (type == EIR_UUID16_ALL))
> + count += field_len/2;
> + else if ((type == EIR_UUID32_SOME) || (type == EIR_UUID32_ALL))
> + count += field_len/4;
> + else if ((type == EIR_UUID128_SOME) || (type == EIR_UUID128_ALL))
> + count += field_len/16;
> + len += field_len + 1;
> + eir_data += field_len + 1;
> + }
> +
> + return count;
> +}
Variables should always be declared in the minimum scope possible. In
the above function only the count and len variables need to be in the
function scope whereas all other should be declared inside the while
loop.
> +static void get_uuids_eir(char **uuids, uint8_t *eir_data)
> +{
> + uint8_t type;
> + uint8_t len = 0;
> + uint8_t field_len = 0;
> + int count = 0;
> + int size;
> + uuid_t service;
> + gboolean service_found;
> +
> + /* Count UUID16, UUID32 and UUID128 */
> + while (len < EIR_DATA_LENGTH) {
> + type = eir_data[1];
> + field_len = eir_data[0];
> + service_found = FALSE;
Same comment regarding the scope of variables.
> + if ((type == EIR_UUID16_SOME) || (type == EIR_UUID16_ALL)) {
> + size = 2;
> + service.type = SDP_UUID16;
> + service_found = TRUE;
> + } else if ((type == EIR_UUID32_SOME) || (type == EIR_UUID32_ALL)) {
> + size = 4;
> + service.type = SDP_UUID32;
> + service_found = TRUE;
> + } else if ((type == EIR_UUID128_SOME) || (type == EIR_UUID128_ALL)) {
> + size = 16;
> + service.type = SDP_UUID128;
> + service_found = TRUE;
> + }
> +
> + if (service_found) {
I think it'd be more readable to have
if (!service_found)
continue;
The two last operations in the loop still need to be executed to which
the solution would be to use a for loop and have the commands in the
third part (i.e. for (...; ...; <whatever needs to be done every time>)
or use a label+goto.
> + uint8_t *data = &eir_data[2];
> + uint16_t val16;
> + uint32_t val32;
> + int i, k;
> +
> + count = field_len/size;
Space before and after /
> +
> + /* Generate uuids in SDP format (EIR data is Little Endian) */
> + switch (service.type) {
> + case SDP_UUID16:
> + for (i = 0; i < count; i++) {
> + val16 = data[1];
> + val16 = (val16<<8) + data[0];
> + service.value.uuid16 = val16;
> + *uuids++ = bt_uuid2string(&service);
> + data += size;
> + }
> + break;
> + case SDP_UUID32:
> + for (i = 0; i < count; i++) {
> + val32 = data[3];
> + for (k = size-2 ; k >= 0; k--)
> + val32 = (val32<<8) + data[k];
Space before and after <<
> + service.value.uuid32 = val32;
> + *uuids++ = bt_uuid2string(&service);
> + data += size;
> + }
> + break;
> + case SDP_UUID128:
> + for (i = 0; i < count; i++) {
> + for (k = 0; k < size; k++)
> + service.value.uuid128.data[k] = data[size-k-1];
Space before and after -
You're running into trouble with the 80-character line limit while still
keeping the code readable, but imho the core issue is just a too complex
and long function. Please consider if you could find a way to refactor
or rearrange it somehow.
> void adapter_emit_device_found(struct btd_adapter *adapter,
> - struct remote_dev_info *dev);
> + struct remote_dev_info *dev, uint8_t *eir_data);
Looks like a mixed tabs & spaces coding style violation.
> diff --git a/src/sdpd-service.c b/src/sdpd-service.c
> index 798e49d..5aa00f1 100644
> --- a/src/sdpd-service.c
> +++ b/src/sdpd-service.c
> @@ -181,23 +181,30 @@ void create_ext_inquiry_response(const char *name,
With your changes this function is growing quite ridiculously huge.
Would there be any way to avoid that?
> - ptr += len + 2;
> + eir_len += (len + 2);
Tab & space before the += (it should just be a space)
> *ptr++ = (did_version & 0xff00) >> 8;
> + eir_len += 10;
> }
Same here.
> + /* Group all UUID128 types */
> + if (!truncated && (eir_len <= EIR_DATA_LENGTH - 2 )) {
> +
> + list = services;
> + index = 0;
> +
> + /* Store UUID128 in place, skipping first 2 bytes to insert type and length later */
Looks to me like this is beyond the 80-character limit.
> + uuid128 = ptr + 2;
> +
> + for (; list; list = list->next) {
> + sdp_record_t *rec = (sdp_record_t *) list->data;
> +
> + if (rec->svclass.type != SDP_UUID128)
> + continue;
> +
> + /* Stop if not enough space to put next UUID128 */
> + if ((eir_len + 2 + SIZEOF_UUID128) > EIR_DATA_LENGTH) {
> + truncated = TRUE;
> + break;
> + }
> +
> + /* Check for duplicates, EIR data is Little Endian */
> + for (i = 0; i < index; i++) {
> + for (k = 0; k < SIZEOF_UUID128; k++) {
> + if (uuid128[i*SIZEOF_UUID128 + k] !=
Space before and after *
> + rec->svclass.value.uuid128.data[SIZEOF_UUID128 - k])
80-character limit exceeded
> + ptr = data + eir_len;
Tab & space before = (it should be just a space)
> +#define EIR_DATA_LENGTH 240
> +
> +#define EIR_FLAGS 0x01 /* Flags */
> +#define EIR_UUID16_SOME 0x02 /* 16-bit UUID, more available */
> +#define EIR_UUID16_ALL 0x03 /* 16-bit UUID, all listed */
> +#define EIR_UUID32_SOME 0x04 /* 32-bit UUID, more available */
> +#define EIR_UUID32_ALL 0x05 /* 32-bit UUID, all listed */
> +#define EIR_UUID128_SOME 0x06 /* 128-bit UUID, more available */
> +#define EIR_UUID128_ALL 0x07 /* 128-bit UUID, all listed */
> +#define EIR_NAME_SHORT 0x08 /* shortened local name */
> +#define EIR_NAME_COMPLETE 0x09 /* complete local name */
> +#define EIR_TX_POWER 0x0A /* Transmit power level */
> +#define OOB_CLASS_OF_DEVICE 0x0D /* Class of Device (OOB only) */
> +#define OOB_SIMPLE_PAIRING_HASH_C 0x0E /* Simple Pairing HashC (OOB only) */
> +#define OOB_SIMPLE_PAIRING_RAND_R 0x0F /* Simple Pairing RandR (OOB only) */
> +#define EIR_DEVICE_ID 0x10 /* Device ID */
> +#define EIR_MANF_DATA 0xFF /* Manufacturer Specific Data */
> +
> +#define SIZEOF_UUID128 16
Do all of these defines really need to be in the sdpd.h header? At least
stuff like SIZEOF_UUID128 should be moved into the .c file that uses it
(if it's really needed at all).
Johan
^ permalink raw reply
* Re: SBC encoder/decoder API & errors handling
From: Johan Hedberg @ 2010-06-30 7:53 UTC (permalink / raw)
To: Siarhei Siamashka; +Cc: linux-bluetooth, Lennart Poettering
In-Reply-To: <201006291845.53998.siarhei.siamashka@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1449 bytes --]
Hi Siarhei,
On Tue, Jun 29, 2010, Siarhei Siamashka wrote:
> When I tried to run some SBC encoder tests a few days ago, I noticed that
> there is a regression introduced by commit:
> http://git.kernel.org/?p=bluetooth/bluez.git;a=commit;h=c43f8bdcc1d527e2d77481a66217771038be3acd
>
> It is caused by the change from 'int' to 'size_t' for the type of variable
> 'encoded' in 'sbcenc.c'. After this modification, the check for 'encoded <= 0'
> does not catch the case when 'sbc_encode' tries to return a negative number in
> 'encoded' variable. Later we end up calling 'write' function with a negative
> size for the data block.
>
> Right now, in the case of error 'sbc_encode' function may either return a
> negative number as a return value, or return a negative value in 'encoded'
> variable. But this second type of error is apparently not handled by anything
> other than 'sbcenc' tool at the moment.
>
> Any opinions about how to fix it in the best way? Because it is a flaw in the
> library API, comments from the interested parties are welcome.
In general the API feels a bit weird in that it can return errors in two
different ways. A less obtrusive fix would be to make the variable type
ssize_t instead of size_t. Regarding breaking the SBC library API, I
don't see any big issue with that since it's internal to bluetoothd and
we can easily update the pulseaudio copy accordingly. Would the attached
patch make sense?
Johan
[-- Attachment #2: sbc.patch --]
[-- Type: text/x-diff, Size: 2593 bytes --]
diff --git a/audio/pcm_bluetooth.c b/audio/pcm_bluetooth.c
index cddeda2..4c0ab6f 100644
--- a/audio/pcm_bluetooth.c
+++ b/audio/pcm_bluetooth.c
@@ -1007,7 +1007,7 @@ static snd_pcm_sframes_t bluetooth_a2dp_write(snd_pcm_ioplug_t *io,
snd_pcm_sframes_t ret = 0;
unsigned int bytes_left;
int frame_size, encoded;
- size_t written;
+ ssize_t written;
uint8_t *buff;
DBG("areas->step=%u areas->first=%u offset=%lu size=%lu",
diff --git a/sbc/sbc.c b/sbc/sbc.c
index 569dd7c..fa542e3 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -743,7 +743,7 @@ static int sbc_analyze_audio(struct sbc_encoder_state *state,
* -99 not implemented
*/
-static SBC_ALWAYS_INLINE int sbc_pack_frame_internal(uint8_t *data,
+static SBC_ALWAYS_INLINE ssize_t sbc_pack_frame_internal(uint8_t *data,
struct sbc_frame *frame, size_t len,
int frame_subbands, int frame_channels,
int joint)
@@ -860,7 +860,7 @@ static SBC_ALWAYS_INLINE int sbc_pack_frame_internal(uint8_t *data,
return data_ptr - data;
}
-static int sbc_pack_frame(uint8_t *data, struct sbc_frame *frame, size_t len,
+static ssize_t sbc_pack_frame(uint8_t *data, struct sbc_frame *frame, size_t len,
int joint)
{
if (frame->subbands == 4) {
@@ -1005,10 +1005,10 @@ ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
}
ssize_t sbc_encode(sbc_t *sbc, const void *input, size_t input_len,
- void *output, size_t output_len, size_t *written)
+ void *output, size_t output_len, ssize_t *written)
{
struct sbc_priv *priv;
- int framelen, samples;
+ ssize_t framelen, samples;
int (*sbc_enc_process_input)(int position,
const uint8_t *pcm, int16_t X[2][SBC_X_BUFFER_SIZE],
int nsamples, int nchannels);
diff --git a/sbc/sbc.h b/sbc/sbc.h
index 91422a9..2f830ad 100644
--- a/sbc/sbc.h
+++ b/sbc/sbc.h
@@ -92,7 +92,7 @@ ssize_t sbc_decode(sbc_t *sbc, const void *input, size_t input_len,
/* Encodes ONE input block into ONE output block */
ssize_t sbc_encode(sbc_t *sbc, const void *input, size_t input_len,
- void *output, size_t output_len, size_t *written);
+ void *output, size_t output_len, ssize_t *written);
/* Returns the output block size in bytes */
size_t sbc_get_frame_length(sbc_t *sbc);
diff --git a/sbc/sbcenc.c b/sbc/sbcenc.c
index b5e0541..3d3a7dc 100644
--- a/sbc/sbcenc.c
+++ b/sbc/sbcenc.c
@@ -50,7 +50,7 @@ static void encode(char *filename, int subbands, int bitpool, int joint,
struct au_header au_hdr;
sbc_t sbc;
int fd, size, srate, codesize, nframes;
- size_t encoded;
+ ssize_t encoded;
ssize_t len;
if (sizeof(au_hdr) != 24) {
^ permalink raw reply related
* Re: [PATCH 0/3] SBC encoder optimizations (scale factors)
From: Johan Hedberg @ 2010-06-30 7:32 UTC (permalink / raw)
To: Siarhei Siamashka; +Cc: linux-bluetooth
In-Reply-To: <1277819327-28592-1-git-send-email-siarhei.siamashka@gmail.com>
Hi Siarhei,
On Tue, Jun 29, 2010, Siarhei Siamashka wrote:
> Some patches for making SBC encoding up to 10% faster on ARM and x86.
>
> Also available in git://gitorious.org/system-performance/bluez-sbc.git
> branch 'sbc-fast-scalefactors-for-master'
>
> Siarhei Siamashka (3):
> sbc: new 'sbc_calc_scalefactors_j' function added to sbc primitives
> sbc: MMX optimization for scale factors calculation
> sbc: ARM NEON optimization for scale factors calculation
>
> sbc/sbc.c | 94 ++++++++++++---------------------------------
> sbc/sbc_primitives.c | 75 ++++++++++++++++++++++++++++++++++++
> sbc/sbc_primitives.h | 4 ++
> sbc/sbc_primitives_mmx.c | 54 ++++++++++++++++++++++++++
> sbc/sbc_primitives_neon.c | 58 +++++++++++++++++++++++++++
> 5 files changed, 216 insertions(+), 69 deletions(-)
All three patches have been pushed upstream. Thanks!
Johan
^ permalink raw reply
* [PATCH] Add udev scripts to gitignore.
From: João Paulo Rechi Vita @ 2010-06-30 1:08 UTC (permalink / raw)
To: linux-bluetooth; +Cc: João Paulo Rechi Vita
---
.gitignore | 3 +++
1 files changed, 3 insertions(+), 0 deletions(-)
diff --git a/.gitignore b/.gitignore
index 30bff3b..a6efe98 100644
--- a/.gitignore
+++ b/.gitignore
@@ -81,6 +81,9 @@ compat/dund
compat/hidd
compat/pand
+scripts/97-bluetooth.rules
+scripts/97-bluetooth-hid2hci.rules
+
doc/*.bak
doc/*.stamp
doc/bluez.*
--
1.6.3.3
^ permalink raw reply related
* SBC encoder/decoder API & errors handling
From: Siarhei Siamashka @ 2010-06-29 18:45 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Lennart Poettering
[-- Attachment #1: Type: Text/Plain, Size: 1038 bytes --]
Hello,
When I tried to run some SBC encoder tests a few days ago, I noticed that
there is a regression introduced by commit:
http://git.kernel.org/?p=bluetooth/bluez.git;a=commit;h=c43f8bdcc1d527e2d77481a66217771038be3acd
It is caused by the change from 'int' to 'size_t' for the type of variable
'encoded' in 'sbcenc.c'. After this modification, the check for 'encoded <= 0'
does not catch the case when 'sbc_encode' tries to return a negative number in
'encoded' variable. Later we end up calling 'write' function with a negative
size for the data block.
Right now, in the case of error 'sbc_encode' function may either return a
negative number as a return value, or return a negative value in 'encoded'
variable. But this second type of error is apparently not handled by anything
other than 'sbcenc' tool at the moment.
Any opinions about how to fix it in the best way? Because it is a flaw in the
library API, comments from the interested parties are welcome.
--
Best regards,
Siarhei Siamashka
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply
* [PATCH 3/3] sbc: ARM NEON optimization for scale factors calculation
From: Siarhei Siamashka @ 2010-06-29 13:48 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Siarhei Siamashka
In-Reply-To: <1277819327-28592-1-git-send-email-siarhei.siamashka@gmail.com>
From: Siarhei Siamashka <siarhei.siamashka@nokia.com>
Improves SBC encoding performance when joint stereo is not used.
Benchmarked on ARM Cortex-A8:
== Before: ==
$ time ./sbcenc -b53 -s8 test.au > /dev/null
real 0m4.756s
user 0m4.313s
sys 0m0.438s
samples % image name symbol name
2569 27.6296 sbcenc sbc_pack_frame
1934 20.8002 sbcenc sbc_analyze_4b_8s_neon
1386 14.9064 sbcenc sbc_calculate_bits
1221 13.1319 sbcenc sbc_calc_scalefactors
996 10.7120 sbcenc sbc_enc_process_input_8s_be
878 9.4429 no-vmlinux /no-vmlinux
204 2.1940 sbcenc sbc_encode
56 0.6023 libc-2.10.1.so memcpy
== After: ==
$ time ./sbcenc -b53 -s8 test.au > /dev/null
real 0m4.220s
user 0m3.797s
sys 0m0.422s
samples % image name symbol name
2563 31.3249 sbcenc sbc_pack_frame
1892 23.1239 sbcenc sbc_analyze_4b_8s_neon
1368 16.7196 sbcenc sbc_calculate_bits
961 11.7453 sbcenc sbc_enc_process_input_8s_be
836 10.2176 no-vmlinux /no-vmlinux
262 3.2022 sbcenc sbc_calc_scalefactors_neon
199 2.4322 sbcenc sbc_encode
49 0.5989 libc-2.10.1.so memcpy
---
sbc/sbc.c | 2 +-
sbc/sbc_primitives_neon.c | 58 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 59 insertions(+), 1 deletions(-)
diff --git a/sbc/sbc.c b/sbc/sbc.c
index 7da7313..569dd7c 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -78,7 +78,7 @@ struct sbc_frame {
uint8_t joint;
/* only the lower 4 bits of every element are to be used */
- uint32_t scale_factor[2][8];
+ uint32_t SBC_ALIGNED scale_factor[2][8];
/* raw integer subband samples in the frame */
int32_t SBC_ALIGNED sb_sample_f[16][2][8];
diff --git a/sbc/sbc_primitives_neon.c b/sbc/sbc_primitives_neon.c
index d20eeca..2a4cdf0 100644
--- a/sbc/sbc_primitives_neon.c
+++ b/sbc/sbc_primitives_neon.c
@@ -237,10 +237,68 @@ static inline void sbc_analyze_4b_8s_neon(int16_t *x,
_sbc_analyze_eight_neon(x + 0, out, analysis_consts_fixed8_simd_even);
}
+static void sbc_calc_scalefactors_neon(
+ 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 += 4) {
+ int blk = blocks;
+ int32_t *in = &sb_sample_f[0][ch][sb];
+ asm volatile (
+ "vmov.s32 q0, %[c1]\n"
+ "vmov.s32 q1, %[c1]\n"
+ "1:\n"
+ "vld1.32 {d16, d17}, [%[in], :128], %[inc]\n"
+ "vabs.s32 q8, q8\n"
+ "vld1.32 {d18, d19}, [%[in], :128], %[inc]\n"
+ "vabs.s32 q9, q9\n"
+ "vld1.32 {d20, d21}, [%[in], :128], %[inc]\n"
+ "vabs.s32 q10, q10\n"
+ "vld1.32 {d22, d23}, [%[in], :128], %[inc]\n"
+ "vabs.s32 q11, q11\n"
+ "vcgt.s32 q12, q8, #0\n"
+ "vcgt.s32 q13, q9, #0\n"
+ "vcgt.s32 q14, q10, #0\n"
+ "vcgt.s32 q15, q11, #0\n"
+ "vadd.s32 q8, q8, q12\n"
+ "vadd.s32 q9, q9, q13\n"
+ "vadd.s32 q10, q10, q14\n"
+ "vadd.s32 q11, q11, q15\n"
+ "vorr.s32 q0, q0, q8\n"
+ "vorr.s32 q1, q1, q9\n"
+ "vorr.s32 q0, q0, q10\n"
+ "vorr.s32 q1, q1, q11\n"
+ "subs %[blk], %[blk], #4\n"
+ "bgt 1b\n"
+ "vorr.s32 q0, q0, q1\n"
+ "vmov.s32 q15, %[c2]\n"
+ "vclz.s32 q0, q0\n"
+ "vsub.s32 q0, q15, q0\n"
+ "vst1.32 {d0, d1}, [%[out], :128]\n"
+ :
+ [blk] "+r" (blk),
+ [in] "+r" (in)
+ :
+ [inc] "r" ((char *) &sb_sample_f[1][0][0] -
+ (char *) &sb_sample_f[0][0][0]),
+ [out] "r" (&scale_factor[ch][sb]),
+ [c1] "i" (1 << SCALE_OUT_BITS),
+ [c2] "i" (31 - SCALE_OUT_BITS)
+ : "d0", "d1", "d2", "d3", "d16", "d17", "d18", "d19",
+ "d20", "d21", "d22", "d23", "d24", "d25", "d26",
+ "d27", "d28", "d29", "d30", "d31", "cc", "memory");
+ }
+ }
+}
+
void sbc_init_primitives_neon(struct sbc_encoder_state *state)
{
state->sbc_analyze_4b_4s = sbc_analyze_4b_4s_neon;
state->sbc_analyze_4b_8s = sbc_analyze_4b_8s_neon;
+ state->sbc_calc_scalefactors = sbc_calc_scalefactors_neon;
state->implementation_info = "NEON";
}
--
1.6.4.4
^ permalink raw reply related
* [PATCH 2/3] sbc: MMX optimization for scale factors calculation
From: Siarhei Siamashka @ 2010-06-29 13:48 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Siarhei Siamashka
In-Reply-To: <1277819327-28592-1-git-send-email-siarhei.siamashka@gmail.com>
From: Siarhei Siamashka <siarhei.siamashka@nokia.com>
Improves SBC encoding performance when joint stereo is not used.
Benchmarked on Pentium-M:
== Before: ==
$ time ./sbcenc -b53 -s8 test.au > /dev/null
real 0m1.439s
user 0m1.336s
sys 0m0.104s
samples % image name symbol name
8642 33.7473 sbcenc sbc_pack_frame
5873 22.9342 sbcenc sbc_analyze_4b_8s_mmx
4435 17.3188 sbcenc sbc_calc_scalefactors
4285 16.7331 sbcenc sbc_calculate_bits
1942 7.5836 sbcenc sbc_enc_process_input_8s_be
322 1.2574 sbcenc sbc_encode
== After: ==
$ time ./sbcenc -b53 -s8 test.au > /dev/null
real 0m1.319s
user 0m1.220s
sys 0m0.084s
samples % image name symbol name
8706 37.9959 sbcenc sbc_pack_frame
5740 25.0513 sbcenc sbc_analyze_4b_8s_mmx
4307 18.7972 sbcenc sbc_calculate_bits
1937 8.4537 sbcenc sbc_enc_process_input_8s_be
1801 7.8602 sbcenc sbc_calc_scalefactors_mmx
307 1.3399 sbcenc sbc_encode
---
sbc/sbc_primitives_mmx.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 54 insertions(+), 0 deletions(-)
diff --git a/sbc/sbc_primitives_mmx.c b/sbc/sbc_primitives_mmx.c
index e6900bc..45c62ac 100644
--- a/sbc/sbc_primitives_mmx.c
+++ b/sbc/sbc_primitives_mmx.c
@@ -276,6 +276,59 @@ static inline void sbc_analyze_4b_8s_mmx(int16_t *x, int32_t *out,
asm volatile ("emms\n");
}
+static void sbc_calc_scalefactors_mmx(
+ int32_t sb_sample_f[16][2][8],
+ uint32_t scale_factor[2][8],
+ int blocks, int channels, int subbands)
+{
+ static const SBC_ALIGNED int32_t consts[2] = {
+ 1 << SCALE_OUT_BITS,
+ 1 << SCALE_OUT_BITS,
+ };
+ int ch, sb;
+ intptr_t blk;
+ for (ch = 0; ch < channels; ch++) {
+ for (sb = 0; sb < subbands; sb += 2) {
+ blk = (blocks - 1) * (((char *) &sb_sample_f[1][0][0] -
+ (char *) &sb_sample_f[0][0][0]));
+ asm volatile (
+ "movq (%4), %%mm0\n"
+ "1:\n"
+ "movq (%1, %0), %%mm1\n"
+ "pxor %%mm2, %%mm2\n"
+ "pcmpgtd %%mm2, %%mm1\n"
+ "paddd (%1, %0), %%mm1\n"
+ "pcmpgtd %%mm1, %%mm2\n"
+ "pxor %%mm2, %%mm1\n"
+
+ "por %%mm1, %%mm0\n"
+
+ "sub %2, %0\n"
+ "jns 1b\n"
+
+ "movd %%mm0, %k0\n"
+ "psrlq $32, %%mm0\n"
+ "bsrl %k0, %k0\n"
+ "subl %5, %k0\n"
+ "movl %k0, (%3)\n"
+
+ "movd %%mm0, %k0\n"
+ "bsrl %k0, %k0\n"
+ "subl %5, %k0\n"
+ "movl %k0, 4(%3)\n"
+ : "+r" (blk)
+ : "r" (&sb_sample_f[0][ch][sb]),
+ "i" ((char *) &sb_sample_f[1][0][0] -
+ (char *) &sb_sample_f[0][0][0]),
+ "r" (&scale_factor[ch][sb]),
+ "r" (&consts),
+ "i" (SCALE_OUT_BITS)
+ : "memory");
+ }
+ }
+ asm volatile ("emms\n");
+}
+
static int check_mmx_support(void)
{
#ifdef __amd64__
@@ -314,6 +367,7 @@ void sbc_init_primitives_mmx(struct sbc_encoder_state *state)
if (check_mmx_support()) {
state->sbc_analyze_4b_4s = sbc_analyze_4b_4s_mmx;
state->sbc_analyze_4b_8s = sbc_analyze_4b_8s_mmx;
+ state->sbc_calc_scalefactors = sbc_calc_scalefactors_mmx;
state->implementation_info = "MMX";
}
}
--
1.6.4.4
^ permalink raw reply related
* [PATCH 1/3] sbc: new 'sbc_calc_scalefactors_j' function added to sbc primitives
From: Siarhei Siamashka @ 2010-06-29 13:48 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Siarhei Siamashka
In-Reply-To: <1277819327-28592-1-git-send-email-siarhei.siamashka@gmail.com>
From: Siarhei Siamashka <siarhei.siamashka@nokia.com>
The code for scale factors calculation with joint stereo support has
been moved to a separate function. It can get platform-specific
SIMD optimizations later for best possible performance.
But even this change in C code improves performance because of the
use of __builtin_clz() instead of loops similar to what was done
to sbc_calc_scalefactors earlier. Also technically it does loop
unrolling by processing two channels at once, which might be either
good or bad for performance (if the registers pressure is increased
and more data is spilled to memory). But the benchmark from 32-bit
x86 system (pentium-m) shows that it got clearly faster:
$ time ./sbcenc.old -b53 -s8 -j test.au > /dev/null
real 0m1.868s
user 0m1.808s
sys 0m0.048s
$ time ./sbcenc.new -b53 -s8 -j test.au > /dev/null
real 0m1.742s
user 0m1.668s
sys 0m0.064s
---
sbc/sbc.c | 92 +++++++++++++-------------------------------------
sbc/sbc_primitives.c | 75 ++++++++++++++++++++++++++++++++++++++++
sbc/sbc_primitives.h | 4 ++
3 files changed, 103 insertions(+), 68 deletions(-)
diff --git a/sbc/sbc.c b/sbc/sbc.c
index 86399dd..7da7313 100644
--- a/sbc/sbc.c
+++ b/sbc/sbc.c
@@ -745,7 +745,8 @@ static int sbc_analyze_audio(struct sbc_encoder_state *state,
static SBC_ALWAYS_INLINE int sbc_pack_frame_internal(uint8_t *data,
struct sbc_frame *frame, size_t len,
- int frame_subbands, int frame_channels)
+ int frame_subbands, int frame_channels,
+ int joint)
{
/* Bitstream writer starts from the fourth byte */
uint8_t *data_ptr = data + 4;
@@ -802,63 +803,6 @@ static SBC_ALWAYS_INLINE int sbc_pack_frame_internal(uint8_t *data,
crc_pos = 16;
if (frame->mode == JOINT_STEREO) {
- /* like frame->sb_sample but joint stereo */
- int32_t sb_sample_j[16][2];
- /* scalefactor and scale_factor in joint case */
- uint32_t scalefactor_j[2];
- uint8_t scale_factor_j[2];
-
- uint8_t joint = 0;
- frame->joint = 0;
-
- for (sb = 0; sb < frame_subbands - 1; sb++) {
- scale_factor_j[0] = 0;
- scalefactor_j[0] = 2 << SCALE_OUT_BITS;
- scale_factor_j[1] = 0;
- scalefactor_j[1] = 2 << SCALE_OUT_BITS;
-
- for (blk = 0; blk < frame->blocks; blk++) {
- uint32_t tmp;
- /* Calculate joint stereo signal */
- sb_sample_j[blk][0] =
- ASR(frame->sb_sample_f[blk][0][sb], 1) +
- ASR(frame->sb_sample_f[blk][1][sb], 1);
- sb_sample_j[blk][1] =
- ASR(frame->sb_sample_f[blk][0][sb], 1) -
- ASR(frame->sb_sample_f[blk][1][sb], 1);
-
- /* calculate scale_factor_j and scalefactor_j for joint case */
- tmp = fabs(sb_sample_j[blk][0]);
- while (scalefactor_j[0] < tmp) {
- scale_factor_j[0]++;
- scalefactor_j[0] *= 2;
- }
- tmp = fabs(sb_sample_j[blk][1]);
- while (scalefactor_j[1] < tmp) {
- scale_factor_j[1]++;
- scalefactor_j[1] *= 2;
- }
- }
-
- /* decide whether to join this subband */
- if ((frame->scale_factor[0][sb] +
- frame->scale_factor[1][sb]) >
- (scale_factor_j[0] +
- scale_factor_j[1])) {
- /* use joint stereo for this subband */
- joint |= 1 << (frame_subbands - 1 - sb);
- frame->joint |= 1 << sb;
- frame->scale_factor[0][sb] = scale_factor_j[0];
- frame->scale_factor[1][sb] = scale_factor_j[1];
- for (blk = 0; blk < frame->blocks; blk++) {
- frame->sb_sample_f[blk][0][sb] =
- sb_sample_j[blk][0];
- frame->sb_sample_f[blk][1][sb] =
- sb_sample_j[blk][1];
- }
- }
- }
-
PUT_BITS(data_ptr, bits_cache, bits_count,
joint, frame_subbands);
crc_header[crc_pos >> 3] = joint;
@@ -916,18 +860,23 @@ static SBC_ALWAYS_INLINE int sbc_pack_frame_internal(uint8_t *data,
return data_ptr - data;
}
-static int sbc_pack_frame(uint8_t *data, struct sbc_frame *frame, size_t len)
+static int sbc_pack_frame(uint8_t *data, struct sbc_frame *frame, size_t len,
+ int joint)
{
if (frame->subbands == 4) {
if (frame->channels == 1)
- return sbc_pack_frame_internal(data, frame, len, 4, 1);
+ return sbc_pack_frame_internal(
+ data, frame, len, 4, 1, joint);
else
- return sbc_pack_frame_internal(data, frame, len, 4, 2);
+ return sbc_pack_frame_internal(
+ data, frame, len, 4, 2, joint);
} else {
if (frame->channels == 1)
- return sbc_pack_frame_internal(data, frame, len, 8, 1);
+ return sbc_pack_frame_internal(
+ data, frame, len, 8, 1, joint);
else
- return sbc_pack_frame_internal(data, frame, len, 8, 2);
+ return sbc_pack_frame_internal(
+ data, frame, len, 8, 2, joint);
}
}
@@ -1121,11 +1070,18 @@ ssize_t sbc_encode(sbc_t *sbc, const void *input, size_t input_len,
samples = sbc_analyze_audio(&priv->enc_state, &priv->frame);
- priv->enc_state.sbc_calc_scalefactors(
- priv->frame.sb_sample_f, priv->frame.scale_factor,
- priv->frame.blocks, priv->frame.channels, priv->frame.subbands);
-
- framelen = sbc_pack_frame(output, &priv->frame, output_len);
+ if (priv->frame.mode == JOINT_STEREO) {
+ int j = priv->enc_state.sbc_calc_scalefactors_j(
+ priv->frame.sb_sample_f, priv->frame.scale_factor,
+ priv->frame.blocks, priv->frame.subbands);
+ framelen = sbc_pack_frame(output, &priv->frame, output_len, j);
+ } else {
+ priv->enc_state.sbc_calc_scalefactors(
+ priv->frame.sb_sample_f, priv->frame.scale_factor,
+ priv->frame.blocks, priv->frame.channels,
+ priv->frame.subbands);
+ framelen = sbc_pack_frame(output, &priv->frame, output_len, 0);
+ }
if (written)
*written = framelen;
diff --git a/sbc/sbc_primitives.c b/sbc/sbc_primitives.c
index 41934d3..c73fb1c 100644
--- a/sbc/sbc_primitives.c
+++ b/sbc/sbc_primitives.c
@@ -440,6 +440,80 @@ static void sbc_calc_scalefactors(
}
}
+static int sbc_calc_scalefactors_j(
+ int32_t sb_sample_f[16][2][8],
+ uint32_t scale_factor[2][8],
+ int blocks, int subbands)
+{
+ int blk, joint = 0;
+ int32_t tmp0, tmp1;
+ uint32_t x, y;
+
+ /* last subband does not use joint stereo */
+ int sb = subbands - 1;
+ x = 1 << SCALE_OUT_BITS;
+ y = 1 << SCALE_OUT_BITS;
+ for (blk = 0; blk < blocks; blk++) {
+ tmp0 = fabs(sb_sample_f[blk][0][sb]);
+ tmp1 = fabs(sb_sample_f[blk][1][sb]);
+ if (tmp0 != 0)
+ x |= tmp0 - 1;
+ if (tmp1 != 0)
+ y |= tmp1 - 1;
+ }
+ scale_factor[0][sb] = (31 - SCALE_OUT_BITS) - sbc_clz(x);
+ scale_factor[1][sb] = (31 - SCALE_OUT_BITS) - sbc_clz(y);
+
+ /* the rest of subbands can use joint stereo */
+ while (--sb >= 0) {
+ int32_t sb_sample_j[16][2];
+ x = 1 << SCALE_OUT_BITS;
+ y = 1 << SCALE_OUT_BITS;
+ for (blk = 0; blk < blocks; blk++) {
+ tmp0 = sb_sample_f[blk][0][sb];
+ tmp1 = sb_sample_f[blk][1][sb];
+ sb_sample_j[blk][0] = ASR(tmp0, 1) + ASR(tmp1, 1);
+ sb_sample_j[blk][1] = ASR(tmp0, 1) - ASR(tmp1, 1);
+ tmp0 = fabs(tmp0);
+ tmp1 = fabs(tmp1);
+ if (tmp0 != 0)
+ x |= tmp0 - 1;
+ if (tmp1 != 0)
+ y |= tmp1 - 1;
+ }
+ scale_factor[0][sb] = (31 - SCALE_OUT_BITS) -
+ sbc_clz(x);
+ scale_factor[1][sb] = (31 - SCALE_OUT_BITS) -
+ sbc_clz(y);
+ x = 1 << SCALE_OUT_BITS;
+ y = 1 << SCALE_OUT_BITS;
+ for (blk = 0; blk < blocks; blk++) {
+ tmp0 = fabs(sb_sample_j[blk][0]);
+ tmp1 = fabs(sb_sample_j[blk][1]);
+ if (tmp0 != 0)
+ x |= tmp0 - 1;
+ if (tmp1 != 0)
+ y |= tmp1 - 1;
+ }
+ x = (31 - SCALE_OUT_BITS) - sbc_clz(x);
+ y = (31 - SCALE_OUT_BITS) - sbc_clz(y);
+
+ /* decide whether to use joint stereo for this subband */
+ if ((scale_factor[0][sb] + scale_factor[1][sb]) > x + y) {
+ joint |= 1 << (subbands - 1 - sb);
+ scale_factor[0][sb] = x;
+ scale_factor[1][sb] = y;
+ for (blk = 0; blk < blocks; blk++) {
+ sb_sample_f[blk][0][sb] = sb_sample_j[blk][0];
+ sb_sample_f[blk][1][sb] = sb_sample_j[blk][1];
+ }
+ }
+ }
+
+ /* bitmask with the information about subbands using joint stereo */
+ return joint;
+}
+
/*
* Detect CPU features and setup function pointers
*/
@@ -457,6 +531,7 @@ void sbc_init_primitives(struct sbc_encoder_state *state)
/* Default implementation for scale factors calculation */
state->sbc_calc_scalefactors = sbc_calc_scalefactors;
+ state->sbc_calc_scalefactors_j = sbc_calc_scalefactors_j;
state->implementation_info = "Generic C";
/* X86/AMD64 optimizations */
diff --git a/sbc/sbc_primitives.h b/sbc/sbc_primitives.h
index 3be02ed..3fec8d5 100644
--- a/sbc/sbc_primitives.h
+++ b/sbc/sbc_primitives.h
@@ -63,6 +63,10 @@ struct sbc_encoder_state {
void (*sbc_calc_scalefactors)(int32_t sb_sample_f[16][2][8],
uint32_t scale_factor[2][8],
int blocks, int channels, int subbands);
+ /* Scale factors calculation with joint stereo support */
+ int (*sbc_calc_scalefactors_j)(int32_t sb_sample_f[16][2][8],
+ uint32_t scale_factor[2][8],
+ int blocks, int subbands);
const char *implementation_info;
};
--
1.6.4.4
^ permalink raw reply related
* [PATCH 0/3] SBC encoder optimizations (scale factors)
From: Siarhei Siamashka @ 2010-06-29 13:48 UTC (permalink / raw)
To: linux-bluetooth; +Cc: Siarhei Siamashka
Some patches for making SBC encoding up to 10% faster on ARM and x86.
Also available in git://gitorious.org/system-performance/bluez-sbc.git
branch 'sbc-fast-scalefactors-for-master'
Siarhei Siamashka (3):
sbc: new 'sbc_calc_scalefactors_j' function added to sbc primitives
sbc: MMX optimization for scale factors calculation
sbc: ARM NEON optimization for scale factors calculation
sbc/sbc.c | 94 ++++++++++++---------------------------------
sbc/sbc_primitives.c | 75 ++++++++++++++++++++++++++++++++++++
sbc/sbc_primitives.h | 4 ++
sbc/sbc_primitives_mmx.c | 54 ++++++++++++++++++++++++++
sbc/sbc_primitives_neon.c | 58 +++++++++++++++++++++++++++
5 files changed, 216 insertions(+), 69 deletions(-)
^ permalink raw reply
* Re: [PATCH] Bluetooth: Reset the security level after an authentication failure
From: Andrei Emeltchenko @ 2010-06-29 12:58 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: johan.hedberg, linux-bluetooth, Johan Hedberg
In-Reply-To: <20100620040807.GA32372@vigoh>
On Sun, Jun 20, 2010 at 7:08 AM, Gustavo F. Padovan <gustavo@padovan.org> wrote:
> Hi Johan,
>
> * johan.hedberg@gmail.com <johan.hedberg@gmail.com> [2010-06-18 11:08:56 +0300]:
>
>> From: Johan Hedberg <johan.hedberg@nokia.com>
>>
>> When authentication fails for a connection the assumed security level
>> should be set back to BT_SECURITY_LOW so that subsequent connect
>> attempts over the same link don't falsely assume that security is
>> adequate enough.
>>
>> Signed-off-by: Johan Hedberg <johan.hedberg@nokia.com>
>> ---
>> net/bluetooth/hci_event.c | 2 ++
>> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> It works fine to me.
We have tested this patch.
>
> Acked-by: Gustavo F. Padovan <padovan@profusion.mobi>
Reviewed-by: Emeltchenko Andrei <andrei.emeltchenko@nokia.com>
Regards,
Andrei Emeltchenko
^ permalink raw reply
* Re: [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn
From: Andrei Emeltchenko @ 2010-06-29 12:48 UTC (permalink / raw)
To: Gustavo F. Padovan; +Cc: Ville Tervo, linux-bluetooth
In-Reply-To: <20100628160751.GA5557@vigoh>
Hi Gustavo,
On Mon, Jun 28, 2010 at 7:07 PM, Gustavo F. Padovan <gustavo@padovan.org> wrote:
> Hi Andrei,
>
> * Ville Tervo <ville.tervo@iki.fi> [2010-05-28 10:14:31 +0300]:
>
>> Hi,
>>
>> On Sun, May 23, 2010 at 5:39 AM, Gustavo F. Padovan <gustavo@padovan.org> wrote:
>>
>> > Using the sock timer like you are you using looks too hackish, there are
>> > kernel struct for such defer works. I still prefer the first solution,
>> > that avoids the call to l2cap_chan_del() only.
>> > But we have to solve the problem with the sock_kill() call, I'm
>> > wondering if add a check inside l2cap_sock_kill is good idea. So we
>> > check if the socket is owned by user and if yes, we just return, however
>> > may have problem with socket refcnt doing that.
>> >
>> > Looking to the rfcomm code I found something that could be cause of the
>> > problem, there isn't any sock_hold() in the rfcomm code, maybe is it
>> > missing? Nevertheless it does the sock_put() without call sock_hold().
>> >
>> > Like you I'm trying to figure out how to fix this issue, I don't know
>> > yet how to fix it properly. I advice to take a look on the rfcomm code
>> > and check if we really are missing a sock_hold() there.
>>
>> Wouldn't backlogging of destructive operations (l2cap disc rsp and
>> req) solve these issues? All operations cannot be backlogged since
>> they cannot mapped to certain sock.
>
> I've been thinking about this issue and now I agree that backlog them is
> the better solution. We're already using backlog queue for ERTM, so you
> will have to extend it only. Check the branch for-next of my tree to see
> how I did that.
IMO The case we are talking about is different. Personally I do not
think we need to backlog
packets for removed connection. Those packets will be discarded anyway
since there will
be no connection and this operation will consume CPU just for nothing.
Regards,
Andrei
^ permalink raw reply
* Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE
From: Andrei Emeltchenko @ 2010-06-29 12:40 UTC (permalink / raw)
To: David Miller
Cc: ospite, alan, marcel, jkosina, mdpoole, hadess, eric.dumazet,
linux-bluetooth, linux-kernel, netdev
In-Reply-To: <20100629.001216.91341775.davem@davemloft.net>
Hi,
On Tue, Jun 29, 2010 at 10:12 AM, David Miller <davem@davemloft.net> wrote:
> From: Antonio Ospite <ospite@studenti.unina.it>
> Date: Mon, 28 Jun 2010 13:14:37 +0200
>
>> On Sun, 13 Jun 2010 18:20:01 -0400
>> Alan Ott <alan@signal11.us> wrote:
>>
>>> This patch adds support or getting and setting feature reports for blue=
tooth
>>> HID devices from HIDRAW.
>>>
>>> Signed-off-by: Alan Ott <alan@signal11.us>
>>> ---
>>
>> Ping.
>
> We effectively don't have a bluetooth maintainer at the current point
> in time. =A0I've tried to let patches sit for a while hoping the listed
> maintainer would do something, at least occaisionally, but that simply
> isn't happening.
> So I'll just pick patches up directly as I find time to review them,
> but I have to warn that for me it's going to be done in a very low
> priority way because I really don't find bluetooth all that exciting. :-)
This would be good. We have a backlog of bluetooth kernel patches
waiting for several months.
This takes too much time to return to them again and again...
Please keep this ML informed.
Regards,
Andrei Emeltchenko
^ permalink raw reply
* Re: [PATCH v3] audio: fix memory leak with typefinding
From: Johan Hedberg @ 2010-06-29 12:04 UTC (permalink / raw)
To: harri.mahonen; +Cc: linux-bluetooth
In-Reply-To: <1277391672-10563-1-git-send-email-harri.mahonen@gmail.com>
Hi Harri,
On Thu, Jun 24, 2010, harri.mahonen@gmail.com wrote:
> From: Harri Mahonen <harri.mahonen@gmail.com>
>
> Each time sbc_typefind is called and there is no syncword, sbc_finalize
> never gets called. Instead of hardcoded syncword check, rely on
> sbc_parse to do it properly.
> ---
> audio/gstbluetooth.c | 16 ++++++++++------
> 1 files changed, 10 insertions(+), 6 deletions(-)
This patch has now been pushed upstream. Thanks.
Johan
^ permalink raw reply
* Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE
From: Johan Hedberg @ 2010-06-29 9:07 UTC (permalink / raw)
To: Jiri Kosina
Cc: David Miller, ospite, alan, marcel, mdpoole, hadess, eric.dumazet,
linux-bluetooth, linux-kernel, netdev
In-Reply-To: <alpine.LNX.2.00.1006291047560.13809@pobox.suse.cz>
Hi,
On Tue, Jun 29, 2010, Jiri Kosina wrote:
> Frankly, I don't understand what exactly the current situation with
> in-kernel bluetooth stack is anyway.
>
> What is the relation between what we have in net/bluetooth and the tree at
> [1], which seems to be quite actively developed?
The difference is that the userspace part has essentially two
maintainers (me and Marcel) whereas the kernel side only has one
(Marcel). So even when Marcel is inactive I can at least take care of
the userspace side. I'd volunteer for the kernel also but I don't really
have any experience there (only 3-4 patches so far).
Johan
^ permalink raw reply
* Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE
From: Jiri Kosina @ 2010-06-29 8:50 UTC (permalink / raw)
To: David Miller
Cc: ospite, alan, marcel, mdpoole, hadess, eric.dumazet,
linux-bluetooth, linux-kernel, netdev
In-Reply-To: <20100629.001216.91341775.davem@davemloft.net>
On Tue, 29 Jun 2010, David Miller wrote:
> >> This patch adds support or getting and setting feature reports for bluetooth
> >> HID devices from HIDRAW.
> >>
> >> Signed-off-by: Alan Ott <alan@signal11.us>
> >> ---
> >
> > Ping.
>
> We effectively don't have a bluetooth maintainer at the current point in
> time. I've tried to let patches sit for a while hoping the listed
> maintainer would do something, at least occaisionally, but that simply
> isn't happening.
Frankly, I don't understand what exactly the current situation with
in-kernel bluetooth stack is anyway.
What is the relation between what we have in net/bluetooth and the tree at
[1], which seems to be quite actively developed?
> So I'll just pick patches up directly as I find time to review them, but
> I have to warn that for me it's going to be done in a very low priority
> way because I really don't find bluetooth all that exciting. :-)
If needed, I can at least take over the net/bluetooth/hidp part, as I
maintain the rest of the HID code anyway.
[1] http://git.kernel.org/?p=bluetooth/bluez.git;a=summary
--
Jiri Kosina
SUSE Labs, Novell Inc.
^ permalink raw reply
* Re: [PATCH 1/1] Bluetooth: hidp: Add support for hidraw HIDIOCGFEATURE and HIDIOCSFEATURE
From: David Miller @ 2010-06-29 7:12 UTC (permalink / raw)
To: ospite
Cc: alan, marcel, jkosina, mdpoole, hadess, eric.dumazet,
linux-bluetooth, linux-kernel, netdev
In-Reply-To: <20100628131437.bde782b6.ospite@studenti.unina.it>
From: Antonio Ospite <ospite@studenti.unina.it>
Date: Mon, 28 Jun 2010 13:14:37 +0200
> On Sun, 13 Jun 2010 18:20:01 -0400
> Alan Ott <alan@signal11.us> wrote:
>
>> This patch adds support or getting and setting feature reports for bluetooth
>> HID devices from HIDRAW.
>>
>> Signed-off-by: Alan Ott <alan@signal11.us>
>> ---
>
> Ping.
We effectively don't have a bluetooth maintainer at the current point
in time. I've tried to let patches sit for a while hoping the listed
maintainer would do something, at least occaisionally, but that simply
isn't happening.
So I'll just pick patches up directly as I find time to review them,
but I have to warn that for me it's going to be done in a very low
priority way because I really don't find bluetooth all that exciting. :-)
^ permalink raw reply
* Re: [PATCH] Fix unblock removed device
From: Johan Hedberg @ 2010-06-28 22:27 UTC (permalink / raw)
To: Daniel Örstadius; +Cc: linux-bluetooth
In-Reply-To: <AANLkTil1_LwZYuTuZT0gQgd7Mxjo0CoY2WVI8CW-f-wj@mail.gmail.com>
Hi Daniel,
On Mon, Jun 28, 2010, Daniel Örstadius wrote:
> This patch unblocks a device when removing it. Adds boolean argument
> to function device_unblock to only emit property changed signal and
> probe drivers when unblocking a device if the device is not unblocked
> as part of being removed.
Thanks for the patch. It has now been pushed upstream with a couple of
over 80 character line fixes. Please pay attention to that in the
future.
Johan
^ permalink raw reply
* Re: [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined
From: Justin P. Mattock @ 2010-06-28 17:56 UTC (permalink / raw)
To: David Howells
Cc: Gustavo F. Padovan, linux-kernel, sds, lenb, linux-bluetooth
In-Reply-To: <7323.1277729576@redhat.com>
On 06/28/2010 05:52 AM, David Howells wrote:
> Justin P. Mattock<justinmattock@gmail.com> wrote:
>
>> - BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
>> - bcsp->msgq_txseq = ++(bcsp->msgq_txseq)& 0x07;
>> + BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq | ret);
>> + ret = ++(bcsp->msgq_txseq)& 0x07;
>
> I don't know what you're trying to do here, but you seem to be trying to send
> the computed value back in time.
>
I was under the impression that hdr[0] |= 0x80 + bcsp->msgq_txseq;
is computing a value for BT_DBG then ret = ++(bcsp->msgq_txseq)& 0x07
computes a value as well i.e.
BT_DBG("Sending packet with seqno %u", hdr[0] | ret);
> The problem is that the compiler is confused about why a '++' operator makes
> any sense here. It doesn't. It should be a '+ 1' instead. I think what you
> want is:
>
> - bcsp->msgq_txseq = ++(bcsp->msgq_txseq)& 0x07;
> + bcsp->msgq_txseq = (bcsp->msgq_txseq + 1)& 0x07;
>
> David
>
yeah I did play around with the ++ and noticed the compiler would be
satisfied if the ++ was not there, but didn't think to just add a + 1
Justin P. Mattock
^ permalink raw reply
* Re: [PATCH] Bluetooth: Fix abuse of the preincrement operator
From: Justin P. Mattock @ 2010-06-28 17:44 UTC (permalink / raw)
To: David Howells; +Cc: linux-bluetooth, linux-kernel, gustavo
In-Reply-To: <20100628125752.8945.50572.stgit@warthog.procyon.org.uk>
On 06/28/2010 05:57 AM, David Howells wrote:
> Fix abuse of the preincrement operator as detected when building with gcc
> 4.6.0:
>
> CC [M] drivers/bluetooth/hci_bcsp.o
> drivers/bluetooth/hci_bcsp.c: In function 'bcsp_prepare_pkt':
> drivers/bluetooth/hci_bcsp.c:247:20: warning: operation on 'bcsp->msgq_txseq' may be undefined
>
> Reported-by: Justin P. Mattock<justinmattock@gmail.com>
> Signed-off-by: David Howells<dhowells@redhat.com>
> ---
>
> drivers/bluetooth/hci_bcsp.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
> index 40aec0f..42d69d4 100644
> --- a/drivers/bluetooth/hci_bcsp.c
> +++ b/drivers/bluetooth/hci_bcsp.c
> @@ -244,7 +244,7 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
> if (rel) {
> hdr[0] |= 0x80 + bcsp->msgq_txseq;
> BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
> - bcsp->msgq_txseq = ++(bcsp->msgq_txseq)& 0x07;
> + bcsp->msgq_txseq = (bcsp->msgq_txseq + 1)& 0x07;
> }
>
> if (bcsp->use_crc)
>
>
ahh.. so it's o.k. to add the value after bcsp->msgq_txseq instead of
before. Anyways build clean over here..
Thanks!
Justin P. Mattock
^ permalink raw reply
* Re: [PATCHv2 1/2] Bluetooth: Check sk is not owned before freeing l2cap_conn
From: Gustavo F. Padovan @ 2010-06-28 16:07 UTC (permalink / raw)
To: Emeltchenko Andrei; +Cc: Ville Tervo, linux-bluetooth
In-Reply-To: <AANLkTilQoPjrNgqjtp7xeQF-yzrE6KNgDdagEXrIIGIv@mail.gmail.com>
Hi Andrei,
* Ville Tervo <ville.tervo@iki.fi> [2010-05-28 10:14:31 +0300]:
> Hi,
>
> On Sun, May 23, 2010 at 5:39 AM, Gustavo F. Padovan <gustavo@padovan.org> wrote:
>
> > Using the sock timer like you are you using looks too hackish, there are
> > kernel struct for such defer works. I still prefer the first solution,
> > that avoids the call to l2cap_chan_del() only.
> > But we have to solve the problem with the sock_kill() call, I'm
> > wondering if add a check inside l2cap_sock_kill is good idea. So we
> > check if the socket is owned by user and if yes, we just return, however
> > may have problem with socket refcnt doing that.
> >
> > Looking to the rfcomm code I found something that could be cause of the
> > problem, there isn't any sock_hold() in the rfcomm code, maybe is it
> > missing? Nevertheless it does the sock_put() without call sock_hold().
> >
> > Like you I'm trying to figure out how to fix this issue, I don't know
> > yet how to fix it properly. I advice to take a look on the rfcomm code
> > and check if we really are missing a sock_hold() there.
>
> Wouldn't backlogging of destructive operations (l2cap disc rsp and
> req) solve these issues? All operations cannot be backlogged since
> they cannot mapped to certain sock.
I've been thinking about this issue and now I agree that backlog them is
the better solution. We're already using backlog queue for ERTM, so you
will have to extend it only. Check the branch for-next of my tree to see
how I did that.
--
Gustavo F. Padovan
http://padovan.org
^ permalink raw reply
* Re: obexd: cannot object-push certain files (large, w/ logs)
From: clemens fischer @ 2010-06-28 13:45 UTC (permalink / raw)
To: linux-bluetooth
In-Reply-To: <AANLkTikrwAgP4bi0fkWWd-mqn_HFmfCWPZkQLe0rWttY@mail.gmail.com>
Hi Luiz Augusto and shy, thanks for your info.
> Luiz Augusto von Dentz:
> You can clearly see that obexd respond the connection request with 200
> (ok) and after that there is no request from the remote device just a
> clear disconnect:
>
> 16:58:37 < ACL data: handle 38 flags 0x02 dlen 15
> 16:58:37 L2CAP(d): cid 0x012b len 11 [psm 3]
> 16:58:37 RFCOMM(d): UIH: cr 0 dlci 18 pf 0 ilen 7 fcs 0x14
> 16:58:37 OBEX: Connect rsp(f): status 200 len 7 version 1.0
> flags 0 mtu 32767
> 16:58:37 Status 200 = Success
> 16:58:37 > HCI Event: Number of Completed Packets (0x13) plen 5
> 16:58:37 handle 38 packets 1
> 16:58:37 > HCI Event: Number of Completed Packets (0x13) plen 5
> 16:58:37 handle 38 packets 1
> 16:58:37 > ACL data: handle 38 flags 0x02 dlen 8
> 16:58:37 L2CAP(d): cid 0x0041 len 4 [psm 3]
> 16:58:37 RFCOMM(s): DISC: cr 1 dlci 18 pf 1 ilen 0 fcs 0xd3
> Have you tried transferring the same file with any other phone/stack?
> This one seems very unlikely to be a obexd bug since there is not even
> a request to transfer anything and as the connection itself seems ok,
> no missing agent apparently, I don't see anything wrong here.
This is interesting. There are files of any size that can reliably be
transfered and others that cannot, also reliably. Your interpretation
of the packet trace seems to indicate that the nokia-3650 firmware
selects between the two kinds. I'm glad that I can trust obexd!
I have no other phone, but there's an application letting me at least
copy those files to a MMC card. The only trouble is that I have to
remove both battery and MMC card from the phone, put it into
a card-reader on the PC, copy any files over and then reassemble that
phone. The phone didn't come with a serial cable, the nokia software on
the CD is window$-only and the linux tools (eg. gnooki, gammu, but also
a number of applets) either require large dependencies or a GNOME/KDE
desktop. But I'm a person with simple needs. I need backups of the
contacts, calendar and notes on the phone. So far I could not find out
the format of the file Backup/Backup.arc which is the reason for sending
or copying certain files one by one.
I bet you guys have special setups with command-line tools in scripts
for testing the differing scenarios a bluetooth device can be used in.
The most basic chain of commands to get push-obex to work was
('00:XX:XX:XX:XX:XX' being the bluetooth address of the phone):
bluez-simple-agent hci0 '00:XX:XX:XX:XX:XX' (starts bluetoothd)
hciconfig putkey '00:XX:XX:XX:XX:XX' (optional)
/l/libexec/obexd -nd -r /tmp/ -a -o
This took me days to find out. Simple commands like this categorized by
purpose might be candidates for a FAQ. Do you have tips where to look
for CLI commands like this? What do you use for testing?
regards, clemens fischer
^ permalink raw reply
* Re: [PATCH] Bluetooth: Fix abuse of the preincrement operator
From: Gustavo F. Padovan @ 2010-06-28 13:12 UTC (permalink / raw)
To: David Howells; +Cc: linux-bluetooth, justinmattock, linux-kernel
In-Reply-To: <20100628125752.8945.50572.stgit@warthog.procyon.org.uk>
Hi David,
* David Howells <dhowells@redhat.com> [2010-06-28 13:57:52 +0100]:
> Fix abuse of the preincrement operator as detected when building with gcc
> 4.6.0:
>
> CC [M] drivers/bluetooth/hci_bcsp.o
> drivers/bluetooth/hci_bcsp.c: In function 'bcsp_prepare_pkt':
> drivers/bluetooth/hci_bcsp.c:247:20: warning: operation on 'bcsp->msgq_txseq' may be undefined
>
> Reported-by: Justin P. Mattock <justinmattock@gmail.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
> drivers/bluetooth/hci_bcsp.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/bluetooth/hci_bcsp.c b/drivers/bluetooth/hci_bcsp.c
> index 40aec0f..42d69d4 100644
> --- a/drivers/bluetooth/hci_bcsp.c
> +++ b/drivers/bluetooth/hci_bcsp.c
> @@ -244,7 +244,7 @@ static struct sk_buff *bcsp_prepare_pkt(struct bcsp_struct *bcsp, u8 *data,
> if (rel) {
> hdr[0] |= 0x80 + bcsp->msgq_txseq;
> BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
> - bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
> + bcsp->msgq_txseq = (bcsp->msgq_txseq + 1) & 0x07;
> }
>
> if (bcsp->use_crc)
>
Acked-by: Gustavo F. Padovan <padovan@profusion.mobi>
--
Gustavo F. Padovan
http://padovan.org
^ permalink raw reply
* Re: [PATCH 5/5]bluetooth:hci_bcsp Fix operation on 'bcsp->msgq_txseq' may be undefined
From: Bernd Petrovitsch @ 2010-06-28 13:02 UTC (permalink / raw)
To: David Howells
Cc: Justin P. Mattock, Gustavo F. Padovan, linux-kernel, sds, lenb,
linux-bluetooth
In-Reply-To: <7323.1277729576@redhat.com>
On Mon, 2010-06-28 at 13:52 +0100, David Howells wrote:
> Justin P. Mattock <justinmattock@gmail.com> wrote:
>
> > - BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq);
> > - bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
> > + BT_DBG("Sending packet with seqno %u", bcsp->msgq_txseq | ret);
> > + ret = ++(bcsp->msgq_txseq) & 0x07;
>
> I don't know what you're trying to do here, but you seem to be trying to send
> the computed value back in time.
>
> The problem is that the compiler is confused about why a '++' operator makes
It's even worse as that expression is explicitly undefined (and should
be fixed anyways and unconditionally).
> any sense here. It doesn't. It should be a '+ 1' instead. I think what you
> want is:
>
> - bcsp->msgq_txseq = ++(bcsp->msgq_txseq) & 0x07;
> + bcsp->msgq_txseq = (bcsp->msgq_txseq + 1) & 0x07;
Yes, that's looks like the most probable intention of it - at least for
one who doesn't know the bluetooth code.
Bernd
--
Bernd Petrovitsch Email : bernd@petrovitsch.priv.at
LUGA : http://www.luga.at
^ 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